feat: TxRaw must follow ADR 027 #9743

amaury1093 · 2021-07-21T14:59:12Z

Description

Closes: #XXXX

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

x/auth/tx/decoder.go

orijbot · 2021-07-21T15:04:38Z

Visit https://dashboard.github.orijtech.com?pr=9743&repo=cosmos%2Fcosmos-sdk to see benchmark details.

x/auth/tx/decoder.go

codecov · 2021-07-21T17:41:14Z

Codecov Report

Merging #9743 (84443c9) into master (0027111) will increase coverage by 27.98%.
The diff coverage is 63.79%.

❗ Current head 84443c9 differs from pull request most recent head 002f2c1. Consider uploading reports for the commit 002f2c1 to get more accurate results

@@             Coverage Diff             @@
##           master    #9743       +/-   ##
===========================================
+ Coverage   35.48%   63.46%   +27.98%     
===========================================
  Files         332      563      +231     
  Lines       32620    37502     +4882     
===========================================
+ Hits        11575    23801    +12226     
+ Misses      19819    11838     -7981     
- Partials     1226     1863      +637

Impacted Files	Coverage Δ
client/keys/types.go	`100.00% <ø> (+100.00%)`	⬆️
client/query.go	`16.98% <ø> (ø)`
client/rpc/block.go	`0.00% <ø> (ø)`
client/rpc/status.go	`67.74% <ø> (ø)`
client/rpc/validators.go	`0.00% <ø> (ø)`
client/test_helpers.go	`0.00% <ø> (ø)`
client/tx/factory.go	`27.00% <ø> (ø)`
client/tx/legacy.go	`68.42% <ø> (ø)`
client/tx/tx.go	`40.83% <ø> (ø)`
client/utils.go	`41.93% <ø> (-41.40%)`	⬇️
... and 683 more

amaury1093 · 2021-07-22T09:22:34Z

R4R again.

… into am/tx-dec-reject

aaronc

Mostly looks good just a couple questions. Thanks!

aaronc · 2021-07-22T17:25:04Z

x/auth/tx/decoder.go

+
+// varintMinLength returns the minimum number of bytes necessary to encode an
+// uint using varint encoding.
+func varintMinLength(n uint64) int {


This looks correct. Is it actually possible to encode a varint with more than the minimum number of bytes? If so, we should definitely check this, I just have trouble wrapping my head around how that woudl happen

Yeah it's possible, have a look at:

cosmos-sdk/x/auth/tx/encode_decode_test.go

Lines 199 to 201 in 84443c9

// bodyBz's length prefix is 5, with `5` as varint encoding. We also try a

// longer varint encoding for 5: `133 00`.

longVarintBodyBz := append(append([]byte{bodyBz[0]}, byte(133), byte(00)), bodyBz[2:]...)

i.e.: if the byte stream is <x 5 x x x x x>, then <x 133 0 x x x x x> is also valid.

Okay, good to know!!

x/auth/tx/decoder.go

aaronc · 2021-07-22T17:45:50Z

Maybe we don't need to block merging, but would be good to get one more set of eyes on this if possible before release. Maybe @robert-zaremba when you're back?

amaury1093 · 2021-07-22T17:47:33Z

Maybe we don't need to block merging, but would be good to get one more set of eyes on this if possible before release. Maybe @robert-zaremba when you're back?

I can ping him tomorrow to review the 0.43 backport PR.

## Description Closes: #XXXX  --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit adff7d8)

## Description Closes: #XXXX  --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit adff7d8) Co-authored-by: Amaury <[email protected]>

feat: TxRaw must follow ADR 027

d8523c7

github-actions bot added the C:x/auth label Jul 21, 2021

amaury1093 requested review from alexanderbez and aaronc July 21, 2021 15:01

amaury1093 commented Jul 21, 2021

View reviewed changes

x/auth/tx/decoder.go Outdated Show resolved Hide resolved

Add varint checks

7dc1385

amaury1093 commented Jul 21, 2021

View reviewed changes

x/auth/tx/decoder.go Outdated Show resolved Hide resolved

amaury1093 marked this pull request as ready for review July 21, 2021 17:11

amaury1093 added 2 commits July 21, 2021 19:27

fix fuzz test

33939f4

better errors

24220c4

amaury1093 added 2 commits July 22, 2021 11:20

Use hardcoded table for varint length

c46b93f

Merge branch 'master' into am/tx-dec-reject

def816c

amaury1093 added 2 commits July 22, 2021 11:24

Comments

e7de668

Merge branch 'am/tx-dec-reject' of ssh://github.com/cosmos/cosmos-sdk…

84443c9

… into am/tx-dec-reject

alexanderbez approved these changes Jul 22, 2021

View reviewed changes

amaury1093 added the backport/0.43.x label Jul 22, 2021

aaronc reviewed Jul 22, 2021

View reviewed changes

Check wireType too

002f2c1

aaronc approved these changes Jul 22, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 22, 2021

mergify bot merged commit adff7d8 into master Jul 22, 2021

mergify bot deleted the am/tx-dec-reject branch July 22, 2021 17:54

mergify bot mentioned this pull request Jul 22, 2021

feat: TxRaw must follow ADR 027 (backport #9743) #9754

Merged

fedekunze mentioned this pull request Nov 14, 2021

all: cherry pick olympus mons tharsis/cosmos-sdk#1

Closed

19 tasks

amaury1093 mentioned this pull request Aug 17, 2022

docs(textual): Require signing over raw bytes #12910

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: TxRaw must follow ADR 027 #9743

feat: TxRaw must follow ADR 027 #9743

amaury1093 commented Jul 21, 2021 •

edited

Loading

orijbot commented Jul 21, 2021

codecov bot commented Jul 21, 2021 •

edited

Loading

amaury1093 commented Jul 22, 2021

aaronc left a comment

aaronc Jul 22, 2021

amaury1093 Jul 22, 2021

aaronc Jul 22, 2021

aaronc commented Jul 22, 2021

amaury1093 commented Jul 22, 2021

	// bodyBz's length prefix is 5, with `5` as varint encoding. We also try a
	// longer varint encoding for 5: `133 00`.
	longVarintBodyBz := append(append([]byte{bodyBz[0]}, byte(133), byte(00)), bodyBz[2:]...)

feat: TxRaw must follow ADR 027 #9743

feat: TxRaw must follow ADR 027 #9743

Conversation

amaury1093 commented Jul 21, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

orijbot commented Jul 21, 2021

codecov bot commented Jul 21, 2021 • edited Loading

Codecov Report

amaury1093 commented Jul 22, 2021

aaronc left a comment

Choose a reason for hiding this comment

aaronc Jul 22, 2021

Choose a reason for hiding this comment

amaury1093 Jul 22, 2021

Choose a reason for hiding this comment

aaronc Jul 22, 2021

Choose a reason for hiding this comment

aaronc commented Jul 22, 2021

amaury1093 commented Jul 22, 2021

amaury1093 commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 21, 2021 •

edited

Loading