BREAKING - Try-runtime: Use proper error types

frame/assets/src/migration.rs

@@ -92,37 +92,41 @@ pub mod v1 {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+		fn pre_upgrade() -> Result<Vec<u8>, DispatchError> {
 			frame_support::ensure!(
 				Pallet::<T>::on_chain_storage_version() == 0,
-				"must upgrade linearly"
+				DispatchError::Other("must upgrade linearly")
 			);
 			let prev_count = Asset::<T>::iter().count();
 			Ok((prev_count as u32).encode())
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn post_upgrade(prev_count: Vec<u8>) -> Result<(), &'static str> {
+		fn post_upgrade(prev_count: Vec<u8>) -> DispatchResult {
 			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,
-				"the asset count before and after the migration should be the same"
+			ensure!(
+				prev_count == post_count,
+				DispatchError::Other(
+					"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,
-				"after migration, the current_version and onchain_version should be the same"
+			ensure!(
+				current_version == onchain_version,
+				DispatchError::Other(
+					"after migration, the current_version and onchain_version should be the same"
+				)
 			);
 
 			Asset::<T>::iter().for_each(|(_id, asset)| {
-				assert!(asset.status == AssetStatus::Live || asset.status == AssetStatus::Frozen, "assets should only be live or frozen. None should be in destroying status, or undefined state")
+				ensure!(asset.status == AssetStatus::Live || asset.status == AssetStatus::Frozen, DispatchError::Other("assets should only be live or frozen. None should be in destroying status, or undefined state"))
 			});
 			Ok(())
 		}

frame/bags-list/src/lib.rs

@@ -55,6 +55,7 @@
 
 use codec::FullCodec;
 use frame_election_provider_support::{ScoreProvider, SortedListProvider};
+use frame_support::dispatch::{DispatchError, DispatchResult};
 use frame_system::ensure_signed;
 use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded, StaticLookup};
 use sp_std::prelude::*;
@@ -267,15 +268,15 @@ pub mod pallet {
 		}
 
 		#[cfg(feature = "try-runtime")]
-		fn try_state(_: BlockNumberFor<T>) -> Result<(), &'static str> {
+		fn try_state(_: BlockNumberFor<T>) -> DispatchResult {
 			<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() -> DispatchResult {
 		List::<T, I>::do_try_state()
 	}
 }
@@ -355,7 +356,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() -> DispatchResult {
 		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 frame_support::dispatch::{DispatchError, DispatchResult};
+
 #[derive(Debug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, PalletError)]
 pub enum ListError {
 	/// A duplicate id has been detected.
@@ -512,18 +515,18 @@ 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() -> DispatchResult {
 		let mut seen_in_list = BTreeSet::new();
 		ensure!(
 			Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)),
-			"duplicate identified",
+			DispatchError::Other("duplicate identified")
 		);
 
 		let iter_count = Self::iter().count() as u32;
 		let stored_count = crate::ListNodes::<T, I>::count();
 		let nodes_count = crate::ListNodes::<T, I>::iter().count() as u32;
-		ensure!(iter_count == stored_count, "iter_count != stored_count");
-		ensure!(stored_count == nodes_count, "stored_count != nodes_count");
+		ensure!(iter_count == stored_count, DispatchError::Other("iter_count != stored_count"));
+		ensure!(stored_count == nodes_count, DispatchError::Other("stored_count != nodes_count"));
 
 		crate::log!(trace, "count of nodes: {}", stored_count);
 
@@ -544,7 +547,10 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 
 		let nodes_in_bags_count =
 			active_bags.clone().fold(0u32, |acc, cur| acc + cur.iter().count() as u32);
-		ensure!(nodes_count == nodes_in_bags_count, "stored_count != nodes_in_bags_count");
+		ensure!(
+			nodes_count == nodes_in_bags_count,
+			DispatchError::Other("stored_count != nodes_in_bags_count")
+		);
 
 		crate::log!(trace, "count of active bags {}", active_bags.count());
 
@@ -750,14 +756,14 @@ 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) -> DispatchResult {
 		frame_support::ensure!(
 			self.head()
 				.map(|head| head.prev().is_none())
 				// if there is no head, then there must not be a tail, meaning that the bag is
 				// empty.
 				.unwrap_or_else(|| self.tail.is_none()),
-			"head has a prev"
+			DispatchError::Other("head has a prev")
 		);
 
 		frame_support::ensure!(
@@ -766,7 +772,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 				// if there is no tail, then there must not be a head, meaning that the bag is
 				// empty.
 				.unwrap_or_else(|| self.head.is_none()),
-			"tail has a next"
+			DispatchError::Other("tail has a next")
 		);
 
 		let mut seen_in_bag = BTreeSet::new();
@@ -775,7 +781,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 				.map(|node| node.id)
 				// each voter is only seen once, thus there is no cycle within a bag
 				.all(|voter| seen_in_bag.insert(voter)),
-			"duplicate found in bag"
+			DispatchError::Other("duplicate found in bag")
 		);
 
 		Ok(())
@@ -895,14 +901,14 @@ 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) -> DispatchResult {
 		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"
+			DispatchError::Other("node does not exist in the bag")
 		);
 
 		let non_terminal_check = !self.is_terminal() &&
@@ -912,7 +918,7 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
 			expected_bag.head.as_ref() == Some(id) || expected_bag.tail.as_ref() == Some(id);
 		frame_support::ensure!(
 			non_terminal_check || terminal_check,
-			"a terminal node is neither its bag head or tail"
+			DispatchError::Other("a terminal node is neither its bag head or tail")
 		);
 
 		Ok(())

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

@@ -21,7 +21,7 @@ use crate::{
 	ListBags, ListNodes,
 };
 use frame_election_provider_support::{SortedListProvider, VoteWeight};
-use frame_support::{assert_ok, assert_storage_noop};
+use frame_support::{assert_ok, assert_storage_noop, dispatch::DispatchError};
 
 fn node(
 	id: AccountId,
@@ -359,7 +359,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(),
+				DispatchError::Other("duplicate identified").into()
+			);
 		});
 
 		// ensure count is in sync with `ListNodes::count()`.
@@ -373,7 +376,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(),
+				DispatchError::Other("iter_count != stored_count").into()
+			);
 		});
 	}
 

frame/bags-list/src/migrations.rs

@@ -35,7 +35,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>, DispatchError> {
 		// The old explicit storage item.
 		#[frame_support::storage_alias]
 		type CounterForListNodes<T: crate::Config<I>, I: 'static> =
@@ -88,14 +88,20 @@ 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>, DispatchError> {
 		// 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");
+		ensure!(
+			crate::ListNodes::<T, I>::iter().count() == 0,
+			DispatchError::Other("list node data is not corrupt")
+		);
 		// We can use the helper `old::ListNode` to get the existing data.
 		let iter_node_count: u32 = old::ListNodes::<T, I>::iter().count() as u32;
 		let tracked_node_count: u32 = old::CounterForListNodes::<T, I>::get();
 		crate::log!(info, "number of nodes before: {:?} {:?}", iter_node_count, tracked_node_count);
-		ensure!(iter_node_count == tracked_node_count, "Node count is wrong.");
+		ensure!(
+			iter_node_count == tracked_node_count,
+			DispatchError::Other("Node count is wrong.")
+		);
 		Ok(iter_node_count.encode())
 	}
 
@@ -119,7 +125,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>) -> DispatchResult {
 		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.
@@ -131,8 +137,14 @@ impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for AddScore<T, I> {
 			iter_node_count_after,
 			tracked_node_count_after,
 		);
-		ensure!(iter_node_count_after == node_count_before, "Not all nodes were migrated.");
-		ensure!(tracked_node_count_after == iter_node_count_after, "Node count is wrong.");
+		ensure!(
+			iter_node_count_after == node_count_before,
+			DispatchError::Other("Not all nodes were migrated.")
+		);
+		ensure!(
+			tracked_node_count_after == iter_node_count_after,
+			DispatchError::Other("Node count is wrong.")
+		);
 		Ok(())
 	}
 }

