-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat(autocli): add address validation #15449
Conversation
…cosmos/cosmos-sdk into JeancarloBarrios/add-address-validation
@@ -29,8 +50,12 @@ func (a addressValue) String() string { | |||
} | |||
|
|||
func (a *addressValue) Set(s string) error { | |||
_, err := types.GetFromBech32(s, a.addressPrefix) |
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.
We should add docs that client v2 only supports bech32 here
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.
Hey Marco sorry for the question, but where exactly do you envision this in the doc should I create a new category for the flags?
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.
utACK. 2 nits.
tools/hubl/go.mod
Outdated
@@ -119,3 +119,9 @@ require ( | |||
pgregory.net/rapid v0.5.5 // indirect | |||
sigs.k8s.io/yaml v1.3.0 // indirect | |||
) | |||
|
|||
|
|||
// Temporal for PR |
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.
Let's not forget to remove that in a follow-up of now using a pseudo version.
Otherwise hubl cannot be installed via go install
.
client/v2/autocli/flag/address.go
Outdated
panic(err) | ||
} | ||
reflectionClient := reflectionv2alpha1.NewReflectionServiceClient(conn) | ||
if ctx == nil { |
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 think we should overwrite a nil context. It may be hard to debug for 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.
What do you sugest I use instead to avoid possible error because I was getting a nil pointer from the ctx being nil?
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 suggest just passing it. The caller needs to ensure ctx isn't nil.
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.
Oh ok perfect, I got a panic from it while testing I will dig into the caller to check what is up
@JeancarloBarrios your pull request is missing a changelog! |
Description
Closes: #13287
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