-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Update ante handlers to use SignatureV2 #6428
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6428 +/- ##
==========================================
- Coverage 62.79% 56.85% -5.95%
==========================================
Files 251 479 +228
Lines 16306 28796 +12490
==========================================
+ Hits 10240 16371 +6131
- Misses 5361 11268 +5907
- Partials 705 1157 +452 |
…ronc/6213-ante-sig-verify
…ronc/6213-ante-sig-verify
…s-sdk into aaronc/6213-ante-sig-verify
It would be great if someone else familiar with the ante handler stack could take a look at this @alexanderbez @alessio @jgimeno. It’s in the critical path for the proto migration |
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.
The decorators all look correct to me. Is this intended to be a backwards-compatible change?
If so, would it make more sense to implement these as separate decorators SigVerificationV2Decorator
, so that people could use the old versions if they wish?
would prefer an approval from one of the SDK code owners before merging
It should be backwards compatible as is. i.e. these decorators are doing the exact same thing they were before except using an API that also supports proto tx’s. So l don’t think making them separate decorators is actually necessary. Thanks for reviewing the logic! |
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.
ACK
* Update ante handlers to use SignatureV2 * WIP on test fixes * WIP on test fixes * Debugging CLI Tests * CHANGELOG.md * CHANGELOG * Add missing tests for ante * Add tests for verify signatures * Update verify tests * Fix register codec issue * debug * Cleanup * Fix verify signature tests * Remove viper * remove keyring usage * Fix review changes * Fix simapp * Fix ante tests * Add reference issue * Add test for multisignature * Wrap sign error with sig * Fix verify sign test * Fix CHANGELOG.md Co-authored-by: sahith-narahari <[email protected]> Co-authored-by: anilCSE <[email protected]> Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: Alexander Bezobchuk <[email protected]>
This is one of several bite-size PR's that are being pulled out of #6216 to implement #6213.
This PR:
SigVerificationDecorator
andSigGasConsumeDecorator
now work with theSignatureV2
infrastructure introduced in Add SignatureV2 infrastructure #6373 .SignatureVerificationGasConsumer
now has the signature:func(meter sdk.GasMeter, sig signing.SignatureV2, params types.Params) error
.NewAnteHandler
andNewSigVerificationDecorator
both now take aSignModeHandler
parameter.SigVerifiableTx
interface now has aGetSignaturesV2() ([]signing.SignatureV2, error)
method and no longer hasGetSignatures
andGetSignBytes
methods.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorer