Remove xcm on_runtime_upgrade pallet hook (#7235)

* move migration stuffs * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Fix test * fix * lint * fix lint * rm extra space * fix lint * PR review * fixes * use saturating_accrue in fn * fix test --------- Co-authored-by: Bastian Köcher <git@kchr.de>
paritytech · Aug 5, 2023 · c7f58c1 · c7f58c1
1 parent dbdf16f
commit c7f58c1
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 35 deletions.
diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs
@@ -689,12 +689,6 @@ pub mod pallet {
 			}
 			weight_used
 		}
-		fn on_runtime_upgrade() -> Weight {
-			// Start a migration (this happens before on_initialize so it'll happen later in this
-			// block, which should be good enough)...
-			CurrentMigration::<T>::put(VersionMigrationStage::default());
-			T::DbWeight::get().writes(1)
-		}
 	}
 
 	pub mod migrations {

diff --git a/xcm/pallet-xcm/src/migration.rs b/xcm/pallet-xcm/src/migration.rs
@@ -25,6 +25,7 @@ const DEFAULT_PROOF_SIZE: u64 = 64 * 1024;
 
 pub mod v1 {
 	use super::*;
+	use crate::{CurrentMigration, VersionMigrationStage};
 
 	/// Named with the 'VersionUnchecked'-prefix because although this implements some version
 	/// checking, the version checking is not complete as it will begin failing after the upgrade is
@@ -33,34 +34,30 @@ pub mod v1 {
 	/// Use experimental [`VersionCheckedMigrateToV1`] instead.
 	pub struct VersionUncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
 	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
-		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<sp_std::vec::Vec<u8>, sp_runtime::TryRuntimeError> {
-			ensure!(StorageVersion::get::<Pallet<T>>() == 0, "must upgrade linearly");
-
-			Ok(sp_std::vec::Vec::new())
-		}
-
 		fn on_runtime_upgrade() -> Weight {
-			if StorageVersion::get::<Pallet<T>>() == 0 {
-				let mut weight = T::DbWeight::get().reads(1);
+			let mut weight = T::DbWeight::get().reads(1);
+
+			if StorageVersion::get::<Pallet<T>>() != 0 {
+				log::warn!("skipping v1, should be removed");
+				return weight
+			}
 
-				let translate = |pre: (u64, u64, u32)| -> Option<(u64, Weight, u32)> {
-					weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
-					let translated = (pre.0, Weight::from_parts(pre.1, DEFAULT_PROOF_SIZE), pre.2);
-					log::info!("Migrated VersionNotifyTarget {:?} to {:?}", pre, translated);
-					Some(translated)
-				};
+			weight.saturating_accrue(T::DbWeight::get().writes(1));
+			CurrentMigration::<T>::put(VersionMigrationStage::default());
 
-				VersionNotifyTargets::<T>::translate_values(translate);
+			let translate = |pre: (u64, u64, u32)| -> Option<(u64, Weight, u32)> {
+				weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));
+				let translated = (pre.0, Weight::from_parts(pre.1, DEFAULT_PROOF_SIZE), pre.2);
+				log::info!("Migrated VersionNotifyTarget {:?} to {:?}", pre, translated);
+				Some(translated)
+			};
 
-				log::info!("v1 applied successfully");
-				StorageVersion::new(1).put::<Pallet<T>>();
+			VersionNotifyTargets::<T>::translate_values(translate);
 
-				weight.saturating_add(T::DbWeight::get().writes(1))
-			} else {
-				log::warn!("skipping v1, should be removed");
-				T::DbWeight::get().reads(1)
-			}
+			log::info!("v1 applied successfully");
+			weight.saturating_accrue(T::DbWeight::get().writes(1));
+			StorageVersion::new(1).put::<Pallet<T>>();
+			weight
 		}
 	}
 

diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs
@@ -16,7 +16,8 @@
 
 use crate::{
 	mock::*, AssetTraps, CurrentMigration, Error, LatestVersionedMultiLocation, Queries,
-	QueryStatus, VersionDiscoveryQueue, VersionNotifiers, VersionNotifyTargets,
+	QueryStatus, VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers,
+	VersionNotifyTargets,
 };
 use frame_support::{
 	assert_noop, assert_ok,
@@ -897,15 +898,15 @@ fn subscription_side_works() {
 		assert_eq!(take_sent_xcm(), vec![(remote.clone(), Xcm(vec![instr]))]);
 
 		// A runtime upgrade which doesn't alter the version sends no notifications.
-		XcmPallet::on_runtime_upgrade();
+		CurrentMigration::<Test>::put(VersionMigrationStage::default());
 		XcmPallet::on_initialize(1);
 		assert_eq!(take_sent_xcm(), vec![]);
 
 		// New version.
 		AdvertisedXcmVersion::set(2);
 
 		// A runtime upgrade which alters the version does send notifications.
-		XcmPallet::on_runtime_upgrade();
+		CurrentMigration::<Test>::put(VersionMigrationStage::default());
 		XcmPallet::on_initialize(2);
 		let instr = QueryResponse {
 			query_id: 0,
@@ -932,7 +933,7 @@ fn subscription_side_upgrades_work_with_notify() {
 		AdvertisedXcmVersion::set(3);
 
 		// A runtime upgrade which alters the version does send notifications.
-		XcmPallet::on_runtime_upgrade();
+		CurrentMigration::<Test>::put(VersionMigrationStage::default());
 		XcmPallet::on_initialize(1);
 
 		let instr1 = QueryResponse {
@@ -982,7 +983,7 @@ fn subscription_side_upgrades_work_without_notify() {
 		VersionNotifyTargets::<Test>::insert(3, v3_location, (72, Weight::zero(), 2));
 
 		// A runtime upgrade which alters the version does send notifications.
-		XcmPallet::on_runtime_upgrade();
+		CurrentMigration::<Test>::put(VersionMigrationStage::default());
 		XcmPallet::on_initialize(1);
 
 		let mut contents = VersionNotifyTargets::<Test>::iter().collect::<Vec<_>>();
@@ -1166,7 +1167,7 @@ fn subscription_side_upgrades_work_with_multistage_notify() {
 		AdvertisedXcmVersion::set(3);
 
 		// A runtime upgrade which alters the version does send notifications.
-		XcmPallet::on_runtime_upgrade();
+		CurrentMigration::<Test>::put(VersionMigrationStage::default());
 		let mut maybe_migration = CurrentMigration::<Test>::take();
 		let mut counter = 0;
 		while let Some(migration) = maybe_migration.take() {