Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable2412] Backport #7028 #7067

Merged
merged 3 commits into from
Jan 14, 2025
Merged

[stable2412] Backport #7028 #7067

merged 3 commits into from
Jan 14, 2025

Conversation

paritytech-cmd-bot-polkadot-sdk[bot]
Copy link

Backport #7028 into stable2412 from gui1117.

See the documentation on how to use this bot.

@gui1117 @github-actions
Make TransactionExtension tuple of tuple transparent for implication (
c78a14d 
#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]>
(cherry picked from commit b5a5ac4)
@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot requested a review from a team as a code owner January 7, 2025 03:41
@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Jan 7, 2025
Merged
@github-actions github-actions bot added the A3-backport Pull request is already reviewed well in another branch. label Jan 7, 2025
gui1117
gui1117 reviewed Jan 7, 2025
View reviewed changes
prdoc/pr_7028.prdoc Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 7, 2025 03:45
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

This pull request is amending an existing release. Please proceed with extreme caution,
as to not impact downstream teams that rely on the stability of it. Some things to consider:

  • Backports are only for 'patch' or 'minor' changes. No 'major' or other breaking change.
  • Should be a legit fix for some bug, not adding tons of new features.
  • Must either be already audited or not need an audit.
Emergency Bypass

If you really need to bypass this check: add validate: false to each crate
in the Prdoc where a breaking change is introduced. This will release a new major
version of that crate and all its reverse dependencies and basically break the release.

@gui1117
Copy link
Contributor

gui1117 commented Jan 7, 2025 •
edited
Loading

This backport is controversial, it is a breaking change both in rust code and in the runtime behavior in general.

  • about rust code the breaking change is minor and there is a direct path for upgrade.
  • about runtime behavior: the implication in transaction extensions are changing, currently I am not aware of any runtime that make use of this implication, the implication was introduced in 2412 release. But if a runtime make use of implication and there is a frontend code that use the implication (like signing implications for a signature transaction extension) then the frontend code will need to be updated. That said there is a direct path for upgrading the frontend as the implication order is now more sensible.

cc @georgepisaltu

acatangiu
acatangiu previously approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine for 2412

georgepisaltu
georgepisaltu previously approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I think this change should make it into the release ASAP, especially because the feature is so new, which should mean we don't break anyone actually.

@gui1117 gui1117 dismissed stale reviews from georgepisaltu and acatangiu via d6d79ba January 8, 2025 08:55
@gui1117
Copy link
Contributor

gui1117 commented Jan 8, 2025

cc @paritytech/release-engineering

@EgorPopelyaev EgorPopelyaev merged commit 4bd1ca9 into stable2412 Jan 14, 2025
195 of 199 checks passed
@EgorPopelyaev EgorPopelyaev deleted the backport-7028-to-stable2412 branch January 14, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gui1117 gui1117 gui1117 left review comments

@georgepisaltu georgepisaltu georgepisaltu left review comments

@acatangiu acatangiu acatangiu left review comments

Assignees
No one assigned
Labels
A3-backport Pull request is already reviewed well in another branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gui1117 @acatangiu @georgepisaltu @EgorPopelyaev