-
Notifications
You must be signed in to change notification settings - Fork 648
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
Enable Merklized Distributions in Assets Pallet #5400
base: master
Are you sure you want to change the base?
Enable Merklized Distributions in Assets Pallet #5400
Conversation
This reverts commit 1cfb29f.
Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
…abrizi/polkadot-sdk into shawntabrizi-proving-trie
This PR is blocked because the data structures in |
@@ -538,4 +538,20 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> { | |||
.saturating_add(T::DbWeight::get().reads(1_u64)) | |||
.saturating_add(T::DbWeight::get().writes(1_u64)) | |||
} | |||
|
|||
fn mint_distribution() -> Weight { | |||
Weight::default() |
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.
TODO prior to merge
@@ -139,6 +139,29 @@ fn assert_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::Runti | |||
frame_system::Pallet::<T>::assert_has_event(generic_event.into()); | |||
} | |||
|
|||
// fn generate_merkle_trie<T: Config<I>, I: 'static>(items: u32) -> (DistributionHashOf<T, I>, |
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.
not needed?
@@ -404,6 +407,9 @@ pub mod pallet { | |||
/// used to set up auto-incrementing asset IDs for this collection. | |||
type CallbackHandle: AssetsCallback<Self::AssetId, Self::AccountId>; | |||
|
|||
/// A type used to verify merkle proofs used for distributions. | |||
type VerifyExistenceProof: VerifyExistenceProof<Hash: Parameter + MaxEncodedLen + Default>; |
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.
type VerifyExistenceProof: VerifyExistenceProof<Hash: Parameter + MaxEncodedLen + Default>; | |
type VerifyExistenceProof: VerifyExistenceProof<Hash = <Self as frame_system>::Hash>; |
or sth along these lines?
|
||
#[derive(Eq, PartialEq, Copy, Clone, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen)] | ||
pub struct DistributionInfo<AssetId, Hash> { | ||
// The asset id we are distributing. | ||
pub asset_id: AssetId, | ||
// The merkle root which represents all the balances to distribute. | ||
pub merkle_root: Hash, | ||
// Whether the distribution is still active. | ||
pub active: bool, | ||
} |
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.
#[derive(Eq, PartialEq, Copy, Clone, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen)] | |
pub struct DistributionInfo<AssetId, Hash> { | |
// The asset id we are distributing. | |
pub asset_id: AssetId, | |
// The merkle root which represents all the balances to distribute. | |
pub merkle_root: Hash, | |
// Whether the distribution is still active. | |
pub active: bool, | |
} | |
/// Foo bar baz | |
#[derive(Eq, PartialEq, Copy, Clone, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen)] | |
pub struct DistributionInfo<AssetId, Hash> { | |
/// The asset id we are distributing. | |
pub asset_id: AssetId, | |
/// The merkle root which represents all the balances to distribute. | |
pub merkle_root: Hash, | |
/// Whether the distribution is still active. | |
pub active: bool, | |
} |
@@ -1798,6 +1848,92 @@ pub mod pallet { | |||
)?; | |||
Ok(()) | |||
} | |||
|
|||
/// Mint a distribution of assets of a particular class. |
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.
Something about this can go into the main top docs and then in https://docs.rs/pallet-assets/latest/pallet_assets/?
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.
Some small nits remaining, but the overall logic is good
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.
First pass. Haven't looked yet at the proving trie code.
DistributionActive, | ||
/// The proof provided could not be verified. | ||
BadProof, | ||
/// The a leaf node was extracted from the proof, but it did not match the expected format. |
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 a leaf node was extracted from the proof, but it did not match the expected format. | |
/// The leaf node was extracted from the proof, but it did not match the expected format. |
pub(super) fn do_mint_distribution( | ||
id: T::AssetId, | ||
merkle_root: DistributionHashOf<T, I>, | ||
maybe_check_issuer: Option<T::AccountId>, |
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.
why Option
? Is always called with Some()
?
if all_refunded { | ||
Self::deposit_event(Event::<T, I>::DistributionCleaned { distribution_id }); | ||
// Refund weight only the amount we actually used. | ||
Ok(Some(T::WeightInfo::destroy_distribution(refund_count)).into()) |
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.
What is the incentive to clean up state? Shouldn't we refund all fees?
|
||
info.active = false; | ||
|
||
MerklizedDistribution::<T, I>::insert(&distribution_id, info); |
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.
Is this never cleaned? Would we not just keep bloating state then?
) -> DispatchResult { | ||
let origin = ensure_signed(origin)?; | ||
let id: T::AssetId = id.into(); | ||
Self::do_mint_distribution(id, merkle_root, Some(origin))?; |
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.
Thinking out loud: Should we not take some kind of deposit here (charging for the storage), that is only refunded when state (MerklizedDistributionTracker
) is cleaned up? What stops some one to create large number of distributions?
/// | ||
/// Weight: `O(N)` where `N` is the maximum number of elements that can be removed at once. | ||
#[pallet::call_index(36)] | ||
#[pallet::weight(T::WeightInfo::destroy_distribution(100u32))] // TODO |
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.
right?
#[pallet::weight(T::WeightInfo::destroy_distribution(100u32))] // TODO | |
#[pallet::weight(T::WeightInfo::destroy_distribution(T::RemoveItemsLimit::get()))] // TODO |
Depends on: #3881
This PR introduces a way for an asset issuer to distribute their token to many users using a single merkle root.
Users then present a merkle proof which is verified, and triggers the
do_mint
function.A tracking storage ensures that each user only claims their distribution a single time.
The design allows for multiple distributions per asset id.