-
Notifications
You must be signed in to change notification settings - Fork 665
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
[Based on transaction extension 3685] make transaction extension pipeline not rejecting not signed origins #5612
Conversation
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.
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:
- change the behavior in the Transaction Extension PR to disallow transactions with no origin
- 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 We can just remove |
This comment was marked as outdated.
This comment was marked as outdated.
…gui-george-tx-ext-stable
ca8835e
to
04af1fc
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
}, | ||
} | ||
|
||
impl<T: Config> core::fmt::Debug for Pre<T> { |
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.
Why does Pre
need to be Debug
?
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.
actually only for some tests.
5c22d66
into
george/restore-gav-tx-ext
Based on #3685
Update transaction extensions so that not signed origins are passing throught and correctly refunded.
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 beOption<Nonce>
, so that we can have proper weight ahead of time and not need to do refund.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