Skip to content

Commit

Permalink
Make TransactionExtension tuple of tuple transparent for implication (
Browse files Browse the repository at this point in the history
#7028)

Currently `(A, B, C)` and `((A, B), C)` change the order of implications
in the transaction extension pipeline. This order is not accessible in
the metadata, because the metadata is just a vector of transaction
extension, the nested structure is not visible.

This PR make the implementation for tuple of `TransactionExtension`
better for tuple of tuple. `(A, B, C)` and `((A, B), C)` don't change
the implication for the validation A.

This is a breaking change but only when using the trait
`TransactionExtension` the code implementing the trait is not breaking
(surprising rust behavior but fine).

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
gui1117 and bkchr authored Jan 4, 2025
1 parent 0b4f131 commit b5a5ac4
Show file tree
Hide file tree
Showing 7 changed files with 279 additions and 24 deletions.
25 changes: 25 additions & 0 deletions prdoc/pr_7028.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
title: 'Fix implication order in implementation of `TransactionExtension` for tuple'
doc:
- audience:
- Runtime Dev
- Runtime User
description: |-
Before this PR, the implications were different in the pipeline `(A, B, C)` and `((A, B), C)`.
This PR fixes this behavior and make nested tuple transparant, the implication order of tuple of
tuple is now the same as in a single tuple.

For runtime users this mean that the implication can be breaking depending on the pipeline used
in the runtime.

For runtime developers this breaks usage of `TransactionExtension::validate`.
When calling `TransactionExtension::validate` the implication must now implement `Implication`
trait, you can use `TxBaseImplication` to wrap the type and use it as the base implication.
E.g. instead of `&(extension_version, call),` you can write `&TxBaseImplication((extension_version, call))`.

crates:
- name: sp-runtime
bump: major
- name: pallet-skip-feeless-payment
bump: major
- name: frame-system
bump: major
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ mod tests {
use crate::mock::{new_test_ext, Test, CALL};
use frame_support::{assert_ok, dispatch::DispatchInfo};
use sp_runtime::{
traits::{AsTransactionAuthorizedOrigin, DispatchTransaction},
traits::{AsTransactionAuthorizedOrigin, DispatchTransaction, TxBaseImplication},
transaction_validity::{TransactionSource::External, TransactionValidityError},
};

Expand Down Expand Up @@ -118,7 +118,7 @@ mod tests {
let info = DispatchInfo::default();
let len = 0_usize;
let (_, _, origin) = CheckNonZeroSender::<Test>::new()
.validate(None.into(), CALL, &info, len, (), CALL, External)
.validate(None.into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
.unwrap();
assert!(!origin.is_transaction_authorized());
})
Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/system/src/extensions/check_nonce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ mod tests {
assert_ok, assert_storage_noop, dispatch::GetDispatchInfo, traits::OriginTrait,
};
use sp_runtime::{
traits::{AsTransactionAuthorizedOrigin, DispatchTransaction},
traits::{AsTransactionAuthorizedOrigin, DispatchTransaction, TxBaseImplication},
transaction_validity::TransactionSource::External,
};

Expand Down Expand Up @@ -335,7 +335,7 @@ mod tests {
let info = DispatchInfo::default();
let len = 0_usize;
let (_, val, origin) = CheckNonce::<Test>(1u64.into())
.validate(None.into(), CALL, &info, len, (), CALL, External)
.validate(None.into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
.unwrap();
assert!(!origin.is_transaction_authorized());
assert_ok!(CheckNonce::<Test>(1u64.into()).prepare(val, &origin, CALL, &info, len));
Expand All @@ -359,7 +359,7 @@ mod tests {
let len = 0_usize;
// run the validation step
let (_, val, origin) = CheckNonce::<Test>(1u64.into())
.validate(Some(1).into(), CALL, &info, len, (), CALL, External)
.validate(Some(1).into(), CALL, &info, len, (), &TxBaseImplication(CALL), External)
.unwrap();
// mutate `AccountData` for the caller
crate::Account::<Test>::mutate(1, |info| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ use frame_support::{
use scale_info::{StaticTypeInfo, TypeInfo};
use sp_runtime::{
traits::{
DispatchInfoOf, DispatchOriginOf, PostDispatchInfoOf, TransactionExtension, ValidateResult,
DispatchInfoOf, DispatchOriginOf, Implication, PostDispatchInfoOf, TransactionExtension,
ValidateResult,
},
transaction_validity::TransactionValidityError,
};
Expand Down Expand Up @@ -147,7 +148,7 @@ where
info: &DispatchInfoOf<T::RuntimeCall>,
len: usize,
self_implicit: S::Implicit,
inherited_implication: &impl Encode,
inherited_implication: &impl Implication,
source: TransactionSource,
) -> ValidateResult<Self::Val, T::RuntimeCall> {
if call.is_feeless(&origin) {
Expand Down
3 changes: 2 additions & 1 deletion substrate/primitives/runtime/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ use std::str::FromStr;

pub mod transaction_extension;
pub use transaction_extension::{
DispatchTransaction, TransactionExtension, TransactionExtensionMetadata, ValidateResult,
DispatchTransaction, Implication, ImplicationParts, TransactionExtension,
TransactionExtensionMetadata, TxBaseImplication, ValidateResult,
};

/// A lazy value.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ where
info,
len,
self.implicit()?,
&(extension_version, call),
&TxBaseImplication((extension_version, call)),
source,
) {
// After validation, some origin must have been authorized.
Expand Down
Loading

0 comments on commit b5a5ac4

Please sign in to comment.