From ff3be276727632e6b9c5d779ae9bc3c638486530 Mon Sep 17 00:00:00 2001 From: Juan Date: Fri, 9 Jun 2023 13:04:31 +0200 Subject: [PATCH] Move `type Migrations` to `Config` (#14309) * move migrate sequence to config * remove commented out code * Update frame/contracts/src/lib.rs Co-authored-by: PG Herveou * remove Migrations generic * make runtime use noop migrations * restrict is_upgrade_supported * undo is_upgrade_supported change * Update bin/node/runtime/src/lib.rs Co-authored-by: PG Herveou * add rust doc example for `Migrations` * feature gate NoopMigration * fix example code * improve example --------- Co-authored-by: PG Herveou --- bin/node/runtime/src/lib.rs | 6 ++ frame/contracts/src/lib.rs | 12 ++- frame/contracts/src/migration.rs | 134 ++++++++++++++----------------- frame/contracts/src/tests.rs | 5 +- 4 files changed, 81 insertions(+), 76 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 6dc9841f6b44f..aad86b28de32f 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -55,6 +55,8 @@ use frame_system::{ pub use node_primitives::{AccountId, Signature}; use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Index, Moment}; use pallet_asset_conversion::{NativeOrAssetId, NativeOrAssetIdConverter}; +#[cfg(feature = "runtime-benchmarks")] +use pallet_contracts::NoopMigration; use pallet_election_provider_multi_phase::SolutionAccuracyOf; use pallet_im_online::sr25519::AuthorityId as ImOnlineId; use pallet_nfts::PalletFeatures; @@ -1240,6 +1242,10 @@ impl pallet_contracts::Config for Runtime { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; + #[cfg(not(feature = "runtime-benchmarks"))] + type Migrations = (); + #[cfg(feature = "runtime-benchmarks")] + type Migrations = (NoopMigration<1>, NoopMigration<2>); } impl pallet_sudo::Config for Runtime { diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 779b713795b64..42691d4b01dc7 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -87,12 +87,12 @@ mod address; mod benchmarking; mod exec; mod gas; -mod migration; mod schedule; mod storage; mod wasm; pub mod chain_extension; +pub mod migration; pub mod weights; #[cfg(test)] @@ -319,6 +319,16 @@ pub mod pallet { /// The maximum length of the debug buffer in bytes. #[pallet::constant] type MaxDebugBufferLen: Get; + + /// The sequence of migration steps that will be applied during a migration. + /// + /// # Examples + /// ``` + /// use pallet_contracts::migration::{v9, v10, v11}; + /// # struct Runtime {}; + /// type Migrations = (v9::Migration, v10::Migration, v11::Migration); + /// ``` + type Migrations: MigrateSequence; } #[pallet::hooks] diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index a75d5cc1a1f4d..ad85da0477148 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -17,21 +17,10 @@ //! Migration framework for pallets. -/// Macro to include all migration modules. -/// We only want to make these modules public when `runtime-benchmarks` is -/// enabled, so we can access migration code in benchmarks. -macro_rules! use_modules { - ($($module:ident),*) => { - $( - #[cfg(feature = "runtime-benchmarks")] - pub mod $module; - #[cfg(not(feature = "runtime-benchmarks"))] - mod $module; - )* - }; -} +pub mod v10; +pub mod v11; +pub mod v9; -use_modules!(v9, v10, v11); use crate::{weights::WeightInfo, Config, Error, MigrationInProgress, Pallet, Weight, LOG_TARGET}; use codec::{Codec, Decode}; use frame_support::{ @@ -58,10 +47,6 @@ fn invalid_version(version: StorageVersion) -> ! { /// The cursor used to store the state of the current migration step. pub type Cursor = BoundedVec>; -// In benchmark and tests we use noop migrations, to test and bench the migration framework itself. -#[cfg(not(any(feature = "runtime-benchmarks", test)))] -type Migrations = (v9::Migration, v10::Migration, v11::Migration); - /// IsFinished describes whether a migration is finished or not. pub enum IsFinished { Yes, @@ -206,23 +191,16 @@ pub trait MigrateSequence: private::Sealed { } /// Performs all necessary migrations based on `StorageVersion`. -#[cfg(not(any(feature = "runtime-benchmarks", test)))] -pub struct Migration>(PhantomData<(T, M)>); - -/// Custom migration for running runtime-benchmarks and tests. -#[cfg(any(feature = "runtime-benchmarks", test))] -pub struct Migration, NoopMigration<2>)>( - PhantomData<(T, M)>, -); +pub struct Migration(PhantomData); #[cfg(feature = "try-runtime")] -impl Migration { +impl Migration { fn run_all_steps() -> Result<(), TryRuntimeError> { let mut weight = Weight::zero(); let name = >::name(); loop { let in_progress_version = >::on_chain_storage_version() + 1; - let state = M::pre_upgrade_step(in_progress_version)?; + let state = T::Migrations::pre_upgrade_step(in_progress_version)?; let (status, w) = Self::migrate(Weight::MAX); weight.saturating_accrue(w); log::info!( @@ -231,7 +209,7 @@ impl Migration { in_progress_version, weight ); - M::post_upgrade_step(in_progress_version, state)?; + T::Migrations::post_upgrade_step(in_progress_version, state)?; if matches!(status, MigrateResult::Completed) { break } @@ -243,7 +221,7 @@ impl Migration { } } -impl OnRuntimeUpgrade for Migration { +impl OnRuntimeUpgrade for Migration { fn on_runtime_upgrade() -> Weight { let name = >::name(); let latest_version = >::current_storage_version(); @@ -274,7 +252,7 @@ impl OnRuntimeUpgrade for Migration { "{name}: Upgrading storage from {storage_version:?} to {latest_version:?}.", ); - let cursor = M::new(storage_version + 1); + let cursor = T::Migrations::new(storage_version + 1); MigrationInProgress::::set(Some(cursor)); #[cfg(feature = "try-runtime")] @@ -295,11 +273,14 @@ impl OnRuntimeUpgrade for Migration { target: LOG_TARGET, "{}: Range supported {:?}, range requested {:?}", >::name(), - M::VERSION_RANGE, + T::Migrations::VERSION_RANGE, (storage_version, target_version) ); - ensure!(M::is_upgrade_supported(storage_version, target_version), "Unsupported upgrade"); + ensure!( + T::Migrations::is_upgrade_supported(storage_version, target_version), + "Unsupported upgrade" + ); Ok(Default::default()) } } @@ -324,11 +305,11 @@ pub enum StepResult { Completed { steps_done: u32 }, } -impl Migration { - /// Verify that each migration's step of the MigrateSequence fits into `Cursor`. +impl Migration { + /// Verify that each migration's step of the [`T::Migrations`] sequence fits into `Cursor`. pub(crate) fn integrity_test() { let max_weight = ::BlockWeights::get().max_block; - M::integrity_test(max_weight) + T::Migrations::integrity_test(max_weight) } /// Migrate @@ -357,33 +338,36 @@ impl Migration { in_progress_version, ); - let result = - match M::steps(in_progress_version, cursor_before.as_ref(), &mut weight_left) { - StepResult::InProgress { cursor, steps_done } => { - *progress = Some(cursor); + let result = match T::Migrations::steps( + in_progress_version, + cursor_before.as_ref(), + &mut weight_left, + ) { + StepResult::InProgress { cursor, steps_done } => { + *progress = Some(cursor); + MigrateResult::InProgress { steps_done } + }, + StepResult::Completed { steps_done } => { + in_progress_version.put::>(); + if >::current_storage_version() != in_progress_version { + log::info!( + target: LOG_TARGET, + "{name}: Next migration is {:?},", + in_progress_version + 1 + ); + *progress = Some(T::Migrations::new(in_progress_version + 1)); MigrateResult::InProgress { steps_done } - }, - StepResult::Completed { steps_done } => { - in_progress_version.put::>(); - if >::current_storage_version() != in_progress_version { - log::info!( - target: LOG_TARGET, - "{name}: Next migration is {:?},", - in_progress_version + 1 - ); - *progress = Some(M::new(in_progress_version + 1)); - MigrateResult::InProgress { steps_done } - } else { - log::info!( - target: LOG_TARGET, - "{name}: All migrations done. At version {:?},", - in_progress_version - ); - *progress = None; - MigrateResult::Completed - } - }, - }; + } else { + log::info!( + target: LOG_TARGET, + "{name}: All migrations done. At version {:?},", + in_progress_version + ); + *progress = None; + MigrateResult::Completed + } + }, + }; (result, weight_limit.saturating_sub(weight_left)) }) @@ -527,11 +511,14 @@ mod test { #[test] fn is_upgrade_supported_works() { - type M = (MockMigration<9>, MockMigration<10>, MockMigration<11>); + type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>); [(1, 1), (8, 11), (9, 11)].into_iter().for_each(|(from, to)| { assert!( - M::is_upgrade_supported(StorageVersion::new(from), StorageVersion::new(to)), + Migrations::is_upgrade_supported( + StorageVersion::new(from), + StorageVersion::new(to) + ), "{} -> {} is supported", from, to @@ -540,7 +527,10 @@ mod test { [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { assert!( - !M::is_upgrade_supported(StorageVersion::new(from), StorageVersion::new(to)), + !Migrations::is_upgrade_supported( + StorageVersion::new(from), + StorageVersion::new(to) + ), "{} -> {} is not supported", from, to @@ -550,27 +540,26 @@ mod test { #[test] fn steps_works() { - type M = (MockMigration<2>, MockMigration<3>); + type Migrations = (MockMigration<2>, MockMigration<3>); let version = StorageVersion::new(2); - let mut cursor = M::new(version); + let mut cursor = Migrations::new(version); let mut weight = Weight::from_all(2); - let result = M::steps(version, &cursor, &mut weight); + let result = Migrations::steps(version, &cursor, &mut weight); cursor = vec![1u8, 0].try_into().unwrap(); assert_eq!(result, StepResult::InProgress { cursor: cursor.clone(), steps_done: 1 }); assert_eq!(weight, Weight::from_all(1)); let mut weight = Weight::from_all(2); assert_eq!( - M::steps(version, &cursor, &mut weight), + Migrations::steps(version, &cursor, &mut weight), StepResult::Completed { steps_done: 1 } ); } #[test] fn no_migration_in_progress_works() { - type M = (MockMigration<1>, MockMigration<2>); - type TestMigration = Migration; + type TestMigration = Migration; ExtBuilder::default().build().execute_with(|| { assert_eq!(StorageVersion::get::>(), 2); @@ -580,8 +569,7 @@ mod test { #[test] fn migration_works() { - type M = (MockMigration<1>, MockMigration<2>); - type TestMigration = Migration; + type TestMigration = Migration; ExtBuilder::default().set_storage_version(0).build().execute_with(|| { assert_eq!(StorageVersion::get::>(), 0); diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index d23a238dcd158..9425f25adf4b8 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -28,8 +28,8 @@ use crate::{ wasm::{Determinism, PrefabWasmModule, ReturnCode as RuntimeReturnCode}, weights::WeightInfo, BalanceOf, Code, CodeStorage, CollectEvents, Config, ContractInfo, ContractInfoOf, DebugInfo, - DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, Origin, Pallet, - Schedule, + DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, NoopMigration, + Origin, Pallet, Schedule, }; use assert_matches::assert_matches; use codec::Encode; @@ -428,6 +428,7 @@ impl Config for Test { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = UnstableInterface; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; + type Migrations = (NoopMigration<1>, NoopMigration<2>); } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);