BREAKING - Try-runtime: Use proper error types (#13993)

* Try-state: DispatchResult as return type

* try_state for the rest of the pallets

* pre_upgrade

* post_upgrade

* try_runtime_upgrade

* fixes

* bags-list fix

* fix

* update test

* warning fix

* ...

* final fixes 🤞

* warning..

* frame-support

* warnings

* Update frame/staking/src/migrations.rs

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* fix

* fix warning

* nit fix

* merge fixes

* small fix

* should be good now

* missed these ones

* introduce TryRuntimeError and TryRuntimeResult

* fixes

* fix

* removed TryRuntimeResult & made some fixes

* fix testsg

* tests passing

* unnecessary imports

* Update frame/assets/src/migration.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

substrate/frame/nomination-pools/src/lib.rs

@@ -378,6 +378,9 @@ use sp_runtime::{
 use sp_staking::{EraIndex, OnStakerSlash, StakingInterface};
 use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, ops::Div, vec::Vec};
 
+#[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))]
+use sp_runtime::TryRuntimeError;
+
 /// The log target of this pallet.
 pub const LOG_TARGET: &str = "runtime::nomination-pools";
 
@@ -2626,7 +2629,7 @@ pub mod pallet {
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
 		#[cfg(feature = "try-runtime")]
-		fn try_state(_n: BlockNumberFor<T>) -> Result<(), &'static str> {
+		fn try_state(_n: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
 			Self::do_try_state(u8::MAX)
 		}
 
@@ -3055,7 +3058,7 @@ impl<T: Config> Pallet<T> {
 	/// multiple `level`s, where the higher the level, the more checks we performs. So,
 	/// `try_state(255)` is the strongest sanity check, and `0` performs no checks.
 	#[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))]
-	pub fn do_try_state(level: u8) -> Result<(), &'static str> {
+	pub fn do_try_state(level: u8) -> Result<(), TryRuntimeError> {
 		if level.is_zero() {
 			return Ok(())
 		}
@@ -3063,12 +3066,24 @@ impl<T: Config> Pallet<T> {
 		// result in the same set of keys, in the same order.
 		let bonded_pools = BondedPools::<T>::iter_keys().collect::<Vec<_>>();
 		let reward_pools = RewardPools::<T>::iter_keys().collect::<Vec<_>>();
-		assert_eq!(bonded_pools, reward_pools);
+		ensure!(
+			bonded_pools == reward_pools,
+			"`BondedPools` and `RewardPools` must all have the EXACT SAME key-set."
+		);
 
-		assert!(Metadata::<T>::iter_keys().all(|k| bonded_pools.contains(&k)));
-		assert!(SubPoolsStorage::<T>::iter_keys().all(|k| bonded_pools.contains(&k)));
+		ensure!(
+			SubPoolsStorage::<T>::iter_keys().all(|k| bonded_pools.contains(&k)),
+			"`SubPoolsStorage` must be a subset of the above superset."
+		);
+		ensure!(
+			Metadata::<T>::iter_keys().all(|k| bonded_pools.contains(&k)),
+			"`Metadata` keys must be a subset of the above superset."
+		);
 
-		assert!(MaxPools::<T>::get().map_or(true, |max| bonded_pools.len() <= (max as usize)));
+		ensure!(
+			MaxPools::<T>::get().map_or(true, |max| bonded_pools.len() <= (max as usize)),
+			Error::<T>::MaxPools
+		);
 
 		for id in reward_pools {
 			let account = Self::create_reward_account(id);
@@ -3088,9 +3103,9 @@ impl<T: Config> Pallet<T> {
 		let mut pools_members = BTreeMap::<PoolId, u32>::new();
 		let mut pools_members_pending_rewards = BTreeMap::<PoolId, BalanceOf<T>>::new();
 		let mut all_members = 0u32;
-		PoolMembers::<T>::iter().for_each(|(_, d)| {
+		PoolMembers::<T>::iter().try_for_each(|(_, d)| -> Result<(), TryRuntimeError> {
 			let bonded_pool = BondedPools::<T>::get(d.pool_id).unwrap();
-			assert!(!d.total_points().is_zero(), "no member should have zero points: {d:?}");
+			ensure!(!d.total_points().is_zero(), "No member should have zero points");
 			*pools_members.entry(d.pool_id).or_default() += 1;
 			all_members += 1;
 
@@ -3103,9 +3118,11 @@ impl<T: Config> Pallet<T> {
 				let pending_rewards = d.pending_rewards(current_rc).unwrap();
 				*pools_members_pending_rewards.entry(d.pool_id).or_default() += pending_rewards;
 			} // else this pool has been heavily slashed and cannot have any rewards anymore.
-		});
 
-		RewardPools::<T>::iter_keys().for_each(|id| {
+			Ok(())
+		})?;
+
+		RewardPools::<T>::iter_keys().try_for_each(|id| -> Result<(), TryRuntimeError> {
 			// the sum of the pending rewards must be less than the leftover balance. Since the
 			// reward math rounds down, we might accumulate some dust here.
 			log!(
@@ -3115,30 +3132,40 @@ impl<T: Config> Pallet<T> {
 				pools_members_pending_rewards.get(&id),
 				RewardPool::<T>::current_balance(id)
 			);
-			assert!(
+			ensure!(
 				RewardPool::<T>::current_balance(id) >=
-					pools_members_pending_rewards.get(&id).copied().unwrap_or_default()
-			)
-		});
-
-		BondedPools::<T>::iter().for_each(|(id, inner)| {
-			let bonded_pool = BondedPool { id, inner };
-			assert_eq!(
-				pools_members.get(&id).copied().unwrap_or_default(),
-				bonded_pool.member_counter
+					pools_members_pending_rewards.get(&id).copied().unwrap_or_default(),
+				"The sum of the pending rewards must be less than the leftover balance."
+			);
+			Ok(())
+		})?;
+
+		BondedPools::<T>::iter().try_for_each(|(id, inner)| -> Result<(), TryRuntimeError> {
+			let bonded_pool = BondedPool { id, inner };
+			ensure!(
+				pools_members.get(&id).copied().unwrap_or_default() ==
+				bonded_pool.member_counter,
+				"Each `BondedPool.member_counter` must be equal to the actual count of members of this pool"
+			);
+			ensure!(
+				MaxPoolMembersPerPool::<T>::get()
+					.map_or(true, |max| bonded_pool.member_counter <= max),
+				Error::<T>::MaxPoolMembers
 			);
-			assert!(MaxPoolMembersPerPool::<T>::get()
-				.map_or(true, |max| bonded_pool.member_counter <= max));
 
 			let depositor = PoolMembers::<T>::get(&bonded_pool.roles.depositor).unwrap();
-			assert!(
+			ensure!(
 				bonded_pool.is_destroying_and_only_depositor(depositor.active_points()) ||
 					depositor.active_points() >= MinCreateBond::<T>::get(),
 				"depositor must always have MinCreateBond stake in the pool, except for when the \
 				pool is being destroyed and the depositor is the last member",
 			);
-		});
-		assert!(MaxPoolMembers::<T>::get().map_or(true, |max| all_members <= max));
+			Ok(())
+		})?;
+		ensure!(
+			MaxPoolMembers::<T>::get().map_or(true, |max| all_members <= max),
+			Error::<T>::MaxPoolMembers
+		);
 
 		if level <= 1 {
 			return Ok(())

substrate/frame/nomination-pools/src/migration.rs

@@ -20,6 +20,9 @@ use crate::log;
 use frame_support::traits::OnRuntimeUpgrade;
 use sp_std::{collections::btree_map::BTreeMap, vec::Vec};
 
+#[cfg(feature = "try-runtime")]
+use sp_runtime::TryRuntimeError;
+
 pub mod v1 {
 	use super::*;
 
@@ -100,9 +103,12 @@ pub mod v1 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
 			// new version must be set.
-			assert_eq!(Pallet::<T>::on_chain_storage_version(), 1);
+			ensure!(
+				Pallet::<T>::on_chain_storage_version() == 1,
+				"The onchain version must be updated after the migration."
+			);
 			Pallet::<T>::try_state(frame_system::Pallet::<T>::block_number())?;
 			Ok(())
 		}
@@ -352,38 +358,47 @@ pub mod v2 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 			// all reward accounts must have more than ED.
-			RewardPools::<T>::iter().for_each(|(id, _)| {
-				assert!(
+			RewardPools::<T>::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> {
+				ensure!(
 					T::Currency::free_balance(&Pallet::<T>::create_reward_account(id)) >=
-						T::Currency::minimum_balance()
-				)
-			});
+						T::Currency::minimum_balance(),
+					"Reward accounts must have greater balance than ED."
+				);
+				Ok(())
+			})?;
 
 			Ok(Vec::new())
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
 			// new version must be set.
-			assert_eq!(Pallet::<T>::on_chain_storage_version(), 2);
+			ensure!(
+				Pallet::<T>::on_chain_storage_version() == 2,
+				"The onchain version must be updated after the migration."
+			);
 
 			// no reward or bonded pool has been skipped.
-			assert_eq!(RewardPools::<T>::iter().count() as u32, RewardPools::<T>::count());
-			assert_eq!(BondedPools::<T>::iter().count() as u32, BondedPools::<T>::count());
+			ensure!(
+				RewardPools::<T>::iter().count() as u32 == RewardPools::<T>::count(),
+				"The count of reward pools must remain the same after the migration."
+			);
+			ensure!(
+				BondedPools::<T>::iter().count() as u32 == BondedPools::<T>::count(),
+				"The count of reward pools must remain the same after the migration."
+			);
 
 			// all reward pools must have exactly ED in them. This means no reward can be claimed,
 			// and that setting reward counters all over the board to zero will work henceforth.
-			RewardPools::<T>::iter().for_each(|(id, _)| {
-				assert_eq!(
-					RewardPool::<T>::current_balance(id),
-					Zero::zero(),
-					"reward pool({}) balance is {:?}",
-					id,
-					RewardPool::<T>::current_balance(id)
+			RewardPools::<T>::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> {
+				ensure!(
+					RewardPool::<T>::current_balance(id) == Zero::zero(),
+					"Reward pool balance must be zero.",
 				);
-			});
+				Ok(())
+			})?;
 
 			log!(info, "post upgrade hook for MigrateToV2 executed.");
 			Ok(())
@@ -435,7 +450,7 @@ pub mod v3 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 			ensure!(
 				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
 				"the on_chain version is equal or more than the current one"
@@ -444,7 +459,7 @@ pub mod v3 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
 			ensure!(
 				Metadata::<T>::iter_keys().all(|id| BondedPools::<T>::contains_key(&id)),
 				"not all of the stale metadata has been removed"
@@ -535,7 +550,7 @@ pub mod v4 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 			ensure!(
 				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
 				"the on_chain version is equal or more than the current one"
@@ -544,7 +559,7 @@ pub mod v4 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
 			// ensure all BondedPools items now contain an `inner.commission: Commission` field.
 			ensure!(
 				BondedPools::<T>::iter().all(|(_, inner)| inner.commission.current.is_none() &&
@@ -620,7 +635,7 @@ pub mod v5 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 			ensure!(
 				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
 				"the on_chain version is equal or more than the current one"
@@ -654,7 +669,7 @@ pub mod v5 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(data: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(data: Vec<u8>) -> Result<(), TryRuntimeError> {
 			let old_rpool_values: u64 = Decode::decode(&mut &data[..]).unwrap();
 			let rpool_keys = RewardPools::<T>::iter_keys().count() as u64;
 			let rpool_values = RewardPools::<T>::iter_values().count() as u64;

substrate/frame/nomination-pools/src/tests.rs

@@ -4269,10 +4269,7 @@ mod create {
 			assert!(!BondedPools::<Runtime>::contains_key(2));
 			assert!(!RewardPools::<Runtime>::contains_key(2));
 			assert!(!PoolMembers::<Runtime>::contains_key(11));
-			assert_err!(
-				StakingMock::active_stake(&next_pool_stash),
-				DispatchError::Other("balance not found")
-			);
+			assert_err!(StakingMock::active_stake(&next_pool_stash), "balance not found");
 
 			Balances::make_free_balance_be(&11, StakingMock::minimum_nominator_bond() + ed);
 			assert_ok!(Pools::create(