Call receiver hooks #28
Conversation
| @@ -1,12 +1,13 @@ | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
remove this use of anyhow in a followup
| @@ -0,0 +1 @@ | |||
| pub mod types; | |||
There was a problem hiding this comment.
TODO: library level code for the receiving actor (don't think there'll be much boilerplate tho)
maybe receiver module should be it's own package?
| if value.is_negative() { | ||
| return Err(TokenError::InvalidNegative(format!( | ||
| "mint amount {} cannot be negative", | ||
| value | ||
| ))); | ||
| } | ||
|
|
||
| let old_state = self.state.clone(); |
There was a problem hiding this comment.
Note: It's slightly sad that this clone followed by the transaction results in a redundant clone, and that we do an actual write to rollback when the old state is often already persisted. The state is small so it doesn't matter too much.
An alternative pattern would involve keeping track of the CID at load, and reverting by re-loading it. It this presents a snapshot (could avoid all the clones this way, but I'm not sure which is cheaper). It doesn't immediately apply here tho since we don't know whether the prior state has been modified since load.
There was a problem hiding this comment.
A quick test using the fvm_integration_tests package seems to show that the clone and revert strategy is cheaper than reloading the old state from the blockstore
Since the root level of state is just a BigInt and 2 CIDs, the only real difference is whether we revert those three values from an in-memory copy (should be cheap) or a copy in the blockstore. In fact, the testing I did seems to suggest that there's no gas difference in making that clone (but not sure if there's something wrong with the testing methodology here).
EDIT: There's some issues with the implementations, I think the "reload from CID" can be made cheaper by omitting a flush in the rollback case. Will investigate further
There was a problem hiding this comment.
Thanks, I'm happy for us to roll with clones then.
|
|
||
| // TODO: invoke the receiver hook on the initial_holder | ||
| // Update state so re-entrant calls see the changes | ||
| self.flush()?; |
There was a problem hiding this comment.
This might flush previously un-flushed state changes. We do need to flush here, but this could surprise someone so it's pointing to something just slightly off in APIs somewhere.
Basically there's a conflict between the need to flush here, and objectives about holding up state changes for batched flush. Maybe something like this is the best compromise available. I can't see how to sensibly avoid a flush between every transfer or mint, even if doing a large batch of them. But then maybe for consistency we give up on the idea of non-persisted changes at the level of Token API methods.
| self.state = old_state; | ||
| self.flush()?; | ||
| Err(e.into()) | ||
| } |
There was a problem hiding this comment.
Is it possible to factor out this repeated code?
There was a problem hiding this comment.
I'll defer this refactor after some more benchmarking to settle on the preferred rollback strategy
alexytsu
force-pushed
the
token/call-hook
branch
from
b00960d
to
19c3cd8
Compare
| if value.is_negative() { | ||
| return Err(TokenError::InvalidNegative(format!( | ||
| "mint amount {} cannot be negative", | ||
| value | ||
| ))); | ||
| } | ||
|
|
||
| let old_state = self.state.clone(); |
There was a problem hiding this comment.
Thanks, I'm happy for us to roll with clones then.
Final bit of functionality before returning to tidy up the unit tests
Calls receiver hooks during minting and transfer, reverting the operation if the hook aborts