feat(pallet): follow psp22 standard for decrease_allowance and burn
#311
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 51.47% 51.52% +0.05%
==========================================
Files 48 48
Lines 4894 4902 +8
Branches 4894 4902 +8
==========================================
+ Hits 2519 2526 +7
- Misses 2326 2327 +1
Partials 49 49
|
There was a problem hiding this comment.
Unfortunately still some things to do.
- Why are there so many changes in other files? Makes the PR very messy.
- Would be nice to cover the changes for the integration tests here as well.
- burn's pre-dispatch weight needs to be changed. Easiest thing to do is adding the benchmark weight for balance read function.
- decrease_allowance needs to be re-benchmarked.
Co-authored-by: Daan van der Plas <[email protected]>
|
@Daanvdplas Weird things with Github commit: https://github.com/orgs/community/discussions/6814. Please ignore those empty files. |
Why are the permissions changing? Can we just not revert whatever was changed so that they dont appear within the PR? edit: I assume '100755 → 100644' shown in the git diff is the permissions change, so perhaps manually restoring the permissions to the previous should resolve. In future, please review the items being added to a commit before committing. Its better to catch these things at source than have many people review. |
| @@ -272,20 +272,28 @@ fn decrease_allowance_works() { | |||
| BadOrigin.with_weight(WeightInfo::approve(0, 0)) | |||
| ); | |||
| } | |||
| assets::create_mint_and_approve(owner, token, owner, value, spender, value); | |||
There was a problem hiding this comment.
The changes to this test demonstrate why a test per behaviour might be preferred. Such an introduction of a feat should of just had a new test added for a new behaviour, which would have been much cleaner to both write and review.
| @@ -416,10 +417,17 @@ fn burn_works() { | |||
| let from = BOB; | |||
| let total_supply = value * 2; | |||
|
|
|||
| // Check error works for `Assets::burn()`. | |||
| assert_noop!(Fungibles::burn(signed(owner), token, from, value), AssetsError::Unknown); | |||
There was a problem hiding this comment.
Whilst the assets error assertions were to ensure that errors are properly handled, which is the case on line 424, this line could have been left in.
| @@ -2,7 +2,7 @@ | |||
| //! Autogenerated weights for `fungibles` | |||
| //! | |||
| //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 40.0.0 | |||
| //! DATE: 2024-09-13, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` | |||
| //! DATE: 2024-10-01, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` | |||
There was a problem hiding this comment.
Well done for re-running! It might be nice to have devnet benchmarking run automatically on PRs in the future, so that changes included in the comments above the weight function signatures in terms of storage reads/writes can be checked.
There was a problem hiding this comment.
Love your suggestion. Created an issue for this: #321
|
Created this PR #322 to substitute this PR due to the file permission changes. Did try to revert but does not work for me. |
Discussion: #300 (comment)
decrease_allowance:Fungibles::decrease_allowance()saturating_sub the value, it shouldchecked_suband throw an error instead. Hence, we don't have to handleInsufficientAllowanceon the contract side.burn:InsufficientBalancecase is not returned in theburnpallet api if thevalueexceeds the minted value. Indecrease_balancemethod ofpallet-assets, it is alsosaturating_sub.