feat: upgrademonitor #2922
feat: upgrademonitor #2922
Conversation
|
On second thought, this tool can accept a pre-signed transaction that is generated but not published. When the upgrademonitor detects that the network is upgrade-able, it can publish the pre-signed transaction. |
WalkthroughThe changes involve the introduction of a new tool called "Upgrade Monitor" for the Celestia network. This tool is designed to monitor upgrades by querying version tallies, publishing transactions, and checking upgrade eligibility through a gRPC interface. The Changes
Assessment against linked issues
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
tools/upgrademonitor/cmd/root.go
Outdated
| var rootCmd = &cobra.Command{ | ||
| Use: "upgrademonitor", | ||
| Short: "upgrademonitor monitors that status of upgrades on a Celestia network.", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| conn, err := grpc.Dial(grpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect: %v", err) | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| ticker := time.NewTicker(time.Duration(pollFrequency) * time.Second) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| resp, err := internal.QueryVersionTally(conn, version) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("version: %v, voting: %v, threshold: %v, total: %v\n", version, resp.GetVotingPower(), resp.GetThresholdPower(), resp.GetTotalVotingPower()) | ||
|
|
||
| if internal.IsUpgradeable(resp) { | ||
| fmt.Printf("the network is upgradeable so publishing %v\n", pathToTransaction) | ||
| resp, err := internal.Publish(conn, pathToTransaction) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("published transaction: %v\n", resp.TxHash) | ||
| return nil // stop the upgrademonitor | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
The cobra.Command setup looks correct, and the RunE function is well-structured to establish a gRPC connection, create a ticker for polling, and handle the upgrade monitoring logic. However, there are a few points to consider:
-
The use of
insecure.NewCredentials()in line 18 is potentially concerning from a security perspective. It's important to ensure that this is acceptable for the intended use case and that any risks are mitigated. -
The
RunEfunction will exit after publishing a transaction when the network is upgradeable (line 43). This behavior should be clearly documented, and it should be confirmed that this is the desired behavior, as it means the tool will not continue to monitor after an upgrade is detected and handled. -
The logging in lines 34 and 42 is directly to stdout. Consider using a logging framework that can be configured for different environments and verbosity levels.
-
Error handling in lines 19 and 39 is straightforward, but it might be beneficial to log additional context or perform cleanup before returning the error.
-
The infinite loop starting at line 27 will run indefinitely unless an upgrade is detected. Ensure that there is a way to gracefully shut down the application if needed, such as by handling a termination signal.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -0,0 +1,13 @@ | |||
| module github.com/celestiaorg/celestia-app/tools/upgrademonitor | |||
There was a problem hiding this comment.
What was the rationale for this having it's own go.mod?
There was a problem hiding this comment.
hmm upgrademonitor is distinct from celestia-app in that the binary will be used independently of celestia-app. A distinct go.mod allows upgrademonitor to manage its own dependencies.
Your question made me realize that we could even extract upgrademonitor entirely to its own repo.
There was a problem hiding this comment.
If we want it in a stand alone repo: celestiaorg/upgrade-monitor#1
|
Closing in favor of putting this in a stand-alone repo. |
Closes #2894
Demo on Loom