-
Notifications
You must be signed in to change notification settings - Fork 6
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Looks good, but I think the permissions should be reverted to clean up the scope of this PR. Good to merge otherwise.
Comments are only comments.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_sub
and throw an error instead. Hence, we don't have to handleInsufficientAllowance
on the contract side.burn
:InsufficientBalance
case is not returned in theburn
pallet api if thevalue
exceeds the minted value. Indecrease_balance
method ofpallet-assets
, it is alsosaturating_sub
.