Refactor module & runtime error handing

core/client/src/block_builder/block_builder.rs

@@ -17,7 +17,6 @@
 use super::api::BlockBuilder as BlockBuilderApi;
 use std::vec::Vec;
 use parity_codec::Encode;
-use runtime_primitives::ApplyOutcome;
 use runtime_primitives::generic::BlockId;
 use runtime_primitives::traits::{
 	Header as HeaderT, Hash, Block as BlockT, One, HashFor, ProvideRuntimeApi, ApiRef, DigestFor,
@@ -104,7 +103,7 @@ where
 				ExecutionContext::BlockConstruction,
 				xt.clone()
 			)? {
-				Ok(ApplyOutcome::Success) | Ok(ApplyOutcome::Fail) => {
+				Ok(_) => {
 					extrinsics.push(xt);
 					Ok(())
 				}

core/sr-primitives/src/generic/unchecked_extrinsic.rs

@@ -22,6 +22,7 @@ use std::fmt;
 use rstd::prelude::*;
 use crate::codec::{Decode, Encode, Codec, Input, HasCompact};
 use crate::traits::{self, Member, SimpleArithmetic, MaybeDisplay, Lookup, Extrinsic};
+use crate::Error;
 use super::CheckedExtrinsic;
 
 #[derive(PartialEq, Eq, Clone, Encode, Decode)]
@@ -83,17 +84,18 @@ where
 	Call: Encode + Member,
 	Signature: Member + traits::Verify<Signer=AccountId> + Codec,
 	AccountId: Member + MaybeDisplay,
-	Context: Lookup<Source=Address, Target=AccountId>,
+	Context: Lookup<Source=Address, Target=AccountId, Error=&'static str>
 {
 	type Checked = CheckedExtrinsic<AccountId, Index, Call>;
+	type Error = Error;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Error> {
 		Ok(match self.signature {
 			Some(SignatureContent{signed, signature, index}) => {
 				let payload = (index, self.function);
-				let signed = context.lookup(signed)?;
+				let signed = context.lookup(signed).map_err(Into::into)?;
 				if !crate::verify_encoded_lazy(&signature, &payload, &signed) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(Error::BadSignature)
 				}
 				CheckedExtrinsic {
 					signed: Some((signed, payload.0)),

core/sr-primitives/src/generic/unchecked_mortal_compact_extrinsic.rs

@@ -24,6 +24,7 @@ use runtime_io::blake2_256;
 use crate::codec::{Decode, Encode, Input, Compact};
 use crate::traits::{self, Member, SimpleArithmetic, MaybeDisplay, CurrentHeight, BlockNumberToHash,
 	Lookup, Checkable, Extrinsic, SaturatedConversion};
+use crate::Error;
 use super::{CheckedExtrinsic, Era};
 
 const TRANSACTION_VERSION: u8 = 1;
@@ -75,19 +76,20 @@ where
 	AccountId: Member + MaybeDisplay,
 	BlockNumber: SimpleArithmetic,
 	Hash: Encode,
-	Context: Lookup<Source=Address, Target=AccountId>
+	Context: Lookup<Source=Address, Target=AccountId, Error=&'static str>
 		+ CurrentHeight<BlockNumber=BlockNumber>
 		+ BlockNumberToHash<BlockNumber=BlockNumber, Hash=Hash>,
 {
 	type Checked = CheckedExtrinsic<AccountId, Index, Call>;
+	type Error = Error;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Error> {
 		Ok(match self.signature {
 			Some((signed, signature, index, era)) => {
 				let current_u64 = context.current_height().saturated_into::<u64>();
 				let h = context.block_number_to_hash(era.birth(current_u64).saturated_into())
-					.ok_or("transaction birth block ancient")?;
-				let signed = context.lookup(signed)?;
+					.ok_or(Error::Unknown("transaction birth block ancient"))?;
+				let signed = context.lookup(signed).map_err(Into::into)?;
 				let raw_payload = (index, self.function, era, h);
 				if !raw_payload.using_encoded(|payload| {
 					if payload.len() > 256 {
@@ -96,7 +98,7 @@ where
 						signature.verify(payload, &signed)
 					}
 				}) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(Error::BadSignature)
 				}
 				CheckedExtrinsic {
 					signed: Some((signed, (raw_payload.0).0)),

core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs

@@ -26,6 +26,7 @@ use crate::traits::{
 	self, Member, SimpleArithmetic, MaybeDisplay, CurrentHeight, BlockNumberToHash,
 	Lookup, Checkable, Extrinsic, SaturatedConversion
 };
+use crate::Error;
 use super::{CheckedExtrinsic, Era};
 
 const TRANSACTION_VERSION: u8 = 1;
@@ -76,19 +77,20 @@ where
 	AccountId: Member + MaybeDisplay,
 	BlockNumber: SimpleArithmetic,
 	Hash: Encode,
-	Context: Lookup<Source=Address, Target=AccountId>
+	Context: Lookup<Source=Address, Target=AccountId, Error=&'static str>
 		+ CurrentHeight<BlockNumber=BlockNumber>
 		+ BlockNumberToHash<BlockNumber=BlockNumber, Hash=Hash>,
 {
 	type Checked = CheckedExtrinsic<AccountId, Index, Call>;
+	type Error = Error;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Error> {
 		Ok(match self.signature {
 			Some((signed, signature, index, era)) => {
 				let current_u64 = context.current_height().saturated_into::<u64>();
 				let h = context.block_number_to_hash(era.birth(current_u64).saturated_into())
-					.ok_or("transaction birth block ancient")?;
-				let signed = context.lookup(signed)?;
+					.ok_or(Error::Unknown("transaction birth block ancient"))?;
+				let signed = context.lookup(signed).map_err(Into::into)?;
 				let raw_payload = (index, self.function, era, h);
 
 				if !raw_payload.using_encoded(|payload| {
@@ -98,7 +100,7 @@ where
 						signature.verify(payload, &signed)
 					}
 				}) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(Error::BadSignature)
 				}
 				CheckedExtrinsic {
 					signed: Some((signed, raw_payload.0)),

core/sr-primitives/src/lib.rs

@@ -48,18 +48,40 @@ pub mod transaction_validity;
 /// Re-export these since they're only "kind of" generic.
 pub use generic::{DigestItem, Digest};
 
-/// A message indicating an invalid signature in extrinsic.
-pub const BAD_SIGNATURE: &str = "bad signature in extrinsic";
-
-/// Full block error message.
-///
-/// This allows modules to indicate that given transaction is potentially valid
-/// in the future, but can't be executed in the current state.
-/// Note this error should be returned early in the execution to prevent DoS,
-/// cause the fees are not being paid if this error is returned.
-///
-/// Example: block gas limit is reached (the transaction can be retried in the next block though).
-pub const BLOCK_FULL: &str = "block size limit is reached";
+/// Error type
+pub enum Error {
+	/// Unknown error
+	/// This exists only to make implementation easier. Should be avoid as much as possible.
+	Unknown(&'static str),
+	/// Indicating an invalid signature in extrinsic.
+	BadSignature,
+	/// Full block error.
+	///
+	/// This allows modules to indicate that given transaction is potentially valid
+	/// in the future, but can't be executed in the current state.
+	/// Note this error should be returned early in the execution to prevent DoS,
+	/// cause the fees are not being paid if this error is returned.
+	///
+	/// Example: block gas limit is reached (the transaction can be retried in the next block though).
+	BlockFull
+}
+
+// Exists for for backward compatibility purpose.
+impl From<Error> for &str {
+	fn from(err: Error) -> &'static str {
+		match err {
+			Error::Unknown(val) => val,
+			Error::BadSignature => "bad signature in extrinsic",
+			Error::BlockFull => "block size limit is reached",
+		}
+	}
+}
+
+impl Into<Error> for &'static str {
+	fn into(self) -> Error {
+		Error::Unknown(self)
+	}
+}
 
 /// Justification type.
 pub type Justification = Vec<u8>;
@@ -486,23 +508,6 @@ impl From<ed25519::Signature> for AnySignature {
 	}
 }
 
-#[derive(Eq, PartialEq, Clone, Copy, Decode)]
-#[cfg_attr(feature = "std", derive(Debug, Serialize))]
-#[repr(u8)]
-/// Outcome of a valid extrinsic application. Capable of being sliced.
-pub enum ApplyOutcome {
-	/// Successful application (extrinsic reported no issue).
-	Success = 0,
-	/// Failed application (extrinsic was probably a no-op other than fees).
-	Fail = 1,
-}
-
-impl codec::Encode for ApplyOutcome {
-	fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F) -> R {
-		f(&[*self as u8])
-	}
-}
-
 #[derive(Eq, PartialEq, Clone, Copy, Decode)]
 #[cfg_attr(feature = "std", derive(Debug, Serialize))]
 #[repr(u8)]
@@ -526,6 +531,29 @@ impl codec::Encode for ApplyError {
 	}
 }
 
+#[derive(Eq, PartialEq, Clone, Copy, Encode, Decode)]
+#[cfg_attr(feature = "std", derive(Debug, Serialize))]
+/// Reason why a dispatch call failed
+pub struct DispatchError {
+	/// Module index, matching the metadata module index
+	pub module: i8, // use i8 instead of u8 because u8 is not supported by parity-codec
+	/// Module specific error value
+	pub error: i8,
+	/// Optional error message
+	#[codec(skip)]
+	pub message: Option<&'static str>,
+}
+
+#[derive(Eq, PartialEq, Clone, Copy, Encode, Decode)]
+ #[cfg_attr(feature = "std", derive(Debug, Serialize))]
+ /// Outcome of a valid extrinsic application. Capable of being sliced.
+ pub enum ApplyOutcome {
+ 	/// Successful application (extrinsic reported no issue).
+ 	Success,
+ 	/// Failed application (extrinsic was probably a no-op other than fees).
+ 	Fail(DispatchError),
+ }
+
 /// Result from attempt to apply an extrinsic.
 pub type ApplyResult = Result<ApplyOutcome, ApplyError>;
 

core/sr-primitives/src/testing.rs

@@ -200,6 +200,7 @@ impl<Call> Debug for TestXt<Call> {
 
 impl<Call: Codec + Sync + Send, Context> Checkable<Context> for TestXt<Call> {
 	type Checked = Self;
+	type Error = &'static str;
 	fn check(self, _: &Context) -> Result<Self::Checked, &'static str> { Ok(self) }
 }
 impl<Call: Codec + Sync + Send> traits::Extrinsic for TestXt<Call> {

core/sr-primitives/src/traits.rs

@@ -69,26 +69,44 @@ impl Verify for substrate_primitives::sr25519::Signature {
 	}
 }
 
+/// EnsureOrigin Error type
+pub trait EnsureOriginError {
+	/// Indicates invalid origin
+	fn invalid_origin() -> Self;
+}
+
 /// Some sort of check on the origin is performed by this object.
 pub trait EnsureOrigin<OuterOrigin> {
 	/// A return type.
 	type Success;
+	/// Error type
+	type Error: EnsureOriginError;
 	/// Perform the origin check.
-	fn ensure_origin(o: OuterOrigin) -> result::Result<Self::Success, &'static str> {
-		Self::try_origin(o).map_err(|_| "Invalid origin")
+	fn ensure_origin(o: OuterOrigin) -> result::Result<Self::Success, Self::Error> {
+		Self::try_origin(o).map_err(|_| Self::Error::invalid_origin())
 	}
 	/// Perform the origin check.
 	fn try_origin(o: OuterOrigin) -> result::Result<Self::Success, OuterOrigin>;
 }
 
+impl EnsureOriginError for () {
+	fn invalid_origin() -> () { }
+}
+
+impl EnsureOriginError for &'static str {
+	fn invalid_origin() -> &'static str { "Invalid origin" }
+}
+
 /// Means of changing one type into another in a manner dependent on the source type.
 pub trait Lookup {
 	/// Type to lookup from.
 	type Source;
 	/// Type to lookup into.
 	type Target;
+	/// Error type
+	type Error;
 	/// Attempt a lookup.
-	fn lookup(&self, s: Self::Source) -> result::Result<Self::Target, &'static str>;
+	fn lookup(&self, s: Self::Source) -> result::Result<Self::Target, Self::Error>;
 }
 
 /// Means of changing one type into another in a manner dependent on the source type.
@@ -99,8 +117,10 @@ pub trait StaticLookup {
 	type Source: Codec + Clone + PartialEq + MaybeDebug;
 	/// Type to lookup into.
 	type Target;
+	/// Error type.
+	type Error: Into<&'static str>; // Into<&'static str> for backward compatibility purpose.
 	/// Attempt a lookup.
-	fn lookup(s: Self::Source) -> result::Result<Self::Target, &'static str>;
+	fn lookup(s: Self::Source) -> result::Result<Self::Target, Self::Error>;
 	/// Convert from Target back to Source.
 	fn unlookup(t: Self::Target) -> Self::Source;
 }
@@ -111,13 +131,15 @@ pub struct IdentityLookup<T>(PhantomData<T>);
 impl<T: Codec + Clone + PartialEq + MaybeDebug> StaticLookup for IdentityLookup<T> {
 	type Source = T;
 	type Target = T;
-	fn lookup(x: T) -> result::Result<T, &'static str> { Ok(x) }
+	type Error = &'static str;
+	fn lookup(x: T) -> result::Result<T, Self::Error> { Ok(x) }
 	fn unlookup(x: T) -> T { x }
 }
 impl<T> Lookup for IdentityLookup<T> {
 	type Source = T;
 	type Target = T;
-	fn lookup(&self, x: T) -> result::Result<T, &'static str> { Ok(x) }
+	type Error = &'static str;
+	fn lookup(&self, x: T) -> result::Result<T, Self::Error> { Ok(x) }
 }
 
 /// Get the "current" block number.
@@ -701,9 +723,11 @@ pub type DigestItemFor<B> = DigestItem<<<B as Block>::Header as Header>::Hash>;
 pub trait Checkable<Context>: Sized {
 	/// Returned if `check` succeeds.
 	type Checked;
+	/// Indicates why `check` failed
+	type Error;
 
 	/// Check self, given an instance of Context.
-	fn check(self, c: &Context) -> Result<Self::Checked, &'static str>;
+	fn check(self, c: &Context) -> Result<Self::Checked, Self::Error>;
 }
 
 /// A "checkable" piece of information, used by the standard Substrate Executive in order to
@@ -713,15 +737,19 @@ pub trait Checkable<Context>: Sized {
 pub trait BlindCheckable: Sized {
 	/// Returned if `check` succeeds.
 	type Checked;
+	/// Returned if `check` failed.
+	type Error;
 
 	/// Check self.
-	fn check(self) -> Result<Self::Checked, &'static str>;
+	fn check(self) -> Result<Self::Checked, Self::Error>;
 }
 
 // Every `BlindCheckable` is also a `StaticCheckable` for arbitrary `Context`.
 impl<T: BlindCheckable, Context> Checkable<Context> for T {
 	type Checked = <Self as BlindCheckable>::Checked;
-	fn check(self, _c: &Context) -> Result<Self::Checked, &'static str> {
+	type Error = <Self as BlindCheckable>::Error;
+
+	fn check(self, _c: &Context) -> Result<Self::Checked, Self::Error> {
 		BlindCheckable::check(self)
 	}
 }

core/test-runtime/src/lib.rs

@@ -37,6 +37,7 @@ use runtime_primitives::{
 	ApplyResult,
 	create_runtime_str,
 	transaction_validity::TransactionValidity,
+	Error,
 	traits::{
 		BlindCheckable, BlakeTwo256, Block as BlockT, Extrinsic as ExtrinsicT,
 		GetNodeBlockType, GetRuntimeBlockType, Verify
@@ -117,18 +118,19 @@ impl serde::Serialize for Extrinsic {
 
 impl BlindCheckable for Extrinsic {
 	type Checked = Self;
+	type Error = Error;
 
-	fn check(self) -> Result<Self, &'static str> {
+	fn check(self) -> Result<Self, Error> {
 		match self {
 			Extrinsic::AuthoritiesChange(new_auth) => Ok(Extrinsic::AuthoritiesChange(new_auth)),
 			Extrinsic::Transfer(transfer, signature) => {
 				if runtime_primitives::verify_encoded_lazy(&signature, &transfer, &transfer.from) {
 					Ok(Extrinsic::Transfer(transfer, signature))
 				} else {
-					Err(runtime_primitives::BAD_SIGNATURE)
+					Err(Error::BadSignature)
 				}
 			},
-			Extrinsic::IncludeData(_) => Err(runtime_primitives::BAD_SIGNATURE),
+			Extrinsic::IncludeData(_) => Err(Error::BadSignature),
 		}
 	}
 }

node-template/runtime/src/lib.rs

@@ -126,6 +126,8 @@ impl system::Trait for Runtime {
 	type Event = Event;
 	/// The ubiquitous origin type.
 	type Origin = Origin;
+	/// The ubiquitous error type.
+	type Error = Error;
 }
 
 impl aura::Trait for Runtime {