From 166ae5ae1295c2431c83421439c3869d4f40de7e Mon Sep 17 00:00:00 2001
From: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>
Date: Tue, 19 Dec 2023 15:23:58 +0200
Subject: [PATCH] [NFTs] Fix consumers issue (#2653)
When we call the `set_accept_ownership` method, we increase the number
of account consumers, but then we don't decrease it on collection
transfer, which leads to the wrong consumers number an account has.
---
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 0471bd67b29..ac73d283ee0 100644
--- a/substrate/frame/nfts/src/features/transfer.rs
+++ b/substrate/frame/nfts/src/features/transfer.rs
@@ -124,10 +124,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
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::<T, I>::get(&owner);
+ let acceptable_collection = OwnershipAcceptance::<T, I>::get(&new_owner);
ensure!(acceptable_collection.as_ref() == Some(&collection), Error::<T, I>::Unaccepted);
// Try to retrieve and mutate the collection details.
@@ -135,27 +135,28 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
// Check if the `origin` is the current owner of the collection.
ensure!(origin == details.owner, Error::<T, I>::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::<T, I>::remove(&details.owner, &collection);
- CollectionAccount::<T, I>::insert(&owner, &collection, ());
+ CollectionAccount::<T, I>::insert(&new_owner, &collection, ());
- details.owner = owner.clone();
- OwnershipAcceptance::<T, I>::remove(&owner);
+ details.owner = new_owner.clone();
+ OwnershipAcceptance::<T, I>::remove(&new_owner);
+ frame_system::Pallet::<T>::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 92b27432ab2..a7d505e2e39 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<T>,
collection: T::CollectionId,
- owner: AccountIdLookupOf<T>,
+ new_owner: AccountIdLookupOf<T>,
) -> 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 0c32aea2be0..9e521537534 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::<Test>::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 63a6e83de07..253a318e474 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<T>,
collection: T::CollectionId,
- owner: AccountIdLookupOf<T>,
+ new_owner: AccountIdLookupOf<T>,
) -> DispatchResult {
let origin = ensure_signed(origin)?;
- let owner = T::Lookup::lookup(owner)?;
+ let new_owner = T::Lookup::lookup(new_owner)?;
- let acceptable_collection = OwnershipAcceptance::<T, I>::get(&owner);
+ let acceptable_collection = OwnershipAcceptance::<T, I>::get(&new_owner);
ensure!(acceptable_collection.as_ref() == Some(&collection), Error::<T, I>::Unaccepted);
Collection::<T, I>::try_mutate(collection.clone(), |maybe_details| {
let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
ensure!(origin == details.owner, Error::<T, I>::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::<T, I>::remove(&details.owner, &collection);
- CollectionAccount::<T, I>::insert(&owner, &collection, ());
- details.owner = owner.clone();
- OwnershipAcceptance::<T, I>::remove(&owner);
+ CollectionAccount::<T, I>::insert(&new_owner, &collection, ());
+
+ details.owner = new_owner.clone();
+ OwnershipAcceptance::<T, I>::remove(&new_owner);
+ frame_system::Pallet::<T>::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 52f7df3b5ef..351dac09f7f 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::<Test>::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);
--
GitLab