From bc3844591c7ec0614446a4bbb174170a9b4aa098 Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko Date: Thu, 7 Dec 2023 17:02:42 +0200 Subject: [PATCH] Fix consumers issue --- substrate/frame/nfts/src/features/transfer.rs | 17 ++++++++------- substrate/frame/nfts/src/lib.rs | 6 +++--- substrate/frame/nfts/src/tests.rs | 5 +++++ substrate/frame/uniques/src/lib.rs | 21 +++++++++++-------- substrate/frame/uniques/src/tests.rs | 3 +++ 5 files changed, 32 insertions(+), 20 deletions(-) diff --git a/substrate/frame/nfts/src/features/transfer.rs b/substrate/frame/nfts/src/features/transfer.rs index 0471bd67b291..ac73d283ee0e 100644 --- a/substrate/frame/nfts/src/features/transfer.rs +++ b/substrate/frame/nfts/src/features/transfer.rs @@ -124,10 +124,10 @@ impl, I: 'static> Pallet { pub(crate) fn do_transfer_ownership( origin: T::AccountId, collection: T::CollectionId, - owner: T::AccountId, + new_owner: T::AccountId, ) -> DispatchResult { // Check if the new owner is acceptable based on the collection's acceptance settings. - let acceptable_collection = OwnershipAcceptance::::get(&owner); + let acceptable_collection = OwnershipAcceptance::::get(&new_owner); ensure!(acceptable_collection.as_ref() == Some(&collection), Error::::Unaccepted); // Try to retrieve and mutate the collection details. @@ -135,27 +135,28 @@ impl, I: 'static> Pallet { let details = maybe_details.as_mut().ok_or(Error::::UnknownCollection)?; // Check if the `origin` is the current owner of the collection. ensure!(origin == details.owner, Error::::NoPermission); - if details.owner == owner { + if details.owner == new_owner { return Ok(()) } // Move the deposit to the new owner. T::Currency::repatriate_reserved( &details.owner, - &owner, + &new_owner, details.owner_deposit, Reserved, )?; // Update account ownership information. CollectionAccount::::remove(&details.owner, &collection); - CollectionAccount::::insert(&owner, &collection, ()); + CollectionAccount::::insert(&new_owner, &collection, ()); - details.owner = owner.clone(); - OwnershipAcceptance::::remove(&owner); + details.owner = new_owner.clone(); + OwnershipAcceptance::::remove(&new_owner); + frame_system::Pallet::::dec_consumers(&new_owner); // Emit `OwnerChanged` event. - Self::deposit_event(Event::OwnerChanged { collection, new_owner: owner }); + Self::deposit_event(Event::OwnerChanged { collection, new_owner }); Ok(()) }) } diff --git a/substrate/frame/nfts/src/lib.rs b/substrate/frame/nfts/src/lib.rs index 92b27432ab21..a7d505e2e397 100644 --- a/substrate/frame/nfts/src/lib.rs +++ b/substrate/frame/nfts/src/lib.rs @@ -1153,11 +1153,11 @@ pub mod pallet { pub fn transfer_ownership( origin: OriginFor, collection: T::CollectionId, - owner: AccountIdLookupOf, + new_owner: AccountIdLookupOf, ) -> DispatchResult { let origin = ensure_signed(origin)?; - let owner = T::Lookup::lookup(owner)?; - Self::do_transfer_ownership(origin, collection, owner) + let new_owner = T::Lookup::lookup(new_owner)?; + Self::do_transfer_ownership(origin, collection, new_owner) } /// Change the Issuer, Admin and Freezer of a collection. diff --git a/substrate/frame/nfts/src/tests.rs b/substrate/frame/nfts/src/tests.rs index aeebf51b7c78..9d725e3fd4c3 100644 --- a/substrate/frame/nfts/src/tests.rs +++ b/substrate/frame/nfts/src/tests.rs @@ -614,8 +614,13 @@ fn transfer_owner_should_work() { Nfts::transfer_ownership(RuntimeOrigin::signed(account(1)), 0, account(2)), Error::::Unaccepted ); + assert_eq!(System::consumers(&account(2)), 0); + assert_ok!(Nfts::set_accept_ownership(RuntimeOrigin::signed(account(2)), Some(0))); + assert_eq!(System::consumers(&account(2)), 1); + assert_ok!(Nfts::transfer_ownership(RuntimeOrigin::signed(account(1)), 0, account(2))); + assert_eq!(System::consumers(&account(2)), 1); // one consumer is added due to deposit repatriation assert_eq!(collections(), vec![(account(2), 0)]); assert_eq!(Balances::total_balance(&account(1)), 98); diff --git a/substrate/frame/uniques/src/lib.rs b/substrate/frame/uniques/src/lib.rs index 8334a8d943e1..740068d99f8d 100644 --- a/substrate/frame/uniques/src/lib.rs +++ b/substrate/frame/uniques/src/lib.rs @@ -856,34 +856,37 @@ pub mod pallet { pub fn transfer_ownership( origin: OriginFor, collection: T::CollectionId, - owner: AccountIdLookupOf, + new_owner: AccountIdLookupOf, ) -> DispatchResult { let origin = ensure_signed(origin)?; - let owner = T::Lookup::lookup(owner)?; + let new_owner = T::Lookup::lookup(new_owner)?; - let acceptable_collection = OwnershipAcceptance::::get(&owner); + let acceptable_collection = OwnershipAcceptance::::get(&new_owner); ensure!(acceptable_collection.as_ref() == Some(&collection), Error::::Unaccepted); Collection::::try_mutate(collection.clone(), |maybe_details| { let details = maybe_details.as_mut().ok_or(Error::::UnknownCollection)?; ensure!(origin == details.owner, Error::::NoPermission); - if details.owner == owner { + if details.owner == new_owner { return Ok(()) } // Move the deposit to the new owner. T::Currency::repatriate_reserved( &details.owner, - &owner, + &new_owner, details.total_deposit, Reserved, )?; + CollectionAccount::::remove(&details.owner, &collection); - CollectionAccount::::insert(&owner, &collection, ()); - details.owner = owner.clone(); - OwnershipAcceptance::::remove(&owner); + CollectionAccount::::insert(&new_owner, &collection, ()); + + details.owner = new_owner.clone(); + OwnershipAcceptance::::remove(&new_owner); + frame_system::Pallet::::dec_consumers(&new_owner); - Self::deposit_event(Event::OwnerChanged { collection, new_owner: owner }); + Self::deposit_event(Event::OwnerChanged { collection, new_owner }); Ok(()) }) } diff --git a/substrate/frame/uniques/src/tests.rs b/substrate/frame/uniques/src/tests.rs index 52f7df3b5efb..351dac09f7f2 100644 --- a/substrate/frame/uniques/src/tests.rs +++ b/substrate/frame/uniques/src/tests.rs @@ -254,8 +254,11 @@ fn transfer_owner_should_work() { Uniques::transfer_ownership(RuntimeOrigin::signed(1), 0, 2), Error::::Unaccepted ); + assert_eq!(System::consumers(&2), 0); assert_ok!(Uniques::set_accept_ownership(RuntimeOrigin::signed(2), Some(0))); + assert_eq!(System::consumers(&2), 1); assert_ok!(Uniques::transfer_ownership(RuntimeOrigin::signed(1), 0, 2)); + assert_eq!(System::consumers(&2), 1); assert_eq!(collections(), vec![(2, 0)]); assert_eq!(Balances::total_balance(&1), 98);