-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: simplify ADR-028 and address.Module #14017
Conversation
types/address/hash.go
Outdated
func Module(moduleName string, key []byte) []byte { | ||
// is constructed from a module name and a sequence of derivation keys (at least one | ||
// derivation key must be provided). | ||
func Module(moduleName string, derivationKey []byte, derivationKeys ...[]byte) []byte { |
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 was thinking to "merge" derivationKey into derivationKeys, but then we would need to panic if someone will provide an empty list.
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 think as is is great, requires at least 1 derivationKey implicitly which is communicated to the caller.
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 don't see any reason to require at least one derivation key for modules. This is inconsistent with what we've discussed for ADR 033. There is always the root module address with no derivation keys.
func Module(moduleName string, derivationKey []byte, derivationKeys ...[]byte) []byte { | |
func Module(moduleName string, derivationKeys ...[]byte) []byte { |
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.
wouldn't this conflict with the existing authtypes.NewModuleAddress
?
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.
No. That uses 20 byte addresses
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.
My preference is to keep backwards compatibility of existing root module accounts.
func Module(moduleName string, derivationKeys ...[]byte) []byte {
if len(derivationKeys) == 0 { return authtypes.NewModuleAddress() }
else { /* do like current code with successive derivations */ }
}
so it's:
- 20 byte address for root module accounts
- 32 byte addresses for sub/derived module accounts
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.
So we need to make decision.
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.
up
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.
Up again, I'm going to update this PR by implementing #14017 (comment) (which is what @AmauryM seconded: #14017 (comment))
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.
SGTM
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.
Lgtm!
Co-authored-by: Julien Robert <julien@rbrt.fr>
Adding 0.47 backport. |
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.
Thank you for this change @robert-zaremba, just some suggestions, please take a look. Thank you Julien for the co-review.
types/address/hash.go
Outdated
func Module(moduleName string, key []byte) []byte { | ||
// is constructed from a module name and a sequence of derivation keys (at least one | ||
// derivation key must be provided). | ||
func Module(moduleName string, derivationKey []byte, derivationKeys ...[]byte) []byte { |
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 think as is is great, requires at least 1 derivationKey implicitly which is communicated to the caller.
types/address/hash.go
Outdated
func Module(moduleName string, key []byte) []byte { | ||
// is constructed from a module name and a sequence of derivation keys (at least one | ||
// derivation key must be provided). | ||
func Module(moduleName string, derivationKey []byte, derivationKeys ...[]byte) []byte { |
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 don't see any reason to require at least one derivation key for modules. This is inconsistent with what we've discussed for ADR 033. There is always the root module address with no derivation keys.
func Module(moduleName string, derivationKey []byte, derivationKeys ...[]byte) []byte { | |
func Module(moduleName string, derivationKeys ...[]byte) []byte { |
@AmauryM would be great to get your review of this too. |
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Personally, I would prefer to have only the "full" ADR-28 addresses for root module accounts. But simplification and compatibility is more important here. |
For security purposes? All root accounts should be initialized on startup |
Mostly for having one general solution (with clear domains - what ADR-028 is mostly about). I think security is OK here, because we have that module name hash. The concern is if someone is doing something weird and creating a module name which is a hash and could collide with an existing public key hash. Or generates multiple module accounts rather than deriving. |
[Cosmos SDK] Kudos, SonarCloud Quality Gate passed! |
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.
Happy to give my pre-approval on this now but I think we should sort out what to do for the root module hash. Maybe on the next community call as you mentioned @robert-zaremba
Co-authored-by: Aaron Craelius <aaron@regen.network>
I believe it closes #13782 too right? |
Can we add a changelog? Then, given the discussion from the community call, we can add automerge. |
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.
It seems a little late to see this pr since it would be merged soon.
After reading through the pr, I think this is mainly talking about the derivation of module type accounts. But I got some questions about the change and would appreciate it if anyone could explain to me:
- Could this possibly bring any consensus-breaking change?
- what are the main benefits of this new pattern of module account derivation change
|
||
```go | ||
func Module(moduleName string, key []byte) []byte{ |
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.
so currently we are not using this function to generate root module account, instead we are using authtypes.NewModuleAddress(modulenName)
, right?
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.
Exactly, now authtypes.NewModuleAddress(modulenName)
falls back to address.Module
now, but, the behavior stays the same as authtypes.NewModuleAddress(modulenName)
: https://github.com/cosmos/cosmos-sdk/pull/14017/files#diff-6cfded83a08be06034f746e83ffa6bbd6c35430cd8ac05015e6bd942c7da1030R73-R76
Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit fd3bac0) # Conflicts: # CHANGELOG.md
Description
Closes: #13910
Closes: #13782
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change