From 504d0f0bf10b827abb6467e8e3e18d5401bd3420 Mon Sep 17 00:00:00 2001 From: chungquantin <56880684+chungquantin@users.noreply.github.com> Date: Thu, 9 Jan 2025 13:12:33 +0700 Subject: [PATCH] feat: allow admin to force cancel approvals --- pallets/nfts/src/features/approvals.rs | 34 +++--- pallets/nfts/src/lib.rs | 61 ++++++++--- pallets/nfts/src/tests.rs | 139 ++++++++++++++++--------- 3 files changed, 154 insertions(+), 80 deletions(-) diff --git a/pallets/nfts/src/features/approvals.rs b/pallets/nfts/src/features/approvals.rs index e40fb6e8..cee53666 100644 --- a/pallets/nfts/src/features/approvals.rs +++ b/pallets/nfts/src/features/approvals.rs @@ -188,7 +188,7 @@ impl, I: 'static> Pallet { Ok(()) } - /// Approves the transfer of all collection items of `owner` to a delegate. + /// Approves the transfer of all collection items of `owner` to a `delegate`. /// /// This function is used to approve the transfer of all (current and future) collection items /// of `owner` to a `delegate`. The `delegate` account will be allowed to take control of the @@ -203,7 +203,7 @@ impl, I: 'static> Pallet { /// /// - `owner`: The owner of the collection items. /// - `collection`: The identifier of the collection. - /// - `delegate`: The account that will be allowed to take control of the collection items. + /// - `delegate`: The account that will be approved to take control of the collection items. /// - `maybe_deadline`: The optional deadline (in block numbers) specifying the time limit for /// the approval. pub(crate) fn do_approve_collection_transfer( @@ -232,9 +232,7 @@ impl, I: 'static> Pallet { let deposit_required = T::CollectionApprovalDeposit::get(); let current_deposit = maybe_approval.take().map(|(_, deposit)| deposit).unwrap_or_default(); - if current_deposit < deposit_required { - T::Currency::reserve(&owner, deposit_required - current_deposit)?; - } + T::Currency::reserve(&owner, deposit_required - current_deposit)?; *maybe_approval = Some((deadline, deposit_required)); Ok(()) }, @@ -266,7 +264,7 @@ impl, I: 'static> Pallet { delegate: T::AccountId, ) -> DispatchResult { let (_, deposit) = CollectionApprovals::::take((&collection, &owner, &delegate)) - .ok_or(Error::::Unapproved)?; + .ok_or(Error::::NotDelegate)?; T::Currency::unreserve(&owner, deposit); @@ -312,12 +310,12 @@ impl, I: 'static> Pallet { Ok(removed_approvals) } - /// Checks whether the `delegate` is approved to transfer collection items of `owner`. + /// Checks whether the `delegate` has permission to transfer collection items of `owner`. /// /// - `collection`: The identifier of the collection. /// - `owner`: The owner of the collection items. - /// - `delegate`: The account to check for approval to transfer collection items of `owner`. - fn check_collection_approval( + /// - `delegate`: The account to check for permission to transfer collection items of `owner`. + fn check_collection_approval_permission( collection: &T::CollectionId, owner: &T::AccountId, delegate: &T::AccountId, @@ -332,24 +330,26 @@ impl, I: 'static> Pallet { Ok(()) } - /// Checks whether the `delegate` is approved by `owner` to transfer its collection item(s). If - /// the `delegate` is approved for all `owner`'s collection items it can transfer every item - /// without requiring explicit approval for an item. + /// Checks whether the `delegate` has permission to transfer `owner`'s collection item(s). + /// If the `delegate` has permission to transfer all `owner`'s collection items, they can + /// transfer any item without needing explicit approval for each individual item. /// /// - `collection`: The identifier of the collection. - /// - `maybe_item`: The optional item of the collection that the delegated account has an - /// approval to transfer. If not provided, an approval to transfer all `owner`'s collection + /// - `maybe_item`: The optional item of the collection that the delegated account has + /// permission to transfer. If not provided, permission to transfer all `owner`'s collection /// items will be checked. /// - `owner`: The owner of the specified collection item. - /// - `delegate`: The account to check for approval to transfer collection item(s) of owner. - pub fn check_approval( + /// - `delegate`: The account to check for permission to transfer collection item(s) from the + /// owner. + pub fn check_approval_permission( collection: &T::CollectionId, maybe_item: &Option, owner: &T::AccountId, delegate: &T::AccountId, ) -> DispatchResult { // Check if `delegate` has permission to transfer `owner`'s collection items. - let Err(error) = Self::check_collection_approval(collection, owner, delegate) else { + let Err(error) = Self::check_collection_approval_permission(collection, owner, delegate) + else { return Ok(()); }; diff --git a/pallets/nfts/src/lib.rs b/pallets/nfts/src/lib.rs index 2851e826..a087f046 100644 --- a/pallets/nfts/src/lib.rs +++ b/pallets/nfts/src/lib.rs @@ -59,8 +59,8 @@ use codec::{Decode, Encode}; use frame_support::{ dispatch::WithPostDispatchInfo, traits::{ - tokens::Locker, BalanceStatus::Reserved, Currency, EnsureOriginWithArg, Incrementable, - ReservableCurrency, + nonfungibles_v2::InspectRole, tokens::Locker, BalanceStatus::Reserved, Currency, + EnsureOriginWithArg, Incrementable, ReservableCurrency, }, }; use frame_system::Config as SystemConfig; @@ -448,7 +448,13 @@ pub mod pallet { NMapKey, // owner NMapKey, // delegate ), - (Option>, DepositBalanceOf), + ( + // The optional deadline for the approval. If specified, the approval is valid on or + // before the given block number. + Option>, + // The balance to be deposited. + DepositBalanceOf, + ), >; #[pallet::event] @@ -494,8 +500,9 @@ pub mod pallet { admin: Option, freezer: Option, }, - /// An `item` of a `collection` has been approved by the `owner` for transfer by - /// a `delegate`. + /// A provided `item` of a `collection`, or if no `item` is provided, all items in the + /// `collection` owned by the `owner` have been approved by the `owner` for transfer by a + /// `delegate`. TransferApproved { collection: T::CollectionId, item: Option, @@ -503,8 +510,9 @@ pub mod pallet { delegate: T::AccountId, deadline: Option>, }, - /// An approval for a `delegate` account to transfer a specific `item` in a `collection` or - /// all collection items owned by the `owner` has been cancelled by the owner. + /// An approval for a `delegate` account to transfer a specific `item` in a `collection`, + /// or if no `item` is provided, all collection items owned by the `owner` have been + /// cancelled by the `owner`. ApprovalCancelled { collection: T::CollectionId, item: Option, @@ -658,8 +666,6 @@ pub mod pallet { NotDelegate, /// The delegate turned out to be different to what was expected. WrongDelegate, - /// No approval exists that would allow the transfer. - Unapproved, /// The named owner has not signed ownership acceptance of the collection. Unaccepted, /// The item is locked (non-transferable). @@ -1088,7 +1094,12 @@ pub mod pallet { Self::do_transfer(collection, item, dest, |_, details| { if details.owner != origin { - Self::check_approval(&collection, &Some(item), &details.owner, &origin)?; + Self::check_approval_permission( + &collection, + &Some(item), + &details.owner, + &origin, + )?; } Ok(()) }) @@ -2089,9 +2100,19 @@ pub mod pallet { collection: T::CollectionId, delegate: AccountIdLookupOf, ) -> DispatchResult { - T::ForceOrigin::ensure_origin(origin)?; + let maybe_check_origin = T::ForceOrigin::try_origin(origin) + .map(|_| None) + .or_else(|origin| ensure_signed(origin).map(Some).map_err(DispatchError::from))?; let delegate = T::Lookup::lookup(delegate)?; let owner = T::Lookup::lookup(owner)?; + + if let Some(check_origin) = maybe_check_origin { + ensure!( + Self::is_admin(&collection, &check_origin) || + Self::is_issuer(&collection, &check_origin), + Error::::NoPermission + ); + } Self::do_cancel_collection_approval(owner, collection, delegate) } @@ -2142,9 +2163,23 @@ pub mod pallet { collection: T::CollectionId, limit: u32, ) -> DispatchResultWithPostInfo { - T::ForceOrigin::ensure_origin(origin) - .map_err(|e| e.with_weight(T::WeightInfo::clear_collection_approvals(0)))?; + let maybe_check_origin = + T::ForceOrigin::try_origin(origin).map(|_| None).or_else(|origin| { + ensure_signed(origin).map(Some).map_err(|e| { + DispatchError::from(e) + .with_weight(T::WeightInfo::clear_collection_approvals(0)) + }) + })?; let owner = T::Lookup::lookup(owner)?; + + if let Some(check_origin) = maybe_check_origin { + ensure!( + Self::is_admin(&collection, &check_origin) || + Self::is_issuer(&collection, &check_origin), + Error::::NoPermission + .with_weight(T::WeightInfo::clear_collection_approvals(0)) + ); + } let removed_approvals = Self::do_clear_collection_approvals(owner, collection, limit)?; Ok(Some(T::WeightInfo::clear_collection_approvals(removed_approvals)).into()) } diff --git a/pallets/nfts/src/tests.rs b/pallets/nfts/src/tests.rs index 5c78450d..04212223 100644 --- a/pallets/nfts/src/tests.rs +++ b/pallets/nfts/src/tests.rs @@ -4469,17 +4469,19 @@ fn force_clear_collection_approvals_works() { let item_owner = account(1); let delegates = 10..20; - for origin in [RuntimeOrigin::signed(item_owner.clone()), none()] { - assert_noop!( - Nfts::force_clear_collection_approvals( - origin, - item_owner.clone(), - collection_id, - 0 - ), - BadOrigin.with_weight(WeightOf::clear_collection_approvals(0)) - ); - } + assert_noop!( + Nfts::force_clear_collection_approvals( + RuntimeOrigin::signed(item_owner.clone()), + item_owner.clone(), + collection_id, + 0 + ), + Error::::NoPermission.with_weight(WeightOf::clear_collection_approvals(0)) + ); + assert_noop!( + Nfts::force_clear_collection_approvals(none(), item_owner.clone(), collection_id, 0), + BadOrigin.with_weight(WeightOf::clear_collection_approvals(0)) + ); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), item_owner.clone(), @@ -4865,7 +4867,7 @@ fn cancel_collection_approval_works() { collection_id, delegate.clone() ), - Error::::Unapproved + Error::::NotDelegate ); assert_ok!(Nfts::approve_collection_transfer( RuntimeOrigin::signed(item_owner.clone()), @@ -4880,7 +4882,7 @@ fn cancel_collection_approval_works() { collection_id, account(69) ), - Error::::Unapproved + Error::::NotDelegate ); // Successfully cancel a collection approval. assert_ok!(Nfts::cancel_collection_approval( @@ -4911,17 +4913,24 @@ fn force_cancel_collection_approval_works() { let item_id = 42; let item_owner = account(2); - 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 - ); - } + assert_noop!( + Nfts::force_cancel_collection_approval( + RuntimeOrigin::signed(item_owner.clone()), + item_owner.clone(), + collection_id, + delegate.clone() + ), + Error::::NoPermission + ); + assert_noop!( + Nfts::force_cancel_collection_approval( + none(), + item_owner.clone(), + collection_id, + delegate.clone() + ), + BadOrigin + ); Balances::make_free_balance_be(&item_owner, 100); assert_ok!(Nfts::force_create( RuntimeOrigin::root(), @@ -4943,7 +4952,7 @@ fn force_cancel_collection_approval_works() { collection_id, delegate.clone() ), - Error::::Unapproved + Error::::NotDelegate ); assert_ok!(Nfts::approve_collection_transfer( RuntimeOrigin::signed(item_owner.clone()), @@ -4959,7 +4968,7 @@ fn force_cancel_collection_approval_works() { collection_id, account(69) ), - Error::::Unapproved + Error::::NotDelegate ); // Successfully cancel a collection approval. assert_ok!(Nfts::force_cancel_collection_approval( @@ -4983,7 +4992,7 @@ fn force_cancel_collection_approval_works() { } #[test] -fn check_approval_without_deadline_works() { +fn check_approval_permission_without_deadline_works() { new_test_ext().execute_with(|| { let collection_id = 0; let collection_owner = account(1); @@ -4999,11 +5008,11 @@ fn check_approval_without_deadline_works() { )); // Item doesn't exist. assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::NoPermission ); assert_noop!( - Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &Some(item_id), &item_owner, &delegate), Error::::UnknownItem ); assert_ok!(Nfts::force_mint( @@ -5015,11 +5024,11 @@ fn check_approval_without_deadline_works() { )); // No approval. assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::NoPermission ); assert_noop!( - Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &Some(item_id), &item_owner, &delegate), Error::::NoPermission ); @@ -5030,8 +5039,13 @@ fn check_approval_without_deadline_works() { delegate.clone(), None )); - assert_ok!(Nfts::check_approval(&collection_id, &None, &item_owner, &delegate)); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); assert_ok!(Nfts::cancel_collection_approval( RuntimeOrigin::signed(item_owner.clone()), collection_id, @@ -5047,10 +5061,15 @@ fn check_approval_without_deadline_works() { None )); assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::NoPermission ); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); assert_ok!(Nfts::cancel_approval( RuntimeOrigin::signed(item_owner.clone()), collection_id, @@ -5072,13 +5091,18 @@ fn check_approval_without_deadline_works() { delegate.clone(), None )); - assert_ok!(Nfts::check_approval(&collection_id, &None, &item_owner, &delegate)); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); }); } #[test] -fn check_approval_with_deadline_works() { +fn check_approval_permission_with_deadline_works() { new_test_ext().execute_with(|| { let collection_id = 0; let collection_owner = account(1); @@ -5108,16 +5132,21 @@ fn check_approval_with_deadline_works() { delegate.clone(), Some(deadline), )); - assert_ok!(Nfts::check_approval(&collection_id, &None, &item_owner, &delegate)); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); // Expire approval. System::set_block_number(deadline + System::block_number() + 1); assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::ApprovalExpired ); assert_noop!( - Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &Some(item_id), &item_owner, &delegate), Error::::NoPermission ); assert_ok!(Nfts::cancel_collection_approval( @@ -5136,18 +5165,23 @@ fn check_approval_with_deadline_works() { Some(deadline), )); assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::NoPermission ); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); // Expire approval. System::set_block_number(deadline + System::block_number() + 1); assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::NoPermission ); assert_noop!( - Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &Some(item_id), &item_owner, &delegate), Error::::ApprovalExpired ); assert_ok!(Nfts::cancel_approval( @@ -5172,16 +5206,21 @@ fn check_approval_with_deadline_works() { delegate.clone(), Some(deadline) )); - assert_ok!(Nfts::check_approval(&collection_id, &None, &item_owner, &delegate)); - assert_ok!(Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate)); + assert_ok!(Nfts::check_approval_permission( + &collection_id, + &Some(item_id), + &item_owner, + &delegate + )); // Expire approvals. System::set_block_number(deadline + System::block_number() + 1); assert_noop!( - Nfts::check_approval(&collection_id, &None, &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &None, &item_owner, &delegate), Error::::ApprovalExpired ); assert_noop!( - Nfts::check_approval(&collection_id, &Some(item_id), &item_owner, &delegate), + Nfts::check_approval_permission(&collection_id, &Some(item_id), &item_owner, &delegate), Error::::ApprovalExpired ); });