feat: cosmovisor batch upgrades #21790
Conversation
WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
psiphi5
requested review from
a team,
julienrbrt and
facundomedica
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (7)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (3)
16-16: Correct grammatical error in command's short descriptionThe
Shortdescription should be "Add APP upgrade binaries to cosmovisor" instead of "Add APP upgrades binary to cosmovisor".Apply this diff to fix the grammatical error:
- Short: "Add APP upgrades binary to cosmovisor", + Short: "Add APP upgrade binaries to cosmovisor",
23-24: Use GoDoc style comments for exported functionsThe exported function
AddBatchUpgradeshould have a comment that starts with the function name to comply with GoDoc conventions.Apply this diff to update the comment:
- // AddBatchUpgrade takes in multiple specified upgrades and creates a single - // batch upgrade file out of them + // AddBatchUpgrade takes in multiple specified upgrades and creates a single + // batch upgrade file out of them.Ensure the comment starts with the function name and ends with a period.
13-13: Add GoDoc comment for exported functionNewBatchAddUpgradeCmdThe exported function
NewBatchAddUpgradeCmdlacks a GoDoc comment. Adding a comment improves documentation and code readability.Apply this diff to add the comment:
+ // NewBatchAddUpgradeCmd creates a new Cobra command for batch adding upgrades. func NewBatchAddUpgradeCmd() *cobra.Command {tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (2)
Line range hint
68-70: Avoid using panic; return an error insteadUsing
panicfor expected errors is discouraged as it can cause the application to crash unexpectedly. Instead, return the error to allow it to be handled gracefully by the caller. Also, consider providing a more descriptive error message.Apply this diff to fix the issue:
if err := plan.ValidateBasic(); err != nil { - panic(fmt.Errorf("something is wrong with cosmovisor: %w", err)) + return fmt.Errorf("invalid upgrade plan: %w", err) }
88-123: Unexport functions that are not intended to be publicThe functions
GetConfigandAddUpgradeCmdare currently exported due to their capitalized names. If these functions are only used within this package, consider renaming them togetConfigandaddUpgradeCmdto reflect that they are unexported, following Go conventions and the Uber Go Style Guide.Apply this diff to make the changes:
// GetConfig returns a Config using passed-in flag -func GetConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { +func getConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { // ... // AddUpgradeCmd parses input flags and adds upgrade info to manifest -func AddUpgradeCmd(cmd *cobra.Command, args []string) error { +func addUpgradeCmd(cmd *cobra.Command, args []string) error { // ... func NewAddUpgradeCmd() *cobra.Command { addUpgrade := &cobra.Command{ // ... - RunE: AddUpgradeCmd, + RunE: addUpgradeCmd, } // ...tools/cosmovisor/process.go (1)
47-62: Use descriptive variable names for better readabilityThe variable
uInfosis not descriptive. Consider renaming it toupgradePlansorplansto improve readability and adhere to the style guide recommendations on naming.Apply this diff to rename
uInfostoupgradePlans:-var uInfos []upgradetypes.Plan +var upgradePlans []upgradetypes.PlanAnd update all occurrences of
uInfoswithin the function accordingly.tools/cosmovisor/args.go (1)
112-112: Adjust the function comment for GoDoc style compliancePer GoDoc conventions, the comment should start with the function name and be a complete sentence that describes its behavior.
Update the comment as follows:
-// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix. +// UpgradeInfoBatchFilePath returns the same path as UpgradeInfoFilePath but with a ".batch" suffix.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
- tools/cosmovisor/process.go (4 hunks)
Additional context used
Path-based instructions (6)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
tools/cosmovisor/CHANGELOG.md
39-39: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
40-40: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
GitHub Check: CodeQL
tools/cosmovisor/process.go
[notice] 201-201: Spawning a Go routine
Spawning a Go routine may be a possible source of non-determinism
Additional comments not posted (3)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
22-22: Addition ofNewBatchAddUpgradeCmd()follows conventions and is well-integrated.The new command
NewBatchAddUpgradeCmd()is consistent with the existing command structure. It adheres to the naming conventions outlined in the Uber Golang style guide, using theNewprefix for constructor functions that return a command.tools/cosmovisor/go.mod (1)
8-9: Verify the necessity of adding dependencies as direct requirementsPlease confirm that
github.com/cometbft/cometbft v0.38.9andgithub.com/fsnotify/fsnotify v1.7.0are directly imported in thecosmovisorcodebase. If these packages are only used indirectly through other dependencies, they may not need to be listed as direct requirements in thego.modfile. Keeping only necessary direct dependencies helps maintain a cleaner dependency graph and reduces potential conflicts.Run the following script to check for direct imports of these packages:
tools/cosmovisor/args.go (1)
113-115: LGTM!The new method correctly adds the batch upgrade info file path functionality and follows existing code patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
Line range hint
1-600: Minor deviations from Uber Go Style Guide.The code follows the Uber Go Style Guide for the most part. There are a few minor deviations:
- Some lines are longer than 120 characters. Consider breaking them into multiple lines for better readability.
- Some error messages don't start with lowercase. Error messages should start with lowercase for consistency.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (5 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/cosmovisor/CHANGELOG.md
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (2)
tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
tools/cosmovisor/process.go (4)
47-63: LGTM!The
loadBatchUpgradeFilefunction correctly loads and sorts the batch upgrade plans. The error handling looks good.
68-177: LGTM!The
BatchWatcherfunction correctly implements the batch upgrade process. It handles new blocks, updates theupgrade-info.jsonfile at the appropriate heights, and reloads the upgrade plans if the batch file is modified. The error handling and logging look good.
201-202: LGTM!The changes to the
Runfunction to start theBatchWatcherin a separate goroutine look good. Using a context to manage the lifecycle of theBatchWatcheris a nice touch.Also applies to: 214-215
207-207: LGTM!Calling
cancel()when a signal is received is the correct way to gracefully exit theBatchWatcher.tools/cosmovisor/args.go (4)
41-41: LGTM!The new constant
EnvCometBftRpcEndpointis defined correctly and follows the existing naming convention.
72-72: Looks good!The new
CometBftRpcEndpointfield is added to theConfigstruct correctly, with appropriate tags and a reasonable default value.
114-117: LGTM!The new
UpgradeInfoBatchFilePathfunction is implemented correctly and provides a clear way to derive the batch upgrade info file path.
217-221: Looks good!The
GetConfigFromEnvfunction andDetailStringmethod are updated appropriately to handle the newCometBftRpcEndpointfield.Also applies to: 559-559
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
68-177: TheBatchUpgradeWatcherfunction logic is correct, but consider refactoring for better readability and maintainability.The
BatchUpgradeWatcherfunction is implemented correctly and handles the core logic of watching for upgrades and replacing the upgrade-info files. However, the function is quite long and could be refactored into smaller functions for better readability and maintainability. For example:
- Extract the logic for replacing the upgrade-info and batch files into a separate function.
- Consider propagating errors up to the caller instead of logging and returning.
- Add a way to gracefully stop the watcher.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (3)
tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (11)
tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (6)
22-22: LGTM!Setting the
RunEfield toAddUpgradeCmdis the correct way to specify the handler function for theadd-upgradecommand.
31-32: Refactoring the function signature is a good improvement!Accepting parameters directly instead of relying on command-line arguments enhances the modularity and reusability of the
addUpgradefunction. This change aligns with best practices for writing clean and maintainable code.
36-36: LGTM!Converting the upgrade name to lowercase, when not disabled by configuration, is a good practice to ensure consistency in naming conventions across the codebase.
Line range hint
66-82: The code segment handles upgrades at a specific height correctly.The logic for creating an upgrade plan, validating it, and saving it to the
upgrade-info.jsonfile is implemented correctly. The code segment aligns with the best practices mentioned in the past review comment about setting appropriate file permissions for non-executable files.
88-100: Extracting the configuration retrieval logic is a good practice.Moving the configuration retrieval logic into the
getConfigFromCmdfunction improves code organization and reusability. The function handles errors appropriately and returns them to the caller, which aligns with best practices for error handling in Go.
102-122: TheAddUpgradeCmdfunction is implemented correctly.The function follows the expected logic for a command handler. It parses the input flags, retrieves the configuration, and calls the refactored
addUpgradefunction with the appropriate parameters. The error handling is done appropriately, and the code segment aligns with the changes made to theaddUpgradefunction signature.tools/cosmovisor/process.go (3)
47-63: LGTM!The
loadBatchUpgradeFilefunction is implemented correctly:
- It handles the file not found error gracefully.
- It uses the
Configfor the file path to allow configurability.- Error messages include the file path for easier debugging.
- It sorts the slice of upgrade plans by height to ensure correct order of processing.
95-101: Good use of polling to wait for the chain process to be ready.Polling the RPC endpoint to wait for the chain process to be ready is a good way to ensure the watcher starts at the correct time and doesn't miss any blocks.
214-214: Starting theBatchUpgradeWatcherin a separate goroutine is a good way to ensure it runs concurrently with the main process.The changes to the
Runfunction to start theBatchUpgradeWatcherin a separate goroutine are implemented correctly:
- It allows the watcher to run concurrently with the main process.
- It passes a context to the watcher, which allows it to be cancelled when the
Runfunction exits.- It starts the watcher before waiting for an upgrade, which ensures it doesn't miss any upgrades.
tools/cosmovisor/args_test.go (2)
480-480: Approve the addition of theCometBftRpcEndpointparameter.The addition of the
CometBftRpcEndpointparameter to theConfigstruct enhances the configurability of the application and aligns with the goal of improving integration with CometBFT.
480-480: Verify the handling of theCometBftRpcEndpointparameter.Please ensure that the
CometBftRpcEndpointparameter is properly validated and handled throughout the codebase to maintain the integrity of the application.Verification successful
CometBftRpcEndpointparameter is handled consistently, but could benefit from additional validationThe
CometBftRpcEndpointparameter is properly used throughout the codebase with a default value and environment variable support. However, consider implementing these improvements:
- Add explicit URL format validation when setting the endpoint.
- Enhance error handling when using the endpoint in HTTP requests and client creation.
- Include documentation about security considerations for this parameter.
These suggestions aim to improve robustness and security, but the current implementation is functional.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `CometBftRpcEndpoint` parameter is properly validated and handled. # Test: Search for occurrences of `CometBftRpcEndpoint`. # Expect: Proper validation and handling of the parameter. rg --type go -A 5 $'CometBftRpcEndpoint'Length of output: 2643
| if err := cmd.Process.Signal(sig); err != nil { | ||
| l.logger.Error("terminated", "error", err, "bin", bin) | ||
| os.Exit(1) | ||
| } | ||
| }() | ||
|
|
||
| go BatchUpgradeWatcher(ctx, l.cfg, l.logger) |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
|
Hi @julienrbrt I've finished the implementation of this feature, would love a code review. |
Description
Closes: #20630
This PR adds a
add-batch-upgradetocosmovisorthat allows to add multiple named upgrades at specific heights.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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
add-batch-upgradecommand for efficient batch upgrades.stdinto enhance input handling.Improvements
Dependency Updates