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

substrate/frame/contracts/src/exec.rs

@@ -23,7 +23,7 @@ use crate::rent;
 use sp_std::prelude::*;
 use sp_runtime::traits::{Bounded, CheckedAdd, CheckedSub, Zero};
 use frame_support::{
-	storage::unhashed,
+	storage::unhashed, dispatch::DispatchError,
 	traits::{WithdrawReason, Currency, Time, Randomness},
 };
 
@@ -66,7 +66,7 @@ impl ExecReturnValue {
 /// non-existent destination contract, etc.).
 #[cfg_attr(test, derive(sp_runtime::RuntimeDebug))]
 pub struct ExecError {
-	pub reason: &'static str,
+	pub reason: DispatchError,
 	/// This is an allocated buffer that may be reused. The buffer must be cleared explicitly
 	/// before reuse.
 	pub buffer: Vec<u8>,
@@ -83,7 +83,9 @@ macro_rules! try_or_exec_error {
 	($e:expr, $buffer:expr) => {
 		match $e {
 			Ok(val) => val,
-			Err(reason) => return Err($crate::exec::ExecError { reason, buffer: $buffer }),
+			Err(reason) => return Err(
+				$crate::exec::ExecError { reason: reason.into(), buffer: $buffer }
+			),
 		}
 	}
 }
@@ -336,7 +338,7 @@ where
 	) -> ExecResult {
 		if self.depth == self.config.max_depth as usize {
 			return Err(ExecError {
-				reason: "reached maximum depth, cannot make a call",
+				reason: "reached maximum depth, cannot make a call".into(),
 				buffer: input_data,
 			});
 		}
@@ -346,7 +348,7 @@ where
 			.is_out_of_gas()
 		{
 			return Err(ExecError {
-				reason: "not enough gas to pay base call fee",
+				reason: "not enough gas to pay base call fee".into(),
 				buffer: input_data,
 			});
 		}
@@ -359,7 +361,7 @@ where
 		// Calls to dead contracts always fail.
 		if let Some(ContractInfo::Tombstone(_)) = contract_info {
 			return Err(ExecError {
-				reason: "contract has been evicted",
+				reason: "contract has been evicted".into(),
 				buffer: input_data,
 			});
 		};
@@ -404,7 +406,7 @@ where
 							.expect("a nested execution context must have a parent; qed");
 						if parent.is_live(&dest) {
 							return Err(ExecError {
-								reason: "contract cannot be destroyed during recursive execution",
+								reason: "contract cannot be destroyed during recursive execution".into(),
 								buffer: output.data,
 							});
 						}
@@ -428,7 +430,7 @@ where
 	) -> Result<(T::AccountId, ExecReturnValue), ExecError> {
 		if self.depth == self.config.max_depth as usize {
 			return Err(ExecError {
-				reason: "reached maximum depth, cannot instantiate",
+				reason: "reached maximum depth, cannot instantiate".into(),
 				buffer: input_data,
 			});
 		}
@@ -438,7 +440,7 @@ where
 			.is_out_of_gas()
 		{
 			return Err(ExecError {
-				reason: "not enough gas to pay base instantiate fee",
+				reason: "not enough gas to pay base instantiate fee".into(),
 				buffer: input_data,
 			});
 		}
@@ -488,7 +490,7 @@ where
 			// Error out if insufficient remaining balance.
 			if nested.overlay.get_balance(&dest) < nested.config.existential_deposit {
 				return Err(ExecError {
-					reason: "insufficient remaining balance",
+					reason: "insufficient remaining balance".into(),
 					buffer: output.data,
 				});
 			}
@@ -603,7 +605,7 @@ fn transfer<'a, T: Trait, V: Vm<T>, L: Loader<T>>(
 	dest: &T::AccountId,
 	value: BalanceOf<T>,
 	ctx: &mut ExecutionContext<'a, T, V, L>,
-) -> Result<(), &'static str> {
+) -> Result<(), DispatchError> {
 	use self::TransferCause::*;
 	use self::TransferFeeKind::*;
 
@@ -637,23 +639,28 @@ fn transfer<'a, T: Trait, V: Vm<T>, L: Loader<T>>(
 	};
 
 	if gas_meter.charge(ctx.config, token).is_out_of_gas() {
-		return Err("not enough gas to pay transfer fee");
+		Err("not enough gas to pay transfer fee")?
 	}
 
 	// We allow balance to go below the existential deposit here:
 	let from_balance = ctx.overlay.get_balance(transactor);
 	let new_from_balance = match from_balance.checked_sub(&value) {
 		Some(b) => b,
-		None => return Err("balance too low to send value"),
+		None => Err("balance too low to send value")?,
 	};
 	if would_create && value < ctx.config.existential_deposit {
-		return Err("value too low to create account");
+		Err("value too low to create account")?
 	}
