Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add wasm command to support v1 gov proposals #1326

Merged
merged 5 commits into from
Apr 21, 2023
Merged

Conversation

pinosu
Copy link
Contributor

@pinosu pinosu commented Apr 10, 2023

Resolves #1301

Implemented option c) A new wasm command could provide a similar interface as v1beta1 proposals but with a v1 message send

@pinosu pinosu marked this pull request as ready for review April 14, 2023 13:58
@pinosu pinosu requested a review from alpe as a code owner April 14, 2023 13:58
Copy link
Contributor

@alpe alpe 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!
When trying to test a submission, I noticed that the new command does not support --keyring-backend.

Error: unknown flag: --keyring-backend

return fmt.Errorf("authority: %s", err)
}

if len(authority) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

here and others: it would be nice if we would fetch the default authority like the wizard does. I would not expect the users to know the address.

@@ -57,7 +57,7 @@ func ValidateSalt(salt []byte) error {
// ValidateVerificationInfo ensure source, builder and checksum constraints
func ValidateVerificationInfo(source, builder string, codeHash []byte) error {
// if any set require others to be set
if len(source) != 0 || len(builder) != 0 || codeHash != nil {
if len(source) != 0 || len(builder) != 0 || len(codeHash) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Please see my comment on the static address and #1356 .
Other than this, the PR looks very good now. 👍

}

if len(authority) == 0 {
authority = sdk.AccAddress(address.Module("gov")).String()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting that the address would be queried but I guess it is alright to set it for the client already.
While the address can be changed in the keeper(s) on init, it can not be modified on the CLI. I have opened #1356 to have a public var instead. Please 👁️

@alpe alpe merged commit 52996db into main Apr 21, 2023
@alpe alpe deleted the 1301-support_proposals branch April 21, 2023 16:28
@alpe alpe mentioned this pull request Apr 21, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proper v1 gov support for wasm msg types
2 participants