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.

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

Implement pallet-assets-freezer #3951

Merged
merged 35 commits into from
Jun 21, 2024

Conversation

pandres95
Copy link
Contributor

@pandres95 pandres95 commented Apr 2, 2024

Closes #3342

cc/ @liamaharon

TODO:

polkadot address: 12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR

@pandres95 pandres95 force-pushed the pandres95--pallet-assets-freezer branch 9 times, most recently from f2c9643 to 5fae57f Compare April 3, 2024 02:01
@liamaharon liamaharon added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 3, 2024
substrate/frame/assets-freezer/README.md Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/impls.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/impls.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/mock.rs Outdated Show resolved Hide resolved
@pandres95 pandres95 force-pushed the pandres95--pallet-assets-freezer branch 2 times, most recently from f59c9ea to 29c0925 Compare April 3, 2024 16:32
@pandres95 pandres95 changed the title FRAME: Pallet Assets Freezer Implemen pallet-assets-freezer Apr 3, 2024
@pandres95 pandres95 changed the title Implemen pallet-assets-freezer Implement pallet-assets-freezer Apr 3, 2024
@pandres95 pandres95 force-pushed the pandres95--pallet-assets-freezer branch 4 times, most recently from b0b6f59 to c4681b6 Compare April 4, 2024 00:49
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Pallet extension generally looking really good!

pallet-assets also needs to be modified to allow attaching it, let me know if it's clear enough from Gav's PR how to could do that or if you need some pointers.

substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/impls.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/tests.rs Show resolved Hide resolved
substrate/frame/assets-freezer/src/types.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/types.rs Outdated Show resolved Hide resolved
@pandres95 pandres95 force-pushed the pandres95--pallet-assets-freezer branch 2 times, most recently from bdff229 to 56ac28b Compare April 4, 2024 20:34
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/assets-freezer/src/impls.rs Outdated Show resolved Hide resolved
Comment on lines 75 to 80
/// The ID type for freezes.
type FreezeIdentifier: Parameter + Member + MaxEncodedLen + Copy;

/// The overarching freeze reason.
#[pallet::no_default_bounds]
type RuntimeFreezeReason: VariantCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you tell please why two separate types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This follows the logic present in pallet-balances:

type RuntimeFreezeReason: VariantCount;

This is used to constraint the maximum amount of freezes to the count of variants in RuntimeFreezeReason on the integrity test, but allowing the implementor of the pallet to use any other type as freeze identifier in case that's needed by the runtime dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

@muharem I was also curious and asked in Element but no one answered.

My best guess is it's to allow constraining the FreezeReasons allowed to be used in the pallet?

Copy link
Contributor

@muharem muharem Apr 11, 2024

Choose a reason for hiding this comment

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

Lets imagine FreezeIdentifier is some non enum type, u32 for example. The overarching freeze reason in this case same - u32. The varian count some const u32 number.
So I would imagine this as

trat Config {
    /// The overarching freeze reason.
    type RuntimeFreezeReason: Parameter + Member + MaxEncodedLen + Copy;
    /// Ax number of freezes possible for a single asset account.
    type MaxFreezes: Get<u32>;
}

where any type that implements VariantCount, implements Get<u32> by default via generic impl.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is actually an adapter VariantCountOf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! In pallet-balances, the struct VariantCountOf is used to constraint the limit of Holds, like this:

pub type Holds<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Blake2_128Concat,
T::AccountId,
BoundedVec<
IdAmount<T::RuntimeHoldReason, T::Balance>,
VariantCountOf<T::RuntimeHoldReason>,
>,
ValueQuery,
>;

This approach works in such a way there's no need to test for integrity, removes the need for a MaxHolds, and the ambiguity between RuntimeFreezeReason and FreezeIdentifier. I can apply the same strategy for RuntimeFreezeReason in pallet-assets-freezer.

Open question: considering a freeze reason can be of type RuntimeFreezeReason or any open-ended type that implements VariantCount, should it really be named Runtime in the type?

Note: I also suggest opening an issue to consider whether to standardize this approach to replace FreezeIdentifier/MaxFreezes in pallet-balances in favour of using RuntimeFreezeReason as the freezes Identifier there. Noticeably, for a potentially future implementation of fungibles::hold in pallet-assets either via a pallet extension, or via a direct implementation of those traits. would be good to apply the same approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muharem just removed FreezeIdentifier in favour of RuntimeFreezeReason as an unambiguous type for identifying freezes on the pallet. Left a comment above to explain the decision, and proposed some other questions and follow-up steps.

@pandres95 pandres95 requested a review from muharem April 10, 2024 16:30
@pandres95 pandres95 force-pushed the pandres95--pallet-assets-freezer branch from 0e5e4c6 to 0f76c41 Compare April 11, 2024 15:49
substrate/frame/assets-freezer/src/lib.rs Outdated Show resolved Hide resolved
@liamaharon
Copy link
Contributor

/tip medium

Copy link

@liamaharon A referendum for a medium (80 DOT) tip was successfully submitted for @pandres95 (12gMhxHw8QjEwLQvnqsmMVY1z5gFa54vND74aMUbhhwN6mJR on polkadot).

Referendum number: 670.
tip

Copy link

The referendum has appeared on Polkassembly.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/decoded-2024-sponsorship-for-active-community-members/7654/71

@muharem
Copy link
Contributor

muharem commented Jun 17, 2024

@pandres95 can you please resolve the conflicts and merge the master. we merge this after

@pandres95
Copy link
Contributor Author

Done @muharem

@muharem
Copy link
Contributor

muharem commented Jun 18, 2024

@pandres95 can you check failing check-umbrella CI, it's marked as required

@pandres95
Copy link
Contributor Author

@muharem fixed

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/56

@muharem muharem added this pull request to the merge queue Jun 21, 2024
Merged via the queue into paritytech:master with commit a477bd0 Jun 21, 2024
155 of 157 checks passed
@pandres95 pandres95 deleted the pandres95--pallet-assets-freezer branch June 21, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Audited
Development

Successfully merging this pull request may close these issues.

[Assets] Implement fungibles::freeze::Mutate
4 participants