[NFTs] Fix consumers issue (paritytech#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

@@ -124,38 +124,39 @@ 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.
 		Collection::<T, I>::try_mutate(collection, |maybe_details| {
 			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(())
 		})
 	}

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.

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);

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(())
 			})
 		}

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);