Use decl_error in stacking module (#4387)

substrate/frame/staking/src/lib.rs

@@ -258,7 +258,7 @@ pub mod inflation;
 use sp_std::{prelude::*, result};
 use codec::{HasCompact, Encode, Decode};
 use frame_support::{
-	decl_module, decl_event, decl_storage, ensure,
+	decl_module, decl_event, decl_storage, ensure, decl_error,
 	weights::SimpleDispatchInfo,
 	traits::{
 		Currency, OnFreeBalanceZero, LockIdentifier, LockableCurrency,
@@ -272,7 +272,7 @@ use sp_runtime::{
 	curve::PiecewiseLinear,
 	traits::{
 		Convert, Zero, One, StaticLookup, CheckedSub, Saturating, Bounded, SaturatedConversion,
-		SimpleArithmetic, EnsureOrigin,
+		SimpleArithmetic, EnsureOrigin, ModuleDispatchError,
 	}
 };
 use sp_staking::{
@@ -783,6 +783,32 @@ decl_event!(
 	}
 );
 
+decl_error! {
+	/// Error for the stacking module.
+	pub enum Error {
+		/// Not a controller account.
+		NotController,
+		/// Not a stash account.
+		NotStash,
+		/// Stash is already bonded.
+		AlreadyBonded,
+		/// Controller is already paired.
+		AlreadyPaired,
+		/// Should be the root origin or the `T::SlashCancelOrigin`.
+		BadOrigin,
+		/// Targets cannot be empty.
+		EmptyTargets,
+		/// Duplicate index.
+		DuplicateIndex,
+		/// Slash record index out of bounds.
+		InvalidSlashIndex,
+		/// Can not bond with value less than minimum balance.
+		InsufficientValue,
+		/// Can not schedule more unlock chunks.
+		NoMoreChunks,
+	}
+}
+
 decl_module! {
 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
 		/// Number of sessions per era.
@@ -791,6 +817,8 @@ decl_module! {
 		/// Number of eras that staked funds must remain bonded for.
 		const BondingDuration: EraIndex = T::BondingDuration::get();
 
+		type Error = Error;
+
 		fn deposit_event() = default;
 
 		fn on_initialize() {
@@ -825,21 +853,21 @@ decl_module! {
 			#[compact] value: BalanceOf<T>,
 			payee: RewardDestination
 		) {
-			let stash = ensure_signed(origin)?;
+			let stash = ensure_signed(origin).map_err(|e| e.as_str())?;
 
 			if <Bonded<T>>::exists(&stash) {
-				return Err("stash already bonded")
+				return Err(Error::AlreadyBonded)
 			}
 
 			let controller = T::Lookup::lookup(controller)?;
 
 			if <Ledger<T>>::exists(&controller) {
-				return Err("controller already paired")
+				return Err(Error::AlreadyPaired)
 			}
 
 			// reject a bond which is considered to be _dust_.
 			if value < T::Currency::minimum_balance() {
-				return Err("can not bond with value less than minimum balance")
+				return Err(Error::InsufficientValue)
 			}
 
 			// You're auto-bonded forever, here. We might improve this by only bonding when
@@ -869,10 +897,10 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(500_000)]
 		fn bond_extra(origin, #[compact] max_additional: BalanceOf<T>) {
-			let stash = ensure_signed(origin)?;
+			let stash = ensure_signed(origin).map_err(|e| e.as_str())?;
 
-			let controller = Self::bonded(&stash).ok_or("not a stash")?;
+			let controller = Self::bonded(&stash).ok_or(Error::NotStash)?;
-			let mut ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let mut ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 
 			let stash_balance = T::Currency::free_balance(&stash);
 
@@ -909,11 +937,11 @@ decl_module! {
 		/// </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(400_000)]
 		fn unbond(origin, #[compact] value: BalanceOf<T>) {
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let mut ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let mut ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			ensure!(
 				ledger.unlocking.len() < MAX_UNLOCKING_CHUNKS,
-				"can not schedule more unlock chunks"
+				Error::NoMoreChunks
 			);
 
 			let mut value = value.min(ledger.active);
@@ -951,8 +979,8 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(400_000)]
 		fn withdraw_unbonded(origin) {
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			let ledger = ledger.consolidate_unlocked(Self::current_era());
 
 			if ledger.unlocking.is_empty() && ledger.active.is_zero() {
@@ -985,8 +1013,8 @@ decl_module! {
 		fn validate(origin, prefs: ValidatorPrefs) {
 			Self::ensure_storage_upgraded();
 
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			let stash = &ledger.stash;
 			<Nominators<T>>::remove(stash);
 			<Validators<T>>::insert(stash, prefs);
@@ -1007,10 +1035,10 @@ decl_module! {
 		fn nominate(origin, targets: Vec<<T::Lookup as StaticLookup>::Source>) {
 			Self::ensure_storage_upgraded();
 
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			let stash = &ledger.stash;
-			ensure!(!targets.is_empty(), "targets cannot be empty");
+			ensure!(!targets.is_empty(), Error::EmptyTargets);
 			let targets = targets.into_iter()
 				.take(MAX_NOMINATIONS)
 				.map(|t| T::Lookup::lookup(t))
@@ -1039,8 +1067,8 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(500_000)]
 		fn chill(origin) {
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			Self::chill_stash(&ledger.stash);
 		}
 
@@ -1057,8 +1085,8 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(500_000)]
 		fn set_payee(origin, payee: RewardDestination) {
-			let controller = ensure_signed(origin)?;
+			let controller = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let ledger = Self::ledger(&controller).ok_or("not a controller")?;
+			let ledger = Self::ledger(&controller).ok_or(Error::NotController)?;
 			let stash = &ledger.stash;
 			<Payee<T>>::insert(stash, payee);
 		}
@@ -1076,11 +1104,11 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(750_000)]
 		fn set_controller(origin, controller: <T::Lookup as StaticLookup>::Source) {
-			let stash = ensure_signed(origin)?;
+			let stash = ensure_signed(origin).map_err(|e| e.as_str())?;
-			let old_controller = Self::bonded(&stash).ok_or("not a stash")?;
+			let old_controller = Self::bonded(&stash).ok_or(Error::NotStash)?;
 			let controller = T::Lookup::lookup(controller)?;
 			if <Ledger<T>>::exists(&controller) {
-				return Err("controller already paired")
+				return Err(Error::AlreadyPaired)
 			}
 			if controller != old_controller {
 				<Bonded<T>>::insert(&stash, &controller);
@@ -1093,7 +1121,7 @@ decl_module! {
 		/// The ideal number of validators.
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn set_validator_count(origin, #[compact] new: u32) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 			ValidatorCount::put(new);
 		}
 
@@ -1106,7 +1134,7 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn force_no_eras(origin) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 			ForceEra::put(Forcing::ForceNone);
 		}
 
@@ -1118,21 +1146,21 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn force_new_era(origin) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 			ForceEra::put(Forcing::ForceNew);
 		}
 
 		/// Set the validators who cannot be slashed (if any).
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn set_invulnerables(origin, validators: Vec<T::AccountId>) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 			<Invulnerables<T>>::put(validators);
 		}
 
 		/// Force a current staker to become completely unstaked, immediately.
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn force_unstake(origin, stash: T::AccountId) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 
 			// remove the lock.
 			T::Currency::remove_lock(STAKING_ID, &stash);
@@ -1147,7 +1175,7 @@ decl_module! {
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FreeOperational]
 		fn force_new_era_always(origin) {
-			ensure_root(origin)?;
+			ensure_root(origin).map_err(|e| e.as_str())?;
 			ForceEra::put(Forcing::ForceAlways);
 		}
 
@@ -1163,7 +1191,7 @@ decl_module! {
 			T::SlashCancelOrigin::try_origin(origin)
 				.map(|_| ())
 				.or_else(ensure_root)
-				.map_err(|_| "bad origin")?;
+				.map_err(|_| Error::BadOrigin)?;
 
 			let mut slash_indices = slash_indices;
 			slash_indices.sort_unstable();
@@ -1173,12 +1201,12 @@ decl_module! {
 				let index = index as usize;
 
 				// if `index` is not duplicate, `removed` must be <= index.
-				ensure!(removed <= index, "duplicate index");
+				ensure!(removed <= index, Error::DuplicateIndex);
 
 				// all prior removals were from before this index, since the
 				// list is sorted.
 				let index = index - removed;
-				ensure!(index < unapplied.len(), "slash record index out of bounds");
+				ensure!(index < unapplied.len(), Error::InvalidSlashIndex);
 
 				unapplied.remove(index);
 			}

substrate/frame/staking/src/tests.rs

@@ -35,7 +35,7 @@ fn force_unstake_works() {
 			"account liquidity restrictions prevent withdrawal"
 		);
 		// Force unstake requires root.
-		assert_noop!(Staking::force_unstake(Origin::signed(11), 11), "RequireRootOrigin");
+		assert_noop!(Staking::force_unstake(Origin::signed(11), 11), "RequireRootOrigin".into());
 		// We now force them to unstake
 		assert_ok!(Staking::force_unstake(Origin::ROOT, 11));
 		// No longer bonded.
@@ -142,7 +142,7 @@ fn change_controller_works() {
 
 		assert_noop!(
 			Staking::validate(Origin::signed(10), ValidatorPrefs::default()),
-			"not a controller"
+			Error::NotController,
 		);
 		assert_ok!(Staking::validate(Origin::signed(5), ValidatorPrefs::default()));
 	})
@@ -680,10 +680,10 @@ fn double_staking_should_fail() {
 		// 4 = not used so far, 1 stashed => not allowed.
 		assert_noop!(
 			Staking::bond(Origin::signed(1), 4, arbitrary_value,
-			RewardDestination::default()), "stash already bonded"
+			RewardDestination::default()), Error::AlreadyBonded,
 		);
 		// 1 = stashed => attempting to nominate should fail.
-		assert_noop!(Staking::nominate(Origin::signed(1), vec![1]), "not a controller");
+		assert_noop!(Staking::nominate(Origin::signed(1), vec![1]), Error::NotController);
 		// 2 = controller  => nominating should work.
 		assert_ok!(Staking::nominate(Origin::signed(2), vec![1]));
 	});
@@ -705,7 +705,7 @@ fn double_controlling_should_fail() {
 		// 2 = controller, 3 stashed (Note that 2 is reused.) => no-op
 		assert_noop!(
 			Staking::bond(Origin::signed(3), 2, arbitrary_value, RewardDestination::default()),
-			"controller already paired",
+			Error::AlreadyPaired,
 		);
 	});
 }
@@ -1152,11 +1152,11 @@ fn too_many_unbond_calls_should_not_work() {
 		// locked at era 1 until 4
 		assert_ok!(Staking::unbond(Origin::signed(10), 1));
 		// can't do more.
-		assert_noop!(Staking::unbond(Origin::signed(10), 1), "can not schedule more unlock chunks");
+		assert_noop!(Staking::unbond(Origin::signed(10), 1), Error::NoMoreChunks);
 
 		start_era(3);
 
-		assert_noop!(Staking::unbond(Origin::signed(10), 1), "can not schedule more unlock chunks");
+		assert_noop!(Staking::unbond(Origin::signed(10), 1), Error::NoMoreChunks);
 		// free up.
 		assert_ok!(Staking::withdraw_unbonded(Origin::signed(10)));
 
@@ -1422,7 +1422,7 @@ fn bond_with_no_staked_value() {
 			// Can't bond with 1
 			assert_noop!(
 				Staking::bond(Origin::signed(1), 2, 1, RewardDestination::Controller),
-				"can not bond with value less than minimum balance",
+				Error::InsufficientValue,
 			);
 			// bonded with absolute minimum value possible.
 			assert_ok!(Staking::bond(Origin::signed(1), 2, 5, RewardDestination::Controller));