refactor token state management #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
By putting the responsibility of calling |
anorth
approved these changes
Jul 24, 2022
| /// | ||
| /// If the closure returns an error, the transaction is dropped atomically and no change is | ||
| /// observed on token state. | ||
| pub fn transaction<F, Res>(&mut self, f: F) -> Result<Res> |
There was a problem hiding this comment.
This is interesting but not quite perfect.
- Ideally don't expose the token state directly to Token users
- Users can still use the API outside a transaction.
I think this method makes a pretty good internal transaction helper. I.e. call it from within Token methods so they are atomic, but don't expose it publicly. So I reckon make it non-pub for this PR, fixing (1).
I'm not sure 2 is a serious problem, but would be nice to address. For application-level transactions, we'd need the callback parameter to be Token or a new TokenTxn trait. If callers strictly used that, it would kinda give (2).
| @@ -266,76 +279,99 @@ where | |||
| ))); | |||
| } | |||
|
|
|||
| let mut state = self.load_state(); | |||
| let _old_state = self.state.clone(); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before addressing the cleanup of tests and additional test-cases from the feedback in #24, I wanted to refactor the API provided by
Tokenfor loading and saving itself to the blockstore.There was some awkwardness noted around the
Token::newpattern. This change addresses that with the following options:Token::newToken::loadPreviously, every call to a method on Token would cause state to be loaded and written to the blockstore. Now, by holding a TokenState rather than a Cid pointing to it, we can cache state changes in memory. This pushes the onus of persistance onto the library consumer, requiring a call to
Token::flushto persist state changes into the blockstore. This should be a more flexible API allowing greater control of when expensive read/writes occur and enables batching of transactions.This incidentally highlights a previously uncaught bug in
transferandburnwhich made multiple sequential calls to changeTokenState. If any step within the transaction fails, previous changes would need to be reverted. This is much easier done if changes are made in memory rather than being committed to the blockstore. To help this, aToken::transactionmethod is introduced to help us write atomic transactions.