Renaming and documentation for ApplyResult, ApplyOutcome and et al (#…

…4134)

* Remove superflous errors from the system module

* Rename and document InclusionOutcome

* Rename InclusionError

* Remove unused inclusion errors.

I left the enumeration though since other elements might be used some day.

* Rename and document DispatchOutcome

* Apply suggestions from code review

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* TransactionValidityError instead of InclusionError

* Rename InclusionOutcome to ApplyExtrinsicResult

* Update docs.

* Update lib.rs

should be → is

* Bump the block builder API version.

* Fix the should_return_runtime_version test

* Clean the evidence

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

@@ -11,7 +11,7 @@ include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));
 use rstd::prelude::*;
 use primitives::OpaqueMetadata;
 use sr_primitives::{
-	ApplyResult, transaction_validity::TransactionValidity, generic, create_runtime_str,
+	ApplyExtrinsicResult, transaction_validity::TransactionValidity, generic, create_runtime_str,
 	impl_opaque_keys, MultiSignature
 };
 use sr_primitives::traits::{
@@ -304,7 +304,7 @@ impl_runtime_apis! {
 	}
 
 	impl block_builder_api::BlockBuilder<Block> for Runtime {
-		fn apply_extrinsic(extrinsic: <Block as BlockT>::Extrinsic) -> ApplyResult {
+		fn apply_extrinsic(extrinsic: <Block as BlockT>::Extrinsic) -> ApplyExtrinsicResult {
 			Executive::apply_extrinsic(extrinsic)
 		}
 

bin/node/executor/src/lib.rs

@@ -45,7 +45,7 @@ mod tests {
 		traits::{CodeExecutor, Externalities}, storage::well_known_keys,
 	};
 	use sr_primitives::{
-		Fixed64, traits::{Header as HeaderT, Hash as HashT, Convert}, ApplyResult,
+		Fixed64, traits::{Header as HeaderT, Hash as HashT, Convert}, ApplyExtrinsicResult,
 		transaction_validity::InvalidTransaction,
 	};
 	use contracts::ContractAddressFor;
@@ -173,7 +173,7 @@ mod tests {
 			true,
 			None,
 		).0.unwrap();
-		let r = ApplyResult::decode(&mut &v.as_encoded()[..]).unwrap();
+		let r = ApplyExtrinsicResult::decode(&mut &v.as_encoded()[..]).unwrap();
 		assert_eq!(r, Err(InvalidTransaction::Payment.into()));
 	}
 
@@ -209,7 +209,7 @@ mod tests {
 			true,
 			None,
 		).0.unwrap();
-		let r = ApplyResult::decode(&mut &v.as_encoded()[..]).unwrap();
+		let r = ApplyExtrinsicResult::decode(&mut &v.as_encoded()[..]).unwrap();
 		assert_eq!(r, Err(InvalidTransaction::Payment.into()));
 	}
 
@@ -854,7 +854,7 @@ mod tests {
 			false,
 			None,
 		).0.unwrap().into_encoded();
-		let r = ApplyResult::decode(&mut &r[..]).unwrap();
+		let r = ApplyExtrinsicResult::decode(&mut &r[..]).unwrap();
 		assert_eq!(r, Err(InvalidTransaction::Payment.into()));
 	}
 
@@ -887,7 +887,7 @@ mod tests {
 			false,
 			None,
 		).0.unwrap().into_encoded();
-		ApplyResult::decode(&mut &r[..])
+		ApplyExtrinsicResult::decode(&mut &r[..])
 			.unwrap()
 			.expect("Extrinsic could be applied")
 			.expect("Extrinsic did not fail");

bin/node/runtime/src/lib.rs

