feat!: Add bech32 prefix to authkeeper #9759

cyberbono3 · 2021-07-23T16:34:46Z

Description

Closes Step1 from #9690

Add auth keeper methods and gRPC queries:
- the NewAccountKeeper should take a string bech32Prefix argument
- auth AccountKeeper implements Codec interface that contains 2 methods: ConvertAddressStringToBytes and ConvertAddressBytesToString
- Add the 3 following gRPC queries:
  - Get bech32 prefix
  - AddressStringToBytes (converts AccountAddr string to AccountAddr bytes)
  - AddressBytesToString ( converts AccountAddr bytes to AccountAddr str)
- Add the corresponding keeper methods
- Add tests

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)

codecov · 2021-07-23T16:44:07Z

Codecov Report

Merging #9759 (3d2d5b0) into master (147d798) will decrease coverage by 0.00%.
The diff coverage is 60.00%.

❗ Current head 3d2d5b0 differs from pull request most recent head 4d2a866. Consider uploading reports for the commit 4d2a866 to get more accurate results

@@            Coverage Diff             @@
##           master    #9759      +/-   ##
==========================================
- Coverage   63.75%   63.74%   -0.01%     
==========================================
  Files         566      567       +1     
  Lines       53402    53422      +20     
==========================================
+ Hits        34048    34056       +8     
- Misses      17435    17445      +10     
- Partials     1919     1921       +2

Impacted Files	Coverage Δ
types/address.go	`68.69% <ø> (ø)`
x/auth/keeper/bech32_codec.go	`55.00% <55.00%> (ø)`
x/auth/keeper/grpc_query.go	`48.00% <60.00%> (-9.15%)`	⬇️
x/auth/keeper/keeper.go	`52.72% <66.66%> (-14.60%)`	⬇️
simapp/app.go	`81.66% <100.00%> (ø)`
crypto/keys/internal/ecdsa/privkey.go	`82.45% <0.00%> (-1.76%)`	⬇️
x/auth/vesting/types/msgs.go	`0.00% <0.00%> (ø)`

orijbot · 2021-07-23T16:48:37Z

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

amaury1093

looks like a good start. I'm not sure the proto field names are perfect, but can't think of anything else for now

simapp/app.go

x/auth/atlas/atlas-v0.39.1.md

x/auth/keeper/grpc_query_test.go

x/auth/keeper/keeper_test.go

x/auth/keeper/keeper.go

…berbono3/add-bech32Prefix-to-authkeeper

blushi

Looks good overall, just left a couple comments

x/auth/keeper/keeper.go

x/auth/keeper/grpc_query_test.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

x/auth/keeper/keeper.go

amaury1093 · 2021-08-24T12:37:18Z

LGTM. Are we just bypassing address verification for now in the auth keeper?

Actually we still do it! We just use the global config for now, let's tackle that in a separate PR (added an item in #9690). See https://github.com/cosmos/cosmos-sdk/pull/9759/files#diff-c73f18717271f4171c3ae22f928c5ab7b9890185a9101828252dacab71e78545R31

amaury1093

lgtm overall, small nits around not exporting too many public methods

x/auth/keeper/grpc_query_test.go

x/auth/keeper/keeper.go

x/auth/keeper/grpc_query_test.go

x/auth/keeper/keeper.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

cyberbono3 · 2021-08-24T15:00:39Z

@AmauryM I addressed your comments, can you take one more look, please?

amaury1093

came up with some new comments while re-reviewing

x/auth/keeper/keeper.go

x/auth/keeper/grpc_query_test.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

## Description Closes **Step1** from cosmos#9690  - Add auth keeper methods and gRPC queries: - the `NewAccountKeeper` should take a `string bech32Prefix` argument - ` auth AccountKeeper` implements `Codec` interface that contains 2 methods: `ConvertAddressStringToBytes` and `ConvertAddressBytesToString` - Add the 3 following gRPC queries: - Get bech32 prefix - `AddressStringToBytes` (converts `AccountAddr` string to `AccountAddr` bytes) - `AddressBytesToString` ( converts `AccountAddr` bytes to `AccountAddr` str) - Add the corresponding keeper methods - Add tests --- ### 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) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] 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)

first draft

030fc2c

github-actions bot added C:x/auth C:x/bank labels Jul 23, 2021

cyberbono3 force-pushed the cyberbono3/add-bech32Prefix-to-authkeeper branch from c71015f to c78a879 Compare July 23, 2021 16:48

cyberbono3 force-pushed the cyberbono3/add-bech32Prefix-to-authkeeper branch from c78a879 to 7da481f Compare July 23, 2021 16:52

tests pass

556810d

cyberbono3 force-pushed the cyberbono3/add-bech32Prefix-to-authkeeper branch from 7da481f to 556810d Compare July 23, 2021 16:54

