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
julienrbrt
added
the
backport/v0.50.x
![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: 3
Configuration used: CodeRabbit UI
Files selected for processing (11)
- client/v2/internal/prompt/validation.go (1 hunks)
- client/v2/internal/prompt/validation_test.go (1 hunks)
- x/auth/autocli.go (1 hunks)
- x/bank/autocli.go (2 hunks)
- x/consensus/autocli.go (2 hunks)
- x/crisis/autocli.go (2 hunks)
- x/distribution/autocli.go (4 hunks)
- x/gov/autocli.go (1 hunks)
- x/mint/autocli.go (2 hunks)
- x/slashing/autocli.go (1 hunks)
- x/staking/autocli.go (1 hunks)
Files skipped from review due to trivial changes (1)
- client/v2/internal/prompt/validation_test.go
Additional comments: 23
x/consensus/autocli.go (2)
1-11: The import of "fmt" and "github.com/cosmos/cosmos-sdk/version" is necessary for the new functionality. Ensure that these imports are used elsewhere in the file to avoid importing them unnecessarily.
31-43: The addition of the
GovProposalfield and the new fieldsUse,Short,Example, andPositionalArgsare in line with the pull request's goal to enable governance proposals through the AutoCLI. Ensure that theExamplefield's usage ofversion.AppNameis consistent with how the application's name should be displayed and that thePositionalArgscorrectly map to the fields expected by theUpdateParamsRPC method.x/slashing/autocli.go (2)
46-54: The new CLI command "update-params-proposal" is correctly flagged as a governance proposal and includes an example usage. However, ensure that the placeholder
{ params }in theExamplefield is intended to be replaced by users with actual JSON-encoded parameters. If not, consider providing a more concrete example.54-54: The
GovProposalfield is set to true, indicating that this command is for submitting a governance proposal. This is a key update for the feature being implemented. Ensure that the corresponding RPC method "UpdateParams" is properly implemented to handle governance proposal submissions.x/auth/autocli.go (1)
- 80-88: The addition of the
UpdateParamsRPC method withGovProposalset to true is a significant change. It allows users to submit governance proposals to update auth module parameters via the CLI. Ensure that theparamsargument is properly validated and that the JSON encoding is handled securely to prevent injection attacks. TheExamplefield uses string interpolation to include the application name, which is a good practice for clarity and maintainability.x/bank/autocli.go (3)
3-10: Imports are correctly added for the new functionality.
113-130: The new fields
Use,Short,Example,PositionalArgs, andGovProposalare correctly added to theUpdateParamsandSetSendEnabledRPC methods to support the new governance proposal feature. The use offmt.Sprintfto generate the example commands is a good practice as it ensures consistency with the application name.126-127: The introduction of
FlagOptionswithuse_default_forprovides additional functionality for the command, allowing users to revert to default settings for a given denom. This is a thoughtful addition to the feature set.Overall, the changes in this file align with the pull request's goal of enabling governance proposals through the AutoCLI and follow best practices for CLI command configuration.
x/crisis/autocli.go (3)
1-9: The import statements have been updated to include necessary packages for the new functionality. Ensure that these packages are used in the code to avoid importing unused dependencies.
24-35: The new
UpdateParamsRPC method has been added with appropriate fields such asUse,Short,Example,PositionalArgs, andGovProposal. Ensure that theExamplefield is correctly formatted and that theparamsplaceholder in the example is intended to be replaced by actual parameters when used.30-30: The use of
fmt.Sprintfto construct the example command is correct, but ensure that theversion.AppNamevariable provides the correct application name for the command example.client/v2/internal/prompt/validation.go (2)
1-36: The new
promptpackage and its functions for input validation are a good addition for ensuring the integrity of user input. It's important to ensure that these functions are integrated and used wherever user input is required for the new AutoCLI governance proposal feature.30-35: The function
ValidatePromptCoinsusessdk.ParseCoinsNormalized, which is appropriate for the validation of coin formats. Ensure that thesdkpackage's version used here supports theParseCoinsNormalizedfunction as expected.x/distribution/autocli.go (6)
4-10: Renaming the package from
distirbuitonv1beta1todistributionv1beta1corrects a typo and improves readability.13-19: The
AutoCLIOptionsfunction has been updated to reflect the new RPC methods and their descriptions. Ensure that all new RPC methods are correctly implemented and that the corresponding server-side implementations are in place.79-85: The transaction (
Tx) service commands have been updated with new RPC methods and descriptions. It is important to verify that these new methods are properly registered and handled in the transaction processing logic.130-135: The new
UpdateParamsRPC method for submitting governance proposals has been added. Ensure that the JSON encoding for theparamsargument is properly documented and validated, and that the governance proposal logic correctly processes these proposals.138-140: The
CommunityPoolSpendRPC method has been marked as skipped due to deprecation. Confirm that this method is indeed deprecated and that any functionality it provided is covered by the newprotocolpoolmodule or equivalent.142-142: The
EnhanceCustomCommandflag has been set totrue. Ensure that the custom command enhancement logic is in place and that it does not introduce any unexpected behavior.x/mint/autocli.go (4)
1-9: The import statements are correctly organized and formatted according to Go conventions, with standard library imports followed by third-party packages.
34-45: The new RPC command option for submitting a proposal to update mint module params is well-defined with clear usage instructions (
Use), a concise description (Short), an example usingfmt.Sprintffor dynamic application name insertion (Example), and theGovProposalfield set to true to indicate this is a governance proposal. ThePositionalArgsfield is also correctly populated with a descriptor for the expected argument.38-40: Ensure that the
paramsargument in theExamplefield is correctly formatted and represents a valid JSON object. The placeholder{ params }should be replaced with an actual example of the JSON-encoded parameters to avoid confusion.40-40: Using
fmt.Sprintfto construct the example command is a good practice as it dynamically includes the application name. However, ensure thatversion.AppNameis correctly set and that the resulting command is tested for correctness.
x/staking/autocli.go
Outdated
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "validator_address"}, {ProtoField: "amount"}, {ProtoField: "creation_height"}}, | ||
| }, | ||
| { | ||
| RpcMethod: "UpdateParams", | ||
| Skip: true, // skipped because authority gated | ||
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update staking module params. Note: params are JSON encoded", | ||
| Example: fmt.Sprintf(`%s tx staking update-params-proposal '{ params }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, |
There was a problem hiding this comment.
The UpdateParams command has been updated to indicate it is a governance proposal. However, the example provided in line 180 is not a valid JSON structure. It would be beneficial for users to see a real-world example of the params structure in JSON format. Consider updating the example to reflect a complete and valid JSON object.
- fmt.Sprintf(`%s tx staking update-params-proposal '{ params }'`, version.AppName),
+ fmt.Sprintf(`%s tx staking update-params-proposal '{"param1":"value1", "param2":"value2"}'`, version.AppName),Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "validator_address"}, {ProtoField: "amount"}, {ProtoField: "creation_height"}}, | |
| }, | |
| { | |
| RpcMethod: "UpdateParams", | |
| Skip: true, // skipped because authority gated | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update staking module params. Note: params are JSON encoded", | |
| Example: fmt.Sprintf(`%s tx staking update-params-proposal '{ params }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "validator_address"}, {ProtoField: "amount"}, {ProtoField: "creation_height"}}, | |
| }, | |
| { | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update staking module params. Note: params are JSON encoded", | |
| Example: fmt.Sprintf(`%s tx staking update-params-proposal '{"param1":"value1", "param2":"value2"}'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (11)
- client/prompt_validation.go (1 hunks)
- client/v2/internal/prompt/validation.go (1 hunks)
- x/auth/autocli.go (1 hunks)
- x/bank/autocli.go (2 hunks)
- x/consensus/autocli.go (2 hunks)
- x/crisis/autocli.go (2 hunks)
- x/distribution/autocli.go (4 hunks)
- x/gov/autocli.go (1 hunks)
- x/mint/autocli.go (2 hunks)
- x/slashing/autocli.go (1 hunks)
- x/staking/autocli.go (1 hunks)
Files skipped from review due to trivial changes (1)
- client/prompt_validation.go
Additional comments: 16
x/crisis/autocli.go (3)
1-9: The import statements are correctly added for the new dependencies. Ensure that these packages are used in the code to avoid importing unused packages.
27-32: The new
UpdateParamsRPC method is added to thecrisismodule's AutoCLI options. The usage string, short description, and example are provided, which is good for clarity and user guidance. However, ensure that theparamspositional argument is correctly parsed and validated in the command implementation to prevent any issues with malformed input.30-30: The use of
fmt.Sprintfto construct the example command is a good practice as it ensures that the example is always up-to-date with the current application name. However, verify thatversion.AppNameis correctly set and that the example command is correctly formatted and tested.x/staking/autocli.go (1)
- 174-186: The addition of the
UpdateParamscommand is a significant change that allows users to submit governance proposals to update staking module parameters. The usage example provided is helpful, but it should be noted that theparamsargument expects a JSON string. It might be beneficial to include a note about the format required for theparamsargument to prevent user errors. Additionally, ensure that theGovProposalfield is properly handled in the command's implementation to differentiate it from regular transactions.x/auth/autocli.go (1)
- 80-88: The addition of the
UpdateParamsRPC method to theauthmodule'sAutoCLIOptionsis a significant change. It allows users to submit governance proposals to update module parameters via the CLI. TheExamplefield provides a clear usage example, which is helpful for users to understand the expected input format. However, the comment on line 85 indicates that the entire params must be provided. It would be beneficial to clarify whether partial updates are supported and if not, to ensure that the CLI provides a clear error message when partial params are provided.Additionally, the
PositionalArgsdescriptor on line 87 specifies that theparamsfield is expected, but it's important to ensure that the CLI correctly parses and validates the JSON input to prevent any issues with malformed proposals.Lastly, setting
GovProposaltotrueon line 88 is a key change that should be highlighted in the documentation to inform users about the governance capabilities of this command.x/bank/autocli.go (3)
3-10: The import statements are correctly organized and necessary for the new functionality. The addition of "fmt" and "github.com/cosmos/cosmos-sdk/version" suggests that formatting and version information will be used in the new CLI commands.
13-13: The comment above the
AutoCLIOptionsfunction provides a clear explanation of its purpose, which is to implement theautocli.HasAutoCLIConfiginterface. This is a good practice for code readability and maintainability.110-130: The new CLI commands for governance proposals (
update-params-proposalandset-send-enabled-proposal) are well-defined with usage examples and positional arguments. The use offmt.Sprintfto include the application name in the example is a good practice for maintainability and ensures consistency across different deployments of the application.However, ensure that the
version.AppNamevariable is correctly set and accessible in this context. If this variable is not set or imported correctly, it could lead to runtime errors when trying to use these CLI commands.Additionally, the
FlagOptionsforuse-default-forin theset-send-enabled-proposalcommand is a good addition for usability, allowing users to revert to default settings for a given denom. Ensure that the corresponding logic to handle this flag is implemented and tested.Lastly, the
GovProposalfield is set totruefor both commands, which correctly flags them as governance proposals. This is a key part of the new feature and should be tested thoroughly to ensure that the proposals are created and submitted correctly.x/distribution/autocli.go (5)
4-10: The package alias
distributionv1beta1has been corrected to match the actual package name, which is a good practice for readability and to avoid confusion.127-142: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [13-140]
The
AutoCLIOptionsfunction has been updated to include new fields such asShort,Example,PositionalArgs, andGovProposal. These additions are likely to improve the user experience by providing more descriptive help messages, usage examples, and the ability to handle governance proposals directly from the CLI. However, ensure that all the examples provided in theExamplefields are tested and work as expected.
130-135: The
UpdateParamsRPC method has been added with aGovProposalfield set totrue, indicating that this command is intended for governance proposals. It is important to ensure that theparamspositional argument is correctly parsed and validated since the comment indicates that the entire params object must be provided. This could be a complex input for users to format correctly, so consider providing additional validation or parsing support if necessary.138-140: The
CommunityPoolSpendRPC method is marked withSkip: true, indicating it has been deprecated in favor ofprotocolpool. It is crucial to communicate these deprecations clearly in theREADME.mdandCHANGELOG.mdto inform users of the changes and guide them towards the new commands.142-142: The
EnhanceCustomCommandfield is set totrue, which suggests that there might be additional custom logic attached to the command processing. Verify that any custom command enhancements are well-documented and tested to ensure they do not introduce unexpected behavior.x/mint/autocli.go (3)
3-9: The import statements are correctly organized and necessary for the new functionality. The
fmtpackage is used for formatting strings, and thegithub.aaakk.us.kg/cosmos/cosmos-sdk/versionpackage is likely used to retrieve the application name for CLI usage examples.40-41: The use of
version.AppNameto dynamically generate the example command is a good practice, ensuring that the example remains accurate even if the application name changes. However, ensure thatversion.AppNameis always set to a meaningful value to prevent confusing or incorrect examples.42-43: The
PositionalArgsfield is correctly used to specify that theparamsfield is expected as a positional argument in the CLI command. TheGovProposalflag is set totrue, indicating that this command is for submitting governance proposals, which aligns with the pull request's goal to introduce governance proposal functionality.
x/slashing/autocli.go
Outdated
| Example: fmt.Sprintf("%s tx slashing unjail --from [validator]", version.AppName), | ||
| }, | ||
| { | ||
| RpcMethod: "UpdateParams", | ||
| Skip: true, // skipped because authority gated | ||
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update slashing module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query slashing params --output json`", version.AppName), | ||
| Example: fmt.Sprintf(`%s tx slashing update-params-proposal '{ "signed_blocks_window": "100", ... }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, | ||
| }, |
There was a problem hiding this comment.
The new UpdateParams command is correctly added with the GovProposal field set to true, indicating that it is a governance proposal. The usage, short description, long description, and example are provided, which is good for clarity and user guidance. However, ensure that the long description correctly references the slashing module instead of the gov module, as this might be a copy-paste error.
- Submit a proposal to update gov module params. Note: the entire params must be provided.
+ Submit a proposal to update slashing module params. Note: the entire params must be provided.Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| Example: fmt.Sprintf("%s tx slashing unjail --from [validator]", version.AppName), | |
| }, | |
| { | |
| RpcMethod: "UpdateParams", | |
| Skip: true, // skipped because authority gated | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update slashing module params. Note: the entire params must be provided.", | |
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query slashing params --output json`", version.AppName), | |
| Example: fmt.Sprintf(`%s tx slashing update-params-proposal '{ "signed_blocks_window": "100", ... }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, | |
| Example: fmt.Sprintf("%s tx slashing unjail --from [validator]", version.AppName), | |
| }, | |
| { | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update slashing module params. Note: the entire params must be provided.", | |
| Long: fmt.Sprintf("Submit a proposal to update slashing module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query slashing params --output json`", version.AppName), | |
| Example: fmt.Sprintf(`%s tx slashing update-params-proposal '{ "signed_blocks_window": "100", ... }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, |
| package prompt | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/url" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| ) | ||
|
|
||
| // ValidatePromptNotEmpty validates that the input is not empty. | ||
| func ValidatePromptNotEmpty(input string) error { | ||
| if input == "" { | ||
| return fmt.Errorf("input cannot be empty") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // ValidatePromptURL validates that the input is a valid URL. | ||
| func ValidatePromptURL(input string) error { | ||
| _, err := url.ParseRequestURI(input) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // ValidatePromptCoins validates that the input is a valid sdk.Coins | ||
| func ValidatePromptCoins(input string) error { | ||
| if _, err := sdk.ParseCoinsNormalized(input); err != nil { | ||
| return fmt.Errorf("invalid coins: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The new validation functions introduced in the prompt package are well-defined and follow Go's idiomatic error handling. Each function provides a clear error message specific to the validation failure, which is good for debugging and user feedback. The use of %w in fmt.Errorf allows for error wrapping, which is a best practice for error handling in Go as it enables errors to be unwrapped and inspected if needed.
However, it's important to ensure that the error messages are consistent and user-friendly since they might be displayed to the end-user. For example, the message "invalid coins" in ValidatePromptCoins could be more descriptive, explaining what makes the coins invalid.
Additionally, consider adding unit tests for these functions to ensure they handle edge cases correctly, such as inputs with leading or trailing spaces, or unusual characters in URLs.
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update gov module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query gov params --output json`", version.AppName), | ||
| Example: fmt.Sprintf(`%s tx gov update-params-proposal '{ params }'`, version.AppName), |
There was a problem hiding this comment.
The example for the "update-params-proposal" command should be updated to provide a more realistic JSON structure for the params argument. This will help users understand the expected format without additional research. It seems this was previously noted and should be addressed.
- Example: fmt.Sprintf(`%s tx gov update-params-proposal '{ params }'`, version.AppName),
+ Example: fmt.Sprintf(`%s tx gov update-params-proposal '{"param_type": "value", ...}'`, version.AppName),Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| Example: fmt.Sprintf(`%s tx gov update-params-proposal '{ params }'`, version.AppName), | |
| Example: fmt.Sprintf(`%s tx gov update-params-proposal '{"param_type": "value", ...}'`, version.AppName), |
x/staking/autocli.go
Outdated
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update staking module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query staking params --output json`", version.AppName), |
There was a problem hiding this comment.
There is a minor inconsistency in the Long description of the UpdateParams command. It mentions "gov module params" instead of "staking module params". This should be corrected to match the context of the command.
- Submit a proposal to update gov module params. Note: the entire params must be provided.
+ Submit a proposal to update staking module params. Note: the entire params must be provided.Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query staking params --output json`", version.AppName), | |
| Long: fmt.Sprintf("Submit a proposal to update staking module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query staking params --output json`", version.AppName), |
x/mint/autocli.go
Outdated
| Service: mintv1beta1.Msg_ServiceDesc.ServiceName, | ||
| RpcCommandOptions: []*autocliv1.RpcCommandOptions{ | ||
| { | ||
| RpcMethod: "UpdateParams", | ||
| Skip: true, // skipped because authority gated | ||
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update mint module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query mint params --output json`", version.AppName), | ||
| Example: fmt.Sprintf(`%s tx mint update-params-proposal '{ "mint_denom": "stake", ... }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The new UpdateParams RPC method is well-documented with usage, short and long descriptions, and an example. The use of fmt.Sprintf to include the application name in the help text is a good practice as it makes the help text dynamic and adaptable to changes in the application name. However, there is a minor inconsistency in the long description: it mentions "gov module params" instead of "mint module params". This should be corrected to avoid confusion.
- Submit a proposal to update gov module params. Note: the entire params must be provided.
+ Submit a proposal to update mint module params. Note: the entire params must be provided.Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| Service: mintv1beta1.Msg_ServiceDesc.ServiceName, | |
| RpcCommandOptions: []*autocliv1.RpcCommandOptions{ | |
| { | |
| RpcMethod: "UpdateParams", | |
| Skip: true, // skipped because authority gated | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update mint module params. Note: the entire params must be provided.", | |
| Long: fmt.Sprintf("Submit a proposal to update gov module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query mint params --output json`", version.AppName), | |
| Example: fmt.Sprintf(`%s tx mint update-params-proposal '{ "mint_denom": "stake", ... }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, | |
| }, | |
| }, | |
| Service: mintv1beta1.Msg_ServiceDesc.ServiceName, | |
| RpcCommandOptions: []*autocliv1.RpcCommandOptions{ | |
| { | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal [params]", | |
| Short: "Submit a proposal to update mint module params. Note: the entire params must be provided.", | |
| Long: fmt.Sprintf("Submit a proposal to update mint module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query mint params --output json`", version.AppName), | |
| Example: fmt.Sprintf(`%s tx mint update-params-proposal '{ "mint_denom": "stake", ... }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, | |
| }, | |
| }, |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/mint/autocli.go (2 hunks)
- x/slashing/autocli.go (1 hunks)
- x/staking/autocli.go (1 hunks)
Additional comments: 3
x/slashing/autocli.go (3)
46-46: The example provided for the
unjailcommand is missing the--chain-idflag, which is typically required for transactions. Ensure that users are aware they need to specify the chain ID when using this command.52-52: The long description for the
update-params-proposalcommand includes a command to view the current parameters. Ensure that the output of thequery slashing params --output jsoncommand is in a format that can be directly used as input to theupdate-params-proposalcommand to avoid any confusion or additional transformation steps for the user.55-55: The
GovProposalfield is set totrueto mark the command as a governance proposal. Ensure that thehandleGovProposalfunction in theautoclipackage is properly handling this new field and that the governance proposal submission process is thoroughly tested.
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update slashing module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update slashing module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query slashing params --output json`", version.AppName), | ||
| Example: fmt.Sprintf(`%s tx slashing update-params-proposal '{ "signed_blocks_window": "100", ... }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, |
There was a problem hiding this comment.
The update-params-proposal command requires the entire set of parameters to be provided. This could be error-prone for users who may only want to update a single parameter. Consider providing functionality that allows for partial updates to parameters, where the user only needs to supply the parameters they wish to change, and the system merges these with the current parameters.
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "validator_address"}, {ProtoField: "amount"}, {ProtoField: "creation_height"}}, | ||
| }, | ||
| { | ||
| RpcMethod: "UpdateParams", | ||
| Skip: true, // skipped because authority gated | ||
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal [params]", | ||
| Short: "Submit a proposal to update staking module params. Note: the entire params must be provided.", | ||
| Long: fmt.Sprintf("Submit a proposal to update staking module params. Note: the entire params must be provided.\n See the fields to fill in by running `%s query staking params --output json`", version.AppName), | ||
| Example: fmt.Sprintf(`%s tx staking update-params-proposal '{ "unbonding_time": "504h0m0s", ... }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, | ||
| }, | ||
| }, | ||
| EnhanceCustomCommand: true, |
There was a problem hiding this comment.
The addition of the UpdateParams command with GovProposal set to true is a significant change. It is important to ensure that the params positional argument is properly validated before being processed to prevent any malformed input from causing issues. Additionally, the documentation provided in the Long description is helpful, but it should also mention any restrictions or formats required for the params JSON object. This will help users understand how to structure their proposals correctly.
Moreover, the Example field provides a good starting point, but it might be beneficial to include a more complete example that shows a typical params object structure. This would give users a clearer idea of what a full proposal might look like.
Lastly, the EnhanceCustomCommand field set to true at the end of the Tx descriptor implies that there might be additional custom logic associated with these commands. It would be prudent to review any such custom logic to ensure it aligns with the new functionality and does not introduce any security or stability issues.
There was a problem hiding this comment.
This will be improved later, where fields will be directly flag options or positional arguments (#18310), then the json won't be necessary.
There was a problem hiding this comment.
@julienrbrt Understood, I look forward to the enhancements detailed in issue #18310. This approach will indeed streamline the user experience by allowing direct input of parameters as flags or positional arguments, eliminating the need for JSON structuring. If there are any interim updates or documentation improvements needed before this change is implemented, please let me know how I can assist.
|
Now that I want to backport this in the next version of client/v2 I will get the issue of bumping the api module again the v0.50... |
| 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.
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.
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
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