Remove all stale on_runtime_upgrade hooks in the runtime (#10650)

* Remove all stale on_runtime_upgrade hooks in the runtime * add docs * cleanup * fix warn * fix more warnings * fix offence test * overwrite the damn UItest
paritytech · Jan 19, 2022 · 33c518e · 33c518e
1 parent bea8f32
commit 33c518e
Show file tree

Hide file tree

Showing 11 changed files with 28 additions and 80 deletions.
diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
@@ -87,12 +87,12 @@
 mod gas;
 mod benchmarking;
 mod exec;
-mod migration;
 mod schedule;
 mod storage;
 mod wasm;
 
 pub mod chain_extension;
+pub mod migration;
 pub mod weights;
 
 #[cfg(test)]
@@ -321,10 +321,6 @@ pub mod pallet {
 			Storage::<T>::process_deletion_queue_batch(weight_limit)
 				.saturating_add(T::WeightInfo::on_initialize())
 		}
-
-		fn on_runtime_upgrade() -> Weight {
-			migration::migrate::<T>()
-		}
 	}
 
 	#[pallet::call]

diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs
@@ -25,6 +25,7 @@ use frame_support::{
 };
 use sp_std::{marker::PhantomData, prelude::*};
 
+/// Wrapper for all migrations of this pallet, based on `StorageVersion`.
 pub fn migrate<T: Config>() -> Weight {
 	use frame_support::traits::StorageVersion;
 

diff --git a/frame/offences/src/lib.rs b/frame/offences/src/lib.rs
@@ -22,7 +22,7 @@
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
 
-mod migration;
+pub mod migration;
 mod mock;
 mod tests;
 
@@ -47,7 +47,6 @@ type ReportIdOf<T> = <T as frame_system::Config>::Hash;
 pub mod pallet {
 	use super::*;
 	use frame_support::pallet_prelude::*;
-	use frame_system::pallet_prelude::*;
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
@@ -110,13 +109,6 @@ pub mod pallet {
 		/// \[kind, timeslot\].
 		Offence { kind: Kind, timeslot: OpaqueTimeSlot },
 	}
-
-	#[pallet::hooks]
-	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-		fn on_runtime_upgrade() -> Weight {
-			migration::remove_deferred_storage::<T>()
-		}
-	}
 }
 
 impl<T: Config, O: Offence<T::IdentificationTuple>>

diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs
@@ -56,8 +56,7 @@ pub fn remove_deferred_storage<T: Config>() -> Weight {
 #[cfg(test)]
 mod test {
 	use super::*;
-	use crate::mock::{new_test_ext, with_on_offence_fractions, Offences, Runtime as T};
-	use frame_support::traits::OnRuntimeUpgrade;
+	use crate::mock::{new_test_ext, with_on_offence_fractions, Runtime as T};
 	use sp_runtime::Perbill;
 	use sp_staking::offence::OffenceDetails;
 
@@ -87,7 +86,7 @@ mod test {
 
 			// when
 			assert_eq!(
-				Offences::on_runtime_upgrade(),
+				remove_deferred_storage::<T>(),
 				<T as frame_system::Config>::DbWeight::get().reads_writes(1, 1),
 			);
 

diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs
@@ -39,9 +39,9 @@ mod impls;
 pub use impls::*;
 
 use crate::{
-	log, migrations, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout,
-	EraRewardPoints, Exposure, Forcing, NegativeImbalanceOf, Nominations, PositiveImbalanceOf,
-	Releases, RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
+	log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, EraRewardPoints,
+	Exposure, Forcing, NegativeImbalanceOf, Nominations, PositiveImbalanceOf, Releases,
+	RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
 	ValidatorPrefs,
 };
 
@@ -660,23 +660,6 @@ pub mod pallet {
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-		fn on_runtime_upgrade() -> Weight {
-			if StorageVersion::<T>::get() == Releases::V6_0_0 {
-				migrations::v7::migrate::<T>()
-			} else {
-				T::DbWeight::get().reads(1)
-			}
-		}
-
-		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<(), &'static str> {
-			if StorageVersion::<T>::get() == Releases::V6_0_0 {
-				migrations::v7::pre_migrate::<T>()
-			} else {
-				Ok(())
-			}
-		}
-
 		fn on_initialize(_now: BlockNumberFor<T>) -> Weight {
 			// just return the weight of the on_finalize.
 			T::DbWeight::get().reads(1)

diff --git a/frame/support/src/traits/hooks.rs b/frame/support/src/traits/hooks.rs
@@ -244,10 +244,15 @@ pub trait Hooks<BlockNumber> {
 	/// # Warning
 	///
 	/// This function will be called before we initialized any runtime state, aka `on_initialize`
-	/// wasn't called yet. So, information like the block number and any other
-	/// block local data are not accessible.
+	/// wasn't called yet. So, information like the block number and any other block local data are
+	/// not accessible.
 	///
 	/// Return the non-negotiable weight consumed for runtime upgrade.
+	///
+	/// While this function can be freely implemented, using `on_runtime_upgrade` from inside the
+	/// pallet is discouraged and might get deprecated in the future. Alternatively, export the same
+	/// logic as a free-function from your pallet, and pass it to `type Executive` from the
+	/// top-level runtime.
 	fn on_runtime_upgrade() -> crate::weights::Weight {
 		0
 	}

diff --git a/frame/support/test/tests/pallet_ui/genesis_default_not_satisfied.stderr b/frame/support/test/tests/pallet_ui/genesis_default_not_satisfied.stderr
@@ -1,11 +1,11 @@
 error[E0277]: the trait bound `pallet::GenesisConfig: std::default::Default` is not satisfied
-   --> $DIR/genesis_default_not_satisfied.rs:22:18
+   --> tests/pallet_ui/genesis_default_not_satisfied.rs:22:18
     |
 22  |     impl<T: Config> GenesisBuild<T> for GenesisConfig {}
     |                     ^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `pallet::GenesisConfig`
     |
 note: required by a bound in `GenesisBuild`
-   --> $DIR/hooks.rs:297:36
+   --> $WORKSPACE/frame/support/src/traits/hooks.rs
     |
-297 | pub trait GenesisBuild<T, I = ()>: Default + sp_runtime::traits::MaybeSerializeDeserialize {
+    | pub trait GenesisBuild<T, I = ()>: Default + sp_runtime::traits::MaybeSerializeDeserialize {
     |                                    ^^^^^^^ required by this bound in `GenesisBuild`
diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs
@@ -33,14 +33,13 @@ mod benchmarking;
 pub mod mock;
 #[cfg(test)]
 mod tests;
-pub mod weights;
 
 mod functions;
 mod impl_nonfungibles;
 mod types;
-pub use types::*;
 
-mod migration;
+pub mod migration;
+pub mod weights;
 
 use codec::{Decode, Encode, HasCompact};
 use frame_support::traits::{BalanceStatus::Reserved, Currency, ReservableCurrency};
@@ -52,6 +51,7 @@ use sp_runtime::{
 use sp_std::prelude::*;
 
 pub use pallet::*;
+pub use types::*;
 pub use weights::WeightInfo;
 
 #[frame_support::pallet]
@@ -316,13 +316,6 @@ pub mod pallet {
 		Unapproved,
 	}
 
-	#[pallet::hooks]
-	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
-		fn on_runtime_upgrade() -> frame_support::weights::Weight {
-			migration::migrate_to_v1::<T, I, Self>()
-		}
-	}
-
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// Get the owner of the asset instance, if the asset exists.
 		pub fn owner(class: T::ClassId, instance: T::InstanceId) -> Option<T::AccountId> {

diff --git a/frame/uniques/src/migration.rs b/frame/uniques/src/migration.rs
@@ -22,6 +22,7 @@ use frame_support::{
 	weights::Weight,
 };
 
+/// Migrate the pallet storage to v1.
 pub fn migrate_to_v1<T: Config<I>, I: 'static, P: GetStorageVersion + PalletInfoAccess>(
 ) -> frame_support::weights::Weight {
 	let on_chain_storage_version = <P as GetStorageVersion>::on_chain_storage_version();

diff --git a/frame/vesting/src/lib.rs b/frame/vesting/src/lib.rs
@@ -45,13 +45,14 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 mod benchmarking;
-mod migrations;
+
 #[cfg(test)]
 mod mock;
 #[cfg(test)]
 mod tests;
 mod vesting_info;
 
+pub mod migrations;
 pub mod weights;
 
 use codec::{Decode, Encode, MaxEncodedLen};
@@ -179,29 +180,6 @@ pub mod pallet {
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<(), &'static str> {
-			if StorageVersion::<T>::get() == Releases::V0 {
-				migrations::v1::pre_migrate::<T>()
-			} else {
-				Ok(())
-			}
-		}
-
-		fn on_runtime_upgrade() -> Weight {
-			if StorageVersion::<T>::get() == Releases::V0 {
-				StorageVersion::<T>::put(Releases::V1);
-				migrations::v1::migrate::<T>().saturating_add(T::DbWeight::get().reads_writes(1, 1))
-			} else {
-				T::DbWeight::get().reads(1)
-			}
-		}
-
-		#[cfg(feature = "try-runtime")]
-		fn post_upgrade() -> Result<(), &'static str> {
-			migrations::v1::post_migrate::<T>()
-		}
-
 		fn integrity_test() {
 			assert!(T::MAX_VESTING_SCHEDULES > 0, "`MaxVestingSchedules` must ge greater than 0");
 		}

diff --git a/frame/vesting/src/migrations.rs b/frame/vesting/src/migrations.rs
@@ -20,11 +20,11 @@
 use super::*;
 
 // Migration from single schedule to multiple schedules.
-pub(crate) mod v1 {
+pub mod v1 {
 	use super::*;
 
 	#[cfg(feature = "try-runtime")]
-	pub(crate) fn pre_migrate<T: Config>() -> Result<(), &'static str> {
+	pub fn pre_migrate<T: Config>() -> Result<(), &'static str> {
 		assert!(StorageVersion::<T>::get() == Releases::V0, "Storage version too high.");
 
 		log::debug!(
@@ -37,7 +37,7 @@ pub(crate) mod v1 {
 
 	/// Migrate from single schedule to multi schedule storage.
 	/// WARNING: This migration will delete schedules if `MaxVestingSchedules < 1`.
-	pub(crate) fn migrate<T: Config>() -> Weight {
+	pub fn migrate<T: Config>() -> Weight {
 		let mut reads_writes = 0;
 
 		Vesting::<T>::translate::<VestingInfo<BalanceOf<T>, T::BlockNumber>, _>(
@@ -65,7 +65,7 @@ pub(crate) mod v1 {
 	}
 
 	#[cfg(feature = "try-runtime")]
-	pub(crate) fn post_migrate<T: Config>() -> Result<(), &'static str> {
+	pub fn post_migrate<T: Config>() -> Result<(), &'static str> {
 		assert_eq!(StorageVersion::<T>::get(), Releases::V1);
 
 		for (_key, schedules) in Vesting::<T>::iter() {