Make decl_error! errors usable (#4449)

* Make `decl_error!` errors usable

This pr implements support for returning errors of different pallets in
a pallet. These errors need to be declared with `decl_error!`.

The pr changes the following:

- Each dispatchable function now returns a `DispatchResult` which is an
alias for `Result<(), DispatchError>`.
- `DispatchError` is an enum that has 4 variants:
  - `Other`: For storing string error messages
  - `CannotLookup`: Variant that is returned when something returns a
  `sp_runtime::LookupError`
  - `BadOrigin`: Variant that is returned for any kind of bad origin
  - `Module`: The error of a specific module. Contains the `index`,
  `error` and the `message`. The index is the index of the module in
  `construct_runtime!`. `error` is the index of the error in the error
  enum declared by `decl_error!`. `message` is the message to the error
  variant (this will not be encoded).
- `construct_runtime!` now creates a new struct `ModuleToIndex`. This
struct implements the trait `ModuleToIndex`.
- `frame_system::Trait` has a new associated type: `ModuleToIndex` that
expects the `ModuleToIndex` generated by `construct_runtime!`.
- All error strings returned in any module are being converted now to `DispatchError`.
- `BadOrigin` is the default error returned by any type that implements `EnsureOrigin`.

* Fix frame system benchmarks

bin/node-template/runtime/src/lib.rs

@@ -157,6 +157,10 @@ impl system::Trait for Runtime {
 	type AvailableBlockRatio = AvailableBlockRatio;
 	/// Version of the runtime.
 	type Version = Version;
+	/// Converts a module to the index of the module in `construct_runtime!`.
+	///
+	/// This type is being generated by `construct_runtime!`.
+	type ModuleToIndex = ModuleToIndex;
 }
 
 impl aura::Trait for Runtime {

bin/node-template/runtime/src/template.rs

@@ -40,7 +40,7 @@ decl_module! {
 		// Just a dummy entry point.
 		// function that can be called by the external world as an extrinsics call
 		// takes a parameter of the type `AccountId`, stores it and emits an event
-		pub fn do_something(origin, something: u32) -> dispatch::Result {
+		pub fn do_something(origin, something: u32) -> dispatch::DispatchResult {
 			// TODO: You only need this if you want to check it was signed.
 			let who = ensure_signed(origin)?;
 
@@ -106,6 +106,7 @@ mod tests {
 		type MaximumBlockLength = MaximumBlockLength;
 		type AvailableBlockRatio = AvailableBlockRatio;
 		type Version = ();
+		type ModuleToIndex = ();
 	}
 	impl Trait for Test {
 		type Event = ();

bin/node/runtime/src/lib.rs

@@ -125,6 +125,7 @@ impl frame_system::Trait for Runtime {
 	type MaximumBlockLength = MaximumBlockLength;
 	type AvailableBlockRatio = AvailableBlockRatio;
 	type Version = Version;
+	type ModuleToIndex = ();
 }
 
 impl pallet_utility::Trait for Runtime {

frame/assets/src/lib.rs

@@ -92,7 +92,7 @@
 //!
 //! decl_module! {
 //! 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
-//! 		pub fn issue_token_airdrop(origin) -> dispatch::Result {
+//! 		pub fn issue_token_airdrop(origin) -> dispatch::DispatchResult {
 //! 			let sender = ensure_signed(origin).map_err(|e| e.as_str())?;
 //!
 //! 			const ACCOUNT_ALICE: u64 = 1;
@@ -133,7 +133,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use frame_support::{Parameter, decl_module, decl_event, decl_storage, decl_error, ensure};
-use sp_runtime::traits::{Member, SimpleArithmetic, Zero, StaticLookup, ModuleDispatchError};
+use sp_runtime::traits::{Member, SimpleArithmetic, Zero, StaticLookup};
 use frame_system::{self as system, ensure_signed};
 use sp_runtime::traits::One;
 
@@ -151,14 +151,14 @@ pub trait Trait: frame_system::Trait {
 
 decl_module! {
 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
-		type Error = Error;
+		type Error = Error<T>;
 
 		fn deposit_event() = default;
 		/// Issue a new class of fungible assets. There are, and will only ever be, `total`
 		/// such assets and they'll all belong to the `origin` initially. It will have an
 		/// identifier `AssetId` instance: this will be specified in the `Issued` event.
 		fn issue(origin, #[compact] total: T::Balance) {
-			let origin = ensure_signed(origin).map_err(|e| e.as_str())?;
+			let origin = ensure_signed(origin)?;
 
 			let id = Self::next_asset_id();
 			<NextAssetId<T>>::mutate(|id| *id += One::one());
@@ -175,12 +175,12 @@ decl_module! {
 			target: <T::Lookup as StaticLookup>::Source,
 			#[compact] amount: T::Balance
 		) {
-			let origin = ensure_signed(origin).map_err(|e| e.as_str())?;
+			let origin = ensure_signed(origin)?;
 			let origin_account = (id, origin.clone());
 			let origin_balance = <Balances<T>>::get(&origin_account);
 			let target = T::Lookup::lookup(target)?;
-			ensure!(!amount.is_zero(), Error::AmountZero);
-			ensure!(origin_balance >= amount, Error::BalanceLow);
+			ensure!(!amount.is_zero(), Error::<T>::AmountZero);
+			ensure!(origin_balance >= amount, Error::<T>::BalanceLow);
 
 			Self::deposit_event(RawEvent::Transferred(id, origin, target.clone(), amount));
 			<Balances<T>>::insert(origin_account, origin_balance - amount);
@@ -189,9 +189,9 @@ decl_module! {
 
 		/// Destroy any assets of `id` owned by `origin`.
 		fn destroy(origin, #[compact] id: T::AssetId) {
-			let origin = ensure_signed(origin).map_err(|e| e.as_str())?;
+			let origin = ensure_signed(origin)?;
 			let balance = <Balances<T>>::take((id, &origin));
-			ensure!(!balance.is_zero(), Error::BalanceZero);
+			ensure!(!balance.is_zero(), Error::<T>::BalanceZero);
 
 			<TotalSupply<T>>::mutate(id, |total_supply| *total_supply -= balance);
 			Self::deposit_event(RawEvent::Destroyed(id, origin, balance));
@@ -215,7 +215,7 @@ decl_event! {
 }
 
 decl_error! {
-	pub enum Error {
+	pub enum Error for Module<T: Trait> {
 		/// Transfer amount should be non-zero
 		AmountZero,
 		/// Account balance must be greater than or equal to the transfer amount
@@ -293,6 +293,7 @@ mod tests {
 		type AvailableBlockRatio = AvailableBlockRatio;
 		type MaximumBlockLength = MaximumBlockLength;
 		type Version = ();
+		type ModuleToIndex = ();
 	}
 	impl Trait for Test {
 		type Event = ();
@@ -353,7 +354,7 @@ mod tests {
 			assert_eq!(Assets::balance(0, 2), 50);
 			assert_ok!(Assets::destroy(Origin::signed(1), 0));
 			assert_eq!(Assets::balance(0, 1), 0);
-			assert_noop!(Assets::transfer(Origin::signed(1), 0, 1, 50), Error::BalanceLow);
+			assert_noop!(Assets::transfer(Origin::signed(1), 0, 1, 50), Error::<Test>::BalanceLow);
 		});
 	}
 
@@ -362,7 +363,7 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			assert_ok!(Assets::issue(Origin::signed(1), 100));
 			assert_eq!(Assets::balance(0, 1), 100);
-			assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 0), Error::AmountZero);
+			assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 0), Error::<Test>::AmountZero);
 		});
 	}
 
@@ -371,7 +372,7 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			assert_ok!(Assets::issue(Origin::signed(1), 100));
 			assert_eq!(Assets::balance(0, 1), 100);
-			assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 101), Error::BalanceLow);
+			assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 101), Error::<Test>::BalanceLow);
 		});
 	}
 
@@ -389,7 +390,7 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			assert_ok!(Assets::issue(Origin::signed(1), 100));
 			assert_eq!(Assets::balance(0, 2), 0);
-			assert_noop!(Assets::destroy(Origin::signed(2), 0), Error::BalanceZero);
+			assert_noop!(Assets::destroy(Origin::signed(2), 0), Error::<Test>::BalanceZero);
 		});
 	}
 }

