revert: Remove SIGN_MODE_AMINO_AUX #10322

amaury1093 · 2021-10-07T13:21:40Z

Description

As part of the TX working group, we decided not to introduce a new sign mode for tipper signing. The tipper will use amino-json to sign via ledger (see #10346).

Also, add signing.SignerData#Address (will be used in subsequent PR #10346)

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/keeper/bech32_codec.go

x/auth/address/bech32.go

simapp/params/proto.go

codecov · 2021-10-13T10:34:41Z

Codecov Report

Merging #10322 (5bdbbf3) into master (26bcb1e) will decrease coverage by 0.04%.
The diff coverage is 37.93%.

❗ Current head 5bdbbf3 differs from pull request most recent head defddc5. Consider uploading reports for the commit defddc5 to get more accurate results

@@            Coverage Diff             @@
##           master   #10322      +/-   ##
==========================================
- Coverage   64.27%   64.23%   -0.05%     
==========================================
  Files         572      570       -2     
  Lines       54230    54130     -100     
==========================================
- Hits        34859    34771      -88     
+ Misses      17389    17378      -11     
+ Partials     1982     1981       -1

Impacted Files	Coverage Δ
server/rosetta/client_online.go	`0.00% <ø> (ø)`
x/auth/client/cli/tx_multisign.go	`0.00% <0.00%> (ø)`
x/auth/client/cli/validate_sigs.go	`0.00% <0.00%> (ø)`
x/auth/keeper/keeper.go	`70.00% <ø> (ø)`
x/auth/tx/config.go	`88.88% <ø> (ø)`
x/auth/tx/mode_handler.go	`78.94% <ø> (-2.01%)`	⬇️
x/auth/tx/direct_aux.go	`80.00% <14.28%> (-20.00%)`	⬇️
client/tx/tx.go	`41.23% <100.00%> (+0.30%)`	⬆️
server/rosetta/converter.go	`56.55% <100.00%> (+0.17%)`	⬆️
x/auth/keeper/bech32_codec.go	`55.00% <100.00%> (ø)`
... and 7 more

robert-zaremba

utACK. We made a concept review on a call with @AmauryM .
Left one question.

robert-zaremba · 2021-10-15T16:05:02Z

simapp/helpers/test_helpers.go

@@ -57,9 +57,11 @@ func GenTx(gen client.TxConfig, msgs []sdk.Msg, feeAmt sdk.Coins, gas uint64, ch
 	// 2nd round: once all signer infos are set, every signer can sign.
 	for i, p := range priv {
 		signerData := authsign.SignerData{
+			Address:       sdk.AccAddress(p.PubKey().Address()).String(),


why this was removed before?

The signerData.Address field was actually never present before, and newly added in this PR

atheeshp

lgtm

## Description R4R Closes: #9912 This PR introduces 1 new middleware: - `TipsMiddleware`: transfer tip from tipper to feePayer when relevant. It also makes sure in the DIRECT_AUX sign mode handler that the fee payer cannot use that sign mode. Depends on: - [x] #10028 - [x] #10268 - [x] #10322 - [x] #10346  --- ### 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... - [ ] 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)

## Description R4R Closes: cosmos#9912 This PR introduces 1 new middleware: - `TipsMiddleware`: transfer tip from tipper to feePayer when relevant. It also makes sure in the DIRECT_AUX sign mode handler that the fee payer cannot use that sign mode. Depends on: - [x] cosmos#10028 - [x] cosmos#10268 - [x] cosmos#10322 - [x] cosmos#10346  --- ### 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... - [ ] 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)

## Description Revert cosmos#10268 As part of the TX working group, we decided not to introduce a new sign mode for tipper signing. The tipper will use amino-json to sign via ledger (see cosmos#10346). Also, add `signing.SignerData#Address` (will be used in subsequent PR cosmos#10346)  --- ### 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... - [ ] 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)

github-actions bot added C:CLI C:x/auth labels Oct 7, 2021

amaury1093 added 4 commits October 12, 2021 11:15

Add IsTipper

6ec5722

Use addr in signer data

6eb5a10

Always populate addr in signer data

72a56cb

fi error messages

c0bd505

amaury1093 force-pushed the am/0fee0gas branch from ddd7ba0 to c0bd505 Compare October 12, 2021 09:22

amaury1093 changed the base branch from am/9912-tips-middleware to master October 12, 2021 09:22

amaury1093 changed the title ~~DNM Proof-of-Concept: Tips without SIGN_MODE_AMINO_AUX~~ revert: Remove SIGN_MODE_AMINO_AUX Oct 12, 2021

make proto gen

551f5f4

github-actions bot added the C:x/authz label Oct 12, 2021

amaury1093 added 2 commits October 12, 2021 12:00

Merge branch 'master' into am/0fee0gas

49987e7

fix build

76b8dff

cosmos deleted a comment from lgtm-com bot Oct 12, 2021

Remove getSequence

3f04021

amaury1093 force-pushed the am/0fee0gas branch from 32ee9b4 to 3f04021 Compare October 12, 2021 13:17

This was referenced Oct 12, 2021

feat: Add fee.{payer,granter} and tip fields to StdSignDoc #10348

Merged

Add feepayer, feegranter and tip fields to StdSignDoc #10346

Closed

amaury1093 added 2 commits October 12, 2021 17:30

Use addressCodec

894a7ce

NewTxConfig with addrCdc

6c66bfb

amaury1093 commented Oct 12, 2021

View reviewed changes

x/auth/keeper/bech32_codec.go Outdated Show resolved Hide resolved

amaury1093 added 5 commits October 12, 2021 17:56

REmove simapp.NewBech32

debee8d

Move bech32 stuff to x/auth/address

e3a7c35

Add changelog

dfd1d82

Move address.Codec to x/auth

7092ee8

Fix test

bac6349

amaury1093 commented Oct 13, 2021

View reviewed changes

x/auth/address/bech32.go Outdated Show resolved Hide resolved

amaury1093 marked this pull request as ready for review October 13, 2021 10:20

amaury1093 requested a review from aaronc as a code owner October 13, 2021 10:20

amaury1093 requested a review from alexanderbez as a code owner October 13, 2021 10:20

amaury1093 requested a review from atheeshp October 13, 2021 10:20

amaury1093 commented Oct 13, 2021

View reviewed changes

simapp/params/proto.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Oct 14, 2021

feat: Add Tips middleware #10208

Merged

23 tasks

amaury1093 added 4 commits October 14, 2021 18:43

revert API breaking changes

5bdbbf3

cleanup

4992111

Less file diff

6d8734a

Merge branch 'master' into am/0fee0gas

1541e4b

robert-zaremba approved these changes Oct 15, 2021

View reviewed changes

atheeshp approved these changes Oct 18, 2021

View reviewed changes

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

amaury1093 added 2 commits October 18, 2021 11:50

Merge branch 'master' into am/0fee0gas

a86e778

Merge branch 'master' into am/0fee0gas

defddc5

mergify bot merged commit 99e2e0f into master Oct 18, 2021

mergify bot deleted the am/0fee0gas branch October 18, 2021 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: Remove SIGN_MODE_AMINO_AUX #10322

revert: Remove SIGN_MODE_AMINO_AUX #10322

amaury1093 commented Oct 7, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

robert-zaremba left a comment

robert-zaremba Oct 15, 2021

amaury1093 Oct 15, 2021

atheeshp left a comment

revert: Remove SIGN_MODE_AMINO_AUX #10322

revert: Remove SIGN_MODE_AMINO_AUX #10322

Conversation

amaury1093 commented Oct 7, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Oct 15, 2021

Choose a reason for hiding this comment

amaury1093 Oct 15, 2021

Choose a reason for hiding this comment

atheeshp left a comment

Choose a reason for hiding this comment

amaury1093 commented Oct 7, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading