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

feat: cli for sending Wallet{Spend}Action transactions #5144

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Conversation

JimLarson
Copy link
Contributor

closes: #5141

Description

Commands for sending WalletAction and WalletSpendAction transactions.

Security Considerations

Ensures address and JSON are valid but performs no other checks.

Documentation Considerations

Needs long description?

Testing Considerations

Needs some end-to-end testing, currently difficult to automate.

@JimLarson JimLarson added enhancement New feature or request cosmic-swingset package: cosmic-swingset wallet labels Apr 19, 2022
@JimLarson JimLarson self-assigned this Apr 19, 2022
@JimLarson JimLarson marked this pull request as ready for review April 19, 2022 00:44
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Thanks for doing this based on a napkin sketch. 😄

The only change I'd suggest is to make --spend more like --allow-spend or --authorize-spend or something.

@@ -18,6 +18,10 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

const (
FlagSpend = "spend"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FlagSpend = "spend"
FlagAllowSpend = "allow-spend"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

},
}

cmd.Flags().Bool(FlagSpend, false, "Send a WalletSpendAction transaction if true")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().Bool(FlagSpend, false, "Send a WalletSpendAction transaction if true")
cmd.Flags().Bool(FlagAllowSpend, false, "Allow the WalletAction to spend assets if specified")

My wording's not so great. Alternatives in this direction would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Wording seems fine except trimmed the redundant "if true" at the end.

@JimLarson JimLarson added the automerge:no-update (expert!) Automatically merge without updates label Apr 21, 2022
@mergify mergify bot merged commit 4dcb676 into master Apr 21, 2022
@mergify mergify bot deleted the 5141-wallet-cli branch April 21, 2022 00:28
dckc added a commit that referenced this pull request Jul 18, 2022
dckc added a commit that referenced this pull request Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates cosmic-swingset package: cosmic-swingset enhancement New feature or request wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: create cli for Msg.Wallet(Spend)Action from x/swingset
2 participants