More integration tests #103

abright · 2022-09-12T09:04:46Z

More tests that build on #95

Adds a configurable test actor for testing different scenarios such as those outlined in #80

Single actor tests implemented:

Mint to A who rejects hook
Mint to token itself (rejected)
Mint to A, hook burns the tokens
A transfers to token itself (should be rejected)
A transfers to A (zero, non-zero)
A transfers to A and rejects hook

Multi-actor tests implemented:

A transfers to B (zero, non-zero, accept, reject)
Mint to A, hook transfers to B
Mint to A, hook transfers to B who rejects (but A keeps the tokens)
A transfer to B, hook transfer to C
A transfer to B, hook transfer to C, hook burns

Cases from #80 still not implemented:

A transfer to B, hook transfer to C (rejected), B hook then transfers to D
A transfer to B, hook transfer back to A (accept / reject)
A transfer to B, hook burns but then aborts (tokens don't move)
A transfer to B, hook transfer to C, but then B hook aborts (no move)

Tests are combined into two files (single- and multi-actor) as a compromise between readability and CI build/run time.

There's also a mechanism to reload the current state from a Cid which is used in Mint, Transfer and TransferFrom to update state and then the return value if the state has changed since calling the receiver hook. This covers cases such as transferring or burning from within the receiver hook. It could also be used to revert state if the hook call aborts, which may be useful to some implementations?

Doing this can unintentionally revert to the pre-hook state if the receiver hook carried out further operations on the balance received

…erywhere can only link against one actor for shared structs, because of the invoke methods using un-mangled names (causing collisions at link time if more than one is included)

original recipient just keeps the tokens in this case, instead of rejecting the transfer they originally received

…dling as Mint does

the caller can check for success or failure of the transfer and also has the TransferReturn data available for use

testing/fil_token_integration/actors/basic_token_actor/src/lib.rs

abright · 2022-09-20T06:22:13Z

testing/fil_token_integration/actors/test_actor/src/lib.rs

+                        amount: token_params.amount,
+                    };
+                    let receipt = sdk::send::send(&Address::new_id(sdk::message::caller()), method_hash!("Burn"), RawBytes::serialize(&burn_params).unwrap(), TokenAmount::zero()).unwrap();
+                    if !receipt.exit_code.is_success() {


Not sure if this should explode or just return the result upstream (so failure to burn will keep the tokens) like Transfer does above

I would say it should abort and so should transfer, but I see you're using this return value in tests, so ok to keep this behaviour if it's useful.

abright · 2022-09-20T06:22:59Z

testing/fil_token_integration/actors/test_actor/src/lib.rs

+                        amount: balance,
+                    };
+                    let receipt = sdk::send::send(&params.token_address, method_hash!("Burn"), RawBytes::serialize(&burn_params).unwrap(), TokenAmount::zero()).unwrap();
+                    if !receipt.exit_code.is_success() {


Same as the Receive hook version, explode here or fail quietly and let the caller figure it out?

I reckon fail for both

coveralls · 2022-09-20T07:35:03Z

Coverage decreased (-0.3%) to 86.152% when pulling 61355fd on token/more-integration-tests into e9371ad on main.

frc46_token/src/token/mod.rs

anorth · 2022-09-20T22:02:20Z

testing/fil_token_integration/actors/basic_token_actor/src/lib.rs

+                to_balance: self.balance_of(to)?,
+                recipient_data: ret.recipient_data,
+            }
+        };


Seeing this, I'm now wondering about yet another change to the hook return to encourage safety. A token impl should generally re-inspect state to create the return value. There's a possible optimisation where the CID hasn't changed, but the general case is to reload.

So now I think we should not return the ret from the hook, but force the caller to recreate it. But make that easy by adding token methods that create a TransferReturn, MintReturn etc from the current state. The hook return would only contain the recipient data.

So, sketching...

let hook_ret = hook.call()?; self.reload()?; // reloads state only if CID has changed return self.util.transfer_return(hook_ret);

I think things get simpler with an approach like that. Might want to rename the underlying functions from mint to mint_init or something to make clearer that it's a multi-step process? I'll leave that alone for the moment but it's a quick enough change afterwards if we want it in there.

The reload in your example probably needs to take the original CID we got when flushing the state before the hook call, but this otherwise looks like a simple enough change. Will try it out once I've addressed other review feedback as it's a more complex change than those.

Had a go at it, definitely feels nicer to use when implementing a token.

anorth · 2022-09-20T22:03:51Z

testing/fil_token_integration/actors/test_actor/src/lib.rs