@@ -29,7 +29,7 @@ use support::{
 use primitives::u32_trait::{_1, _2, _3, _4};
 use node_primitives::{AccountId, AccountIndex, Balance, BlockNumber, Hash, Index, Moment, Signature};
 use sr_api::impl_runtime_apis;
-use sr_primitives::{Permill, Perbill, ApplyResult, impl_opaque_keys, generic, create_runtime_str};
+use sr_primitives::{Permill, Perbill, ApplyExtrinsicResult, impl_opaque_keys, generic, create_runtime_str};
 use sr_primitives::curve::PiecewiseLinear;
 use sr_primitives::transaction_validity::TransactionValidity;
 use sr_primitives::traits::{
@@ -584,7 +584,7 @@ impl_runtime_apis! {
 	}
 
 	impl block_builder_api::BlockBuilder<Block> for Runtime {
-		fn apply_extrinsic(extrinsic: <Block as BlockT>::Extrinsic) -> ApplyResult {
+		fn apply_extrinsic(extrinsic: <Block as BlockT>::Extrinsic) -> ApplyExtrinsicResult {
 			Executive::apply_extrinsic(extrinsic)
 		}
 

client/api/src/error.rs

@@ -18,7 +18,7 @@
 
 use std::{self, error, result};
 use state_machine;
-use sr_primitives::ApplyError;
+use sr_primitives::transaction_validity::TransactionValidityError;
 use consensus;
 use derive_more::{Display, From};
 
@@ -37,9 +37,9 @@ pub enum Error {
 	/// Unknown block.
 	#[display(fmt = "UnknownBlock: {}", _0)]
 	UnknownBlock(String),
-	/// Applying extrinsic error.
-	#[display(fmt = "Extrinsic error: {:?}", _0)]
-	ApplyExtrinsicFailed(ApplyError),
+	/// The `apply_extrinsic` is not valid due to the given `TransactionValidityError`.
+	#[display(fmt = "Extrinsic is not valid: {:?}", _0)]
+	ApplyExtrinsicFailed(TransactionValidityError),
 	/// Execution error.
 	#[display(fmt = "Execution: {}", _0)]
 	Execution(Box<dyn state_machine::Error>),
@@ -126,7 +126,12 @@ impl<'a> From<&'a str> for Error {
 
 impl From<block_builder::ApplyExtrinsicFailed> for Error {
 	fn from(err: block_builder::ApplyExtrinsicFailed) -> Self {
-		Self::ApplyExtrinsicFailed(err.0)
+		use block_builder::ApplyExtrinsicFailed;
+		match err {
+			ApplyExtrinsicFailed::Validity(tx_validity) => Self::ApplyExtrinsicFailed(tx_validity),
+			ApplyExtrinsicFailed::Msg(msg) => Self::Msg(msg),
+		}
+
 	}
 }
 

client/block-builder/src/lib.rs

@@ -32,18 +32,41 @@ use sr_primitives::{
 		Header as HeaderT, Hash, Block as BlockT, HashFor, ProvideRuntimeApi, ApiRef, DigestFor,
 		NumberFor, One,
 	},
+	transaction_validity::TransactionValidityError,
 };
 
 use primitives::ExecutionContext;
-
 use state_machine::StorageProof;
-
 use sr_api::{Core, ApiExt, ApiErrorFor};
 
+#[allow(deprecated)]
+use runtime_api::compatability_v3;
+
 pub use runtime_api::BlockBuilder as BlockBuilderApi;
 
 /// Error when the runtime failed to apply an extrinsic.
-pub struct ApplyExtrinsicFailed(pub sr_primitives::ApplyError);
+pub enum ApplyExtrinsicFailed {
+	/// The transaction cannot be included into the current block.
+	///
+	/// This doesn't necessary mean that the transaction itself is invalid, but it might be just
+	/// unappliable onto the current block.
+	Validity(TransactionValidityError),
+	/// This is used for miscelanious errors that can be represented by string and not handleable.
+	///
+	/// This will become obsolete with complete migration to v4 APIs.
+	Msg(String),
+}
+
+#[allow(deprecated)]
+impl From<compatability_v3::ApplyError> for ApplyExtrinsicFailed {
+	fn from(e: compatability_v3::ApplyError) -> Self {
+		use self::compatability_v3::ApplyError::*;
+		match e {
+			Validity(tx_validity) => Self::Validity(tx_validity),
+			e => Self::Msg(format!("Apply extrinsic failed: {:?}", e)),
+		}
+	}
+}
 
 /// Utility for building new (valid) blocks from a stream of extrinsics.
 pub struct BlockBuilder<'a, Block: BlockT, A: ProvideRuntimeApi> {
@@ -107,21 +130,43 @@ where
 		let block_id = &self.block_id;
 		let extrinsics = &mut self.extrinsics;
 
-		self.api.map_api_result(|api| {
-			match api.apply_extrinsic_with_context(
+		if self
+			.api
+			.has_api_with::<dyn BlockBuilderApi<Block, Error = ApiErrorFor<A, Block>>, _>(
 				block_id,
-				ExecutionContext::BlockConstruction,
-				xt.clone()
-			)? {
-				Ok(_) => {
-					extrinsics.push(xt);
-					Ok(())
+				|version| version < 4,
+			)?
+		{
+			// Run compatibility fallback for v3.
+			self.api.map_api_result(|api| {
+				#[allow(deprecated)]
+				match api.apply_extrinsic_before_version_4_with_context(
+					block_id,
+					ExecutionContext::BlockConstruction,
+					xt.clone(),
+				)? {
+					Ok(_) => {
+						extrinsics.push(xt);
+						Ok(())
+					}
+					Err(e) => Err(ApplyExtrinsicFailed::from(e))?,
 				}
-				Err(e) => {
-					Err(ApplyExtrinsicFailed(e))?
+			})
+		} else {
+			self.api.map_api_result(|api| {
+				match api.apply_extrinsic_with_context(
+					block_id,
+					ExecutionContext::BlockConstruction,
+					xt.clone(),
+				)? {
+					Ok(_) => {
+						extrinsics.push(xt);
+						Ok(())
+					}
+					Err(tx_validity) => Err(ApplyExtrinsicFailed::Validity(tx_validity))?,
 				}
-			}
-		})
+			})
+		}
 	}
 
 	/// Consume the builder to return a valid `Block` containing all pushed extrinsics.

client/rpc/src/state/tests.rs

@@ -280,7 +280,7 @@ fn should_return_runtime_version() {
 
 	let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
 		\"specVersion\":1,\"implVersion\":1,\"apis\":[[\"0xdf6acb689907609b\",2],\
-		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",1],[\"0x40fe3ad401f8959a\",3],\
+		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",1],[\"0x40fe3ad401f8959a\",4],\
 		[\"0xc6e9a76309f39b09\",1],[\"0xdd718d5cc53262d4\",1],[\"0xcbca25e39f142387\",1],\
 		[\"0xf78b278be53f454c\",1],[\"0xab3c0572291feb8b\",1],[\"0xbc9d89904f5b923f\",1]]}";
 

palette/executive/src/lib.rs

@@ -79,7 +79,7 @@
 use rstd::{prelude::*, marker::PhantomData};
 use support::weights::{GetDispatchInfo, WeighBlock, DispatchInfo};
 use sr_primitives::{
-	generic::Digest, ApplyResult,
+	generic::Digest, ApplyExtrinsicResult,
 	traits::{
 		self, Header, Zero, One, Checkable, Applyable, CheckEqual, OnFinalize, OnInitialize,
 		NumberFor, Block as BlockT, OffchainWorker, Dispatchable,
@@ -229,9 +229,10 @@ where
 	}
 
 	/// Apply extrinsic outside of the block execution function.
+	///
 	/// This doesn't attempt to validate anything regarding the block, but it builds a list of uxt
 	/// hashes.
-	pub fn apply_extrinsic(uxt: Block::Extrinsic) -> ApplyResult {
+	pub fn apply_extrinsic(uxt: Block::Extrinsic) -> ApplyExtrinsicResult {
 		let encoded = uxt.encode();
 		let encoded_len = encoded.len();
 		Self::apply_extrinsic_with_len(uxt, encoded_len, Some(encoded))
@@ -252,7 +253,7 @@ where
 		uxt: Block::Extrinsic,
 		encoded_len: usize,
 		to_note: Option<Vec<u8>>,
-	) -> ApplyResult {
+	) -> ApplyExtrinsicResult {
 		// Verify that the signature is good.
 		let xt = uxt.check(&Default::default())?;
 
@@ -322,7 +323,7 @@ mod tests {
 	use sr_primitives::{
 		generic::Era, Perbill, DispatchError, testing::{Digest, Header, Block},
 		traits::{Bounded, Header as HeaderT, BlakeTwo256, IdentityLookup, ConvertInto},
-		transaction_validity::{InvalidTransaction, UnknownTransaction}, ApplyError,
+		transaction_validity::{InvalidTransaction, UnknownTransaction, TransactionValidityError},
 	};
 	use support::{
 		impl_outer_event, impl_outer_origin, parameter_types, impl_outer_dispatch,
@@ -448,7 +449,7 @@ mod tests {
 	impl ValidateUnsigned for Runtime {
 		type Call = Call;
 
-		fn pre_dispatch(_call: &Self::Call) -> Result<(), ApplyError> {
+		fn pre_dispatch(_call: &Self::Call) -> Result<(), TransactionValidityError> {
 			Ok(())
 		}
 
@@ -701,7 +702,7 @@ mod tests {
 				} else {
 					assert_eq!(
 						Executive::apply_extrinsic(xt),
-						Err(ApplyError::Validity(InvalidTransaction::Payment.into())),
+						Err(InvalidTransaction::Payment.into()),
 					);
 					assert_eq!(<balances::Module<Runtime>>::total_balance(&1), 111);
 				}

palette/support/src/unsigned.rs

@@ -18,9 +18,9 @@
 #[allow(deprecated)]
 pub use crate::sr_primitives::traits::ValidateUnsigned;
 #[doc(hidden)]
-pub use crate::sr_primitives::transaction_validity::{TransactionValidity, UnknownTransaction};
-#[doc(hidden)]
-pub use crate::sr_primitives::ApplyError;
+pub use crate::sr_primitives::transaction_validity::{
+	TransactionValidity, UnknownTransaction, TransactionValidityError,
+};
 
 
 /// Implement `ValidateUnsigned` for `Runtime`.
@@ -70,7 +70,7 @@ macro_rules! impl_outer_validate_unsigned {
 		impl $crate::unsigned::ValidateUnsigned for $runtime {
 			type Call = Call;
 
-			fn pre_dispatch(call: &Self::Call) -> Result<(), $crate::unsigned::ApplyError> {
+			fn pre_dispatch(call: &Self::Call) -> Result<(), $crate::unsigned::TransactionValidityError> {
 				#[allow(unreachable_patterns)]
 				match call {
 					$( Call::$module(inner_call) => $module::pre_dispatch(inner_call), )*

palette/system/src/lib.rs

@@ -98,7 +98,7 @@ use rstd::fmt::Debug;
 use sr_version::RuntimeVersion;
 use sr_primitives::{
 	RuntimeDebug,
-	generic::{self, Era}, Perbill, ApplyError, ApplyOutcome, DispatchError,
+	generic::{self, Era}, Perbill, DispatchOutcome, DispatchError,
 	transaction_validity::{
 		ValidTransaction, TransactionPriority, TransactionLongevity, TransactionValidityError,
 		InvalidTransaction, TransactionValidity,
@@ -320,8 +320,6 @@ decl_event!(
 decl_error! {
 	/// Error for the System module
 	pub enum Error {
-		BadSignature,
-		BlockFull,
 		RequireSignedOrigin,
 		RequireRootOrigin,
 		RequireNoOrigin,
@@ -754,7 +752,7 @@ impl<T: Trait> Module<T> {
 	}
 
 	/// To be called immediately after an extrinsic has been applied.
-	pub fn note_applied_extrinsic(r: &ApplyOutcome, _encoded_len: u32, info: DispatchInfo) {
+	pub fn note_applied_extrinsic(r: &DispatchOutcome, _encoded_len: u32, info: DispatchInfo) {
 		Self::deposit_event(
 			match r {
 				Ok(()) => Event::ExtrinsicSuccess(info),
@@ -865,7 +863,7 @@ impl<T: Trait + Send + Sync> SignedExtension for CheckWeight<T> {
 		_call: &Self::Call,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> Result<(), ApplyError> {
+	) -> Result<(), TransactionValidityError> {
 		let next_len = Self::check_block_length(info, len)?;
 		AllExtrinsicsLen::put(next_len);
 		let next_weight = Self::check_weight(info)?;
@@ -945,7 +943,7 @@ impl<T: Trait> SignedExtension for CheckNonce<T> {
 		_call: &Self::Call,
 		_info: Self::DispatchInfo,
 		_len: usize,
-	) -> Result<(), ApplyError> {
+	) -> Result<(), TransactionValidityError> {
 		let expected = <AccountNonce<T>>::get(who);
 		if self.0 != expected {
 			return Err(

primitives/block-builder/runtime-api/Cargo.toml

@@ -8,12 +8,14 @@ edition = "2018"
 sr-primitives = { path = "../../sr-primitives", default-features = false }
 sr-api = { path = "../../sr-api", default-features = false }
 rstd = { package = "sr-std", path = "../../sr-std", default-features = false }
+codec = { package = "parity-scale-codec", version = "1.0.6", default-features = false }
 inherents = { package = "substrate-inherents", path = "../../inherents", default-features = false }
 
 [features]
 default = [ "std" ]
 std = [
 	"sr-primitives/std",
+	"codec/std",
 	"inherents/std",
 	"sr-api/std",
 	"rstd/std",