feat(client/v2): support gov proposals#18461
Conversation
WalkthroughWalkthroughThe updates across various files introduce a new feature for governance proposals, allowing users to submit proposals to update module parameters via the CLI. This includes new commands, flags, and validation functions to support the feature. Breaking changes are made by removing address codecs from certain options, and the context now handles address codecs. The Changes
TipsChat with CodeRabbit Bot (
|
github-actions
Bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
| Short: "Submit a proposal to update auth module params. Note: the entire params must be provided.", | ||
| Example: fmt.Sprintf(`%s tx auth update-params-proposal '{ "max_memo_characters": 0, "tx_sig_limit": 0, "tx_size_cost_per_byte": 0, "sig_verify_cost_ed25519": 0, "sig_verify_cost_secp256k1": 0 }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, |
There was a problem hiding this comment.
this is optional, if the authority is set to an multsig how would this change the flow of logic here?
There was a problem hiding this comment.
is there a flag allowing users to set this to false?
There was a problem hiding this comment.
We can add a flag and skip the gov proposal. I initially didn't do it, as it already didn't existed, but we can easily support this yes.
There was a problem hiding this comment.
Majority of users will have their module authority be gov so that's why it's done like that.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/v2/README.md (2 hunks)
- client/v2/autocli/msg.go (5 hunks)
- client/v2/internal/flags/flags.go (1 hunks)
Files skipped from review due to trivial changes (2)
- client/v2/README.md
- client/v2/internal/flags/flags.go
Additional comments: 5
client/v2/autocli/msg.go (5)
- 120-126: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [120-130]
The context is being set on the command, but it's not clear if this context is being used properly downstream. Ensure that the context set here is being utilized in all subsequent calls that require a context, especially since it's being overwritten with a new value containing the
client.ClientContextKey.
152-165: The code is using a custom address codec to convert the signer's address from bytes to a string. This is a critical operation and should be handled with care. Ensure that the address codec is thoroughly tested, especially for edge cases and invalid inputs.
187-189: The
proto.Mergefunction is used to merge the input message into a new dynamic message. This is necessary because the SDK only supports proto v1 messages, while AutoCLI uses proto v2 messages. Ensure that this conversion does not lose any information or introduce any inconsistencies.203-210: The command's usage is silenced, which is a common practice for subcommands to avoid cluttering the output. However, ensure that this does not hinder the user experience, especially when it comes to understanding how to use the command or troubleshooting errors.
215-252: The
handleGovProposalfunction is setting the authority field of the message to the gov module address and creating a gov proposal. This is a significant change in behavior and should be carefully reviewed to ensure that it aligns with the intended governance proposal flow. Additionally, the error messages should be clear and informative to aid in debugging.
odeke-em
left a comment
odeke-em
left a comment
There was a problem hiding this comment.
Thanks Julien, just some minor drive-by comments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/prompt_validation.go (1 hunks)
- client/v2/autocli/msg.go (5 hunks)
- client/v2/internal/prompt/validation.go (1 hunks)
Additional comments: 6
client/v2/internal/prompt/validation.go (1)
- 1-36: The new
promptpackage and its functions for input validation are well-defined and follow good coding practices. The use offmt.Errorfto wrap underlying errors with additional context is particularly good for debugging and user feedback. Ensure that these functions are being used wherever user input is accepted in the CLI to maintain consistency.client/v2/autocli/msg.go (5)
123-125: The context is being set on the command, but it's not clear if this context is being used properly downstream. Ensure that the context set here is being propagated and utilized in all subsequent calls that require a context.
152-165: The code is checking if the address field is empty and then setting it to the signer's address. This is a good use of default values, but ensure that this behavior is documented and expected by the users of the function. If the address field is supposed to be set by the caller, this might override their input unexpectedly.
187-189: The
proto.Mergefunction is used to merge the input message into a new dynamic message. This is necessary because the SDK only supports proto v1 messages, while AutoCLI uses proto v2 messages. Ensure that this conversion does not lead to any data loss or transformation issues.203-210: The command is being configured to not show usage on error if it's a subcommand. This is a common pattern for CLI tools to avoid cluttering the output with usage text when the error is not related to command syntax. The addition of governance proposal flags is also a good practice to allow users to control the behavior of the command.
215-253: The
handleGovProposalfunction is setting the authority field to the governance module address and creating a governance proposal. This is a critical piece of functionality that should be thoroughly tested to ensure that it behaves correctly. The error handling and message setting seem appropriate, but it's important to ensure that the governance proposal is created with all the necessary information and that any errors are handled gracefully.
| return fmt.Errorf("invalid address: %w", err) | ||
| } | ||
|
|
||
| // ValidatePromptYesNo validates that the input is valid sdk.COins | ||
| // ValidatePromptCoins validates that the input contains valid sdk.Coins | ||
| func ValidatePromptCoins(input string) error { | ||
| if _, err := sdk.ParseCoinsNormalized(input); err != nil { | ||
| return fmt.Errorf("invalid coins: %w", err) |
There was a problem hiding this comment.
The change from sdk.COins to sdk.Coins corrects a typo in the function ValidatePromptCoins, ensuring that the function checks for valid sdk.Coins objects. This is a necessary fix for the function to work correctly.
There was a problem hiding this comment.
lol. Changing a comment does not change the function behavior :D
There was a problem hiding this comment.
@julienrbrt You're absolutely right, updating the comment itself doesn't alter the function's behavior. The correction was made to ensure that the comment accurately reflects what the function does, which is important for code readability and maintainability. If there's anything else that needs attention in the code or comments, please let me know!
(cherry picked from commit 9c3386f) # Conflicts: # client/v2/CHANGELOG.md # client/v2/autocli/msg.go # client/v2/go.mod # client/v2/go.sum # go.mod # go.sum # simapp/go.mod # tests/go.mod # tests/starship/tests/go.mod # x/accounts/go.mod # x/auth/go.mod # x/auth/go.sum # x/authz/go.mod # x/bank/autocli.go # x/bank/go.mod # x/bank/go.sum # x/circuit/go.mod # x/circuit/go.sum # x/distribution/go.mod # x/distribution/go.sum # x/evidence/go.mod # x/evidence/go.sum # x/feegrant/go.mod # x/feegrant/go.sum # x/gov/go.mod # x/group/go.mod # x/group/go.sum # x/mint/go.mod # x/mint/go.sum # x/nft/go.mod # x/nft/go.sum # x/params/go.mod # x/params/go.sum # x/protocolpool/go.mod # x/protocolpool/go.sum # x/slashing/go.mod # x/slashing/go.sum # x/staking/go.mod # x/staking/go.sum # x/upgrade/client/cli/tx.go # x/upgrade/go.mod # x/upgrade/go.sum
4 participants
Description
Closes: #18376
ref: #17222
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.mdmake lintandmake testReviewers 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 changeSummary by CodeRabbit
New Features
Enhancements
RpcCommandOptions.Bug Fixes
Documentation
Refactor
Style
Tests