From f20bb8b196103dfa0e226dfbcb6c87d649fa4d2a Mon Sep 17 00:00:00 2001 From: Stanislav Tkach Date: Tue, 17 Dec 2019 07:03:24 +0100 Subject: [PATCH] Use decl_error in stacking module (#4387) --- substrate/frame/staking/src/lib.rs | 98 ++++++++++++++++++---------- substrate/frame/staking/src/tests.rs | 16 ++--- 2 files changed, 71 insertions(+), 43 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 3d641e24a5..bc43b54e91 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/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 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, payee: RewardDestination ) { - let stash = ensure_signed(origin)?; + let stash = ensure_signed(origin).map_err(|e| e.as_str())?; if >::exists(&stash) { - return Err("stash already bonded") + return Err(Error::AlreadyBonded) } let controller = T::Lookup::lookup(controller)?; if >::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 = SimpleDispatchInfo::FixedNormal(500_000)] fn bond_extra(origin, #[compact] max_additional: BalanceOf) { - 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 mut ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = Self::bonded(&stash).ok_or(Error::NotStash)?; + 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 = SimpleDispatchInfo::FixedNormal(400_000)] fn unbond(origin, #[compact] value: BalanceOf) { - let controller = ensure_signed(origin)?; - let mut ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + 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 = SimpleDispatchInfo::FixedNormal(400_000)] fn withdraw_unbonded(origin) { - let controller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + 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 ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + let ledger = Self::ledger(&controller).ok_or(Error::NotController)?; let stash = &ledger.stash; >::remove(stash); >::insert(stash, prefs); @@ -1007,10 +1035,10 @@ decl_module! { fn nominate(origin, targets: Vec<::Source>) { Self::ensure_storage_upgraded(); - let controller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + 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 = SimpleDispatchInfo::FixedNormal(500_000)] fn chill(origin) { - let controller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + let ledger = Self::ledger(&controller).ok_or(Error::NotController)?; Self::chill_stash(&ledger.stash); } @@ -1057,8 +1085,8 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(500_000)] fn set_payee(origin, payee: RewardDestination) { - let controller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or("not a controller")?; + let controller = ensure_signed(origin).map_err(|e| e.as_str())?; + let ledger = Self::ledger(&controller).ok_or(Error::NotController)?; let stash = &ledger.stash; >::insert(stash, payee); } @@ -1076,11 +1104,11 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(750_000)] fn set_controller(origin, controller: ::Source) { - let stash = ensure_signed(origin)?; - let old_controller = Self::bonded(&stash).ok_or("not a stash")?; + let stash = ensure_signed(origin).map_err(|e| e.as_str())?; + let old_controller = Self::bonded(&stash).ok_or(Error::NotStash)?; let controller = T::Lookup::lookup(controller)?; if >::exists(&controller) { - return Err("controller already paired") + return Err(Error::AlreadyPaired) } if controller != old_controller { >::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 = 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 = 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) { - ensure_root(origin)?; + ensure_root(origin).map_err(|e| e.as_str())?; >::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 = 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); } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index c31cdf7611..33bf860b2c 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/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));