Skip to content

Commit

Permalink
fix: remove unused storage fields from CollectionDetails
Browse files Browse the repository at this point in the history
  • Loading branch information
@chungquantin
chungquantin committed Nov 22, 2024
1 parent f815bf2 commit 2226f61
Show file tree
Hide file tree
Showing 8 changed files with 283 additions and 362 deletions.
5 changes: 0 additions & 5 deletions pallets/nfts/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ benchmarks_instance_pallet! {
let m in 0 .. 1_000;
let c in 0 .. 1_000;
let a in 0 .. 1_000;
let h in 0 .. 1_000;
let l in 0 .. 1_000;

let (collection, caller, _) = create_collection::<T, I>();
Expand All @@ -288,10 +287,6 @@ benchmarks_instance_pallet! {
for i in 0..a {
add_collection_attribute::<T, I>(i as u16);
}
for i in 0..h {
mint_item::<T, I>(i as u16);
burn_item::<T, I>(i as u16);
}
for i in 0..l {
approve_collection::<T, I>(i);
}
Expand Down
10 changes: 1 addition & 9 deletions pallets/nfts/src/features/create_delete_collection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
items: 0,
item_metadatas: 0,
item_configs: 0,
item_holders: 0,
attributes: 0,
allowances: 0,
},
Expand Down Expand Up @@ -112,6 +111,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
ensure!(collection_details.owner == check_owner, Error::<T, I>::NoPermission);
}
ensure!(collection_details.items == 0, Error::<T, I>::CollectionNotEmpty);
ensure!(collection_details.allowances == 0, Error::<T, I>::AllowancesNotEmpty);
ensure!(collection_details.attributes == witness.attributes, Error::<T, I>::BadWitness);
ensure!(
collection_details.item_metadatas == witness.item_metadatas,
Expand All @@ -121,10 +121,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
collection_details.item_configs == witness.item_configs,
Error::<T, I>::BadWitness
);
ensure!(
collection_details.item_holders == witness.item_holders,
Error::<T, I>::BadWitness
);
ensure!(collection_details.allowances == witness.allowances, Error::<T, I>::BadWitness);

for (_, metadata) in ItemMetadataOf::<T, I>::drain_prefix(collection) {
Expand All @@ -144,9 +140,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}
}

let _ =
AccountBalance::<T, I>::clear_prefix(collection, collection_details.items, None);
let _ = Allowances::<T, I>::clear_prefix((collection,), collection_details.items, None);
CollectionAccount::<T, I>::remove(&collection_details.owner, &collection);

Check warning on line 143 in pallets/nfts/src/features/create_delete_collection.rs

View workflow job for this annotation

GitHub Actions / clippy

the borrowed expression implements the required traits 

warning: the borrowed expression implements the required traits
   --> pallets/nfts/src/features/create_delete_collection.rs:143:65
    |
143 |             CollectionAccount::<T, I>::remove(&collection_details.owner, &collection);
    |                                                                          ^^^^^^^^^^^ help: change this to: `collection`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
T::Currency::unreserve(&collection_details.owner, collection_details.owner_deposit);
CollectionConfigOf::<T, I>::remove(collection);
Expand All @@ -157,7 +150,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Ok(DestroyWitness {
item_metadatas: collection_details.item_metadatas,
item_configs: collection_details.item_configs,
item_holders: collection_details.item_holders,
attributes: collection_details.attributes,
allowances: collection_details.allowances,
})
Expand Down
15 changes: 3 additions & 12 deletions pallets/nfts/src/features/create_delete_item.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

collection_details.items.saturating_inc();

let account_balance =
AccountBalance::<T, I>::mutate(collection, &mint_to, |balance| -> u32 {
balance.saturating_inc();
*balance
});
if account_balance == 1 {
collection_details.item_holders.saturating_inc();
}
AccountBalance::<T, I>::mutate(collection, &mint_to, |balance| {
balance.saturating_inc();
});

let collection_config = Self::get_collection_config(&collection)?;
let deposit_amount =
Expand Down Expand Up @@ -259,10 +254,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}
}

if AccountBalance::<T, I>::get(collection, &details.owner) == 1 {
collection_details.item_holders.saturating_dec();
}

Ok(details.owner)
},
)?;
Expand Down
19 changes: 5 additions & 14 deletions pallets/nfts/src/features/transfer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
) -> DispatchResult,
) -> DispatchResult {
// Retrieve collection details.
let mut collection_details =
let collection_details =
Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;

Check warning on line 58 in pallets/nfts/src/features/transfer.rs

View workflow job for this annotation

GitHub Actions / clippy

the borrowed expression implements the required traits 

warning: the borrowed expression implements the required traits
  --> pallets/nfts/src/features/transfer.rs:58:28
   |
58 |             Collection::<T, I>::get(&collection).ok_or(Error::<T, I>::UnknownCollection)?;
   |                                     ^^^^^^^^^^^ help: change this to: `collection`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

// Ensure the item is not locked.
Expand Down Expand Up @@ -87,22 +87,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
with_details(&collection_details, &mut details)?;

// Update account balance of the owner.
let owner_balance =
AccountBalance::<T, I>::mutate(collection, &details.owner, |balance| -> u32 {
balance.saturating_dec();
*balance
});
if owner_balance == 0 {
collection_details.item_holders.saturating_dec();
}
AccountBalance::<T, I>::mutate(collection, &details.owner, |balance| {
balance.saturating_dec();
});
// Update account balance of the destination account.
let dest_balance = AccountBalance::<T, I>::mutate(collection, &dest, |balance| -> u32 {
AccountBalance::<T, I>::mutate(collection, &dest, |balance| {
balance.saturating_inc();
*balance
});
if dest_balance == 1 {
collection_details.item_holders.saturating_inc();
}

// Update account ownership information.
Account::<T, I>::remove((&details.owner, &collection, &item));
Expand Down
6 changes: 2 additions & 4 deletions pallets/nfts/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,8 @@ pub mod pallet {
CollectionNotEmpty,
/// The witness data should be provided.
WitnessRequired,
/// Cant' delete collections whose allowances.
AllowancesNotEmpty,
}

#[pallet::call]
Expand Down Expand Up @@ -835,8 +837,6 @@ pub mod pallet {
witness.item_metadatas,
witness.item_configs,
witness.attributes,
witness.item_holders,
witness.allowances,
))]
pub fn destroy(
origin: OriginFor<T>,
Expand All @@ -852,8 +852,6 @@ pub mod pallet {
details.item_metadatas,
details.item_configs,
details.attributes,
details.item_holders,
details.allowances,
))
.into())
}
Expand Down
58 changes: 11 additions & 47 deletions pallets/nfts/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ fn basic_minting_should_work() {
assert_eq!(collections(), vec![(account(1), 0)]);
assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, 42, account(1), None));
assert_eq!(AccountBalance::<Test>::get(0, account(1)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_eq!(items(), vec![(account(1), 0, 42)]);

assert_ok!(Nfts::force_create(
Expand All @@ -176,37 +175,10 @@ fn basic_minting_should_work() {
assert_eq!(collections(), vec![(account(1), 0), (account(2), 1)]);
assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(2)), 1, 69, account(1), None));
assert_eq!(AccountBalance::<Test>::get(1, account(1)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_eq!(items(), vec![(account(1), 0, 42), (account(1), 1, 69)]);
});
}

#[test]
fn collection_item_holders_should_works() {
new_test_ext().execute_with(|| {
assert_ok!(Nfts::force_create(
RuntimeOrigin::root(),
account(1),
default_collection_config()
));
assert_eq!(collections(), vec![(account(1), 0)]);
let total = 5;
for i in 0..total {
assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, i, account(1), None));
}
assert_eq!(AccountBalance::<Test>::get(0, account(1)), total);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);

assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, total, account(2), None));
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 2);

assert_ok!(Nfts::burn(RuntimeOrigin::signed(account(2)), 0, total));
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 0);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
});
}

#[test]
fn lifecycle_should_work() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -251,7 +223,6 @@ fn lifecycle_should_work() {
assert_eq!(Collection::<Test>::get(0).unwrap().items, 3);
assert_eq!(Collection::<Test>::get(0).unwrap().item_metadatas, 0);
assert_eq!(Collection::<Test>::get(0).unwrap().item_configs, 3);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 3);

assert_eq!(Balances::reserved_balance(&account(1)), 8);
assert_ok!(Nfts::transfer(RuntimeOrigin::signed(account(1)), 0, 70, account(2)));
Expand Down Expand Up @@ -346,7 +317,6 @@ fn destroy_should_work() {

assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, 42, account(2), None));
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_ok!(Nfts::approve_transfer(
RuntimeOrigin::signed(account(1)),
0,
Expand All @@ -367,12 +337,21 @@ fn destroy_should_work() {
assert_eq!(Collection::<Test>::get(0).unwrap().item_configs, 1);
assert_eq!(ItemConfigOf::<Test>::iter_prefix(0).count() as u32, 1);
assert!(ItemConfigOf::<Test>::contains_key(0, 42));
assert_noop!(
Nfts::destroy(
RuntimeOrigin::signed(account(1)),
0,
Nfts::get_destroy_witness(&0).unwrap()
),
Error::<Test>::AllowancesNotEmpty
);
assert_ok!(Nfts::cancel_approval(RuntimeOrigin::signed(account(1)), 0, None, account(3)));
assert_ok!(Nfts::destroy(
RuntimeOrigin::signed(account(1)),
0,
Nfts::get_destroy_witness(&0).unwrap()
));
assert_eq!(AccountBalance::<Test>::iter_prefix(0).count(), 0);
assert_eq!(AccountBalance::<Test>::get(0, account(1)), 0);
assert_eq!(Allowances::<Test>::iter_prefix((0,)).count(), 0);
assert!(!ItemConfigOf::<Test>::contains_key(0, 42));
assert_eq!(ItemConfigOf::<Test>::iter_prefix(0).count() as u32, 0);
Expand All @@ -390,7 +369,6 @@ fn mint_should_work() {
assert_ok!(Nfts::mint(RuntimeOrigin::signed(account(1)), 0, 42, account(1), None));
assert_eq!(AccountBalance::<Test>::get(0, account(1)), 1);
assert_eq!(Nfts::owner(0, 42).unwrap(), account(1));
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_eq!(collections(), vec![(account(1), 0)]);
assert_eq!(items(), vec![(account(1), 0, 42)]);

Expand Down Expand Up @@ -456,7 +434,6 @@ fn mint_should_work() {
Some(MintWitness { mint_price: Some(1), ..Default::default() })
));
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 2);
assert_eq!(Balances::total_balance(&account(2)), 99);

// validate types
Expand Down Expand Up @@ -496,7 +473,6 @@ fn mint_should_work() {
Some(MintWitness { owned_item: Some(43), ..Default::default() })
));
assert_eq!(AccountBalance::<Test>::get(1, account(2)), 1);
assert_eq!(Collection::<Test>::get(1).unwrap().item_holders, 1);
assert!(events().contains(&Event::<Test>::PalletAttributeSet {
collection: 0,
item: Some(43),
Expand Down Expand Up @@ -533,11 +509,9 @@ fn transfer_should_work() {
account(2),
default_item_config()
));
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_ok!(Nfts::transfer(RuntimeOrigin::signed(account(2)), 0, 42, account(3)));
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 0);
assert_eq!(AccountBalance::<Test>::get(0, account(3)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_eq!(items(), vec![(account(3), 0, 42)]);
assert_noop!(
Nfts::transfer(RuntimeOrigin::signed(account(2)), 0, 42, account(4)),
Expand All @@ -555,7 +529,6 @@ fn transfer_should_work() {
assert_eq!(AccountBalance::<Test>::get(0, account(2)), 0);
assert_eq!(AccountBalance::<Test>::get(0, account(3)), 0);
assert_eq!(AccountBalance::<Test>::get(0, account(4)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
// validate we can't transfer non-transferable items
let collection_id = 1;
assert_ok!(Nfts::force_create(
Expand Down Expand Up @@ -1810,7 +1783,6 @@ fn burn_works() {
default_item_config()
));
assert_eq!(AccountBalance::<Test>::get(0, account(5)), 2);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_eq!(Balances::reserved_balance(account(1)), 2);

assert_noop!(
Expand All @@ -1819,10 +1791,8 @@ fn burn_works() {
);
assert_ok!(Nfts::burn(RuntimeOrigin::signed(account(5)), 0, 42));
assert_eq!(AccountBalance::<Test>::get(0, account(5)), 1);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 1);
assert_ok!(Nfts::burn(RuntimeOrigin::signed(account(5)), 0, 69));
assert_eq!(AccountBalance::<Test>::get(0, account(5)), 0);
assert_eq!(Collection::<Test>::get(0).unwrap().item_holders, 0);
assert_eq!(Balances::reserved_balance(account(1)), 0);
});
}
Expand Down Expand Up @@ -4154,13 +4124,7 @@ fn clear_collection_metadata_works() {
assert_ok!(Nfts::destroy(
RuntimeOrigin::signed(account(1)),
0,
DestroyWitness {
item_configs: 0,
item_metadatas: 0,
attributes: 0,
allowances: 0,
item_holders: 0
}
DestroyWitness { item_configs: 0, item_metadatas: 0, attributes: 0, allowances: 0 }
));
assert_eq!(Collection::<Test>::get(0), None);
assert_eq!(Balances::reserved_balance(&account(1)), 10);
Expand Down
6 changes: 0 additions & 6 deletions pallets/nfts/src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ pub struct CollectionDetails<AccountId, DepositBalance> {
pub item_metadatas: u32,
/// The total number of outstanding item configs of this collection.
pub item_configs: u32,
/// The total number of accounts that hold items of the collection.
pub item_holders: u32,
/// The total number of attributes for this collection.
pub attributes: u32,
/// The total number of allowances to spend all items within collections.
Expand All @@ -121,9 +119,6 @@ pub struct DestroyWitness {
/// The total number of outstanding item configs of this collection.
#[codec(compact)]
pub item_configs: u32,
/// The total number of accounts that hold items of the collection.
#[codec(compact)]
pub item_holders: u32,
/// The total number of attributes for this collection.
#[codec(compact)]
pub attributes: u32,
Expand All @@ -137,7 +132,6 @@ impl<AccountId, DepositBalance> CollectionDetails<AccountId, DepositBalance> {
DestroyWitness {
item_metadatas: self.item_metadatas,
item_configs: self.item_configs,
item_holders: self.item_holders,
attributes: self.attributes,
allowances: self.allowances,
}
Expand Down
Loading

0 comments on commit 2226f61

Please sign in to comment.