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.

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

[Based on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins #5612

Open
wants to merge 16 commits into
base: george/restore-gav-tx-ext
Choose a base branch
from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Sep 6, 2024

Based on #3685

Update transaction extensions so that not signed origins are passing throught and correctly refunded.

  • payments transaction extensions is done with refund.
    Ideally the transaction extension should be an option, so that we don't need to refund and instead we don't charge when the transaction extension is not used.
  • PrevalidateAttests is refactor to have the weight at the correct place.
  • CheckNonce is refunded for when nonce is not checked.
    Same ideally Nonce should be Option<Nonce>, so that we can have proper weight ahead of time and not need to do refund.
  • bridges transaction extensions, allow not signed origins. But benchmark and weight will be done in another PR

I'm thinking we should be able to support multiple version of transaction extension at the same time in the runtime. So we can easily extend the capability of the transaction extensions.
Then we can have optional transaction payment and optional check nonce easily.

@georgepisaltu

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.

Looks good so far, let's add a test for each of the transaction payment extensions which checks the NoCharge behavior and asserts the full weight is refunded.

However, we should not merge this change together with phase 1 of extrinsic horizon (Transaction Extension), at least the way it is now.

This is because from the moment the transaction extension PR is merged, general transactions can be constructed. In the current verison of the Transaction Extension PR there is no DenyNone or custom Applyable logic to deny transactions with a None origin.

We need to either:

  1. change the behavior in the Transaction Extension PR to disallow transactions with no origin
  2. wait for the phase 2 PR to be completed to merge this (in other words, develop this PR further with the other changes)

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 7, 2024

We need to either:

1. change the behavior in the Transaction Extension PR to disallow transactions with no origin

2. wait for the phase 2 PR to be completed to merge this (in other words, develop this PR further with the other changes)

I would choose 1.

I was thinking with 2. that ValidateUnsigned will only be deprecated not removed.
And there is still inherent which are bare transaction which dispatch a None origin. Those transaction must be denied in the transaction extension. Either DenyNone or in the DispatchTransaction/CheckExtrinsic implementation in sp-runtime.

We can just remove DenyNone, people can implement it themselves if they have their own implementation of Extrinsic.

@paritytech-cicd-pr

This comment was marked as outdated.

@gui1117 gui1117 changed the title WIP: Transaction extension horizon: phase 2: make transaction extension pipeline not rejecting origins by default [Base on trransaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments Sep 20, 2024
@gui1117 gui1117 changed the title [Base on trransaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments [Base on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments Sep 20, 2024
@gui1117 gui1117 changed the title [Base on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments [Based on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments Sep 20, 2024
@gui1117 gui1117 changed the title [Based on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins for payments [Based on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins Sep 20, 2024
@gui1117 gui1117 marked this pull request as ready for review September 20, 2024 02:21
@gui1117 gui1117 requested a review from a team as a code owner September 20, 2024 02:21
@paritytech-review-bot paritytech-review-bot bot requested a review from a team September 20, 2024 04:03
@gui1117

This comment was marked as resolved.

@command-bot

This comment was marked as resolved.

@command-bot

This comment was marked as resolved.

@paritytech paritytech deleted a comment from command-bot bot Sep 20, 2024
@paritytech paritytech deleted a comment from command-bot bot Sep 20, 2024
},
}

impl<T: Config> core::fmt::Debug for Pre<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does Pre need to be Debug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants