Enable Merklized Distributions in Assets Pallet#5400
Enable Merklized Distributions in Assets Pallet#5400shawntabrizi wants to merge 83 commits intoparitytech:masterfrom
Conversation
This reverts commit 1cfb29f.
Co-authored-by: Ankan <[email protected]>
…abrizi/polkadot-sdk into shawntabrizi-proving-trie
|
This PR is blocked because the data structures in |
| Ok(()) | ||
| } | ||
|
|
||
| /// Mint a distribution of assets of a particular class. |
There was a problem hiding this comment.
Something about this can go into the main top docs and then in https://docs.rs/pallet-assets/latest/pallet_assets/?
kianenigma
left a comment
kianenigma
left a comment
There was a problem hiding this comment.
Some small nits remaining, but the overall logic is good
Ank4n
left a comment
Ank4n
left a comment
There was a problem hiding this comment.
First pass. Haven't looked yet at the proving trie code.
| 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.
What is the incentive to clean up state? Shouldn't we refund all fees?
There was a problem hiding this comment.
anyone can clean up state... but yeah we could add a deposit here for doing a distribution
| ) -> 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.
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?
Co-authored-by: Ankan <[email protected]>
| ensure!(check_issuer == details.issuer, Error::<T, I>::NoPermission); | ||
| } | ||
|
|
||
| let info = DistributionInfo { |
There was a problem hiding this comment.
Perhaps you have already considered this, but I'm still a little concerned that an account can create a bunch of distributions without paying any storage cost.
We could limit this by either
- Requiring a deposit when creating a distribution.
- Or, limit it to one active distribution per asset ID.
wdyt?
There was a problem hiding this comment.
If we don't expect multiple active distribution for an asset, probably that is the most straightforward. We can use AssetId as the key for MerklizedDistribution.
There was a problem hiding this comment.
i am okay with you adding storage deposit to this PR. i would like to hand it off to someone else to own and take to finish line
|
|
||
| #[pallet::storage] | ||
| /// Merklized distribution of an asset. | ||
| pub(super) type MerklizedDistribution<T: Config<I>, I: 'static = ()> = StorageMap< |
There was a problem hiding this comment.
nit: could be a CountedStorageMap to count active distributions.
There was a problem hiding this comment.
no, because the count would go down when we clean up a distribution. and we want the count to remain unique
| /// | ||
| /// - `distribution_id`: The identifier of the distribution. | ||
| /// - `merkle_proof`: The merkle proof of the account and balance in a compact base-16 | ||
| /// merkle trie used to authorize minting. |
Ank4n
left a comment
Ank4n
left a comment
There was a problem hiding this comment.
Just one concern about preventing distribution spam, otherwise happy to approve.
|
Why did this PR never get merged? Was it audited? |
Are you not the owner here? You should drive it to completion (audited, merged, released, integrated to fellowship runtimes), or find another owner to take responsibility for doing it. |
6 participants
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_mintfunction.A tracking storage ensures that each user only claims their distribution a single time.
The design allows for multiple distributions per asset id.