-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
crypto/keyring: change addrKey to store chain-agnostic addresses #5858
Conversation
This pull request introduces 1 alert when merging c8163ba into a1ac04c - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #5858 +/- ##
==========================================
+ Coverage 56.75% 56.77% +0.01%
==========================================
Files 344 344
Lines 20294 20307 +13
==========================================
+ Hits 11518 11529 +11
- Misses 7895 7896 +1
- Partials 881 882 +1 |
266ba0e
to
6819337
Compare
crypto/keyring/keyring.go
Outdated
@@ -581,3 +582,7 @@ func newRealPrompt(dir string, buf io.Reader) func(string) (string, error) { | |||
} | |||
} | |||
} | |||
|
|||
func addrKey(address types.AccAddress) []byte { | |||
return []byte(fmt.Sprintf("%s.%s", hex.EncodeToString(address.Bytes()), addressSuffix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why encode to hex when we can just use the String
interpretation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because accounts types String
methods depend on the notorious Config
singleton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change it for the old Keybase implementations to avoid breaking gaiacli
versions used for Cosmos Hub
145e84f
to
59d58f8
Compare
Keyrings store keys by name and hexbytes representation of address. This turns keyring internal storage more chain-agnostic and types.Config independent. Obsolete Keybase internal state representation is not affected.
Keyrings store keys by name and hexbytes representation of address. This turns keyring internal storage more chain-agnostic and types.Config independent. Obsolete Keybase internal state representation is not affected.
ae0c933
to
7a74fe4
Compare
@alexanderbez comments have been incorporated, please review again when you spare a minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Keyrings store keys by name and hexbytes representation
of address. This turns keyring internal storage more
chain-agnostic and types.Config independent.
Obsolete Keybase internal state representation is not affected.
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)