frame/collective/src/lib.rs

@@ -348,9 +348,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>) -> DispatchResult {
+			Self::do_try_state()
 		}
 	}
 

frame/contracts/src/migration.rs

@@ -66,7 +66,7 @@ impl<T: Config> OnRuntimeUpgrade for Migration<T> {
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+	fn pre_upgrade() -> Result<Vec<u8>, DispatchError> {
 		let version = <Pallet<T>>::on_chain_storage_version();
 
 		if version == 7 {
@@ -77,7 +77,7 @@ impl<T: Config> OnRuntimeUpgrade for Migration<T> {
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
+	fn post_upgrade(state: Vec<u8>) -> DispatchResult {
 		let version = Decode::decode(&mut state.as_ref()).map_err(|_| "Cannot decode version")?;
 		post_checks::post_upgrade::<T>(version)
 	}
@@ -355,11 +355,14 @@ mod v8 {
 	}
 
 	#[cfg(feature = "try-runtime")]
-	pub fn pre_upgrade<T: Config>() -> Result<(), &'static str> {
+	pub fn pre_upgrade<T: Config>() -> Result<(), DispatchError> {
 		use frame_support::traits::ReservableCurrency;
 		for (key, value) in ContractInfoOf::<T, OldContractInfo<T>>::iter() {
 			let reserved = T::Currency::reserved_balance(&key);
-			ensure!(reserved >= value.storage_deposit, "Reserved balance out of sync.");
+			ensure!(
+				reserved >= value.storage_deposit,
+				DispatchError::Other("Reserved balance out of sync.")
+			);
 		}
 		Ok(())
 	}
@@ -418,7 +421,7 @@ mod post_checks {
 	type ContractInfoOf<T: Config, V> =
 		StorageMap<Pallet<T>, Twox64Concat, <T as frame_system::Config>::AccountId, V>;
 
-	pub fn post_upgrade<T: Config>(old_version: StorageVersion) -> Result<(), &'static str> {
+	pub fn post_upgrade<T: Config>(old_version: StorageVersion) -> DispatchResult {
 		if old_version < 7 {
 			return Ok(())
 		}
@@ -434,15 +437,15 @@ mod post_checks {
 		Ok(())
 	}
 
-	fn v8<T: Config>() -> Result<(), &'static str> {
+	fn v8<T: Config>() -> DispatchResult {
 		use frame_support::traits::ReservableCurrency;
 		for (key, value) in ContractInfoOf::<T, ContractInfo<T>>::iter() {
 			let reserved = T::Currency::reserved_balance(&key);
 			let stored = value
 				.storage_base_deposit
 				.saturating_add(value.storage_byte_deposit)
 				.saturating_add(value.storage_item_deposit);
-			ensure!(reserved >= stored, "Reserved balance out of sync.");
+			ensure!(reserved >= stored, DispatchError::Other("Reserved balance out of sync."));
 
 			let mut storage_bytes = 0u32;
 			let mut storage_items = 0u32;
@@ -455,17 +458,23 @@ mod post_checks {
 				storage_bytes.saturating_accrue(len);
 				storage_items.saturating_accrue(1);
 			}
-			ensure!(storage_bytes == value.storage_bytes, "Storage bytes do not match.",);
-			ensure!(storage_items == value.storage_items, "Storage items do not match.",);
+			ensure!(
+				storage_bytes == value.storage_bytes,
+				DispatchError::Other("Storage bytes do not match.")
+			);
+			ensure!(
+				storage_items == value.storage_items,
+				DispatchError::Other("Storage items do not match.")
+			);
 		}
 		Ok(())
 	}
 
-	fn v9<T: Config>() -> Result<(), &'static str> {
+	fn v9<T: Config>() -> DispatchResult {
 		for value in CodeStorage::<T>::iter_values() {
 			ensure!(
 				value.determinism == Determinism::Enforced,
-				"All pre-existing codes need to be deterministic."
+				DispatchError::Other("All pre-existing codes need to be deterministic.")
 			);
 		}
 		Ok(())