Migrate membership, nicks, scored-pool and session to decl_error (#4463)

* Migrate membership, nicks, scored-pool and session to decl_error

* Fix tests

* Update frame/scored-pool/src/tests.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Remove InsufficientBalance error from scored-pool

* Replace Error::<Test, DefaultInstance> with Error::<Test, _>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

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<T: Trait<I>, I: Instance> {
+		/// Already a member.
+		AlreadyMember,
+		/// Not a member.
+		NotMember,
+	}
+}
+
 decl_module! {
 	pub struct Module<T: Trait<I>, 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 = <Members<T, I>>::get();
-			let location = members.binary_search(&who).err().ok_or("already a member")?;
+			let location = members.binary_search(&who).err().ok_or(Error::<T, I>::AlreadyMember)?;
 			members.insert(location, who.clone());
 			<Members<T, I>>::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 = <Members<T, I>>::get();
-			let location = members.binary_search(&who).ok().ok_or("not a member")?;
+			let location = members.binary_search(&who).ok().ok_or(Error::<T, I>::NotMember)?;
 			members.remove(location);
 			<Members<T, I>>::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 = <Members<T, I>>::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::<T, I>::NotMember)?;
+			let _ = members.binary_search(&add).err().ok_or(Error::<T, I>::AlreadyMember)?;
 			members[location] = add.clone();
 			members.sort();
 			<Members<T, I>>::put(&members);
@@ -176,8 +183,7 @@ decl_module! {
 		fn reset_members(origin, members: Vec<T::AccountId>) {
 			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 = <Members<T, I>>::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::<T, I>::NotMember)?;
+				let _ = members.binary_search(&new).err().ok_or(Error::<T, I>::AlreadyMember)?;
 				members[location] = new.clone();
 				members.sort();
 				<Members<T, I>>::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::<Test, _>::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::<Test, _>::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::<Test, _>::NotMember);
+			assert_noop!(Membership::swap_member(Origin::signed(3), 10, 30), Error::<Test, _>::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::<Test, _>::NotMember);
+			assert_noop!(Membership::change_key(Origin::signed(10), 20), Error::<Test, _>::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());

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<T: Trait> {
+		/// 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<T: Trait> for enum Call where origin: T::Origin {
+		type Error = Error<T>;
+
 		fn deposit_event() = default;
 
 		/// Reservation fee.
@@ -131,8 +145,8 @@ decl_module! {
 		fn set_name(origin, name: Vec<u8>) {
 			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::<T>::TooShort,);
+			ensure!(name.len() <= T::MaxLength::get(), Error::<T>::TooLong);
 
 			let deposit = if let Some((_, deposit)) = <NameOf<T>>::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 = <NameOf<T>>::take(&sender).ok_or("Not named")?.1;
+			let deposit = <NameOf<T>>::take(&sender).ok_or(Error::<T>::Unnamed)?.1;
 
 			let _ = T::Currency::unreserve(&sender, deposit.clone());
 
@@ -184,13 +198,12 @@ decl_module! {
 		fn kill_name(origin, target: <T::Lookup as StaticLookup>::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 = <NameOf<T>>::take(&target).ok_or("Not named")?.1;
+			let deposit = <NameOf<T>>::take(&target).ok_or(Error::<T>::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: <T::Lookup as StaticLookup>::Source, name: Vec<u8>) {
 			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 = <NameOf<T>>::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::<Test>::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::<Test>::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::<Test>::TooShort);
 			assert_noop!(
 				Nicks::set_name(Origin::signed(1), b"Gavin James Wood, Esquire".to_vec()),
-				"Name too long"
+				Error::<Test>::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);
 		});
 	}
 }

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<T: Trait<I>, I: Instance> {
+		/// Already a member.
+		AlreadyInPool,
+		/// Index out of bounds.
+		InvalidIndex,
+		/// Index does not match requested account.
+		WrongAccountIndex,
+	}
+}
+
 decl_module! {
 	pub struct Module<T: Trait<I>, I: Instance=DefaultInstance>
 		for enum Call
 		where origin: T::Origin
 	{
+		type Error = Error<T, I>;
+
 		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!(!<CandidateExists<T, I>>::exists(&who), "already a member");
+			ensure!(!<CandidateExists<T, I>>::exists(&who), Error::<T, I>::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<T: Trait<I>, I: Instance> Module<T, I> {
 		mut pool: PoolT<T, I>,
 		remove: T::AccountId,
 		index: u32
-	) -> Result<(), &'static str> {
+	) -> Result<(), Error<T, I>> {
 		// 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<T: Trait<I>, I: Instance> Module<T, I> {
 		pool: &PoolT<T, I>,
 		who: &T::AccountId,
 		index: u32
-	) -> Result<(), &'static str> {
-		ensure!(index < pool.len() as u32, "index out of bounds");
+	) -> Result<(), Error<T, I>> {
+		ensure!(index < pool.len() as u32, Error::<T, I>::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::<T, I>::WrongAccountIndex);
 
 		Ok(())
 	}

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<Test>;
 type System = frame_system::Module<Test>;
 type Balances = pallet_balances::Module<Test>;
 
-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::<Test, _>::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::<Test, _>::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::<Test, _>::InvalidIndex);
+		assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, oob_index, 99), Error::<Test, _>::InvalidIndex);
+		assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, oob_index), Error::<Test, _>::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::<Test, _>::WrongAccountIndex);
+		assert_noop!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, index, 99), Error::<Test, _>::WrongAccountIndex);
+		assert_noop!(ScoredPool::kick(Origin::signed(KickOrigin::get()), who, index), Error::<Test, _>::WrongAccountIndex);
 	});
 }
 

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<T: Trait> {
+		/// Invalid ownership proof.
+		InvalidProof,
+		/// No associated validator ID for account.
+		NoAssociatedValidatorId,
+		/// Registered duplicate key.
+		DuplicatedKey,
+	}
+}
+
 decl_module! {
 	pub struct Module<T: Trait> 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<T>;
+
 		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<u8>) -> 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::<T>::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::<T>::NoAssociatedValidatorId)?;
 
 			Self::do_set_keys(&who, keys)?;
 
@@ -640,7 +651,7 @@ impl<T: Trait> Module<T> {
 			// ensure keys are without duplication.
 			ensure!(
 				Self::key_owner(*id, key).map_or(true, |owner| &owner == who),
-				"registered duplicate key"
+				Error::<T>::DuplicatedKey,
 			);
 
 			if let Some(old) = old_keys.as_ref().map(|k| k.get_raw(*id)) {

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<u32>) {
 			T::SlashCancelOrigin::try_origin(origin)
 				.map(|_| ())
-				.or_else(ensure_root)
-				.map_err(|_| Error::<T>::BadOrigin)?;
+				.or_else(ensure_root)?;
 
 			let mut slash_indices = slash_indices;
 			slash_indices.sort_unstable();

substrate/frame/treasury/src/lib.rs

@@ -166,7 +166,7 @@ decl_module! {
 		/// # </weight>
 		#[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 = <Proposals<T>>::take(proposal_id).ok_or(Error::<T>::InvalidProposalIndex)?;
 
 			let value = proposal.bond;
@@ -184,7 +184,7 @@ decl_module! {
 		/// # </weight>
 		#[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!(<Proposals<T>>::exists(proposal_id), Error::<T>::InvalidProposalIndex);