-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic token actor example #55
Conversation
a024bb9
to
dcf0d68
Compare
1c3c4b3
to
e9a4956
Compare
423d0c5
to
e8165f5
Compare
let res = token_actor.mint(params).unwrap(); | ||
|
||
let cid = token_actor.util.flush().unwrap(); | ||
sdk::sself::set_root(&cid).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this (set_root
), other calls to the actor will continue to use the previous state. I think most of the other methods that change state need to do this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't considered this. It seems that it would be necessary to set the new root in order to handle re-entrant calls.
However, doesn't this need to happen before the receiver hook is called (i.e. within mint
) rather than after mint
since the execution flow is passed to the called actor by the VM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll need to set root before the receiver hook. Fun!
Alex, maybe we need to split up the mint/transfer behaviour into two parts, or take a callback that persists state, or something like that? The library can't do it internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mint and Transfer currently both flush state before calling the receiver hook but don't set the root at that time. Splitting mint/transfer and the receiver hook calls might be the nicest way to do things so any state persistence stuff can be handled outside of that (so token mutates state, but never stores/loads).
I've been having thoughts about making load, flush/store, and maybe a transaction thing like this into a trait for state structs, it should be generic enough that we could write the one implementation and just impl PersistentState for MyState {}
?
The transaction option would save state (and set the root) and then revert if necessary based on the result of a callback, so we'd call the receiver hook there. I guess there might be other uses for that beyond receiver hooks too?
minting would then look something like this:
let mint = state.token.mint(...)?;
state.call_or_revert(call_receiver_hook(mint));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been having thoughts about making load, flush/store, and maybe a transaction thing like this into a trait
Put that onto a list for possible developer tooling things to do later. It's beyond scope for the tokens, but I'm keen to gather ideas like this.
}, | ||
"TokensReceived" => { | ||
// TokensReceived is passed a TokenReceivedParams | ||
//let _params: TokenReceivedParams = deserialize_params(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to explode at runtime when I build the actor with it. Not sure what's wrong yet as it should be the same type we're send
ing over when mint()
calls the hook. Need to look into it further.
For now, the receiver just accepts everything.
also disables default features because we don't need GPU support (or the dependencies that brings) for running tests
* 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
* 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
66b496a
to
ec0176b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the changes on the library
I opened this PR so can't ✅ it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think there's just a couple small things to fix up
testing/fil_token_integration/actors/basic_token_actor/src/lib.rs
Outdated
Show resolved
Hide resolved
Ok(new_amount) | ||
}); | ||
if self.same_address(operator, owner) { | ||
return Err(TokenError::InvalidOperator(*operator)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird to deny this, but I kinda get it. It would be more flexible to allow it (and skip the allowance check)
No description provided.