+    Transfer(Address, RawBytes),
+    /// Burn incoming tokens
+    Burn,
+}


testing/fil_token_integration/actors/test_actor/src/lib.rs

anorth · 2022-09-20T22:06:51Z

testing/fil_token_integration/actors/test_actor/src/lib.rs

+                        amount: balance,
+                    };
+                    let receipt = sdk::send::send(&params.token_address, method_hash!("Burn"), RawBytes::serialize(&burn_params).unwrap(), TokenAmount::zero()).unwrap();
+                    if !receipt.exit_code.is_success() {


I reckon fail for both

testing/fil_token_integration/tests/frc46_multi_actor_tests.rs

anorth · 2022-09-20T22:13:52Z

testing/fil_token_integration/tests/frc46_multi_actor_tests.rs

+    assert!(ret_val.msg_receipt.exit_code.is_success());
+    // check the receipt we got in return data
+    let receipt = ret_val.msg_receipt.return_data.deserialize::<Receipt>().unwrap();
+    assert!(!receipt.exit_code.is_success());


Critical to assert all the balances here, and after every transfer.

Think I've covered all these now.

Also added some extra checks of TransferReturn data in a couple of the more complex cases. I want to do more of that in a follow-up along with the remaining few test cases. The multi-layered stuff gets messy real quick because each struct serializes the next layer in its return_data or recipient_data. I want to spend some time coming up with a nicer way to handle that stuff but I figure it's a low priority for the moment.

testing/fil_token_integration/tests/frc46_multi_actor_tests.rs

testing/fil_token_integration/tests/frc46_single_actor_tests.rs

…an Action

…ken balance

…educe test case size

…fterwards

…Return afterwards

…rn afterwards

anorth · 2022-09-21T23:26:09Z

frc46_token/src/token/mod.rs

+    /// Replace the current state with another
+    /// The previous state is returned and can be safely dropped
+    pub fn replace(&mut self, state: TokenState) -> TokenState {
+        std::mem::replace(self.state, state)


I don't think this is what I want. I think I was confused when I asked for it.

What I need would be be parallel with wrap, taking a mutable reference. I have a new State (embedded in another struct) that I've loaded into memory, and now want a token wrapping it. I want future mutations to mutate that reference in place (so I can persist later). I don't want to further mutate the state behind the old reference.

Perhaps I could just wrap again to get a new token in this case, but a re-wrap(&mut new_state) would still be convenient.

We should probably restore the old replace method for its use in testing.

What you're describing might be better done with std::mem::swap. Though it may also be better to just treat the Token as disposable and create a new one as needed. Which I guess is what we'd end up doing in a re-wrap function - create a new Token by taking everything but the state from the old one.

This new replace doesn't hurt the tests, it's a better version of what they were using before.

anorth · 2022-09-21T23:33:32Z

frc46_token/src/token/mod.rs

-            let supply = state.change_supply_by(amount)?;
-            Ok(MintReturn { balance, supply: supply.clone(), recipient_data: RawBytes::default() })
+            state.change_balance_by(&bs, owner_id, amount)?;
+            state.change_supply_by(amount)?;


Discussion: in most cases, when there is no recursion, the value returned here will be the right one. It would be nice to avoid traversing the balance table again to look up the value when that happens. So storing it in the intermediate for that case could help. That then raises the question of flowing information through to the token so that it knows whether it needs to look up a new balance. Maybe an internal dirty flag or mutation count?

I think this is a nuanced enough issue that you should write an issue and defer it to a new PR. Let's get it correct now, and then optimise.

Made issue #110 for it

anorth · 2022-09-21T23:36:47Z

frc46_token/src/token/types.rs

+#[derive(Debug)]
+pub struct MintIntermediate {
+    /// Recipient address to use for querying balance
+    pub recipient: Address,


Should we restrict visibility of these to the crate? I'm undecided, so ok to leave public for now.

testing/fil_token_integration/actors/test_actor/src/lib.rs

abright mentioned this pull request Sep 13, 2022

Transfer integration test #95

Merged

abright force-pushed the token/transfer-integration-test branch 2 times, most recently from 9c41e14 to 39fc914 Compare September 14, 2022 06:07

Base automatically changed from token/transfer-integration-test to main September 14, 2022 07:06

abright force-pushed the token/more-integration-tests branch from 965e280 to 99f29a7 Compare September 14, 2022 07:56

abright mentioned this pull request Sep 16, 2022

Benchmark revert strategies #61

Closed

abright force-pushed the token/more-integration-tests branch 2 times, most recently from d0edd2f to 6ec08a3 Compare September 20, 2022 05:50

abright added 18 commits September 20, 2022 15:52

Add configurable test actor

aa4eaf5

WIP configurable actor tests

23293ac

Add operator_data to MintParams in the basic token actor

0424763

Don't blindly flush state after minting

4c550fb

Doing this can unintentionally revert to the pre-hook state if the receiver hook carried out further operations on the balance received

test: receiver hook burns incoming tokens after mint

13bd598

MintParams formatting

e3f5639

wip make minting aware of changed state after receiver hook returns

a5a4988

wip better state reloading mechanism

3071ff1

more common stuff

e2557f6

single-actor tests

0813771

keep TestAction and ActionParams as part of test_actor

a8cec0b

duplicate MintParams in common to avoid linking in the token actor ev…

fc5a45a

…erywhere can only link against one actor for shared structs, because of the invoke methods using un-mangled names (causing collisions at link time if more than one is included)

wip multi-actor tests

9f530df

don't explode the test actor if transfer within receiver hook fails

21cacac

original recipient just keeps the tokens in this case, instead of rejecting the transfer they originally received

add test to mint tokens and then transfer inside the receiver hook

7c48437

update Transfer and TransferFrom to have the same post-hook state han…

30c0dfb

…dling as Mint does

return Transfer call result in test actor

e81c3ad

the caller can check for success or failure of the transfer and also has the TransferReturn data available for use

add transfer->hook transfer->accept/burn tests

c494ff9

abright force-pushed the token/more-integration-tests branch from 6ec08a3 to c494ff9 Compare September 20, 2022 06:02

abright commented Sep 20, 2022

View reviewed changes

update transfer_tokens test to use the common helpers

ea6793c

abright changed the title ~~WIP more integration tests~~ More integration tests Sep 20, 2022

abright marked this pull request as ready for review September 20, 2022 07:29

abright requested review from alexytsu and anorth September 20, 2022 07:44

anorth reviewed Sep 20, 2022

View reviewed changes

abright added 4 commits September 21, 2022 12:25

improve Token::replace and make public

3fbc19b

abort with an error instead of ignoring calls to Accept or Reject in …

cc49735

…an Action

rename get_balance to token_balance to avoid confusion with native to…

37df1eb

…ken balance

separate individual frc46 tests into blocks

2acef81

abright force-pushed the token/more-integration-tests branch from cc59756 to 2acef81 Compare September 21, 2022 04:46

abright added 7 commits September 21, 2022 15:12

add call_method_ok and mint_tokens_ok helpers to assert success and r…

517abcf

…educe test case size

add token balance assertion helpers to further compress test case code

d503d76

add more balance asserts and check the more complex transfer results

f9286e5

remove unnecessary comment

00e6c98

return intermediate data through hook call and construct MintReturn a…

ad9ac07

…fterwards

return intermediate data through hook call and construct TransferFrom…

1755a96

…Return afterwards

return intermediate data through hook call and construct TransferRetu…

15afad7

…rn afterwards

abright requested a review from anorth September 21, 2022 23:00

anorth approved these changes Sep 21, 2022

View reviewed changes

abright added 2 commits September 23, 2022 02:07

move transfer and burn actions in the test actor into separate functions

6d6fdb9

bump frc46_token version to 0.2.0

61355fd

abright merged commit a58f063 into main Sep 23, 2022

abright deleted the token/more-integration-tests branch September 23, 2022 00:13

abright mentioned this pull request Sep 23, 2022

Mint/Transfer return optimisation #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More integration tests #103

More integration tests #103

abright commented Sep 12, 2022 •

edited

Loading

abright Sep 20, 2022

anorth Sep 20, 2022

abright Sep 20, 2022

anorth Sep 20, 2022

coveralls commented Sep 20, 2022 •

edited

Loading

anorth Sep 20, 2022

abright Sep 21, 2022

abright Sep 21, 2022

anorth Sep 20, 2022

anorth Sep 20, 2022

anorth Sep 20, 2022

abright Sep 21, 2022

anorth Sep 21, 2022

abright Sep 22, 2022

anorth Sep 21, 2022

abright Sep 23, 2022

anorth Sep 21, 2022

More integration tests #103

More integration tests #103

Conversation

abright commented Sep 12, 2022 • edited Loading

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

coveralls commented Sep 20, 2022 • edited Loading

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

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

abright commented Sep 12, 2022 •

edited

Loading

coveralls commented Sep 20, 2022 •

edited

Loading