frame/aura/src/mock.rs

@@ -60,6 +60,7 @@ impl frame_system::Trait for Test {
 	type AvailableBlockRatio = AvailableBlockRatio;
 	type MaximumBlockLength = MaximumBlockLength;
 	type Version = ();
+	type ModuleToIndex = ();
 }
 
 impl pallet_timestamp::Trait for Test {

frame/authority-discovery/src/lib.rs

@@ -165,6 +165,7 @@ mod tests {
 		type AvailableBlockRatio = AvailableBlockRatio;
 		type MaximumBlockLength = MaximumBlockLength;
 		type Version = ();
+		type ModuleToIndex = ();
 	}
 
 	impl_outer_origin! {

frame/authorship/src/lib.rs

@@ -184,12 +184,12 @@ decl_module! {
 
 		/// Provide a set of uncles.
 		#[weight = SimpleDispatchInfo::FixedOperational(10_000)]
-		fn set_uncles(origin, new_uncles: Vec<T::Header>) -> dispatch::Result {
+		fn set_uncles(origin, new_uncles: Vec<T::Header>) -> dispatch::DispatchResult {
 			ensure_none(origin)?;
 			ensure!(new_uncles.len() <= MAX_UNCLES, "Too many uncles");
 
 			if <Self as Store>::DidSetUncles::get() {
-				return Err("Uncles already set in block.");
+				Err("Uncles already set in block.")?
 			}
 			<Self as Store>::DidSetUncles::put(true);
 
@@ -219,7 +219,7 @@ impl<T: Trait> Module<T> {
 		}
 	}
 
-	fn verify_and_import_uncles(new_uncles: Vec<T::Header>) -> dispatch::Result {
+	fn verify_and_import_uncles(new_uncles: Vec<T::Header>) -> dispatch::DispatchResult {
 		let now = <frame_system::Module<T>>::block_number();
 
 		let mut uncles = <Self as Store>::Uncles::get();
@@ -408,6 +408,7 @@ mod tests {
 		type AvailableBlockRatio = AvailableBlockRatio;
 		type MaximumBlockLength = MaximumBlockLength;
 		type Version = ();
+		type ModuleToIndex = ();
 	}
 
 	parameter_types! {
@@ -565,7 +566,7 @@ mod tests {
 				);
 				assert_eq!(
 					Authorship::verify_and_import_uncles(vec![uncle_a.clone(), uncle_a.clone()]),
-					Err("uncle already included"),
+					Err("uncle already included".into()),
 				);
 			}
 
@@ -580,7 +581,7 @@ mod tests {
 
 				assert_eq!(
 					Authorship::verify_and_import_uncles(vec![uncle_a.clone()]),
-					Err("uncle already included"),
+					Err("uncle already included".into()),
 				);
 			}
 
@@ -590,7 +591,7 @@ mod tests {
 
 				assert_eq!(
 					Authorship::verify_and_import_uncles(vec![uncle_clone]),
-					Err("uncle already included"),
+					Err("uncle already included".into()),
 				);
 			}
 
@@ -599,7 +600,7 @@ mod tests {
 				let unsealed = create_header(3, canon_chain.canon_hash(2), [2; 32].into());
 				assert_eq!(
 					Authorship::verify_and_import_uncles(vec![unsealed]),
-					Err("no author"),
+					Err("no author".into()),
 				);
 			}
 
@@ -614,7 +615,7 @@ mod tests {
 
 				assert_eq!(
 					Authorship::verify_and_import_uncles(vec![gen_2]),
-					Err("uncle not recent enough to be included"),
+					Err("uncle not recent enough to be included".into()),
 				);
 			}
 

frame/babe/src/mock.rs

@@ -65,6 +65,7 @@ impl frame_system::Trait for Test {
 	type MaximumBlockWeight = MaximumBlockWeight;
 	type AvailableBlockRatio = AvailableBlockRatio;
 	type MaximumBlockLength = MaximumBlockLength;
+	type ModuleToIndex = ();
 }
 
 impl_opaque_keys! {