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

refactor: allow getting address prefix from address codec #15913

Closed
wants to merge 10 commits into from
Closed

refactor: allow getting address prefix from address codec #15913

wants to merge 10 commits into from

Conversation

julienrbrt
Copy link
Member

Description

Needed by #15906 so we can avoid passing config.
This PR does not need a changelog.

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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt julienrbrt requested a review from a team as a code owner April 23, 2023 15:18
@github-actions
Copy link
Contributor

@julienrbrt your pull request is missing a changelog!

@julienrbrt
Copy link
Member Author

@julienrbrt your pull request is missing a changelog!

We do not need one here.

tac0turtle
tac0turtle reviewed Apr 23, 2023
View reviewed changes
core/address/codec.go Outdated
@@ -2,6 +2,8 @@ package address

// Codec defines an interface to convert addresses from and to string/bytes.
type Codec interface {
// GetBech32Prefix returns the bech32 prefix
GetBech32Prefix() string
Copy link
Member

Choose a reason for hiding this comment

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

can we avoid adding bech32 specific to this interface, this interface is meant to be agnostic

Copy link
Member Author

Choose a reason for hiding this comment

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

If I rename it to GetAddressPrefix and that it returns "" when there is none, how does it sound for you?

@julienrbrt julienrbrt changed the title feat: allow to get bech32 prefix from address codec ref: allow getting address prefix from address codec Apr 23, 2023
@julienrbrt julienrbrt changed the title ref: allow getting address prefix from address codec refactor: allow getting address prefix from address codec Apr 23, 2023
@github-actions github-actions bot added C:CLI C:Confix Issues and PR related to Confix C:Cosmovisor Issues and PR related to Cosmovisor C:Hubl Tool: Hubl C:Rosetta Issues and PR related to Rosetta C:x/circuit labels Apr 23, 2023
@julienrbrt julienrbrt mentioned this pull request Apr 23, 2023
Merged
25 tasks
aaronc
aaronc reviewed Apr 23, 2023
View reviewed changes
core/address/codec.go
@@ -2,8 +2,10 @@ package address

// Codec defines an interface to convert addresses from and to string/bytes.
type Codec interface {
// StringToBytes decodes text to bytes
// GetAddressPrefix returns the address prefix (if any).
GetAddressPrefix() string
Copy link
Member

Choose a reason for hiding this comment

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

We can't assume the codec is always bech32. I'd rather find another way to deal with this

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes (#15913 (comment)), so do you find this naming not fitting if the address has no prefix?

I do not know if you've seen the autocli PR but this come handy there.

Copy link
Member Author

@julienrbrt julienrbrt Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is that fine for autocli to depend directly on auth then?
Personally, I don't like it, but that would solve the issue I have there.
Another way I thought would be autocli to depend on auth config, but this was still less clean than with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

since autocli is built off of grpc why not use

cosmos-sdk/proto/cosmos/auth/v1beta1/query.proto

Line 64 in be081b1

rpc Bech32Prefix(Bech32PrefixRequest) returns (Bech32PrefixResponse) {
. This way autocli/hubl will only work with bech32 addresses for now, but there can be a fallback or a way for a user to load their own

Copy link
Member Author

@julienrbrt julienrbrt Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is almost what it already uses there, and it works fine for hubl, which will build the command after having queried an endpoint. However, for normal commands, like simd, gaiad, etc.. When we enrich the root command, we have no endpoint to query. The information is however available, and imho this was the cleanest way to get it.

We could however:

  • Add an argument in EnhanceRootCommand for the bech32 prefix (easiest but most ugly because for hubl we'll need to pass an empty string)
  • Inject address codec and add api to get address prefix (this was my preferred solution and what is done in this PR)
  • Inject auth configuration and use config.Bech32Prefix for the address prefix, which is fine for app wiring app, but it makes it weird when you have to construct AppOptions manually
  • Inject auth, then we have a hard dependency on auth for autocli

I am open to other suggestions however.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just change the flag.Builder argument to take address.Codec instead of bech32 prefix. I think that would resolve this without needing to change address.Codec. Also if we do need the actual prefix let's use an extension interface instead of changing address.Codec

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think of the problem this way. This would indeed work too, and only complexify the flag builder
I will close this then and re-open it shortly with only the clean-ups that were done here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't assume the codec is always bech32. I'd rather find another way to deal with this

While we can't assume this, this is pretty baked into the SDK and I personally can't see this changing in the future.

@julienrbrt julienrbrt closed this Apr 24, 2023
@julienrbrt julienrbrt deleted the julien/codec-add-get-prefix branch April 24, 2023 14:10
@julienrbrt julienrbrt mentioned this pull request Apr 24, 2023
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@aaronc aaronc aaronc left review comments

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

Assignees
No one assigned
Labels
C:CLI C:Confix Issues and PR related to Confix C:Cosmovisor Issues and PR related to Cosmovisor C:Hubl Tool: Hubl C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/authz C:x/circuit C:x/distribution distribution module related C:x/feegrant C:x/gov C:x/group C:x/nft C:x/staking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@julienrbrt @aaronc @alexanderbez @tac0turtle