Refactor module & runtime error handing

core/client/src/block_builder/block_builder.rs

@@ -104,7 +104,7 @@ where
 				ExecutionContext::BlockConstruction,
 				xt.clone()
 			)? {
-				Ok(ApplyOutcome::Success) | Ok(ApplyOutcome::Fail) => {
+				Ok(ApplyOutcome::Success) | Ok(ApplyOutcome::Fail(_)) => {
 					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::PrimitiveError;
 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 = PrimitiveError;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Self::Error> {
 		Ok(match self.signature {
 			Some(SignatureContent{signed, signature, index}) => {
 				let payload = (index, self.function);
 				let signed = context.lookup(signed)?;
 				if !crate::verify_encoded_lazy(&signature, &payload, &signed) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(PrimitiveError::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::PrimitiveError;
 use super::{CheckedExtrinsic, Era};
 
 const TRANSACTION_VERSION: u8 = 1;
@@ -75,13 +76,14 @@ 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 = PrimitiveError;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Self::Error> {
 		Ok(match self.signature {
 			Some((signed, signature, index, era)) => {
 				let current_u64 = context.current_height().saturated_into::<u64>();
@@ -96,7 +98,7 @@ where
 						signature.verify(payload, &signed)
 					}
 				}) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(PrimitiveError::BadSignature)
 				}
 				CheckedExtrinsic {
 					signed: Some((signed, (raw_payload.0).0)),
@@ -198,6 +200,7 @@ mod tests {
 	impl Lookup for TestContext {
 		type Source = u64;
 		type Target = u64;
+		type Error = &'static str;
 		fn lookup(&self, s: u64) -> Result<u64, &'static str> { Ok(s) }
 	}
 	impl CurrentHeight for TestContext {
@@ -256,7 +259,7 @@ mod tests {
 	fn badly_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, vec![0u8]), Era::immortal());
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]
@@ -284,14 +287,14 @@ mod tests {
 	fn too_late_mortal_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, vec![0u8;0], Era::mortal(32, 10), 10u64).encode()), Era::mortal(32, 10));
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]
 	fn too_early_mortal_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, vec![0u8;0], Era::mortal(32, 43), 43u64).encode()), Era::mortal(32, 43));
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]

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::PrimitiveError;
 use super::{CheckedExtrinsic, Era};
 
 const TRANSACTION_VERSION: u8 = 1;
@@ -76,13 +77,14 @@ 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 = PrimitiveError;
 
-	fn check(self, context: &Context) -> Result<Self::Checked, &'static str> {
+	fn check(self, context: &Context) -> Result<Self::Checked, Self::Error> {
 		Ok(match self.signature {
 			Some((signed, signature, index, era)) => {
 				let current_u64 = context.current_height().saturated_into::<u64>();
@@ -98,7 +100,7 @@ where
 						signature.verify(payload, &signed)
 					}
 				}) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(PrimitiveError::BadSignature)
 				}
 				CheckedExtrinsic {
 					signed: Some((signed, raw_payload.0)),
@@ -199,7 +201,8 @@ mod tests {
 	impl Lookup for TestContext {
 		type Source = u64;
 		type Target = u64;
-		fn lookup(&self, s: u64) -> Result<u64, &'static str> { Ok(s) }
+		type Error = &'static str;
+		fn lookup(&self, s: u64) -> Result<u64, Self::Error> { Ok(s) }
 	}
 	impl CurrentHeight for TestContext {
 		type BlockNumber = u64;
@@ -257,7 +260,7 @@ mod tests {
 	fn badly_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, vec![0u8]), Era::immortal());
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]
@@ -285,14 +288,14 @@ mod tests {
 	fn too_late_mortal_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, vec![0u8;0], Era::mortal(32, 10), 10u64).encode()), Era::mortal(32, 10));
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]
 	fn too_early_mortal_signed_check_should_fail() {
 		let ux = Ex::new_signed(0, vec![0u8;0], DUMMY_ACCOUNTID, TestSig(DUMMY_ACCOUNTID, (DUMMY_ACCOUNTID, vec![0u8;0], Era::mortal(32, 43), 43u64).encode()), Era::mortal(32, 43));
 		assert!(ux.is_signed().unwrap_or(false));
-		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(crate::BAD_SIGNATURE));
+		assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err(PrimitiveError::BadSignature));
 	}
 
 	#[test]

core/sr-primitives/src/lib.rs

@@ -54,18 +54,41 @@ pub use generic::{DigestItem, Digest};
 /// Re-export this since it's part of the API of this crate.
 pub use substrate_primitives::crypto::{key_types, KeyTypeId};
 
