From 6a2aad1ce4ecdb8320bd0701804f92c93b7b584e Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Tue, 23 May 2023 08:56:10 +0200 Subject: [PATCH] BREAKING - Try-runtime: Use proper error types (#13993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Co-authored-by: Liam Aharon Co-authored-by: Keith Yeung --- frame/assets/src/migration.rs | 26 +++-- frame/bags-list/src/lib.rs | 9 +- frame/bags-list/src/list/mod.rs | 16 +-- frame/bags-list/src/list/tests.rs | 11 +- frame/bags-list/src/migrations.rs | 9 +- frame/collective/src/lib.rs | 105 +++++++++--------- frame/contracts/src/migration.rs | 19 ++-- frame/democracy/src/migrations.rs | 14 +-- .../election-provider-multi-phase/src/lib.rs | 32 ++++-- frame/election-provider-support/src/lib.rs | 5 +- frame/elections-phragmen/src/lib.rs | 33 +++--- frame/executive/src/lib.rs | 5 +- frame/fast-unstake/src/lib.rs | 7 +- frame/fast-unstake/src/migrations.rs | 19 +++- frame/multisig/src/migrations.rs | 4 +- frame/nfts/src/migration.rs | 11 +- frame/nomination-pools/src/lib.rs | 73 ++++++++---- frame/nomination-pools/src/migration.rs | 67 ++++++----- frame/nomination-pools/src/tests.rs | 5 +- frame/offences/src/migration.rs | 6 +- frame/preimage/src/migration.rs | 13 ++- frame/referenda/src/migration.rs | 16 +-- frame/scheduler/src/migration.rs | 27 +++-- frame/staking/src/migrations.rs | 29 +++-- frame/staking/src/pallet/impls.rs | 44 +++++--- frame/staking/src/pallet/mod.rs | 2 +- .../procedural/src/pallet/expand/hooks.rs | 10 +- frame/support/src/dispatch.rs | 10 +- frame/support/src/migrations.rs | 6 +- frame/support/src/traits/hooks.rs | 25 +++-- frame/support/src/traits/try_runtime.rs | 9 +- frame/support/test/tests/pallet.rs | 17 ++- primitives/runtime/src/lib.rs | 3 + utils/frame/try-runtime/cli/src/lib.rs | 6 +- 34 files changed, 418 insertions(+), 275 deletions(-) diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 90400ef5bd1dc..d854a64afb57f 100644 --- a/frame/assets/src/migration.rs +++ b/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, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { frame_support::ensure!( Pallet::::on_chain_storage_version() == 0, "must upgrade linearly" @@ -102,13 +105,13 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { + fn post_upgrade(prev_count: Vec) -> 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::::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" ); @@ -116,17 +119,18 @@ pub mod v1 { let onchain_version = Pallet::::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::::iter().for_each(|(_id, asset)| { - assert!( + Asset::::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(()) } } diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index d4d54b9a134bb..156c52cc87c45 100644 --- a/frame/bags-list/src/lib.rs +++ b/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,7 +270,7 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_: BlockNumberFor) -> Result<(), &'static str> { + fn try_state(_: BlockNumberFor) -> Result<(), TryRuntimeError> { >::try_state() } } @@ -275,7 +278,7 @@ pub mod pallet { #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] impl, I: 'static> Pallet { - pub fn do_try_state() -> Result<(), &'static str> { + pub fn do_try_state() -> Result<(), TryRuntimeError> { List::::do_try_state() } } @@ -355,7 +358,7 @@ impl, I: 'static> SortedListProvider for Pallet } #[cfg(feature = "try-runtime")] - fn try_state() -> Result<(), &'static str> { + fn try_state() -> Result<(), TryRuntimeError> { Self::do_try_state() } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index f667f4c101ef8..d8626080e2523 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/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, I: 'static> List { /// * 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, I: 'static> Bag { /// * 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, I: 'static> Node { } #[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::::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) && diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index f5afdc24f2608..fd4ad8f893af3 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/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::::get(10).unwrap().insert_unchecked(2, 10); - assert_eq!(List::::do_try_state(), Err("duplicate identified")); + assert_eq!( + List::::do_try_state(), + TryRuntimeError::Other("duplicate identified").into() + ); }); // ensure count is in sync with `ListNodes::count()`. @@ -373,7 +377,10 @@ mod list { CounterForListNodes::::mutate(|counter| *counter += 1); assert_eq!(crate::ListNodes::::count(), 5); - assert_eq!(List::::do_try_state(), Err("iter_count != stored_count")); + assert_eq!( + List::::do_try_state(), + TryRuntimeError::Other("iter_count != stored_count").into() + ); }); } diff --git a/frame/bags-list/src/migrations.rs b/frame/bags-list/src/migrations.rs index 5f9bb8f73ac60..7df63a6a44c54 100644 --- a/frame/bags-list/src/migrations.rs +++ b/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, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { // The old explicit storage item. #[frame_support::storage_alias] type CounterForListNodes, I: 'static> = @@ -88,7 +91,7 @@ mod old { pub struct AddScore, I: 'static = ()>(sp_std::marker::PhantomData<(T, I)>); impl, I: 'static> OnRuntimeUpgrade for AddScore { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { // The list node data should be corrupt at this point, so this is zero. ensure!(crate::ListNodes::::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, I: 'static> OnRuntimeUpgrade for AddScore { } #[cfg(feature = "try-runtime")] - fn post_upgrade(node_count_before: Vec) -> Result<(), &'static str> { + fn post_upgrade(node_count_before: Vec) -> 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. diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 12917f5bd6e34..f904b42af028d 100644 --- a/frame/collective/src/lib.rs +++ b/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, I: 'static> Hooks> for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { - Self::do_try_state()?; - Ok(()) + fn try_state(_n: BlockNumberFor) -> Result<(), TryRuntimeError> { + Self::do_try_state() } } @@ -967,77 +969,78 @@ impl, I: 'static> Pallet { /// 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() == >::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(()) + })?; - >::iter_keys().try_for_each(|proposal_hash| -> DispatchResult { - ensure!( - Self::proposals().contains(&proposal_hash), - DispatchError::Other( + >::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(()) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 96a4c3203474c..0b7c094ddb176 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -28,6 +28,9 @@ use frame_support::{ use sp_runtime::traits::Saturating; use sp_std::{marker::PhantomData, prelude::*}; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// Performs all necessary migrations based on `StorageVersion`. pub struct Migration(PhantomData); impl OnRuntimeUpgrade for Migration { @@ -66,7 +69,7 @@ impl OnRuntimeUpgrade for Migration { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { let version = >::on_chain_storage_version(); if version == 7 { @@ -77,7 +80,7 @@ impl OnRuntimeUpgrade for Migration { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { + fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { let version = Decode::decode(&mut state.as_ref()).map_err(|_| "Cannot decode version")?; post_checks::post_upgrade::(version) } @@ -355,7 +358,7 @@ mod v8 { } #[cfg(feature = "try-runtime")] - pub fn pre_upgrade() -> Result<(), &'static str> { + pub fn pre_upgrade() -> Result<(), TryRuntimeError> { use frame_support::traits::ReservableCurrency; for (key, value) in ContractInfoOf::>::iter() { let reserved = T::Currency::reserved_balance(&key); @@ -418,7 +421,7 @@ mod post_checks { type ContractInfoOf = StorageMap, Twox64Concat, ::AccountId, V>; - pub fn post_upgrade(old_version: StorageVersion) -> Result<(), &'static str> { + pub fn post_upgrade(old_version: StorageVersion) -> Result<(), TryRuntimeError> { if old_version < 7 { return Ok(()) } @@ -434,7 +437,7 @@ mod post_checks { Ok(()) } - fn v8() -> Result<(), &'static str> { + fn v8() -> Result<(), TryRuntimeError> { use frame_support::traits::ReservableCurrency; for (key, value) in ContractInfoOf::>::iter() { let reserved = T::Currency::reserved_balance(&key); @@ -455,13 +458,13 @@ 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, "Storage bytes do not match."); + ensure!(storage_items == value.storage_items, "Storage items do not match."); } Ok(()) } - fn v9() -> Result<(), &'static str> { + fn v9() -> Result<(), TryRuntimeError> { for value in CodeStorage::::iter_values() { ensure!( value.determinism == Determinism::Enforced, diff --git a/frame/democracy/src/migrations.rs b/frame/democracy/src/migrations.rs index fe2e445bd02a6..397f42dce7210 100644 --- a/frame/democracy/src/migrations.rs +++ b/frame/democracy/src/migrations.rs @@ -61,12 +61,12 @@ pub mod v1 { impl> OnRuntimeUpgrade for Migration { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { - assert_eq!(StorageVersion::get::>(), 0, "can only upgrade from version 0"); + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + ensure!(StorageVersion::get::>() == 0, "can only upgrade from version 0"); let props_count = v0::PublicProps::::get().len(); log::info!(target: TARGET, "{} public proposals will be migrated.", props_count,); - ensure!(props_count <= T::MaxProposals::get() as usize, "too many proposals"); + ensure!(props_count <= T::MaxProposals::get() as usize, Error::::TooMany); let referenda_count = v0::ReferendumInfoOf::::iter().count(); log::info!(target: TARGET, "{} referenda will be migrated.", referenda_count); @@ -133,15 +133,15 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { - assert_eq!(StorageVersion::get::>(), 1, "must upgrade"); + fn post_upgrade(state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + ensure!(StorageVersion::get::>() == 1, "must upgrade"); let (old_props_count, old_ref_count): (u32, u32) = Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); let new_props_count = crate::PublicProps::::get().len() as u32; - assert_eq!(new_props_count, old_props_count, "must migrate all public proposals"); + ensure!(new_props_count == old_props_count, "must migrate all public proposals"); let new_ref_count = crate::ReferendumInfoOf::::iter().count() as u32; - assert_eq!(new_ref_count, old_ref_count, "must migrate all referenda"); + ensure!(new_ref_count == old_ref_count, "must migrate all referenda"); log::info!( target: TARGET, diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index c5247e3c21c23..98fa880349eeb 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -257,6 +257,9 @@ use sp_runtime::{ }; use sp_std::prelude::*; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + #[cfg(feature = "runtime-benchmarks")] mod benchmarking; #[cfg(test)] @@ -883,7 +886,7 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), &'static str> { + fn try_state(_n: T::BlockNumber) -> Result<(), TryRuntimeError> { Self::do_try_state() } } @@ -1579,7 +1582,7 @@ impl Pallet { #[cfg(feature = "try-runtime")] impl Pallet { - fn do_try_state() -> Result<(), &'static str> { + fn do_try_state() -> Result<(), TryRuntimeError> { Self::try_state_snapshot()?; Self::try_state_signed_submissions_map()?; Self::try_state_phase_off() @@ -1588,7 +1591,7 @@ impl Pallet { // [`Snapshot`] state check. Invariants: // - [`DesiredTargets`] exists if and only if [`Snapshot`] is present. // - [`SnapshotMetadata`] exist if and only if [`Snapshot`] is present. - fn try_state_snapshot() -> Result<(), &'static str> { + fn try_state_snapshot() -> Result<(), TryRuntimeError> { if >::exists() && >::exists() && >::exists() @@ -1600,7 +1603,7 @@ impl Pallet { { Ok(()) } else { - Err("If snapshot exists, metadata and desired targets should be set too. Otherwise, none should be set.") + Err("If snapshot exists, metadata and desired targets should be set too. Otherwise, none should be set.".into()) } } @@ -1608,28 +1611,34 @@ impl Pallet { // - All [`SignedSubmissionIndices`] are present in [`SignedSubmissionsMap`], and no more; // - [`SignedSubmissionNextIndex`] is not present in [`SignedSubmissionsMap`]; // - [`SignedSubmissionIndices`] is sorted by election score. - fn try_state_signed_submissions_map() -> Result<(), &'static str> { + fn try_state_signed_submissions_map() -> Result<(), TryRuntimeError> { let mut last_score: ElectionScore = Default::default(); let indices = >::get(); for (i, indice) in indices.iter().enumerate() { let submission = >::get(indice.2); if submission.is_none() { - return Err("All signed submissions indices must be part of the submissions map") + return Err( + "All signed submissions indices must be part of the submissions map".into() + ) } if i == 0 { last_score = indice.0 } else { if last_score.strict_threshold_better(indice.0, Perbill::zero()) { - return Err("Signed submission indices vector must be ordered by election score") + return Err( + "Signed submission indices vector must be ordered by election score".into() + ) } last_score = indice.0; } } if >::iter().nth(indices.len()).is_some() { - return Err("Signed submissions map length should be the same as the indices vec length") + return Err( + "Signed submissions map length should be the same as the indices vec length".into() + ) } match >::get() { @@ -1637,7 +1646,8 @@ impl Pallet { next => if >::get(next).is_some() { return Err( - "The next submissions index should not be in the submissions maps already", + "The next submissions index should not be in the submissions maps already" + .into(), ) } else { Ok(()) @@ -1647,12 +1657,12 @@ impl Pallet { // [`Phase::Off`] state check. Invariants: // - If phase is `Phase::Off`, [`Snapshot`] must be none. - fn try_state_phase_off() -> Result<(), &'static str> { + fn try_state_phase_off() -> Result<(), TryRuntimeError> { match Self::current_phase().is_off() { false => Ok(()), true => if >::get().is_some() { - Err("Snapshot must be none when in Phase::Off") + Err("Snapshot must be none when in Phase::Off".into()) } else { Ok(()) }, diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index ee0e41a90cd60..6e6d3341bfc8a 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -189,6 +189,9 @@ pub use sp_npos_elections::{ }; pub use traits::NposSolution; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + // re-export for the solution macro, with the dependencies of the macro. #[doc(hidden)] pub use codec; @@ -564,7 +567,7 @@ pub trait SortedListProvider { /// Check internal state of the list. Only meant for debugging. #[cfg(feature = "try-runtime")] - fn try_state() -> Result<(), &'static str>; + fn try_state() -> Result<(), TryRuntimeError>; /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 1d7c79fe3ca97..33a8634cb3bc9 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -115,6 +115,9 @@ use sp_runtime::{ }; use sp_std::{cmp::Ordering, prelude::*}; +#[cfg(any(feature = "try-runtime", test))] +use sp_runtime::TryRuntimeError; + mod benchmarking; pub mod weights; pub use weights::WeightInfo; @@ -327,7 +330,7 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), &'static str> { + fn try_state(_n: T::BlockNumber) -> Result<(), TryRuntimeError> { Self::do_try_state() } } @@ -1193,7 +1196,7 @@ impl ContainsLengthBound for Pallet { #[cfg(any(feature = "try-runtime", test))] impl Pallet { - fn do_try_state() -> Result<(), &'static str> { + fn do_try_state() -> Result<(), TryRuntimeError> { Self::try_state_members()?; Self::try_state_runners_up()?; Self::try_state_candidates()?; @@ -1204,20 +1207,20 @@ impl Pallet { /// [`Members`] state checks. Invariants: /// - Members are always sorted based on account ID. - fn try_state_members() -> Result<(), &'static str> { + fn try_state_members() -> Result<(), TryRuntimeError> { let mut members = Members::::get().clone(); members.sort_by_key(|m| m.who.clone()); if Members::::get() == members { Ok(()) } else { - Err("try_state checks: Members must be always sorted by account ID") + Err("try_state checks: Members must be always sorted by account ID".into()) } } // [`RunnersUp`] state checks. Invariants: // - Elements are sorted based on weight (worst to best). - fn try_state_runners_up() -> Result<(), &'static str> { + fn try_state_runners_up() -> Result<(), TryRuntimeError> { let mut sorted = RunnersUp::::get(); // worst stake first sorted.sort_by(|a, b| a.stake.cmp(&b.stake)); @@ -1225,27 +1228,28 @@ impl Pallet { if RunnersUp::::get() == sorted { Ok(()) } else { - Err("try_state checks: Runners Up must always be sorted by stake (worst to best)") + Err("try_state checks: Runners Up must always be sorted by stake (worst to best)" + .into()) } } // [`Candidates`] state checks. Invariants: // - Always sorted based on account ID. - fn try_state_candidates() -> Result<(), &'static str> { + fn try_state_candidates() -> Result<(), TryRuntimeError> { let mut candidates = Candidates::::get().clone(); candidates.sort_by_key(|(c, _)| c.clone()); if Candidates::::get() == candidates { Ok(()) } else { - Err("try_state checks: Candidates must be always sorted by account ID") + Err("try_state checks: Candidates must be always sorted by account ID".into()) } } // [`Candidates`] and [`RunnersUp`] state checks. Invariants: // - Candidates and runners-ups sets are disjoint. - fn try_state_candidates_runners_up_disjoint() -> Result<(), &'static str> { + fn try_state_candidates_runners_up_disjoint() -> Result<(), TryRuntimeError> { match Self::intersects(&Self::candidates_ids(), &Self::runners_up_ids()) { - true => Err("Candidates and runners up sets should always be disjoint"), + true => Err("Candidates and runners up sets should always be disjoint".into()), false => Ok(()), } } @@ -1253,11 +1257,12 @@ impl Pallet { // [`Members`], [`Candidates`] and [`RunnersUp`] state checks. Invariants: // - Members and candidates sets are disjoint; // - Members and runners-ups sets are disjoint. - fn try_state_members_disjoint() -> Result<(), &'static str> { + fn try_state_members_disjoint() -> Result<(), TryRuntimeError> { match Self::intersects(&Pallet::::members_ids(), &Self::candidates_ids()) && Self::intersects(&Pallet::::members_ids(), &Self::runners_up_ids()) { - true => Err("Members set should be disjoint from candidates and runners-up sets"), + true => + Err("Members set should be disjoint from candidates and runners-up sets".into()), false => Ok(()), } } @@ -1265,14 +1270,14 @@ impl Pallet { // [`Members`], [`RunnersUp`] and approval stake state checks. Invariants: // - Selected members should have approval stake; // - Selected RunnersUp should have approval stake. - fn try_state_members_approval_stake() -> Result<(), &'static str> { + fn try_state_members_approval_stake() -> Result<(), TryRuntimeError> { match Members::::get() .iter() .chain(RunnersUp::::get().iter()) .all(|s| s.stake != BalanceOf::::zero()) { true => Ok(()), - false => Err("Members and RunnersUp must have approval stake"), + false => Err("Members and RunnersUp must have approval stake".into()), } } diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index fd76fefadff4b..9ec78f254212e 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -137,6 +137,9 @@ use sp_runtime::{ }; use sp_std::{marker::PhantomData, prelude::*}; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + #[allow(dead_code)] const LOG_TARGET: &str = "runtime::executive"; @@ -338,7 +341,7 @@ where /// `true`. Also, if set to `true`, it runs the `pre_upgrade` and `post_upgrade` hooks. pub fn try_runtime_upgrade( checks: frame_try_runtime::UpgradeCheckSelect, - ) -> Result { + ) -> Result { if checks.try_state() { let _guard = frame_support::StorageNoopGuard::default(); >::try_state( diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 3e79bf4077d20..f8a8e00346a34 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -91,6 +91,9 @@ pub mod pallet { use sp_std::{prelude::*, vec::Vec}; pub use weights::WeightInfo; + #[cfg(feature = "try-runtime")] + use sp_runtime::TryRuntimeError; + #[derive(scale_info::TypeInfo, codec::Encode, codec::Decode, codec::MaxEncodedLen)] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] @@ -228,10 +231,10 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), &'static str> { + fn try_state(_n: T::BlockNumber) -> Result<(), TryRuntimeError> { // ensure that the value of `ErasToCheckPerBlock` is less than // `T::MaxErasToCheckPerBlock`. - assert!( + ensure!( ErasToCheckPerBlock::::get() <= T::MaxErasToCheckPerBlock::get(), "the value of `ErasToCheckPerBlock` is greater than `T::MaxErasToCheckPerBlock`", ); diff --git a/frame/fast-unstake/src/migrations.rs b/frame/fast-unstake/src/migrations.rs index e5ef919298a4f..564388407045e 100644 --- a/frame/fast-unstake/src/migrations.rs +++ b/frame/fast-unstake/src/migrations.rs @@ -25,6 +25,11 @@ pub mod v1 { use sp_staking::EraIndex; use sp_std::prelude::*; + #[cfg(feature = "try-runtime")] + use frame_support::ensure; + #[cfg(feature = "try-runtime")] + use sp_runtime::TryRuntimeError; + pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { fn on_runtime_upgrade() -> Weight { @@ -65,14 +70,20 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { - assert_eq!(Pallet::::on_chain_storage_version(), 0); + fn pre_upgrade() -> Result, TryRuntimeError> { + ensure!( + Pallet::::on_chain_storage_version() == 0, + "The onchain storage version must be zero for the migration to execute." + ); Ok(Default::default()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), &'static str> { - assert_eq!(Pallet::::on_chain_storage_version(), 1); + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + ensure!( + Pallet::::on_chain_storage_version() == 1, + "The onchain version must be updated after the migration." + ); Ok(()) } } diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 2a9c858a5552f..298e73c5d7576 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -43,7 +43,7 @@ pub mod v1 { pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 1, "this migration can be deleted"); @@ -72,7 +72,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 2, "this migration needs to be removed"); ensure!(onchain == 1, "this migration needs to be run"); diff --git a/frame/nfts/src/migration.rs b/frame/nfts/src/migration.rs index 33ee87e4b9284..ba14492be84ca 100644 --- a/frame/nfts/src/migration.rs +++ b/frame/nfts/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}; @@ -90,7 +93,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { let current_version = Pallet::::current_storage_version(); let onchain_version = Pallet::::on_chain_storage_version(); ensure!(onchain_version == 0 && current_version == 1, "migration from version 0 to 1."); @@ -99,13 +102,13 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { + fn post_upgrade(prev_count: Vec) -> 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 = Collection::::iter().count() as u32; - assert_eq!( - prev_count, post_count, + ensure!( + prev_count == post_count, "the records count before and after the migration should be the same" ); diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index cc68f54bd55cf..66c5f786f1e25 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/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 Hooks> for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { + fn try_state(_n: BlockNumberFor) -> Result<(), TryRuntimeError> { Self::do_try_state(u8::MAX) } @@ -3055,7 +3058,7 @@ impl Pallet { /// 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 Pallet { // result in the same set of keys, in the same order. let bonded_pools = BondedPools::::iter_keys().collect::>(); let reward_pools = RewardPools::::iter_keys().collect::>(); - assert_eq!(bonded_pools, reward_pools); + ensure!( + bonded_pools == reward_pools, + "`BondedPools` and `RewardPools` must all have the EXACT SAME key-set." + ); - assert!(Metadata::::iter_keys().all(|k| bonded_pools.contains(&k))); - assert!(SubPoolsStorage::::iter_keys().all(|k| bonded_pools.contains(&k))); + ensure!( + SubPoolsStorage::::iter_keys().all(|k| bonded_pools.contains(&k)), + "`SubPoolsStorage` must be a subset of the above superset." + ); + ensure!( + Metadata::::iter_keys().all(|k| bonded_pools.contains(&k)), + "`Metadata` keys must be a subset of the above superset." + ); - assert!(MaxPools::::get().map_or(true, |max| bonded_pools.len() <= (max as usize))); + ensure!( + MaxPools::::get().map_or(true, |max| bonded_pools.len() <= (max as usize)), + Error::::MaxPools + ); for id in reward_pools { let account = Self::create_reward_account(id); @@ -3088,9 +3103,9 @@ impl Pallet { let mut pools_members = BTreeMap::::new(); let mut pools_members_pending_rewards = BTreeMap::>::new(); let mut all_members = 0u32; - PoolMembers::::iter().for_each(|(_, d)| { + PoolMembers::::iter().try_for_each(|(_, d)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPools::::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 Pallet { 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::::iter_keys().for_each(|id| { + Ok(()) + })?; + + RewardPools::::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 Pallet { pools_members_pending_rewards.get(&id), RewardPool::::current_balance(id) ); - assert!( + ensure!( RewardPool::::current_balance(id) >= - pools_members_pending_rewards.get(&id).copied().unwrap_or_default() - ) - }); + 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::::iter().for_each(|(id, inner)| { + BondedPools::::iter().try_for_each(|(id, inner)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPool { id, inner }; - assert_eq!( - pools_members.get(&id).copied().unwrap_or_default(), - bonded_pool.member_counter + 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::::get() + .map_or(true, |max| bonded_pool.member_counter <= max), + Error::::MaxPoolMembers ); - assert!(MaxPoolMembersPerPool::::get() - .map_or(true, |max| bonded_pool.member_counter <= max)); let depositor = PoolMembers::::get(&bonded_pool.roles.depositor).unwrap(); - assert!( + ensure!( bonded_pool.is_destroying_and_only_depositor(depositor.active_points()) || depositor.active_points() >= MinCreateBond::::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::::get().map_or(true, |max| all_members <= max)); + Ok(()) + })?; + ensure!( + MaxPoolMembers::::get().map_or(true, |max| all_members <= max), + Error::::MaxPoolMembers + ); if level <= 1 { return Ok(()) diff --git a/frame/nomination-pools/src/migration.rs b/frame/nomination-pools/src/migration.rs index 45d6424119900..2c84e4e6d3c8e 100644 --- a/frame/nomination-pools/src/migration.rs +++ b/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) -> Result<(), &'static str> { + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { // new version must be set. - assert_eq!(Pallet::::on_chain_storage_version(), 1); + ensure!( + Pallet::::on_chain_storage_version() == 1, + "The onchain version must be updated after the migration." + ); Pallet::::try_state(frame_system::Pallet::::block_number())?; Ok(()) } @@ -352,38 +358,47 @@ pub mod v2 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { // all reward accounts must have more than ED. - RewardPools::::iter().for_each(|(id, _)| { - assert!( + RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { + ensure!( T::Currency::free_balance(&Pallet::::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) -> Result<(), &'static str> { + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { // new version must be set. - assert_eq!(Pallet::::on_chain_storage_version(), 2); + ensure!( + Pallet::::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::::iter().count() as u32, RewardPools::::count()); - assert_eq!(BondedPools::::iter().count() as u32, BondedPools::::count()); + ensure!( + RewardPools::::iter().count() as u32 == RewardPools::::count(), + "The count of reward pools must remain the same after the migration." + ); + ensure!( + BondedPools::::iter().count() as u32 == BondedPools::::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::::iter().for_each(|(id, _)| { - assert_eq!( - RewardPool::::current_balance(id), - Zero::zero(), - "reward pool({}) balance is {:?}", - id, - RewardPool::::current_balance(id) + RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { + ensure!( + RewardPool::::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, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { ensure!( Pallet::::current_storage_version() > Pallet::::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) -> Result<(), &'static str> { + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { ensure!( Metadata::::iter_keys().all(|id| BondedPools::::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, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { ensure!( Pallet::::current_storage_version() > Pallet::::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) -> Result<(), &'static str> { + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { // ensure all BondedPools items now contain an `inner.commission: Commission` field. ensure!( BondedPools::::iter().all(|(_, inner)| inner.commission.current.is_none() && @@ -620,7 +635,7 @@ pub mod v5 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { ensure!( Pallet::::current_storage_version() > Pallet::::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) -> Result<(), &'static str> { + fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { let old_rpool_values: u64 = Decode::decode(&mut &data[..]).unwrap(); let rpool_keys = RewardPools::::iter_keys().count() as u64; let rpool_values = RewardPools::::iter_values().count() as u64; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 4cb255e23b4b8..49a0596587706 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4269,10 +4269,7 @@ mod create { assert!(!BondedPools::::contains_key(2)); assert!(!RewardPools::::contains_key(2)); assert!(!PoolMembers::::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( diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index 07bd68407d378..14dbd606dfe87 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -29,6 +29,8 @@ use sp_std::vec::Vec; #[cfg(feature = "try-runtime")] use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; mod v0 { use super::*; @@ -51,7 +53,7 @@ pub mod v1 { pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 1, "pallet_offences::MigrateToV1 migration can be deleted"); @@ -81,7 +83,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain == 1, "pallet_offences::MigrateToV1 needs to be run"); ensure!( diff --git a/frame/preimage/src/migration.rs b/frame/preimage/src/migration.rs index be352201da6cd..46e555498cd2d 100644 --- a/frame/preimage/src/migration.rs +++ b/frame/preimage/src/migration.rs @@ -24,6 +24,11 @@ use frame_support::{ }; use sp_std::collections::btree_map::BTreeMap; +#[cfg(feature = "try-runtime")] +use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// The log target. const TARGET: &'static str = "runtime::preimage::migration::v1"; @@ -78,8 +83,8 @@ pub mod v1 { impl OnRuntimeUpgrade for Migration { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { - assert_eq!(StorageVersion::get::>(), 0, "can only upgrade from version 0"); + fn pre_upgrade() -> Result, TryRuntimeError> { + ensure!(StorageVersion::get::>() == 0, "can only upgrade from version 0"); let images = v0::image_count::().expect("v0 storage corrupted"); log::info!(target: TARGET, "Migrating {} images", &images); @@ -148,7 +153,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { + fn post_upgrade(state: Vec) -> DispatchResult { let old_images: u32 = Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); let new_images = image_count::().expect("V1 storage corrupted"); @@ -161,7 +166,7 @@ pub mod v1 { old_images ); } - assert_eq!(StorageVersion::get::>(), 1, "must upgrade"); + ensure!(StorageVersion::get::>() == 1, "must upgrade"); Ok(()) } } diff --git a/frame/referenda/src/migration.rs b/frame/referenda/src/migration.rs index c27ab452ac637..6f796ca40d9be 100644 --- a/frame/referenda/src/migration.rs +++ b/frame/referenda/src/migration.rs @@ -22,6 +22,9 @@ use codec::{Decode, Encode, EncodeLike, MaxEncodedLen}; use frame_support::{pallet_prelude::*, storage_alias, traits::OnRuntimeUpgrade}; use log; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// Initial version of storage types. pub mod v0 { use super::*; @@ -95,9 +98,9 @@ pub mod v1 { pub struct MigrateV0ToV1(PhantomData<(T, I)>); impl, I: 'static> OnRuntimeUpgrade for MigrateV0ToV1 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { let onchain_version = Pallet::::on_chain_storage_version(); - assert_eq!(onchain_version, 0, "migration from version 0 to 1."); + ensure!(onchain_version == 0, "migration from version 0 to 1."); let referendum_count = v0::ReferendumInfoFor::::iter().count(); log::info!( target: TARGET, @@ -147,16 +150,13 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { + fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { let onchain_version = Pallet::::on_chain_storage_version(); - assert_eq!(onchain_version, 1, "must upgrade from version 0 to 1."); + ensure!(onchain_version == 1, "must upgrade from version 0 to 1."); let pre_referendum_count: u32 = Decode::decode(&mut &state[..]) .expect("failed to decode the state from pre-upgrade."); let post_referendum_count = ReferendumInfoFor::::iter().count() as u32; - assert_eq!( - post_referendum_count, pre_referendum_count, - "must migrate all referendums." - ); + ensure!(post_referendum_count == pre_referendum_count, "must migrate all referendums."); log::info!(target: TARGET, "migrated all referendums."); Ok(()) } diff --git a/frame/scheduler/src/migration.rs b/frame/scheduler/src/migration.rs index 5b3a7631eeac9..7f23d962063bc 100644 --- a/frame/scheduler/src/migration.rs +++ b/frame/scheduler/src/migration.rs @@ -20,6 +20,9 @@ use super::*; use frame_support::traits::OnRuntimeUpgrade; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// The log target. const TARGET: &'static str = "runtime::scheduler::migration"; @@ -97,8 +100,8 @@ pub mod v3 { impl> OnRuntimeUpgrade for MigrateToV4 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { - assert_eq!(StorageVersion::get::>(), 3, "Can only upgrade from version 3"); + fn pre_upgrade() -> Result, TryRuntimeError> { + ensure!(StorageVersion::get::>() == 3, "Can only upgrade from version 3"); let agendas = Agenda::::iter_keys().count() as u32; let decodable_agendas = Agenda::::iter_values().count() as u32; @@ -125,7 +128,7 @@ pub mod v3 { agenda.len(), max_scheduled_per_block, ); - return Err("Agenda would overflow `MaxScheduledPerBlock`.") + return Err("Agenda would overflow `MaxScheduledPerBlock`.".into()) } } // Check that bounding the calls will not overflow `MAX_LENGTH`. @@ -142,7 +145,7 @@ pub mod v3 { block_number, l, ); - return Err("Call is too large.") + return Err("Call is too large.".into()) } }, _ => (), @@ -169,12 +172,12 @@ pub mod v3 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { - assert_eq!(StorageVersion::get::>(), 4, "Must upgrade"); + fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { + ensure!(StorageVersion::get::>() == 4, "Must upgrade"); // Check that everything decoded fine. for k in crate::Agenda::::iter_keys() { - assert!(crate::Agenda::::try_get(k).is_ok(), "Cannot decode V4 Agenda"); + ensure!(crate::Agenda::::try_get(k).is_ok(), "Cannot decode V4 Agenda"); } let old_agendas: u32 = @@ -210,7 +213,7 @@ pub mod v4 { impl OnRuntimeUpgrade for CleanupAgendas { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { assert_eq!( StorageVersion::get::>(), 4, @@ -285,8 +288,8 @@ pub mod v4 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), &'static str> { - assert_eq!(StorageVersion::get::>(), 4, "Version must not change"); + fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { + ensure!(StorageVersion::get::>() == 4, "Version must not change"); let (old_agendas, non_empty_agendas): (u32, u32) = Decode::decode(&mut state.as_ref()).expect("Must decode pre_upgrade state"); @@ -305,7 +308,7 @@ pub mod v4 { old_agendas, new_agendas ), } - assert_eq!(new_agendas, non_empty_agendas, "Expected to keep all non-empty agendas"); + ensure!(new_agendas == non_empty_agendas, "Expected to keep all non-empty agendas"); Ok(()) } @@ -496,7 +499,7 @@ mod test { // The pre_upgrade hook fails: let err = v3::MigrateToV4::::pre_upgrade().unwrap_err(); - assert!(err.contains("Call is too large")); + assert!(err == "Call is too large".into()); // But the migration itself works: let _w = v3::MigrateToV4::::on_runtime_upgrade(); diff --git a/frame/staking/src/migrations.rs b/frame/staking/src/migrations.rs index 23bcfa4398627..0a27290daacd8 100644 --- a/frame/staking/src/migrations.rs +++ b/frame/staking/src/migrations.rs @@ -23,6 +23,11 @@ use frame_support::{ traits::OnRuntimeUpgrade, }; +#[cfg(feature = "try-runtime")] +use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// Used for release versioning upto v12. /// /// Obsolete from v13. Keeping around to make encoding/decoding of old migration code easier. @@ -58,7 +63,7 @@ pub mod v13 { pub struct MigrateToV13(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV13 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V12_0_0, "Required v12 before upgrading to v13" @@ -84,7 +89,7 @@ pub mod v13 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { frame_support::ensure!( Pallet::::on_chain_storage_version() == 13, "v13 not applied" @@ -114,7 +119,7 @@ pub mod v12 { pub struct MigrateToV12(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV12 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V11_0_0, "Expected v11 before upgrading to v12" @@ -146,7 +151,7 @@ pub mod v12 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V12_0_0, "v12 not applied" @@ -170,7 +175,7 @@ pub mod v11 { for MigrateToV11 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V10_0_0, "must upgrade linearly" @@ -217,7 +222,7 @@ pub mod v11 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V11_0_0, "wrong version after the upgrade" @@ -332,7 +337,7 @@ pub mod v9 { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V8_0_0, "must upgrade linearly" @@ -343,17 +348,21 @@ pub mod v9 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { + fn post_upgrade(prev_count: Vec) -> 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 = T::VoterList::count(); let validators = Validators::::count(); - assert!(post_count == prev_count + validators); + ensure!( + post_count == prev_count + validators, + "`VoterList` count after the migration must equal to the sum of \ + previous count and the current number of validators" + ); frame_support::ensure!( StorageVersion::::get() == ObsoleteReleases::V9_0_0, - "must upgrade " + "must upgrade" ); Ok(()) } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 82a0956da7b61..44d5674f2f816 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -51,6 +51,11 @@ use crate::{ use super::{pallet::*, STAKING_ID}; +#[cfg(feature = "try-runtime")] +use frame_support::ensure; +#[cfg(any(test, feature = "try-runtime"))] +use sp_runtime::TryRuntimeError; + /// The maximum number of iterations that we do whilst iterating over `T::VoterList` in /// `get_npos_voters`. /// @@ -1467,7 +1472,7 @@ impl SortedListProvider for UseValidatorsMap { 0 } #[cfg(feature = "try-runtime")] - fn try_state() -> Result<(), &'static str> { + fn try_state() -> Result<(), TryRuntimeError> { Ok(()) } @@ -1544,7 +1549,7 @@ impl SortedListProvider for UseNominatorsAndValidatorsM } #[cfg(feature = "try-runtime")] - fn try_state() -> Result<(), &'static str> { + fn try_state() -> Result<(), TryRuntimeError> { Ok(()) } @@ -1713,7 +1718,7 @@ impl StakingInterface for Pallet { #[cfg(any(test, feature = "try-runtime"))] impl Pallet { - pub(crate) fn do_try_state(_: BlockNumberFor) -> Result<(), &'static str> { + pub(crate) fn do_try_state(_: BlockNumberFor) -> Result<(), TryRuntimeError> { ensure!( T::VoterList::iter() .all(|x| >::contains_key(&x) || >::contains_key(&x)), @@ -1726,7 +1731,7 @@ impl Pallet { Self::check_count() } - fn check_count() -> Result<(), &'static str> { + fn check_count() -> Result<(), TryRuntimeError> { ensure!( ::VoterList::count() == Nominators::::count() + Validators::::count(), @@ -1739,18 +1744,19 @@ impl Pallet { ensure!( ValidatorCount::::get() <= ::MaxWinners::get(), - "validator count exceeded election max winners" + Error::::TooManyValidators ); Ok(()) } - fn check_ledgers() -> Result<(), &'static str> { + fn check_ledgers() -> Result<(), TryRuntimeError> { Bonded::::iter() .map(|(_, ctrl)| Self::ensure_ledger_consistent(ctrl)) - .collect::>() + .collect::, _>>()?; + Ok(()) } - fn check_exposures() -> Result<(), &'static str> { + fn check_exposures() -> Result<(), TryRuntimeError> { // a check per validator to ensure the exposure struct is always sane. let era = Self::active_era().unwrap().index; ErasStakers::::iter_prefix_values(era) @@ -1766,10 +1772,10 @@ impl Pallet { ); Ok(()) }) - .collect::>() + .collect::>() } - fn check_nominators() -> Result<(), &'static str> { + fn check_nominators() -> Result<(), TryRuntimeError> { // a check per nominator to ensure their entire stake is correctly distributed. Will only // kick-in if the nomination was submitted before the current era. let era = Self::active_era().unwrap().index; @@ -1783,27 +1789,33 @@ impl Pallet { } }, ) - .map(|nominator| { + .map(|nominator| -> Result<(), TryRuntimeError> { // must be bonded. Self::ensure_is_stash(&nominator)?; let mut sum = BalanceOf::::zero(); T::SessionInterface::validators() .iter() .map(|v| Self::eras_stakers(era, v)) - .map(|e| { + .map(|e| -> Result<(), TryRuntimeError> { let individual = e.others.iter().filter(|e| e.who == nominator).collect::>(); let len = individual.len(); match len { 0 => { /* not supporting this validator at all. */ }, 1 => sum += individual[0].value, - _ => return Err("nominator cannot back a validator more than once."), + _ => + return Err( + "nominator cannot back a validator more than once.".into() + ), }; Ok(()) }) - .collect::>() + .collect::, _>>()?; + Ok(()) }) - .collect::>() + .collect::, _>>()?; + + Ok(()) } fn ensure_is_stash(who: &T::AccountId) -> Result<(), &'static str> { @@ -1811,7 +1823,7 @@ impl Pallet { Ok(()) } - fn ensure_ledger_consistent(ctrl: T::AccountId) -> Result<(), &'static str> { + fn ensure_ledger_consistent(ctrl: T::AccountId) -> Result<(), TryRuntimeError> { // ensures ledger.total == ledger.active + sum(ledger.unlocking). let ledger = Self::ledger(ctrl.clone()).ok_or("Not a controller.")?; let real_total: BalanceOf = diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 2b33573ac210f..35aa2626e3b7c 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -805,7 +805,7 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(n: BlockNumberFor) -> Result<(), &'static str> { + fn try_state(n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state(n) } } diff --git a/frame/support/procedural/src/pallet/expand/hooks.rs b/frame/support/procedural/src/pallet/expand/hooks.rs index d7d8bbded95d6..ef22f5a3af5be 100644 --- a/frame/support/procedural/src/pallet/expand/hooks.rs +++ b/frame/support/procedural/src/pallet/expand/hooks.rs @@ -105,7 +105,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { current_version, ); - return Err("On chain and current storage version do not match. Missing runtime upgrade?"); + return Err("On chain and current storage version do not match. Missing runtime upgrade?".into()); } } } else { @@ -128,7 +128,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { ); return Err("On chain storage version set, while the pallet doesn't \ - have the `#[pallet::storage_version(VERSION)]` attribute."); + have the `#[pallet::storage_version(VERSION)]` attribute.".into()); } } }; @@ -211,7 +211,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<#frame_support::sp_std::vec::Vec, &'static str> { + fn pre_upgrade() -> Result<#frame_support::sp_std::vec::Vec, #frame_support::sp_runtime::TryRuntimeError> { < Self as @@ -220,7 +220,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { } #[cfg(feature = "try-runtime")] - fn post_upgrade(state: #frame_support::sp_std::vec::Vec) -> Result<(), &'static str> { + fn post_upgrade(state: #frame_support::sp_std::vec::Vec) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { #post_storage_version_check < @@ -268,7 +268,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { fn try_state( n: ::BlockNumber, _s: #frame_support::traits::TryStateSelect - ) -> Result<(), &'static str> { + ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { #log_try_state < Self as #frame_support::traits::Hooks< diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index 071fb7c9d5aaf..47c7b22f5bf15 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -2097,7 +2097,7 @@ macro_rules! decl_module { fn try_state( _: <$trait_instance as $system::Config>::BlockNumber, _: $crate::traits::TryStateSelect, - ) -> Result<(), &'static str> { + ) -> Result<(), $crate::sp_runtime::TryRuntimeError> { let pallet_name = << $trait_instance as @@ -2144,12 +2144,12 @@ macro_rules! decl_module { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<$crate::sp_std::vec::Vec, &'static str> { + fn pre_upgrade() -> Result<$crate::sp_std::vec::Vec, $crate::sp_runtime::TryRuntimeError> { Ok($crate::sp_std::vec::Vec::new()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_: $crate::sp_std::vec::Vec) -> Result<(), &'static str> { + fn post_upgrade(_: $crate::sp_std::vec::Vec) -> Result<(), $crate::sp_runtime::TryRuntimeError> { Ok(()) } } @@ -2182,12 +2182,12 @@ macro_rules! decl_module { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<$crate::sp_std::vec::Vec, &'static str> { + fn pre_upgrade() -> Result<$crate::sp_std::vec::Vec, $crate::sp_runtime::TryRuntimeError> { Ok($crate::sp_std::vec::Vec::new()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_: $crate::sp_std::vec::Vec) -> Result<(), &'static str> { + fn post_upgrade(_: $crate::sp_std::vec::Vec) -> Result<(), $crate::sp_runtime::TryRuntimeError> { Ok(()) } } diff --git a/frame/support/src/migrations.rs b/frame/support/src/migrations.rs index 381f1feda9430..8bda2662a237e 100644 --- a/frame/support/src/migrations.rs +++ b/frame/support/src/migrations.rs @@ -184,7 +184,7 @@ impl, DbWeight: Get> frame_support::traits } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { let hashed_prefix = twox_128(P::get().as_bytes()); match contains_prefixed_key(&hashed_prefix) { true => log::info!("Found {} keys pre-removal 👀", P::get()), @@ -197,12 +197,12 @@ impl, DbWeight: Get> frame_support::traits } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { let hashed_prefix = twox_128(P::get().as_bytes()); match contains_prefixed_key(&hashed_prefix) { true => { log::error!("{} has keys remaining post-removal ❗", P::get()); - return Err("Keys remaining post-removal, this should never happen 🚨") + return Err("Keys remaining post-removal, this should never happen 🚨".into()) }, false => log::info!("No {} keys found post-removal 🎉", P::get()), }; diff --git a/frame/support/src/traits/hooks.rs b/frame/support/src/traits/hooks.rs index a4c6776572fac..12dcd6af0f791 100644 --- a/frame/support/src/traits/hooks.rs +++ b/frame/support/src/traits/hooks.rs @@ -22,6 +22,9 @@ use impl_trait_for_tuples::impl_for_tuples; use sp_runtime::traits::AtLeast32BitUnsigned; use sp_std::prelude::*; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + /// The block initialization trait. /// /// Implementing this lets you express what should happen for your pallet when the block is @@ -136,7 +139,7 @@ pub trait OnRuntimeUpgrade { /// Same as `on_runtime_upgrade`, but perform the optional `pre_upgrade` and `post_upgrade` as /// well. #[cfg(feature = "try-runtime")] - fn try_on_runtime_upgrade(checks: bool) -> Result { + fn try_on_runtime_upgrade(checks: bool) -> Result { let maybe_state = if checks { let _guard = frame_support::StorageNoopGuard::default(); let state = Self::pre_upgrade()?; @@ -167,7 +170,7 @@ pub trait OnRuntimeUpgrade { /// This hook must not write to any state, as it would make the main `on_runtime_upgrade` path /// inaccurate. #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { Ok(Vec::new()) } @@ -182,7 +185,7 @@ pub trait OnRuntimeUpgrade { /// This hook must not write to any state, as it would make the main `on_runtime_upgrade` path /// inaccurate. #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { Ok(()) } } @@ -201,7 +204,7 @@ impl OnRuntimeUpgrade for Tuple { /// consecutive migrations for the same pallet without errors. Therefore pre and post upgrade /// hooks for tuples are a noop. #[cfg(feature = "try-runtime")] - fn try_on_runtime_upgrade(checks: bool) -> Result { + fn try_on_runtime_upgrade(checks: bool) -> Result { let mut weight = Weight::zero(); let mut errors = Vec::new(); @@ -224,12 +227,12 @@ impl OnRuntimeUpgrade for Tuple { errors.iter().for_each(|err| { log::error!( target: "try-runtime", - "{}", + "{:?}", err ); }); - return Err("Detected multiple errors while executing `try_on_runtime_upgrade`, check the logs!") + return Err("Detected multiple errors while executing `try_on_runtime_upgrade`, check the logs!".into()) } Ok(weight) @@ -305,7 +308,7 @@ pub trait Hooks { /// /// This hook should not alter any storage. #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumber) -> Result<(), &'static str> { + fn try_state(_n: BlockNumber) -> Result<(), TryRuntimeError> { Ok(()) } @@ -317,7 +320,7 @@ pub trait Hooks { /// /// This hook is never meant to be executed on-chain but is meant to be used by testing tools. #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { Ok(Vec::new()) } @@ -329,7 +332,7 @@ pub trait Hooks { /// /// This hook is never meant to be executed on-chain but is meant to be used by testing tools. #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { Ok(()) } @@ -411,13 +414,13 @@ mod tests { } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { + fn pre_upgrade() -> Result, TryRuntimeError> { Pre::mutate(|s| s.push(stringify!($name))); Ok(Vec::new()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), &'static str> { + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { Post::mutate(|s| s.push(stringify!($name))); Ok(()) } diff --git a/frame/support/src/traits/try_runtime.rs b/frame/support/src/traits/try_runtime.rs index bebc248721c99..cb18f9d5b71c0 100644 --- a/frame/support/src/traits/try_runtime.rs +++ b/frame/support/src/traits/try_runtime.rs @@ -19,6 +19,7 @@ use impl_trait_for_tuples::impl_for_tuples; use sp_arithmetic::traits::AtLeast32BitUnsigned; +use sp_runtime::TryRuntimeError; use sp_std::prelude::*; /// Which state tests to execute. @@ -129,7 +130,7 @@ impl core::str::FromStr for UpgradeCheckSelect { /// This hook should not alter any storage. pub trait TryState { /// Execute the state checks. - fn try_state(_: BlockNumber, _: Select) -> Result<(), &'static str>; + fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError>; } #[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] @@ -139,7 +140,7 @@ impl TryState Result<(), &'static str> { + fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { Select::None => Ok(()), Select::All => { @@ -148,7 +149,7 @@ impl TryState { - let functions: &[fn(BlockNumber, Select) -> Result<(), &'static str>] = + let functions: &[fn(BlockNumber, Select) -> Result<(), TryRuntimeError>] = &[for_tuples!(#( Tuple::try_state ),*)]; let skip = n.clone() % (functions.len() as u32).into(); let skip: u32 = @@ -163,7 +164,7 @@ impl TryState { let try_state_fns: &[( &'static str, - fn(BlockNumber, Select) -> Result<(), &'static str>, + fn(BlockNumber, Select) -> Result<(), TryRuntimeError>, )] = &[for_tuples!( #( (::name(), Tuple::try_state) ),* )]; diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 7f15ad1f9298a..f6b5858f113fa 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -2110,9 +2110,11 @@ fn post_runtime_upgrade_detects_storage_version_issues() { // Call `on_genesis` to put the storage version of `Example` into the storage. Example::on_genesis(); // The version isn't changed, we should detect it. - assert!(Executive::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost) - .unwrap_err() - .contains("On chain and current storage version do not match")); + assert!( + Executive::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost).unwrap_err() == + "On chain and current storage version do not match. Missing runtime upgrade?" + .into() + ); }); TestExternalities::default().execute_with(|| { @@ -2138,9 +2140,12 @@ fn post_runtime_upgrade_detects_storage_version_issues() { // `CustomUpgradePallet4` will set a storage version for `Example4` while this doesn't has // any storage version "enabled". - assert!(ExecutiveWithUpgradePallet4::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost) - .unwrap_err() - .contains("On chain storage version set, while the pallet doesn't")); + assert!( + ExecutiveWithUpgradePallet4::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost) + .unwrap_err() == "On chain storage version set, while the pallet \ + doesn't have the `#[pallet::storage_version(VERSION)]` attribute." + .into() + ); }); } diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 7be5bebf5de80..363881e431e0e 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -784,6 +784,9 @@ pub type ApplyExtrinsicResult = pub type ApplyExtrinsicResultWithInfo = Result, transaction_validity::TransactionValidityError>; +/// The error type used as return type in try runtime hooks. +pub type TryRuntimeError = DispatchError; + /// Verify a signature on an encoded value in a lazy manner. This can be /// an optimization if the signature scheme has an "unsigned" escape hash. pub fn verify_encoded_lazy( diff --git a/utils/frame/try-runtime/cli/src/lib.rs b/utils/frame/try-runtime/cli/src/lib.rs index 4893c464be516..3c60184adbbdd 100644 --- a/utils/frame/try-runtime/cli/src/lib.rs +++ b/utils/frame/try-runtime/cli/src/lib.rs @@ -122,10 +122,10 @@ //! //! ```ignore //! #[cfg(feature = "try-runtime")] -//! fn pre_upgrade() -> Result, &'static str> {} +//! fn pre_upgrade() -> Result, TryRuntimeError> {} //! //! #[cfg(feature = "try-runtime")] -//! fn post_upgrade(state: Vec) -> Result<(), &'static str> {} +//! fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> {} //! ``` //! //! (The pallet macro syntax will support this simply as a part of `#[pallet::hooks]`). @@ -141,7 +141,7 @@ //! //! ```ignore //! #[cfg(feature = "try-runtime")] -//! fn try_state(_: BlockNumber) -> Result<(), &'static str> {} +//! fn try_state(_: BlockNumber) -> Result<(), TryRuntimeError> {} //! ``` //! //! which is called on numerous code paths in the try-runtime tool. These checks should ensure that