Custom runtime module errors (#3433)

* srml-system checks * wip * more modules compiles * node-runtime checks * build.sh passes * include dispatch error in failed event * revert some unnecessary changes * refactor based on comments * more compile error fixes * avoid unnecessary into * reorder code * fixes some tests * manually implement encode & decode to avoid i8 workaround * more test fixes * more fixes * more error fixes * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * address comments * test for DispatchError encoding * tyep alias for democracy * make error printable * line width * fix balances tests * fix executive test * fix system tests * bump version * ensure consistent method signature * Apply suggestions from code review Co-Authored-By: Gavin Wood <github@gavwood.com> * changes based on review * Add issue number for TODOs * fix * line width * fix test * Update core/sr-primitives/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update core/sr-primitives/src/traits.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update srml/council/src/motions.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update srml/council/src/motions.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * update based on review * More concrete macro matching * fix test build issue * Update hex-literal dependency version. (#3141) * Update hex-literal dep version. * Update lock file. * Start to rework the new error handling * More work to get it back compiling * Start to fix after master merge * The great transaction error handling refactoring * Make `decl_error` errors convertible to `&'static str` * Make srml-executive build again * Fix `sr-primitives` tests * More fixes * Last round of fix ups * Fix build * Fix build * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Rename some stuff * Fixes after master merge * Adds `CheckBlockGasLimit` signed extension * Remove debug stuff * Fix srml-balances test * Rename `InvalidIndex` to `CannotLookup` * Remove weird generic parameters * Rename function again * Fix import * Document the signed extension * Change from `Into` to `From` * Update srml/contracts/src/lib.rs Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Fix compilation * Update srml/contracts/src/lib.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update core/sr-primitives/src/transaction_validity.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Remove unused code * Fix compilation * Some cleanups * Fix compile errors * Make `TransactionValidity` a `Result` * Apply suggestions from code review Co-Authored-By: Gavin Wood <gavin@parity.io> * Beautify the code a little bit and fix test * Make `CannotLookup` an inherent error declared by `decl_error!` * Adds some documentation * Make `ApplyOutcome` a result * Up the spec_version * Apply suggestions from code review Co-Authored-By: Gavin Wood <gavin@parity.io> Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com>
paritytech · Sep 4, 2019 · 5420de3 · 5420de3
1 parent 26a802b
commit 5420de3
Show file tree

Hide file tree

Showing 46 changed files with 1,255 additions and 626 deletions.
diff --git a/core/basic-authorship/src/basic_authorship.rs b/core/basic-authorship/src/basic_authorship.rs
@@ -30,9 +30,10 @@ use inherents::InherentData;
 use log::{error, info, debug, trace};
 use primitives::{H256, Blake2Hasher, ExecutionContext};
 use sr_primitives::{
-	traits::{Block as BlockT, Hash as HashT, Header as HeaderT, ProvideRuntimeApi, DigestFor, BlakeTwo256},
+	traits::{
+		Block as BlockT, Hash as HashT, Header as HeaderT, ProvideRuntimeApi, DigestFor, BlakeTwo256
+	},
 	generic::BlockId,
-	ApplyError,
 };
 use transaction_pool::txpool::{self, Pool as TransactionPool};
 use substrate_telemetry::{telemetry, CONSENSUS_INFO};
@@ -170,15 +171,15 @@ impl<Block, B, E, RA, A> Proposer<Block, SubstrateClient<B, E, Block, RA>, A>	wh
 				Ok(()) => {
 					debug!("[{:?}] Pushed to the block.", pending.hash);
 				}
-				Err(error::Error::ApplyExtrinsicFailed(ApplyError::FullBlock)) => {
+				Err(error::Error::ApplyExtrinsicFailed(e)) if e.exhausted_resources() => {
 					if is_first {
 						debug!("[{:?}] Invalid transaction: FullBlock on empty block", pending.hash);
 						unqueue_invalid.push(pending.hash.clone());
 					} else if skipped < MAX_SKIPPED_TRANSACTIONS {
 						skipped += 1;
 						debug!(
 							"Block seems full, but will try {} more transactions before quitting.",
-							MAX_SKIPPED_TRANSACTIONS - skipped
+							MAX_SKIPPED_TRANSACTIONS - skipped,
 						);
 					} else {
 						debug!("Block is full, proceed with proposing.");

diff --git a/core/client/src/block_builder/block_builder.rs b/core/client/src/block_builder/block_builder.rs
@@ -17,7 +17,6 @@
 use super::api::BlockBuilder as BlockBuilderApi;
 use std::vec::Vec;
 use codec::Encode;
-use sr_primitives::ApplyOutcome;
 use sr_primitives::generic::BlockId;
 use sr_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(())
 				}

diff --git a/core/rpc/api/src/author/error.rs b/core/rpc/api/src/author/error.rs
@@ -101,15 +101,15 @@ impl From<Error> for rpc::Error {
 				message: format!("Verification Error: {}", e).into(),
 				data: Some(format!("{:?}", e).into()),
 			},
-			Error::Pool(PoolError::InvalidTransaction(code)) => rpc::Error {
+			Error::Pool(PoolError::InvalidTransaction(e)) => rpc::Error {
 				code: rpc::ErrorCode::ServerError(POOL_INVALID_TX),
 				message: "Invalid Transaction".into(),
-				data: Some(code.into()),
+				data: serde_json::to_value(e).ok(),
 			},
-			Error::Pool(PoolError::UnknownTransactionValidity(code)) => rpc::Error {
+			Error::Pool(PoolError::UnknownTransaction(e)) => rpc::Error {
 				code: rpc::ErrorCode::ServerError(POOL_UNKNOWN_VALIDITY),
 				message: "Unknown Transaction Validity".into(),
-				data: Some(code.into()),
+				data: serde_json::to_value(e).ok(),
 			},
 			Error::Pool(PoolError::TemporarilyBanned) => rpc::Error {
 				code: rpc::ErrorCode::ServerError(POOL_TEMPORARILY_BANNED),
@@ -133,7 +133,7 @@ impl From<Error> for rpc::Error {
 			},
 			Error::Pool(PoolError::ImmediatelyDropped) => rpc::Error {
 				code: rpc::ErrorCode::ServerError(POOL_IMMEDIATELY_DROPPED),
-				message: "Immediately Dropped" .into(),
+				message: "Immediately Dropped".into(),
 				data: Some("The transaction couldn't enter the pool because of the limit".into()),
 			},
 			Error::UnsupportedKeyType => rpc::Error {

diff --git a/core/sr-io/src/lib.rs b/core/sr-io/src/lib.rs
@@ -55,7 +55,13 @@ pub mod offchain;
 /// Trait for things which can be printed.
 pub trait Printable {
 	/// Print the object.
-	fn print(self);
+	fn print(&self);
+}
+
+impl Printable for u8 {
+	fn print(&self) {
+		u64::from(*self).print()
+	}
 }
 
 /// Converts a public trait definition into a private trait and set of public functions

diff --git a/core/sr-io/with_std.rs b/core/sr-io/with_std.rs
@@ -479,19 +479,19 @@ pub fn with_storage<R, F: FnOnce() -> R>(
 }
 
 impl<'a> Printable for &'a [u8] {
-	fn print(self) {
-		println!("Runtime: {}", HexDisplay::from(&self));
+	fn print(&self) {
+		println!("Runtime: {}", HexDisplay::from(self));
 	}
 }
 
 impl<'a> Printable for &'a str {
-	fn print(self) {
+	fn print(&self) {
 		println!("Runtime: {}", self);
 	}
 }
 
 impl Printable for u64 {
-	fn print(self) {
+	fn print(&self) {
 		println!("Runtime: {}", self);
 	}
 }

diff --git a/core/sr-io/without_std.rs b/core/sr-io/without_std.rs
@@ -1220,23 +1220,24 @@ unsafe fn from_raw_parts(ptr: *mut u8, len: u32) -> Option<Vec<u8>> {
 impl Api for () {}
 
 impl<'a> Printable for &'a [u8] {
-	fn print(self) {
+	fn print(&self) {
 		unsafe {
 			ext_print_hex.get()(self.as_ptr(), self.len() as u32);
 		}
 	}
 }
 
 impl<'a> Printable for &'a str {
-	fn print(self) {
+	fn print(&self) {
 		unsafe {
 			ext_print_utf8.get()(self.as_ptr() as *const u8, self.len() as u32);
 		}
 	}
 }
 
 impl Printable for u64 {
-	fn print(self) {
-		unsafe { ext_print_num.get()(self); }
+	fn print(&self) {
+		unsafe { ext_print_num.get()(*self); }
 	}
 }
+
diff --git a/core/sr-primitives/src/generic/checked_extrinsic.rs b/core/sr-primitives/src/generic/checked_extrinsic.rs
@@ -17,10 +17,8 @@
 //! Generic implementation of an extrinsic that has passed the verification
 //! stage.
 
-use rstd::result::Result;
 use crate::traits::{
-	self, Member, MaybeDisplay, SignedExtension, DispatchError, Dispatchable, DispatchResult,
-	ValidateUnsigned
+	self, Member, MaybeDisplay, SignedExtension, Dispatchable, ValidateUnsigned,
 };
 use crate::weights::{GetDispatchInfo, DispatchInfo};
 use crate::transaction_validity::TransactionValidity;
@@ -55,28 +53,24 @@ where
 		self.signed.as_ref().map(|x| &x.0)
 	}
 
-	fn validate<U: ValidateUnsigned<Call=Self::Call>>(&self,
+	fn validate<U: ValidateUnsigned<Call = Self::Call>>(
+		&self,
 		info: DispatchInfo,
 		len: usize,
 	) -> TransactionValidity {
 		if let Some((ref id, ref extra)) = self.signed {
-			Extra::validate(extra, id, &self.function, info, len).into()
+			Extra::validate(extra, id, &self.function, info, len)
 		} else {
-			match Extra::validate_unsigned(&self.function, info, len) {
-				Ok(extra) => match U::validate_unsigned(&self.function) {
-					TransactionValidity::Valid(v) =>
-						TransactionValidity::Valid(v.combine_with(extra)),
-					x => x,
-				},
-				x => x.into(),
-			}
+			let valid = Extra::validate_unsigned(&self.function, info, len)?;
+			Ok(valid.combine_with(U::validate_unsigned(&self.function)?))
 		}
 	}
 
-	fn dispatch(self,
+	fn apply(
+		self,
 		info: DispatchInfo,
 		len: usize,
-	) -> Result<DispatchResult, DispatchError> {
+	) -> crate::ApplyResult {
 		let (maybe_who, pre) = if let Some((id, extra)) = self.signed {
 			let pre = Extra::pre_dispatch(extra, &id, &self.function, info, len)?;
 			(Some(id), pre)
@@ -86,7 +80,7 @@ where
 		};
 		let res = self.function.dispatch(Origin::from(maybe_who));
 		Extra::post_dispatch(pre, info, len);
-		Ok(res)
+		Ok(res.map_err(Into::into))
 	}
 }
 

diff --git a/core/sr-primitives/src/generic/unchecked_extrinsic.rs b/core/sr-primitives/src/generic/unchecked_extrinsic.rs
@@ -21,9 +21,11 @@ use std::fmt;
 
 use rstd::prelude::*;
 use runtime_io::blake2_256;
-use crate::codec::{Decode, Encode, Input, Error};
-use crate::traits::{self, Member, MaybeDisplay, SignedExtension, Checkable, Extrinsic};
-use super::CheckedExtrinsic;
+use codec::{Decode, Encode, Input, Error};
+use crate::{
+	traits::{self, Member, MaybeDisplay, SignedExtension, Checkable, Extrinsic},
+	generic::CheckedExtrinsic, transaction_validity::{TransactionValidityError, InvalidTransaction},
+};
 
 const TRANSACTION_VERSION: u8 = 3;
 
@@ -101,19 +103,19 @@ where
 	Signature: Member + traits::Verify<Signer=AccountId>,
 	Extra: SignedExtension<AccountId=AccountId>,
 	AccountId: Member + MaybeDisplay,
-	Lookup: traits::Lookup<Source=Address, Target=AccountId>
+	Lookup: traits::Lookup<Source=Address, Target=AccountId>,
 {
 	type Checked = CheckedExtrinsic<AccountId, Call, Extra>;
 
-	fn check(self, lookup: &Lookup) -> Result<Self::Checked, &'static str> {
+	fn check(self, lookup: &Lookup) -> Result<Self::Checked, TransactionValidityError> {
 		Ok(match self.signature {
 			Some((signed, signature, extra)) => {
 				let signed = lookup.lookup(signed)?;
 				let raw_payload = SignedPayload::new(self.function, extra)?;
 				if !raw_payload.using_encoded(|payload| {
 					signature.verify(payload, &signed)
 				}) {
-					return Err(crate::BAD_SIGNATURE)
+					return Err(InvalidTransaction::BadProof.into())
 				}
 
 				let (function, extra, _) = raw_payload.deconstruct();
@@ -136,9 +138,9 @@ where
 /// is going to be different than the `SignaturePayload` - so the thing the extrinsic
 /// actually contains.
 pub struct SignedPayload<Call, Extra: SignedExtension>((
-		Call,
-		Extra,
-		Extra::AdditionalSigned,
+	Call,
+	Extra,
+	Extra::AdditionalSigned,
 ));
 
 impl<Call, Extra> SignedPayload<Call, Extra> where
@@ -148,7 +150,7 @@ impl<Call, Extra> SignedPayload<Call, Extra> where
 	/// Create new `SignedPayload`.
 	///
 	/// This function may fail if `additional_signed` of `Extra` is not available.
-	pub fn new(call: Call, extra: Extra) -> Result<Self, &'static str> {
+	pub fn new(call: Call, extra: Extra) -> Result<Self, TransactionValidityError> {
 		let additional_signed = extra.additional_signed()?;
 		let raw_payload = (call, extra, additional_signed);
 		Ok(Self(raw_payload))
@@ -256,7 +258,12 @@ where
 	Extra: SignedExtension,
 {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-		write!(f, "UncheckedExtrinsic({:?}, {:?})", self.signature.as_ref().map(|x| (&x.0, &x.2)), self.function)
+		write!(
+			f,
+			"UncheckedExtrinsic({:?}, {:?})",
+			self.signature.as_ref().map(|x| (&x.0, &x.2)),
+			self.function,
+		)
 	}
 }
 
@@ -265,15 +272,10 @@ mod tests {
 	use super::*;
 	use runtime_io::blake2_256;
 	use crate::codec::{Encode, Decode};
-	use crate::traits::{SignedExtension, Lookup};
+	use crate::traits::{SignedExtension, IdentityLookup};
 	use serde::{Serialize, Deserialize};
 
-	struct TestContext;
-	impl Lookup for TestContext {
-		type Source = u64;
-		type Target = u64;
-		fn lookup(&self, s: u64) -> Result<u64, &'static str> { Ok(s) }
-	}
+	type TestContext = IdentityLookup<u64>;
 
 	#[derive(Eq, PartialEq, Clone, Debug, Serialize, Deserialize, Encode, Decode)]
 	struct TestSig(u64, Vec<u8>);
@@ -298,7 +300,7 @@ mod tests {
 		type AdditionalSigned = ();
 		type Pre = ();
 
-		fn additional_signed(&self) -> rstd::result::Result<(), &'static str> { Ok(()) }
+		fn additional_signed(&self) -> rstd::result::Result<(), TransactionValidityError> { Ok(()) }
 	}
 
 	type Ex = UncheckedExtrinsic<TestAccountId, TestCall, TestSig, TestExtra>;
@@ -340,7 +342,7 @@ mod tests {
 	fn unsigned_check_should_work() {
 		let ux = Ex::new_unsigned(vec![0u8; 0]);
 		assert!(!ux.is_signed().unwrap_or(false));
-		assert!(<Ex as Checkable<TestContext>>::check(ux, &TestContext).is_ok());
+		assert!(<Ex as Checkable<TestContext>>::check(ux, &Default::default()).is_ok());
 	}
 
 	#[test]
@@ -349,10 +351,13 @@ mod tests {
 			vec![0u8; 0],
 			TEST_ACCOUNT,
 			TestSig(TEST_ACCOUNT, vec![0u8; 0]),
-			TestExtra
+			TestExtra,
 		);
 		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, &Default::default()),
+			Err(InvalidTransaction::BadProof.into()),
+		);
 	}
 
 	#[test]
@@ -361,12 +366,12 @@ mod tests {
 			vec![0u8; 0],
 			TEST_ACCOUNT,
 			TestSig(TEST_ACCOUNT, (vec![0u8; 0], TestExtra).encode()),
-			TestExtra
+			TestExtra,
 		);
 		assert!(ux.is_signed().unwrap_or(false));
 		assert_eq!(
-			<Ex as Checkable<TestContext>>::check(ux, &TestContext),
-			Ok(CEx { signed: Some((TEST_ACCOUNT, TestExtra)), function: vec![0u8; 0] })
+			<Ex as Checkable<TestContext>>::check(ux, &Default::default()),
+			Ok(CEx { signed: Some((TEST_ACCOUNT, TestExtra)), function: vec![0u8; 0] }),
 		);
 	}