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.

Sign up for GitHub

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

Jump to bottom

R4R: Refactor Validator Account Types/Bech32 Prefixing #2040

Merged
merged 41 commits into from
Aug 31, 2018
Merged

R4R: Refactor Validator Account Types/Bech32 Prefixing #2040

merged 41 commits into from
Aug 31, 2018

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Aug 15, 2018 •
edited
Loading

  • Introduce new Bech32 prefixes: cosmosconsaddr and cosmosconspub. These
    now represent a validator's Tendermint signing key and address.
    • Introduced new address type ConsAddress
  • Re-purposed Bech32 prefixes cosmosvaladdr and cosmosvalpub to be used
    exclusively to represent a validator's operator address and signing key
    respectively.
  • Changed Validator.operator type from sdk.AccAddress to sdk.ValAddress
  • Updated gaiacli keys show to include new --bech flag and respective REST endpoint to support custom Bech32 encoding (cons|acc|val)
  • Updated all usage of sdk.ValAddress and sdk.AccAddress accordingly along
    with auxiliary functions used.

I understand this is a relatively "big" PR, but a majority of the changes are simply type changes (sdk.AccAddres => sdk.ValAddress or vice versa), as such I think it'll help to focus on the following files:

  • types/account.go
  • client/keys/show.go
  • client/keys/utils.go
  • docs/validators/validator-faq.md
  • cmd/gaia/app/genesis.go
  • docs/spec/other/bech32.md
  • x/gov/tally.go

Hopefully this reduces confusion 🤞

closes: #1910

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
  • Updated cmd/gaia and examples/

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)

@alexanderbez alexanderbez changed the title WIP: Refactor Validator Account Types/Bech32 Prefixing R4R: Refactor Validator Account Types/Bech32 Prefixing Aug 23, 2018
@alexanderbez
Copy link
Contributor Author

r4r @cwgoes & @rigelrozanski

/cc @faboweb

@cwgoes
Copy link
Contributor

cwgoes commented Aug 23, 2018

Does this implement #2103 (or would anything need to be changed to implement the revised spec)?

@alexanderbez
Copy link
Contributor Author

@cwgoes this PR does not address or implement #2103 as that was done in silo. To incorporate into this PR, its simply a matter of updating the bech32 prefix in types/account.go

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Aug 23, 2018 •
edited
Loading

I actually think the signing info logic was never updated -- the signing info is done based on the validator's TM key pair. Tbh, I'm a bit confused how validator's are instantiated/created.

In genesis.go, it used to be validators were created with acc addr and pubkey (operator):
https://github.com/cosmos/cosmos-sdk/blob/develop/cmd/gaia/app/genesis.go#L195-L196

I changed this to now be a val addr and pubkey (operator):
https://github.com/cosmos/cosmos-sdk/blob/bez/1910-refactor-bech32-stake-prefixing/cmd/gaia/app/genesis.go#L195-L196

But signing info works based off of the TM signing key, which is now of type cons addr and pubkey. Where does the keypair actually come from? How does signing info work?
https://github.com/cosmos/cosmos-sdk/blob/develop/x/slashing/signing_info.go#L80-L85

Shouldn't GetValidatorSigningInfoKey take a sdk.ConsAddress and the validator be instantiated with MustGetConsPubKeyBech32(genTx.PubKey)? But the genTx.PubKey does not seem to be the TM pubkey, unless I'm mistaken. Or is signing-info wrong?

@rigelrozanski

EDIT: @cwgoes helped clarify things for me a bit. The validator should be instantiated with sdk.MustGetConsPubKeyBech32.

rigelrozanski
rigelrozanski reviewed Aug 24, 2018
View reviewed changes
client/keys/show.go Show resolved Hide resolved
types/account.go
s.Write([]byte(fmt.Sprintf("%p", ca)))
default:
s.Write([]byte(fmt.Sprintf("%X", []byte(ca))))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

small side point - it may be nice to reuse some of the basic logic for the different address types such as this function Format - we could create an address interface to implement some of this stuff. Probably for a later refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually had an Address interface implemented, but then later decided to remove it as I thought it was too much overhead for the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah "for a future pr"

@rigelrozanski
Copy link
Contributor

looks like need to merge develop but let's merge soon other than that

@alexanderbez
Copy link
Contributor Author

@rigelrozanski for sure. I'll handle these conflicts today.

@alexanderbez
Copy link
Contributor Author

@rigelrozanski this is good for another review. I also updated the Cosmos account bech32 prefix to reflect @jaekwon changes (cosmos and cosmospub).

@rigelrozanski rigelrozanski mentioned this pull request Aug 31, 2018
Merged
5 tasks
rigelrozanski
rigelrozanski approved these changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

nice work! Added a minor formatting commit

@rigelrozanski rigelrozanski merged commit 2d92803 into develop Aug 31, 2018
@rigelrozanski rigelrozanski deleted the bez/1910-refactor-bech32-stake-prefixing branch August 31, 2018 04:06
@alexanderbez
Copy link
Contributor Author

Thank you @rigelrozanski -- these are notable changes. How do you suggest we inform others?

@rigelrozanski
Copy link
Contributor

Good question - I'll mention it to @faboweb - up and coming!

@alexanderbez alexanderbez mentioned this pull request Sep 6, 2018
Merged
5 tasks
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
@glnro @MSalopek @sainoe
chore: v9 upgrade handler (cosmos#2040)
4882768 
* Upgrade to v9

* Update handler

* Fix linter

* Remove old or basic upgrades from codecov

* add execute rights to test scripts (cosmos#2041)

* Add e2e tests

* Update code smell

* Remove and refactor duplicated query exec

* Fix linter

* Update ICS dependency

* Update dependency

* Fix typo

Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com>

Co-authored-by: MSalopek <35486649+MSalopek@users.noreply.github.com>
Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faboweb faboweb faboweb left review comments

@rigelrozanski rigelrozanski rigelrozanski approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
C:x/staking T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Staking use sdk.AccAddress instead of sdk.ValAddress
4 participants
@alexanderbez @cwgoes @rigelrozanski @faboweb