Split receiver hook calls from minting and transfer - part 1 #76
Conversation
fil_fungible_token/src/token/mod.rs
Outdated
|
|
||
| // Increase the balance of the actor and increase total supply | ||
| // todo: probably don't need transaction thing here, the entire minting+receiver hook operation belongs in one |
There was a problem hiding this comment.
I think it is still a useful property that this token object maintains state that makes sense, even if it returns an error. e.g. caller code could try one thing, then if it fails try another (assuming that no state changes were made, even in memory, by the first op)
There was a problem hiding this comment.
It didn't seem so useful to me because my original design was going to move the entire mint call into a similar thing. Also wanted to have a "save but revert on failure" version for the receiver hook call, but I think that stuff needs to belong to the state as it's not limited to Token API use (it'd build on my idea from #55 (comment) ).
I don't think any of this is needed in the abort-on-failure design so I'll remove the comment here and leave the transaction alone.
There was a problem hiding this comment.
It's a good point for discussion whether we need this transactional behaviour in the state object, or only in the token object. I could easily be convinced of the latter, but let's defer for another PR.
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
this means we'll get back meaningful errors if an unwrap() explodes
abright
force-pushed
the
token/split-mint-transfer
branch
from
401c961
to
634c8e8
Compare
|
This updated version should be a bit cleaner and doesn't bother trying to restore state if minting or the receiver hook call fails - we'll just abort. |
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
abright
changed the title
They're expecting the old behaviour of reverting state (instead of aborting) when a mint or transfer (or receiver hook) fails. Once there's a new method of calling the receiver hook (through a type returned by mint or transfer calls) they can either be adapted to that or rewritten as integration tests
abright
force-pushed
the
token/split-mint-transfer
branch
from
e845f57
to
468bd9b
Compare
Discussed this with @abright and we thought it would make sense for the new behaviour to be tested at the integration level as it would be awkward to have unit tests assert behaviour which happens across multiple library methods. Andrew pointed out also that we might get better testing here if we eventually had the |
Sure, only test what we need to, but let's not have ignored tests hanging around. |
* wip: split minting and receiver hook calling, save state at actor level * simplify call_receiver_hook * don't try to reset state on minting failure, let invoke deal with it * set a panic hook to use the vm::abort like built-in actors do this means we'll get back meaningful errors if an unwrap() explodes * token code cleanup * gracefully handle empty return data on minting failure in mint_tokens test * further cleanup * use a more appropriate error code for "unknown method" abort * split receiver hook call out of transfer and transfer_from * remove call_receiver_hook_or_revert * ignore failing mint/transfer tests They're expecting the old behaviour of reverting state (instead of aborting) when a mint or transfer (or receiver hook) fails. Once there's a new method of calling the receiver hook (through a type returned by mint or transfer calls) they can either be adapted to that or rewritten as integration tests
* add integration test contract * write example basic token actor * wip * add build script for example actor * update example actor to latest token api * lock fvm version for example actor test also disables default features because we don't need GPU support (or the dependencies that brings) for running tests * add fil_integration_tests to Makefile * pretty-print the message results in token actor test * use a helper function to simplify messaging in tests * add a barebones actor to implement a receiver hook * extend the mint implementation and mint tokens to the receiver actor * Update the token-actor interface (#75) * update the public token-actor interface * update example actor to match new interface * check receiver hooks * cleanup naming * align with FRC-0046 * fix CI build * exclude wasm actors from code coverage * rearrange params * use RawBytes rather than Default * split burn/burn_from transfer/transfer_from * Split receiver hook calls from minting and transfer - part 1 (#76) * wip: split minting and receiver hook calling, save state at actor level * simplify call_receiver_hook * don't try to reset state on minting failure, let invoke deal with it * set a panic hook to use the vm::abort like built-in actors do this means we'll get back meaningful errors if an unwrap() explodes * token code cleanup * gracefully handle empty return data on minting failure in mint_tokens test * further cleanup * use a more appropriate error code for "unknown method" abort * split receiver hook call out of transfer and transfer_from * remove call_receiver_hook_or_revert * ignore failing mint/transfer tests They're expecting the old behaviour of reverting state (instead of aborting) when a mint or transfer (or receiver hook) fails. Once there's a new method of calling the receiver hook (through a type returned by mint or transfer calls) they can either be adapted to that or rewritten as integration tests * update dispatch crate name * code cleanup * set_root after flushing state in method calls, so the state tree is kept up to date Co-authored-by: Andrew Bright <abright@users.noreply.github.com>
Splits the mint and transfer operations and receiver hook calls, so we can handle saving state and making the receiver hook call up at the actor level. This part 1 PR does the split but could use some API improvements like a return type to wrap up the receiver hook stuff as suggested in #76 (review) below.
There's also a behaviour change here: we'll ultimately abort on failure results and won't revert state ourselves
NOTE: it breaks a few of the existing unit tests that expect the state to revert on failure/rejection, they're ignored for now and can be addressed in part 2.