From cc432acc83f3e3bf993b372f785d0fd0ff2a9495 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 10:48:50 +0400 Subject: [PATCH 1/6] fix uniques total_deposit not decrementing on clear collection --- substrate/frame/uniques/src/lib.rs | 4 ++- substrate/frame/uniques/src/tests.rs | 41 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/substrate/frame/uniques/src/lib.rs b/substrate/frame/uniques/src/lib.rs index f7cc6b044d729..2291d19de2bfb 100644 --- a/substrate/frame/uniques/src/lib.rs +++ b/substrate/frame/uniques/src/lib.rs @@ -1399,7 +1399,7 @@ pub mod pallet { .map(|_| None) .or_else(|origin| ensure_signed(origin).map(Some))?; - let details = + let mut details = Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; if let Some(check_owner) = &maybe_check_owner { ensure!(check_owner == &details.owner, Error::::NoPermission); @@ -1411,6 +1411,8 @@ pub mod pallet { let deposit = metadata.take().ok_or(Error::::UnknownCollection)?.deposit; T::Currency::unreserve(&details.owner, deposit); + details.total_deposit.saturating_reduce(deposit); + Collection::::insert(&collection, details); Self::deposit_event(Event::CollectionMetadataCleared { collection }); Ok(()) }) diff --git a/substrate/frame/uniques/src/tests.rs b/substrate/frame/uniques/src/tests.rs index afd0352bf90eb..f5cbf82bf8f28 100644 --- a/substrate/frame/uniques/src/tests.rs +++ b/substrate/frame/uniques/src/tests.rs @@ -1062,3 +1062,44 @@ fn buy_item_should_work() { } }); } + +#[test] +fn clear_collection_metadata_works() { + new_test_ext().execute_with(|| { + // Start with an account with 100 balance, 10 of which are reserved + Balances::make_free_balance_be(&1, 100); + Balances::reserve(&1, 10).unwrap(); + + // Create a Unique which increases reserved balance by 2. + assert_ok!(Uniques::create(RuntimeOrigin::signed(1), 0, 123)); + assert_eq!(Balances::reserved_balance(&1), 12); + + // Set collection metadata which increases reserved balance by 10. + assert_ok!(Uniques::set_collection_metadata( + RuntimeOrigin::signed(1), + 0, + bvec![0, 0, 0, 0, 0, 0, 0, 0, 0], + false + )); + + assert_eq!(Balances::reserved_balance(&1), 22); + + // The total deposit of the collection is now 12 + assert_eq!(Collection::::get(0).unwrap().total_deposit, 12); + + assert_ok!(Uniques::clear_collection_metadata(RuntimeOrigin::signed(1), 0)); + // Reserved balance and total_deposit decreased by the collection metadata deposit (-10) + assert_eq!(Balances::reserved_balance(&1), 12); + assert_eq!(Collection::::get(0).unwrap().total_deposit, 2); + + // Destroy the collection and get the final 2 back + assert_ok!(Uniques::destroy( + RuntimeOrigin::signed(1), + 0, + DestroyWitness { items: 0, item_metadatas: 0, attributes: 0 } + )); + + // Reserved balance back to the original 10 + assert_eq!(Balances::reserved_balance(&1), 10); + }); +} From dc391c8567f9fe0987d59aca5efd7fcda9bdbdcf Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 10:59:05 +0400 Subject: [PATCH 2/6] fix nft metadata not clearing correctly --- substrate/frame/nfts/src/features/metadata.rs | 4 +- substrate/frame/nfts/src/tests.rs | 41 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nfts/src/features/metadata.rs b/substrate/frame/nfts/src/features/metadata.rs index e177f39bb8b81..85edd294d50b7 100644 --- a/substrate/frame/nfts/src/features/metadata.rs +++ b/substrate/frame/nfts/src/features/metadata.rs @@ -247,7 +247,7 @@ impl, I: 'static> Pallet { ); } - let details = + let mut details = Collection::::get(&collection).ok_or(Error::::UnknownCollection)?; let collection_config = Self::get_collection_config(&collection)?; @@ -260,6 +260,8 @@ impl, I: 'static> Pallet { CollectionMetadataOf::::try_mutate_exists(collection, |metadata| { let deposit = metadata.take().ok_or(Error::::UnknownCollection)?.deposit; T::Currency::unreserve(&details.owner, deposit); + details.owner_deposit.saturating_reduce(deposit); + Collection::::insert(&collection, details); Self::deposit_event(Event::CollectionMetadataCleared { collection }); Ok(()) }) diff --git a/substrate/frame/nfts/src/tests.rs b/substrate/frame/nfts/src/tests.rs index 6bf9427f4e6cd..0599d538868f5 100644 --- a/substrate/frame/nfts/src/tests.rs +++ b/substrate/frame/nfts/src/tests.rs @@ -3835,3 +3835,44 @@ fn basic_create_collection_with_id_should_work() { ); }); } + +#[test] +fn clear_collection_metadata_works() { + new_test_ext().execute_with(|| { + // Start with an account with 100 tokens, 10 of which are reserved + Balances::make_free_balance_be(&account(1), 100); + Balances::reserve(&account(1), 10).unwrap(); + + // Creating a collection increases owner deposit by 2 + assert_ok!(Nfts::create( + RuntimeOrigin::signed(account(1)), + account(1), + collection_config_with_all_settings_enabled() + )); + assert_eq!(Collection::::get(0).unwrap().owner_deposit, 2); + assert_eq!(Balances::reserved_balance(&account(1)), 12); + + // Setting collection metadata increases owner deposit by 10 + assert_ok!(Nfts::set_collection_metadata( + RuntimeOrigin::signed(account(1)), + 0, + bvec![0, 0, 0, 0, 0, 0, 0, 0, 0], + )); + assert_eq!(Collection::::get(0).unwrap().owner_deposit, 12); + assert_eq!(Balances::reserved_balance(&account(1)), 22); + + // Clearing collection metadata decreases owner deposit by 10 + assert_ok!(Nfts::clear_collection_metadata(RuntimeOrigin::signed(account(1)), 0)); + // Reserved balance decreased by the collection metadata deposit (-10) + assert_eq!(Collection::::get(0).unwrap().owner_deposit, 2); + assert_eq!(Balances::reserved_balance(&account(1)), 12); + + assert_ok!(Nfts::destroy( + // Destroying the collection decreases reserved balance back to the original + RuntimeOrigin::signed(account(1)), + 0, + DestroyWitness { item_configs: 0, item_metadatas: 0, attributes: 0 } + )); + assert_eq!(Balances::reserved_balance(&account(1)), 10); + }); +} From 705db689a85dbf44c849514b70f8bc52514be3ee Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 10:59:13 +0400 Subject: [PATCH 3/6] clean up test --- substrate/frame/uniques/src/tests.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/substrate/frame/uniques/src/tests.rs b/substrate/frame/uniques/src/tests.rs index f5cbf82bf8f28..fc3e72524c39e 100644 --- a/substrate/frame/uniques/src/tests.rs +++ b/substrate/frame/uniques/src/tests.rs @@ -1070,36 +1070,32 @@ fn clear_collection_metadata_works() { Balances::make_free_balance_be(&1, 100); Balances::reserve(&1, 10).unwrap(); - // Create a Unique which increases reserved balance by 2. + // Create a Unique which increases total_deposit by 2 assert_ok!(Uniques::create(RuntimeOrigin::signed(1), 0, 123)); + assert_eq!(Collection::::get(0).unwrap().total_deposit, 2); assert_eq!(Balances::reserved_balance(&1), 12); - // Set collection metadata which increases reserved balance by 10. + // Set collection metadata which increases total_deposit by 10 assert_ok!(Uniques::set_collection_metadata( RuntimeOrigin::signed(1), 0, bvec![0, 0, 0, 0, 0, 0, 0, 0, 0], false )); - - assert_eq!(Balances::reserved_balance(&1), 22); - - // The total deposit of the collection is now 12 assert_eq!(Collection::::get(0).unwrap().total_deposit, 12); + assert_eq!(Balances::reserved_balance(&1), 22); + // Clearing collection metadata reduces total_deposit by the expected amount assert_ok!(Uniques::clear_collection_metadata(RuntimeOrigin::signed(1), 0)); - // Reserved balance and total_deposit decreased by the collection metadata deposit (-10) - assert_eq!(Balances::reserved_balance(&1), 12); assert_eq!(Collection::::get(0).unwrap().total_deposit, 2); + assert_eq!(Balances::reserved_balance(&1), 12); - // Destroy the collection and get the final 2 back + // Destroying the collection reduces the reserved balance by the remaining total_deposit assert_ok!(Uniques::destroy( RuntimeOrigin::signed(1), 0, DestroyWitness { items: 0, item_metadatas: 0, attributes: 0 } )); - - // Reserved balance back to the original 10 assert_eq!(Balances::reserved_balance(&1), 10); }); } From e748dabcc763013f1be36620129b09e97190cb69 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 11:03:05 +0400 Subject: [PATCH 4/6] check storage is cleaned up --- substrate/frame/nfts/src/tests.rs | 3 ++- substrate/frame/uniques/src/tests.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nfts/src/tests.rs b/substrate/frame/nfts/src/tests.rs index 0599d538868f5..04458332932c2 100644 --- a/substrate/frame/nfts/src/tests.rs +++ b/substrate/frame/nfts/src/tests.rs @@ -3867,12 +3867,13 @@ fn clear_collection_metadata_works() { assert_eq!(Collection::::get(0).unwrap().owner_deposit, 2); assert_eq!(Balances::reserved_balance(&account(1)), 12); + // Destroying the collection removes it from storage assert_ok!(Nfts::destroy( - // Destroying the collection decreases reserved balance back to the original RuntimeOrigin::signed(account(1)), 0, DestroyWitness { item_configs: 0, item_metadatas: 0, attributes: 0 } )); + assert_eq!(Collection::::get(0), None); assert_eq!(Balances::reserved_balance(&account(1)), 10); }); } diff --git a/substrate/frame/uniques/src/tests.rs b/substrate/frame/uniques/src/tests.rs index fc3e72524c39e..5dfe43c96888d 100644 --- a/substrate/frame/uniques/src/tests.rs +++ b/substrate/frame/uniques/src/tests.rs @@ -1090,12 +1090,13 @@ fn clear_collection_metadata_works() { assert_eq!(Collection::::get(0).unwrap().total_deposit, 2); assert_eq!(Balances::reserved_balance(&1), 12); - // Destroying the collection reduces the reserved balance by the remaining total_deposit + // Destroying the collection removes it from storage assert_ok!(Uniques::destroy( RuntimeOrigin::signed(1), 0, DestroyWitness { items: 0, item_metadatas: 0, attributes: 0 } )); + assert_eq!(Collection::::get(0), None); assert_eq!(Balances::reserved_balance(&1), 10); }); } From 686e505dd97d60ddd4e45358929fac95ac31e1d5 Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 11:11:03 +0400 Subject: [PATCH 5/6] prdoc --- prdoc/pr_3976.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_3976.prdoc diff --git a/prdoc/pr_3976.prdoc b/prdoc/pr_3976.prdoc new file mode 100644 index 0000000000000..95afab6d42406 --- /dev/null +++ b/prdoc/pr_3976.prdoc @@ -0,0 +1,14 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Decrement total_deposit when clearing collection metadata + +doc: + - audience: Runtime Dev + description: Decrements total_deposit by the appropriate amount when collection metadata is cleared. + +crates: + - name: pallet-uniques + bump: patch + - name: pallet-nfts + bump: patch From afcc99bc6332db1742336b1a7dbbf68c3f0eb2db Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 4 Apr 2024 21:02:16 +0400 Subject: [PATCH 6/6] remove redundant comment --- substrate/frame/nfts/src/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/nfts/src/tests.rs b/substrate/frame/nfts/src/tests.rs index 04458332932c2..4d23aca64ceb1 100644 --- a/substrate/frame/nfts/src/tests.rs +++ b/substrate/frame/nfts/src/tests.rs @@ -3863,7 +3863,6 @@ fn clear_collection_metadata_works() { // Clearing collection metadata decreases owner deposit by 10 assert_ok!(Nfts::clear_collection_metadata(RuntimeOrigin::signed(account(1)), 0)); - // Reserved balance decreased by the collection metadata deposit (-10) assert_eq!(Collection::::get(0).unwrap().owner_deposit, 2); assert_eq!(Balances::reserved_balance(&account(1)), 12);