-	T::Currency::ensure_can_withdraw(transactor, value, WithdrawReason::Transfer.into(), new_from_balance)?;
+	T::Currency::ensure_can_withdraw(
+		transactor,
+		value,
+		WithdrawReason::Transfer.into(),
+		new_from_balance,
+	)?;
 
 	let new_to_balance = match to_balance.checked_add(&value) {
 		Some(b) => b,
-		None => return Err("destination balance too high to receive value"),
+		None => Err("destination balance too high to receive value")?,
 	};
 
 	if transactor != dest {
@@ -821,6 +828,7 @@ mod tests {
 	};
 	use std::{cell::RefCell, rc::Rc, collections::HashMap, marker::PhantomData};
 	use assert_matches::assert_matches;
+	use sp_runtime::DispatchError;
 
 	const ALICE: u64 = 1;
 	const BOB: u64 = 2;
@@ -1176,7 +1184,10 @@ mod tests {
 
 			assert_matches!(
 				result,
-				Err(ExecError { reason: "balance too low to send value", buffer: _ })
+				Err(ExecError {
+					reason: DispatchError::Other("balance too low to send value"),
+					buffer: _,
+				})
 			);
 			assert_eq!(ctx.overlay.get_balance(&origin), 0);
 			assert_eq!(ctx.overlay.get_balance(&dest), 0);
@@ -1313,7 +1324,10 @@ mod tests {
 				// Verify that we've got proper error and set `reached_bottom`.
 				assert_matches!(
 					r,
-					Err(ExecError { reason: "reached maximum depth, cannot make a call", buffer: _ })
+					Err(ExecError {
+						reason: DispatchError::Other("reached maximum depth, cannot make a call"),
+						buffer: _,
+					})
 				);
 				*reached_bottom = true;
 			} else {
@@ -1583,7 +1597,7 @@ mod tests {
 
 		let mut loader = MockLoader::empty();
 		let dummy_ch = loader.insert(
-			|_| Err(ExecError { reason: "It's a trap!", buffer: Vec::new() })
+			|_| Err(ExecError { reason: "It's a trap!".into(), buffer: Vec::new() })
 		);
 		let instantiator_ch = loader.insert({
 			let dummy_ch = dummy_ch.clone();
@@ -1596,7 +1610,7 @@ mod tests {
 						ctx.gas_meter,
 						vec![]
 					),
-					Err(ExecError { reason: "It's a trap!", buffer: _ })
+					Err(ExecError { reason: DispatchError::Other("It's a trap!"), buffer: _ })
 				);
 
 				exec_success()

substrate/frame/contracts/src/gas.rs

@@ -21,6 +21,7 @@ use sp_runtime::traits::{
 };
 use frame_support::{
 	traits::{Currency, ExistenceRequirement, Imbalance, OnUnbalanced, WithdrawReason}, StorageValue,
+	dispatch::DispatchError,
 };
 
 #[cfg(test)]
@@ -201,7 +202,7 @@ impl<T: Trait> GasMeter<T> {
 pub fn buy_gas<T: Trait>(
 	transactor: &T::AccountId,
 	gas_limit: Gas,
-) -> Result<(GasMeter<T>, NegativeImbalanceOf<T>), &'static str> {
+) -> Result<(GasMeter<T>, NegativeImbalanceOf<T>), DispatchError> {
 	// Buy the specified amount of gas.
 	let gas_price = <Module<T>>::gas_price();
 	let cost = if gas_price.is_zero() {

substrate/frame/contracts/src/lib.rs

@@ -119,7 +119,7 @@ use sp_runtime::{
 	},
 	RuntimeDebug,
 };
-use frame_support::dispatch::{Result, Dispatchable};
+use frame_support::dispatch::{DispatchResult, Dispatchable};
 use frame_support::{
 	Parameter, decl_module, decl_event, decl_storage, storage::child,
 	parameter_types, IsSubType,
@@ -539,10 +539,10 @@ decl_module! {
 		/// Updates the schedule for metering contracts.
 		///
 		/// The schedule must have a greater version than the stored schedule.
-		pub fn update_schedule(origin, schedule: Schedule) -> Result {
+		pub fn update_schedule(origin, schedule: Schedule) -> DispatchResult {
 			ensure_root(origin)?;
 			if <Module<T>>::current_schedule().version >= schedule.version {
-				return Err("new schedule must have a greater version than current");
+				Err("new schedule must have a greater version than current")?
 			}
 
 			Self::deposit_event(RawEvent::ScheduleUpdated(schedule.version));
@@ -557,7 +557,7 @@ decl_module! {
 			origin,
 			#[compact] gas_limit: Gas,
 			code: Vec<u8>
-		) -> Result {
+		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
 
 			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
@@ -570,7 +570,7 @@ decl_module! {
 
 			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
 
-			result.map(|_| ())
+			result.map(|_| ()).map_err(Into::into)
 		}
 
 		/// Makes a call to an account, optionally transferring some balance.
@@ -586,13 +586,13 @@ decl_module! {
 			#[compact] value: BalanceOf<T>,
 			#[compact] gas_limit: Gas,
 			data: Vec<u8>
-		) -> Result {
+		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
 			let dest = T::Lookup::lookup(dest)?;
 
 			Self::bare_call(origin, dest, value, gas_limit, data)
 				.map(|_| ())
-				.map_err(|e| e.reason)
+				.map_err(|e| e.reason.into())
 		}
 
 		/// Instantiates a new contract from the `codehash` generated by `put_code`, optionally transferring some balance.
@@ -611,7 +611,7 @@ decl_module! {
 			#[compact] gas_limit: Gas,
 			code_hash: CodeHash<T>,
 			data: Vec<u8>
-		) -> Result {
+		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
 
 			Self::execute_wasm(origin, gas_limit, |ctx, gas_meter| {
@@ -619,7 +619,7 @@ decl_module! {
 					.map(|(_address, output)| output)
 			})
 			.map(|_| ())
-			.map_err(|e| e.reason)
+			.map_err(|e| e.reason.into())
 		}
 
 		/// Allows block producers to claim a small reward for evicting a contract. If a block producer
@@ -636,10 +636,10 @@ decl_module! {
 				(Ok(frame_system::RawOrigin::None), Some(aux_sender)) => {
 					(false, aux_sender)
 				},
-				_ => return Err(
+				_ => Err(
 					"Invalid surcharge claim: origin must be signed or \
 					inherent and auxiliary sender only provided on inherent"
-				),
+				)?,
 			};
 
 			// Add some advantage for block producers (who send unsigned extrinsics) by
@@ -783,7 +783,7 @@ impl<T: Trait> Module<T> {
 		code_hash: CodeHash<T>,
 		rent_allowance: BalanceOf<T>,
 		delta: Vec<exec::StorageKey>
-	) -> Result {
+	) -> DispatchResult {
 		let mut origin_contract = <ContractInfoOf<T>>::get(&origin)
 			.and_then(|c| c.get_alive())
 			.ok_or("Cannot restore from inexisting or tombstone contract")?;
@@ -791,7 +791,7 @@ impl<T: Trait> Module<T> {
 		let current_block = <frame_system::Module<T>>::block_number();
 
 		if origin_contract.last_write == Some(current_block) {
-			return Err("Origin TrieId written in the current block");
+			Err("Origin TrieId written in the current block")?
 		}
 
 		let dest_tombstone = <ContractInfoOf<T>>::get(&dest)
@@ -816,7 +816,7 @@ impl<T: Trait> Module<T> {
 						origin_contract.child_trie_unique_id(),
 						&blake2_256(key),
 					);
-					
+
 					(key, value)
 				})
 			})
@@ -841,7 +841,7 @@ impl<T: Trait> Module<T> {
 				);
 			}
 
-			return Err("Tombstones don't match");
+			return Err("Tombstones don't match".into());
 		}
 
 		origin_contract.storage_size -= key_values_taken.iter()

substrate/frame/contracts/src/tests.rs

@@ -117,6 +117,7 @@ impl frame_system::Trait for Test {
 	type AvailableBlockRatio = AvailableBlockRatio;
 	type MaximumBlockLength = MaximumBlockLength;
 	type Version = ();
+	type ModuleToIndex = ();
 }
 impl pallet_balances::Trait for Test {
 	type Balance = u64;

substrate/frame/contracts/src/wasm/mod.rs

@@ -161,6 +161,7 @@ mod tests {
 	use wabt;
 	use hex_literal::hex;
 	use assert_matches::assert_matches;
+	use sp_runtime::DispatchError;
 
 	#[derive(Debug, PartialEq, Eq)]
 	struct DispatchEntry(Call);
@@ -1429,7 +1430,7 @@ mod tests {
 				MockExt::default(),
 				&mut gas_meter
 			),
-			Err(ExecError { reason: "during execution", buffer: _ })
+			Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ })
 		);
 	}
 
@@ -1471,7 +1472,7 @@ mod tests {
 				MockExt::default(),
 				&mut gas_meter
 			),
-			Err(ExecError { reason: "during execution", buffer: _ })
+			Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ })
 		);
 	}
 

substrate/frame/contracts/src/wasm/runtime.rs

@@ -98,7 +98,7 @@ pub(crate) fn to_execution_result<E: Ext>(
 		// validated by the code preparation process. However, because panics are really
 		// undesirable in the runtime code, we treat this as a trap for now. Eventually, we might
 		// want to revisit this.
-		Ok(_) => Err(ExecError { reason: "return type error", buffer: runtime.scratch_buf }),
+		Ok(_) => Err(ExecError { reason: "return type error".into(), buffer: runtime.scratch_buf }),
 		// `Error::Module` is returned only if instantiation or linking failed (i.e.
 		// wasm binary tried to import a function that is not provided by the host).
 		// This shouldn't happen because validation process ought to reject such binaries.
@@ -106,10 +106,10 @@ pub(crate) fn to_execution_result<E: Ext>(
 		// Because panics are really undesirable in the runtime code, we treat this as
 		// a trap for now. Eventually, we might want to revisit this.
 		Err(sp_sandbox::Error::Module) =>
-			Err(ExecError { reason: "validation error", buffer: runtime.scratch_buf }),
+			Err(ExecError { reason: "validation error".into(), buffer: runtime.scratch_buf }),
 		// Any other kind of a trap should result in a failure.
 		Err(sp_sandbox::Error::Execution) | Err(sp_sandbox::Error::OutOfBounds) =>
-			Err(ExecError { reason: "during execution", buffer: runtime.scratch_buf }),
+			Err(ExecError { reason: "during execution".into(), buffer: runtime.scratch_buf }),
 	}
 }