Fix sequence number handling for LegacyAmino > SignatureV2 #7285
Conversation
…e without SkipSequenceCheck
clevinson
requested review from
aaronc,
alessio and
alexanderbez
as code owners
Codecov Report
@@ Coverage Diff @@
## master #7285 +/- ##
==========================================
+ Coverage 55.60% 56.60% +0.99%
==========================================
Files 457 435 -22
Lines 27440 31712 +4272
==========================================
+ Hits 15257 17949 +2692
- Misses 11083 12291 +1208
- Partials 1100 1472 +372 |
x/auth/ante/sigverify.go
Outdated
| } | ||
| return false | ||
| default: | ||
| panic("Type Mismatch") |
There was a problem hiding this comment.
WIP, sorry, forgot to mark as draft. Would this be better off returning false, or an error?
|
Moved this back to draft mode as it seems we still have incomplete work to do. Also, can't we add an integration test for the REST handler? It'd be no different than the CLI integration tests. Then again, this is for the legacy API correct? |
|
Sorry! this was meant to be draft. Thanks for reverting.
@alexanderbez Yes this is for the legacy API. I thought this WIP test for broadcasting signed transaction was testing the rest handler (not CLI)... isn't it? I was using some CLI stuff to generate a valid signature & tx to send over REST though... My thought was to get this test working, and then add a subsequent tx to it to check for non-zero sequences. Should i be looking at something else for rest handler tests? |
|
The test case looks correct to me |
|
You're absolutely correct @clevinson. That strategy looks correct to me :) |
|
New rest handler test is passing, marking R4R. @zmanian @amaurymartiny @alexanderbez |
x/auth/ante/sigverify.go
Outdated
| @@ -237,7 +253,9 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul | |||
| err := authsigning.VerifySignature(pubKey, signerData, sig.Data, svd.signModeHandler, tx) | |||
| if err != nil { | |||
| var errMsg string | |||
| if sig.SkipSequenceCheck { | |||
| if OnlyLegacyAminoSigners(sig.Data) { | |||
There was a problem hiding this comment.
in line 230 we already evaluated OnlyLegacyAminoSigners(sig.Data). So let's store the result above and use it in both if statements.
There was a problem hiding this comment.
Shouldn't we just fail a transaction that contains mixed Sign modes?
There was a problem hiding this comment.
Depending on the users. This would solve my other concern.
There was a problem hiding this comment.
Mixed sign modes I believe is a valid use case that we need to support (see ADR020, and this issue thread on protobuf tx signing). When dealing with multisig transactions, each signer may be using their own independent sign mode, right?
|
It looks like some miscellaneous stuff was committed accidentally in |
|
Should i rebase & cherry pick the commit with artifact files out ? looks like the folder is 78Mb :/ |
|
I think they should be eliminated when squashing the commits? |
Yeah, I would say so too. |
Co-authored-by: Zaki Manian <zaki@manian.org>
clevinson
added
the
A:automerge
Description
I've added a new rest handler test in
x/auth/client/rest/rest_test.gowith which i've been able to reproduce @zmanian's error.Additionally, I implemented the basic logic @aaronc described (checking for any SIGN_MODE_DIRECT modes in SignatureData), as an alternative to SkipSequenceCheck flag. This strategy does infact get my tests to pass.
Would be great if @zmanian can have a look.
closes: #7229
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/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes