feat(serverv2/cometbft) Read config from commands & handle FlagNode
#20621
Conversation
|
Warning Rate limit exceeded@hieuvubk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 46 minutes and 17 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CobraCommand
participant CometBFTServer
participant rpcClient
participant Config
User ->> CobraCommand: Executes Command
CobraCommand ->> CometBFTServer: Calls Command Function
CometBFTServer ->> rpcClient: Passes cmd to rpcClient(cmd)
rpcClient ->> Config: Fetches Config using cmd
Config ->> rpcClient: Returns Config
rpcClient ->> CometBFTServer: Returns rpcClient instance
CometBFTServer ->> User: Executes Command Logic
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 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@hieuvubk your pull request is missing a changelog! |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (1)
server/v2/cometbft/commands.go (1)
Line range hint
404-417: Ensure error handling for configuration and state initialization inBootstrapStateCmd.The
BootstrapStateCmdfunction lacks comprehensive error handling for configuration retrieval and state initialization, which could lead to failures during runtime:+ if cfg == nil { + return fmt.Errorf("failed to retrieve configuration") + } + if height == 0 && err != nil { + return fmt.Errorf("failed to retrieve latest version: %w", err) + }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/cometbft/commands.go (10 hunks)
Additional context used
Path-based instructions (1)
server/v2/cometbft/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/commands.go
Outdated
| func (s *CometBFTServer[T]) rpcClient(cmd *cobra.Command) (rpc.CometRPC, error) { | ||
| if s.config.Standalone { | ||
| client, err := rpchttp.New(s.config.CmtConfig.RPC.ListenAddress) | ||
| client, err := rpchttp.New(client.GetConfigFromCmd(cmd).RPC.ListenAddress) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return client, nil | ||
| } | ||
|
|
||
| if s.Node == nil || cmd.Flags().Changed(flags.FlagNode) { | ||
| rpcURI, _ := cmd.Flags().GetString(flags.FlagNode) | ||
| if rpcURI != "" { | ||
| return rpchttp.New(rpcURI) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider handling the error when fetching the FlagNode string.
The current implementation assumes successful retrieval of the FlagNode string without checking for errors. This could lead to unexpected behavior if the flag is not set or there is an issue with the command flags. Consider adding error handling:
- rpcURI, _ := cmd.Flags().GetString(flags.FlagNode)
+ rpcURI, err := cmd.Flags().GetString(flags.FlagNode)
+ if err != nil {
+ return nil, fmt.Errorf("failed to get FlagNode: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *CometBFTServer[T]) rpcClient(cmd *cobra.Command) (rpc.CometRPC, error) { | |
| if s.config.Standalone { | |
| client, err := rpchttp.New(s.config.CmtConfig.RPC.ListenAddress) | |
| client, err := rpchttp.New(client.GetConfigFromCmd(cmd).RPC.ListenAddress) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return client, nil | |
| } | |
| if s.Node == nil || cmd.Flags().Changed(flags.FlagNode) { | |
| rpcURI, _ := cmd.Flags().GetString(flags.FlagNode) | |
| if rpcURI != "" { | |
| return rpchttp.New(rpcURI) | |
| } | |
| } | |
| func (s *CometBFTServer[T]) rpcClient(cmd *cobra.Command) (rpc.CometRPC, error) { | |
| if s.config.Standalone { | |
| client, err := rpchttp.New(client.GetConfigFromCmd(cmd).RPC.ListenAddress) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return client, nil | |
| } | |
| if s.Node == nil || cmd.Flags().Changed(flags.FlagNode) { | |
| rpcURI, err := cmd.Flags().GetString(flags.FlagNode) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get FlagNode: %w", err) | |
| } | |
| if rpcURI != "" { | |
| return rpchttp.New(rpcURI) | |
| } | |
| } |
| cmtConfig := client.GetConfigFromCmd(cmd) | ||
| nodeKey, err := p2p.LoadNodeKey(cmtConfig.NodeKeyFile()) |
There was a problem hiding this comment.
Validate the existence of NodeKeyFile before attempting to load it.
The ShowNodeIDCmd function attempts to load a node key file without prior validation of its existence or accessibility, which could lead to runtime errors if the file is missing or inaccessible:
+ if _, err := os.Stat(cmtConfig.NodeKeyFile()); os.IsNotExist(err) {
+ return fmt.Errorf("node key file does not exist: %s", cmtConfig.NodeKeyFile())
+ }Committable suggestion was skipped due to low confidence.
| @@ -241,7 +250,7 @@ | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| typ, _ := cmd.Flags().GetString(auth.FlagType) | |||
|
|
|||
| rpcclient, err := s.rpcClient() | |||
| rpcclient, err := s.rpcClient(cmd) | |||
There was a problem hiding this comment.
Improve error messages for user clarity in QueryBlockCmd.
The error messages in QueryBlockCmd are generic and do not provide enough context to the user about what went wrong. Enhancing these messages can improve user experience:
- return fmt.Errorf("argument should be a block height")
+ return fmt.Errorf("invalid argument: expected a block height, got '%s'", args[0])
- return fmt.Errorf("no block found with height %s", args[0])
+ return fmt.Errorf("no block found with the specified height: %s", args[0])
- return fmt.Errorf("no block found with hash %s", args[0])
+ return fmt.Errorf("no block found with the specified hash: %s", args[0])Also applies to: 253-253
Committable suggestion was skipped due to low confidence.
| @@ -188,7 +197,7 @@ | |||
| version.AppName, | |||
| ), | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| rpcclient, err := s.rpcClient() | |||
| rpcclient, err := s.rpcClient(cmd) | |||
There was a problem hiding this comment.
Refactor repeated error handling in QueryBlocksCmd for clarity and maintenance.
The error handling in QueryBlocksCmd is repetitive and could be streamlined for better readability and maintenance:
- if err != nil {
- return err
- }
+ if err != nil {
+ return fmt.Errorf("failed to query blocks: %w", err)
+ }Also applies to: 200-200
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rpcclient, err := s.rpcClient(cmd) | |
| rpcclient, err := s.rpcClient(cmd) | |
| if err != nil { | |
| return fmt.Errorf("failed to query blocks: %w", err) | |
| } |
| @@ -46,7 +54,7 @@ | |||
| Use: "status", | |||
| Short: "Query remote node for status", | |||
| RunE: func(cmd *cobra.Command, _ []string) error { | |||
| rpcclient, err := s.rpcClient() | |||
| rpcclient, err := s.rpcClient(cmd) | |||
There was a problem hiding this comment.
Ensure proper error handling and user feedback for command execution.
The StatusCommand function lacks detailed error messages which could help in diagnosing issues more effectively. Consider enhancing the error handling:
- return err
+ return fmt.Errorf("failed to get RPC client: %w", err)
- return err
+ return fmt.Errorf("failed to get status from RPC client: %w", err)
- return err
+ return fmt.Errorf("failed to marshal status output: %w", err)Also applies to: 57-57
Committable suggestion was skipped due to low confidence.
| @@ -332,7 +341,7 @@ | |||
|
|
|||
| // TODO: we should be able to do this without using client context | |||
|
|
|||
| node, err := s.rpcClient() | |||
| node, err := s.rpcClient(cmd) | |||
There was a problem hiding this comment.
Add error handling for potential nil pointer in QueryBlockResultsCmd.
The function QueryBlockResultsCmd does not handle the scenario where node could be nil after an error, which might lead to a runtime panic:
+ if node == nil {
+ return fmt.Errorf("RPC client is unavailable")
+ }Also applies to: 344-344
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| node, err := s.rpcClient(cmd) | |
| node, err := s.rpcClient(cmd) | |
| if node == nil { | |
| return fmt.Errorf("RPC client is unavailable") | |
| } |
| @@ -97,7 +106,7 @@ | |||
| Use: "show-validator", | |||
| Short: "Show this node's CometBFT validator info", | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| cfg := s.config.CmtConfig | |||
| cfg := client.GetConfigFromCmd(cmd) | |||
There was a problem hiding this comment.
Handle potential nil pointer dereference in ShowValidatorCmd.
The function ShowValidatorCmd does not check if privValidator is nil after loading, which could lead to a nil pointer dereference if the loading fails:
+ if privValidator == nil {
+ return fmt.Errorf("failed to load private validator")
+ }Committable suggestion was skipped due to low confidence.
| @@ -131,7 +140,7 @@ | |||
| Use: "show-address", | |||
| Short: "Shows this node's CometBFT validator consensus address", | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| cfg := s.config.CmtConfig | |||
| cfg := client.GetConfigFromCmd(cmd) | |||
There was a problem hiding this comment.
Consider adding detailed error handling for address conversion in ShowAddressCmd.
The conversion of the validator address to a string is straightforward but does not account for potential issues in the address format or conversion process. Adding error handling could improve robustness:
+ if valConsAddr == nil {
+ return fmt.Errorf("invalid validator consensus address")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg := client.GetConfigFromCmd(cmd) | |
| cfg := client.GetConfigFromCmd(cmd) | |
| + if valConsAddr == nil { | |
| + return fmt.Errorf("invalid validator consensus address") | |
| + } |
server/v2/cometbft/commands.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return client, nil | ||
| } | ||
|
|
||
| if s.Node == nil || cmd.Flags().Changed(flags.FlagNode) { | ||
| rpcURI, _ := cmd.Flags().GetString(flags.FlagNode) |
There was a problem hiding this comment.
since we can return an error here i would not silent ignore it; even though it's unlikely to happen due to line above
* main: fix(x/staking): stop validators from rotating to the same key on the same block (#20649) perf: add cache to address codec (#20122) build(deps): Bump google.golang.org/protobuf from 1.34.1 to 1.34.2 (#20632) fix: remove recipient amount from map (#20625) fix(proto): remove conditional preventing proper generated file placement (#20650) (serverv2/cometbft) Read config from commands & handle `FlagNode` (#20621) fix(x/consensus): fix .proto file placement (#20646) fix(store): avoid nil error on not exhausted payload stream (#20644) fix (x/accounts): Fix genesis condition check (#20645) feat(accounts): add genesis account initialization (#20642) fix(x/gov): limit execution in gov (#20348)
julienrbrt
changed the title
FlagNodeFlagNode
Description
Closes: #20610
rpcClientchecks ifflags.FlagNodeexisted in cmd then handleAuthor 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