diff --git a/pallets/nfts/README.md b/pallets/nfts/README.md index 1056388a..a1825221 100644 --- a/pallets/nfts/README.md +++ b/pallets/nfts/README.md @@ -72,7 +72,7 @@ The NFTs pallet in Substrate is designed to make the following possible: * `lock_item_transfer`: Prevent an individual item from being transferred. * `unlock_item_transfer`: Revert the effects of a previous `lock_item_transfer`. * `clear_all_transfer_approvals`: Clears all transfer approvals set by calling the `approve_transfer`. -* `clear_all_collection_approvals`: Clears all collection approvals set by calling the `approve_collection_transfer`. +* `clear_collection_approvals`: Clears collection approvals set by calling the `approve_collection_transfer`. * `lock_collection`: Prevent all items within a collection from being transferred (making them all `soul bound`). * `lock_item_properties`: Lock item's metadata or attributes. * `transfer_ownership`: Alter the owner of a collection, moving all associated deposits. (Ownership of individual items diff --git a/pallets/nfts/src/features/approvals.rs b/pallets/nfts/src/features/approvals.rs index 26cd89d5..4c580421 100644 --- a/pallets/nfts/src/features/approvals.rs +++ b/pallets/nfts/src/features/approvals.rs @@ -289,9 +289,9 @@ impl, I: 'static> Pallet { owner: T::AccountId, collection: T::CollectionId, limit: u32, - ) -> DispatchResult { + ) -> Result { if limit == 0 { - return Ok(()); + return Ok(0); } let mut removed_approvals: u32 = 0; let mut deposits: BalanceOf = Zero::zero(); @@ -306,7 +306,7 @@ impl, I: 'static> Pallet { T::Currency::unreserve(&owner, deposits); Self::deposit_event(Event::ApprovalsCancelled { collection, item: None, owner }); - Ok(()) + Ok(removed_approvals) } /// Checks whether the `delegate` is approved to transfer collection items of `owner`. diff --git a/pallets/nfts/src/lib.rs b/pallets/nfts/src/lib.rs index 9f7254ed..daa04338 100644 --- a/pallets/nfts/src/lib.rs +++ b/pallets/nfts/src/lib.rs @@ -56,9 +56,12 @@ use alloc::{boxed::Box, vec, vec::Vec}; use core::cmp::Ordering; use codec::{Decode, Encode}; -use frame_support::traits::{ - tokens::Locker, BalanceStatus::Reserved, Currency, EnsureOriginWithArg, Incrementable, - ReservableCurrency, +use frame_support::{ + dispatch::WithPostDispatchInfo, + traits::{ + tokens::Locker, BalanceStatus::Reserved, Currency, EnsureOriginWithArg, Incrementable, + ReservableCurrency, + }, }; use frame_system::Config as SystemConfig; pub use pallet::*; @@ -1403,7 +1406,7 @@ pub mod pallet { /// /// Origin must be the `ForceOrigin`. /// - /// - `owner`: The account granting approval for delegated transfer. + /// - `owner`: The owner of the approvals granted by the `origin`. /// - `collection`: The collection of the item to be approved for delegated transfer. /// - `delegate`: The account to delegate permission to transfer collection items owned by /// the `owner`. @@ -1485,7 +1488,7 @@ pub mod pallet { /// Origin must be `ForceOrigin`. /// /// Arguments: - /// - `owner`: The account cancelling approval for delegated transfer. + /// - `owner`: The owner of the approval to be cancelled by the `origin`. /// - `collection`: The collection of whose approval will be cancelled. /// - `delegate`: The account that is going to lose their approval. /// @@ -1549,10 +1552,11 @@ pub mod pallet { origin: OriginFor, collection: T::CollectionId, limit: u32, - ) -> DispatchResult { - let origin = ensure_signed(origin)?; - Self::do_clear_collection_approvals(origin, collection, limit)?; - Ok(()) + ) -> DispatchResultWithPostInfo { + let origin = ensure_signed(origin) + .map_err(|e| e.with_weight(T::WeightInfo::clear_collection_approvals(0)))?; + let removed_approvals = Self::do_clear_collection_approvals(origin, collection, limit)?; + Ok(Some(T::WeightInfo::clear_collection_approvals(removed_approvals)).into()) } /// Force-cancel all collection approvals granted by `owner` account, up to a specified @@ -1561,7 +1565,7 @@ pub mod pallet { /// Origin must be `ForceOrigin`. /// /// Arguments: - /// - `owner`: The account clearing all collection approvals. + /// - `owner`: The owner of the approvals to be cleared by the `origin`. /// - `collection`: The collection whose approvals will be cleared. /// - `limit`: The amount of collection approvals that will be cleared. /// @@ -1575,11 +1579,12 @@ pub mod pallet { owner: AccountIdLookupOf, collection: T::CollectionId, limit: u32, - ) -> DispatchResult { - T::ForceOrigin::ensure_origin(origin)?; + ) -> DispatchResultWithPostInfo { + T::ForceOrigin::ensure_origin(origin) + .map_err(|e| e.with_weight(T::WeightInfo::clear_collection_approvals(0)))?; let owner = T::Lookup::lookup(owner)?; - Self::do_clear_collection_approvals(owner, collection, limit)?; - Ok(()) + let removed_approvals = Self::do_clear_collection_approvals(owner, collection, limit)?; + Ok(Some(T::WeightInfo::clear_collection_approvals(removed_approvals)).into()) } /// Disallows changing the metadata or attributes of the item. diff --git a/pallets/nfts/src/tests.rs b/pallets/nfts/src/tests.rs index 4584a3bb..1b3de023 100644 --- a/pallets/nfts/src/tests.rs +++ b/pallets/nfts/src/tests.rs @@ -20,6 +20,7 @@ use enumflags2::BitFlags; use frame_support::{ assert_noop, assert_ok, + dispatch::{DispatchResultWithPostInfo, WithPostDispatchInfo}, pallet_prelude::MaxEncodedLen, traits::{ tokens::nonfungibles_v2::{Create, Destroy, Inspect, Mutate}, @@ -32,17 +33,28 @@ use pallet_balances::Error as BalancesError; use sp_core::{bounded::BoundedVec, Pair}; use sp_runtime::{ traits::{Dispatchable, IdentifyAccount}, - MultiSignature, MultiSigner, + DispatchError::BadOrigin, + DispatchResult, MultiSignature, MultiSigner, }; use crate::{mock::*, Event, SystemConfig, *}; +type CollectionId = ::CollectionId; type AccountIdOf = ::AccountId; +type WeightOf = ::WeightInfo; fn account(id: u8) -> AccountIdOf { [id; 32].into() } +fn root() -> RuntimeOrigin { + RuntimeOrigin::root() +} + +fn none() -> RuntimeOrigin { + RuntimeOrigin::none() +} + fn items() -> Vec<(AccountIdOf, u32, u32)> { let mut r: Vec<_> = Account::::iter().map(|x| x.0).collect(); r.sort(); @@ -150,6 +162,49 @@ fn item_config_from_disabled_settings(settings: BitFlags) -> ItemCo ItemConfig { settings: ItemSettings::from_disabled(settings) } } +fn clear_collection_approvals( + origin: RuntimeOrigin, + maybe_owner: Option, + collection: CollectionId, + limit: u32, +) -> DispatchResultWithPostInfo { + match maybe_owner { + Some(owner) => Nfts::force_clear_collection_approvals(origin, owner, collection, limit), + None => Nfts::clear_collection_approvals(origin, collection, limit), + } +} + +fn approve_collection_transfer( + origin: RuntimeOrigin, + maybe_owner: Option, + collection: CollectionId, + delegate: AccountIdOf, + maybe_deadline: Option>, +) -> DispatchResult { + match maybe_owner { + Some(owner) => Nfts::force_approve_collection_transfer( + origin, + owner, + collection, + delegate, + maybe_deadline, + ), + None => Nfts::approve_collection_transfer(origin, collection, delegate, maybe_deadline), + } +} + +fn cancel_collection_approval( + origin: RuntimeOrigin, + maybe_owner: Option, + collection: CollectionId, + delegate: AccountIdOf, +) -> DispatchResult { + match maybe_owner { + Some(owner) => Nfts::force_cancel_collection_approval(origin, owner, collection, delegate), + None => Nfts::cancel_collection_approval(origin, collection, delegate), + } +} + #[test] fn basic_setup_works() { new_test_ext().execute_with(|| { @@ -229,7 +284,7 @@ fn lifecycle_should_work() { bvec![0, 0] )); assert_eq!(Balances::reserved_balance(&owner), 5); - assert!(CollectionMetadataOf::::contains_key(0)); + assert!(CollectionMetadataOf::::contains_key(collection_id)); assert_ok!(Nfts::force_mint( RuntimeOrigin::signed(owner.clone()), @@ -369,10 +424,10 @@ fn destroy_with_bad_witness_should_not_work() { fn destroy_should_work() { new_test_ext().execute_with(|| { let collection_id = 0; - let item_id = 42; let collection_owner = account(1); - let item_owner = account(2); let delegate = account(3); + let item_id = 42; + let item_owner = account(2); Balances::make_free_balance_be(&collection_owner, 100); Balances::make_free_balance_be(&item_owner, 100); @@ -389,7 +444,6 @@ fn destroy_should_work() { item_owner.clone(), None )); - assert_eq!(AccountBalance::::get(collection_id, &item_owner), 1); assert_ok!(Nfts::approve_collection_transfer( RuntimeOrigin::signed(item_owner.clone()), collection_id, @@ -431,9 +485,6 @@ fn destroy_should_work() { collection_id, Nfts::get_destroy_witness(&collection_id).unwrap() )); - assert_eq!(AccountBalance::::get(collection_id, &collection_owner), 0); - assert!(!AccountBalance::::contains_key(collection_id, &collection_owner)); - assert_eq!(CollectionApprovals::::iter_prefix((collection_id,)).count(), 0); assert!(!ItemConfigOf::::contains_key(collection_id, item_id)); assert_eq!(ItemConfigOf::::iter_prefix(collection_id).count() as u32, 0); }); @@ -1829,11 +1880,11 @@ fn force_update_collection_should_work() { #[test] fn burn_works() { new_test_ext().execute_with(|| { + let admin = account(2); let collection_id = 0; + let collection_owner = account(1); let item_id_1 = 42; let item_id_2 = 69; - let collection_owner = account(1); - let admin = account(2); let item_owner = account(5); Balances::make_free_balance_be(&collection_owner, 100); @@ -1846,8 +1897,8 @@ fn burn_works() { RuntimeOrigin::signed(collection_owner.clone()), collection_id, Some(admin.clone()), - Some(account(2)), Some(account(3)), + Some(account(4)), )); assert_noop!( @@ -1878,8 +1929,7 @@ fn burn_works() { ); assert_ok!(Nfts::burn(RuntimeOrigin::signed(item_owner.clone()), collection_id, item_id_1)); assert_ok!(Nfts::burn(RuntimeOrigin::signed(item_owner.clone()), collection_id, item_id_2)); - assert_eq!(AccountBalance::::get(collection_id, &item_owner), 0); - assert_eq!(AccountBalance::::contains_key(collection_id, &item_owner), false); + assert!(!AccountBalance::::contains_key(collection_id, &item_owner)); assert_eq!(Balances::reserved_balance(collection_owner), 0); }); } @@ -1920,6 +1970,13 @@ fn approval_lifecycle_works() { account(2), None )); + assert!(events().contains(&Event::TransferApproved { + collection: 0, + item: Some(42), + owner: account(4), + delegate: account(2), + deadline: None + })); assert_ok!(Nfts::transfer(RuntimeOrigin::signed(account(2)), 0, 42, account(2))); // ensure we can't buy an item when the collection has a NonTransferableItems flag @@ -1958,9 +2015,9 @@ fn check_approval_works_without_deadline_works() { new_test_ext().execute_with(|| { let collection_id = 0; let collection_owner = account(1); + let delegate = account(3); let item_id = 42; let item_owner = account(2); - let delegate = account(3); Balances::make_free_balance_be(&item_owner, 100); // TODO: we should return a clear error for smart contracts. @@ -2068,9 +2125,9 @@ fn check_approval_with_deadline_works() { new_test_ext().execute_with(|| { let collection_id = 0; let collection_owner = account(1); + let delegate = account(3); let item_id = 42; let item_owner = account(2); - let delegate = account(3); Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( @@ -2168,152 +2225,188 @@ fn check_approval_with_deadline_works() { #[test] fn cancel_approval_works() { new_test_ext().execute_with(|| { + let collection_owner = account(1); + let collection_id = 0; + let delegate = account(3); + let item_id = 42; + let item_owner = account(2); + + Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), - account(1), + collection_owner.clone(), default_collection_config() )); assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(account(1)), - 0, - 42, - account(2), + RuntimeOrigin::signed(collection_owner.clone()), + collection_id, + item_id, + item_owner.clone(), default_item_config() )); assert_ok!(Nfts::approve_transfer( - RuntimeOrigin::signed(account(2)), - 0, - 42, - account(3), + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone(), None )); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(2)), 1, 42, account(3)), + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + 1, + item_id, + delegate.clone() + ), Error::::UnknownItem ); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(2)), 0, 43, account(3)), + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + 43, + delegate.clone() + ), Error::::UnknownItem ); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(3)), 0, 42, account(3)), + Nfts::cancel_approval( + RuntimeOrigin::signed(delegate.clone()), + collection_id, + item_id, + delegate.clone() + ), Error::::NoPermission ); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(2)), 0, 42, account(4)), + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + account(4) + ), Error::::NotDelegate ); - assert_ok!(Nfts::cancel_approval(RuntimeOrigin::signed(account(2)), 0, 42, account(3))); + // Throws `DelegateApprovalConflict`` if the delegate has been granted a collection + // approval. + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone(), + None + )); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(2)), 0, 42, account(3)), - Error::::NotDelegate + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone() + ), + Error::::DelegateApprovalConflict ); - - let current_block = 1; - System::set_block_number(current_block); - assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(account(1)), - 0, - 69, - account(2), - default_item_config() + assert_ok!(Nfts::cancel_collection_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone() )); - // approval expires after 2 blocks. - assert_ok!(Nfts::approve_transfer( - RuntimeOrigin::signed(account(2)), - 0, - 42, - account(3), - Some(2) + + assert_ok!(Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone() )); + assert!(events().contains(&Event::::ApprovalCancelled { + collection: collection_id, + item: Some(item_id), + owner: item_owner.clone(), + delegate: delegate.clone() + })); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::signed(account(5)), 0, 42, account(3)), - Error::::NoPermission + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone() + ), + Error::::NotDelegate ); - System::set_block_number(current_block + 3); - // 5 can cancel the approval since the deadline has passed. - assert_ok!(Nfts::cancel_approval(RuntimeOrigin::signed(account(5)), 0, 42, account(3))); - assert_eq!(approvals(0, 69), vec![]); - }); -} - -#[test] -fn cancel_collection_approval_works() { - new_test_ext().execute_with(|| { - let collection_id = 0; - let item_id = 42; - let collection_owner = account(1); - let item_owner = account(2); - let delegate = account(3); - - Balances::make_free_balance_be(&item_owner, 100); - assert_ok!(Nfts::force_create( - RuntimeOrigin::root(), - collection_owner.clone(), - default_collection_config() - )); + let current_block = 1; + System::set_block_number(current_block); assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(collection_owner), + RuntimeOrigin::signed(collection_owner.clone()), collection_id, - item_id, + 69, item_owner.clone(), default_item_config() )); - - assert_ok!(Nfts::approve_collection_transfer( + // approval expires after 2 blocks. + assert_ok!(Nfts::approve_transfer( RuntimeOrigin::signed(item_owner.clone()), collection_id, + item_id, delegate.clone(), - None + Some(2) )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); - - // Cancel an unapproved delegate. assert_noop!( - Nfts::cancel_collection_approval( - RuntimeOrigin::signed(item_owner.clone()), - 1, + Nfts::cancel_approval( + RuntimeOrigin::signed(account(5)), + collection_id, + item_id, delegate.clone() ), - Error::::Unapproved + Error::::NoPermission ); - // Successfully cancel a collection approval. - assert_ok!(Nfts::cancel_collection_approval( - RuntimeOrigin::signed(item_owner.clone()), + System::set_block_number(current_block + 3); + // 5 can cancel the approval since the deadline has passed. + assert_ok!(Nfts::cancel_approval( + RuntimeOrigin::signed(account(5)), collection_id, + item_id, delegate.clone() )); - assert_eq!(Balances::reserved_balance(&item_owner), 0); assert!(events().contains(&Event::::ApprovalCancelled { collection: collection_id, - item: None, - owner: item_owner.clone(), - delegate: delegate.clone() + item: Some(item_id), + owner: item_owner, + delegate })); - assert_eq!( - CollectionApprovals::::get((collection_id, item_owner, delegate.clone())), - None - ); - - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(delegate), collection_id, item_id, account(4)), - Error::::NoPermission - ); + assert_eq!(approvals(collection_id, 69), vec![]); }); } #[test] -fn force_cancel_collection_approvals_work() { +fn cancel_collection_approval_works() { new_test_ext().execute_with(|| { let collection_id = 0; - let item_id = 42; let collection_owner = account(1); - let item_owner = account(2); let delegate = account(3); + let item_id = 42; + let item_owner = account(2); + + // Origin checks for the `cancel_collection_approval`. + for origin in [root(), none()] { + assert_noop!( + Nfts::cancel_collection_approval(origin, collection_id, delegate.clone()), + BadOrigin + ); + } + // Origin checks for the `force_cancel_collection_approval`. + for origin in [RuntimeOrigin::signed(item_owner.clone()), none()] { + assert_noop!( + Nfts::force_cancel_collection_approval( + origin, + item_owner.clone(), + collection_id, + delegate.clone() + ), + BadOrigin + ); + } Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( @@ -2329,48 +2422,58 @@ fn force_cancel_collection_approvals_work() { default_item_config() )); - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - collection_id, - delegate.clone(), - None - )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); + for (origin, maybe_owner) in + [(RuntimeOrigin::signed(item_owner.clone()), None), (root(), Some(item_owner.clone()))] + { + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone(), + None + )); + assert_eq!(Balances::reserved_balance(&item_owner), 1); - // Cancel an unapproved delegate. - assert_noop!( - Nfts::force_cancel_collection_approval( - RuntimeOrigin::root(), + // Cancel an unapproved delegate. + assert_noop!( + cancel_collection_approval( + origin.clone(), + maybe_owner.clone(), + 1, + delegate.clone() + ), + Error::::Unapproved + ); + + // Successfully cancel a collection approval. + assert_ok!(cancel_collection_approval( + origin.clone(), + maybe_owner.clone(), + collection_id, + delegate.clone() + )); + assert_eq!(Balances::reserved_balance(&item_owner), 0); + assert!(events().contains(&Event::::ApprovalCancelled { + collection: collection_id, + item: None, + owner: item_owner.clone(), + delegate: delegate.clone() + })); + assert!(!CollectionApprovals::::contains_key(( + collection_id, item_owner.clone(), - 1, delegate.clone() - ), - Error::::Unapproved - ); - - // Successfully cancel a collection approval. - assert_ok!(Nfts::force_cancel_collection_approval( - RuntimeOrigin::root(), - item_owner.clone(), - collection_id, - delegate.clone() - )); - assert_eq!(Balances::reserved_balance(&item_owner), 0); - assert!(events().contains(&Event::::ApprovalCancelled { - collection: collection_id, - item: None, - owner: item_owner.clone(), - delegate: delegate.clone() - })); - assert_eq!( - CollectionApprovals::::get((collection_id, item_owner, delegate.clone())), - None - ); + ))); - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(delegate), collection_id, item_id, account(4)), - Error::::NoPermission - ); + assert_noop!( + Nfts::transfer( + RuntimeOrigin::signed(delegate.clone()), + collection_id, + item_id, + account(4) + ), + Error::::NoPermission + ); + } }); } @@ -2413,20 +2516,36 @@ fn approving_multiple_accounts_works() { account(5), Some(2) )); + assert_eq!( + events().last_chunk::<3>(), + Some(&[ + Event::TransferApproved { + collection: 0, + item: Some(42), + owner: account(2), + delegate: account(3), + deadline: None + }, + Event::TransferApproved { + collection: 0, + item: Some(42), + owner: account(2), + delegate: account(4), + deadline: None + }, + Event::TransferApproved { + collection: 0, + item: Some(42), + owner: account(2), + delegate: account(5), + deadline: Some(current_block + 2) + } + ]) + ); assert_eq!( approvals(0, 42), vec![(account(3), None), (account(4), None), (account(5), Some(current_block + 2))] ); - - assert_ok!(Nfts::transfer(RuntimeOrigin::signed(account(4)), 0, 42, account(6))); - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(7)), - Error::::NoPermission - ); - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(account(5)), 0, 42, account(8)), - Error::::NoPermission - ); }); } @@ -2466,138 +2585,47 @@ fn approvals_limit_works() { #[test] fn approve_collection_transfer_works() { new_test_ext().execute_with(|| { - let (collection, locked_collection) = (0, 1); - let item = 42; + let (collection_id, locked_collection_id) = (0, 1); let collection_owner = account(1); - let item_owner = account(2); let delegate = account(3); + let item_id = 42; + let item_owner = account(2); + + // Origin checks for the `approve_collection_transfer`. + for origin in [root(), none()] { + assert_noop!( + Nfts::approve_collection_transfer(origin, collection_id, delegate.clone(), None), + BadOrigin + ); + } + // Origin checks for the `force_approve_collection_transfer`. + for origin in [RuntimeOrigin::signed(item_owner.clone()), none()] { + assert_noop!( + Nfts::force_approve_collection_transfer( + origin, + item_owner.clone(), + collection_id, + delegate.clone(), + None + ), + BadOrigin + ); + } Balances::make_free_balance_be(&item_owner, 100); Balances::make_free_balance_be(&delegate, 100); - assert_ok!(Nfts::force_create( - RuntimeOrigin::root(), - collection_owner.clone(), - default_collection_config() - )); - assert_ok!(Nfts::force_create( - RuntimeOrigin::root(), - collection_owner.clone(), - default_collection_config() - )); - - // Approve collection without items, throws error `Error::NoItemOwned`. - assert_noop!( - Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - collection, - delegate.clone(), - None - ), - Error::::NoItemOwned - ); - - assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(collection_owner.clone()), - collection, - item, - item_owner.clone(), - default_item_config() - )); - assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(collection_owner.clone()), - locked_collection, - item, - item_owner.clone(), - default_item_config() - )); - - // Throws error `Error::ItemsNonTransferable`. - assert_ok!(Nfts::lock_collection( - RuntimeOrigin::signed(collection_owner), - locked_collection, - CollectionSettings::from_disabled(CollectionSetting::TransferableItems.into()) - )); - assert_noop!( - Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - locked_collection, - delegate.clone(), - None - ), - Error::::ItemsNonTransferable - ); // Approve unknown collection, throws error `Error::NoItemOwned`. assert_noop!( Nfts::approve_collection_transfer( RuntimeOrigin::signed(item_owner.clone()), - 2, + collection_id, delegate.clone(), None ), Error::::NoItemOwned ); - // Approval expires after `deadline`. - let deadline = 10; - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - collection, - delegate.clone(), - Some(deadline) - )); - assert_ok!(Nfts::check_approval(&collection, &None, &item_owner, &delegate)); - System::set_block_number(deadline + 2); - assert_noop!( - Nfts::check_approval(&collection, &None, &item_owner, &delegate), - Error::::ApprovalExpired - ); - - // Approve delegate of an existing expired approval to transfer. - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - collection, - delegate.clone(), - Some(deadline) - )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); - let now = System::block_number(); - assert!(events().contains(&Event::::TransferApproved { - collection: 0, - item: None, - owner: item_owner.clone(), - delegate: delegate.clone(), - deadline: Some(now + deadline) - })); - assert_eq!( - CollectionApprovals::::get((0, item_owner.clone(), delegate.clone())), - Some((Some(now + deadline), 1)) - ); - - // Approve same delegate again not updating the total reserved funds. - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(item_owner.clone()), - collection, - delegate.clone(), - None - )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); - - assert_ok!(Nfts::transfer(RuntimeOrigin::signed(delegate), collection, item, account(4))); - }); -} - -#[test] -fn force_approve_collection_transfer_works() { - new_test_ext().execute_with(|| { - let (collection, locked_collection) = (0, 1); - let item = 42; - let collection_owner = account(1); - let item_owner = account(2); - let delegate = account(3); - - Balances::make_free_balance_be(&item_owner, 100); - Balances::make_free_balance_be(&delegate, 100); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), collection_owner.clone(), @@ -2611,10 +2639,9 @@ fn force_approve_collection_transfer_works() { // Approve collection without items, throws error `Error::NoItemOwned`. assert_noop!( - Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - collection, + Nfts::approve_collection_transfer( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, delegate.clone(), None ), @@ -2623,97 +2650,84 @@ fn force_approve_collection_transfer_works() { assert_ok!(Nfts::force_mint( RuntimeOrigin::signed(collection_owner.clone()), - collection, - item, + collection_id, + item_id, item_owner.clone(), default_item_config() )); assert_ok!(Nfts::force_mint( RuntimeOrigin::signed(collection_owner.clone()), - locked_collection, - item, + locked_collection_id, + item_id, item_owner.clone(), default_item_config() )); - // Throws error `Error::ItemsNonTransferable`. - assert_ok!(Nfts::lock_collection( - RuntimeOrigin::signed(collection_owner), - locked_collection, - CollectionSettings::from_disabled(CollectionSetting::TransferableItems.into()) - )); - assert_noop!( - Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - locked_collection, + for (origin, maybe_item_owner) in + [(RuntimeOrigin::signed(item_owner.clone()), None), (root(), Some(item_owner.clone()))] + { + // Throws error `Error::ItemsNonTransferable`. + assert_ok!(Nfts::lock_collection( + RuntimeOrigin::signed(collection_owner.clone()), + locked_collection_id, + CollectionSettings::from_disabled(CollectionSetting::TransferableItems.into()) + )); + assert_noop!( + approve_collection_transfer( + origin.clone(), + maybe_item_owner.clone(), + locked_collection_id, + delegate.clone(), + None + ), + Error::::ItemsNonTransferable + ); + + // Approve unknown collection, throws error `Error::NoItemOwned`. + assert_noop!( + approve_collection_transfer( + origin.clone(), + maybe_item_owner.clone(), + 2, + delegate.clone(), + None + ), + Error::::NoItemOwned + ); + + assert_ok!(approve_collection_transfer( + origin.clone(), + maybe_item_owner.clone(), + collection_id, delegate.clone(), None - ), - Error::::ItemsNonTransferable - ); - - // Approve unknown collection, throws error `Error::NoItemOwned`. - assert_noop!( - Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - 2, + )); + assert!(events().contains(&Event::::TransferApproved { + collection: collection_id, + item: None, + owner: item_owner.clone(), + delegate: delegate.clone(), + deadline: None, + })); + assert_eq!(Balances::reserved_balance(&item_owner), 1); + + // Approve same delegate again not updating the total reserved funds. + assert_ok!(approve_collection_transfer( + origin.clone(), + maybe_item_owner.clone(), + collection_id, delegate.clone(), None - ), - Error::::NoItemOwned - ); - - // Approval expires after `deadline`. - let deadline = 10; - assert_ok!(Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - collection, - delegate.clone(), - Some(deadline) - )); - assert_ok!(Nfts::check_approval(&collection, &None, &item_owner, &delegate)); - System::set_block_number(deadline + 2); - assert_noop!( - Nfts::check_approval(&collection, &None, &item_owner, &delegate), - Error::::ApprovalExpired - ); - - // Approve delegate of an existing expired approval to transfer. - assert_ok!(Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - collection, - delegate.clone(), - Some(deadline) - )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); - let now = System::block_number(); - assert!(events().contains(&Event::::TransferApproved { - collection: 0, - item: None, - owner: item_owner.clone(), - delegate: delegate.clone(), - deadline: Some(now + deadline) - })); - assert_eq!( - CollectionApprovals::::get((0, item_owner.clone(), delegate.clone())), - Some((Some(now + deadline), 1)) - ); - - // Approve same delegate again not updating the total reserved funds. - assert_ok!(Nfts::force_approve_collection_transfer( - RuntimeOrigin::root(), - item_owner.clone(), - collection, - delegate.clone(), - None - )); - assert_eq!(Balances::reserved_balance(&item_owner), 1); + )); + assert_eq!(Balances::reserved_balance(&item_owner), 1); - assert_ok!(Nfts::transfer(RuntimeOrigin::signed(delegate), collection, item, account(4))); + // Clean up. + assert_ok!(Nfts::cancel_collection_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone() + )); + } }); } @@ -2772,11 +2786,11 @@ fn approval_deadline_works() { #[test] fn cancel_approval_works_with_admin() { new_test_ext().execute_with(|| { - let collection_id = 0; + let delegate = account(3); let item_id = 42; - let collection_owner = account(1); let item_owner = account(2); - let delegate = account(3); + let collection_id = 0; + let collection_owner = account(1); Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( @@ -2827,7 +2841,8 @@ fn cancel_approval_works_with_admin() { Error::::NotDelegate ); - // delegate approval conflicts. + // Throws `DelegateApprovalConflict`` if the delegate has been granted a collection + // approval. assert_ok!(Nfts::approve_collection_transfer( RuntimeOrigin::signed(item_owner.clone()), collection_id, @@ -2855,9 +2870,16 @@ fn cancel_approval_works_with_admin() { item_id, delegate.clone() )); + assert!(events().contains(&Event::::ApprovalCancelled { + collection: collection_id, + item: Some(item_id), + owner: item_owner.clone(), + delegate + })); + assert_noop!( Nfts::cancel_approval( - RuntimeOrigin::signed(item_owner.clone()), + RuntimeOrigin::signed(item_owner), collection_id, item_id, account(5) @@ -2870,184 +2892,185 @@ fn cancel_approval_works_with_admin() { #[test] fn cancel_approval_works_with_force() { new_test_ext().execute_with(|| { + let collection_owner = account(1); + let collection_id = 0; + let delegate = account(3); + let item_id = 42; + let item_owner = account(2); + + Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), - account(1), + collection_owner.clone(), default_collection_config() )); assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(account(1)), - 0, - 42, - account(2), + RuntimeOrigin::signed(collection_owner.clone()), + collection_id, + item_id, + item_owner.clone(), default_item_config() )); assert_ok!(Nfts::approve_transfer( - RuntimeOrigin::signed(account(2)), - 0, - 42, - account(3), + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone(), None )); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::root(), 1, 42, account(1)), + Nfts::cancel_approval(RuntimeOrigin::root(), 1, item_id, collection_owner.clone()), Error::::UnknownItem ); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::root(), 0, 43, account(1)), + Nfts::cancel_approval( + RuntimeOrigin::root(), + collection_id, + 43, + collection_owner.clone() + ), Error::::UnknownItem ); assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::root(), 0, 42, account(4)), + Nfts::cancel_approval(RuntimeOrigin::root(), collection_id, item_id, account(4)), Error::::NotDelegate ); - assert_ok!(Nfts::cancel_approval(RuntimeOrigin::root(), 0, 42, account(3))); - assert_noop!( - Nfts::cancel_approval(RuntimeOrigin::root(), 0, 42, account(1)), - Error::::NotDelegate - ); - }); -} - -#[test] -fn clear_all_transfer_approvals_works() { - new_test_ext().execute_with(|| { - assert_ok!(Nfts::force_create( - RuntimeOrigin::root(), - account(1), - default_collection_config() - )); - assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(account(1)), - 0, - 42, - account(2), - default_item_config() - )); - - assert_ok!(Nfts::approve_transfer( - RuntimeOrigin::signed(account(2)), - 0, - 42, - account(3), - None - )); - assert_ok!(Nfts::approve_transfer( - RuntimeOrigin::signed(account(2)), - 0, - 42, - account(4), + // Throws `DelegateApprovalConflict`` if the delegate has been granted a collection + // approval. + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone(), None )); - assert_noop!( - Nfts::clear_all_transfer_approvals(RuntimeOrigin::signed(account(3)), 0, 42), - Error::::NoPermission + Nfts::cancel_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + item_id, + delegate.clone() + ), + Error::::DelegateApprovalConflict ); + assert_ok!(Nfts::cancel_collection_approval( + RuntimeOrigin::signed(item_owner.clone()), + collection_id, + delegate.clone() + )); - assert_ok!(Nfts::clear_all_transfer_approvals(RuntimeOrigin::signed(account(2)), 0, 42)); - - assert!(events().contains(&Event::::AllApprovalsCancelled { - collection: 0, - item: Some(42), - owner: account(2), + assert_ok!(Nfts::cancel_approval( + RuntimeOrigin::root(), + collection_id, + item_id, + delegate.clone() + )); + assert!(events().contains(&Event::::ApprovalCancelled { + collection: collection_id, + item: Some(item_id), + owner: item_owner, + delegate })); - assert_eq!(approvals(0, 42), vec![]); - assert_eq!(CollectionApprovals::::iter_prefix((0, account(2))).count(), 0); - - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(account(3)), 0, 42, account(5)), - Error::::NoPermission - ); assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(account(4)), 0, 42, account(5)), - Error::::NoPermission + Nfts::cancel_approval(RuntimeOrigin::root(), collection_id, item_id, collection_owner), + Error::::NotDelegate ); }); } #[test] -fn clear_collection_approvals_works() { +fn clear_all_transfer_approvals_works() { new_test_ext().execute_with(|| { let collection_id = 0; - let item_id = 42; - let owner = account(1); + let collection_owner = account(1); let delegate_1 = account(3); let delegate_2 = account(4); - let balance = 100; + let item_id = 42; + let item_owner = account(2); - Balances::make_free_balance_be(&owner, balance); + Balances::make_free_balance_be(&collection_owner, 100); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), - owner.clone(), + collection_owner.clone(), default_collection_config() )); assert_ok!(Nfts::force_mint( - RuntimeOrigin::signed(account(1)), + RuntimeOrigin::signed(collection_owner.clone()), + collection_id, + 43, + collection_owner.clone(), + default_item_config() + )); + assert_ok!(Nfts::force_mint( + RuntimeOrigin::signed(collection_owner.clone()), collection_id, item_id, - owner.clone(), + item_owner.clone(), default_item_config() )); - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(owner.clone()), + assert_ok!(Nfts::approve_transfer( + RuntimeOrigin::signed(item_owner.clone()), collection_id, + item_id, delegate_1.clone(), None )); - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(owner.clone()), + assert_ok!(Nfts::approve_transfer( + RuntimeOrigin::signed(item_owner.clone()), collection_id, + item_id, delegate_2.clone(), None )); - // Remove zero collection approvals, no event emitted. - assert_ok!(Nfts::clear_collection_approvals( - RuntimeOrigin::signed(owner.clone()), - collection_id, - 0 - )); - assert_eq!(Balances::free_balance(&owner), balance - 2); - assert_eq!( - CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), - 2 + + assert_noop!( + Nfts::clear_all_transfer_approvals( + RuntimeOrigin::signed(delegate_1.clone()), + collection_id, + item_id + ), + Error::::NoPermission ); - assert!(!events().contains(&Event::::ApprovalsCancelled { - collection: collection_id, - item: None, - owner: owner.clone(), - })); - // Partially removes collection approvals. - assert_ok!(Nfts::clear_collection_approvals( - RuntimeOrigin::signed(owner.clone()), + // Throws `DelegateApprovalConflict` if there are existing collection approvals. + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(collection_owner.clone()), collection_id, - 1 + delegate_1.clone(), + None )); - assert_eq!(Balances::free_balance(&owner), balance - 1); - assert_eq!( - CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), - 1 + assert_noop!( + Nfts::clear_all_transfer_approvals( + RuntimeOrigin::signed(collection_owner.clone()), + collection_id, + item_id + ), + Error::::DelegateApprovalConflict ); + assert_ok!(Nfts::cancel_collection_approval( + RuntimeOrigin::signed(collection_owner.clone()), + collection_id, + delegate_1.clone() + )); - // Successfully remove all collection approvals. - assert_ok!(Nfts::clear_collection_approvals( - RuntimeOrigin::signed(owner.clone()), + assert_ok!(Nfts::clear_all_transfer_approvals( + RuntimeOrigin::signed(item_owner.clone()), collection_id, - 2 + item_id )); - assert!(events().contains(&Event::::ApprovalsCancelled { + + assert!(events().contains(&Event::::AllApprovalsCancelled { collection: collection_id, - item: None, - owner: owner.clone(), + item: Some(item_id), + owner: item_owner.clone(), })); - assert_eq!(Balances::free_balance(&owner), balance); - assert!(CollectionApprovals::::iter_prefix((collection_id, owner)) - .count() - .is_zero()); + assert_eq!(approvals(collection_id, item_id), vec![]); + assert_eq!( + CollectionApprovals::::iter_prefix((collection_id, item_owner.clone())).count(), + 0 + ); assert_noop!( Nfts::transfer(RuntimeOrigin::signed(delegate_1), collection_id, item_id, account(5)), @@ -3061,16 +3084,30 @@ fn clear_collection_approvals_works() { } #[test] -fn force_clear_collection_approvals_work() { +fn clear_collection_approvals_works() { new_test_ext().execute_with(|| { + let balance = 100; let collection_id = 0; - let item_id = 42; - let owner = account(1); let delegate_1 = account(3); let delegate_2 = account(4); - let balance = 100; + let item_id = 42; + let owner = account(1); + + // Origin checks for the `clear_collection_approvals`. + for origin in [root(), none()] { + assert_noop!( + Nfts::clear_collection_approvals(origin, collection_id, 0), + BadOrigin.with_weight(WeightOf::::clear_collection_approvals(0)) + ); + } + // Origin checks for the `force_clear_collection_approvals`. + for origin in [RuntimeOrigin::signed(owner.clone()), none()] { + assert_noop!( + Nfts::force_clear_collection_approvals(origin, owner.clone(), collection_id, 0), + BadOrigin.with_weight(WeightOf::::clear_collection_approvals(0)) + ); + } - Balances::make_free_balance_be(&owner, balance); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), owner.clone(), @@ -3084,74 +3121,85 @@ fn force_clear_collection_approvals_work() { default_item_config() )); - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(owner.clone()), - collection_id, - delegate_1.clone(), - None - )); - assert_ok!(Nfts::approve_collection_transfer( - RuntimeOrigin::signed(owner.clone()), - collection_id, - delegate_2.clone(), - None - )); - // Remove zero collection approvals, no event emitted. - assert_ok!(Nfts::force_clear_collection_approvals( - RuntimeOrigin::root(), - owner.clone(), - collection_id, - 0 - )); - assert_eq!(Balances::free_balance(&owner), balance - 2); - assert_eq!( - CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), - 2 - ); - assert!(!events().contains(&Event::::ApprovalsCancelled { - collection: collection_id, - item: None, - owner: owner.clone(), - })); + for (origin, maybe_owner) in + [(root(), Some(owner.clone())), (RuntimeOrigin::signed(owner.clone()), None)] + { + Balances::make_free_balance_be(&owner, balance); - // Partially removes collection approvals. - assert_ok!(Nfts::force_clear_collection_approvals( - RuntimeOrigin::root(), - owner.clone(), - collection_id, - 1 - )); - assert_eq!(Balances::free_balance(&owner), balance - 1); - assert_eq!( - CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), - 1 - ); + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(owner.clone()), + collection_id, + delegate_1.clone(), + None + )); + assert_ok!(Nfts::approve_collection_transfer( + RuntimeOrigin::signed(owner.clone()), + collection_id, + delegate_2.clone(), + None + )); + // Removes zero collection approval. + assert_eq!( + clear_collection_approvals(origin.clone(), maybe_owner.clone(), collection_id, 0), + Ok(Some(WeightOf::::clear_collection_approvals(0)).into()) + ); + assert_eq!(Balances::free_balance(&owner), balance - 2); + assert_eq!( + CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), + 2 + ); + assert!(!events().contains(&Event::::ApprovalsCancelled { + collection: collection_id, + item: None, + owner: owner.clone(), + })); + + // Partially removes collection approvals. + assert_eq!( + clear_collection_approvals(origin.clone(), maybe_owner.clone(), collection_id, 1), + Ok(Some(WeightOf::::clear_collection_approvals(1)).into()) + ); + assert_eq!(Balances::free_balance(&owner), balance - 1); + assert_eq!( + CollectionApprovals::::iter_prefix((collection_id, owner.clone())).count(), + 1 + ); - // Successfully remove all collection approvals. - assert_ok!(Nfts::force_clear_collection_approvals( - RuntimeOrigin::root(), - owner.clone(), - collection_id, - 2 - )); - assert!(events().contains(&Event::::ApprovalsCancelled { - collection: collection_id, - item: None, - owner: owner.clone(), - })); - assert_eq!(Balances::free_balance(&owner), balance); - assert!(CollectionApprovals::::iter_prefix((collection_id, owner)) - .count() - .is_zero()); + // Successfully removes all collection approvals. Only charges post-dispatch weight for + // the removed approvals. + assert_eq!( + clear_collection_approvals(origin.clone(), maybe_owner.clone(), collection_id, 10), + Ok(Some(WeightOf::::clear_collection_approvals(1)).into()) + ); + assert!(events().contains(&Event::::ApprovalsCancelled { + collection: collection_id, + item: None, + owner: owner.clone(), + })); + assert_eq!(Balances::free_balance(&owner), balance); + assert!(CollectionApprovals::::iter_prefix((collection_id, owner.clone())) + .count() + .is_zero()); - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(delegate_1), collection_id, item_id, account(5)), - Error::::NoPermission - ); - assert_noop!( - Nfts::transfer(RuntimeOrigin::signed(delegate_2), collection_id, item_id, account(5)), - Error::::NoPermission - ); + assert_noop!( + Nfts::transfer( + RuntimeOrigin::signed(delegate_1.clone()), + collection_id, + item_id, + account(5) + ), + Error::::NoPermission + ); + assert_noop!( + Nfts::transfer( + RuntimeOrigin::signed(delegate_2.clone()), + collection_id, + item_id, + account(5) + ), + Error::::NoPermission + ); + } }); } @@ -3159,8 +3207,8 @@ fn force_clear_collection_approvals_work() { fn collection_item_works() { new_test_ext().execute_with(|| { let collection_id = 0; - let user_id = account(1); let total_items = 10; + let user_id = account(1); // No collection. assert_eq!(Nfts::collection_items(collection_id), None);