Put AccountSequence in SignerInfo #6997

amaury1093 · 2020-08-10T14:19:00Z

Description

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.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

amaury1093 · 2020-08-10T14:20:04Z

docs/architecture/adr-020-protobuf-transaction-encoding.md

@@ -28,7 +28,7 @@ querying. However, this ADR solely focuses on transactions. Querying should be
 addressed in a future ADR, but it should build off of these proposals.

 Based on detailed discussions ([\#6030](https://github.com/cosmos/cosmos-sdk/issues/6030)
-and [\#6078](https://github.com/cosmos/cosmos-sdk/issues/6078)), the original 
+and [\#6078](https://github.com/cosmos/cosmos-sdk/issues/6078)), the original


sorry, spacing changes are made by my editor... but i think they make sense.

Thank you editor. By the way, have you considered adding an .editorconfig to this repo? I made very good experience with it and use it in all projects: https://editorconfig.org/

codecov · 2020-08-10T14:23:38Z

Codecov Report

Merging #6997 into master will increase coverage by 0.05%.
The diff coverage is 87.27%.

@@            Coverage Diff             @@
##           master    #6997      +/-   ##
==========================================
+ Coverage   54.49%   54.54%   +0.05%     
==========================================
  Files         550      550              
  Lines       37706    37684      -22     
==========================================
+ Hits        20546    20554       +8     
+ Misses      15472    15441      -31     
- Partials     1688     1689       +1

…966-seq-in-signerinfo

amaury1093 · 2020-08-12T14:50:57Z

x/auth/signing/sign_mode_handler.go

+	// for replay protection. This field is only useful for Legacy Amino signing,
+	// since in SIGN_MODE_DIRECT the account sequence is already in the signer
+	// info.
 	AccountSequence uint64


I would have loved to remove AccountSequence from SignerData, but afaiu it is still used for SIGN_MODE_LEGACY. So keeping it here.

amaury1093 · 2020-08-12T14:54:54Z

x/auth/ante/sigverify.go

+		// Check account sequence number.
+		if sig.AccountSequence != acc.GetSequence() {
+			return ctx, sdkerrors.Wrapf(
+				sdkerrors.ErrWrongAccountSequence,
+				"account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.AccountSequence,
+			)
+		}


A separate ante handler would also have made sense, but since it's just 7 lines, I thought just to leave it here.

Amazing to pull out this error case. Much appreciated

Yes, this is the a huge benefit for clients.

amaury1093 · 2020-08-12T15:01:30Z

A concern I have is how to handle StdTx. Basically, StdTx needs to hold the account sequences of signers, or else functions like StdSignatureToSignatureV2 result in a loss of information.

…osmos-sdk into am-6966-seq-in-signerinfo

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

ethanfrey

In general, it looks good and I am very happy for this better error message on incorrect sequence numbers (the most common case of a theoretically valid tx failing... when they end up out of order).

I think there is some simplification that can be made and the test constants cleaned up a bit. Also the AccountSequence vs Sequence issue... but I think you inherited the naming mis-match

client/testutil/suite.go

client/tx/tx.go

x/auth/ante/ante_test.go

ethanfrey · 2020-08-14T12:22:50Z

x/auth/ante/sigverify.go

+		// Check account sequence number.
+		if sig.AccountSequence != acc.GetSequence() {
+			return ctx, sdkerrors.Wrapf(
+				sdkerrors.ErrWrongAccountSequence,
+				"account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.AccountSequence,
+			)
+		}


Yes, this is the a huge benefit for clients.

x/auth/ante/sigverify_test.go

x/auth/ante/testutil_test.go

…966-seq-in-signerinfo

x/bank/app_test.go

x/auth/types/amino_signing_test.go

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

…966-seq-in-signerinfo

…osmos-sdk into am-6966-seq-in-signerinfo

* WIP test the grounds * Update ADR020 * Fix compile errors * Fix ADR * Make ante tests pass * Fix remaining ante handler tests * Simplify code * Fix x/bank app_test * Fix tests * Remove useless accSeq from signerdata * Fix test * Update simapp/helpers/test_helpers.go Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Update simapp/helpers/test_helpers.go Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Address rewview * Update x/auth/ante/sigverify.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * Update x/auth/ante/sigverify_test.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * Update x/auth/tx/builder_test.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * Update x/auth/tx/builder_test.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * Update x/auth/tx/direct_test.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * Update x/auth/tx/builder_test.go Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> * AccSeq -> Seq * Address reviews * Better variable naming * Fix variable assign * Remove old SetSignerInfo * Fix test * proto-gen * Make proto-gen * Reput gw comment * Add Changelog * Update x/bank/app_test.go Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Update x/bank/app_test.go Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: SaReN <sahithnarahari@gmail.com> Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

amaury1093 added 4 commits August 10, 2020 12:19

WIP test the grounds

8b9e820

Update ADR020

364c7aa

Fix compile errors

4f789d6

Fix ADR

3e71d87

amaury1093 commented Aug 10, 2020

View reviewed changes

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

fcf8554

…966-seq-in-signerinfo

amaury1093 added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: never don't do this! and removed T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Aug 11, 2020

amaury1093 added 9 commits August 11, 2020 16:34

Make ante tests pass

a4fcf67

Fix remaining ante handler tests

99a59f4

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

74ecbff

…966-seq-in-signerinfo

Simplify code

48b380d

Fix x/bank app_test

a2d42aa

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

4d10cd1

…966-seq-in-signerinfo

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

0231b26

…966-seq-in-signerinfo

Fix tests

e2d8b5c

Merge branch 'master' into am-6966-seq-in-signerinfo

813961b

amaury1093 commented Aug 12, 2020

View reviewed changes

amaury1093 added 2 commits August 12, 2020 16:51

Merge branch 'master' into am-6966-seq-in-signerinfo

10f4849

Remove useless accSeq from signerdata

76123e3

amaury1093 commented Aug 12, 2020

View reviewed changes

amaury1093 added 3 commits August 12, 2020 18:00

Fix test

576a1d9

Merge branch 'am-6966-seq-in-signerinfo' of ssh://github.com/cosmos/c…

7256038

…osmos-sdk into am-6966-seq-in-signerinfo

Merge branch 'master' into am-6966-seq-in-signerinfo

020ab39

amaury1093 marked this pull request as ready for review August 12, 2020 16:42

amaury1093 requested review from aaronc and alessio as code owners August 12, 2020 16:42

amaury1093 and others added 5 commits August 14, 2020 14:00

Update x/auth/tx/builder_test.go

f5c04c7

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

Update x/auth/tx/direct_test.go

14b7a62

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

Update x/auth/tx/builder_test.go

7f44ee5

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

AccSeq -> Seq

dd5f6c9

Address reviews

5b8edb0

ethanfrey reviewed Aug 14, 2020

View reviewed changes

amaury1093 added 11 commits August 14, 2020 15:32

Better variable naming

2f4b399

Fix variable assign

9cdeaa0

Remove old SetSignerInfo

9f2d19b

Fix test

c4dda3b

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

03a3cf8

…966-seq-in-signerinfo

proto-gen

ed552df

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

be7410c

…966-seq-in-signerinfo

Make proto-gen

4f01844

Reput gw comment

2fdf052

Add Changelog

8f102b5

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

55fac18

…966-seq-in-signerinfo

alexanderbez reviewed Aug 20, 2020

View reviewed changes

x/bank/app_test.go Outdated Show resolved Hide resolved

alexanderbez reviewed Aug 20, 2020

View reviewed changes

x/bank/app_test.go Outdated Show resolved Hide resolved

alexanderbez reviewed Aug 20, 2020

View reviewed changes

x/auth/types/amino_signing_test.go Show resolved Hide resolved

amaury1093 and others added 5 commits August 20, 2020 18:26

Update x/bank/app_test.go

432128f

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/bank/app_test.go

dfe6082

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-6…

2675653

…966-seq-in-signerinfo

Merge branch 'am-6966-seq-in-signerinfo' of ssh://github.com/cosmos/c…

9eca77d

…osmos-sdk into am-6966-seq-in-signerinfo

Merge branch 'master' into am-6966-seq-in-signerinfo

7d1d281

alexanderbez approved these changes Aug 21, 2020

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 21, 2020

mergify bot merged commit 50bab8f into master Aug 21, 2020

mergify bot deleted the am-6966-seq-in-signerinfo branch August 21, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put AccountSequence in SignerInfo #6997

Put AccountSequence in SignerInfo #6997

amaury1093 commented Aug 10, 2020 •

edited

Loading

amaury1093 Aug 10, 2020

webmaster128 Aug 14, 2020

codecov bot commented Aug 10, 2020 •

edited

Loading

amaury1093 Aug 12, 2020

amaury1093 Aug 12, 2020

webmaster128 Aug 14, 2020

ethanfrey Aug 14, 2020

amaury1093 commented Aug 12, 2020

ethanfrey left a comment

ethanfrey Aug 14, 2020

Put AccountSequence in SignerInfo #6997

Put AccountSequence in SignerInfo #6997

Conversation

amaury1093 commented Aug 10, 2020 • edited Loading

Description

amaury1093 Aug 10, 2020

Choose a reason for hiding this comment

webmaster128 Aug 14, 2020

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2020 • edited Loading

Codecov Report

amaury1093 Aug 12, 2020

Choose a reason for hiding this comment

amaury1093 Aug 12, 2020

Choose a reason for hiding this comment

webmaster128 Aug 14, 2020

Choose a reason for hiding this comment

ethanfrey Aug 14, 2020

Choose a reason for hiding this comment

amaury1093 commented Aug 12, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Aug 14, 2020

Choose a reason for hiding this comment

amaury1093 commented Aug 10, 2020 •

edited

Loading

codecov bot commented Aug 10, 2020 •

edited

Loading