From 6a9d9d104e4e19b6b57680fc1d930f66ee903a3f Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 20 Sep 2022 11:59:46 +0200 Subject: [PATCH 01/62] Support repeated destroys to safely destroy large assets --- frame/assets/src/functions.rs | 50 ++++++++++++++++++++++++++++------- frame/assets/src/lib.rs | 6 +++++ frame/assets/src/tests.rs | 18 +++++++++++++ frame/assets/src/types.rs | 4 +++ 4 files changed, 68 insertions(+), 10 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 0f8e7096e80c1..ad36dd07045d7 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -679,35 +679,65 @@ impl, I: 'static> Pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } + ensure!(details.is_frozen, Error::::BadWitness); ensure!(details.accounts <= witness.accounts, Error::::BadWitness); ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); ensure!(details.approvals <= witness.approvals, Error::::BadWitness); - for (who, v) in Account::::drain_prefix(id) { + let accounts_to_delete: Vec<(T::AccountId, _)> = + Account::::iter_prefix(id).take(witness.destroy_count as usize).collect(); + for (who, v) in accounts_to_delete { + Account::::remove(id, &who); + // We have to force this as it's destroying the entire asset class. // This could mean that some accounts now have irreversibly reserved // funds. let _ = Self::dead_account(&who, &mut details, &v.reason, true); dead_accounts.push(who); } - debug_assert_eq!(details.accounts, 0); - debug_assert_eq!(details.sufficients, 0); - let metadata = Metadata::::take(&id); - T::Currency::unreserve( - &details.owner, - details.deposit.saturating_add(metadata.deposit), - ); + // FIXME:(delete comment before merge). What was the point of these 2 debug asserts? + // debug_assert_eq!(details.accounts, 0); + // debug_assert_eq!(details.sufficients, 0); - for ((owner, _), approval) in Approvals::::drain_prefix((&id,)) { + // FIXME:(delete comment before merge) An ideal implementation could be to + // unreserve&drain only approval for transactions sent by or recieved by one of the + // accounts which was deleted above. But it's not clear if that's strongly desired. + // This current approach buts the approval storage in an unusable state, since some + // vital transactions might be deleted for accounts that have not yet been deleted. + for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { T::Currency::unreserve(&owner, approval.deposit); + println!("IN APprovals iter {:?} {:?}", &owner, &approval); + Approvals::::remove((id, owner, destination)); + } + + // for ((owner, _), approval) in Approvals::::drain_prefix((&id,)) { + // T::Currency::unreserve(&owner, approval.deposit); + // } + + let accounts_remaining = Account::::iter_prefix(id).count(); + if accounts_remaining > 0 { + Self::deposit_event(Event::PartiallyDestroyed { + asset_id: id, + accounts_destroyed: dead_accounts.len() as u32, + accounts_remaining: accounts_remaining as u32, + }); + } else { + // NOTE:(delete comment before merge) It's not yet clear what this block does, + // but it appears to be an operation to be done once after cleanup. + let metadata = Metadata::::take(&id); + T::Currency::unreserve( + &details.owner, + details.deposit.saturating_add(metadata.deposit), + ); + Self::deposit_event(Event::Destroyed { asset_id: id }); } - Self::deposit_event(Event::Destroyed { asset_id: id }); Ok(DestroyWitness { accounts: details.accounts, sufficients: details.sufficients, approvals: details.approvals, + destroy_count: dead_accounts.len() as u32, }) }, )?; diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index fbb4a3ea1568b..5b39c3c328734 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -409,6 +409,12 @@ pub mod pallet { AssetThawed { asset_id: T::AssetId }, /// An asset class was destroyed. Destroyed { asset_id: T::AssetId }, + /// An asset class was only destroyed. The destroy action should be re-executed. + PartiallyDestroyed { + asset_id: T::AssetId, + accounts_destroyed: u32, + accounts_remaining: u32, + }, /// Some asset class was force-created. ForceCreated { asset_id: T::AssetId, owner: T::AccountId }, /// New metadata has been set for an asset. diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 598b6049b3d57..442bc351b8346 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -363,6 +363,24 @@ fn destroy_should_refund_approvals() { }); } +#[test] +fn partial_destroy_should_work() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 1)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 10)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 10)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 3, 10)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 4, 10)); + println!("TEST2🔥"); + let w = Asset::::get(0).unwrap().destroy_witness(); + + assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); + // TODO: + // - assert event is created correctly for partial destruction + // - reexec the destroy action and check that correct event is also created + }) +} + #[test] fn non_providing_should_work() { new_test_ext().execute_with(|| { diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 677fc5847c614..eaa92ffe64b7e 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -64,6 +64,7 @@ impl AssetDetails Date: Tue, 20 Sep 2022 12:29:36 +0200 Subject: [PATCH 02/62] require freezing accounts before destroying --- frame/assets/src/functions.rs | 12 +++++------- frame/assets/src/tests.rs | 1 + 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index ad36dd07045d7..d578068258e31 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -706,15 +706,13 @@ impl, I: 'static> Pallet { // This current approach buts the approval storage in an unusable state, since some // vital transactions might be deleted for accounts that have not yet been deleted. for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { - T::Currency::unreserve(&owner, approval.deposit); - println!("IN APprovals iter {:?} {:?}", &owner, &approval); - Approvals::::remove((id, owner, destination)); + println!("IN Approvals iter {:?} {:?}", &owner, &approval); + if dead_accounts.contains(&owner) { + T::Currency::unreserve(&owner, approval.deposit); + Approvals::::remove((id, owner, destination)); + }; } - // for ((owner, _), approval) in Approvals::::drain_prefix((&id,)) { - // T::Currency::unreserve(&owner, approval.deposit); - // } - let accounts_remaining = Account::::iter_prefix(id).count(); if accounts_remaining > 0 { Self::deposit_event(Event::PartiallyDestroyed { diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 442bc351b8346..78d174b3074ed 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -372,6 +372,7 @@ fn partial_destroy_should_work() { assert_ok!(Assets::mint(Origin::signed(1), 0, 3, 10)); assert_ok!(Assets::mint(Origin::signed(1), 0, 4, 10)); println!("TEST2🔥"); + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); From cbe14b2da20e2858c46a33fdc577ec315e39ab13 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 21 Sep 2022 17:27:35 +0200 Subject: [PATCH 03/62] support only deleting asset as final stage when there's no assets left --- frame/assets/src/functions.rs | 5 +++++ frame/assets/src/tests.rs | 28 +++++++++++++++++++++++----- frame/assets/src/types.rs | 2 +- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index d578068258e31..51a95ee22b136 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -679,6 +679,7 @@ impl, I: 'static> Pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } + ensure!(details.is_frozen, Error::::BadWitness); ensure!(details.accounts <= witness.accounts, Error::::BadWitness); ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); @@ -686,6 +687,7 @@ impl, I: 'static> Pallet { let accounts_to_delete: Vec<(T::AccountId, _)> = Account::::iter_prefix(id).take(witness.destroy_count as usize).collect(); + for (who, v) in accounts_to_delete { Account::::remove(id, &who); @@ -715,6 +717,9 @@ impl, I: 'static> Pallet { let accounts_remaining = Account::::iter_prefix(id).count(); if accounts_remaining > 0 { + // Reintroduce the details, so the asset is not deleted yet. + let _ = maybe_details.insert(details.clone()); + Self::deposit_event(Event::PartiallyDestroyed { asset_id: id, accounts_destroyed: dead_accounts.len() as u32, diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 78d174b3074ed..f718a6ccbc2fe 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -297,6 +297,7 @@ fn lifecycle_should_work() { assert_ok!(Assets::mint(Origin::signed(1), 0, 20, 100)); assert_eq!(Account::::iter_prefix(0).count(), 2); + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -317,6 +318,7 @@ fn lifecycle_should_work() { assert_ok!(Assets::mint(Origin::signed(1), 0, 20, 100)); assert_eq!(Account::::iter_prefix(0).count(), 2); + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::root(), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -335,6 +337,7 @@ fn destroy_with_bad_witness_should_not_work() { let mut w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::mint(Origin::signed(1), 0, 10, 100)); // witness too low + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); assert_noop!(Assets::destroy(Origin::signed(1), 0, w), Error::::BadWitness); // witness too high is okay though w.accounts += 2; @@ -354,6 +357,7 @@ fn destroy_should_refund_approvals() { assert_ok!(Assets::approve_transfer(Origin::signed(1), 0, 4, 50)); assert_eq!(Balances::reserved_balance(&1), 3); + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -371,14 +375,27 @@ fn partial_destroy_should_work() { assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 10)); assert_ok!(Assets::mint(Origin::signed(1), 0, 3, 10)); assert_ok!(Assets::mint(Origin::signed(1), 0, 4, 10)); - println!("TEST2🔥"); + assert_ok!(Assets::mint(Origin::signed(1), 0, 5, 10)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 6, 10)); assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); + let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); - // TODO: - // - assert event is created correctly for partial destruction - // - reexec the destroy action and check that correct event is also created + System::assert_has_event(RuntimeEvent::Assets(crate::Event::PartiallyDestroyed { + asset_id: 0, + accounts_destroyed: 5, + accounts_remaining: 1, + })); + // PartiallyDestroyed Asset should continue to exist + assert!(Asset::::contains_key(0)); + + // Second call to destroy on PartiallyDestroyed asset + let w2 = Asset::::get(0).unwrap().destroy_witness(); + assert_ok!(Assets::destroy(Origin::signed(1), 0, w2)); + System::assert_has_event(RuntimeEvent::Assets(crate::Event::Destroyed { asset_id: 0 })); + + // Destroyed Asset should not exist + assert!(!Asset::::contains_key(0)); }) } @@ -737,6 +754,7 @@ fn destroy_calls_died_hooks() { assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 100)); // Destroy the asset. + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index eaa92ffe64b7e..7bf2cb10584e2 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -64,7 +64,7 @@ impl AssetDetails Date: Thu, 22 Sep 2022 12:27:44 +0200 Subject: [PATCH 04/62] pre: introduce the RemoveKeyLimit config parameter --- bin/node/runtime/src/lib.rs | 2 ++ frame/assets/src/functions.rs | 2 +- frame/assets/src/lib.rs | 6 ++++++ frame/assets/src/mock.rs | 5 +++++ frame/assets/src/tests.rs | 2 +- 5 files changed, 15 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 0d8f00550ed1a..7d2c118a2911d 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1410,6 +1410,7 @@ parameter_types! { pub const StringLimit: u32 = 50; pub const MetadataDepositBase: Balance = 10 * DOLLARS; pub const MetadataDepositPerByte: Balance = 1 * DOLLARS; + pub const RemoveKeysLimit: u32 = 1000; } impl pallet_assets::Config for Runtime { @@ -1427,6 +1428,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = pallet_assets::weights::SubstrateWeight; + type RemoveKeysLimit = RemoveKeysLimit; } parameter_types! { diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 51a95ee22b136..49fb4fa02a3fb 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -708,7 +708,7 @@ impl, I: 'static> Pallet { // This current approach buts the approval storage in an unusable state, since some // vital transactions might be deleted for accounts that have not yet been deleted. for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { - println!("IN Approvals iter {:?} {:?}", &owner, &approval); + // println!("IN Approvals iter {:?} {:?}", &owner, &approval); if dead_accounts.contains(&owner) { T::Currency::unreserve(&owner, approval.deposit); Approvals::::remove((id, owner, destination)); diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 031189dd5b299..cebf98dc4df19 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -193,6 +193,10 @@ pub mod pallet { + MaxEncodedLen + TypeInfo; + /// Max number of storage keys to remove per extrinsic call. + #[pallet::constant] + type RemoveKeysLimit: Get; + /// Identifier for the class of asset. type AssetId: Member + Parameter @@ -576,6 +580,7 @@ pub mod pallet { } /// Destroy a class of fungible assets. + /// Due to weight restrictions, this function may need to be called multiple /// /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the /// owner of the asset `id`. @@ -593,6 +598,7 @@ pub mod pallet { /// - `c = (witness.accounts - witness.sufficients)` /// - `s = witness.sufficients` /// - `a = witness.approvals` + /// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads #[pallet::weight(T::WeightInfo::destroy( witness.accounts.saturating_sub(witness.sufficients), witness.sufficients, diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 1f105eb6b4fbc..22db1b122179b 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -84,6 +84,10 @@ impl pallet_balances::Config for Test { type ReserveIdentifier = [u8; 8]; } +parameter_types! { + pub const RemoveKeysLimit: u32 = 1000; +} + impl Config for Test { type RuntimeEvent = RuntimeEvent; type Balance = u64; @@ -99,6 +103,7 @@ impl Config for Test { type Freezer = TestFreezer; type WeightInfo = (); type Extra = (); + type RemoveKeysLimit = RemoveKeysLimit; } use std::collections::HashMap; diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 1431ccb533d14..28954eb1a3d6e 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -355,7 +355,7 @@ fn destroy_with_bad_witness_should_not_work() { let mut w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 10, 100)); // witness too low - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); assert_noop!(Assets::destroy(RuntimeOrigin::signed(1), 0, w), Error::::BadWitness); // witness too high is okay though w.accounts += 2; From c9ce171a84e67530a72e0ae7e27973667292d75b Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 22 Sep 2022 13:16:19 +0200 Subject: [PATCH 05/62] debug_ensure empty account in the right if block --- frame/assets/src/functions.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 49fb4fa02a3fb..c301b19dafea5 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -698,10 +698,6 @@ impl, I: 'static> Pallet { dead_accounts.push(who); } - // FIXME:(delete comment before merge). What was the point of these 2 debug asserts? - // debug_assert_eq!(details.accounts, 0); - // debug_assert_eq!(details.sufficients, 0); - // FIXME:(delete comment before merge) An ideal implementation could be to // unreserve&drain only approval for transactions sent by or recieved by one of the // accounts which was deleted above. But it's not clear if that's strongly desired. @@ -726,6 +722,9 @@ impl, I: 'static> Pallet { accounts_remaining: accounts_remaining as u32, }); } else { + debug_assert_eq!(details.accounts, 0); + debug_assert_eq!(details.sufficients, 0); + // NOTE:(delete comment before merge) It's not yet clear what this block does, // but it appears to be an operation to be done once after cleanup. let metadata = Metadata::::take(&id); From 3067e381aeb0c915152db968a2ae963229f76476 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 22 Sep 2022 17:06:10 +0200 Subject: [PATCH 06/62] update to having separate max values for accounts and approvals --- bin/node/runtime/src/lib.rs | 3 ++- frame/assets/src/benchmarking.rs | 3 +-- frame/assets/src/functions.rs | 39 +++++++++++++++++--------------- frame/assets/src/lib.rs | 16 +++++++------ frame/assets/src/mock.rs | 6 +++-- frame/assets/src/tests.rs | 30 ++++++++++++------------ frame/assets/src/types.rs | 4 ---- frame/assets/src/weights.rs | 14 +++--------- 8 files changed, 55 insertions(+), 60 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 7d2c118a2911d..4091aef70e340 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1428,7 +1428,8 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = pallet_assets::weights::SubstrateWeight; - type RemoveKeysLimit = RemoveKeysLimit; + type RemoveAccountsLimit = RemoveAccountsLimit; + type RemoveApprovalsLimit = RemoveApprovalsLimit; } parameter_types! { diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index ca891c2f42e4a..7eec9a5471d1a 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -168,11 +168,10 @@ benchmarks_instance_pallet! { destroy { let c in 0 .. 5_000; - let s in 0 .. 5_000; let a in 0 .. 5_00; let (caller, _) = create_default_asset::(true); add_consumers::(caller.clone(), c); - add_sufficients::(caller.clone(), s); + // add_sufficients::(caller.clone(), s); add_approvals::(caller.clone(), a); let witness = Asset::::get(T::AssetId::default()).unwrap().destroy_witness(); }: _(SystemOrigin::Signed(caller), Default::default(), witness) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index c301b19dafea5..ab10753c5de13 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -685,10 +685,18 @@ impl, I: 'static> Pallet { ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); ensure!(details.approvals <= witness.approvals, Error::::BadWitness); - let accounts_to_delete: Vec<(T::AccountId, _)> = - Account::::iter_prefix(id).take(witness.destroy_count as usize).collect(); + let mut removed_accounts = 0u32; + let mut removed_approvals = 0u32; + + let accounts_to_delete: Vec<(T::AccountId, _)> = Account::::iter_prefix(id) + .take(T::RemoveAccountsLimit::get() as usize) + .collect(); for (who, v) in accounts_to_delete { + if removed_accounts >= T::RemoveAccountsLimit::get() { + break + } + Account::::remove(id, &who); // We have to force this as it's destroying the entire asset class. @@ -696,19 +704,17 @@ impl, I: 'static> Pallet { // funds. let _ = Self::dead_account(&who, &mut details, &v.reason, true); dead_accounts.push(who); + removed_accounts += 1; } - // FIXME:(delete comment before merge) An ideal implementation could be to - // unreserve&drain only approval for transactions sent by or recieved by one of the - // accounts which was deleted above. But it's not clear if that's strongly desired. - // This current approach buts the approval storage in an unusable state, since some - // vital transactions might be deleted for accounts that have not yet been deleted. for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { - // println!("IN Approvals iter {:?} {:?}", &owner, &approval); - if dead_accounts.contains(&owner) { - T::Currency::unreserve(&owner, approval.deposit); - Approvals::::remove((id, owner, destination)); - }; + if removed_approvals >= T::RemoveApprovalsLimit::get() { + break + } + + T::Currency::unreserve(&owner, approval.deposit); + Approvals::::remove((id, owner, destination)); + removed_approvals += 1; } let accounts_remaining = Account::::iter_prefix(id).count(); @@ -725,8 +731,6 @@ impl, I: 'static> Pallet { debug_assert_eq!(details.accounts, 0); debug_assert_eq!(details.sufficients, 0); - // NOTE:(delete comment before merge) It's not yet clear what this block does, - // but it appears to be an operation to be done once after cleanup. let metadata = Metadata::::take(&id); T::Currency::unreserve( &details.owner, @@ -736,10 +740,9 @@ impl, I: 'static> Pallet { } Ok(DestroyWitness { - accounts: details.accounts, - sufficients: details.sufficients, - approvals: details.approvals, - destroy_count: dead_accounts.len() as u32, + accounts: removed_accounts, + sufficients: 0, + approvals: removed_approvals, }) }, )?; diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index cebf98dc4df19..0bef21025d312 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -193,9 +193,13 @@ pub mod pallet { + MaxEncodedLen + TypeInfo; - /// Max number of storage keys to remove per extrinsic call. + /// Max number of accounts to destroy per extrinsic call. #[pallet::constant] - type RemoveKeysLimit: Get; + type RemoveAccountsLimit: Get; + + /// Max number of approvalss to destroy per extrinsic call. + #[pallet::constant] + type RemoveApprovalsLimit: Get; /// Identifier for the class of asset. type AssetId: Member @@ -600,9 +604,8 @@ pub mod pallet { /// - `a = witness.approvals` /// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads #[pallet::weight(T::WeightInfo::destroy( - witness.accounts.saturating_sub(witness.sufficients), - witness.sufficients, - witness.approvals, + T::RemoveAccountsLimit::get(), + T::RemoveApprovalsLimit::get(), ))] pub fn destroy( origin: OriginFor, @@ -615,8 +618,7 @@ pub mod pallet { }; let details = Self::do_destroy(id, witness, maybe_check_owner)?; Ok(Some(T::WeightInfo::destroy( - details.accounts.saturating_sub(details.sufficients), - details.sufficients, + details.accounts, details.approvals, )) .into()) diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 22db1b122179b..256ff196e9830 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -85,7 +85,8 @@ impl pallet_balances::Config for Test { } parameter_types! { - pub const RemoveKeysLimit: u32 = 1000; + pub const RemoveAccountsLimit: u32 = 5; + pub const RemoveApprovalsLimit: u32 = 5; } impl Config for Test { @@ -103,7 +104,8 @@ impl Config for Test { type Freezer = TestFreezer; type WeightInfo = (); type Extra = (); - type RemoveKeysLimit = RemoveKeysLimit; + type RemoveAccountsLimit = RemoveAccountsLimit; + type RemoveApprovalsLimit = RemoveApprovalsLimit; } use std::collections::HashMap; diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 28954eb1a3d6e..b0907a9722a48 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -315,7 +315,7 @@ fn lifecycle_should_work() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 20, 100)); assert_eq!(Account::::iter_prefix(0).count(), 2); - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -336,7 +336,7 @@ fn lifecycle_should_work() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 20, 100)); assert_eq!(Account::::iter_prefix(0).count(), 2); - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(RuntimeOrigin::root(), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -355,7 +355,7 @@ fn destroy_with_bad_witness_should_not_work() { let mut w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 10, 100)); // witness too low - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); assert_noop!(Assets::destroy(RuntimeOrigin::signed(1), 0, w), Error::::BadWitness); // witness too high is okay though w.accounts += 2; @@ -375,7 +375,7 @@ fn destroy_should_refund_approvals() { assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 4, 50)); assert_eq!(Balances::reserved_balance(&1), 3); - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -388,17 +388,17 @@ fn destroy_should_refund_approvals() { #[test] fn partial_destroy_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 1)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 10)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 10)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 3, 10)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 4, 10)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 5, 10)); - assert_ok!(Assets::mint(Origin::signed(1), 0, 6, 10)); - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 3, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 4, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 5, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 6, 10)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); + assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); System::assert_has_event(RuntimeEvent::Assets(crate::Event::PartiallyDestroyed { asset_id: 0, accounts_destroyed: 5, @@ -409,7 +409,7 @@ fn partial_destroy_should_work() { // Second call to destroy on PartiallyDestroyed asset let w2 = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(Origin::signed(1), 0, w2)); + assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w2)); System::assert_has_event(RuntimeEvent::Assets(crate::Event::Destroyed { asset_id: 0 })); // Destroyed Asset should not exist @@ -826,7 +826,7 @@ fn destroy_calls_died_hooks() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 100)); // Destroy the asset. - assert_ok!(Assets::freeze_asset(Origin::signed(1), 0)); + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); let w = Asset::::get(0).unwrap().destroy_witness(); assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 7bf2cb10584e2..677fc5847c614 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -64,7 +64,6 @@ impl AssetDetails Weight; fn force_create() -> Weight; - fn destroy(c: u32, s: u32, a: u32, ) -> Weight; + fn destroy(c: u32, a: u32, ) -> Weight; fn mint() -> Weight; fn burn() -> Weight; fn transfer() -> Weight; @@ -89,21 +89,17 @@ impl WeightInfo for SubstrateWeight { // Storage: System Account (r:5000 w:5000) // Storage: Assets Metadata (r:1 w:0) // Storage: Assets Approvals (r:501 w:500) - fn destroy(c: u32, s: u32, a: u32, ) -> Weight { + fn destroy(c: u32, a: u32, ) -> Weight { Weight::from_ref_time(0 as u64) // Standard Error: 37_000 .saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64)) - // Standard Error: 37_000 - .saturating_add(Weight::from_ref_time(19_333_000 as u64).saturating_mul(s as u64)) // Standard Error: 375_000 .saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64)) .saturating_add(T::DbWeight::get().reads(5 as u64)) .saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(c as u64))) - .saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(s as u64))) .saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(a as u64))) .saturating_add(T::DbWeight::get().writes(2 as u64)) .saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(c as u64))) - .saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(s as u64))) .saturating_add(T::DbWeight::get().writes((1 as u64).saturating_mul(a as u64))) } // Storage: Assets Asset (r:1 w:1) @@ -274,21 +270,17 @@ impl WeightInfo for () { // Storage: System Account (r:5000 w:5000) // Storage: Assets Metadata (r:1 w:0) // Storage: Assets Approvals (r:501 w:500) - fn destroy(c: u32, s: u32, a: u32, ) -> Weight { + fn destroy(c: u32, a: u32, ) -> Weight { Weight::from_ref_time(0 as u64) // Standard Error: 37_000 .saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64)) - // Standard Error: 37_000 - .saturating_add(Weight::from_ref_time(19_333_000 as u64).saturating_mul(s as u64)) // Standard Error: 375_000 .saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64)) .saturating_add(RocksDbWeight::get().reads(5 as u64)) .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(s as u64))) .saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(a as u64))) .saturating_add(RocksDbWeight::get().writes(2 as u64)) .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(s as u64))) .saturating_add(RocksDbWeight::get().writes((1 as u64).saturating_mul(a as u64))) } // Storage: Assets Asset (r:1 w:1) From 7c4f6108d1d449625c1d9c445f518ebbdaa5ccaa Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Mon, 26 Sep 2022 16:25:14 +0200 Subject: [PATCH 07/62] add tests and use RemoveKeyLimit constant --- bin/node/runtime/src/lib.rs | 3 +- frame/assets/src/functions.rs | 7 +- frame/assets/src/lib.rs | 182 ++++++++++++++++-- frame/assets/src/mock.rs | 6 +- frame/assets/src/tests.rs | 98 ++++++---- frame/assets/src/types.rs | 11 ++ .../asset-tx-payment/src/tests.rs | 5 + 7 files changed, 248 insertions(+), 64 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 4091aef70e340..7d2c118a2911d 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1428,8 +1428,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = pallet_assets::weights::SubstrateWeight; - type RemoveAccountsLimit = RemoveAccountsLimit; - type RemoveApprovalsLimit = RemoveApprovalsLimit; + type RemoveKeysLimit = RemoveKeysLimit; } parameter_types! { diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index ab10753c5de13..7732d0e123e6e 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -652,6 +652,7 @@ impl, I: 'static> Pallet { sufficients: 0, approvals: 0, is_frozen: false, + status: AssetStatus::Live, }, ); Self::deposit_event(Event::ForceCreated { asset_id: id, owner }); @@ -689,11 +690,11 @@ impl, I: 'static> Pallet { let mut removed_approvals = 0u32; let accounts_to_delete: Vec<(T::AccountId, _)> = Account::::iter_prefix(id) - .take(T::RemoveAccountsLimit::get() as usize) + .take(T::RemoveKeysLimit::get() as usize) .collect(); for (who, v) in accounts_to_delete { - if removed_accounts >= T::RemoveAccountsLimit::get() { + if removed_accounts >= T::RemoveKeysLimit::get() { break } @@ -708,7 +709,7 @@ impl, I: 'static> Pallet { } for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { - if removed_approvals >= T::RemoveApprovalsLimit::get() { + if removed_approvals >= T::RemoveKeysLimit::get() { break } diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 0bef21025d312..0ab9f63df96ea 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -193,13 +193,9 @@ pub mod pallet { + MaxEncodedLen + TypeInfo; - /// Max number of accounts to destroy per extrinsic call. + /// Max number of storage keys to destroy per extrinsic call. #[pallet::constant] - type RemoveAccountsLimit: Get; - - /// Max number of approvalss to destroy per extrinsic call. - #[pallet::constant] - type RemoveApprovalsLimit: Get; + type RemoveKeysLimit: Get; /// Identifier for the class of asset. type AssetId: Member @@ -341,6 +337,7 @@ pub mod pallet { sufficients: 0, approvals: 0, is_frozen: false, + status: AssetStatus::Live, }, ); } @@ -415,6 +412,10 @@ pub mod pallet { AssetFrozen { asset_id: T::AssetId }, /// Some asset `asset_id` was thawed. AssetThawed { asset_id: T::AssetId }, + /// Accounts were destroyed for given asset. + DestroyedAccounts { asset_id: T::AssetId, accounts_destroyed: u32, accounts_remaining: u32 }, + /// Approvals were destroyed for given asset. + DestroyedApprovals { asset_id: T::AssetId, approvals_destroyed: u32 }, /// An asset class was destroyed. Destroyed { asset_id: T::AssetId }, /// An asset class was only destroyed. The destroy action should be re-executed. @@ -545,6 +546,7 @@ pub mod pallet { sufficients: 0, approvals: 0, is_frozen: false, + status: AssetStatus::Live, }, ); Self::deposit_event(Event::Created { asset_id: id, creator: owner, owner: admin }); @@ -602,26 +604,166 @@ pub mod pallet { /// - `c = (witness.accounts - witness.sufficients)` /// - `s = witness.sufficients` /// - `a = witness.approvals` - /// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads - #[pallet::weight(T::WeightInfo::destroy( - T::RemoveAccountsLimit::get(), - T::RemoveApprovalsLimit::get(), - ))] - pub fn destroy( + // /// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads + // #[pallet::weight(T::WeightInfo::destroy( + // T::RemoveAccountsLimit::get(), + // T::RemoveApprovalsLimit::get(), + // ))] + // pub fn destroy( + // origin: OriginFor, + // #[pallet::compact] id: T::AssetId, + // witness: DestroyWitness, + // ) -> DispatchResultWithPostInfo { + // let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { + // Ok(_) => None, + // Err(origin) => Some(ensure_signed(origin)?), + // }; + // let details = Self::do_destroy(id, witness, maybe_check_owner)?; + // Ok(Some(T::WeightInfo::destroy(details.accounts, details.approvals)).into()) + // } + + /// Start the process of destroying an asset + #[pallet::weight(T::WeightInfo::mint())] + pub fn start_destroy( + origin: OriginFor, + #[pallet::compact] id: T::AssetId, + ) -> DispatchResult { + let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { + Ok(_) => None, + Err(origin) => Some(ensure_signed(origin)?), + }; + let _ = Asset::::try_mutate_exists( + id, + |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::BadWitness); + details.status = AssetStatus::Destroying; + // TODO: Remove previlleged roles. How? + + Ok(()) + }, + )?; + Ok(()) + } + + /// Cleanup Accounts + #[pallet::weight(T::WeightInfo::mint())] + pub fn destroy_accounts( + origin: OriginFor, + #[pallet::compact] id: T::AssetId, + ) -> DispatchResult { + let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { + Ok(_) => None, + Err(origin) => Some(ensure_signed(origin)?), + }; + let mut dead_accounts: Vec = vec![]; + let _ = Asset::::try_mutate_exists( + id, + |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::BadWitness); + // Should only destroy accounts while the asset is being destroyed + ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); + + let mut removed_accounts = 0; + for (who, v) in Account::::drain_prefix(id) { + if removed_accounts >= T::RemoveKeysLimit::get() { + break + } + let _ = Self::dead_account(&who, &mut details, &v.reason, true); + dead_accounts.push(who); + removed_accounts += 1; + } + + Self::deposit_event(Event::DestroyedAccounts { + asset_id: id, + accounts_destroyed: removed_accounts as u32, + accounts_remaining: details.accounts as u32, + }); + + Ok(()) + }, + )?; + + for who in dead_accounts { + T::Freezer::died(id, &who); + } + Ok(()) + } + + #[pallet::weight(T::WeightInfo::mint())] + pub fn destroy_approvals( + origin: OriginFor, + #[pallet::compact] id: T::AssetId, + ) -> DispatchResult { + let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { + Ok(_) => None, + Err(origin) => Some(ensure_signed(origin)?), + }; + + let details = Asset::::get(id).ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::BadWitness); + // Should only destroy accounts while the asset is being destroyed + ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); + + let mut removed_approvals = 0; + for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { + if removed_approvals >= T::RemoveKeysLimit::get() { + break + } + T::Currency::unreserve(&owner, approval.deposit); + removed_approvals += 1; + } + + Self::deposit_event(Event::DestroyedApprovals { + asset_id: id, + approvals_destroyed: removed_approvals as u32, + }); + + Ok(()) + } + + /// + #[pallet::weight(T::WeightInfo::mint())] + pub fn finish_destroy( origin: OriginFor, #[pallet::compact] id: T::AssetId, - witness: DestroyWitness, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { Ok(_) => None, Err(origin) => Some(ensure_signed(origin)?), }; - let details = Self::do_destroy(id, witness, maybe_check_owner)?; - Ok(Some(T::WeightInfo::destroy( - details.accounts, - details.approvals, - )) - .into()) + let _ = Asset::::try_mutate_exists( + id, + |maybe_details| -> Result<(), DispatchError> { + let details = maybe_details.take().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::Unknown); + ensure!(details.accounts == 0, Error::::Unknown); + ensure!(details.approvals == 0, Error::::Unknown); + + let metadata = Metadata::::take(&id); + T::Currency::unreserve( + &details.owner, + details.deposit.saturating_add(metadata.deposit), + ); + Self::deposit_event(Event::Destroyed { asset_id: id }); + + Ok(()) + }, + )?; + Ok(()) } /// Mint assets of a particular class. diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 256ff196e9830..4aaa0ecb9d1be 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -85,8 +85,7 @@ impl pallet_balances::Config for Test { } parameter_types! { - pub const RemoveAccountsLimit: u32 = 5; - pub const RemoveApprovalsLimit: u32 = 5; + pub const RemoveKeysLimit: u32 = 5; } impl Config for Test { @@ -104,8 +103,7 @@ impl Config for Test { type Freezer = TestFreezer; type WeightInfo = (); type Extra = (); - type RemoveAccountsLimit = RemoveAccountsLimit; - type RemoveApprovalsLimit = RemoveApprovalsLimit; + type RemoveKeysLimit = RemoveKeysLimit; } use std::collections::HashMap; diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index b0907a9722a48..0e93a5ab40422 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -316,8 +316,11 @@ fn lifecycle_should_work() { assert_eq!(Account::::iter_prefix(0).count(), 2); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_eq!(Balances::reserved_balance(&1), 0); assert!(!Asset::::contains_key(0)); @@ -337,8 +340,11 @@ fn lifecycle_should_work() { assert_eq!(Account::::iter_prefix(0).count(), 2); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::root(), 0, w)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_eq!(Balances::reserved_balance(&1), 0); assert!(!Asset::::contains_key(0)); @@ -347,23 +353,6 @@ fn lifecycle_should_work() { }); } -#[test] -fn destroy_with_bad_witness_should_not_work() { - new_test_ext().execute_with(|| { - Balances::make_free_balance_be(&1, 100); - assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1)); - let mut w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 10, 100)); - // witness too low - assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_noop!(Assets::destroy(RuntimeOrigin::signed(1), 0, w), Error::::BadWitness); - // witness too high is okay though - w.accounts += 2; - w.sufficients += 2; - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); - }); -} - #[test] fn destroy_should_refund_approvals() { new_test_ext().execute_with(|| { @@ -376,8 +365,12 @@ fn destroy_should_refund_approvals() { assert_eq!(Balances::reserved_balance(&1), 3); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); + + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_eq!(Balances::reserved_balance(&1), 0); // all approvals are removed @@ -397,8 +390,11 @@ fn partial_destroy_should_work() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 6, 10)); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + System::assert_has_event(RuntimeEvent::Assets(crate::Event::PartiallyDestroyed { asset_id: 0, accounts_destroyed: 5, @@ -408,8 +404,12 @@ fn partial_destroy_should_work() { assert!(Asset::::contains_key(0)); // Second call to destroy on PartiallyDestroyed asset - let w2 = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w2)); + + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + System::assert_has_event(RuntimeEvent::Assets(crate::Event::Destroyed { asset_id: 0 })); // Destroyed Asset should not exist @@ -607,8 +607,22 @@ fn origin_guards_should_work() { Assets::force_transfer(RuntimeOrigin::signed(2), 0, 1, 2, 100), Error::::NoPermission ); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_noop!(Assets::destroy(RuntimeOrigin::signed(2), 0, w), Error::::NoPermission); + assert_noop!( + Assets::start_destroy(RuntimeOrigin::signed(2), 0), + Error::::NoPermission + ); + assert_noop!( + Assets::destroy_accounts(RuntimeOrigin::signed(2), 0), + Error::::NoPermission + ); + assert_noop!( + Assets::destroy_approvals(RuntimeOrigin::signed(2), 0), + Error::::NoPermission + ); + assert_noop!( + Assets::finish_destroy(RuntimeOrigin::signed(2), 0), + Error::::NoPermission + ); }); } @@ -819,23 +833,37 @@ fn set_metadata_should_work() { /// Destroying an asset calls the `FrozenBalance::died` hooks of all accounts. #[test] -fn destroy_calls_died_hooks() { +fn destroy_accounts_calls_died_hooks() { new_test_ext().execute_with(|| { assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 50)); // Create account 1 and 2. assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 100)); - // Destroy the asset. + // Destroy the accounts. assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - let w = Asset::::get(0).unwrap().destroy_witness(); - assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); - // Asset is gone and accounts 1 and 2 died. - assert!(Asset::::get(0).is_none()); + // Accounts 1 and 2 died. assert_eq!(hooks(), vec![Hook::Died(0, 1), Hook::Died(0, 2)]); }) } +/// Destroying an asset calls the `FrozenBalance::died` hooks of all accounts. +#[test] +fn finish_destroy_asset_destroys_asset() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 50)); + // Destroy the accounts. + assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0)); + + // Asset is gone + assert!(Asset::::get(0).is_none()); + }) +} + #[test] fn freezer_should_work() { new_test_ext().execute_with(|| { diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 677fc5847c614..a37615590b8f9 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -29,6 +29,15 @@ pub(super) type DepositBalanceOf = pub(super) type AssetAccountOf = AssetAccount<>::Balance, DepositBalanceOf, >::Extra>; +#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, MaxEncodedLen, TypeInfo)] +pub(super) enum AssetStatus { + /// The asset is active and able to be used. + Live, + /// The asset is currently being destroyed, and all actions are no longer permitted on the + /// asset + Destroying, +} + #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, MaxEncodedLen, TypeInfo)] pub struct AssetDetails { /// Can change `owner`, `issuer`, `freezer` and `admin` accounts. @@ -56,6 +65,8 @@ pub struct AssetDetails { pub(super) approvals: u32, /// Whether the asset is frozen for non-admin transfers. pub(super) is_frozen: bool, + /// The status of the asset + pub(super) status: AssetStatus, } impl AssetDetails { diff --git a/frame/transaction-payment/asset-tx-payment/src/tests.rs b/frame/transaction-payment/asset-tx-payment/src/tests.rs index cdf7d17898145..147cbe5dc6a90 100644 --- a/frame/transaction-payment/asset-tx-payment/src/tests.rs +++ b/frame/transaction-payment/asset-tx-payment/src/tests.rs @@ -152,6 +152,10 @@ impl pallet_transaction_payment::Config for Runtime { type OperationalFeeMultiplier = ConstU8<5>; } +parameter_types! { + pub const RemoveKeysLimit: u32 = 1000; +} + impl pallet_assets::Config for Runtime { type RuntimeEvent = RuntimeEvent; type Balance = Balance; @@ -167,6 +171,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = (); + type RemoveKeysLimit = RemoveKeysLimit; } pub struct HardcodedAuthor; From aca497fa4432541743c14382facb489d221dda44 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 10:01:03 +0200 Subject: [PATCH 08/62] add useful comments to the extrinsics, and calculate returned weight --- frame/assets/src/functions.rs | 10 +- frame/assets/src/lib.rs | 168 +++++++++++++++++++--------------- frame/assets/src/tests.rs | 61 +++++++----- frame/assets/src/weights.rs | 87 +++++++++++------- 4 files changed, 188 insertions(+), 138 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 7732d0e123e6e..fdf6f7e941f83 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -723,11 +723,11 @@ impl, I: 'static> Pallet { // Reintroduce the details, so the asset is not deleted yet. let _ = maybe_details.insert(details.clone()); - Self::deposit_event(Event::PartiallyDestroyed { - asset_id: id, - accounts_destroyed: dead_accounts.len() as u32, - accounts_remaining: accounts_remaining as u32, - }); + // Self::deposit_event(Event::PartiallyDestroyed { + // asset_id: id, + // accounts_destroyed: dead_accounts.len() as u32, + // accounts_remaining: accounts_remaining as u32, + // }); } else { debug_assert_eq!(details.accounts, 0); debug_assert_eq!(details.sufficients, 0); diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 0ab9f63df96ea..4d79113d03ce4 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -415,15 +415,14 @@ pub mod pallet { /// Accounts were destroyed for given asset. DestroyedAccounts { asset_id: T::AssetId, accounts_destroyed: u32, accounts_remaining: u32 }, /// Approvals were destroyed for given asset. - DestroyedApprovals { asset_id: T::AssetId, approvals_destroyed: u32 }, - /// An asset class was destroyed. - Destroyed { asset_id: T::AssetId }, - /// An asset class was only destroyed. The destroy action should be re-executed. - PartiallyDestroyed { + DestroyedApprovals { asset_id: T::AssetId, - accounts_destroyed: u32, - accounts_remaining: u32, + approvals_destroyed: u32, + approvals_remaining: u32, }, + /// An asset class was destroyed. + Destroyed { asset_id: T::AssetId }, + /// Some asset class was force-created. ForceCreated { asset_id: T::AssetId, owner: T::AccountId }, /// New metadata has been set for an asset. @@ -585,45 +584,18 @@ pub mod pallet { Self::do_force_create(id, owner, is_sufficient, min_balance) } - /// Destroy a class of fungible assets. - /// Due to weight restrictions, this function may need to be called multiple + /// Start the process of destroying a class of fungible asset + /// start_destroy is the first in a series of extrinsics that should be called, to allow + /// destroying an asset. /// /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the /// owner of the asset `id`. /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing - /// asset. - /// - /// Emits `Destroyed` event when successful. - /// - /// NOTE: It can be helpful to first freeze an asset before destroying it so that you - /// can provide accurate witness information and prevent users from manipulating state - /// in a way that can make it harder to destroy. - /// - /// Weight: `O(c + p + a)` where: - /// - `c = (witness.accounts - witness.sufficients)` - /// - `s = witness.sufficients` - /// - `a = witness.approvals` - // /// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads - // #[pallet::weight(T::WeightInfo::destroy( - // T::RemoveAccountsLimit::get(), - // T::RemoveApprovalsLimit::get(), - // ))] - // pub fn destroy( - // origin: OriginFor, - // #[pallet::compact] id: T::AssetId, - // witness: DestroyWitness, - // ) -> DispatchResultWithPostInfo { - // let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { - // Ok(_) => None, - // Err(origin) => Some(ensure_signed(origin)?), - // }; - // let details = Self::do_destroy(id, witness, maybe_check_owner)?; - // Ok(Some(T::WeightInfo::destroy(details.accounts, details.approvals)).into()) - // } - - /// Start the process of destroying an asset - #[pallet::weight(T::WeightInfo::mint())] + /// asset. + /// + /// Assets must be freezed before calling start_destroy. + #[pallet::weight(T::WeightInfo::start_destroy())] pub fn start_destroy( origin: OriginFor, #[pallet::compact] id: T::AssetId, @@ -649,17 +621,32 @@ pub mod pallet { Ok(()) } - /// Cleanup Accounts - #[pallet::weight(T::WeightInfo::mint())] + /// Destroy all accounts associated with a given asset. + /// `destroy_accounts` should only be called after `start_destroy` has been called, and the + /// asset is in a `Destroying` state + /// + /// Due to weight restrictions, this function may need to be called multiple + /// times to fully destroy all accounts. It will destroy `RemoveKeysLimit` accounts at a + /// time. + /// + /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the + /// owner of the asset `id`. + /// + /// - `id`: The identifier of the asset to be destroyed. This must identify an existing + /// asset. + /// + /// Each call Emits the `Event::DestroyedAccounts` event. + #[pallet::weight(T::WeightInfo::destroy_accounts(T::RemoveKeysLimit::get()))] pub fn destroy_accounts( origin: OriginFor, #[pallet::compact] id: T::AssetId, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { Ok(_) => None, Err(origin) => Some(ensure_signed(origin)?), }; let mut dead_accounts: Vec = vec![]; + let mut removed_accounts = 0; let _ = Asset::::try_mutate_exists( id, |maybe_details| -> Result<(), DispatchError> { @@ -671,14 +658,13 @@ pub mod pallet { // Should only destroy accounts while the asset is being destroyed ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); - let mut removed_accounts = 0; for (who, v) in Account::::drain_prefix(id) { + let _ = Self::dead_account(&who, &mut details, &v.reason, true); + dead_accounts.push(who); + removed_accounts = removed_accounts.saturating_add(1); if removed_accounts >= T::RemoveKeysLimit::get() { break } - let _ = Self::dead_account(&who, &mut details, &v.reason, true); - dead_accounts.push(who); - removed_accounts += 1; } Self::deposit_event(Event::DestroyedAccounts { @@ -694,46 +680,80 @@ pub mod pallet { for who in dead_accounts { T::Freezer::died(id, &who); } - Ok(()) + Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into()) } - #[pallet::weight(T::WeightInfo::mint())] + /// Destroy all approvals associated with a given asset. + /// `destroy_approvals` should only be called after `start_destroy` has been called, and the + /// asset is in a `Destroying` state + /// + /// Due to weight restrictions, this function may need to be called multiple + /// times to fully destroy all approvals. It will destroy `RemoveKeysLimit` approvals at a + /// time. + /// + /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the + /// owner of the asset `id`. + /// + /// - `id`: The identifier of the asset to be destroyed. This must identify an existing + /// asset. + /// + /// Each call Emits the `Event::DestroyedApprovals` event. + #[pallet::weight(T::WeightInfo::destroy_approvals(T::RemoveKeysLimit::get()))] pub fn destroy_approvals( origin: OriginFor, #[pallet::compact] id: T::AssetId, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { Ok(_) => None, Err(origin) => Some(ensure_signed(origin)?), }; - let details = Asset::::get(id).ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - ensure!(details.is_frozen, Error::::BadWitness); - // Should only destroy accounts while the asset is being destroyed - ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); - let mut removed_approvals = 0; - for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { - if removed_approvals >= T::RemoveKeysLimit::get() { - break - } - T::Currency::unreserve(&owner, approval.deposit); - removed_approvals += 1; - } + let _ = Asset::::try_mutate_exists( + id, + |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + + ensure!(details.is_frozen, Error::::BadWitness); + // Should only destroy accounts while the asset is being destroyed + ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); - Self::deposit_event(Event::DestroyedApprovals { - asset_id: id, - approvals_destroyed: removed_approvals as u32, - }); + for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { + T::Currency::unreserve(&owner, approval.deposit); + removed_approvals = removed_approvals.saturating_add(1); + details.approvals = details.approvals.saturating_sub(1); + if removed_approvals >= T::RemoveKeysLimit::get() { + break + } + } + Self::deposit_event(Event::DestroyedApprovals { + asset_id: id, + approvals_destroyed: removed_approvals as u32, + approvals_remaining: details.approvals as u32, + }); + Ok(()) + }, + )?; - Ok(()) + Ok(Some(T::WeightInfo::destroy_approvals(removed_approvals)).into()) } + /// Complete destroying asset and unreserve currency. + /// `finish_destroy` should only be called after `start_destroy` has been called, and the + /// asset is in a `Destroying` state. All accounts or approvals should be destroyed before + /// hand. /// - #[pallet::weight(T::WeightInfo::mint())] + /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the + /// owner of the asset `id`. + /// + /// - `id`: The identifier of the asset to be destroyed. This must identify an existing + /// asset. + /// + /// Each successful call Emits the `Event::Destroyed` event. + #[pallet::weight(T::WeightInfo::finish_destroy())] pub fn finish_destroy( origin: OriginFor, #[pallet::compact] id: T::AssetId, @@ -750,8 +770,8 @@ pub mod pallet { ensure!(details.owner == check_owner, Error::::NoPermission); } ensure!(details.is_frozen, Error::::Unknown); - ensure!(details.accounts == 0, Error::::Unknown); - ensure!(details.approvals == 0, Error::::Unknown); + ensure!(details.accounts == 0, Error::::InUse); + ensure!(details.approvals == 0, Error::::InUse); let metadata = Metadata::::take(&id); T::Currency::unreserve( diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 0e93a5ab40422..d20719b7f52a0 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -316,10 +316,10 @@ fn lifecycle_should_work() { assert_eq!(Account::::iter_prefix(0).count(), 2); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -340,10 +340,10 @@ fn lifecycle_should_work() { assert_eq!(Account::::iter_prefix(0).count(), 2); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -366,10 +366,10 @@ fn destroy_should_refund_approvals() { assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0)); assert_eq!(Balances::reserved_balance(&1), 0); @@ -388,27 +388,40 @@ fn partial_destroy_should_work() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 4, 10)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 5, 10)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 6, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 7, 10)); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + // Asset is in use, as all the accounts have not yet been destroyed. + // We need to call destroy_accounts or destroy_approvals again until asset is completely + // cleaned up. + assert_noop!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0), Error::::InUse); - System::assert_has_event(RuntimeEvent::Assets(crate::Event::PartiallyDestroyed { + System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedAccounts { asset_id: 0, accounts_destroyed: 5, - accounts_remaining: 1, + accounts_remaining: 2, })); - // PartiallyDestroyed Asset should continue to exist + System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedApprovals { + asset_id: 0, + approvals_destroyed: 0, + approvals_remaining: 0, + })); + // Partially destroyed Asset should continue to exist assert!(Asset::::contains_key(0)); // Second call to destroy on PartiallyDestroyed asset - - assert_ok!(Assets::start_destroy(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(0), 0)); - assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(0), 0)); + assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); + System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedAccounts { + asset_id: 0, + accounts_destroyed: 2, + accounts_remaining: 0, + })); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::destroy_approvals(RuntimeOrigin::signed(1), 0)); + assert_ok!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0)); System::assert_has_event(RuntimeEvent::Assets(crate::Event::Destroyed { asset_id: 0 })); diff --git a/frame/assets/src/weights.rs b/frame/assets/src/weights.rs index 899797b731253..c657509f8ce07 100644 --- a/frame/assets/src/weights.rs +++ b/frame/assets/src/weights.rs @@ -46,7 +46,10 @@ use sp_std::marker::PhantomData; pub trait WeightInfo { fn create() -> Weight; fn force_create() -> Weight; - fn destroy(c: u32, a: u32, ) -> Weight; + fn start_destroy() -> Weight; + fn destroy_accounts(m: u32) -> Weight; + fn destroy_approvals(m: u32) -> Weight; + fn finish_destroy() -> Weight; fn mint() -> Weight; fn burn() -> Weight; fn transfer() -> Weight; @@ -84,24 +87,31 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } - // Storage: Assets Asset (r:1 w:1) - // Storage: Assets Account (r:5002 w:5001) - // Storage: System Account (r:5000 w:5000) - // Storage: Assets Metadata (r:1 w:0) - // Storage: Assets Approvals (r:501 w:500) - fn destroy(c: u32, a: u32, ) -> Weight { - Weight::from_ref_time(0 as u64) - // Standard Error: 37_000 - .saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64)) - // Standard Error: 375_000 - .saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64)) - .saturating_add(T::DbWeight::get().reads(5 as u64)) - .saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(c as u64))) - .saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(a as u64))) - .saturating_add(T::DbWeight::get().writes(2 as u64)) - .saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(c as u64))) - .saturating_add(T::DbWeight::get().writes((1 as u64).saturating_mul(a as u64))) + + fn start_destroy() -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(T::DbWeight::get().reads(1 as u64)) + .saturating_add(T::DbWeight::get().writes(1 as u64)) } + + fn destroy_accounts(_m: u32) -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(T::DbWeight::get().reads(1 as u64)) + .saturating_add(T::DbWeight::get().writes(1 as u64)) + } + + fn destroy_approvals(_m: u32) -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(T::DbWeight::get().reads(1 as u64)) + .saturating_add(T::DbWeight::get().writes(1 as u64)) + } + + fn finish_destroy() -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(T::DbWeight::get().reads(1 as u64)) + .saturating_add(T::DbWeight::get().writes(1 as u64)) + } + // Storage: Assets Asset (r:1 w:1) // Storage: Assets Account (r:1 w:1) fn mint() -> Weight { @@ -265,24 +275,31 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } - // Storage: Assets Asset (r:1 w:1) - // Storage: Assets Account (r:5002 w:5001) - // Storage: System Account (r:5000 w:5000) - // Storage: Assets Metadata (r:1 w:0) - // Storage: Assets Approvals (r:501 w:500) - fn destroy(c: u32, a: u32, ) -> Weight { - Weight::from_ref_time(0 as u64) - // Standard Error: 37_000 - .saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64)) - // Standard Error: 375_000 - .saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64)) - .saturating_add(RocksDbWeight::get().reads(5 as u64)) - .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(a as u64))) - .saturating_add(RocksDbWeight::get().writes(2 as u64)) - .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().writes((1 as u64).saturating_mul(a as u64))) + + fn start_destroy() -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64)) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + + fn destroy_accounts(_m: u32) -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64)) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) + } + + fn destroy_approvals(_m: u32) -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64)) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) + } + + fn finish_destroy() -> Weight { + Weight::from_ref_time(15_473_000 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64)) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) + } + // Storage: Assets Asset (r:1 w:1) // Storage: Assets Account (r:1 w:1) fn mint() -> Weight { From 93d886dd98602d8605ccd5fc398fee6719029259 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 13:43:15 +0200 Subject: [PATCH 09/62] add benchmarking for start_destroy and finish destroy --- frame/assets/src/benchmarking.rs | 60 ++++++++++++++++--- frame/assets/src/functions.rs | 96 ------------------------------ frame/assets/src/impl_fungibles.rs | 16 ----- frame/assets/src/lib.rs | 3 + frame/assets/src/types.rs | 24 -------- 5 files changed, 55 insertions(+), 144 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index 7eec9a5471d1a..dee419adeeff9 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -166,19 +166,63 @@ benchmarks_instance_pallet! { assert_last_event::(Event::ForceCreated { asset_id: Default::default(), owner: caller }.into()); } - destroy { - let c in 0 .. 5_000; - let a in 0 .. 5_00; + start_destroy { + let (caller, caller_lookup) = create_default_minted_asset::(true, 100u32.into()); + Assets::::freeze_asset( + SystemOrigin::Signed(caller.clone()).into(), + Default::default(), + )?; + }:_(SystemOrigin::Signed(caller), Default::default()) + verify { + assert_last_event::(Event::Destroying { asset_id: Default::default() }.into()); + } + + destroy_accounts { + let s in 0 .. 5; let (caller, _) = create_default_asset::(true); - add_consumers::(caller.clone(), c); // add_sufficients::(caller.clone(), s); - add_approvals::(caller.clone(), a); - let witness = Asset::::get(T::AssetId::default()).unwrap().destroy_witness(); - }: _(SystemOrigin::Signed(caller), Default::default(), witness) + Assets::::freeze_asset( + SystemOrigin::Signed(caller.clone()).into(), + Default::default(), + )?; + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + }:_(SystemOrigin::Signed(caller), Default::default()) verify { - assert_last_event::(Event::Destroyed { asset_id: Default::default() }.into()); + assert_last_event::(Event::DestroyedAccounts { + asset_id: Default::default() , + accounts_destroyed: 5, + accounts_remaining: 0, + }.into()); } + finish_destroy { + let (caller, caller_lookup) = create_default_asset::(true); + Assets::::freeze_asset( + SystemOrigin::Signed(caller.clone()).into(), + Default::default(), + )?; + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + }:_(SystemOrigin::Signed(caller), Default::default()) + verify { + assert_last_event::(Event::Destroyed { + asset_id: Default::default() , + }.into() + ); + } + + // destroy { + // let c in 0 .. 5_000; + // let a in 0 .. 5_00; + // let (caller, _) = create_default_asset::(true); + // add_consumers::(caller.clone(), c); + // // add_sufficients::(caller.clone(), s); + // add_approvals::(caller.clone(), a); + // let witness = Asset::::get(T::AssetId::default()).unwrap().destroy_witness(); + // }: _(SystemOrigin::Signed(caller), Default::default(), witness) + // verify { + // assert_last_event::(Event::Destroyed { asset_id: Default::default() }.into()); + // } + mint { let (caller, caller_lookup) = create_default_asset::(true); let amount = T::Balance::from(100u32); diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index fdf6f7e941f83..a8203342e9ae7 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -659,102 +659,6 @@ impl, I: 'static> Pallet { Ok(()) } - /// Destroy an existing asset. - /// - /// * `id`: The asset you want to destroy. - /// * `witness`: Witness data needed about the current state of the asset, used to confirm - /// complexity of the operation. - /// * `maybe_check_owner`: An optional check before destroying the asset, if the provided - /// account is the owner of that asset. Can be used for authorization checks. - pub(super) fn do_destroy( - id: T::AssetId, - witness: DestroyWitness, - maybe_check_owner: Option, - ) -> Result { - let mut dead_accounts: Vec = vec![]; - - let result_witness: DestroyWitness = Asset::::try_mutate_exists( - id, - |maybe_details| -> Result { - let mut details = maybe_details.take().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - - ensure!(details.is_frozen, Error::::BadWitness); - ensure!(details.accounts <= witness.accounts, Error::::BadWitness); - ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); - ensure!(details.approvals <= witness.approvals, Error::::BadWitness); - - let mut removed_accounts = 0u32; - let mut removed_approvals = 0u32; - - let accounts_to_delete: Vec<(T::AccountId, _)> = Account::::iter_prefix(id) - .take(T::RemoveKeysLimit::get() as usize) - .collect(); - - for (who, v) in accounts_to_delete { - if removed_accounts >= T::RemoveKeysLimit::get() { - break - } - - Account::::remove(id, &who); - - // We have to force this as it's destroying the entire asset class. - // This could mean that some accounts now have irreversibly reserved - // funds. - let _ = Self::dead_account(&who, &mut details, &v.reason, true); - dead_accounts.push(who); - removed_accounts += 1; - } - - for ((owner, destination), approval) in Approvals::::iter_prefix((id,)) { - if removed_approvals >= T::RemoveKeysLimit::get() { - break - } - - T::Currency::unreserve(&owner, approval.deposit); - Approvals::::remove((id, owner, destination)); - removed_approvals += 1; - } - - let accounts_remaining = Account::::iter_prefix(id).count(); - if accounts_remaining > 0 { - // Reintroduce the details, so the asset is not deleted yet. - let _ = maybe_details.insert(details.clone()); - - // Self::deposit_event(Event::PartiallyDestroyed { - // asset_id: id, - // accounts_destroyed: dead_accounts.len() as u32, - // accounts_remaining: accounts_remaining as u32, - // }); - } else { - debug_assert_eq!(details.accounts, 0); - debug_assert_eq!(details.sufficients, 0); - - let metadata = Metadata::::take(&id); - T::Currency::unreserve( - &details.owner, - details.deposit.saturating_add(metadata.deposit), - ); - Self::deposit_event(Event::Destroyed { asset_id: id }); - } - - Ok(DestroyWitness { - accounts: removed_accounts, - sufficients: 0, - approvals: removed_approvals, - }) - }, - )?; - - // Execute hooks outside of `mutate`. - for who in dead_accounts { - T::Freezer::died(id, &who); - } - Ok(result_witness) - } - /// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate' /// while reserving `T::ApprovalDeposit` from owner /// diff --git a/frame/assets/src/impl_fungibles.rs b/frame/assets/src/impl_fungibles.rs index 842ee5c102c1d..afca512efe541 100644 --- a/frame/assets/src/impl_fungibles.rs +++ b/frame/assets/src/impl_fungibles.rs @@ -179,22 +179,6 @@ impl, I: 'static> fungibles::Create for Pallet } } -impl, I: 'static> fungibles::Destroy for Pallet { - type DestroyWitness = DestroyWitness; - - fn get_destroy_witness(asset: &T::AssetId) -> Option { - Asset::::get(asset).map(|asset_details| asset_details.destroy_witness()) - } - - fn destroy( - id: T::AssetId, - witness: Self::DestroyWitness, - maybe_check_owner: Option, - ) -> Result { - Self::do_destroy(id, witness, maybe_check_owner) - } -} - impl, I: 'static> fungibles::metadata::Inspect<::AccountId> for Pallet { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 4d79113d03ce4..5d7affcdcf57a 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -420,6 +420,8 @@ pub mod pallet { approvals_destroyed: u32, approvals_remaining: u32, }, + /// An asset class is in the process of being destroyed. + Destroying { asset_id: T::AssetId }, /// An asset class was destroyed. Destroyed { asset_id: T::AssetId }, @@ -615,6 +617,7 @@ pub mod pallet { details.status = AssetStatus::Destroying; // TODO: Remove previlleged roles. How? + Self::deposit_event(Event::Destroying { asset_id: id }); Ok(()) }, )?; diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index a37615590b8f9..0eb437d0321a1 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -69,16 +69,6 @@ pub struct AssetDetails { pub(super) status: AssetStatus, } -impl AssetDetails { - pub fn destroy_witness(&self) -> DestroyWitness { - DestroyWitness { - accounts: self.accounts, - sufficients: self.sufficients, - approvals: self.approvals, - } - } -} - /// Data concerning an approval. #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, MaxEncodedLen, TypeInfo)] pub struct Approval { @@ -150,20 +140,6 @@ pub struct AssetMetadata { pub(super) is_frozen: bool, } -/// Witness data for the destroy transactions. -#[derive(Copy, Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, MaxEncodedLen, TypeInfo)] -pub struct DestroyWitness { - /// The number of accounts holding the asset. - #[codec(compact)] - pub(super) accounts: u32, - /// The number of accounts holding the asset with a self-sufficient reference. - #[codec(compact)] - pub(super) sufficients: u32, - /// The number of transfer-approvals of the asset. - #[codec(compact)] - pub(super) approvals: u32, -} - /// Trait for allowing a minimum balance on the account to be specified, beyond the /// `minimum_balance` of the asset. This is additive - the `minimum_balance` of the asset must be /// met *and then* anything here in addition. From 2d3b6507817d66f6e2e697074bfc806e4a6eb2a8 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 16:21:39 +0200 Subject: [PATCH 10/62] push failing benchmark logic --- frame/assets/src/benchmarking.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index dee419adeeff9..d8af99cc0083e 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -178,9 +178,7 @@ benchmarks_instance_pallet! { } destroy_accounts { - let s in 0 .. 5; - let (caller, _) = create_default_asset::(true); - // add_sufficients::(caller.clone(), s); + let (caller, _) = create_default_minted_asset::(true, 100u32.into()); Assets::::freeze_asset( SystemOrigin::Signed(caller.clone()).into(), Default::default(), @@ -188,11 +186,11 @@ benchmarks_instance_pallet! { Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); }:_(SystemOrigin::Signed(caller), Default::default()) verify { - assert_last_event::(Event::DestroyedAccounts { - asset_id: Default::default() , - accounts_destroyed: 5, - accounts_remaining: 0, - }.into()); + // assert_last_event::(Event::DestroyedAccounts { + // asset_id: Default::default() , + // accounts_destroyed: 5, + // accounts_remaining: 0, + // }.into()); } finish_destroy { From 5a612d1d14df9a31b33c69067178369c3a03a174 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 18:34:20 +0200 Subject: [PATCH 11/62] add benchmark tests for new functions --- frame/assets/src/benchmarking.rs | 45 +++++++++++++++++++------------- frame/assets/src/lib.rs | 3 +++ 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index d8af99cc0083e..ea337aa17aad2 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -178,7 +178,29 @@ benchmarks_instance_pallet! { } destroy_accounts { + let c in 0 .. T::RemoveKeysLimit::get(); + let (caller, _) = create_default_asset::(true); + add_sufficients::(caller.clone(), c); + Assets::::freeze_asset( + SystemOrigin::Signed(caller.clone()).into(), + Default::default(), + )?; + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + }:_(SystemOrigin::Signed(caller), Default::default()) + verify { + assert_last_event::(Event::DestroyedAccounts { + asset_id: Default::default() , + accounts_destroyed: c, + accounts_remaining: 0, + }.into()); + } + + destroy_approvals { + let c in 0 .. T::RemoveKeysLimit::get(); + let a in 0 .. T::RemoveKeysLimit::get(); let (caller, _) = create_default_minted_asset::(true, 100u32.into()); + add_consumers::(caller.clone(), c); + add_approvals::(caller.clone(), a); Assets::::freeze_asset( SystemOrigin::Signed(caller.clone()).into(), Default::default(), @@ -186,11 +208,11 @@ benchmarks_instance_pallet! { Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); }:_(SystemOrigin::Signed(caller), Default::default()) verify { - // assert_last_event::(Event::DestroyedAccounts { - // asset_id: Default::default() , - // accounts_destroyed: 5, - // accounts_remaining: 0, - // }.into()); + assert_last_event::(Event::DestroyedApprovals { + asset_id: Default::default() , + approvals_destroyed: a, + approvals_remaining: 0, + }.into()); } finish_destroy { @@ -208,19 +230,6 @@ benchmarks_instance_pallet! { ); } - // destroy { - // let c in 0 .. 5_000; - // let a in 0 .. 5_00; - // let (caller, _) = create_default_asset::(true); - // add_consumers::(caller.clone(), c); - // // add_sufficients::(caller.clone(), s); - // add_approvals::(caller.clone(), a); - // let witness = Asset::::get(T::AssetId::default()).unwrap().destroy_witness(); - // }: _(SystemOrigin::Signed(caller), Default::default(), witness) - // verify { - // assert_last_event::(Event::Destroyed { asset_id: Default::default() }.into()); - // } - mint { let (caller, caller_lookup) = create_default_asset::(true); let amount = T::Balance::from(100u32); diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 5d7affcdcf57a..2cf22f9c8df13 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -120,6 +120,9 @@ //! * [`System`](../frame_system/index.html) //! * [`Support`](../frame_support/index.html) +// This recursion limit is needed because we have too many benchmarks and benchmarking will fail if +// we add more without this limit. +#![recursion_limit = "1024"] // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] From b9612553a3bb0a5e12769060a559ad64fe097917 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 19:29:59 +0200 Subject: [PATCH 12/62] update weights via local benchmarks --- frame/assets/src/benchmarking.rs | 2 - frame/assets/src/weights.rs | 76 +++++++++++++++++++++++--------- frame/assets/src/weights2.rs | 72 ++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 22 deletions(-) create mode 100644 frame/assets/src/weights2.rs diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index ea337aa17aad2..d85e21f590d70 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -196,10 +196,8 @@ benchmarks_instance_pallet! { } destroy_approvals { - let c in 0 .. T::RemoveKeysLimit::get(); let a in 0 .. T::RemoveKeysLimit::get(); let (caller, _) = create_default_minted_asset::(true, 100u32.into()); - add_consumers::(caller.clone(), c); add_approvals::(caller.clone(), a); Assets::::freeze_asset( SystemOrigin::Signed(caller.clone()).into(), diff --git a/frame/assets/src/weights.rs b/frame/assets/src/weights.rs index c657509f8ce07..60a6a0e0caeea 100644 --- a/frame/assets/src/weights.rs +++ b/frame/assets/src/weights.rs @@ -47,7 +47,7 @@ pub trait WeightInfo { fn create() -> Weight; fn force_create() -> Weight; fn start_destroy() -> Weight; - fn destroy_accounts(m: u32) -> Weight; + fn destroy_accounts(c: u32) -> Weight; fn destroy_approvals(m: u32) -> Weight; fn finish_destroy() -> Weight; fn mint() -> Weight; @@ -88,27 +88,45 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes(1 as u64)) } + // Storage: Assets Asset (r:1 w:1) fn start_destroy() -> Weight { - Weight::from_ref_time(15_473_000 as u64) + Weight::from_ref_time(31_000_000 as u64) .saturating_add(T::DbWeight::get().reads(1 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } - fn destroy_accounts(_m: u32) -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(T::DbWeight::get().reads(1 as u64)) - .saturating_add(T::DbWeight::get().writes(1 as u64)) + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Account (r:1 w:0) + // Storage: System Account (r:20 w:20) + /// The range of component `c` is `[0, 1000]`. + fn destroy_accounts(c: u32, ) -> Weight { + Weight::from_ref_time(37_000_000 as u64) + // Standard Error: 19_301 + .saturating_add(Weight::from_ref_time(25_467_908 as u64).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(2 as u64)) + .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64))) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) + .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64))) } - fn destroy_approvals(_m: u32) -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(T::DbWeight::get().reads(1 as u64)) + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Approvals (r:1 w:0) + /// The range of component `a` is `[0, 1000]`. + fn destroy_approvals(a: u32, ) -> Weight { + Weight::from_ref_time(39_000_000 as u64) + // Standard Error: 14_298 + .saturating_add(Weight::from_ref_time(27_632_144 as u64).saturating_mul(a as u64)) + .saturating_add(T::DbWeight::get().reads(2 as u64)) + .saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(a as u64))) .saturating_add(T::DbWeight::get().writes(1 as u64)) + .saturating_add(T::DbWeight::get().writes((1 as u64).saturating_mul(a as u64))) } + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Metadata (r:1 w:0) fn finish_destroy() -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(T::DbWeight::get().reads(1 as u64)) + Weight::from_ref_time(33_000_000 as u64) + .saturating_add(T::DbWeight::get().reads(2 as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } @@ -276,27 +294,45 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes(1 as u64)) } + // Storage: Assets Asset (r:1 w:1) fn start_destroy() -> Weight { - Weight::from_ref_time(15_473_000 as u64) + Weight::from_ref_time(31_000_000 as u64) .saturating_add(RocksDbWeight::get().reads(1 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } - fn destroy_accounts(_m: u32) -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(RocksDbWeight::get().reads(1 as u64)) + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Account (r:1 w:0) + // Storage: System Account (r:20 w:20) + /// The range of component `c` is `[0, 1000]`. + fn destroy_accounts(c: u32, ) -> Weight { + Weight::from_ref_time(37_000_000 as u64) + // Standard Error: 19_301 + .saturating_add(Weight::from_ref_time(25_467_908 as u64).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(2 as u64)) + .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64))) .saturating_add(RocksDbWeight::get().writes(1 as u64)) + .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64))) } - fn destroy_approvals(_m: u32) -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(RocksDbWeight::get().reads(1 as u64)) + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Approvals (r:1 w:0) + /// The range of component `a` is `[0, 1000]`. + fn destroy_approvals(a: u32, ) -> Weight { + Weight::from_ref_time(39_000_000 as u64) + // Standard Error: 14_298 + .saturating_add(Weight::from_ref_time(27_632_144 as u64).saturating_mul(a as u64)) + .saturating_add(RocksDbWeight::get().reads(2 as u64)) + .saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(a as u64))) .saturating_add(RocksDbWeight::get().writes(1 as u64)) + .saturating_add(RocksDbWeight::get().writes((1 as u64).saturating_mul(a as u64))) } + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Metadata (r:1 w:0) fn finish_destroy() -> Weight { - Weight::from_ref_time(15_473_000 as u64) - .saturating_add(RocksDbWeight::get().reads(1 as u64)) + Weight::from_ref_time(33_000_000 as u64) + .saturating_add(RocksDbWeight::get().reads(2 as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } diff --git a/frame/assets/src/weights2.rs b/frame/assets/src/weights2.rs new file mode 100644 index 0000000000000..7b06d310b6060 --- /dev/null +++ b/frame/assets/src/weights2.rs @@ -0,0 +1,72 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Autogenerated weights for pallet_assets +//! +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev +//! DATE: 2022-09-27, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! HOSTNAME: `MacBook-Pro-6`, CPU: `` +//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 + +// Executed Command: +// ./target/release/substrate +// benchmark +// pallet +// --chain=dev +// --steps=50 +// --repeat=20 +// --pallet=pallet_assets +// --execution=wasm +// --wasm-execution=compiled +// --template=./.maintain/frame-weight-template.hbs +// --output=./frame/assets/src/weights2.rs +// --extrinsic=finish_destroy + +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use sp_std::marker::PhantomData; + +/// Weight functions needed for pallet_assets. +pub trait WeightInfo { + fn finish_destroy() -> Weight; +} + +/// Weights for pallet_assets using the Substrate node and recommended hardware. +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Metadata (r:1 w:0) + fn finish_destroy() -> Weight { + Weight::from_ref_time(33_000_000 as u64) + .saturating_add(T::DbWeight::get().reads(2 as u64)) + .saturating_add(T::DbWeight::get().writes(1 as u64)) + } +} + +// For backwards compatibility and tests +impl WeightInfo for () { + // Storage: Assets Asset (r:1 w:1) + // Storage: Assets Metadata (r:1 w:0) + fn finish_destroy() -> Weight { + Weight::from_ref_time(33_000_000 as u64) + .saturating_add(RocksDbWeight::get().reads(2 as u64)) + .saturating_add(RocksDbWeight::get().writes(1 as u64)) + } +} From 91e3c18a548ad7e86c470feb3027532009732e03 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 27 Sep 2022 19:32:13 +0200 Subject: [PATCH 13/62] remove extra weight file --- frame/assets/src/weights2.rs | 72 ------------------------------------ 1 file changed, 72 deletions(-) delete mode 100644 frame/assets/src/weights2.rs diff --git a/frame/assets/src/weights2.rs b/frame/assets/src/weights2.rs deleted file mode 100644 index 7b06d310b6060..0000000000000 --- a/frame/assets/src/weights2.rs +++ /dev/null @@ -1,72 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2022 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Autogenerated weights for pallet_assets -//! -//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2022-09-27, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` -//! HOSTNAME: `MacBook-Pro-6`, CPU: `` -//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 - -// Executed Command: -// ./target/release/substrate -// benchmark -// pallet -// --chain=dev -// --steps=50 -// --repeat=20 -// --pallet=pallet_assets -// --execution=wasm -// --wasm-execution=compiled -// --template=./.maintain/frame-weight-template.hbs -// --output=./frame/assets/src/weights2.rs -// --extrinsic=finish_destroy - -#![cfg_attr(rustfmt, rustfmt_skip)] -#![allow(unused_parens)] -#![allow(unused_imports)] - -use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; -use sp_std::marker::PhantomData; - -/// Weight functions needed for pallet_assets. -pub trait WeightInfo { - fn finish_destroy() -> Weight; -} - -/// Weights for pallet_assets using the Substrate node and recommended hardware. -pub struct SubstrateWeight(PhantomData); -impl WeightInfo for SubstrateWeight { - // Storage: Assets Asset (r:1 w:1) - // Storage: Assets Metadata (r:1 w:0) - fn finish_destroy() -> Weight { - Weight::from_ref_time(33_000_000 as u64) - .saturating_add(T::DbWeight::get().reads(2 as u64)) - .saturating_add(T::DbWeight::get().writes(1 as u64)) - } -} - -// For backwards compatibility and tests -impl WeightInfo for () { - // Storage: Assets Asset (r:1 w:1) - // Storage: Assets Metadata (r:1 w:0) - fn finish_destroy() -> Weight { - Weight::from_ref_time(33_000_000 as u64) - .saturating_add(RocksDbWeight::get().reads(2 as u64)) - .saturating_add(RocksDbWeight::get().writes(1 as u64)) - } -} From 60f954a6cc5cce9f4ca9ce5ed1207387171a2d60 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 28 Sep 2022 15:46:03 +0200 Subject: [PATCH 14/62] Update frame/assets/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- frame/assets/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 2cf22f9c8df13..fa00366992073 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -662,7 +662,7 @@ pub mod pallet { } ensure!(details.is_frozen, Error::::BadWitness); // Should only destroy accounts while the asset is being destroyed - ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); for (who, v) in Account::::drain_prefix(id) { let _ = Self::dead_account(&who, &mut details, &v.reason, true); From ce00b5beb7780bf430a9a13b545a754915055571 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 28 Sep 2022 15:46:18 +0200 Subject: [PATCH 15/62] Update frame/assets/src/types.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- frame/assets/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 0eb437d0321a1..50a56c4b0746c 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -34,7 +34,7 @@ pub(super) enum AssetStatus { /// The asset is active and able to be used. Live, /// The asset is currently being destroyed, and all actions are no longer permitted on the - /// asset + /// asset. Once set to `Destroying`, the asset can never transition back to a `Live` state. Destroying, } From 5492b6df2b6b81f3892591d9d6c2eacb479571f3 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 28 Sep 2022 15:46:38 +0200 Subject: [PATCH 16/62] Update frame/assets/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- frame/assets/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index fa00366992073..9caaeb8610f02 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -424,7 +424,7 @@ pub mod pallet { approvals_remaining: u32, }, /// An asset class is in the process of being destroyed. - Destroying { asset_id: T::AssetId }, + DestructionStarted { asset_id: T::AssetId }, /// An asset class was destroyed. Destroyed { asset_id: T::AssetId }, From bc20a6c01d3c0170d867e66f9a65a356a92c3041 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 28 Sep 2022 16:02:22 +0200 Subject: [PATCH 17/62] effect some changes from codereview --- bin/node/runtime/src/lib.rs | 3 +-- frame/assets/src/lib.rs | 9 ++++++--- frame/assets/src/mock.rs | 6 +----- frame/transaction-payment/asset-tx-payment/src/tests.rs | 6 +----- 4 files changed, 9 insertions(+), 15 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 5ba3b21ed1026..5285b87eb1ad0 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1417,7 +1417,6 @@ parameter_types! { pub const StringLimit: u32 = 50; pub const MetadataDepositBase: Balance = 10 * DOLLARS; pub const MetadataDepositPerByte: Balance = 1 * DOLLARS; - pub const RemoveKeysLimit: u32 = 1000; } impl pallet_assets::Config for Runtime { @@ -1435,7 +1434,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = pallet_assets::weights::SubstrateWeight; - type RemoveKeysLimit = RemoveKeysLimit; + type RemoveKeysLimit = ConstU32<1000>; } parameter_types! { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 9caaeb8610f02..dadc69f98d249 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -496,6 +496,9 @@ pub mod pallet { NoDeposit, /// The operation would result in funds being burned. WouldBurn, + /// The asset is a live asset and is actively being used. Usually emit for operations such + /// as `start_destroy` which require the asset to be in a destroying state. + LiveAsset, } #[pallet::call] @@ -618,9 +621,8 @@ pub mod pallet { } ensure!(details.is_frozen, Error::::BadWitness); details.status = AssetStatus::Destroying; - // TODO: Remove previlleged roles. How? - Self::deposit_event(Event::Destroying { asset_id: id }); + Self::deposit_event(Event::DestructionStarted { asset_id: id }); Ok(()) }, )?; @@ -725,7 +727,7 @@ pub mod pallet { ensure!(details.is_frozen, Error::::BadWitness); // Should only destroy accounts while the asset is being destroyed - ensure!(details.status == AssetStatus::Destroying, Error::::Unknown); + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { T::Currency::unreserve(&owner, approval.deposit); @@ -776,6 +778,7 @@ pub mod pallet { ensure!(details.owner == check_owner, Error::::NoPermission); } ensure!(details.is_frozen, Error::::Unknown); + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); ensure!(details.accounts == 0, Error::::InUse); ensure!(details.approvals == 0, Error::::InUse); diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 4aaa0ecb9d1be..432c5bd50feea 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -84,10 +84,6 @@ impl pallet_balances::Config for Test { type ReserveIdentifier = [u8; 8]; } -parameter_types! { - pub const RemoveKeysLimit: u32 = 5; -} - impl Config for Test { type RuntimeEvent = RuntimeEvent; type Balance = u64; @@ -103,7 +99,7 @@ impl Config for Test { type Freezer = TestFreezer; type WeightInfo = (); type Extra = (); - type RemoveKeysLimit = RemoveKeysLimit; + type RemoveKeysLimit = ConstU32<5>; } use std::collections::HashMap; diff --git a/frame/transaction-payment/asset-tx-payment/src/tests.rs b/frame/transaction-payment/asset-tx-payment/src/tests.rs index 147cbe5dc6a90..c965b791f5a66 100644 --- a/frame/transaction-payment/asset-tx-payment/src/tests.rs +++ b/frame/transaction-payment/asset-tx-payment/src/tests.rs @@ -152,10 +152,6 @@ impl pallet_transaction_payment::Config for Runtime { type OperationalFeeMultiplier = ConstU8<5>; } -parameter_types! { - pub const RemoveKeysLimit: u32 = 1000; -} - impl pallet_assets::Config for Runtime { type RuntimeEvent = RuntimeEvent; type Balance = Balance; @@ -171,7 +167,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = (); - type RemoveKeysLimit = RemoveKeysLimit; + type RemoveKeysLimit = ConstU32<1000>; } pub struct HardcodedAuthor; From 6ceb5925c44ba1a023dd062eda10ee1fa15e5bc1 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 29 Sep 2022 15:27:03 +0200 Subject: [PATCH 18/62] use NotFrozen error --- frame/assets/src/lib.rs | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index dadc69f98d249..de3f4f706cbcc 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -499,6 +499,8 @@ pub mod pallet { /// The asset is a live asset and is actively being used. Usually emit for operations such /// as `start_destroy` which require the asset to be in a destroying state. LiveAsset, + /// The asset should be frozen before the given operation. + NotFrozen, } #[pallet::call] @@ -619,7 +621,7 @@ pub mod pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } - ensure!(details.is_frozen, Error::::BadWitness); + ensure!(details.is_frozen, Error::::NotFrozen); details.status = AssetStatus::Destroying; Self::deposit_event(Event::DestructionStarted { asset_id: id }); @@ -662,7 +664,7 @@ pub mod pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } - ensure!(details.is_frozen, Error::::BadWitness); + ensure!(details.is_frozen, Error::::NotFrozen); // Should only destroy accounts while the asset is being destroyed ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); @@ -699,9 +701,6 @@ pub mod pallet { /// times to fully destroy all approvals. It will destroy `RemoveKeysLimit` approvals at a /// time. /// - /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the - /// owner of the asset `id`. - /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing /// asset. /// @@ -711,21 +710,13 @@ pub mod pallet { origin: OriginFor, #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { - let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { - Ok(_) => None, - Err(origin) => Some(ensure_signed(origin)?), - }; - let mut removed_approvals = 0; let _ = Asset::::try_mutate_exists( id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - ensure!(details.is_frozen, Error::::BadWitness); + ensure!(details.is_frozen, Error::::NotFrozen); // Should only destroy accounts while the asset is being destroyed ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); @@ -777,7 +768,7 @@ pub mod pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } - ensure!(details.is_frozen, Error::::Unknown); + ensure!(details.is_frozen, Error::::NotFrozen); ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); ensure!(details.accounts == 0, Error::::InUse); ensure!(details.approvals == 0, Error::::InUse); From 2edac522c9a5e6c5a7b5e9bc16d7b6e8c4c7a780 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 29 Sep 2022 17:36:01 +0200 Subject: [PATCH 19/62] remove origin checks, as anyone can complete destruction after owner has begun the process; Add live check for other extrinsics --- frame/assets/src/functions.rs | 10 +++++++++- frame/assets/src/lib.rs | 24 ++++++++++-------------- frame/assets/src/tests.rs | 12 ------------ 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index a8203342e9ae7..425f37ccb4d05 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -205,6 +205,7 @@ impl, I: 'static> Pallet { keep_alive: bool, ) -> Result { let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(!details.is_frozen, Error::::Frozen); let account = Account::::get(id, who).ok_or(Error::::NoAccount)?; @@ -300,6 +301,7 @@ impl, I: 'static> Pallet { ensure!(!Account::::contains_key(id, &who), Error::::AlreadyExists); let deposit = T::AssetAccountDeposit::get(); let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); let reason = Self::new_account(&who, &mut details, Some(deposit))?; T::Currency::reserve(&who, deposit)?; Asset::::insert(&id, details); @@ -390,7 +392,7 @@ impl, I: 'static> Pallet { Self::can_increase(id, beneficiary, amount, true).into_result()?; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); check(details)?; Account::::try_mutate(id, beneficiary, |maybe_account| -> DispatchResult { @@ -436,6 +438,7 @@ impl, I: 'static> Pallet { ensure!(check_admin == details.admin, Error::::NoPermission); } + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); debug_assert!(details.supply >= actual, "checked in prep; qed"); details.supply = details.supply.saturating_sub(actual); @@ -472,6 +475,7 @@ impl, I: 'static> Pallet { Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); check(actual, details)?; @@ -552,6 +556,8 @@ impl, I: 'static> Pallet { Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + // Check admin rights. if let Some(need_admin) = maybe_need_admin { ensure!(need_admin == details.admin, Error::::NoPermission); @@ -670,6 +676,7 @@ impl, I: 'static> Pallet { amount: T::Balance, ) -> DispatchResult { let mut d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(!d.is_frozen, Error::::Frozen); Approvals::::try_mutate( (id, &owner, &delegate), @@ -766,6 +773,7 @@ impl, I: 'static> Pallet { symbol.clone().try_into().map_err(|_| Error::::BadMetadata)?; let d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(from == &d.owner, Error::::NoPermission); Metadata::::try_mutate_exists(id, |metadata| { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index de3f4f706cbcc..8437a67680a11 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -499,6 +499,8 @@ pub mod pallet { /// The asset is a live asset and is actively being used. Usually emit for operations such /// as `start_destroy` which require the asset to be in a destroying state. LiveAsset, + /// The asset is not live, and likely being destroyed. + AssetNotLive, /// The asset should be frozen before the given operation. NotFrozen, } @@ -651,19 +653,13 @@ pub mod pallet { origin: OriginFor, #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { - let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { - Ok(_) => None, - Err(origin) => Some(ensure_signed(origin)?), - }; + let _ = ensure_signed(origin)?; let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; let _ = Asset::::try_mutate_exists( id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } ensure!(details.is_frozen, Error::::NotFrozen); // Should only destroy accounts while the asset is being destroyed ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); @@ -710,6 +706,7 @@ pub mod pallet { origin: OriginFor, #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { + let _ = ensure_signed(origin)?; let mut removed_approvals = 0; let _ = Asset::::try_mutate_exists( id, @@ -757,17 +754,11 @@ pub mod pallet { origin: OriginFor, #[pallet::compact] id: T::AssetId, ) -> DispatchResult { - let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { - Ok(_) => None, - Err(origin) => Some(ensure_signed(origin)?), - }; + let _ = ensure_signed(origin)?; let _ = Asset::::try_mutate_exists( id, |maybe_details| -> Result<(), DispatchError> { let details = maybe_details.take().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } ensure!(details.is_frozen, Error::::NotFrozen); ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); ensure!(details.accounts == 0, Error::::InUse); @@ -959,6 +950,7 @@ pub mod pallet { let origin = ensure_signed(origin)?; let d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(origin == d.freezer, Error::::NoPermission); let who = T::Lookup::lookup(who)?; @@ -990,6 +982,7 @@ pub mod pallet { let origin = ensure_signed(origin)?; let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(origin == details.admin, Error::::NoPermission); let who = T::Lookup::lookup(who)?; @@ -1308,6 +1301,7 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_asset| { let mut asset = maybe_asset.take().ok_or(Error::::Unknown)?; + ensure!(asset.status == AssetStatus::Live, Error::::AssetNotLive); asset.owner = T::Lookup::lookup(owner)?; asset.issuer = T::Lookup::lookup(issuer)?; asset.admin = T::Lookup::lookup(admin)?; @@ -1376,6 +1370,7 @@ pub mod pallet { let owner = ensure_signed(origin)?; let delegate = T::Lookup::lookup(delegate)?; let mut d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); let approval = Approvals::::take((id, &owner, &delegate)).ok_or(Error::::Unknown)?; T::Currency::unreserve(&owner, approval.deposit); @@ -1408,6 +1403,7 @@ pub mod pallet { delegate: AccountIdLookupOf, ) -> DispatchResult { let mut d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); T::ForceOrigin::try_origin(origin) .map(|_| ()) .or_else(|origin| -> DispatchResult { diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index d20719b7f52a0..6e67ed2ef0100 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -624,18 +624,6 @@ fn origin_guards_should_work() { Assets::start_destroy(RuntimeOrigin::signed(2), 0), Error::::NoPermission ); - assert_noop!( - Assets::destroy_accounts(RuntimeOrigin::signed(2), 0), - Error::::NoPermission - ); - assert_noop!( - Assets::destroy_approvals(RuntimeOrigin::signed(2), 0), - Error::::NoPermission - ); - assert_noop!( - Assets::finish_destroy(RuntimeOrigin::signed(2), 0), - Error::::NoPermission - ); }); } From aa2aecf4bfc164adae4bae17dbd5fb917ef8e470 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 29 Sep 2022 17:58:20 +0200 Subject: [PATCH 20/62] fix comments about Origin behaviour --- frame/assets/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 8437a67680a11..dac1f9e9c7d5f 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -641,9 +641,6 @@ pub mod pallet { /// times to fully destroy all accounts. It will destroy `RemoveKeysLimit` accounts at a /// time. /// - /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the - /// owner of the asset `id`. - /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing /// asset. /// @@ -742,9 +739,6 @@ pub mod pallet { /// asset is in a `Destroying` state. All accounts or approvals should be destroyed before /// hand. /// - /// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the - /// owner of the asset `id`. - /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing /// asset. /// From 10fdd9090b69ff6e42a49ef2c203ea080d92020b Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 29 Sep 2022 18:27:18 +0200 Subject: [PATCH 21/62] add AssetStatus docs --- frame/assets/src/types.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 50a56c4b0746c..f42d4640f9a1e 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -29,6 +29,8 @@ pub(super) type DepositBalanceOf = pub(super) type AssetAccountOf = AssetAccount<>::Balance, DepositBalanceOf, >::Extra>; +/// AssetStatus holds the current state of the asset. It could either be Live and available for use, +/// or in a Destroying state. #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, MaxEncodedLen, TypeInfo)] pub(super) enum AssetStatus { /// The asset is active and able to be used. From 139d2e9235ce45501140cdff8b2158f29efe8be8 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 5 Oct 2022 13:00:19 +0200 Subject: [PATCH 22/62] modularize logic to allow calling logic in on_idle and on_initialize hooks --- frame/assets/src/functions.rs | 113 ++++++++++++++++++++++++++++++++++ frame/assets/src/lib.rs | 100 ++---------------------------- 2 files changed, 118 insertions(+), 95 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 425f37ccb4d05..6d09ffb1b4e54 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -665,6 +665,119 @@ impl, I: 'static> Pallet { Ok(()) } + /// Start the process of destroying an asset, by setting the asset status to Destroying, and + /// emitting the DestructionStarted event. + pub(super) fn do_start_destroy( + id: T::AssetId, + maybe_check_owner: Option, + ) -> DispatchResult { + let _ = + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::NotFrozen); + details.status = AssetStatus::Destroying; + + Self::deposit_event(Event::DestructionStarted { asset_id: id }); + Ok(()) + })?; + Ok(()) + } + + /// Destroy accounts associated with a given asset up to the max (T::RemoveKeysLimit). + /// + /// Each call emits the `Event::DestroyedAccountss` event. + pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { + let mut dead_accounts: Vec = vec![]; + let mut removed_accounts = 0; + let _ = + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(details.is_frozen, Error::::NotFrozen); + // Should only destroy accounts while the asset is being destroyed + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + + for (who, v) in Account::::drain_prefix(id) { + let _ = Self::dead_account(&who, &mut details, &v.reason, true); + dead_accounts.push(who); + removed_accounts = removed_accounts.saturating_add(1); + if removed_accounts >= T::RemoveKeysLimit::get() { + break + } + } + + Self::deposit_event(Event::DestroyedAccounts { + asset_id: id, + accounts_destroyed: removed_accounts as u32, + accounts_remaining: details.accounts as u32, + }); + + Ok(()) + })?; + + for who in dead_accounts { + T::Freezer::died(id, &who); + } + Ok(removed_accounts) + } + + /// Destroy approvals associated with a given asset up to the max (T::RemoveKeysLimit). + /// + /// Each call emits the `Event::DestroyedApprovals` event + pub(super) fn do_destroy_approvals(id: T::AssetId) -> Result { + let mut removed_approvals = 0; + let _ = + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + + ensure!(details.is_frozen, Error::::NotFrozen); + // Should only destroy accounts while the asset is being destroyed + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + + for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { + T::Currency::unreserve(&owner, approval.deposit); + removed_approvals = removed_approvals.saturating_add(1); + details.approvals = details.approvals.saturating_sub(1); + if removed_approvals >= T::RemoveKeysLimit::get() { + break + } + } + Self::deposit_event(Event::DestroyedApprovals { + asset_id: id, + approvals_destroyed: removed_approvals as u32, + approvals_remaining: details.approvals as u32, + }); + Ok(()) + })?; + Ok(removed_approvals) + } + + /// Complete destrouing asset and unreserve the currency. + /// + /// On success, the `Event::Destroyed` event is emitted. + pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult { + let _ = + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let details = maybe_details.take().ok_or(Error::::Unknown)?; + ensure!(details.is_frozen, Error::::NotFrozen); + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + ensure!(details.accounts == 0, Error::::InUse); + ensure!(details.approvals == 0, Error::::InUse); + + let metadata = Metadata::::take(&id); + T::Currency::unreserve( + &details.owner, + details.deposit.saturating_add(metadata.deposit), + ); + Self::deposit_event(Event::Destroyed { asset_id: id }); + + Ok(()) + })?; + Ok(()) + } + /// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate' /// while reserving `T::ApprovalDeposit` from owner /// diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index dac1f9e9c7d5f..ed30f56459cdd 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -616,21 +616,7 @@ pub mod pallet { Ok(_) => None, Err(origin) => Some(ensure_signed(origin)?), }; - let _ = Asset::::try_mutate_exists( - id, - |maybe_details| -> Result<(), DispatchError> { - let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - ensure!(details.is_frozen, Error::::NotFrozen); - details.status = AssetStatus::Destroying; - - Self::deposit_event(Event::DestructionStarted { asset_id: id }); - Ok(()) - }, - )?; - Ok(()) + Self::do_start_destroy(id, maybe_check_owner) } /// Destroy all accounts associated with a given asset. @@ -651,42 +637,11 @@ pub mod pallet { #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; - let mut dead_accounts: Vec = vec![]; - let mut removed_accounts = 0; - let _ = Asset::::try_mutate_exists( - id, - |maybe_details| -> Result<(), DispatchError> { - let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); - // Should only destroy accounts while the asset is being destroyed - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); - - for (who, v) in Account::::drain_prefix(id) { - let _ = Self::dead_account(&who, &mut details, &v.reason, true); - dead_accounts.push(who); - removed_accounts = removed_accounts.saturating_add(1); - if removed_accounts >= T::RemoveKeysLimit::get() { - break - } - } - - Self::deposit_event(Event::DestroyedAccounts { - asset_id: id, - accounts_destroyed: removed_accounts as u32, - accounts_remaining: details.accounts as u32, - }); - - Ok(()) - }, - )?; - - for who in dead_accounts { - T::Freezer::died(id, &who); - } + let removed_accounts = Self::do_destroy_accounts(id)?; Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into()) } - /// Destroy all approvals associated with a given asset. + /// Destroy all approvals associated with a given asset up to the max (T::RemoveKeysLimit), /// `destroy_approvals` should only be called after `start_destroy` has been called, and the /// asset is in a `Destroying` state /// @@ -704,33 +659,7 @@ pub mod pallet { #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; - let mut removed_approvals = 0; - let _ = Asset::::try_mutate_exists( - id, - |maybe_details| -> Result<(), DispatchError> { - let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - - ensure!(details.is_frozen, Error::::NotFrozen); - // Should only destroy accounts while the asset is being destroyed - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); - - for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { - T::Currency::unreserve(&owner, approval.deposit); - removed_approvals = removed_approvals.saturating_add(1); - details.approvals = details.approvals.saturating_sub(1); - if removed_approvals >= T::RemoveKeysLimit::get() { - break - } - } - Self::deposit_event(Event::DestroyedApprovals { - asset_id: id, - approvals_destroyed: removed_approvals as u32, - approvals_remaining: details.approvals as u32, - }); - Ok(()) - }, - )?; - + let removed_approvals = Self::do_destroy_approvals(id)?; Ok(Some(T::WeightInfo::destroy_approvals(removed_approvals)).into()) } @@ -749,26 +678,7 @@ pub mod pallet { #[pallet::compact] id: T::AssetId, ) -> DispatchResult { let _ = ensure_signed(origin)?; - let _ = Asset::::try_mutate_exists( - id, - |maybe_details| -> Result<(), DispatchError> { - let details = maybe_details.take().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); - ensure!(details.accounts == 0, Error::::InUse); - ensure!(details.approvals == 0, Error::::InUse); - - let metadata = Metadata::::take(&id); - T::Currency::unreserve( - &details.owner, - details.deposit.saturating_add(metadata.deposit), - ); - Self::deposit_event(Event::Destroyed { asset_id: id }); - - Ok(()) - }, - )?; - Ok(()) + Self::do_finish_destroy(id) } /// Mint assets of a particular class. From 597f532752357cc6a5ca79e4555043e10f96a584 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 6 Oct 2022 15:04:04 +0200 Subject: [PATCH 23/62] introduce simple migration for assets details --- frame/assets/src/lib.rs | 1 + frame/assets/src/migration.rs | 97 +++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 frame/assets/src/migration.rs diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index ed30f56459cdd..f3c698a08da1b 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -128,6 +128,7 @@ #[cfg(feature = "runtime-benchmarks")] mod benchmarking; +pub mod migration; #[cfg(test)] pub mod mock; #[cfg(test)] diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs new file mode 100644 index 0000000000000..e8054fbf99cb3 --- /dev/null +++ b/frame/assets/src/migration.rs @@ -0,0 +1,97 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// use crate::{Config, Pallet, Weight}; +// use frame_support::{ +// storage::migration, +// traits::{Get, PalletInfoAccess}, +// }; +// use sp_std::prelude::*; + +use super::*; +use frame_support::traits::{ + GetStorageVersion, OnRuntimeUpgrade, PalletInfoAccess, StorageVersion, +}; +use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; + +pub mod v1 { + use frame_support::{pallet_prelude::*, weights::Weight}; + + use super::*; + + #[derive(Decode)] + pub struct OldAssetDetails { + pub owner: AccountId, + pub issuer: AccountId, + pub admin: AccountId, + pub freezer: AccountId, + pub supply: Balance, + pub deposit: DepositBalance, + pub min_balance: Balance, + pub is_sufficient: bool, + pub accounts: u32, + pub sufficients: u32, + pub approvals: u32, + pub is_frozen: bool, + } + + impl OldAssetDetails { + fn migrate_to_v1(self) -> AssetDetails { + AssetDetails { + owner: self.owner, + issuer: self.issuer, + admin: self.admin, + freezer: self.freezer, + supply: self.supply, + deposit: self.deposit, + min_balance: self.min_balance, + is_sufficient: self.is_sufficient, + accounts: self.accounts, + sufficients: self.sufficients, + approvals: self.approvals, + is_frozen: self.is_frozen, + status: AssetStatus::Live, + } + } + } + + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let current_version = Pallet::::current_storage_version(); + let onchain_version = Pallet::::on_chain_storage_version(); + if onchain_version < 1 { + let mut translated = 0u64; + Asset::::translate::< + OldAssetDetails>, + _, + >(|_key, old_value| { + translated.saturating_inc(); + Some(old_value.migrate_to_v1()) + }); + current_version.put::>(); + // log::info!(target: "runtime::assets", "Upgraded {} pools, storage to version + // {:?}", tranlated, current); + T::DbWeight::get().reads_writes(translated + 1, translated + 1) + } else { + // log::info!(target: "runtime::assets", "Migration did not execute. This probably + // should be removed"); + T::DbWeight::get().reads(1) + } + } + } +} From 70930ac26b454358c0eb1951c95d89f0920aa361 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 7 Oct 2022 15:07:49 +0200 Subject: [PATCH 24/62] reintroduce logging in the migrations --- frame/assets/src/migration.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index e8054fbf99cb3..1bd8341bbdc3d 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -23,10 +23,7 @@ // use sp_std::prelude::*; use super::*; -use frame_support::traits::{ - GetStorageVersion, OnRuntimeUpgrade, PalletInfoAccess, StorageVersion, -}; -use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; +use frame_support::{log, traits::OnRuntimeUpgrade}; pub mod v1 { use frame_support::{pallet_prelude::*, weights::Weight}; @@ -84,12 +81,10 @@ pub mod v1 { Some(old_value.migrate_to_v1()) }); current_version.put::>(); - // log::info!(target: "runtime::assets", "Upgraded {} pools, storage to version - // {:?}", tranlated, current); + log::info!(target: "runtime::assets", "Upgraded {} pools, storage to version {:?}", translated, current_version); T::DbWeight::get().reads_writes(translated + 1, translated + 1) } else { - // log::info!(target: "runtime::assets", "Migration did not execute. This probably - // should be removed"); + log::info!(target: "runtime::assets", "Migration did not execute. This probably should be removed"); T::DbWeight::get().reads(1) } } From dc4a24335f3433a7845d0b84fdb352e2d0fc1d6f Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 12 Oct 2022 12:22:17 +0200 Subject: [PATCH 25/62] move deposit_Event out of the mutate block --- frame/assets/src/functions.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 6d09ffb1b4e54..90f5ace4d7ff5 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -692,6 +692,7 @@ impl, I: 'static> Pallet { pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; + let mut remaining_accounts = 0; let _ = Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; @@ -707,19 +708,19 @@ impl, I: 'static> Pallet { break } } - - Self::deposit_event(Event::DestroyedAccounts { - asset_id: id, - accounts_destroyed: removed_accounts as u32, - accounts_remaining: details.accounts as u32, - }); - + remaining_accounts = details.accounts; Ok(()) })?; for who in dead_accounts { T::Freezer::died(id, &who); } + + Self::deposit_event(Event::DestroyedAccounts { + asset_id: id, + accounts_destroyed: removed_accounts as u32, + accounts_remaining: remaining_accounts as u32, + }); Ok(removed_accounts) } From 05d778ddbfa3a6908a4939f3cfc7cbe7016e6a74 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 12 Oct 2022 12:23:35 +0200 Subject: [PATCH 26/62] Update frame/assets/src/functions.rs Co-authored-by: Muharem Ismailov --- frame/assets/src/functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 90f5ace4d7ff5..3f934c217d08c 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -688,7 +688,7 @@ impl, I: 'static> Pallet { /// Destroy accounts associated with a given asset up to the max (T::RemoveKeysLimit). /// - /// Each call emits the `Event::DestroyedAccountss` event. + /// Each call emits the `Event::DestroyedAccounts` event. pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; From 62d4f139e540cf5684ca8f081feb39869f03951a Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 12 Oct 2022 12:24:09 +0200 Subject: [PATCH 27/62] Update frame/assets/src/migration.rs Co-authored-by: Muharem Ismailov --- frame/assets/src/migration.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 1bd8341bbdc3d..77fbb6c176f2e 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -15,13 +15,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -// use crate::{Config, Pallet, Weight}; -// use frame_support::{ -// storage::migration, -// traits::{Get, PalletInfoAccess}, -// }; -// use sp_std::prelude::*; - use super::*; use frame_support::{log, traits::OnRuntimeUpgrade}; From e9e108bfe25e1e26c9b733f6a6305fb10d9c680a Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 12 Oct 2022 12:33:18 +0200 Subject: [PATCH 28/62] move AssetNotLive checkout out of the mutate blocks --- frame/assets/src/functions.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 90f5ace4d7ff5..6b1591adcf198 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -432,13 +432,15 @@ impl, I: 'static> Pallet { maybe_check_admin: Option, f: DebitFlags, ) -> Result { + let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + let actual = Self::decrease_balance(id, target, amount, f, |actual, details| { // Check admin rights. if let Some(check_admin) = maybe_check_admin { ensure!(check_admin == details.admin, Error::::NoPermission); } - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); debug_assert!(details.supply >= actual, "checked in prep; qed"); details.supply = details.supply.saturating_sub(actual); @@ -470,13 +472,14 @@ impl, I: 'static> Pallet { return Ok(amount) } + let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + let actual = Self::prep_debit(id, target, amount, f)?; let mut target_died: Option = None; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); - check(actual, details)?; Account::::try_mutate(id, target, |maybe_account| -> DispatchResult { @@ -544,6 +547,8 @@ impl, I: 'static> Pallet { if amount.is_zero() { return Ok((amount, None)) } + let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); // Figure out the debit and credit, together with side-effects. let debit = Self::prep_debit(id, source, amount, f.into())?; @@ -556,8 +561,6 @@ impl, I: 'static> Pallet { Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); - // Check admin rights. if let Some(need_admin) = maybe_need_admin { ensure!(need_admin == details.admin, Error::::NoPermission); From 544ac52318a2116b0d865ce201cc963cab8265fc Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 13 Oct 2022 14:38:22 +0200 Subject: [PATCH 29/62] rename RemoveKeysLimit to RemoveItemsLimit --- bin/node/runtime/src/lib.rs | 2 +- frame/assets/src/benchmarking.rs | 4 ++-- frame/assets/src/functions.rs | 8 ++++---- frame/assets/src/lib.rs | 16 ++++++++++------ frame/assets/src/mock.rs | 2 +- .../asset-tx-payment/src/tests.rs | 2 +- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 60736a7a81dca..212decb58a8e0 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1438,7 +1438,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = pallet_assets::weights::SubstrateWeight; - type RemoveKeysLimit = ConstU32<1000>; + type RemoveItemsLimit = ConstU32<1000>; } parameter_types! { diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index d85e21f590d70..dfeb43363f8ae 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -178,7 +178,7 @@ benchmarks_instance_pallet! { } destroy_accounts { - let c in 0 .. T::RemoveKeysLimit::get(); + let c in 0 .. T::RemoveItemsLimit::get(); let (caller, _) = create_default_asset::(true); add_sufficients::(caller.clone(), c); Assets::::freeze_asset( @@ -196,7 +196,7 @@ benchmarks_instance_pallet! { } destroy_approvals { - let a in 0 .. T::RemoveKeysLimit::get(); + let a in 0 .. T::RemoveItemsLimit::get(); let (caller, _) = create_default_minted_asset::(true, 100u32.into()); add_approvals::(caller.clone(), a); Assets::::freeze_asset( diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 751aa06c7ad11..b5e8ed08a114b 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -689,7 +689,7 @@ impl, I: 'static> Pallet { Ok(()) } - /// Destroy accounts associated with a given asset up to the max (T::RemoveKeysLimit). + /// Destroy accounts associated with a given asset up to the max (T::RemoveItemsLimit). /// /// Each call emits the `Event::DestroyedAccounts` event. pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { @@ -707,7 +707,7 @@ impl, I: 'static> Pallet { let _ = Self::dead_account(&who, &mut details, &v.reason, true); dead_accounts.push(who); removed_accounts = removed_accounts.saturating_add(1); - if removed_accounts >= T::RemoveKeysLimit::get() { + if removed_accounts >= T::RemoveItemsLimit::get() { break } } @@ -727,7 +727,7 @@ impl, I: 'static> Pallet { Ok(removed_accounts) } - /// Destroy approvals associated with a given asset up to the max (T::RemoveKeysLimit). + /// Destroy approvals associated with a given asset up to the max (T::RemoveItemsLimit). /// /// Each call emits the `Event::DestroyedApprovals` event pub(super) fn do_destroy_approvals(id: T::AssetId) -> Result { @@ -744,7 +744,7 @@ impl, I: 'static> Pallet { T::Currency::unreserve(&owner, approval.deposit); removed_approvals = removed_approvals.saturating_add(1); details.approvals = details.approvals.saturating_sub(1); - if removed_approvals >= T::RemoveKeysLimit::get() { + if removed_approvals >= T::RemoveItemsLimit::get() { break } } diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index f3c698a08da1b..b1b1d8ffbd4a1 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -176,8 +176,12 @@ pub mod pallet { use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; + /// The current storage version. + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); #[pallet::config] @@ -199,7 +203,7 @@ pub mod pallet { /// Max number of storage keys to destroy per extrinsic call. #[pallet::constant] - type RemoveKeysLimit: Get; + type RemoveItemsLimit: Get; /// Identifier for the class of asset. type AssetId: Member @@ -625,14 +629,14 @@ pub mod pallet { /// asset is in a `Destroying` state /// /// Due to weight restrictions, this function may need to be called multiple - /// times to fully destroy all accounts. It will destroy `RemoveKeysLimit` accounts at a + /// times to fully destroy all accounts. It will destroy `RemoveItemsLimit` accounts at a /// time. /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing /// asset. /// /// Each call Emits the `Event::DestroyedAccounts` event. - #[pallet::weight(T::WeightInfo::destroy_accounts(T::RemoveKeysLimit::get()))] + #[pallet::weight(T::WeightInfo::destroy_accounts(T::RemoveItemsLimit::get()))] pub fn destroy_accounts( origin: OriginFor, #[pallet::compact] id: T::AssetId, @@ -642,19 +646,19 @@ pub mod pallet { Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into()) } - /// Destroy all approvals associated with a given asset up to the max (T::RemoveKeysLimit), + /// Destroy all approvals associated with a given asset up to the max (T::RemoveItemsLimit), /// `destroy_approvals` should only be called after `start_destroy` has been called, and the /// asset is in a `Destroying` state /// /// Due to weight restrictions, this function may need to be called multiple - /// times to fully destroy all approvals. It will destroy `RemoveKeysLimit` approvals at a + /// times to fully destroy all approvals. It will destroy `RemoveItemsLimit` approvals at a /// time. /// /// - `id`: The identifier of the asset to be destroyed. This must identify an existing /// asset. /// /// Each call Emits the `Event::DestroyedApprovals` event. - #[pallet::weight(T::WeightInfo::destroy_approvals(T::RemoveKeysLimit::get()))] + #[pallet::weight(T::WeightInfo::destroy_approvals(T::RemoveItemsLimit::get()))] pub fn destroy_approvals( origin: OriginFor, #[pallet::compact] id: T::AssetId, diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 432c5bd50feea..1512fb61863c8 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -99,7 +99,7 @@ impl Config for Test { type Freezer = TestFreezer; type WeightInfo = (); type Extra = (); - type RemoveKeysLimit = ConstU32<5>; + type RemoveItemsLimit = ConstU32<5>; } use std::collections::HashMap; diff --git a/frame/transaction-payment/asset-tx-payment/src/tests.rs b/frame/transaction-payment/asset-tx-payment/src/tests.rs index a02c2afb0b937..8b01d6a6fdf18 100644 --- a/frame/transaction-payment/asset-tx-payment/src/tests.rs +++ b/frame/transaction-payment/asset-tx-payment/src/tests.rs @@ -167,7 +167,7 @@ impl pallet_assets::Config for Runtime { type Freezer = (); type Extra = (); type WeightInfo = (); - type RemoveKeysLimit = ConstU32<1000>; + type RemoveItemsLimit = ConstU32<1000>; } pub struct HardcodedAuthor; From 9206acf8f4e29d402873111f141d403e88ca2673 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 13 Oct 2022 14:40:09 +0200 Subject: [PATCH 30/62] update docs --- frame/assets/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index b1b1d8ffbd4a1..096a13ad6221d 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -201,7 +201,8 @@ pub mod pallet { + MaxEncodedLen + TypeInfo; - /// Max number of storage keys to destroy per extrinsic call. + /// Max number of items to destroy per extrinsic call. + /// This number should be less than the value that can fit in a block for the various extrinsics #[pallet::constant] type RemoveItemsLimit: Get; From 29c77454ccc0510b873e362bc3db9c605ec22baf Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 11:14:24 +0200 Subject: [PATCH 31/62] fix event name in benchmark --- frame/assets/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index dfeb43363f8ae..f0757f2d3c891 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -174,7 +174,7 @@ benchmarks_instance_pallet! { )?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { - assert_last_event::(Event::Destroying { asset_id: Default::default() }.into()); + assert_last_event::(Event::DestructionStarted { asset_id: Default::default() }.into()); } destroy_accounts { From 2986f29c3d4d91dcaa741a8113a4df95bfbf9cbe Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 11:25:07 +0200 Subject: [PATCH 32/62] fix cargo fmt. --- frame/assets/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 096a13ad6221d..cc64f899ce19c 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -202,7 +202,8 @@ pub mod pallet { + TypeInfo; /// Max number of items to destroy per extrinsic call. - /// This number should be less than the value that can fit in a block for the various extrinsics + /// This number should be less than the value that can fit in a block for the various + /// extrinsics #[pallet::constant] type RemoveItemsLimit: Get; From 6ac84a129c5f34dbd1c588e1c2408eeba2ce0ade Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 11:34:46 +0200 Subject: [PATCH 33/62] fix lint in benchmarking --- frame/assets/src/benchmarking.rs | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index f0757f2d3c891..cd625f55813ae 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -78,25 +78,6 @@ fn swap_is_sufficient, I: 'static>(s: &mut bool) { }); } -fn add_consumers, I: 'static>(minter: T::AccountId, n: u32) { - let origin = SystemOrigin::Signed(minter); - let mut s = false; - swap_is_sufficient::(&mut s); - for i in 0..n { - let target = account("consumer", i, SEED); - T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance()); - let target_lookup = T::Lookup::unlookup(target); - assert!(Assets::::mint( - origin.clone().into(), - Default::default(), - target_lookup, - 100u32.into() - ) - .is_ok()); - } - swap_is_sufficient::(&mut s); -} - fn add_sufficients, I: 'static>(minter: T::AccountId, n: u32) { let origin = SystemOrigin::Signed(minter); let mut s = true; @@ -185,7 +166,7 @@ benchmarks_instance_pallet! { SystemOrigin::Signed(caller.clone()).into(), Default::default(), )?; - Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { assert_last_event::(Event::DestroyedAccounts { @@ -203,7 +184,7 @@ benchmarks_instance_pallet! { SystemOrigin::Signed(caller.clone()).into(), Default::default(), )?; - Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { assert_last_event::(Event::DestroyedApprovals { @@ -219,7 +200,7 @@ benchmarks_instance_pallet! { SystemOrigin::Signed(caller.clone()).into(), Default::default(), )?; - Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default()); + Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { assert_last_event::(Event::Destroyed { From 791aa7f21357e65969eb5e7f3021937a68e11217 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:33:13 +0200 Subject: [PATCH 34/62] Empty commit to trigger CI From 450a698b7fb41a290c2355d91b7000d3db55c26d Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:46:33 +0200 Subject: [PATCH 35/62] Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index cc64f899ce19c..c9e074edf0836 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -434,7 +434,6 @@ pub mod pallet { DestructionStarted { asset_id: T::AssetId }, /// An asset class was destroyed. Destroyed { asset_id: T::AssetId }, - /// Some asset class was force-created. ForceCreated { asset_id: T::AssetId, owner: T::AccountId }, /// New metadata has been set for an asset. From 3daef795d98d49392c4d0c4ea7652e2beeecb20f Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:46:49 +0200 Subject: [PATCH 36/62] Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index c9e074edf0836..26f271092db3a 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -641,6 +641,7 @@ pub mod pallet { pub fn destroy_accounts( origin: OriginFor, #[pallet::compact] id: T::AssetId, + max_items: u32, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; let removed_accounts = Self::do_destroy_accounts(id)?; From ea34cf507fa3d8f52200aa68d1aab41c73c9197f Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:50:51 +0200 Subject: [PATCH 37/62] Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/functions.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index b5e8ed08a114b..7e5cdcae8f3f8 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -692,6 +692,7 @@ impl, I: 'static> Pallet { /// Destroy accounts associated with a given asset up to the max (T::RemoveItemsLimit). /// /// Each call emits the `Event::DestroyedAccounts` event. + /// Returns the number of destroyed accounts. pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; From 84cbb47f0c82b681ff5579850715d033a7d4c0df Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:51:03 +0200 Subject: [PATCH 38/62] Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 7e5cdcae8f3f8..e16996b3c64db 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -759,7 +759,7 @@ impl, I: 'static> Pallet { Ok(removed_approvals) } - /// Complete destrouing asset and unreserve the currency. + /// Complete destroying an asset and unreserve the deposit. /// /// On success, the `Event::Destroyed` event is emitted. pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult { From 082a10c41ce1d0b7188a5c27bb9e78145e9bf054 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:51:17 +0200 Subject: [PATCH 39/62] Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/functions.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index e16996b3c64db..38428c1e4d663 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -779,8 +779,7 @@ impl, I: 'static> Pallet { Self::deposit_event(Event::Destroyed { asset_id: id }); Ok(()) - })?; - Ok(()) + }) } /// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate' From e84d5ae30c181dbb63b4df8136de139c35cfc488 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 14:51:24 +0200 Subject: [PATCH 40/62] Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 26f271092db3a..0158272a86591 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -201,9 +201,9 @@ pub mod pallet { + MaxEncodedLen + TypeInfo; - /// Max number of items to destroy per extrinsic call. - /// This number should be less than the value that can fit in a block for the various - /// extrinsics + /// Max number of items to destroy per `destroy_accounts` and `destroy_approvals` call. + /// + /// Must be configured to result in a weight that makes each call fit in a block. #[pallet::constant] type RemoveItemsLimit: Get; From 78cde15cbc6609368b47c70de0b25445528c4884 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 15:27:39 +0200 Subject: [PATCH 41/62] Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/functions.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 38428c1e4d663..24e337a15d99a 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -731,6 +731,7 @@ impl, I: 'static> Pallet { /// Destroy approvals associated with a given asset up to the max (T::RemoveItemsLimit). /// /// Each call emits the `Event::DestroyedApprovals` event + /// Returns the number of destroyed approvals. pub(super) fn do_destroy_approvals(id: T::AssetId) -> Result { let mut removed_approvals = 0; let _ = From c6470fb9eeb798bc20e39fb32ff83b6c4eca5b5a Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 17:17:01 +0200 Subject: [PATCH 42/62] effect change suggested during code review --- frame/assets/src/functions.rs | 55 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 24e337a15d99a..fa6afe3374184 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -674,25 +674,23 @@ impl, I: 'static> Pallet { id: T::AssetId, maybe_check_owner: Option, ) -> DispatchResult { - let _ = - Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { - let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - ensure!(details.is_frozen, Error::::NotFrozen); - details.status = AssetStatus::Destroying; + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.is_frozen, Error::::NotFrozen); + details.status = AssetStatus::Destroying; - Self::deposit_event(Event::DestructionStarted { asset_id: id }); - Ok(()) - })?; - Ok(()) + Self::deposit_event(Event::DestructionStarted { asset_id: id }); + Ok(()) + }) } /// Destroy accounts associated with a given asset up to the max (T::RemoveItemsLimit). /// /// Each call emits the `Event::DestroyedAccounts` event. - /// Returns the number of destroyed accounts. + /// Returns the number of destroyed accounts. pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; @@ -764,23 +762,22 @@ impl, I: 'static> Pallet { /// /// On success, the `Event::Destroyed` event is emitted. pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult { - let _ = - Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { - let details = maybe_details.take().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); - ensure!(details.accounts == 0, Error::::InUse); - ensure!(details.approvals == 0, Error::::InUse); - - let metadata = Metadata::::take(&id); - T::Currency::unreserve( - &details.owner, - details.deposit.saturating_add(metadata.deposit), - ); - Self::deposit_event(Event::Destroyed { asset_id: id }); + Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { + let details = maybe_details.take().ok_or(Error::::Unknown)?; + ensure!(details.is_frozen, Error::::NotFrozen); + ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + ensure!(details.accounts == 0, Error::::InUse); + ensure!(details.approvals == 0, Error::::InUse); + + let metadata = Metadata::::take(&id); + T::Currency::unreserve( + &details.owner, + details.deposit.saturating_add(metadata.deposit), + ); + Self::deposit_event(Event::Destroyed { asset_id: id }); - Ok(()) - }) + Ok(()) + }) } /// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate' From 85e50b9c155eb50aa89fcef524b5919f00632ed2 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 14 Oct 2022 18:15:19 +0200 Subject: [PATCH 43/62] move limit to a single location --- frame/assets/src/functions.rs | 14 ++++++++++---- frame/assets/src/lib.rs | 5 ++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index fa6afe3374184..e16c72c3ea15b 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -691,7 +691,10 @@ impl, I: 'static> Pallet { /// /// Each call emits the `Event::DestroyedAccounts` event. /// Returns the number of destroyed accounts. - pub(super) fn do_destroy_accounts(id: T::AssetId) -> Result { + pub(super) fn do_destroy_accounts( + id: T::AssetId, + max_items: u32, + ) -> Result { let mut dead_accounts: Vec = vec![]; let mut removed_accounts = 0; let mut remaining_accounts = 0; @@ -706,7 +709,7 @@ impl, I: 'static> Pallet { let _ = Self::dead_account(&who, &mut details, &v.reason, true); dead_accounts.push(who); removed_accounts = removed_accounts.saturating_add(1); - if removed_accounts >= T::RemoveItemsLimit::get() { + if removed_accounts >= max_items { break } } @@ -730,7 +733,10 @@ impl, I: 'static> Pallet { /// /// Each call emits the `Event::DestroyedApprovals` event /// Returns the number of destroyed approvals. - pub(super) fn do_destroy_approvals(id: T::AssetId) -> Result { + pub(super) fn do_destroy_approvals( + id: T::AssetId, + max_items: u32, + ) -> Result { let mut removed_approvals = 0; let _ = Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { @@ -744,7 +750,7 @@ impl, I: 'static> Pallet { T::Currency::unreserve(&owner, approval.deposit); removed_approvals = removed_approvals.saturating_add(1); details.approvals = details.approvals.saturating_sub(1); - if removed_approvals >= T::RemoveItemsLimit::get() { + if removed_approvals >= max_items { break } } diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 0158272a86591..e0546c1022cd4 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -641,10 +641,9 @@ pub mod pallet { pub fn destroy_accounts( origin: OriginFor, #[pallet::compact] id: T::AssetId, - max_items: u32, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; - let removed_accounts = Self::do_destroy_accounts(id)?; + let removed_accounts = Self::do_destroy_accounts(id, T::RemoveItemsLimit::get())?; Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into()) } @@ -666,7 +665,7 @@ pub mod pallet { #[pallet::compact] id: T::AssetId, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; - let removed_approvals = Self::do_destroy_approvals(id)?; + let removed_approvals = Self::do_destroy_approvals(id, T::RemoveItemsLimit::get())?; Ok(Some(T::WeightInfo::destroy_approvals(removed_approvals)).into()) } From df27e0bc47977aff879261356070ca78ebc1da0d Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 18 Oct 2022 13:15:22 +0200 Subject: [PATCH 44/62] Update frame/assets/src/functions.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- frame/assets/src/functions.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index e16c72c3ea15b..cf81dda1d4298 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -668,8 +668,8 @@ impl, I: 'static> Pallet { Ok(()) } - /// Start the process of destroying an asset, by setting the asset status to Destroying, and - /// emitting the DestructionStarted event. + /// Start the process of destroying an asset, by setting the asset status to `Destroying`, and + /// emitting the `DestructionStarted` event. pub(super) fn do_start_destroy( id: T::AssetId, maybe_check_owner: Option, From ca9fa2efdd4a81a93c2e5ef64c821d23a9bfb0d1 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 18 Oct 2022 13:29:54 +0200 Subject: [PATCH 45/62] rename events --- frame/assets/src/functions.rs | 4 ++-- frame/assets/src/lib.rs | 4 ++-- frame/assets/src/tests.rs | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index cf81dda1d4298..427136d436b12 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -721,7 +721,7 @@ impl, I: 'static> Pallet { T::Freezer::died(id, &who); } - Self::deposit_event(Event::DestroyedAccounts { + Self::deposit_event(Event::AccountsDestroyed { asset_id: id, accounts_destroyed: removed_accounts as u32, accounts_remaining: remaining_accounts as u32, @@ -754,7 +754,7 @@ impl, I: 'static> Pallet { break } } - Self::deposit_event(Event::DestroyedApprovals { + Self::deposit_event(Event::ApprovalsDestroyed { asset_id: id, approvals_destroyed: removed_approvals as u32, approvals_remaining: details.approvals as u32, diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index e0546c1022cd4..a65daee86b73d 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -423,9 +423,9 @@ pub mod pallet { /// Some asset `asset_id` was thawed. AssetThawed { asset_id: T::AssetId }, /// Accounts were destroyed for given asset. - DestroyedAccounts { asset_id: T::AssetId, accounts_destroyed: u32, accounts_remaining: u32 }, + AccountsDestroyed { asset_id: T::AssetId, accounts_destroyed: u32, accounts_remaining: u32 }, /// Approvals were destroyed for given asset. - DestroyedApprovals { + ApprovalsDestroyed { asset_id: T::AssetId, approvals_destroyed: u32, approvals_remaining: u32, diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 6e67ed2ef0100..a910f7e251ae4 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -399,12 +399,12 @@ fn partial_destroy_should_work() { // cleaned up. assert_noop!(Assets::finish_destroy(RuntimeOrigin::signed(1), 0), Error::::InUse); - System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedAccounts { + System::assert_has_event(RuntimeEvent::Assets(crate::Event::AccountsDestroyed { asset_id: 0, accounts_destroyed: 5, accounts_remaining: 2, })); - System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedApprovals { + System::assert_has_event(RuntimeEvent::Assets(crate::Event::ApprovalsDestroyed { asset_id: 0, approvals_destroyed: 0, approvals_remaining: 0, @@ -414,7 +414,7 @@ fn partial_destroy_should_work() { // Second call to destroy on PartiallyDestroyed asset assert_ok!(Assets::destroy_accounts(RuntimeOrigin::signed(1), 0)); - System::assert_has_event(RuntimeEvent::Assets(crate::Event::DestroyedAccounts { + System::assert_has_event(RuntimeEvent::Assets(crate::Event::AccountsDestroyed { asset_id: 0, accounts_destroyed: 2, accounts_remaining: 0, From 9609f139d23c00bfe95183a07294152baa8c4154 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 18 Oct 2022 16:39:08 +0200 Subject: [PATCH 46/62] fix weight typo, using rocksdb instead of T::DbWeight. Pending generating weights --- frame/assets/src/weights.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/assets/src/weights.rs b/frame/assets/src/weights.rs index 60a6a0e0caeea..46f2f09186174 100644 --- a/frame/assets/src/weights.rs +++ b/frame/assets/src/weights.rs @@ -103,10 +103,10 @@ impl WeightInfo for SubstrateWeight { Weight::from_ref_time(37_000_000 as u64) // Standard Error: 19_301 .saturating_add(Weight::from_ref_time(25_467_908 as u64).saturating_mul(c as u64)) - .saturating_add(RocksDbWeight::get().reads(2 as u64)) - .saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().writes(1 as u64)) - .saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64))) + .saturating_add(T::DbWeight::get().reads(2 as u64)) + .saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(c as u64))) + .saturating_add(T::DbWeight::get().writes(1 as u64)) + .saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(c as u64))) } // Storage: Assets Asset (r:1 w:1) From 26d27ccca03849017a95c00c2b6f28f8fa950e25 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Tue, 18 Oct 2022 18:03:20 +0200 Subject: [PATCH 47/62] switch to using dead_account.len() --- frame/assets/src/functions.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 427136d436b12..c25cc8eb6686e 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -696,7 +696,6 @@ impl, I: 'static> Pallet { max_items: u32, ) -> Result { let mut dead_accounts: Vec = vec![]; - let mut removed_accounts = 0; let mut remaining_accounts = 0; let _ = Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { @@ -708,8 +707,7 @@ impl, I: 'static> Pallet { for (who, v) in Account::::drain_prefix(id) { let _ = Self::dead_account(&who, &mut details, &v.reason, true); dead_accounts.push(who); - removed_accounts = removed_accounts.saturating_add(1); - if removed_accounts >= max_items { + if dead_accounts.len() >= (max_items as usize) { break } } @@ -717,16 +715,16 @@ impl, I: 'static> Pallet { Ok(()) })?; - for who in dead_accounts { + for who in &dead_accounts { T::Freezer::died(id, &who); } Self::deposit_event(Event::AccountsDestroyed { asset_id: id, - accounts_destroyed: removed_accounts as u32, + accounts_destroyed: dead_accounts.len() as u32, accounts_remaining: remaining_accounts as u32, }); - Ok(removed_accounts) + Ok(dead_accounts.len() as u32) } /// Destroy approvals associated with a given asset up to the max (T::RemoveItemsLimit). From 72ec8f81c45dbafb47073b8a1c4304fab9d4d8cd Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 20 Oct 2022 15:35:29 +0200 Subject: [PATCH 48/62] rename event in the benchmarks --- frame/assets/src/benchmarking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index cd625f55813ae..89e340068fecd 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -169,7 +169,7 @@ benchmarks_instance_pallet! { Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { - assert_last_event::(Event::DestroyedAccounts { + assert_last_event::(Event::AccountsDestroyed { asset_id: Default::default() , accounts_destroyed: c, accounts_remaining: 0, @@ -187,7 +187,7 @@ benchmarks_instance_pallet! { Assets::::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?; }:_(SystemOrigin::Signed(caller), Default::default()) verify { - assert_last_event::(Event::DestroyedApprovals { + assert_last_event::(Event::ApprovalsDestroyed { asset_id: Default::default() , approvals_destroyed: a, approvals_remaining: 0, From ee30cf8583ec4169ec8bebb31cf24f59b660db7b Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 20 Oct 2022 16:37:42 +0200 Subject: [PATCH 49/62] empty to retrigger CI From 773212e69709beb2f3292ed20495f12dce1b8afa Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 20 Oct 2022 17:51:53 +0200 Subject: [PATCH 50/62] trigger CI to check cumulus dependency From a4be57ffec60f2886f590bb38098869c900a9112 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 20 Oct 2022 19:37:52 +0200 Subject: [PATCH 51/62] trigger CI for dependent cumulus From 9c06bb8345e8b7c75d70053ed1ff847716910609 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Mon, 24 Oct 2022 16:16:22 +0200 Subject: [PATCH 52/62] Update frame/assets/src/migration.rs Co-authored-by: Oliver Tale-Yazdi --- frame/assets/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 77fbb6c176f2e..35181d742e0c9 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -64,7 +64,7 @@ pub mod v1 { fn on_runtime_upgrade() -> Weight { let current_version = Pallet::::current_storage_version(); let onchain_version = Pallet::::on_chain_storage_version(); - if onchain_version < 1 { + if onchain_version == 0 && current_version == 1 { let mut translated = 0u64; Asset::::translate::< OldAssetDetails>, From 4eef0691f2374736ea30dd1036f9f67ffad37b84 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 26 Oct 2022 17:39:16 +0200 Subject: [PATCH 53/62] move is-frozen to the assetStatus enum (#12547) --- frame/assets/src/functions.rs | 25 ++++++++++--------------- frame/assets/src/lib.rs | 15 +++++++++------ frame/assets/src/migration.rs | 5 +++-- frame/assets/src/types.rs | 4 ++-- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index c25cc8eb6686e..f87af72965831 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -157,7 +157,7 @@ impl, I: 'static> Pallet { if details.supply.checked_sub(&amount).is_none() { return Underflow } - if details.is_frozen { + if details.status == AssetStatus::Frozen { return Frozen } if amount.is_zero() { @@ -205,8 +205,8 @@ impl, I: 'static> Pallet { keep_alive: bool, ) -> Result { let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); - ensure!(!details.is_frozen, Error::::Frozen); + ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); + ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); let account = Account::::get(id, who).ok_or(Error::::NoAccount)?; ensure!(!account.is_frozen, Error::::Frozen); @@ -325,7 +325,7 @@ impl, I: 'static> Pallet { let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!(account.balance.is_zero() || allow_burn, Error::::WouldBurn); - ensure!(!details.is_frozen, Error::::Frozen); + ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); ensure!(!account.is_frozen, Error::::Frozen); T::Currency::unreserve(&who, deposit); @@ -392,7 +392,7 @@ impl, I: 'static> Pallet { Self::can_increase(id, beneficiary, amount, true).into_result()?; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); check(details)?; Account::::try_mutate(id, beneficiary, |maybe_account| -> DispatchResult { @@ -433,7 +433,7 @@ impl, I: 'static> Pallet { f: DebitFlags, ) -> Result { let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); let actual = Self::decrease_balance(id, target, amount, f, |actual, details| { // Check admin rights. @@ -660,7 +660,6 @@ impl, I: 'static> Pallet { accounts: 0, sufficients: 0, approvals: 0, - is_frozen: false, status: AssetStatus::Live, }, ); @@ -679,7 +678,6 @@ impl, I: 'static> Pallet { if let Some(check_owner) = maybe_check_owner { ensure!(details.owner == check_owner, Error::::NoPermission); } - ensure!(details.is_frozen, Error::::NotFrozen); details.status = AssetStatus::Destroying; Self::deposit_event(Event::DestructionStarted { asset_id: id }); @@ -700,8 +698,7 @@ impl, I: 'static> Pallet { let _ = Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); - // Should only destroy accounts while the asset is being destroyed + // Should only destroy accounts while the asset is in a destroying state ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); for (who, v) in Account::::drain_prefix(id) { @@ -740,8 +737,7 @@ impl, I: 'static> Pallet { Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); - // Should only destroy accounts while the asset is being destroyed + // Should only destroy accounts while the asset is in a destroying state. ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { @@ -768,7 +764,6 @@ impl, I: 'static> Pallet { pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult { Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let details = maybe_details.take().ok_or(Error::::Unknown)?; - ensure!(details.is_frozen, Error::::NotFrozen); ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); ensure!(details.accounts == 0, Error::::InUse); ensure!(details.approvals == 0, Error::::InUse); @@ -795,8 +790,8 @@ impl, I: 'static> Pallet { amount: T::Balance, ) -> DispatchResult { let mut d = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); - ensure!(!d.is_frozen, Error::::Frozen); + ensure!(d.status != AssetStatus::Destroying, Error::::AssetNotLive); + ensure!(d.status != AssetStatus::Frozen, Error::::Frozen); Approvals::::try_mutate( (id, &owner, &delegate), |maybe_approved| -> DispatchResult { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index a65daee86b73d..e2690373bd4e2 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -346,7 +346,6 @@ pub mod pallet { accounts: 0, sufficients: 0, approvals: 0, - is_frozen: false, status: AssetStatus::Live, }, ); @@ -562,7 +561,6 @@ pub mod pallet { accounts: 0, sufficients: 0, approvals: 0, - is_frozen: false, status: AssetStatus::Live, }, ); @@ -925,7 +923,7 @@ pub mod pallet { let d = maybe_details.as_mut().ok_or(Error::::Unknown)?; ensure!(origin == d.freezer, Error::::NoPermission); - d.is_frozen = true; + d.status = AssetStatus::Frozen; Self::deposit_event(Event::::AssetFrozen { asset_id: id }); Ok(()) @@ -951,8 +949,9 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_details| { let d = maybe_details.as_mut().ok_or(Error::::Unknown)?; ensure!(origin == d.admin, Error::::NoPermission); + ensure!(d.status != AssetStatus::Destroying, Error::::AssetNotLive); - d.is_frozen = false; + d.status = AssetStatus::Live; Self::deposit_event(Event::::AssetThawed { asset_id: id }); Ok(()) @@ -1211,14 +1210,18 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_asset| { let mut asset = maybe_asset.take().ok_or(Error::::Unknown)?; - ensure!(asset.status == AssetStatus::Live, Error::::AssetNotLive); + ensure!(asset.status != AssetStatus::Destroying, Error::::AssetNotLive); asset.owner = T::Lookup::lookup(owner)?; asset.issuer = T::Lookup::lookup(issuer)?; asset.admin = T::Lookup::lookup(admin)?; asset.freezer = T::Lookup::lookup(freezer)?; asset.min_balance = min_balance; asset.is_sufficient = is_sufficient; - asset.is_frozen = is_frozen; + if is_frozen { + asset.status = AssetStatus::Frozen; + } else { + asset.status = AssetStatus::Live; + } *maybe_asset = Some(asset); Self::deposit_event(Event::AssetStatusChanged { asset_id: id }); diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 35181d742e0c9..887ad8c7bb856 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -41,6 +41,8 @@ pub mod v1 { impl OldAssetDetails { fn migrate_to_v1(self) -> AssetDetails { + let status = if self.is_frozen { AssetStatus::Frozen } else { AssetStatus::Live }; + AssetDetails { owner: self.owner, issuer: self.issuer, @@ -53,8 +55,7 @@ pub mod v1 { accounts: self.accounts, sufficients: self.sufficients, approvals: self.approvals, - is_frozen: self.is_frozen, - status: AssetStatus::Live, + status, } } } diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index f42d4640f9a1e..557af6bd3f488 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -35,6 +35,8 @@ pub(super) type AssetAccountOf = pub(super) enum AssetStatus { /// The asset is active and able to be used. Live, + /// Whether the asset is frozen for non-admin transfers. + Frozen, /// The asset is currently being destroyed, and all actions are no longer permitted on the /// asset. Once set to `Destroying`, the asset can never transition back to a `Live` state. Destroying, @@ -65,8 +67,6 @@ pub struct AssetDetails { pub(super) sufficients: u32, /// The total number of approvals. pub(super) approvals: u32, - /// Whether the asset is frozen for non-admin transfers. - pub(super) is_frozen: bool, /// The status of the asset pub(super) status: AssetStatus, } From 486814f61166f61f4bbdd177a971091290fd1dc6 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 26 Oct 2022 18:35:17 +0200 Subject: [PATCH 54/62] add pre and post migration hooks --- frame/assets/src/migration.rs | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 887ad8c7bb856..89f8d39a9049c 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -82,5 +82,41 @@ pub mod v1 { T::DbWeight::get().reads(1) } } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + frame_support::ensure!( + Pallet::::on_chain_storage_version() == 0, + "must upgrade linearly" + ); + let prev_count = Asset::::iter().count(); + Ok((prev_count as u32).encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { + let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( + "the state parameter should be something that was generated by pre_upgrade", + ); + let post_count = Asset::::iter().count() as u32; + assert_eq!( + prev_count, post_count, + "the asset count before and after the migration should be the same" + ); + + let current_version = Pallet::::current_storage_version(); + let onchain_version = Pallet::::on_chain_storage_version(); + + frame_support::ensure!(current_version == 1, "must_upgrade"); + assert_eq!( + current_version, onchain_version, + "after migration, the current_version and onchain_version should be the same" + ); + + Asset::::iter().for_each(|(_id, asset)| { + assert!(asset.status == AssetStatus::Live || asset.status == AssetStatus::Frozen, "assets should only be live or frozen. None should be in destroying status, or undefined state") + }); + Ok(()) + } } } From c185cb081b27df1a0a9c1383143828b075758084 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 26 Oct 2022 19:05:54 +0200 Subject: [PATCH 55/62] update do_transfer logic to add new assert for more correct error messages --- frame/assets/src/functions.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index f87af72965831..bd6e2da14e095 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -548,6 +548,7 @@ impl, I: 'static> Pallet { return Ok((amount, None)) } let details = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); // Figure out the debit and credit, together with side-effects. From 91095affbfce55a4227f3ea40b6640a91cfa9b62 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Wed, 26 Oct 2022 19:23:09 +0200 Subject: [PATCH 56/62] trigger CI From 9ef3c2b6db8a0cf2f75cd1ec44ca6b77e2892000 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 27 Oct 2022 17:19:28 +0200 Subject: [PATCH 57/62] switch checking AssetStatus from checking Destroying state to checking live state --- frame/assets/src/functions.rs | 19 +++++++++++-------- frame/assets/src/lib.rs | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index bd6e2da14e095..c4b9ee27e2560 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -323,9 +323,8 @@ impl, I: 'static> Pallet { let mut account = Account::::get(id, &who).ok_or(Error::::NoDeposit)?; let deposit = account.reason.take_deposit().ok_or(Error::::NoDeposit)?; let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; - + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(account.balance.is_zero() || allow_burn, Error::::WouldBurn); - ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); ensure!(!account.is_frozen, Error::::Frozen); T::Currency::unreserve(&who, deposit); @@ -392,7 +391,7 @@ impl, I: 'static> Pallet { Self::can_increase(id, beneficiary, amount, true).into_result()?; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; - ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); check(details)?; Account::::try_mutate(id, beneficiary, |maybe_account| -> DispatchResult { @@ -432,8 +431,11 @@ impl, I: 'static> Pallet { maybe_check_admin: Option, f: DebitFlags, ) -> Result { - let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); + let d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!( + d.status == AssetStatus::Live || d.status == AssetStatus::Frozen, + Error::::AssetNotLive + ); let actual = Self::decrease_balance(id, target, amount, f, |actual, details| { // Check admin rights. @@ -548,7 +550,6 @@ impl, I: 'static> Pallet { return Ok((amount, None)) } let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); // Figure out the debit and credit, together with side-effects. @@ -791,8 +792,7 @@ impl, I: 'static> Pallet { amount: T::Balance, ) -> DispatchResult { let mut d = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(d.status != AssetStatus::Destroying, Error::::AssetNotLive); - ensure!(d.status != AssetStatus::Frozen, Error::::Frozen); + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); Approvals::::try_mutate( (id, &owner, &delegate), |maybe_approved| -> DispatchResult { @@ -842,6 +842,9 @@ impl, I: 'static> Pallet { ) -> DispatchResult { let mut owner_died: Option = None; + let d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); + Approvals::::try_mutate_exists( (id, &owner, delegate), |maybe_approved| -> DispatchResult { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index e2690373bd4e2..e9b8d2fda7eb8 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -858,7 +858,10 @@ pub mod pallet { let origin = ensure_signed(origin)?; let d = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); + ensure!( + d.status == AssetStatus::Live || d.status == AssetStatus::Frozen, + Error::::AssetNotLive + ); ensure!(origin == d.freezer, Error::::NoPermission); let who = T::Lookup::lookup(who)?; @@ -890,7 +893,10 @@ pub mod pallet { let origin = ensure_signed(origin)?; let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); + ensure!( + details.status == AssetStatus::Live || details.status == AssetStatus::Frozen, + Error::::AssetNotLive + ); ensure!(origin == details.admin, Error::::NoPermission); let who = T::Lookup::lookup(who)?; @@ -921,6 +927,7 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_details| { let d = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(origin == d.freezer, Error::::NoPermission); d.status = AssetStatus::Frozen; @@ -949,7 +956,7 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_details| { let d = maybe_details.as_mut().ok_or(Error::::Unknown)?; ensure!(origin == d.admin, Error::::NoPermission); - ensure!(d.status != AssetStatus::Destroying, Error::::AssetNotLive); + ensure!(d.status == AssetStatus::Frozen, Error::::NotFrozen); d.status = AssetStatus::Live; @@ -979,6 +986,7 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_details| { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::LiveAsset); ensure!(origin == details.owner, Error::::NoPermission); if details.owner == owner { return Ok(()) @@ -1024,6 +1032,7 @@ pub mod pallet { Asset::::try_mutate(id, |maybe_details| { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(origin == details.owner, Error::::NoPermission); details.issuer = issuer.clone(); @@ -1082,6 +1091,7 @@ pub mod pallet { let origin = ensure_signed(origin)?; let d = Asset::::get(id).ok_or(Error::::Unknown)?; + ensure!(d.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(origin == d.owner, Error::::NoPermission); Metadata::::try_mutate_exists(id, |metadata| { From 6165576f22ef3ed228bd3c641580b5e6a16c7b31 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 27 Oct 2022 18:02:00 +0200 Subject: [PATCH 58/62] fix error type in tests from Frozen to AssetNotLive --- frame/assets/src/tests.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index a910f7e251ae4..bd36168853039 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -570,7 +570,10 @@ fn transferring_frozen_asset_should_not_work() { assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_eq!(Assets::balance(0, 1), 100); assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); - assert_noop!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50), Error::::Frozen); + assert_noop!( + Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50), + Error::::AssetNotLive + ); assert_ok!(Assets::thaw_asset(RuntimeOrigin::signed(1), 0)); assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50)); }); @@ -586,7 +589,7 @@ fn approve_transfer_frozen_asset_should_not_work() { assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0)); assert_noop!( Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50), - Error::::Frozen + Error::::AssetNotLive ); assert_ok!(Assets::thaw_asset(RuntimeOrigin::signed(1), 0)); assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50)); From 7811b6209df96da1229cd4ef3ae3ca227b1d03ee Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Thu, 27 Oct 2022 18:31:42 +0200 Subject: [PATCH 59/62] trigger CI From cd3e28dd48185d17523e09a0fc21c0b5adb1aa77 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Fri, 28 Oct 2022 01:23:55 +0200 Subject: [PATCH 60/62] change ensure check for fn reducible_balance() --- frame/assets/src/functions.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index c4b9ee27e2560..530187755a4d2 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -205,8 +205,7 @@ impl, I: 'static> Pallet { keep_alive: bool, ) -> Result { let details = Asset::::get(id).ok_or(Error::::Unknown)?; - ensure!(details.status != AssetStatus::Destroying, Error::::AssetNotLive); - ensure!(details.status != AssetStatus::Frozen, Error::::Frozen); + ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); let account = Account::::get(id, who).ok_or(Error::::NoAccount)?; ensure!(!account.is_frozen, Error::::Frozen); From aee596e2ce10b87848306b772d72a852855782a4 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Mon, 7 Nov 2022 13:29:14 +0200 Subject: [PATCH 61/62] change the error type to Error:::IncorrectStatus to be clearer --- frame/assets/src/functions.rs | 6 +++--- frame/assets/src/lib.rs | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 530187755a4d2..f7f11cafecbe2 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -700,7 +700,7 @@ impl, I: 'static> Pallet { Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; // Should only destroy accounts while the asset is in a destroying state - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + ensure!(details.status == AssetStatus::Destroying, Error::::IncorrectStatus); for (who, v) in Account::::drain_prefix(id) { let _ = Self::dead_account(&who, &mut details, &v.reason, true); @@ -739,7 +739,7 @@ impl, I: 'static> Pallet { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; // Should only destroy accounts while the asset is in a destroying state. - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + ensure!(details.status == AssetStatus::Destroying, Error::::IncorrectStatus); for ((owner, _), approval) in Approvals::::drain_prefix((id,)) { T::Currency::unreserve(&owner, approval.deposit); @@ -765,7 +765,7 @@ impl, I: 'static> Pallet { pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult { Asset::::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> { let details = maybe_details.take().ok_or(Error::::Unknown)?; - ensure!(details.status == AssetStatus::Destroying, Error::::LiveAsset); + ensure!(details.status == AssetStatus::Destroying, Error::::IncorrectStatus); ensure!(details.accounts == 0, Error::::InUse); ensure!(details.approvals == 0, Error::::InUse); diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index e9b8d2fda7eb8..8f4851f579025 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -506,6 +506,8 @@ pub mod pallet { LiveAsset, /// The asset is not live, and likely being destroyed. AssetNotLive, + /// The asset status is not the expected status. + IncorrectStatus, /// The asset should be frozen before the given operation. NotFrozen, } From 30814df63dca111c9c47922650414ca476677b21 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Mon, 7 Nov 2022 13:57:53 +0200 Subject: [PATCH 62/62] Trigger CI