From b064f1a25c78d7478641f9527873d8f027c53d5e Mon Sep 17 00:00:00 2001 From: Rodrigo Quelhas <22591718+RomarQ@users.noreply.github.com> Date: Mon, 20 May 2024 10:41:51 +0100 Subject: [PATCH] remove localAssets lazy migration (#2797) * remove localAssets lazy migration * remove unused type --- pallets/moonbeam-lazy-migrations/src/lib.rs | 137 ------------------ pallets/moonbeam-lazy-migrations/src/tests.rs | 63 +------- runtime/moonbase/src/asset_config.rs | 3 - runtime/moonbase/tests/xcm_mock/parachain.rs | 26 ---- runtime/moonbeam/src/asset_config.rs | 3 - runtime/moonriver/src/asset_config.rs | 2 - .../test-lazy-migrations-storage-removal.ts | 95 ------------ .../smoke/test-foreign-asset-consistency.ts | 4 +- 8 files changed, 3 insertions(+), 330 deletions(-) delete mode 100644 test/suites/dev/moonbase/test-pov/test-lazy-migrations-storage-removal.ts diff --git a/pallets/moonbeam-lazy-migrations/src/lib.rs b/pallets/moonbeam-lazy-migrations/src/lib.rs index 3574ccf425..0258b3a6d0 100644 --- a/pallets/moonbeam-lazy-migrations/src/lib.rs +++ b/pallets/moonbeam-lazy-migrations/src/lib.rs @@ -37,25 +37,16 @@ pub use pallet::*; pub mod pallet { use super::*; use frame_support::pallet_prelude::*; - use frame_support::traits::ReservableCurrency; use frame_system::pallet_prelude::*; use sp_core::H160; pub const ARRAY_LIMIT: u32 = 1000; pub type GetArrayLimit = ConstU32; - const INTERMEDIATES_NODES_SIZE: u64 = 4096; - const MAX_LOCAL_ASSETS_STORAGE_ENTRY_SIZE: u64 = - (/* biggest key on moonbeam */136) + (/* biggest value on moonbeam */142); - /// Pallet for multi block migrations #[pallet::pallet] pub struct Pallet(PhantomData); - #[pallet::storage] - /// If true, it means that LocalAssets storage has been removed. - pub(crate) type LocalAssetsMigrationCompleted = StorageValue<_, bool, ValueQuery>; - #[pallet::storage] /// The total number of suicided contracts that were removed pub(crate) type SuicidedContractsRemoved = StorageValue<_, u32, ValueQuery>; @@ -68,16 +59,8 @@ pub mod pallet { #[pallet::error] pub enum Error { - /// There are no more storage entries to be removed - AllStorageEntriesHaveBeenRemoved, /// The limit cannot be zero LimitCannotBeZero, - /// The maximum number of assets cannot be zero - MaxAssetsCannotBeZero, - /// The limit for unlocking funds is too high - UnlockLimitTooHigh, - /// There are no more VotingOf entries to be removed and democracy funds to be unlocked - AllDemocracyFundsUnlocked, /// There must be at least one address AddressesLengthCannotBeZero, /// The contract is not corrupted (Still exist or properly suicided) @@ -86,126 +69,6 @@ pub mod pallet { #[pallet::call] impl Pallet { - // TODO(rodrigo): This extrinsic should be removed once LocalAssets pallet storage is removed - #[pallet::call_index(0)] - #[pallet::weight({ - // "*limit" is used twice to account to the possibility that we may need to unreserve - // deposits for every approval - let possible_iterations = max_assets.saturating_add(*limit).saturating_add(*limit); - let proof_size = INTERMEDIATES_NODES_SIZE + (MAX_LOCAL_ASSETS_STORAGE_ENTRY_SIZE - .saturating_mul(::from(possible_iterations))); - - Weight::from_parts(0, proof_size) - .saturating_add(::DbWeight::get() - .reads_writes((*max_assets + *limit + 1).into(), (*limit + 1).into())) - })] - pub fn clear_local_assets_storage( - origin: OriginFor, - max_assets: u32, - limit: u32, - ) -> DispatchResultWithPostInfo { - ensure_signed(origin)?; - ensure!(limit != 0, Error::::LimitCannotBeZero); - ensure!(max_assets != 0, Error::::MaxAssetsCannotBeZero); - - ensure!( - !LocalAssetsMigrationCompleted::::get(), - Error::::AllStorageEntriesHaveBeenRemoved - ); - - let mut allowed_removals = limit; - - const PALLET_PREFIX: &'static str = "LocalAssets"; - - struct LocalAssetsStorageAsset; - impl frame_support::traits::StorageInstance for LocalAssetsStorageAsset { - const STORAGE_PREFIX: &'static str = "Asset"; - fn pallet_prefix() -> &'static str { - PALLET_PREFIX - } - } - struct LocalAssetsStorageApprovals; - impl frame_support::traits::StorageInstance for LocalAssetsStorageApprovals { - const STORAGE_PREFIX: &'static str = "Approvals"; - fn pallet_prefix() -> &'static str { - PALLET_PREFIX - } - } - - /// Data concerning an approval. - /// Duplicated the type from pallet_assets (The original type is private) - #[derive( - Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default, MaxEncodedLen, TypeInfo, - )] - pub struct Approval { - /// The amount of funds approved for the balance transfer from the owner to some delegated - /// target. - pub(super) amount: Balance, - /// The amount reserved on the owner's account to hold this item in storage. - pub(super) deposit: DepositBalance, - } - - type AssetMap = frame_support::storage::types::StorageMap< - LocalAssetsStorageAsset, - Blake2_128Concat, - u128, - // It is fine to add a dummy `Value` type - // The value is not going to be decoded, since we only care about the keys) - (), - >; - - for asset_id in AssetMap::iter_keys().take(max_assets as usize) { - let approvals_iter = frame_support::storage::types::StorageNMap::< - LocalAssetsStorageApprovals, - ( - NMapKey, // asset id - NMapKey, // owner - NMapKey, // delegate - ), - Approval, - >::drain_prefix((asset_id,)); - for ((owner, _), approval) in approvals_iter { - allowed_removals = allowed_removals.saturating_sub(1); - // Unreserve deposit (most likely will be zero) - pallet_balances::Pallet::::unreserve(&owner, approval.deposit); - // Check if the removal limit was reached - if allowed_removals < 1 { - break; - } - } - // Remove asset, since it does not contain more approvals - AssetMap::remove(asset_id); - allowed_removals = allowed_removals.saturating_sub(1); - // Check if the removal limit was reached - if allowed_removals < 1 { - break; - } - } - - // Remove remaining storage - if allowed_removals > 0 { - let hashed_prefix = sp_io::hashing::twox_128(PALLET_PREFIX.as_bytes()); - - let keys_removed = - match sp_io::storage::clear_prefix(&hashed_prefix, Some(allowed_removals)) { - sp_io::KillStorageResult::AllRemoved(value) => { - LocalAssetsMigrationCompleted::::set(true); - value - } - sp_io::KillStorageResult::SomeRemaining(value) => value, - }; - - allowed_removals = allowed_removals.saturating_sub(keys_removed); - } - - log::info!( - "Removed {} storge keys 🧹", - limit.saturating_sub(allowed_removals) - ); - - Ok(Pays::No.into()) - } - // TODO(rodrigo): This extrinsic should be removed once the storage of destroyed contracts // has been removed #[pallet::call_index(1)] diff --git a/pallets/moonbeam-lazy-migrations/src/tests.rs b/pallets/moonbeam-lazy-migrations/src/tests.rs index 675d36f169..a0e1257ea4 100644 --- a/pallets/moonbeam-lazy-migrations/src/tests.rs +++ b/pallets/moonbeam-lazy-migrations/src/tests.rs @@ -20,7 +20,7 @@ use { mock::{ExtBuilder, LazyMigrations, RuntimeOrigin, Test}, Error, }, - frame_support::{assert_noop, assert_ok}, + frame_support::assert_noop, rlp::RlpStream, sp_core::{H160, H256}, sp_io::hashing::keccak_256, @@ -263,64 +263,3 @@ fn test_clear_suicided_mixed_suicided_and_non_suicided() { ); }) } - -/// TODO(rodrigo): This test should be removed once LocalAssets pallet storage is removed -#[test] -fn test_call_clear_local_assets_storage() { - let mut context = ExtBuilder::default().build(); - - let pallet_prefix = sp_io::hashing::twox_128("LocalAssets".as_bytes()); - let total_storage_entries: u8 = 5; - - let gen_dummy_entry = |dummy: u8| -> [u8; 32] { - [pallet_prefix, sp_io::hashing::twox_128(&[dummy])] - .concat() - .try_into() - .unwrap() - }; - - context.execute_with(|| { - for i in 0u8..total_storage_entries { - let dummy_data = gen_dummy_entry(i); - sp_io::storage::set(&dummy_data, &dummy_data); - dbg!(sp_io::storage::exists(&dummy_data)); - } - }); - - // Commit changes - let _ = context.commit_all(); - - // Next block - context.execute_with(|| { - // Check that the storage entries exist before attempting to remove it - for i in 0u8..total_storage_entries { - let dummy_data = gen_dummy_entry(i); - assert!(sp_io::storage::exists(&dummy_data)); - } - // Clear all storage entries - assert_ok!(LazyMigrations::clear_local_assets_storage( - RuntimeOrigin::signed(AccountId32::from([0; 32])), - 1, - total_storage_entries.into() - )); - // Check that all storage entries got deleted - for i in 0u8..total_storage_entries { - let dummy_data = gen_dummy_entry(i); - assert!(!sp_io::storage::exists(&dummy_data)); - } - }); - - // Commit changes - let _ = context.commit_all(); - - // Next block - context.execute_with(|| { - // No more storage entries to be removed (expect failure) - assert!(LazyMigrations::clear_local_assets_storage( - RuntimeOrigin::signed(AccountId32::from([0; 32])), - 1, - 1 - ) - .is_err()) - }); -} diff --git a/runtime/moonbase/src/asset_config.rs b/runtime/moonbase/src/asset_config.rs index ebb9aa45f5..27b87c4c3a 100644 --- a/runtime/moonbase/src/asset_config.rs +++ b/runtime/moonbase/src/asset_config.rs @@ -179,9 +179,6 @@ pub type ForeignAssetModifierOrigin = EitherOfDiverse< >, >; -pub type LocalAssetModifierOrigin = - EitherOfDiverse, governance::custom_origins::GeneralAdmin>; - impl pallet_asset_manager::Config for Runtime { type RuntimeEvent = RuntimeEvent; type Balance = Balance; diff --git a/runtime/moonbase/tests/xcm_mock/parachain.rs b/runtime/moonbase/tests/xcm_mock/parachain.rs index 20c6d4d477..ad4802344b 100644 --- a/runtime/moonbase/tests/xcm_mock/parachain.rs +++ b/runtime/moonbase/tests/xcm_mock/parachain.rs @@ -126,7 +126,6 @@ impl pallet_balances::Config for Runtime { } pub type ForeignAssetInstance = (); -pub type LocalAssetInstance = pallet_assets::Instance1; // Required for runtime benchmarks pallet_assets::runtime_benchmarks_enabled! { @@ -174,30 +173,6 @@ impl pallet_assets::Config for Runtime { } } -impl pallet_assets::Config for Runtime { - type RuntimeEvent = RuntimeEvent; - type Balance = Balance; - type AssetId = AssetId; - type Currency = Balances; - type ForceOrigin = EnsureRoot; - type AssetDeposit = AssetDeposit; - type MetadataDepositBase = MetadataDepositBase; - type MetadataDepositPerByte = MetadataDepositPerByte; - type ApprovalDeposit = ApprovalDeposit; - type StringLimit = AssetsStringLimit; - type Freezer = (); - type Extra = (); - type AssetAccountDeposit = AssetAccountDeposit; - type WeightInfo = pallet_assets::weights::SubstrateWeight; - type RemoveItemsLimit = ConstU32<656>; - type AssetIdParameter = AssetId; - type CreateOrigin = AsEnsureOriginWithArg>; - type CallbackHandle = (); - pallet_assets::runtime_benchmarks_enabled! { - type BenchmarkHelper = BenchmarkHelper; - } -} - /// Type for specifying how a `Location` can be converted into an `AccountId`. This is used /// when determining ownership of accounts for asset transacting and when attempting to use XCM /// `Transact` in order to determine the dispatch Origin. @@ -1092,7 +1067,6 @@ construct_runtime!( AssetManager: pallet_asset_manager, XcmTransactor: pallet_xcm_transactor, Treasury: pallet_treasury, - LocalAssets: pallet_assets::, Proxy: pallet_proxy, Timestamp: pallet_timestamp, diff --git a/runtime/moonbeam/src/asset_config.rs b/runtime/moonbeam/src/asset_config.rs index f99093eb24..200841c5b6 100644 --- a/runtime/moonbeam/src/asset_config.rs +++ b/runtime/moonbeam/src/asset_config.rs @@ -177,9 +177,6 @@ pub type ForeignAssetModifierOrigin = EitherOfDiverse< >, >; -pub type LocalAssetModifierOrigin = - EitherOfDiverse, governance::custom_origins::GeneralAdmin>; - impl pallet_asset_manager::Config for Runtime { type RuntimeEvent = RuntimeEvent; type Balance = Balance; diff --git a/runtime/moonriver/src/asset_config.rs b/runtime/moonriver/src/asset_config.rs index 5508414b22..7ecc6bd182 100644 --- a/runtime/moonriver/src/asset_config.rs +++ b/runtime/moonriver/src/asset_config.rs @@ -177,8 +177,6 @@ pub type ForeignAssetModifierOrigin = EitherOfDiverse< governance::custom_origins::GeneralAdmin, >, >; -pub type LocalAssetModifierOrigin = - EitherOfDiverse, governance::custom_origins::GeneralAdmin>; impl pallet_asset_manager::Config for Runtime { type RuntimeEvent = RuntimeEvent; diff --git a/test/suites/dev/moonbase/test-pov/test-lazy-migrations-storage-removal.ts b/test/suites/dev/moonbase/test-pov/test-lazy-migrations-storage-removal.ts deleted file mode 100644 index 4618456d99..0000000000 --- a/test/suites/dev/moonbase/test-pov/test-lazy-migrations-storage-removal.ts +++ /dev/null @@ -1,95 +0,0 @@ -import "@moonbeam-network/api-augment"; -import { beforeAll, describeSuite, expect } from "@moonwall/cli"; -import { alith, baltathar } from "@moonwall/util"; -import { ApiPromise } from "@polkadot/api"; -import { u128 } from "@polkadot/types"; - -describeSuite({ - id: "D012703", - title: "Remove LocalAssets storage - PoV Size validation", - foundationMethods: "dev", - testCases: ({ context, it, log }) => { - let assetId: u128; - let api: ApiPromise; - beforeAll(async () => { - api = context.polkadotJs(); - - await context.createBlock(api.tx.assets.transfer(assetId, baltathar.address, 1000)); - }); - - it({ - id: "T01", - title: "Validate storage removal uses a reasonable proof size", - test: async function () { - const max_assets = 1; - const total_entries = 9000; - // sp_io::hashing::twox_128("LocalAssets".as_bytes()); - const pallet_name_hash = "0xbebaa96ee6c1d0e946832368c6396271"; - - const dummy_storage: [string, string][] = []; - for (let i = 0; i < total_entries; i++) { - const dummy_data = i.toString(16).padStart(200, "0"); - dummy_storage.push([pallet_name_hash + dummy_data, dummy_data]); - } - - // Insert 500 entries per block to not reach limits - for (let i = 0; i < total_entries; i += 500) { - await context.createBlock( - api.tx.sudo - .sudo(api.tx.system.setStorage(dummy_storage.slice(i, i + 500))) - .signAsync(alith) - ); - } - - // Check the pallet storage size - const full_size = (await api.rpc.state.getStorageSize(pallet_name_hash)).toNumber(); - expect(full_size).to.be.equal(1_800_000); - - // editorconfig-checker-disable - // The constant `MAX_POV_SIZE` comes from: https://github.com/paritytech/polkadot-sdk/blob/b79bf4fb1fec1f7a7483f9a2baa0a1e7a4fcb9c8/polkadot/primitives/src/v6/mod.rs#L391 - // editorconfig-checker-enable - const MAX_POV_SIZE = 5 * 1024 * 1024; // 5MB - const reasonable_max_pov_size = MAX_POV_SIZE / 5; // 1MB - - let current_size = full_size; - while (current_size > 0) { - // The migration is not complet yet - expect( - (await api.query["moonbeamLazyMigrations"].localAssetsMigrationCompleted()).toHuman() - ).to.be.false; - - // Remove 2000 entries each time - const entries_to_remove = 2000; - const result = await context.createBlock( - api.tx["moonbeamLazyMigrations"] - .clearLocalAssetsStorage(max_assets, entries_to_remove) - .signAsync(alith) - ); - - // Validate that we are within the reasonable proof size - expect(result.block.proofSize).to.be.lessThan(reasonable_max_pov_size); - log( - `Removed ${entries_to_remove} entries from LocalAssets \ - storage (proof_size: ${result.block.proofSize}).` - ); - - // Validate that some storage got removed - const new_size = (await api.rpc.state.getStorageSize(pallet_name_hash)).toNumber(); - expect(new_size).to.be.lessThan(current_size); - - // Update current storage size - current_size = new_size; - } - - // Validate that the whole storage got removed - current_size = (await api.rpc.state.getStorageSize(pallet_name_hash)).toNumber(); - expect(current_size).to.be.equal(0); - - // The migration should be complete - expect( - (await api.query["moonbeamLazyMigrations"].localAssetsMigrationCompleted()).toHuman() - ).to.be.true; - }, - }); - }, -}); diff --git a/test/suites/smoke/test-foreign-asset-consistency.ts b/test/suites/smoke/test-foreign-asset-consistency.ts index de37683f0a..e2ac50813a 100644 --- a/test/suites/smoke/test-foreign-asset-consistency.ts +++ b/test/suites/smoke/test-foreign-asset-consistency.ts @@ -91,7 +91,7 @@ describeSuite({ expect( failedAssetReserveMappings.length, `Failed foreign asset entries: ${failedAssetReserveMappings - .map(({ assetId }) => `expected: ${assetId} to be present in localAssets `) + .map(({ assetId }) => `expected: ${assetId} to be present in foreignAssets `) .join(`, `)}` ).to.equal(0); log( @@ -117,7 +117,7 @@ describeSuite({ expect( failedXcmPaymentAssets.length, `Failed xcm fee assets: ${failedXcmPaymentAssets - .map(({ assetType }) => `expected: ${assetType} to be present in localAssets `) + .map(({ assetType }) => `expected: ${assetType} to be present in foreignAssets `) .join(`\n`)}` ).to.equal(0); log(