cyberbono3 requested a review from amaury1093 July 23, 2021 16:56

cyberbono3 added 2 commits July 23, 2021 19:09

add DefaultBech32Prefix

8ec7872

add comments to auth/query.proto

52ec23d

amaury1093 reviewed Jul 26, 2021

View reviewed changes

cyberbono3 added 5 commits July 26, 2021 14:13

set sdk.Bech32MainPrefix argument in NewAccountKeeeper()

e636781

update x/auth/atlas/atlas-v0.39.1.md

9788e2c

fix TestBech32Prefix

3fcd6c3

fix tests

64f031f

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into cy…

befc823

…berbono3/add-bech32Prefix-to-authkeeper

cyberbono3 changed the title ~~Add bech32 prefix to authkeeper~~ refactor!: Add bech32 prefix to authkeeper Jul 26, 2021

gofmt

413cf7a

cyberbono3 changed the title ~~refactor!: Add bech32 prefix to authkeeper~~ feat!: Add bech32 prefix to authkeeper Jul 26, 2021

github-actions bot added the C:CLI label Jul 26, 2021

update atlas-v0.39.1.md

d9be317

cyberbono3 marked this pull request as ready for review July 26, 2021 14:53

cyberbono3 requested review from aaronc and alexanderbez as code owners July 26, 2021 14:53

Merge branch 'master' into cyberbono3/add-bech32Prefix-to-authkeeper

1dd726b

amaury1093 assigned amaury1093 and blushi Jul 26, 2021

blushi reviewed Jul 29, 2021

View reviewed changes

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

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

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

cyberbono3 and others added 3 commits August 24, 2021 15:04

Update types/address/codec.go

3678bce

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/keeper.go

53844bb

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Fix comments

b3fb3bd

cyberbono3 commented Aug 24, 2021

View reviewed changes

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

amaury1093 reviewed Aug 24, 2021

View reviewed changes

cyberbono3 and others added 7 commits August 24, 2021 15:46

Update x/auth/keeper/keeper.go

131de71

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/grpc_query_test.go

b32ce72

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/keeper.go

acc6675

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/keeper.go

0b27c66

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/grpc_query_test.go

d4267bf

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/grpc_query_test.go

9aae756

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

address reviewer comments

3d2d5b0

amaury1093 reviewed Aug 25, 2021

View reviewed changes

cyberbono3 and others added 3 commits August 25, 2021 13:13

Update x/auth/keeper/keeper.go

5d0ba44

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/grpc_query_test.go

a351a89

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Update x/auth/keeper/grpc_query_test.go

aa76cbc

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

cyberbono3 force-pushed the cyberbono3/add-bech32Prefix-to-authkeeper branch 2 times, most recently from ab34ebf to 16f55fc Compare August 25, 2021 10:51

apply reviewer comments

fb9e5a3

cyberbono3 force-pushed the cyberbono3/add-bech32Prefix-to-authkeeper branch from 16f55fc to fb9e5a3 Compare August 25, 2021 10:53

Merge branch 'master' into cyberbono3/add-bech32Prefix-to-authkeeper

4d2a866

amaury1093 approved these changes Aug 25, 2021

View reviewed changes

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

mergify bot merged commit 59640fb into master Aug 25, 2021

mergify bot deleted the cyberbono3/add-bech32Prefix-to-authkeeper branch August 25, 2021 11:17

c4-bot-3 mentioned this pull request Jun 20, 2024

Wrong address prefix for ethermint bech32 account leads to inability to authorize users code-423n4/2024-05-canto-validation#29

Open

howlbot-integration bot mentioned this pull request Jun 21, 2024

Wrong address prefix for ethermint bech32 account leads to inability to authorize users code-423n4/2024-05-canto-findings#17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add bech32 prefix to authkeeper #9759

feat!: Add bech32 prefix to authkeeper #9759

cyberbono3 commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 23, 2021 •

edited

Loading

orijbot commented Jul 23, 2021

amaury1093 left a comment

blushi left a comment

amaury1093 commented Aug 24, 2021 •

edited

Loading

amaury1093 left a comment

cyberbono3 commented Aug 24, 2021 •

edited

Loading

amaury1093 left a comment

feat!: Add bech32 prefix to authkeeper #9759

feat!: Add bech32 prefix to authkeeper #9759

Conversation

cyberbono3 commented Jul 23, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jul 23, 2021 • edited Loading

Codecov Report

orijbot commented Jul 23, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

amaury1093 commented Aug 24, 2021 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

cyberbono3 commented Aug 24, 2021 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

cyberbono3 commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 23, 2021 •

edited

Loading

amaury1093 commented Aug 24, 2021 •

edited

Loading

cyberbono3 commented Aug 24, 2021 •

edited

Loading