Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Assets] Implement pallet-assets-holder #4530

Open
wants to merge 97 commits into
base: master
Choose a base branch
from
Open

[Assets] Implement pallet-assets-holder #4530

wants to merge 97 commits into from
+1,874 −63

Conversation

pandres95
Copy link
Contributor

@pandres95 pandres95 commented May 21, 2024 •
edited
Loading

Closes #4315

@pandres95 pandres95 requested a review from a team as a code owner May 21, 2024 05:43
@pandres95 pandres95 marked this pull request as draft May 21, 2024 05:43
@pandres95 pandres95 mentioned this pull request May 21, 2024
Open
3 tasks
@pandres95 pandres95 force-pushed the pallet-assets-holder branch from c627c51 to 89ea43a Compare June 17, 2024 16:53
pandres95 added 4 commits July 10, 2024 14:34
@pandres95
change(pallet-assets): adjust reducible/can_decrease to be in line wi…
2ab3535 
…th the tokens' _balance components_ model.

On the most recent documentation about tokens (both _fungible_ and _fungibles_ (sets)), the model for calculating the different balance components is explained. The prior implementation of `pallet-assets` featured a weird definition of how to handle them that was not in line with the expected (see <https://paritytech.github.io/polkadot-sdk/master/frame_support/traits/tokens/fungible/index.html#holds-and-freezes>) definition of tokens.

This commit changes this implementation for methods `reducible_balance` and `can_decrease` to introduce the calculation of `spendable` balance, and the consequences derived of decreasing a balance that may include both `frozen` and `held` balances.
@pandres95
Merge branch 'master' into pallet-assets-holder
9d326b8
@pandres95
change(pallet-assets): introduce tests to check for holds and freezes…
c76b654 
… working together
@pandres95
Merge branch 'master' into pallet-assets-holder
c5690e1
@pandres95 pandres95 marked this pull request as ready for review July 11, 2024 09:45
@pandres95 pandres95 requested review from athei and a team as code owners July 11, 2024 09:45
@bkchr
Copy link
Member

bkchr commented Jul 24, 2024

@joepetrowski @muharem can you please take a look at this and if we need this?

muharem
muharem reviewed Jul 25, 2024
View reviewed changes
Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pandres95 I do not really understand the plan. I see only first step in the issue. also are you sure we need this new trait? can we do the same with fungibles traits? I see that we have a similar FrozenBalance trait, but it probably was introduced before the fungibles was. can we bring the hold implementation with a single PR? it would make it more clear and we get it faster. this PR wont bring anything useful, only breaking changes.

@pandres95
Copy link
Contributor Author

pandres95 commented Jul 25, 2024 •
edited
Loading

Sure! Can complete the second step in the same PR.

The reason behind this trait is that in some prior meeting Gav advised against exhaustively modifying some core pallets like pallet-assets and instead recommended extending them. That is, precisely the reasoning for having created pallet-assets-freezer.

@muharem
Copy link
Contributor

muharem commented Jul 25, 2024 •
edited
Loading

@pandres95 yes, it should be done as an extension. there is also a died hook, the new trait make sense to me now.
so I would only advise to have a single PR, since HeldBalance along does not bring anything, and only breaking changes.
if introduced and merged separately, it is possible to get the HeldBalance and the holder pallet in different release versions. Two separate audits would be required. also I think we can review it better in a single PR.

@pandres95 pandres95 changed the title [Assets] Define HeldBalance [Assets] Implement pallet-assets-holder Jul 29, 2024
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 1, 2024 02:28
@pandres95
Copy link
Contributor Author

@muharem the pallet implementation is done and ready to review

@pandres95 pandres95 requested a review from muharem August 1, 2024 02:33
@pandres95 pandres95 requested a review from ggwpez November 6, 2024 16:31
@pandres95
Copy link
Contributor Author

pandres95 commented Nov 12, 2024 •
edited
Loading

@gui1117 AFAIK, I've already addressed the changes you requested on your review. Let me know if there's something additional you need from my end. 😁

muharem
muharem reviewed Nov 19, 2024
View reviewed changes
substrate/frame/assets/src/types.rs Outdated Show resolved Hide resolved
substrate/frame/assets/src/functions.rs Outdated Show resolved Hide resolved
@pandres95 pandres95 force-pushed the pallet-assets-holder branch from 7105fc3 to 4f50052 Compare November 27, 2024 14:24
@pandres95 pandres95 requested a review from muharem November 27, 2024 14:31
@pandres95 pandres95 force-pushed the pallet-assets-holder branch from 3d79a10 to 17e4b16 Compare November 27, 2024 15:46
@pandres95 pandres95 force-pushed the pallet-assets-holder branch from 48fe5d3 to 19e59ed Compare December 9, 2024 04:06
@pandres95 pandres95 force-pushed the pallet-assets-holder branch from 19e59ed to f2d33e0 Compare December 9, 2024 04:21
gui1117
gui1117 reviewed Dec 9, 2024
View reviewed changes
umbrella/Cargo.toml Outdated Show resolved Hide resolved
substrate/frame/assets/src/functions.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muharem muharem muharem left review comments

@gui1117 gui1117 gui1117 approved these changes

@athei athei Awaiting requested review from athei

@gavofyork gavofyork Awaiting requested review from gavofyork

@joepetrowski joepetrowski Awaiting requested review from joepetrowski

@ggwpez ggwpez Awaiting requested review from ggwpez

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define and implement Hold for Assets
9 participants
@pandres95 @bkchr @muharem @Polkadot-Forum @paritytech-cicd-pr @gui1117 @gavofyork @ggwpez @joepetrowski