-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134
Changes from 5 commits
7572b98
8e4a5e2
a62d4f3
b8c7ec1
14bdeb5
d8ee205
3dcbb48
56e530c
7e04dc6
0459481
c53659b
d916bc4
d034332
89cd7f4
7c40059
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -348,56 +348,41 @@ 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, | ||
|
|
||
| /// Reason why an extrinsic couldn't be included into a block. | ||
|
pepyakin marked this conversation as resolved.
Outdated
|
||
| pub enum InclusionError { | ||
|
pepyakin marked this conversation as resolved.
Outdated
|
||
| /// Any error to do with the transaction validity. | ||
| Validity(transaction_validity::TransactionValidityError), | ||
|
pepyakin marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| impl ApplyError { | ||
| impl InclusionError { | ||
| /// 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 { | ||
| impl From<InclusionError> for &'static str { | ||
| fn from(err: InclusionError) -> &'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(), | ||
| InclusionError::Validity(v) => v.into(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<transaction_validity::TransactionValidityError> for ApplyError { | ||
| impl From<transaction_validity::TransactionValidityError> for InclusionError { | ||
| fn from(err: transaction_validity::TransactionValidityError) -> Self { | ||
| ApplyError::Validity(err) | ||
| InclusionError::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 +436,27 @@ 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. | ||
|
pepyakin marked this conversation as resolved.
|
||
| pub type DispatchOutcome = Result<(), DispatchError>; | ||
|
|
||
| /// The outcome of inclusion of an extrinsic into a block. | ||
| /// | ||
| /// This type is typically used in the context of `BlockBuilder` to signal that the extrinsic | ||
| /// in question cannot be included. It is fair to say that a valid block doesn't contain any | ||
|
pepyakin marked this conversation as resolved.
Outdated
|
||
| /// extrinsic that would have had a negative inclusion outcome. On successful inclusion this type | ||
| /// supplies the result of the extrinsic dispatch. | ||
| /// | ||
| /// 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't name weight here and just say block resources. Weight is Palette specific while I suppose these error types are.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh, that's a good point. I am mistakingly assumed that we are in the pallete context. |
||
| /// 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make it very clear that a dispatch has two phases 1- pre-dispatch stuff and 2- the dispatch itself. A signature check, and any other code that we might put in A weight check, nonce and other things that happen either in signedExtensions or dispatch code itself are in the second group. I think a wrong nonce gives you So I would make this distinction very clear in the docs. (take the above with a grain of slat and double check, I didn't look super deep into the code.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on this, without any explanation, I find it confusing that signature check and weight check are enumerated next to each other, as it seems that they are natively different types of checks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I would redirect the reader from this documentation to how
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, they are different checks in practice, but we are dealing with the non-pallete context, from the block builder API perspective, from that PoV they are essentially the same, isn't it This also relates to referencing the user to the documentation of |
||
| pub type InclusionOutcome = Result<DispatchOutcome, InclusionError>; | ||
|
|
||
| /// 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>( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be renamed as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My logic was that I'd leave this as is to denote that
apply_extrinsicfailed because of the following reason which is represented byInclusionError.I thought maybe it would be better to name
apply_extrinsicasinclude_extrinsic, but that is getting out of hand...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I am not strong on it. Give me a sign (thumbs up will do) and I will make the change