Play Hero game in unit test #206

lxfind · 2022-01-19T03:06:36Z

Add a unit test that plays the hero game. It works!

In order to do so, this PR also fixed a few things:

In package building, made it such that the modules are ordered topologically.
A few function interfaces in Move are changed so that they can be called from outside.

oxade · 2022-01-20T07:04:51Z

Awesome! Will test. it out

lxfind · 2022-01-21T00:15:01Z

btw this PR is ready for review.

sblackshear

Now this is gameplay!! Thanks for doing this!

A thought for the future: my first reaction to this PR was "this is awesome". My second was "oh man, we shouldn't expect average devs to write tests that look like this". Ideally, you'd be able to concisely write a test scenario like this in pure Move with unit tests. Unfortunately, Transfer::transfer makes that tricky--once you pass an object into transfer, there's no way for the unit tests to get it back and continue running the scenario.

Not immediately sure what the best solution to this is. I wonder if there's a way for us to write a FastX-specific wrapper for the unit testing framework that understands transfer + enables us to write scenarios like this. I think it should probably be possible if we have a clear idea of what we want--we should think a bit and then ask Tim.

fastpay_core/src/unit_tests/authority_tests.rs

sblackshear · 2022-01-21T04:09:13Z

fastpay_core/src/unit_tests/authority_tests.rs

+        response.signed_effects.as_ref().unwrap().effects.status,
+        ExecutionStatus::Success
+    );
+    let package_object = &response


Unrelated: I think we need need some utility functions to make these sorts of lookups more ergonomic...

I think some work needs to go in to making the OrderResponse more organized. For example, it should at least separate the gas object with the rest of the objects in the response.

Agreed. Currently in client we have to retrieve the mutated objects and check to make sure the gas is indeed there.
I think it deserved it's own spot.

sblackshear · 2022-01-21T04:09:44Z

fastpay_core/src/unit_tests/authority_tests.rs

+        vec![],
+    )
+    .await;
+    assert_eq!(effects.status, ExecutionStatus::Success);


sblackshear · 2022-01-21T04:24:04Z

fastx_programmability/examples/sources/Hero.move

@@ -99,7 +101,7 @@ module Examples::Hero {

    /// Slay the `boar` with the `hero`'s sword, get experience.
    /// Aborts if the hero has 0 HP or is not strong enough to slay the boar
-    public fun slay(hero: &mut Hero, boar: Boar) {
+    public fun slay(hero: &mut Hero, boar: Boar, _ctx: &mut TxContext) {


The need for a dummy TxContext parameters to allow this as an entrypoint makes me a bit sad. We could easily relax the entrypoint signature rules to enforce that the last argument is optionally &mut TxContext, which would make things a bit more ergonomic. But it will also implicitly make a lot of functions into entrypoints, which may be confusing to the programmer and/or to someone trying to understand the public API of a module by looking at its entrypoints... And there are probably already lots of functions that happen to satisfy the entrypoint signature rules, but aren't intended to be entrypoints. Hmm.

Am curious to hear your thoughts on this. I have a vague idea for a long-term solution:repurpose the public(script) feature of Move bytecode to allow adapters to decide on the interpretation of script with their own entrypoint signature constraints instead of harcoding one interpretation in the VM. This will decouple entrypoint. But I am less sure of what to do in the short/medium term.

I think in the short/medium term we should make the same enforcement that Diem does: only scripts functions are entry functions. So most module functions will not be callable. This will also encourage script writers to bundle things together instead of calling function one-by-one.

fastpay_core/src/unit_tests/authority_tests.rs

sblackshear · 2022-01-21T04:36:36Z

fastx_programmability/framework/src/lib.rs

+            // Collect all module names from the current package to be published.
+            // For each transistive dependent module, if they are not to be published,
+            // they must have a non-zero address (meaning they are already published on-chain).
+            // TODO: Shall we also check if they are really on-chain in the future?


I think this seems useful. We will need an impl StateView from the caller to run the linker, which will check both that the deps are on-chain and that they match the imports in the package to be published.

fastx_programmability/framework/src/lib.rs

huitseeker

A few nits, and a peeve on the get_key_pair_from_bytes.

huitseeker · 2022-01-21T18:41:26Z

fastx_programmability/framework/src/lib.rs

+                        && m.self_id().address() == &AccountAddress::ZERO
+                })
+            {
+                return Err(FastPayError::ModulePublishFailure { error: format!("Denpendent modules must have already been published on-chain with non-0 addresses. Violated by module {:?}", m.self_id()) });


Suggested change

return Err(FastPayError::ModulePublishFailure { error: format!("Denpendent modules must have already been published on-chain with non-0 addresses. Violated by module {:?}", m.self_id()) });

return Err(FastPayError::ModulePublishFailure { error: format!("Dependent modules must have been published on-chain with non-0 addresses, unlike module {:?}", m.self_id()) });

huitseeker · 2022-01-21T18:46:26Z

fastx_types/src/base_types.rs

@@ -189,6 +190,11 @@ pub fn get_key_pair() -> (FastPayAddress, KeyPair) {
    (PublicKeyBytes(keypair.public.to_bytes()), KeyPair(keypair))
 }

+pub fn get_key_pair_from_bytes(bytes: &[u8]) -> (FastPayAddress, KeyPair) {


I would rather make this return a KeyPair, and equip KeyPair with a public(&self) -> PublicKeyBytes, and inline that call to let name = keypair.public()wherever you need it.

It makes more sense w.r.t what a KeyPair is, and It's more maintainable if we ever try to revisit the Address - PublicKeyBytes relationship.

fastpay_core/src/unit_tests/authority_tests.rs

lxfind changed the title ~~[Draft] Play Hero game in unit test~~ Play Hero game in unit test Jan 20, 2022

lxfind requested review from sblackshear, gdanezis, oxade, huitseeker and patrickkuo January 20, 2022 06:53

sblackshear approved these changes Jan 21, 2022

View reviewed changes

huitseeker reviewed Jan 21, 2022

View reviewed changes

Play Hero game

0a4ec31

lxfind merged commit 0523095 into MystenLabs:main Jan 21, 2022

lxfind mentioned this pull request Jan 21, 2022

Add gas object to OrderInfoResponse #244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Play Hero game in unit test #206

Play Hero game in unit test #206

lxfind commented Jan 19, 2022 •

edited

Loading

oxade commented Jan 20, 2022

lxfind commented Jan 21, 2022

sblackshear left a comment

sblackshear Jan 21, 2022

lxfind Jan 21, 2022

oxade Jan 21, 2022

sblackshear Jan 21, 2022

sblackshear Jan 21, 2022

lxfind Jan 21, 2022

sblackshear Jan 21, 2022

huitseeker left a comment

huitseeker Jan 21, 2022

huitseeker Jan 21, 2022

	return Err(FastPayError::ModulePublishFailure { error: format!("Denpendent modules must have already been published on-chain with non-0 addresses. Violated by module {:?}", m.self_id()) });
	return Err(FastPayError::ModulePublishFailure { error: format!("Dependent modules must have been published on-chain with non-0 addresses, unlike module {:?}", m.self_id()) });

Play Hero game in unit test #206

Play Hero game in unit test #206

Conversation

lxfind commented Jan 19, 2022 • edited Loading

oxade commented Jan 20, 2022

lxfind commented Jan 21, 2022

sblackshear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind commented Jan 19, 2022 •

edited

Loading