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>

frame/assets/src/migration.rs

@@ -18,6 +18,9 @@
 use super::*;
 use frame_support::{log, traits::OnRuntimeUpgrade};
 
+#[cfg(feature = "try-runtime")]
+use sp_runtime::TryRuntimeError;
+
 pub mod v1 {
 	use frame_support::{pallet_prelude::*, weights::Weight};
 
@@ -92,7 +95,7 @@ pub mod v1 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 			frame_support::ensure!(
 				Pallet::<T>::on_chain_storage_version() == 0,
 				"must upgrade linearly"
@@ -102,31 +105,32 @@ pub mod v1 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(prev_count: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(prev_count: Vec<u8>) -> Result<(), TryRuntimeError> {
 			let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect(
 				"the state parameter should be something that was generated by pre_upgrade",
 			);
 			let post_count = Asset::<T>::iter().count() as u32;
-			assert_eq!(
-				prev_count, post_count,
+			ensure!(
+				prev_count == post_count,
 				"the asset count before and after the migration should be the same"
 			);
 
 			let current_version = Pallet::<T>::current_storage_version();
 			let onchain_version = Pallet::<T>::on_chain_storage_version();
 
 			frame_support::ensure!(current_version == 1, "must_upgrade");
-			assert_eq!(
-				current_version, onchain_version,
+			ensure!(
+				current_version == onchain_version,
 				"after migration, the current_version and onchain_version should be the same"
 			);
 
-			Asset::<T>::iter().for_each(|(_id, asset)| {
-				assert!(
+			Asset::<T>::iter().try_for_each(|(_id, asset)| -> Result<(), TryRuntimeError> {
+				ensure!(
 					asset.status == AssetStatus::Live || asset.status == AssetStatus::Frozen,
-					"assets should only be live or frozen. None should be in destroying status, or undefined state"
-				)
-			});
+					 "assets should only be live or frozen. None should be in destroying status, or undefined state"
+				);
+				Ok(())
+			})?;
 			Ok(())
 		}
 	}

frame/bags-list/src/lib.rs

@@ -59,6 +59,9 @@ use frame_system::ensure_signed;
 use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded, StaticLookup};
 use sp_std::prelude::*;
 
+#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
+use sp_runtime::TryRuntimeError;
+
 #[cfg(any(feature = "runtime-benchmarks", test))]
 mod benchmarks;
 
@@ -267,15 +270,15 @@ pub mod pallet {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn try_state(_: BlockNumberFor<T>) -> Result<(), &'static str> {
+		fn try_state(_: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
 			<Self as SortedListProvider<T::AccountId>>::try_state()
 		}
 	}
 }
 
 #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
-	pub fn do_try_state() -> Result<(), &'static str> {
+	pub fn do_try_state() -> Result<(), TryRuntimeError> {
 		List::<T, I>::do_try_state()
 	}
 }
@@ -355,7 +358,7 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn try_state() -> Result<(), &'static str> {
+	fn try_state() -> Result<(), TryRuntimeError> {
 		Self::do_try_state()
 	}
 

frame/bags-list/src/list/mod.rs

@@ -42,6 +42,9 @@ use sp_std::{
 	prelude::*,
 };
 
+#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
+use sp_runtime::TryRuntimeError;
+
 #[derive(Debug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, PalletError)]
 pub enum ListError {
 	/// A duplicate id has been detected.
@@ -512,11 +515,11 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 	/// * and sanity-checks all bags and nodes. This will cascade down all the checks and makes sure
 	/// all bags and nodes are checked per *any* update to `List`.
 	#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
-	pub(crate) fn do_try_state() -> Result<(), &'static str> {
+	pub(crate) fn do_try_state() -> Result<(), TryRuntimeError> {
 		let mut seen_in_list = BTreeSet::new();
 		ensure!(
 			Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)),
-			"duplicate identified",
+			"duplicate identified"
 		);
 
 		let iter_count = Self::iter().count() as u32;
@@ -750,7 +753,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 	/// * Ensures tail has no next.
 	/// * Ensures there are no loops, traversal from head to tail is correct.
 	#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
-	fn do_try_state(&self) -> Result<(), &'static str> {
+	fn do_try_state(&self) -> Result<(), TryRuntimeError> {
 		frame_support::ensure!(
 			self.head()
 				.map(|head| head.prev().is_none())
@@ -895,15 +898,12 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
 	}
 
 	#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
-	fn do_try_state(&self) -> Result<(), &'static str> {
+	fn do_try_state(&self) -> Result<(), TryRuntimeError> {
 		let expected_bag = Bag::<T, I>::get(self.bag_upper).ok_or("bag not found for node")?;
 
 		let id = self.id();
 
-		frame_support::ensure!(
-			expected_bag.contains(id),
-			"node does not exist in the expected bag"
-		);
+		frame_support::ensure!(expected_bag.contains(id), "node does not exist in the bag");
 
 		let non_terminal_check = !self.is_terminal() &&
 			expected_bag.head.as_ref() != Some(id) &&

frame/bags-list/src/list/tests.rs

@@ -22,6 +22,7 @@ use crate::{
 };
 use frame_election_provider_support::{SortedListProvider, VoteWeight};
 use frame_support::{assert_ok, assert_storage_noop};
+use sp_runtime::TryRuntimeError;
 
 fn node(
 	id: AccountId,
@@ -359,7 +360,10 @@ mod list {
 		// make sure there are no duplicates.
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			Bag::<Runtime>::get(10).unwrap().insert_unchecked(2, 10);
-			assert_eq!(List::<Runtime>::do_try_state(), Err("duplicate identified"));
+			assert_eq!(
+				List::<Runtime>::do_try_state(),
+				TryRuntimeError::Other("duplicate identified").into()
+			);
 		});
 
 		// ensure count is in sync with `ListNodes::count()`.
@@ -373,7 +377,10 @@ mod list {
 			CounterForListNodes::<Runtime>::mutate(|counter| *counter += 1);
 			assert_eq!(crate::ListNodes::<Runtime>::count(), 5);
 
-			assert_eq!(List::<Runtime>::do_try_state(), Err("iter_count != stored_count"));
+			assert_eq!(
+				List::<Runtime>::do_try_state(),
+				TryRuntimeError::Other("iter_count != stored_count").into()
+			);
 		});
 	}
 

frame/bags-list/src/migrations.rs

@@ -24,6 +24,9 @@ use frame_support::traits::OnRuntimeUpgrade;
 
 #[cfg(feature = "try-runtime")]
 use frame_support::ensure;
+#[cfg(feature = "try-runtime")]
+use sp_runtime::TryRuntimeError;
+
 #[cfg(feature = "try-runtime")]
 use sp_std::vec::Vec;
 
@@ -35,7 +38,7 @@ impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix<T,
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 		// The old explicit storage item.
 		#[frame_support::storage_alias]
 		type CounterForListNodes<T: crate::Config<I>, I: 'static> =
@@ -88,7 +91,7 @@ mod old {
 pub struct AddScore<T: crate::Config<I>, I: 'static = ()>(sp_std::marker::PhantomData<(T, I)>);
 impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for AddScore<T, I> {
 	#[cfg(feature = "try-runtime")]
-	fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
 		// The list node data should be corrupt at this point, so this is zero.
 		ensure!(crate::ListNodes::<T, I>::iter().count() == 0, "list node data is not corrupt");
 		// We can use the helper `old::ListNode` to get the existing data.
@@ -119,7 +122,7 @@ impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for AddScore<T, I> {
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn post_upgrade(node_count_before: Vec<u8>) -> Result<(), &'static str> {
+	fn post_upgrade(node_count_before: Vec<u8>) -> Result<(), TryRuntimeError> {
 		let node_count_before: u32 = Decode::decode(&mut node_count_before.as_slice())
 			.expect("the state parameter should be something that was generated by pre_upgrade");
 		// Now the list node data is not corrupt anymore.

frame/collective/src/lib.rs

@@ -60,6 +60,9 @@ use frame_support::{
 	weights::Weight,
 };
 
+#[cfg(any(feature = "try-runtime", test))]
+use sp_runtime::TryRuntimeError;
+
 #[cfg(test)]
 mod tests;
 
@@ -346,9 +349,8 @@ pub mod pallet {
 	#[pallet::hooks]
 	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
 		#[cfg(feature = "try-runtime")]
-		fn try_state(_n: BlockNumberFor<T>) -> Result<(), &'static str> {
-			Self::do_try_state()?;
-			Ok(())
+		fn try_state(_n: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
+			Self::do_try_state()
 		}
 	}
 
@@ -967,77 +969,78 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Looking at prime account:
 	/// * The prime account must be a member of the collective.
 	#[cfg(any(feature = "try-runtime", test))]
-	fn do_try_state() -> DispatchResult {
-		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
-			ensure!(
-				Self::proposal_of(proposal).is_some(),
-				DispatchError::Other(
+	fn do_try_state() -> Result<(), TryRuntimeError> {
+		Self::proposals()
+			.into_iter()
+			.try_for_each(|proposal| -> Result<(), TryRuntimeError> {
+				ensure!(
+					Self::proposal_of(proposal).is_some(),
 					"Proposal hash from `Proposals` is not found inside the `ProposalOf` mapping."
-				)
-			);
-			Ok(())
-		})?;
+				);
+				Ok(())
+			})?;
 
 		ensure!(
 			Self::proposals().into_iter().count() <= Self::proposal_count() as usize,
-			DispatchError::Other("The actual number of proposals is greater than `ProposalCount`")
+			"The actual number of proposals is greater than `ProposalCount`"
 		);
 		ensure!(
 			Self::proposals().into_iter().count() == <ProposalOf<T, I>>::iter_keys().count(),
-			DispatchError::Other("Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`")
+			"Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`"
 		);
 
-		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
-			if let Some(votes) = Self::voting(proposal) {
-				let ayes = votes.ayes.len();
-				let nays = votes.nays.len();
-
-				ensure!(
-					ayes.saturating_add(nays) <= T::MaxMembers::get() as usize,
-					DispatchError::Other("The sum of ayes and nays is greater than `MaxMembers`")
-				);
-			}
-			Ok(())
-		})?;
+		Self::proposals()
+			.into_iter()
+			.try_for_each(|proposal| -> Result<(), TryRuntimeError> {
+				if let Some(votes) = Self::voting(proposal) {
+					let ayes = votes.ayes.len();
+					let nays = votes.nays.len();
+
+					ensure!(
+						ayes.saturating_add(nays) <= T::MaxMembers::get() as usize,
+						"The sum of ayes and nays is greater than `MaxMembers`"
+					);
+				}
+				Ok(())
+			})?;
 
 		let mut proposal_indices = vec![];
-		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
-			if let Some(votes) = Self::voting(proposal) {
-				let proposal_index = votes.index;
-				ensure!(
-					!proposal_indices.contains(&proposal_index),
-					DispatchError::Other("The proposal index is not unique.")
-				);
-				proposal_indices.push(proposal_index);
-			}
-			Ok(())
-		})?;
+		Self::proposals()
+			.into_iter()
+			.try_for_each(|proposal| -> Result<(), TryRuntimeError> {
+				if let Some(votes) = Self::voting(proposal) {
+					let proposal_index = votes.index;
+					ensure!(
+						!proposal_indices.contains(&proposal_index),
+						"The proposal index is not unique."
+					);
+					proposal_indices.push(proposal_index);
+				}
+				Ok(())
+			})?;
 
-		<Voting<T, I>>::iter_keys().try_for_each(|proposal_hash| -> DispatchResult {
-			ensure!(
-				Self::proposals().contains(&proposal_hash),
-				DispatchError::Other(
+		<Voting<T, I>>::iter_keys().try_for_each(
+			|proposal_hash| -> Result<(), TryRuntimeError> {
+				ensure!(
+					Self::proposals().contains(&proposal_hash),
 					"`Proposals` doesn't contain the proposal hash from the `Voting` storage map."
-				)
-			);
-			Ok(())
-		})?;
+				);
+				Ok(())
+			},
+		)?;
 
 		ensure!(
 			Self::members().len() <= T::MaxMembers::get() as usize,
-			DispatchError::Other("The member count is greater than `MaxMembers`.")
+			"The member count is greater than `MaxMembers`."
 		);
 
 		ensure!(
 			Self::members().windows(2).all(|members| members[0] <= members[1]),
-			DispatchError::Other("The members are not sorted by value.")
+			"The members are not sorted by value."
 		);
 
 		if let Some(prime) = Self::prime() {
-			ensure!(
-				Self::members().contains(&prime),
-				DispatchError::Other("Prime account is not a member.")
-			);
+			ensure!(Self::members().contains(&prime), "Prime account is not a member.");
 		}
 
 		Ok(())