-/// 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";
+#[cfg_attr(test, derive(PartialEq, Debug))]
+/// Primitive Error type
+pub enum PrimitiveError {
+	/// Unknown error
+	/// This exists only to make implementation easier. Should be avoid as much as possible.
+	Other(&'static str),
+	/// Indicating an invalid signature in extrinsic.
+	BadSignature,
+	/// 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).
+	BlockFull,
+}
+
+// Exists for for backward compatibility purpose.
+impl Into<&'static str> for PrimitiveError {
+	fn into(self) -> &'static str {
+		match self {
+			PrimitiveError::Other(val) => val,
+			PrimitiveError::BadSignature => "bad signature in extrinsic",
+			PrimitiveError::BlockFull => "block size limit is reached",
+		}
+	}
+}
+
+impl From<&'static str> for PrimitiveError {
+	fn from(val: &'static str) -> PrimitiveError {
+		PrimitiveError::Other(val)
+	}
+}
 
 /// Justification type.
 pub type Justification = Vec<u8>;
@@ -544,21 +567,14 @@ impl From<ed25519::Signature> for AnySignature {
 	}
 }
 
-#[derive(Eq, PartialEq, Clone, Copy, Decode)]
+#[derive(Eq, PartialEq, Clone, Copy, Encode, 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,
+	Success,
 	/// 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])
-	}
+	Fail(DispatchError),
 }
 
 #[derive(Eq, PartialEq, Clone, Copy, Decode)]
@@ -578,12 +594,40 @@ pub enum ApplyError {
 	FullBlock = 255,
 }
 
-impl codec::Encode for ApplyError {
+impl Encode for ApplyError {
 	fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F) -> R {
 		f(&[*self as u8])
 	}
 }
 
+#[derive(Eq, PartialEq, Clone, Copy)]
+#[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: u8,
+	/// Module specific error value
+	pub error: u8,
+	/// Optional error message.
+	pub message: Option<&'static str>,
+}
+
+impl Encode for DispatchError {
+	fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F) -> R {
+		f(&[self.module, self.error])
+	}
+}
+
+impl Decode for DispatchError {
+	fn decode<R: codec::Input>(input: &mut R) -> Option<Self> {
+		Some(DispatchError {
+			module: input.read_byte()?,
+			error: input.read_byte()?,
+			message: None,
+		})
+	}
+}
+
 /// Result from attempt to apply an extrinsic.
 pub type ApplyResult = Result<ApplyOutcome, ApplyError>;
 
@@ -754,6 +798,7 @@ impl traits::Extrinsic for OpaqueExtrinsic {
 
 #[cfg(test)]
 mod tests {
+	use super::DispatchError;
 	use crate::codec::{Encode, Decode};
 
 	macro_rules! per_thing_upper_test {
@@ -867,4 +912,21 @@ mod tests {
 			((Into::<U256>::into(std::u128::MAX) * 999_999u32) / 1_000_000u32).as_u128()
 		);
 	}
+
+	#[test]
+	fn dispatch_error_encoding() {
+		let error = DispatchError {
+			module: 1,
+			error: 2,
+			message: Some("error message"),
+		};
+		let encoded = error.encode();
+		let decoded = DispatchError::decode(&mut &*encoded).unwrap();
+		assert_eq!(encoded, vec![1, 2]);
+		assert_eq!(decoded, DispatchError {
+			module: 1,
+			error: 2,
+			message: None,
+		});
+	}
 }

core/sr-primitives/src/testing.rs

@@ -22,6 +22,7 @@ use crate::codec::{Codec, Encode, Decode};
 use crate::traits::{self, Checkable, Applyable, BlakeTwo256, OpaqueKeys, TypedKey};
 use crate::{generic, KeyTypeId};
 use crate::weights::{Weighable, Weight};
+use crate::PrimitiveError;
 pub use substrate_primitives::H256;
 use substrate_primitives::U256;
 use substrate_primitives::ed25519::{Public as AuthorityId};
@@ -216,7 +217,8 @@ impl<Call> Debug for TestXt<Call> {
 
 impl<Call: Codec + Sync + Send, Context> Checkable<Context> for TestXt<Call> {
 	type Checked = Self;
-	fn check(self, _: &Context) -> Result<Self::Checked, &'static str> { Ok(self) }
+	type Error = PrimitiveError;
+	fn check(self, _: &Context) -> Result<Self::Checked, Self::Error> { Ok(self) }
 }
 impl<Call: Codec + Sync + Send> traits::Extrinsic for TestXt<Call> {
 	fn is_signed(&self) -> Option<bool> {