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

substrate/primitives/sr-primitives/src/generic/checked_extrinsic.rs

@@ -74,7 +74,7 @@ where
 		self,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> crate::ApplyResult {
+	) -> crate::ApplyExtrinsicResult {
 		let (maybe_who, pre) = if let Some((id, extra)) = self.signed {
 			let pre = Extra::pre_dispatch(extra, &id, &self.function, info.clone(), len)?;
 			(Some(id), pre)

substrate/primitives/sr-primitives/src/lib.rs

@@ -346,58 +346,12 @@ impl From<ed25519::Signature> for AnySignature {
 	}
 }
 
-#[derive(Eq, PartialEq, Clone, Copy, Decode, Encode, RuntimeDebug)]
-#[cfg_attr(feature = "std", derive(Serialize))]
-/// Reason why an extrinsic couldn't be applied (i.e. invalid extrinsic).
-pub enum ApplyError {
-	/// General error to do with the permissions of the sender.
-	NoPermission,
-
-	/// General error to do with the state of the system in general.
-	BadState,
-
-	/// Any error to do with the transaction validity.
-	Validity(transaction_validity::TransactionValidityError),
-}
-
-impl ApplyError {
-	/// Returns if the reason for the error was block resource exhaustion.
-	pub fn exhausted_resources(&self) -> bool {
-		match self {
-			Self::Validity(e) => e.exhausted_resources(),
-			_ => false,
-		}
-	}
-}
-
-impl From<ApplyError> for &'static str {
-	fn from(err: ApplyError) -> &'static str {
-		match err {
-			ApplyError::NoPermission => "Transaction does not have required permissions",
-			ApplyError::BadState => "System state currently prevents this transaction",
-			ApplyError::Validity(v) => v.into(),
-		}
-	}
-}
-
-impl From<transaction_validity::TransactionValidityError> for ApplyError {
-	fn from(err: transaction_validity::TransactionValidityError) -> Self {
-		ApplyError::Validity(err)
-	}
-}
-
-/// The outcome of applying a transaction.
-pub type ApplyOutcome = Result<(), DispatchError>;
-
-impl From<DispatchError> for ApplyOutcome {
+impl From<DispatchError> for DispatchOutcome {
 	fn from(err: DispatchError) -> Self {
 		Err(err)
 	}
 }
 
-/// Result from attempt to apply an extrinsic.
-pub type ApplyResult = Result<ApplyOutcome, ApplyError>;
-
 #[derive(Eq, PartialEq, Clone, Copy, Encode, Decode, RuntimeDebug)]
 #[cfg_attr(feature = "std", derive(Serialize))]
 /// Reason why a dispatch call failed
@@ -451,6 +405,37 @@ impl From<&'static str> for DispatchError {
 	}
 }
 
