Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(serverv2/cometbft) Read config from commands & handle FlagNode #20621

Merged
merged 5 commits into from
Jun 12, 2024
Merged

feat(serverv2/cometbft) Read config from commands & handle FlagNode #20621

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5a5ff4
read comet config from cmd
hieuvubk Jun 11, 2024
382207c
handle node flag in commands
hieuvubk Jun 11, 2024
f5390a2
Merge branch 'main' into hieu/comet_commands
hieuvubk Jun 12, 2024
bfa5e7e
return err
hieuvubk Jun 12, 2024
e999f74
Merge branch 'main' into hieu/comet_commands
hieuvubk Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions server/v2/cometbft/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,30 @@ import (
"cosmossdk.io/server/v2/cometbft/client/rpc"
"cosmossdk.io/server/v2/cometbft/flags"
auth "cosmossdk.io/x/auth/client/cli"
"github.com/cosmos/cosmos-sdk/client"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"
"github.com/cosmos/cosmos-sdk/version"
)

func (s *CometBFTServer[T]) rpcClient() (rpc.CometRPC, error) {
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we can return an error here i would not silent ignore it; even though it's unlikely to happen due to line above

if rpcURI != "" {
return rpchttp.New(rpcURI)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)
}
}

return local.New(s.Node), nil
}

Expand All @@ -46,7 +54,7 @@ func (s *CometBFTServer[T]) StatusCommand() *cobra.Command {
Use: "status",
Short: "Query remote node for status",
RunE: func(cmd *cobra.Command, _ []string) error {
rpcclient, err := s.rpcClient()
rpcclient, err := s.rpcClient(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return err
}
Expand Down Expand Up @@ -80,7 +88,8 @@ func (s *CometBFTServer[T]) ShowNodeIDCmd() *cobra.Command {
Use: "show-node-id",
Short: "Show this node's ID",
RunE: func(cmd *cobra.Command, args []string) error {
nodeKey, err := p2p.LoadNodeKey(s.config.CmtConfig.NodeKeyFile())
cmtConfig := client.GetConfigFromCmd(cmd)
nodeKey, err := p2p.LoadNodeKey(cmtConfig.NodeKeyFile())
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return err
}
Expand All @@ -97,7 +106,7 @@ func (s *CometBFTServer[T]) ShowValidatorCmd() *cobra.Command {
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

privValidator := pvm.LoadFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile())
pk, err := privValidator.GetPubKey()
if err != nil {
Expand Down Expand Up @@ -131,7 +140,7 @@ func (s *CometBFTServer[T]) ShowAddressCmd() *cobra.Command {
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
cfg := client.GetConfigFromCmd(cmd)
cfg := client.GetConfigFromCmd(cmd)
+ if valConsAddr == nil {
+ return fmt.Errorf("invalid validator consensus address")
+ }

privValidator := pvm.LoadFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile())
// TODO: use address codec?
valConsAddr := (sdk.ConsAddress)(privValidator.GetAddress())
Expand Down Expand Up @@ -188,7 +197,7 @@ for. Each module documents its respective events under 'xx_events.md'.
version.AppName,
),
RunE: func(cmd *cobra.Command, args []string) error {
rpcclient, err := s.rpcClient()
rpcclient, err := s.rpcClient(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
rpcclient, err := s.rpcClient(cmd)
rpcclient, err := s.rpcClient(cmd)
if err != nil {
return fmt.Errorf("failed to query blocks: %w", err)
}

if err != nil {
return err
}
Expand Down Expand Up @@ -241,7 +250,7 @@ $ %s query block --%s=%s <hash>
RunE: func(cmd *cobra.Command, args []string) error {
typ, _ := cmd.Flags().GetString(auth.FlagType)

rpcclient, err := s.rpcClient()
rpcclient, err := s.rpcClient(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return err
}
Expand Down Expand Up @@ -332,7 +341,7 @@ func (s *CometBFTServer[T]) QueryBlockResultsCmd() *cobra.Command {

// TODO: we should be able to do this without using client context

node, err := s.rpcClient()
node, err := s.rpcClient(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
node, err := s.rpcClient(cmd)
node, err := s.rpcClient(cmd)
if node == nil {
return fmt.Errorf("RPC client is unavailable")
}

if err != nil {
return err
}
Expand Down Expand Up @@ -392,6 +401,7 @@ func (s *CometBFTServer[T]) BootstrapStateCmd() *cobra.Command {
Short: "Bootstrap CometBFT state at an arbitrary block height using a light client",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
cfg := client.GetConfigFromCmd(cmd)
height, err := cmd.Flags().GetUint64("height")
if err != nil {
return err
Expand All @@ -404,7 +414,7 @@ func (s *CometBFTServer[T]) BootstrapStateCmd() *cobra.Command {
}

// TODO genensis doc provider and apphash
return node.BootstrapState(cmd.Context(), s.config.CmtConfig, cmtcfg.DefaultDBProvider, nil, height, nil)
return node.BootstrapState(cmd.Context(), cfg, cmtcfg.DefaultDBProvider, nil, height, nil)
},
}

Expand Down
Loading