[Assets] Implement `pallet-assets-holder` #4530

pandres95 · 2024-05-21T05:43:03Z

…nce based on whether there's held assets or not

…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.

… working together

bkchr · 2024-07-24T09:24:24Z

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

muharem

@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 · 2024-07-25T11:39:47Z

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 · 2024-07-25T12:56:43Z

@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.

…allet-assets`

pandres95 · 2024-08-01T02:32:10Z

@muharem the pallet implementation is done and ready to review

…ures

muharem

few minor requests

prdoc/pr_4530.prdoc

substrate/frame/assets/src/mock.rs

substrate/frame/assets-freezer/src/impls.rs

substrate/frame/assets/src/functions.rs

…ied`

…alance on hold, but frozen value is zero, as the account would need to exist

…lder`.

… to reflect the changes in the balance model, so a first reader can understand the differences between _free_ balance, _reducible_ balance and balance _on hold_.

prdoc/pr_4530.prdoc

substrate/frame/assets-holder/src/lib.rs

substrate/frame/assets/src/tests.rs

gui1117 · 2024-11-04T05:47:47Z

substrate/frame/assets/src/functions.rs

@@ -361,7 +386,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			T::Currency::unreserve(&who, deposit);
 		}

-		if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) {
+		if let Remove = Self::dead_account(id.clone(), &who, &mut details, &account.reason, false)?


This operation happens after the unreserve. So in case we return an error, the storage is tainted.

do_refund is used in trait implementation, so if a user of the trait doesn't revert when receiving an error then we have an issue.

We should do this operation in a transaction.

Been testing this.

My conclusion is the following: even though the deposit is unreserved earlier in the code, if an error is returned, the storage is not tainted.

I understand it's easy to think that by calling T::Currency::unreserved, which doesn't return a Result type would cause that change to be directly written in the storage. It's not necessarily like that.

Consider how FRAME is built. Storage is not directly touched, but handled in a transactional layered way. So, storage is written if and only if the call execution is correct.

This is also why we assert_noop instead of doing assert_err. An example of that, here:

polkadot-sdk/substrate/frame/assets/src/tests.rs

Line 1405 in 6bebe7e

assert_noop!(

I added a small test to prove the storage is not tainted (@muharem, we should probably keep this test, wdyt?) on 3e797b1.

But overall, the behaviour is the same: storage is never modified if an extrinsic fails, regardless of how it is modified.

do_refund is not strictly a call implementation. It is also used in traits in the implementation of fungibles::Refund.

The trait fungibles::Refund doesn't specifies that the storage must be rollbacked in case of error.

Somebody can write:

// Ignore because we don't need to enforce the success. let _ignore = fungibles::Refund::refund(asset_id, some_account);

And now storage is inconsistent.

I understand you're proposing these T::Currency::unreserve calls should be wrapped inside a transactional macro? Or does the entire call need to be wrapped?

I guess wrapping from the unreserve call until dead_account or the end of the block into one transactional is more efficient.
Because the early checks can return without starting the transactional.

EDIT: but both implementation would satisfy me

Done in d2e2d18

gui1117 · 2024-11-04T05:48:13Z

substrate/frame/assets/src/functions.rs

@@ -397,7 +424,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

 		T::Currency::unreserve(&depositor, deposit);

-		if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) {
+		if let Remove = Self::dead_account(id.clone(), &who, &mut details, &account.reason, false)?


same storage is tainted.

See #4530 (comment)

Done in d2e2d18

gui1117 · 2024-11-04T05:50:40Z

substrate/frame/assets/src/functions.rs

+					details,
+					&source_account.reason,
+					false,
+				)?);


same storage is tainted

See #4530 (comment)

Done in d2e2d18.

Note: This function didn't change as it didn't have a reference to T::Currency::unreserve prior to dead_account.

substrate/frame/assets/src/functions.rs

gui1117 · 2024-11-04T05:58:56Z

substrate/frame/assets/src/functions.rs

@@ -782,7 +828,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 					} else if let Some(deposit) = v.reason.take_deposit() {
 						T::Currency::unreserve(&who, deposit);
 					}
-					if let Remove = Self::dead_account(&who, &mut details, &v.reason, false) {
+					if let Remove =
+						Self::dead_account(id.clone(), &who, &mut details, &v.reason, false)?


storage is tainted.

See #4530 (comment)

Done in d2e2d18

substrate/frame/assets/src/types.rs

outdated

gui1117 · 2024-11-04T06:05:23Z

Looks good, some last changes

pandres95

Change this (thanks @ggwpez)

substrate/frame/assets-holder/src/lib.rs

…wpez

…wpez for the suggestion)

…rency::unreserve` that are followed by a potential failure of `dead_account`. This would prevent cases where `refund` is called externally by another pallet, and not as a part of a direct extrinsic call within the pallet.

…illustrate the changes.

change(pallet-assets): define HeldBalance

a0f5015

pandres95 requested a review from a team as a code owner May 21, 2024 05:43

pandres95 marked this pull request as draft May 21, 2024 05:43

pandres95 mentioned this pull request May 21, 2024

Define and implement Hold for Assets #4315

Open

3 tasks

pandres95 added 5 commits June 10, 2024 11:29

change(pallet-assets): implement Holder to calculate reducible bala…

6d2aa57

…nce based on whether there's held assets or not

change: add Holder type to missing pallet_assets Config implementations

029e887

change(pallet-assets): call died hook for Holder whenever necessary

bf376a5

Merge branch 'master' into pallet-assets-holder

fe951fa

fix(pallet-asset-conversion-ops): missing Holder in mock config

89ea43a

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

Merge branch 'master' into pallet-assets-holder

9d326b8

change(pallet-assets): introduce tests to check for holds and freezes…

c76b654

… working together

