Fix approval assets bug plus ts tests #963
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
girazoki
added
B7-runtimenoteworthy
librelois
approved these changes
Nov 5, 2021
| isFrozen: false, | ||
| }; | ||
|
|
||
| async function mockAssetBalance(context, assetBalance, assetDetails, sudoAccount, assetId) { |
There was a problem hiding this comment.
It would be great to move this in util for future tests
There was a problem hiding this comment.
Yes, I want to talk to @joelamouche for that
There was a problem hiding this comment.
@girazoki pls create ticket
There was a problem hiding this comment.
girazoki
added a commit
that referenced
this pull request
Nov 9, 2021
* pallet-assets fixing error matching * Add erc20 instance contract * TS tests with flag to true * More typescript tests * prettier * EditorConfig * Modify integration tests
crystalin
pushed a commit
that referenced
this pull request
Nov 10, 2021
* Fix approval assets bug plus ts tests (#963) * pallet-assets fixing error matching * Add erc20 instance contract * TS tests with flag to true * More typescript tests * prettier * EditorConfig * Modify integration tests * bump spec version * Remove blake2 contract
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.
What does it do?
Context: in pallet-assets, approvals (when you approve someone else to spend some amount from your balance) are incremental, while in the reference ERC20 interface they are not. That is why, to match the ERC20, we need to first cancel the previous approval in pallet-assets and make a new one substituting the previous one. Now, pallet-assets does not have a public getter for approvals yet (this will come in 0.9.12 after our PR was merged) and that means we dont have an easy way of knowing whether there exists a previous approval before canceling it.
The bug came because I decided to always call cancel_approval, and match the error against the case where the approval was not present. This works well with native execution, but wasm does not allow us to format the error from the dispatchable and therefore we cannot do this error matching.
This PR fixes this by:
Calling cancel_approval instead of force_cancel_approval. This way we make sure that the preliminary checks that we pass in cancel_approval are the same as those in approve_transfer, except for the "does an approval exist already" check. See https://github.com/paritytech/substrate/blob/bf43f7420a8548f10a010e61ffab52dabadb4272/frame/assets/src/lib.rs#L1139 and https://github.com/paritytech/substrate/blob/bf43f7420a8548f10a010e61ffab52dabadb4272/frame/assets/src/lib.rs#L1189.
Continue the execution of the precompile with approve_transfer if the error is of the type OtherError after calling cancel_approval. We know this error only returns if the dispatchable failed. If the dispatchable failed because of any of the reasons that are not "does an approval exist already", we know approve_transfer will fail anyway. If it failed because of the check "does an approval exist already", we know we should continue with approve_transfer execution.
This PR also adds the missing typescript tests, by mocking the balance of the asset with sudo. Further, I cleaned the non-matching db write cost
What important points reviewers should know?
This is temporary until we can retrieve the allowance in 0.9.12
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?