diff --git a/substrate/frame/membership/src/lib.rs b/substrate/frame/membership/src/lib.rs index 0a7f8ec7fc..3a65f1604e 100644 --- a/substrate/frame/membership/src/lib.rs +++ b/substrate/frame/membership/src/lib.rs @@ -24,7 +24,7 @@ use sp_std::prelude::*; use frame_support::{ - decl_module, decl_storage, decl_event, + decl_module, decl_storage, decl_event, decl_error, traits::{ChangeMembers, InitializeMembers}, weights::SimpleDispatchInfo, }; @@ -93,6 +93,16 @@ decl_event!( } ); +decl_error! { + /// Error for the nicks module. + pub enum Error for Module, I: Instance> { + /// Already a member. + AlreadyMember, + /// Not a member. + NotMember, + } +} + decl_module! { pub struct Module, I: Instance=DefaultInstance> for enum Call @@ -107,11 +117,10 @@ decl_module! { fn add_member(origin, who: T::AccountId) { T::AddOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let mut members = >::get(); - let location = members.binary_search(&who).err().ok_or("already a member")?; + let location = members.binary_search(&who).err().ok_or(Error::::AlreadyMember)?; members.insert(location, who.clone()); >::put(&members); @@ -127,11 +136,10 @@ decl_module! { fn remove_member(origin, who: T::AccountId) { T::RemoveOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let mut members = >::get(); - let location = members.binary_search(&who).ok().ok_or("not a member")?; + let location = members.binary_search(&who).ok().ok_or(Error::::NotMember)?; members.remove(location); >::put(&members); @@ -147,14 +155,13 @@ decl_module! { fn swap_member(origin, remove: T::AccountId, add: T::AccountId) { T::SwapOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; if remove == add { return Ok(()) } let mut members = >::get(); - let location = members.binary_search(&remove).ok().ok_or("not a member")?; - let _ = members.binary_search(&add).err().ok_or("already a member")?; + let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; + let _ = members.binary_search(&add).err().ok_or(Error::::AlreadyMember)?; members[location] = add.clone(); members.sort(); >::put(&members); @@ -176,8 +183,7 @@ decl_module! { fn reset_members(origin, members: Vec) { T::ResetOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let mut members = members; members.sort(); @@ -198,8 +204,8 @@ decl_module! { if remove != new { let mut members = >::get(); - let location = members.binary_search(&remove).ok().ok_or("not a member")?; - let _ = members.binary_search(&new).err().ok_or("already a member")?; + let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; + let _ = members.binary_search(&new).err().ok_or(Error::::AlreadyMember)?; members[location] = new.clone(); members.sort(); >::put(&members); @@ -225,7 +231,7 @@ mod tests { use sp_core::H256; // The testing primitives are very useful for avoiding having to work with signatures // or public keys. `u64` is used as the `AccountId` and no `Signature`s are requried. - use sp_runtime::{Perbill, traits::{BlakeTwo256, IdentityLookup}, testing::Header}; + use sp_runtime::{Perbill, traits::{BlakeTwo256, IdentityLookup}, testing::Header, traits::BadOrigin}; use frame_system::EnsureSignedBy; impl_outer_origin! { @@ -328,8 +334,8 @@ mod tests { #[test] fn add_member_works() { new_test_ext().execute_with(|| { - assert_noop!(Membership::add_member(Origin::signed(5), 15), "bad origin"); - assert_noop!(Membership::add_member(Origin::signed(1), 10), "already a member"); + assert_noop!(Membership::add_member(Origin::signed(5), 15), BadOrigin); + assert_noop!(Membership::add_member(Origin::signed(1), 10), Error::::AlreadyMember); assert_ok!(Membership::add_member(Origin::signed(1), 15)); assert_eq!(Membership::members(), vec![10, 15, 20, 30]); assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); @@ -339,8 +345,8 @@ mod tests { #[test] fn remove_member_works() { new_test_ext().execute_with(|| { - assert_noop!(Membership::remove_member(Origin::signed(5), 20), "bad origin"); - assert_noop!(Membership::remove_member(Origin::signed(2), 15), "not a member"); + assert_noop!(Membership::remove_member(Origin::signed(5), 20), BadOrigin); + assert_noop!(Membership::remove_member(Origin::signed(2), 15), Error::::NotMember); assert_ok!(Membership::remove_member(Origin::signed(2), 20)); assert_eq!(Membership::members(), vec![10, 30]); assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); @@ -350,9 +356,9 @@ mod tests { #[test] fn swap_member_works() { new_test_ext().execute_with(|| { - assert_noop!(Membership::swap_member(Origin::signed(5), 10, 25), "bad origin"); - assert_noop!(Membership::swap_member(Origin::signed(3), 15, 25), "not a member"); - assert_noop!(Membership::swap_member(Origin::signed(3), 10, 30), "already a member"); + assert_noop!(Membership::swap_member(Origin::signed(5), 10, 25), BadOrigin); + assert_noop!(Membership::swap_member(Origin::signed(3), 15, 25), Error::::NotMember); + assert_noop!(Membership::swap_member(Origin::signed(3), 10, 30), Error::::AlreadyMember); assert_ok!(Membership::swap_member(Origin::signed(3), 20, 20)); assert_eq!(Membership::members(), vec![10, 20, 30]); assert_ok!(Membership::swap_member(Origin::signed(3), 10, 25)); @@ -373,8 +379,8 @@ mod tests { #[test] fn change_key_works() { new_test_ext().execute_with(|| { - assert_noop!(Membership::change_key(Origin::signed(3), 25), "not a member"); - assert_noop!(Membership::change_key(Origin::signed(10), 20), "already a member"); + assert_noop!(Membership::change_key(Origin::signed(3), 25), Error::::NotMember); + assert_noop!(Membership::change_key(Origin::signed(10), 20), Error::::AlreadyMember); assert_ok!(Membership::change_key(Origin::signed(10), 40)); assert_eq!(Membership::members(), vec![20, 30, 40]); assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); @@ -393,7 +399,7 @@ mod tests { #[test] fn reset_members_works() { new_test_ext().execute_with(|| { - assert_noop!(Membership::reset_members(Origin::signed(1), vec![20, 40, 30]), "bad origin"); + assert_noop!(Membership::reset_members(Origin::signed(1), vec![20, 40, 30]), BadOrigin); assert_ok!(Membership::reset_members(Origin::signed(4), vec![20, 40, 30])); assert_eq!(Membership::members(), vec![20, 30, 40]); assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); diff --git a/substrate/frame/nicks/src/lib.rs b/substrate/frame/nicks/src/lib.rs index d2f0b4d8c9..f407c4aad0 100644 --- a/substrate/frame/nicks/src/lib.rs +++ b/substrate/frame/nicks/src/lib.rs @@ -43,7 +43,7 @@ use sp_runtime::{ traits::{StaticLookup, EnsureOrigin, Zero} }; use frame_support::{ - decl_module, decl_event, decl_storage, ensure, + decl_module, decl_event, decl_storage, ensure, decl_error, traits::{Currency, ReservableCurrency, OnUnbalanced, Get}, weights::SimpleDispatchInfo, }; @@ -97,9 +97,23 @@ decl_event!( } ); +decl_error! { + /// Error for the nicks module. + pub enum Error for Module { + /// A name is too short. + TooShort, + /// A name is too long. + TooLong, + /// An account in't named. + Unnamed, + } +} + decl_module! { // Simple declaration of the `Module` type. Lets the macro know what it's working on. pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; /// Reservation fee. @@ -131,8 +145,8 @@ decl_module! { fn set_name(origin, name: Vec) { let sender = ensure_signed(origin)?; - ensure!(name.len() >= T::MinLength::get(), "Name too short"); - ensure!(name.len() <= T::MaxLength::get(), "Name too long"); + ensure!(name.len() >= T::MinLength::get(), Error::::TooShort,); + ensure!(name.len() <= T::MaxLength::get(), Error::::TooLong); let deposit = if let Some((_, deposit)) = >::get(&sender) { Self::deposit_event(RawEvent::NameSet(sender.clone())); @@ -160,7 +174,7 @@ decl_module! { fn clear_name(origin) { let sender = ensure_signed(origin)?; - let deposit = >::take(&sender).ok_or("Not named")?.1; + let deposit = >::take(&sender).ok_or(Error::::Unnamed)?.1; let _ = T::Currency::unreserve(&sender, deposit.clone()); @@ -184,13 +198,12 @@ decl_module! { fn kill_name(origin, target: ::Source) { T::ForceOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; // Figure out who we're meant to be clearing. let target = T::Lookup::lookup(target)?; // Grab their deposit (and check that they have one). - let deposit = >::take(&target).ok_or("Not named")?.1; + let deposit = >::take(&target).ok_or(Error::::Unnamed)?.1; // Slash their deposit from them. T::Slashed::on_unbalanced(T::Currency::slash_reserved(&target, deposit.clone()).0); @@ -213,8 +226,7 @@ decl_module! { fn force_name(origin, target: ::Source, name: Vec) { T::ForceOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let target = T::Lookup::lookup(target)?; let deposit = >::get(&target).map(|x| x.1).unwrap_or_else(Zero::zero); @@ -235,7 +247,7 @@ mod tests { // The testing primitives are very useful for avoiding having to work with signatures // or public keys. `u64` is used as the `AccountId` and no `Signature`s are required. use sp_runtime::{ - Perbill, testing::Header, traits::{BlakeTwo256, IdentityLookup}, + Perbill, testing::Header, traits::{BlakeTwo256, IdentityLookup, BadOrigin}, }; impl_outer_origin! { @@ -336,7 +348,7 @@ mod tests { new_test_ext().execute_with(|| { assert_noop!( Nicks::set_name(Origin::signed(2), b"Dr. David Brubeck, III".to_vec()), - "Name too long" + Error::::TooLong, ); assert_ok!(Nicks::set_name(Origin::signed(2), b"Dave".to_vec())); @@ -369,18 +381,18 @@ mod tests { #[test] fn error_catching_should_work() { new_test_ext().execute_with(|| { - assert_noop!(Nicks::clear_name(Origin::signed(1)), "Not named"); + assert_noop!(Nicks::clear_name(Origin::signed(1)), Error::::Unnamed); assert_noop!(Nicks::set_name(Origin::signed(3), b"Dave".to_vec()), "not enough free funds"); - assert_noop!(Nicks::set_name(Origin::signed(1), b"Ga".to_vec()), "Name too short"); + assert_noop!(Nicks::set_name(Origin::signed(1), b"Ga".to_vec()), Error::::TooShort); assert_noop!( Nicks::set_name(Origin::signed(1), b"Gavin James Wood, Esquire".to_vec()), - "Name too long" + Error::::TooLong ); assert_ok!(Nicks::set_name(Origin::signed(1), b"Dave".to_vec())); - assert_noop!(Nicks::kill_name(Origin::signed(2), 1), "bad origin"); - assert_noop!(Nicks::force_name(Origin::signed(2), 1, b"Whatever".to_vec()), "bad origin"); + assert_noop!(Nicks::kill_name(Origin::signed(2), 1), BadOrigin); + assert_noop!(Nicks::force_name(Origin::signed(2), 1, b"Whatever".to_vec()), BadOrigin); }); } } diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index 65a867df60..685345aff6 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -94,7 +94,7 @@ use sp_std::{ prelude::*, }; use frame_support::{ - decl_module, decl_storage, decl_event, ensure, + decl_module, decl_storage, decl_event, ensure, decl_error, traits::{ChangeMembers, InitializeMembers, Currency, Get, ReservableCurrency}, }; use frame_system::{self as system, ensure_root, ensure_signed}; @@ -222,11 +222,25 @@ decl_event!( } ); +decl_error! { + /// Error for the scored-pool module. + pub enum Error for Module, I: Instance> { + /// Already a member. + AlreadyInPool, + /// Index out of bounds. + InvalidIndex, + /// Index does not match requested account. + WrongAccountIndex, + } +} + decl_module! { pub struct Module, I: Instance=DefaultInstance> for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; /// Every `Period` blocks the `Members` set is refreshed from the @@ -251,11 +265,10 @@ decl_module! { /// the index of the transactor in the `Pool`. pub fn submit_candidacy(origin) { let who = ensure_signed(origin)?; - ensure!(!>::exists(&who), "already a member"); + ensure!(!>::exists(&who), Error::::AlreadyInPool); let deposit = T::CandidateDeposit::get(); - T::Currency::reserve(&who, deposit) - .map_err(|_| "balance too low to submit candidacy")?; + T::Currency::reserve(&who, deposit)?; // can be inserted as last element in pool, since entities with // `None` are always sorted to the end. @@ -305,8 +318,7 @@ decl_module! { ) { T::KickOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let who = T::Lookup::lookup(dest)?; @@ -331,8 +343,7 @@ decl_module! { ) { T::ScoreOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| "bad origin")?; + .or_else(ensure_root)?; let who = T::Lookup::lookup(dest)?; @@ -414,7 +425,7 @@ impl, I: Instance> Module { mut pool: PoolT, remove: T::AccountId, index: u32 - ) -> Result<(), &'static str> { + ) -> Result<(), Error> { // all callers of this function in this module also check // the index for validity before calling this function. // nevertheless we check again here, to assert that there was @@ -444,11 +455,11 @@ impl, I: Instance> Module { pool: &PoolT, who: &T::AccountId, index: u32 - ) -> Result<(), &'static str> { - ensure!(index < pool.len() as u32, "index out of bounds"); + ) -> Result<(), Error> { + ensure!(index < pool.len() as u32, Error::::InvalidIndex); let (index_who, _index_score) = &pool[index as usize]; - ensure!(index_who == who, "index does not match requested account"); + ensure!(index_who == who, Error::::WrongAccountIndex); Ok(()) } diff --git a/substrate/frame/scored-pool/src/tests.rs b/substrate/frame/scored-pool/src/tests.rs index 0b3ede9ee0..6b6649f73c 100644 --- a/substrate/frame/scored-pool/src/tests.rs +++ b/substrate/frame/scored-pool/src/tests.rs @@ -20,15 +20,12 @@ use super::*; use mock::*; use frame_support::{assert_ok, assert_noop}; -use sp_runtime::traits::OnInitialize; +use sp_runtime::traits::{OnInitialize, BadOrigin}; type ScoredPool = Module; type System = frame_system::Module; type Balances = pallet_balances::Module; -const OOB_ERR: &str = "index out of bounds"; -const INDEX_ERR: &str = "index does not match requested account"; - #[test] fn query_membership_works() { new_test_ext().execute_with(|| { @@ -44,11 +41,11 @@ fn submit_candidacy_must_not_work() { new_test_ext().execute_with(|| { assert_noop!( ScoredPool::submit_candidacy(Origin::signed(99)), - "balance too low to submit candidacy" + "not enough free funds" ); assert_noop!( ScoredPool::submit_candidacy(Origin::signed(40)), - "already a member" + Error::::AlreadyInPool ); }); } @@ -111,7 +108,7 @@ fn kicking_works_only_for_authorized() { new_test_ext().execute_with(|| { let who = 40; let index = find_in_pool(who).expect("entity must be in pool") as u32; - assert_noop!(ScoredPool::kick(Origin::signed(99), who, index), "bad origin"); + assert_noop!(ScoredPool::kick(Origin::signed(99), who, index), BadOrigin); }); } @@ -203,7 +200,7 @@ fn withdraw_candidacy_must_only_work_for_members() { new_test_ext().execute_with(|| { let who = 77; let index = 0; - assert_noop!( ScoredPool::withdraw_candidacy(Origin::signed(who), index), INDEX_ERR); + assert_noop!( ScoredPool::withdraw_candidacy(Origin::signed(who), index), Error::::WrongAccountIndex); }); } @@ -212,9 +209,9 @@ fn oob_index_should_abort() { new_test_ext().execute_with(|| { let who = 40; let oob_index = ScoredPool::pool().len() as u32; - assert_noop!(ScoredPool::withdraw_candidacy(Origin::signed(who), oob_index), OOB_ERR); - assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, oob_index, 99), OOB_ERR); - assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, oob_index), OOB_ERR); + assert_noop!(ScoredPool::withdraw_candidacy(Origin::signed(who), oob_index), Error::::InvalidIndex); + assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, oob_index, 99), Error::::InvalidIndex); + assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, oob_index), Error::::InvalidIndex); }); } @@ -223,9 +220,9 @@ fn index_mismatches_should_abort() { new_test_ext().execute_with(|| { let who = 40; let index = 3; - assert_noop!(ScoredPool::withdraw_candidacy(Origin::signed(who), index), INDEX_ERR); - assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, index, 99), INDEX_ERR); - assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, index), INDEX_ERR); + assert_noop!(ScoredPool::withdraw_candidacy(Origin::signed(who), index), Error::::WrongAccountIndex); + assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, index, 99), Error::::WrongAccountIndex); + assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, index), Error::::WrongAccountIndex); }); } diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 5b3e4e2aee..79bace0d4a 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -125,7 +125,7 @@ use sp_runtime::{KeyTypeId, Perbill, RuntimeAppPublic, BoundToRuntimeAppPublic}; use frame_support::weights::SimpleDispatchInfo; use sp_runtime::traits::{Convert, Zero, Member, OpaqueKeys}; use sp_staking::SessionIndex; -use frame_support::{dispatch, ConsensusEngineId, decl_module, decl_event, decl_storage}; +use frame_support::{dispatch, ConsensusEngineId, decl_module, decl_event, decl_storage, decl_error}; use frame_support::{ensure, traits::{OnFreeBalanceZero, Get, FindAuthor, ValidatorRegistration}, Parameter}; use frame_system::{self as system, ensure_signed}; @@ -464,12 +464,26 @@ decl_event!( } ); +decl_error! { + /// Error for the session module. + pub enum Error for Module { + /// Invalid ownership proof. + InvalidProof, + /// No associated validator ID for account. + NoAssociatedValidatorId, + /// Registered duplicate key. + DuplicatedKey, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { /// Used as first key for `NextKeys` and `KeyOwner` to put all the data into the same branch /// of the trie. const DEDUP_KEY_PREFIX: &[u8] = DEDUP_KEY_PREFIX; + type Error = Error; + fn deposit_event() = default; /// Sets the session key(s) of the function caller to `key`. @@ -486,12 +500,9 @@ decl_module! { fn set_keys(origin, keys: T::Keys, proof: Vec) -> dispatch::DispatchResult { let who = ensure_signed(origin)?; - ensure!(keys.ownership_proof_is_valid(&proof), "invalid ownership proof"); + ensure!(keys.ownership_proof_is_valid(&proof), Error::::InvalidProof); - let who = match T::ValidatorIdOf::convert(who) { - Some(val_id) => val_id, - None => Err("no associated validator ID for account.")?, - }; + let who = T::ValidatorIdOf::convert(who).ok_or(Error::::NoAssociatedValidatorId)?; Self::do_set_keys(&who, keys)?; @@ -640,7 +651,7 @@ impl Module { // ensure keys are without duplication. ensure!( Self::key_owner(*id, key).map_or(true, |owner| &owner == who), - "registered duplicate key" + Error::::DuplicatedKey, ); if let Some(old) = old_keys.as_ref().map(|k| k.get_raw(*id)) { diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index f8cdd27059..34c42da359 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -794,8 +794,6 @@ decl_error! { AlreadyBonded, /// Controller is already paired. AlreadyPaired, - /// Should be the root origin or the `T::SlashCancelOrigin`. - BadOrigin, /// Targets cannot be empty. EmptyTargets, /// Duplicate index. @@ -1190,8 +1188,7 @@ decl_module! { fn cancel_deferred_slash(origin, era: EraIndex, slash_indices: Vec) { T::SlashCancelOrigin::try_origin(origin) .map(|_| ()) - .or_else(ensure_root) - .map_err(|_| Error::::BadOrigin)?; + .or_else(ensure_root)?; let mut slash_indices = slash_indices; slash_indices.sort_unstable(); diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index ee0e1adc5e..6e1c6fb8d8 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -166,7 +166,7 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedOperational(100_000)] fn reject_proposal(origin, #[compact] proposal_id: ProposalIndex) { - T::RejectOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; + T::RejectOrigin::ensure_origin(origin)?; let proposal = >::take(proposal_id).ok_or(Error::::InvalidProposalIndex)?; let value = proposal.bond; @@ -184,7 +184,7 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedOperational(100_000)] fn approve_proposal(origin, #[compact] proposal_id: ProposalIndex) { - T::ApproveOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; + T::ApproveOrigin::ensure_origin(origin)?; ensure!(>::exists(proposal_id), Error::::InvalidProposalIndex);