Merge branch 'master' into pallet-assets-holder

c5690e1

pandres95 marked this pull request as ready for review July 11, 2024 09:45

pandres95 requested review from athei and a team as code owners July 11, 2024 09:45

muharem reviewed Jul 25, 2024

View reviewed changes

change(pallet-assets-freezer): adjust mock and tests to changes in `p…

a4bb10c

…allet-assets`

pandres95 changed the title ~~[Assets] Define HeldBalance~~ [Assets] Implement pallet-assets-holder Jul 29, 2024

feat(pallet-assets-holder): pallet implementation

32fa5c1

paritytech-review-bot bot requested a review from a team August 1, 2024 02:28

Merge branch 'master' into pallet-assets-holder

fe0a5e5

pandres95 requested a review from muharem August 1, 2024 02:33

pandres95 added 2 commits July 31, 2024 22:44

fix: fmt / cargo fmt

b93337e

chore: umbrella

4e0ced7

pandres95 added 5 commits September 23, 2024 11:33

change(Cargo): update lockfile

c81ad85

fix(pallet-pass): missing clone for asset id

bd72de1

make ci happy

4bab6f1

Merge branch 'master' into pallet-assets-holder

f1e5b9b

change(pallet-assets): tests to assert calling on dead_account fail…

8d46fa3

…ures

pandres95 force-pushed the pallet-assets-holder branch from fd3adef to 8d46fa3 Compare October 7, 2024 07:36

Merge branch 'master' into pallet-assets-holder

8b9b6db

pandres95 mentioned this pull request Oct 7, 2024

[Assets] Check for ContainsFreezes on dead_account #5948

Open

pandres95 added 2 commits October 7, 2024 02:55

Merge branch 'master' into pallet-assets-holder

ba8c452

change(pallet-assets-holder): make CI happy

7d84bee

muharem reviewed Oct 28, 2024

View reviewed changes

pandres95 added 7 commits November 3, 2024 18:25

change(prdoc): apply suggestions and clarify changes in balance model

608b688

Merge branch 'master' into pallet-assets-holder

f12acd2

Merge branch 'master' into pallet-assets-holder

6ce2b83

change(pallet-assets-freezer): handle assert_debugs when calling `d…

189d8d4

…ied`

fix(pallet-assets): on can_decrease, return WouldDie if there's b…

6130c60

…alance on hold, but frozen value is zero, as the account would need to exist

change(pallet-assets): assert died hooks for both Freezer AND `Ho…

f7d0a41

…lder`.

change(pallet-assets): document the balance field of AssetAccount…

6bebe7e

… to reflect the changes in the balance model, so a first reader can understand the differences between _free_ balance, _reducible_ balance and balance _on hold_.

pandres95 requested a review from muharem November 4, 2024 00:35

gui1117 reviewed Nov 4, 2024

View reviewed changes

gui1117 self-requested a review November 4, 2024 06:04

pandres95 added 2 commits November 5, 2024 16:06

change(pallet-assets): assert deposit

3e797b1

fix(pallet-assets): typo (thanks to @gui1117 for the suggestion)

595b838

pandres95 commented Nov 6, 2024

View reviewed changes

substrate/frame/assets-holder/src/lib.rs Outdated Show resolved Hide resolved

pandres95 added 3 commits November 6, 2024 10:19

change(pallet-assets-holder): comment events as docstring (thanks @gg…

17ba78c

…wpez for the suggestion)

fix(prdoc): extend explanation of changes in balance model to better …

c10123a

…illustrate the changes.

pandres95 requested a review from ggwpez November 6, 2024 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Assets] Implement `pallet-assets-holder` #4530

[Assets] Implement `pallet-assets-holder` #4530

pandres95 commented May 21, 2024 •

edited

Loading

bkchr commented Jul 24, 2024

muharem left a comment

pandres95 commented Jul 25, 2024 •

edited

Loading

muharem commented Jul 25, 2024 •

edited

Loading

pandres95 commented Aug 1, 2024

muharem left a comment

gui1117 Nov 4, 2024

pandres95 Nov 5, 2024 •

edited

Loading

gui1117 Nov 6, 2024

gui1117 Nov 6, 2024

pandres95 Nov 6, 2024

gui1117 Nov 6, 2024 •

edited

Loading

pandres95 Nov 6, 2024

gui1117 Nov 4, 2024

pandres95 Nov 5, 2024

pandres95 Nov 6, 2024

gui1117 Nov 4, 2024

pandres95 Nov 5, 2024

pandres95 Nov 6, 2024 •

edited

Loading

gui1117 Nov 4, 2024

pandres95 Nov 5, 2024

pandres95 Nov 6, 2024

gui1117 commented Nov 4, 2024

pandres95 left a comment

[Assets] Implement pallet-assets-holder #4530

Are you sure you want to change the base?

[Assets] Implement pallet-assets-holder #4530

Conversation

pandres95 commented May 21, 2024 • edited Loading

bkchr commented Jul 24, 2024

muharem left a comment

Choose a reason for hiding this comment

pandres95 commented Jul 25, 2024 • edited Loading

muharem commented Jul 25, 2024 • edited Loading

pandres95 commented Aug 1, 2024

muharem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandres95 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandres95 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 commented Nov 4, 2024

pandres95 left a comment

Choose a reason for hiding this comment

[Assets] Implement `pallet-assets-holder` #4530

[Assets] Implement `pallet-assets-holder` #4530

pandres95 commented May 21, 2024 •

edited

Loading

pandres95 commented Jul 25, 2024 •

edited

Loading

muharem commented Jul 25, 2024 •

edited

Loading

pandres95 Nov 5, 2024 •

edited

Loading

gui1117 Nov 6, 2024 •

edited

Loading

pandres95 Nov 6, 2024 •

edited

Loading