Fungibles and Non-Fungibles Create and Destroy Traits + Assets and Uniques Implementation #9844
Conversation
shawntabrizi
added
B7-runtimenoteworthy
C1-low
shawntabrizi
changed the title
shawntabrizi
added
the
D5-nicetohaveaudit ⚠️
| // NOTE: could use postinfo to reflect the actual number of | ||
| // accounts/sufficient/approvals |
There was a problem hiding this comment.
I fixed this TODO, you can see we now refund the weight using proper details.
This should be the only "new" logic
| Asset::<T, I>::try_mutate_exists(id, |maybe_details| { | ||
| let mut details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?; | ||
| if let Some(check_owner) = maybe_check_owner { |
There was a problem hiding this comment.
Just copied this logic directly into do_destroy
| ensure!(!Asset::<T, I>::contains_key(id), Error::<T, I>::InUse); | ||
| ensure!(!min_balance.is_zero(), Error::<T, I>::MinBalanceZero); | ||
|
|
||
| Asset::<T, I>::insert( |
There was a problem hiding this comment.
Just copied this logic into do_force_create
| Class::<T, I>::try_mutate_exists(class, |maybe_details| { | ||
| let class_details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?; | ||
| if let Some(check_owner) = maybe_check_owner { |
There was a problem hiding this comment.
Just moved this logic into do_destroy_class
Yes, zero logic changes other than the weight refund |
| fn destroy( | ||
| id: Self::AssetId, | ||
| witness: Self::DestroyWitness, | ||
| maybe_check_owner: Option<AccountId>, |
There was a problem hiding this comment.
suggestion: add an auto-implemented destroy function without the witness parameter (and maybe rename this to destroy_with_witness)
There was a problem hiding this comment.
No, I think that would be a dangerous API. Destroy can have varying complexity based on the number of assets it contains. The user should always investigate the witness information before executing the operation. If they truly with to be oblivious of this check, they can always provide a witness with max values, but I don't think we should encourage it.
|
LGTM. The only caveat is I have not thought too deeply on exactly how other pallets may be using this trait, but for the use cases I understand this seems clean and sensible. |
|
bot merge |
|
Trying merge. |
ghost
merged commit 
…iques Implementation (paritytech#9844) * refactor `do_destroy` * destroy trait * refactor do_force_create * impl create trait * do not bleed weight into api * Do the same for uniques * add docs
This PR introduces the traits
CreateandDestroyto the overarchingfungiblesandnonfungiblesdefinition.These traits, as they sound, allow you to create and destroy assets within some pallet which manages multiple fungible or nonfungible assets.
We implement this trait in the Assets and Uniques pallet by doing a trivial refactor of the logic in the
destroyandforce_createextrinsic, and then using that logic for the trait.This should help other developers who want to build on top of the Assets or Uniques pallet as a basis for managing tokens in their runtime.
Replaces: #9824