+/// This type specifies the outcome of dispatching a call to a module.
+///
+/// In case of failure an error specific to the module is returned.
+///
+/// Failure of the module call dispatching doesn't invalidate the extrinsic and it is still included
+/// in the block, therefore all state changes performed by the dispatched call are still persisted.
+///
+/// For example, if the dispatching of an extrinsic involves inclusion fee payment then these
+/// changes are going to be preserved even if the call dispatched failed.
+pub type DispatchOutcome = Result<(), DispatchError>;
+
+/// The result of applying of an extrinsic.
+///
+/// This type is typically used in the context of `BlockBuilder` to signal that the extrinsic
+/// in question cannot be included.
+///
+/// A block containing extrinsics that have a negative inclusion outcome is invalid. A negative
+/// result can only occur during the block production, where such extrinsics are detected and
+/// removed from the block that is being created and the transaction pool.
+///
+/// To rehash: every extrinsic in a valid block must return a positive `ApplyExtrinsicResult`.
+///
+/// Examples of reasons preventing inclusion in a block:
+/// - More block weight is required to process the extrinsic than is left in the block being built.
+///   This doesn't neccessarily mean that the extrinsic is invalid, since it can still be
+///   included in the next block if it has enough spare weight available.
+/// - The sender doesn't have enough funds to pay the transaction inclusion fee. Including such
+///   a transaction in the block doesn't make sense.
+/// - The extrinsic supplied a bad signature. This transaction won't become valid ever.
+pub type ApplyExtrinsicResult = Result<DispatchOutcome, transaction_validity::TransactionValidityError>;
+
 /// Verify a signature on an encoded value in a lazy manner. This can be
 /// an optimization if the signature scheme has an "unsigned" escape hash.
 pub fn verify_encoded_lazy<V: Verify, T: codec::Encode>(

substrate/primitives/sr-primitives/src/testing.rs

@@ -25,7 +25,7 @@ use crate::traits::{
 };
 #[allow(deprecated)]
 use crate::traits::ValidateUnsigned;
-use crate::{generic, KeyTypeId, ApplyResult};
+use crate::{generic, KeyTypeId, ApplyExtrinsicResult};
 pub use primitives::{H256, sr25519};
 use primitives::{crypto::{CryptoType, Dummy, key_types, Public}, U256};
 use crate::transaction_validity::{TransactionValidity, TransactionValidityError};
@@ -354,7 +354,7 @@ impl<Origin, Call, Extra, Info> Applyable for TestXt<Call, Extra> where
 		self,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> ApplyResult {
+	) -> ApplyExtrinsicResult {
 		let maybe_who = if let Some((who, extra)) = self.0 {
 			Extra::pre_dispatch(extra, &who, &self.1, info, len)?;
 			Some(who)

substrate/primitives/sr-primitives/src/traits.rs

@@ -754,7 +754,7 @@ pub trait SignedExtension: Codec + Debug + Sync + Send + Clone + Eq + PartialEq
 		call: &Self::Call,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> Result<Self::Pre, crate::ApplyError> {
+	) -> Result<Self::Pre, TransactionValidityError> {
 		self.validate(who, call, info.clone(), len)
 			.map(|_| Self::Pre::default())
 			.map_err(Into::into)
@@ -788,7 +788,7 @@ pub trait SignedExtension: Codec + Debug + Sync + Send + Clone + Eq + PartialEq
 		call: &Self::Call,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> Result<Self::Pre, crate::ApplyError> {
+	) -> Result<Self::Pre, TransactionValidityError> {
 		Self::validate_unsigned(call, info.clone(), len)
 			.map(|_| Self::Pre::default())
 			.map_err(Into::into)
@@ -835,7 +835,7 @@ impl<AccountId, Call, Info: Clone> SignedExtension for Tuple {
 	}
 
 	fn pre_dispatch(self, who: &Self::AccountId, call: &Self::Call, info: Self::DispatchInfo, len: usize)
-		-> Result<Self::Pre, crate::ApplyError>
+		-> Result<Self::Pre, TransactionValidityError>
 	{
 		Ok(for_tuples!( ( #( Tuple.pre_dispatch(who, call, info.clone(), len)? ),* ) ))
 	}
@@ -854,7 +854,7 @@ impl<AccountId, Call, Info: Clone> SignedExtension for Tuple {
 		call: &Self::Call,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> Result<Self::Pre, crate::ApplyError> {
+	) -> Result<Self::Pre, TransactionValidityError> {
 		Ok(for_tuples!( ( #( Tuple::pre_dispatch_unsigned(call, info.clone(), len)? ),* ) ))
 	}
 
@@ -912,7 +912,7 @@ pub trait Applyable: Sized + Send + Sync {
 		self,
 		info: Self::DispatchInfo,
 		len: usize,
-	) -> crate::ApplyResult;
+	) -> crate::ApplyExtrinsicResult;
 }
 
 /// Auxiliary wrapper that holds an api instance and binds it to the given lifetime.
@@ -992,7 +992,7 @@ pub trait ValidateUnsigned {
 	/// this function again to make sure we never include an invalid transaction.
 	///
 	/// Changes made to storage WILL be persisted if the call returns `Ok`.
-	fn pre_dispatch(call: &Self::Call) -> Result<(), crate::ApplyError> {
+	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
 		Self::validate_unsigned(call)
 			.map(|_| ())
 			.map_err(Into::into)

substrate/primitives/sr-primitives/src/transaction_validity.rs

@@ -146,18 +146,6 @@ impl From<UnknownTransaction> for TransactionValidityError {
 	}
 }
 
-impl From<InvalidTransaction> for crate::ApplyError {
-	fn from(invalid: InvalidTransaction) -> crate::ApplyError {
-		TransactionValidityError::from(invalid).into()
-	}
-}
-
-impl From<UnknownTransaction> for crate::ApplyError {
-	fn from(unknown: UnknownTransaction) -> crate::ApplyError {
-		TransactionValidityError::from(unknown).into()
-	}
-}
-
 /// Information on a transaction's validity and, if valid, on how it relates to other transactions.
 pub type TransactionValidity = Result<ValidTransaction, TransactionValidityError>;