Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

[NFTs] Add the new owner param to mint() method #12997

Merged
paritytech-processbot merged 3 commits into from
Dec 22, 2022
Merged

[NFTs] Add the new owner param to mint() method #12997

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
534304e
Add the new `owner` param to mint() method
Dec 21, 2022
76dfa14
Fmt
Dec 21, 2022
f24b765
Address comments
Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions frame/nfts/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ fn mint_item<T: Config<I>, I: 'static>(
SystemOrigin::Signed(caller.clone()).into(),
T::Helper::collection(0),
item,
caller_lookup.clone(),
None,
));
(item, caller, caller_lookup)
Expand Down Expand Up @@ -174,7 +175,7 @@ benchmarks_instance_pallet! {
let m in 0 .. 1_000;
let a in 0 .. 1_000;

let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
add_collection_metadata::<T, I>();
for i in 0..n {
mint_item::<T, I>(i as u16);
Expand All @@ -200,7 +201,7 @@ benchmarks_instance_pallet! {
mint {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let item = T::Helper::item(0);
}: _(SystemOrigin::Signed(caller.clone()), collection, item, None)
}: _(SystemOrigin::Signed(caller.clone()), collection, item, caller_lookup, None)
verify {
assert_last_event::<T, I>(Event::Issued { collection, item, owner: caller }.into());
}
Expand All @@ -222,7 +223,7 @@ benchmarks_instance_pallet! {
}

transfer {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);

let target: T::AccountId = account("target", 0, SEED);
Expand All @@ -235,7 +236,7 @@ benchmarks_instance_pallet! {

redeposit {
let i in 0 .. 5_000;
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let items = (0..i).map(|x| mint_item::<T, I>(x as u16).0).collect::<Vec<_>>();
Nfts::<T, I>::force_collection_config(
SystemOrigin::Root.into(),
Expand All @@ -248,15 +249,15 @@ benchmarks_instance_pallet! {
}

lock_item_transfer {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
}: _(SystemOrigin::Signed(caller.clone()), T::Helper::collection(0), T::Helper::item(0))
verify {
assert_last_event::<T, I>(Event::ItemTransferLocked { collection: T::Helper::collection(0), item: T::Helper::item(0) }.into());
}

unlock_item_transfer {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
Nfts::<T, I>::lock_item_transfer(
SystemOrigin::Signed(caller.clone()).into(),
Expand All @@ -269,7 +270,7 @@ benchmarks_instance_pallet! {
}

lock_collection {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let lock_settings = CollectionSettings::from_disabled(
CollectionSetting::TransferableItems |
CollectionSetting::UnlockedMetadata |
Expand Down Expand Up @@ -324,7 +325,7 @@ benchmarks_instance_pallet! {
}

force_collection_config {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let origin = T::ForceOrigin::successful_origin();
let call = Call::<T, I>::force_collection_config {
collection,
Expand All @@ -336,7 +337,7 @@ benchmarks_instance_pallet! {
}

lock_item_properties {
let (collection, caller, caller_lookup) = create_collection::<T, I>();
let (collection, caller, _) = create_collection::<T, I>();
let (item, ..) = mint_item::<T, I>(0);
let lock_metadata = true;
let lock_attributes = true;
Expand Down
3 changes: 2 additions & 1 deletion frame/nfts/src/features/create_delete_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
pub fn do_mint(
collection: T::CollectionId,
item: T::ItemId,
caller: T::AccountId,
owner: T::AccountId,
item_config: ItemConfig,
deposit_collection_owner: bool,
Expand Down Expand Up @@ -58,7 +59,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
};
let deposit_account = match deposit_collection_owner {
true => collection_details.owner.clone(),
false => owner.clone(),
false => caller,
};

let owner = owner.clone();
Expand Down
1 change: 1 addition & 0 deletions frame/nfts/src/impl_nonfungibles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ impl<T: Config<I>, I: 'static> Mutate<<T as SystemConfig>::AccountId, ItemConfig
*collection,
*item,
who.clone(),
who.clone(),
*item_config,
deposit_collection_owner,
|_, _| Ok(()),
Expand Down
17 changes: 10 additions & 7 deletions frame/nfts/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,9 +726,11 @@ pub mod pallet {
origin: OriginFor<T>,
collection: T::CollectionId,
item: T::ItemId,
owner: AccountIdLookupOf<T>,
witness_data: Option<MintWitness<T::ItemId>>,
) -> DispatchResult {
let caller = ensure_signed(origin)?;
let owner = T::Lookup::lookup(owner)?;

let collection_config = Self::get_collection_config(&collection)?;
let item_settings = collection_config.mint_settings.default_item_settings;
Expand All @@ -738,9 +740,15 @@ pub mod pallet {
collection,
item,
caller.clone(),
owner.clone(),
item_config,
false,
|collection_details, collection_config| {
// Issuer can mint regardless of mint settings
if Self::has_role(&collection, &caller, CollectionRole::Issuer) {
return Ok(())
}

let mint_settings = collection_config.mint_settings;
let now = frame_system::Pallet::<T>::block_number();

Expand All @@ -752,12 +760,7 @@ pub mod pallet {
}

match mint_settings.mint_type {
MintType::Issuer => {
ensure!(
Self::has_role(&collection, &caller, CollectionRole::Issuer),
Error::<T, I>::NoPermission
)
},
MintType::Issuer => return Err(Error::<T, I>::NoPermission.into()),
MintType::HolderOf(collection_id) => {
let MintWitness { owner_of_item } =
witness_data.ok_or(Error::<T, I>::BadWitness)?;
Expand Down Expand Up @@ -838,7 +841,7 @@ pub mod pallet {
Error::<T, I>::NoPermission
);
}
Self::do_mint(collection, item, owner, item_config, true, |_, _| Ok(()))
Self::do_mint(collection, item, owner.clone(), owner, item_config, true, |_, _| Ok(()))

@melekes melekes Dec 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

q: why are we passing owner.clone() as a caller and not using let caller = ensure_signed(origin)?; like we do in mint?

@jsidorenko jsidorenko Dec 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The force_mint() method supports root calls, that's why we can't simply put ensure_signed(origin) here. On the other hand, Self::do_mint needs to know a caller to take the deposit.
Idk, maybe we should rename caller into depositor in the do_mint method, wdyt?

@melekes melekes Dec 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Idk, maybe we should rename caller into depositor in the do_mint method, wdyt?

I'd be in favor

@joepetrowski joepetrowski Dec 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think owner is confusing since CollectionRole has an Owner variant. What about in do_mint(), force_mint(), and mint() we use:

  • beneficiary: Account into which the item will be minted. (Consistent with assets)
  • depositor: Account placing the deposit.

Then the owner term is specific to the collection role.

@jsidorenko jsidorenko Dec 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}

/// Destroy a single item.
Expand Down
Loading