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

Refactor usages of bech32 ValAddress signers #15689

Closed
Tracked by #15677
aaronc opened this issue Apr 4, 2023 · 2 comments · Fixed by #15709
Closed
Tracked by #15677

Refactor usages of bech32 ValAddress signers #15689

aaronc opened this issue Apr 4, 2023 · 2 comments · Fixed by #15709
Labels
needs-triage Issue that needs to be triaged

Comments

@aaronc
Copy link
Member

aaronc commented Apr 4, 2023

Problem Definition

While working on #11275, I discovered that some Msg's use ValAddress bech32 encoding rather than AccAddress bech32 for Msg signers, (i.e. cosmosvaloper12345abcef vs cosmos12345abcef), specifically:

  • x/staking
    • MsgCreateValidator
    • MsgEditValidator
  • x/slashing
    • MsgUnjail
  • x/distribution
    • MsgWithdrawValidatorCommission

Proposal

I can think of a few approaches for dealing with this:

  1. Refactor these Msg's to use AccAddress bech32
  2. Support either ValAddress or AccAddress in address.Codec and decode both to address bytes
  3. Add some special annotations to indicate which signers use ValAddress encoding. For instance we could write (cosmos_proto.scalar) = "cosmos.ValidatorAddressString" on these fields to indicate the encoding

I'm personally leaning towards approach 3 because it's the most explicit and isn't protocol breaking. I actually see many messages which use validator addresses annotated with cosmos.AddressString as their cosmos_proto.scalar encoding and this is just incorrect. At a minimum we should fix these annotations. If we do fix them, then it's a matter of refactoring our existing GetSigners code to handle these cases. This is annoying for sure, but better long term as we can also refactor this code to support bytes signers in the future as some people discussed in #13140.

@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Apr 4, 2023
@aaronc aaronc mentioned this issue Apr 4, 2023
8 tasks
@tac0turtle
Copy link
Member

I would rather go with 1. Valper should be an internal accounting thing. Signers should all be the same format imo.

@aaronc
Copy link
Member Author

aaronc commented Apr 4, 2023

What do you think about doing this in a way that will allow bytes addresses in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Issue that needs to be triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants