Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom key types and address formats #4232

Merged
merged 9 commits into from
May 2, 2019

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Apr 29, 2019

Resolves #3685. (Was PR #4166 before the develop branch was removed.)

Adds:

  • an additional parameter to NewAnteHandler for a custom SignatureVerificationGasConsumer (the existing one is now called `DefaultSigVerificationGasConsumer)
  • add an addressVerifier field to sdk.Config which allows for custom address verification (to override the current fixed 20 byte address format)

Changes:

  • the DefaultSigVerificationGasConsumer now uses type switching as opposed to string comparison. Other zones like Ethermint can now concretely specify which key types they accept.

  • 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.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #4232 into master will increase coverage by <.01%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #4232      +/-   ##
==========================================
+ Coverage   60.18%   60.19%   +<.01%     
==========================================
  Files         212      212              
  Lines       15187    15168      -19     
==========================================
- Hits         9140     9130      -10     
+ Misses       5418     5413       -5     
+ Partials      629      625       -4

1 similar comment
@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #4232 into master will increase coverage by <.01%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #4232      +/-   ##
==========================================
+ Coverage   60.18%   60.19%   +<.01%     
==========================================
  Files         212      212              
  Lines       15187    15168      -19     
==========================================
- Hits         9140     9130      -10     
+ Misses       5418     5413       -5     
+ Partials      629      625       -4

@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #4232 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4232      +/-   ##
==========================================
+ Coverage   60.18%   60.29%   +0.11%     
==========================================
  Files         212      212              
  Lines       15187    15171      -16     
==========================================
+ Hits         9140     9148       +8     
+ Misses       5418     5403      -15     
+ Partials      629      620       -9

@aaronc aaronc marked this pull request as ready for review April 29, 2019 21:01
@aaronc
Copy link
Member Author

aaronc commented Apr 29, 2019

@alexanderbez I reopened #4166 over here and made the changes you requested.

x/auth/ante_test.go Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from sabau April 30, 2019 15:55
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, just a few minor bits of feedback.

.pending/breaking/sdk/The-default-signatur Outdated Show resolved Hide resolved
.pending/breaking/sdk/The-default-signatur Outdated Show resolved Hide resolved
.pending/improvements/sdk/Add-SetAddressVerifi Outdated Show resolved Hide resolved
.pending/improvements/sdk/Add-an-additional-pa Outdated Show resolved Hide resolved
@@ -86,6 +86,18 @@ func AccAddressFromHex(address string) (addr AccAddress, err error) {
return AccAddress(bz), nil
}

func VerifyAddressFormat(bz []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a quick godoc for this?

@alexanderbez alexanderbez added ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels May 1, 2019
@@ -86,6 +86,18 @@ func AccAddressFromHex(address string) (addr AccAddress, err error) {
return AccAddress(bz), nil
}

func VerifyAddressFormat(bz []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing godoc comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to have a test case as well

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor changes are required, it looks good otherwise.

@alessio
Copy link
Contributor

alessio commented May 1, 2019

oh lol, I've left essentially the same feedback as @alexanderbez, I didn't see it coming through. Please address the comments and feel free to dismiss my review 👍

EDIT: removed dup comments/suggestions

alexanderbez and others added 3 commits May 2, 2019 14:37
Co-Authored-By: aaronc <aaron@regen.network>
Co-Authored-By: aaronc <aaron@regen.network>
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACKed. Will merge shortly.

@alessio alessio merged commit 114de63 into cosmos:master May 2, 2019
@ryanchristo ryanchristo deleted the regen-network/address-format branch December 12, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixed 20 byte addresses reduce security and don't accommodate all use cases
4 participants