refactor(zetaclient)!: cli improvements #3122
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes significant updates across various files, primarily focused on enhancing the Zeta client command-line interface (CLI) and its underlying functionalities. Key changes involve the introduction of new features, such as a whitelist message ability for SPL tokens and improvements in build reproducibility. Several files have been deleted, including those related to HSM functionalities, while new files have been added for relayer key management and TSS operations. The overall structure of the CLI has been refined for better modularity and clarity. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
fbac
as code owners
swift1337
added
UPGRADE_LIGHT_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3122 +/- ##
===========================================
- Coverage 63.54% 63.29% -0.26%
===========================================
Files 421 422 +1
Lines 29838 29959 +121
===========================================
Hits 18962 18962
- Misses 10036 10157 +121
Partials 840 840
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (34)
cmd/cosmos.go (2)
1-2: Enhance package documentation for better clarity.The current package documentation is minimal. Consider expanding it to include:
- The purpose and scope of the package
- The relationship with Cosmos SDK
- Initialization requirements and usage guidelines
-// Package cmd provides cosmos constants for ZetaClient. +// Package cmd provides Cosmos SDK configuration and constants for ZetaClient. +// +// This package centralizes all Cosmos-related configuration, including Bech32 address +// prefixes, denomination validation, and HD wallet path settings. It ensures proper +// initialization of Cosmos SDK parameters required for ZetaClient operation through +// a thread-safe configuration mechanism.
17-18: Document regex pattern and simplify type declaration.Two suggestions for improvement:
- The
DenomRegexpattern should be documented to explain its validation rules.- The explicit
stringtype forZetaChainHDPathis redundant in Go.- DenomRegex = `[a-zA-Z][a-zA-Z0-9:\\/\\\-\\_\\.]{2,127}` - ZetaChainHDPath string = `m/44'/60'/0'/0/0` + // DenomRegex defines the pattern for valid denomination identifiers: + // - Must start with a letter + // - Can contain alphanumeric characters and :\/\-_. + // - Length must be between 3 and 128 characters + DenomRegex = `[a-zA-Z][a-zA-Z0-9:\\/\\\-\\_\\.]{2,127}` + ZetaChainHDPath = `m/44'/60'/0'/0/0`cmd/zetaclientd/tss.go (2)
48-68: Improve timeout handling and user feedback for the pre-parameter generation.The function works correctly but could benefit from the following improvements:
- Make the timeout configurable
- Add context for cancellation support
- Provide progress feedback for this long-running operation
Consider this enhanced implementation:
-func TSSGeneratePreParams(_ *cobra.Command, args []string) error { +func TSSGeneratePreParams(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() startTime := time.Now() - preParams, err := keygen.GeneratePreParams(time.Second * 300) + + // Show progress indicator + done := make(chan struct{}) + go func() { + ticker := time.NewTicker(time.Second * 5) + defer ticker.Stop() + for { + select { + case <-done: + return + case <-ticker.C: + fmt.Printf("Still generating pre-parameters... (%s elapsed)\n", + time.Since(startTime).Round(time.Second)) + } + } + }() + + preParams, err := keygen.GeneratePreParams(ctx, time.Second * 300) + close(done) if err != nil { return err }
1-68: Consider further modularization of TSS-related functionality.While the current implementation aligns with the PR objectives of reducing logic in
cmd/zetaclientd, consider moving the core TSS functionality (encryption and pre-parameter generation) into a dedicated package (e.g.,pkg/tss). This would:
- Further reduce command file logic
- Improve testability
- Make the functionality reusable outside the CLI context
cmd/zetaclientd/utils.go (2)
Line range hint
25-50: Add documentation and improve code organizationThe function implementation is solid, but could benefit from:
- Documentation explaining its purpose, parameters, and return values
- Constants for configuration keys
- Separation of HSM-specific logic
+// createZetacoreClient initializes and returns a new zetacore client using the provided configuration. +// It handles both standard and HSM key modes, setting up the necessary keyring and authorization. +// Parameters: +// cfg: The configuration containing connection and key settings +// hotkeyPassword: Password for the hot key +// logger: Logger instance for operation logging +// Returns: +// *zetacore.Client: Initialized client instance +// error: Any error encountered during initialization func createZetacoreClient(cfg config.Config, hotkeyPassword string, logger zerolog.Logger) (*zetacore.Client, error) { + const ( + errKeyringBase = "failed to get keyring base" + errGranterAddress = "failed to get granter address" + errClientCreate = "failed to create zetacore client" + ) + hotKey := cfg.AuthzHotkey if cfg.HsmMode { hotKey = cfg.HsmHotKey } kb, _, err := keys.GetKeyringKeybase(cfg, hotkeyPassword) if err != nil { - return nil, errors.Wrap(err, "failed to get keyring base") + return nil, errors.Wrap(err, errKeyringBase) } granterAddress, err := sdk.AccAddressFromBech32(cfg.AuthzGranter) if err != nil { - return nil, errors.Wrap(err, "failed to get granter address") + return nil, errors.Wrap(err, errGranterAddress) } k := keys.NewKeysWithKeybase(kb, granterAddress, cfg.AuthzHotkey, hotkeyPassword) client, err := zetacore.NewClient(k, chainIP, hotKey, cfg.ChainID, cfg.HsmMode, logger) if err != nil { - return nil, errors.Wrap(err, "failed to create zetacore client") + return nil, errors.Wrap(err, errClientCreate) } return client, nil }
77-96: Enhance peer validation and add documentationThe function should include:
- Documentation explaining the expected peer format
- Constants for magic numbers and expected format
- More robust IP validation
+// validatePeer checks if the provided seed peer string matches the expected format: +// "protocol://ip:port/node_id". For example: "tcp://192.168.1.1:26656/nodeid123" func validatePeer(seedPeer string) error { + const ( + expectedParts = 7 + ipIndex = 2 + idIndex = 6 + ) + parsedPeer := strings.Split(seedPeer, "/") - if len(parsedPeer) < 7 { + if len(parsedPeer) < expectedParts { return errors.New("seed peer missing IP or ID or both, seed: " + seedPeer) } - seedIP := parsedPeer[2] - seedID := parsedPeer[6] + seedIP := parsedPeer[ipIndex] + seedID := parsedPeer[idIndex] - if net.ParseIP(seedIP) == nil { + ip := net.ParseIP(seedIP) + if ip == nil || ip.To4() == nil { return errors.New("invalid seed IP address format, seed: " + seedPeer) } if len(seedID) == 0 { return errors.New("seed id is empty, seed: " + seedPeer) } return nil }contrib/localnet/scripts/start-zetaclientd.sh (5)
Line range hint
1-10: Enhance security for SSH daemon initialization.The script starts the SSH daemon without validating its configuration or ensuring secure defaults. Consider adding security checks and hardening measures.
Apply these improvements:
#!/bin/bash +set -euo pipefail + +# Validate SSH configuration +if [ ! -f /etc/ssh/sshd_config ]; then + echo "Error: SSH configuration file missing" + exit 1 +fi + /usr/sbin/sshd
Line range hint
12-16: Add error handling for JSON operations.The
set_sepolia_endpointfunction modifies configuration without validating the operation's success.Apply these improvements:
set_sepolia_endpoint() { + local config_file="/root/.zetacored/config/zetaclient_config.json" + local temp_file="/tmp/zetaclient_config.tmp.json" + + if [ ! -f "$config_file" ]; then + echo "Error: Config file not found: $config_file" + return 1 + fi + - jq '.EVMChainConfigs."11155111".Endpoint = "http://eth2:8545"' /root/.zetacored/config/zetaclient_config.json > tmp.json && mv tmp.json /root/.zetacored/config/zetaclient_config.json + if ! jq '.EVMChainConfigs."11155111".Endpoint = "http://eth2:8545"' "$config_file" > "$temp_file"; then + echo "Error: Failed to update config file" + return 1 + fi + mv "$temp_file" "$config_file" }
Line range hint
18-24: Enhance security for private key handling.The
import_relayer_keyfunction handles sensitive private key data. The command change aligns with PR objectives to streamline CLI.Apply these improvements:
import_relayer_key() { local num="$1" + local config_file="/root/config.yml" + + if [ ! -f "$config_file" ]; then + echo "Error: Config file not found: $config_file" + return 1 + } - privkey_solana=$(yq -r ".observer_relayer_accounts.relayer_accounts[${num}].solana_private_key" /root/config.yml) + # Use process substitution to avoid storing private key in a variable + if ! zetaclientd relayer import-key --network=7 --password=pass_relayerkey \ + --private-key="$(yq -r ".observer_relayer_accounts.relayer_accounts[${num}].solana_private_key" "$config_file")"; then + echo "Error: Failed to import relayer key" + return 1 + fi - zetaclientd relayer import-key --network=7 --private-key="$privkey_solana" --password=pass_relayerkey }
Line range hint
26-37: Improve pre-params handling reliability.The pre-params handling lacks proper validation and error handling for file operations.
Apply these improvements:
+validate_preparams() { + local file="$1" + if [ ! -f "$file" ]; then + return 1 + fi + # Add validation of file content + if ! jq empty "$file" 2>/dev/null; then + echo "Error: Invalid JSON in pre-params file" + return 1 + fi + return 0 +} + PREPARAMS_PATH="/root/preparams/${HOSTNAME}.json" if [[ -n "${ZETACLIENTD_GEN_PREPARAMS}" ]]; then - if [ ! -f "$PREPARAMS_PATH" ]; then + if ! validate_preparams "$PREPARAMS_PATH"; then zetaclientd gen-pre-params "$PREPARAMS_PATH" + if ! validate_preparams "$PREPARAMS_PATH"; then + echo "Error: Failed to generate valid pre-params" + exit 1 + fi fi else echo "Using static preparams" - cp "/root/static-preparams/${HOSTNAME}.json" "${PREPARAMS_PATH}" + if ! cp "/root/static-preparams/${HOSTNAME}.json" "${PREPARAMS_PATH}"; then + echo "Error: Failed to copy static pre-params" + exit 1 + fi + if ! validate_preparams "$PREPARAMS_PATH"; then + echo "Error: Invalid static pre-params" + exit 1 + fi fi
Line range hint
39-127: Add timeouts and validation for initialization steps.The initialization process lacks timeout mechanisms and proper validation of configuration merging.
Key improvements needed:
- Add timeouts for wait conditions
- Validate merged configuration
- Add error handling for initialization steps
Apply these improvements:
+# Add timeout function +wait_with_timeout() { + local timeout=$1 + local condition=$2 + local message=$3 + local start_time=$(date +%s) + + while ! eval "$condition"; do + current_time=$(date +%s) + if [ $((current_time - start_time)) -gt "$timeout" ]; then + echo "Timeout waiting for: $message" + return 1 + fi + echo "Waiting for $message..." + sleep 5 + done + return 0 +} + -while [ ! -f ~/.ssh/authorized_keys ]; do - echo "Waiting for authorized_keys file to exist..." - sleep 1 -done +if ! wait_with_timeout 300 "[ -f ~/.ssh/authorized_keys ]" "authorized_keys file"; then + exit 1 +fi -while ! curl -s -o /dev/null zetacore0:26657/status ; do - echo "Waiting for zetacore0 rpc" - sleep 10 -done +if ! wait_with_timeout 600 "curl -s -o /dev/null zetacore0:26657/status" "zetacore0 RPC"; then + exit 1 +fi # Validate merged configuration +validate_config() { + local config_file="$1" + if ! jq empty "$config_file" 2>/dev/null; then + echo "Error: Invalid JSON in config file" + return 1 + fi + # Add additional validation as needed + return 0 +} if [[ -f /root/zetaclient-config-overlay.json ]]; then - jq -s '.[0] * .[1]' /root/.zetacored/config/zetaclient_config.json /root/zetaclient-config-overlay.json > /tmp/merged_config.json - mv /tmp/merged_config.json /root/.zetacored/config/zetaclient_config.json + if ! jq -s '.[0] * .[1]' /root/.zetacored/config/zetaclient_config.json /root/zetaclient-config-overlay.json > /tmp/merged_config.json; then + echo "Error: Failed to merge configurations" + exit 1 + fi + if ! validate_config /tmp/merged_config.json; then + echo "Error: Invalid merged configuration" + exit 1 + fi + mv /tmp/merged_config.json /root/.zetacored/config/zetaclient_config.json ficmd/zetaclientd/initconfig.go (2)
13-34: Enhance struct documentation and field organization.Consider the following improvements:
- Add detailed field documentation to explain the purpose and valid values for each configuration option.
- Group related fields together (e.g., logging-related, p2p-related, HSM-related).
// initializeConfigOptions is a set of CLI options for `init` command. type initializeConfigOptions struct { + // Core Configuration peer string publicIP string chainID string zetacoreURL string configUpdateTicker uint64 + // Authorization authzGranter string authzHotkey string + // Logging Configuration logFormat string logSampler bool level int8 + // P2P Settings p2pDiagnostic bool p2pDiagnosticTicker uint64 + // Security & Keys TssPath string TestTssKeysign bool KeyringBackend string HsmMode bool HsmHotKey string RelayerKeyPath string + // Miscellaneous preParamsPath string }
1-108: Consider implementing a builder pattern for configuration.The current implementation mixes flag setup, validation, and configuration creation. Consider implementing a builder pattern to:
- Separate concerns between flag parsing and configuration creation
- Enable programmatic configuration creation for testing
- Provide better validation at compile-time
- Make the configuration process more maintainable and testable
Would you like me to provide an example implementation using the builder pattern?
cmd/zetaclientd/inbound.go (4)
28-31: Add documentation and follow Go naming conventions.The struct and its fields should be documented and follow Go naming conventions for better maintainability.
+// inboundOptions holds configuration for inbound operations type inboundOptions struct { - zetaNode string - zetaChainID string + // zetaNode represents the public IP address of the Zeta node + zetaNode string + // zetaChainID represents the chain identifier + zetaChainID string }
Line range hint
42-182: Refactor the function to improve maintainability.The function is too long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Argument parsing and validation
- Chain-specific ballot processing
- Ballot querying and display
Here's a suggested structure:
func InboundGetBallot(cmd *cobra.Command, args []string) error { params, err := parseAndValidateArgs(cmd, args) if err != nil { return err } client, ctx, err := setupZetaCoreClient(params) if err != nil { return err } ballotID, err := processBallotByChainType(ctx, client, params) if err != nil { return err } return displayBallotInfo(ctx, client, ballotID) }
Line range hint
102-147: Add proper resource cleanup.The EVM client connection should be properly closed to prevent resource leaks.
if chainIDFromConfig == chainID { ethRPC = ethrpc.NewEthRPC(evmConfig.Endpoint) client, err = ethclient.Dial(evmConfig.Endpoint) if err != nil { return err } + defer client.Close() evmObserver.WithEvmClient(client)
Line range hint
159-182: Enhance error messages and logging.The ballot query and display section could benefit from more descriptive error messages and structured logging.
ballot, err := client.GetBallot(ctx, ballotIdentifier) if err != nil { - return err + return fmt.Errorf("failed to retrieve ballot %s: %w", ballotIdentifier, err) } +// Use structured logging instead of fmt.Printf +logger := zerolog.Ctx(ctx) for _, vote := range ballot.Voters { - fmt.Printf("%s : %s \n", vote.VoterAddress, vote.VoteType) + logger.Info(). + Str("voter", vote.VoterAddress). + Str("vote", vote.VoteType). + Msg("Vote recorded") }zetaclient/config/types.go (1)
26-27: LGTM! Consider adding documentation.The introduction of
DefaultRelayerDirconstant improves maintainability by centralizing the relayer keys directory configuration.Consider adding a documentation comment to describe the purpose of this constant:
+// DefaultRelayerDir is the default directory name for storing relayer keys DefaultRelayerDir = "relayer-keys"zetaclient/tss/generate.go (4)
25-35: Improve godoc formatting for better documentation.The documentation should follow standard godoc formatting conventions. Consider this structure:
-// Generate generates a new TSS if keygen is set. -// If a TSS was generated successfully in the past,and the keygen was successful, the function will return without doing anything. -// If a keygen has been set the functions will wait for the correct block to arrive and generate a new TSS. -// In case of a successful keygen a TSS success vote is broadcasted to zetacore and the newly generate TSS is tested. The generated keyshares are stored in the correct directory -// In case of a failed keygen a TSS failed vote is broadcasted to zetacore. +// Generate generates a new TSS if keygen is set. It handles the following cases: +// - Returns immediately if TSS was previously generated successfully +// - Waits for the correct block and generates new TSS if keygen is set +// - Broadcasts success/failure votes to zetacore based on generation outcome +// - Stores generated keyshares in the designated directory
Line range hint
36-124: Consider breaking down the complex monitoring loop into smaller functions.The current implementation combines multiple responsibilities within a single loop. Consider extracting the following into separate functions:
- Block monitoring
- TSS generation state management
- Vote broadcasting logic
This would improve readability, testability, and maintainability.
Example structure:
func Generate(ctx context.Context, ...) error { monitor := newTSSMonitor(logger, zetaCoreClient) for state := range monitor.Watch(ctx) { if err := handleTSSState(ctx, state, keygenTssServer); err != nil { logger.Error().Err(err).Msg("TSS state handling failed") continue } } return nil }
Line range hint
126-177: Enhance error handling with wrapped errors for better context.The error handling could be improved by:
- Using
fmt.Errorfwith%wfor error wrapping- Extracting blame handling into a separate function
Example:
func handleBlame(ctx context.Context, blame *tss.Blame, req *keygen.Request) error { digest, err := digestReq(req) if err != nil { return fmt.Errorf("failed to digest request: %w", err) } // ... rest of blame handling logic }
Line range hint
179-183: Enhance error reporting in TestTSS function.Consider wrapping the error from
TestKeysignwith additional context about the test operation:- err := TestKeysign(pubkey, tssServer) - if err != nil { - return err + if err := TestKeysign(pubkey, tssServer); err != nil { + return fmt.Errorf("TSS keysign test failed for pubkey %s: %w", pubkey, err) }zetaclient/tss/tss_signer.go (6)
2-3: Consider adding more context to the TODO comment.The TODO comment about revamping the package should include more details about the planned changes and their rationale. This helps other developers understand the direction and scope of future changes.
-// TODO revamp the whole package -// https://github.com/zeta-chain/node/issues/3119 +// TODO(revamp): Refactor the entire package to improve modularity and testability. +// This includes separating the TSS server logic, improving error handling, and adding comprehensive tests. +// Tracking issue: https://github.com/zeta-chain/node/issues/3119
Line range hint
90-124: Consider splitting the constructor function for better testability.The
Newfunction has multiple responsibilities: initializing the TSS instance, loading files, verifying keyshares, and setting up metrics. Consider splitting these into separate functions that can be tested independently.func New( ctx context.Context, client interfaces.ZetacoreClient, tssHistoricalList []observertypes.TSS, hotkeyPassword string, tssServer *tss.TssServer, ) (*TSS, error) { logger := log.With().Str("module", "tss_signer").Logger() app, err := zctx.FromContext(ctx) if err != nil { return nil, err } - newTss := TSS{ + newTss, err := initializeTSS(app, tssServer, client, logger) + if err != nil { + return nil, fmt.Errorf("initialize TSS: %w", err) + } - Server: tssServer, - Keys: make(map[string]*Key), - CurrentPubkey: app.GetCurrentTssPubKey(), - logger: logger, - ZetacoreClient: client, - KeysignsTracker: NewKeysignsTracker(logger), - } err = newTss.LoadTssFilesFromDirectory(app.Config().TssPath) if err != nil { return nil, err } // ... rest of the function } +func initializeTSS(app *zctx.App, tssServer *tss.TssServer, client interfaces.ZetacoreClient, logger zerolog.Logger) (*TSS, error) { + return &TSS{ + Server: tssServer, + Keys: make(map[string]*Key), + CurrentPubkey: app.GetCurrentTssPubKey(), + logger: logger, + ZetacoreClient: client, + KeysignsTracker: NewKeysignsTracker(logger), + }, nil +}
Line range hint
271-321: Enhance error handling in SignBatch function.The
SignBatchfunction has several critical points where error handling could be improved:
- The error messages are not descriptive enough
- Some error conditions are not properly propagated
- The signature verification is commented out without explanation
func (tss *TSS) SignBatch( ctx context.Context, digests [][]byte, height uint64, nonce uint64, chainID int64, ) ([][65]byte, error) { if len(digests) == 0 { + return nil, fmt.Errorf("empty digest list") } tssPubkey := tss.CurrentPubkey digestBase64 := make([]string, len(digests)) for i, digest := range digests { + if len(digest) == 0 { + return nil, fmt.Errorf("empty digest at index %d", i) + } digestBase64[i] = base64.StdEncoding.EncodeToString(digest) } // Uncomment and fix the signature verification or remove it with explanation - //if !verifySignatures(tssPubkey, signatures, digests) { - // log.Error().Err(err).Msgf("signature verification failure") - // return [][65]byte{}, fmt.Errorf("signuature verification fail") - //}
Line range hint
502-526: TestKeysign function needs improvement.The
TestKeysignfunction has several issues:
- It's in the main package but marked as a test function
- Uses hardcoded test data
- Lacks proper test assertions and error messages
Consider moving this to a proper test file:
-func TestKeysign(tssPubkey string, tssServer tss.TssServer) error { +// TestKeysignWithServer is a helper function for integration tests +func TestKeysignWithServer(t *testing.T, tssPubkey string, tssServer tss.TssServer) { + t.Helper() testCases := []struct { name string data []byte wantErr bool }{ { name: "valid signature", data: []byte("hello meta"), wantErr: false, }, // Add more test cases } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + H := crypto.Keccak256Hash(tc.data) + // ... rest of the test + }) + } }
Line range hint
528-531: Improve environment flag handling.The
IsEnvFlagEnabledfunction could be enhanced to support more flexible environment variable configurations and provide better validation.-func IsEnvFlagEnabled(flag string) bool { - value := os.Getenv(flag) - return value == "true" || value == "1" +func IsEnvFlagEnabled(flag string) bool { + value := strings.ToLower(strings.TrimSpace(os.Getenv(flag))) + return value == "true" || value == "1" || value == "yes" || value == "on" }
Line range hint
533-583: Critical security concern in verifySignature function.The
verifySignaturefunction has several security-critical issues:
- Logging of sensitive cryptographic material
- Insufficient error handling for cryptographic operations
- TODO comment indicates it should be moved but remains in production code
func verifySignature(tssPubkey string, signature []keysign.Signature, H []byte) bool { if len(signature) == 0 { - log.Warn().Msg("verify_signature: empty signature array") + log.Error().Msg("verify_signature: empty signature array") return false } pubkey, err := cosmos.GetPubKeyFromBech32(cosmos.Bech32PubKeyTypeAccPub, tssPubkey) if err != nil { - log.Error().Msg("get pubkey from bech32 fail") + log.Error().Err(err).Msg("failed to get pubkey from bech32") return false } - log.Info().Msgf("pubkey %s recovered pubkey %s", pubkey.String(), hex.EncodeToString(compressedPubkey)) + // Don't log sensitive cryptographic material in production + log.Debug().Msg("signature verification completed") return bytes.Equal(pubkey.Bytes(), compressedPubkey) }changelog.md (1)
9-10: Enhance the changelog entry with more details.The changelog entry for PR #3122 could be more descriptive to better communicate the impact and scope of the changes.
Consider expanding the entry with:
-* [3122](https://github.com/zeta-chain/node/pull/3122) - improve & refactor zetaclientd cli +* [3122](https://github.com/zeta-chain/node/pull/3122) - improve & refactor zetaclientd cli: + - Consolidated command structure with single init() function + - Streamlined CLI for better usability + - Simplified command configuration processcmd/zetaclientd/relayer.go (3)
44-44: Update function comments to follow GoDoc conventionsThe comments for
RelayerImportKeyandRelayerShowAddressshould start with the function name and be written in full sentences to comply with GoDoc standards. This improves documentation quality and readability.Apply these diffs to update the comments:
For
RelayerImportKey:-// RelayerImportKey imports a relayer private key +// RelayerImportKey imports a relayer private key.For
RelayerShowAddress:-// RelayerShowAddress shows the relayer address +// RelayerShowAddress shows the relayer address.Also applies to: 94-94
90-90: Use structured logging instead offmt.Printffor outputUtilizing
fmt.Printffor outputting operational messages can hinder log management and scalability. Adopting a structured logging approach enhances log handling and integration with logging systems.Apply these diffs to replace
fmt.Printfwith a logging function:At line 90:
- fmt.Printf("successfully imported relayer key: %s\n", fileName) + log.Printf("Successfully imported relayer key: %s", fileName)At line 120:
- fmt.Printf("relayer address (%s): %s\n", networkName, address) + log.Printf("Relayer address (%s): %s", networkName, address)Ensure that you import the
logpackage and configure the logger as needed.Also applies to: 120-120
48-55: Simplify input validation logic for clarityThe current switch statement for input validation could be refactored into sequential
ifstatements for improved readability and maintainability.Refactor the validation logic as follows:
-func RelayerImportKey(_ *cobra.Command, _ []string) error { - // validate private key and password - - switch { - case relayerOpts.privateKey == "": - return errors.New("must provide a private key") - case relayerOpts.password == "": - return errors.New("must provide a password") - case !keys.IsRelayerPrivateKeyValid(relayerOpts.privateKey, chains.Network(relayerOpts.network)): - return errors.New("invalid private key") - } +func RelayerImportKey(_ *cobra.Command, _ []string) error { + // validate private key and password + + if relayerOpts.privateKey == "" { + return errors.New("must provide a private key") + } + if relayerOpts.password == "" { + return errors.New("must provide a password") + } + if !keys.IsRelayerPrivateKeyValid(relayerOpts.privateKey, chains.Network(relayerOpts.network)) { + return errors.New("invalid private key") + }This change enhances code clarity by making the validation steps explicit.
cmd/zetaclientd/start.go (1)
41-44: Address the TODO and initializepreParamsappropriatelyThe code contains a TODO comment indicating a need for revamping and references issues #3119 and #3112. It's important to ensure that
preParamsis properly initialized to prevent potentialnilpointer dereferences during the TSS setup.Would you like assistance in implementing the necessary changes or opening a GitHub issue to track this task?
cmd/zetaclientd/main.go (1)
116-119: Simplify error message and exit code handlingIncluding "Exit code 1" in the error message is redundant since the program exits with code 1. Additionally, consider using
fmt.Fprintfto write errors toos.Stderr.Apply this diff to enhance error handling:
if err := RootCmd.ExecuteContext(ctx); err != nil { - fmt.Printf("Error: %s. Exit code 1\n", err) + fmt.Fprintf(os.Stderr, "Error: %s\n", err) os.Exit(1) }This ensures that error messages are sent to the standard error stream and avoids unnecessary information in the output.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
changelog.md(1 hunks)cmd/config.go(0 hunks)cmd/cosmos.go(1 hunks)cmd/zetaclientd/encrypt_tss.go(0 hunks)cmd/zetaclientd/gen_pre_params.go(0 hunks)cmd/zetaclientd/hsm.go(0 hunks)cmd/zetaclientd/import_relayer_keys.go(0 hunks)cmd/zetaclientd/inbound.go(2 hunks)cmd/zetaclientd/init.go(0 hunks)cmd/zetaclientd/initconfig.go(1 hunks)cmd/zetaclientd/main.go(1 hunks)cmd/zetaclientd/relayer.go(1 hunks)cmd/zetaclientd/root.go(0 hunks)cmd/zetaclientd/start.go(8 hunks)cmd/zetaclientd/start_utils.go(0 hunks)cmd/zetaclientd/tss.go(1 hunks)cmd/zetaclientd/utils.go(2 hunks)cmd/zetaclientd/version.go(0 hunks)contrib/localnet/scripts/start-zetaclientd.sh(1 hunks)zetaclient/config/types.go(1 hunks)zetaclient/tss/generate.go(3 hunks)zetaclient/tss/tss_signer.go(2 hunks)
💤 Files with no reviewable changes (9)
- cmd/config.go
- cmd/zetaclientd/encrypt_tss.go
- cmd/zetaclientd/gen_pre_params.go
- cmd/zetaclientd/hsm.go
- cmd/zetaclientd/import_relayer_keys.go
- cmd/zetaclientd/init.go
- cmd/zetaclientd/root.go
- cmd/zetaclientd/start_utils.go
- cmd/zetaclientd/version.go
🧰 Additional context used
📓 Path-based instructions (12)
cmd/cosmos.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/initconfig.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/main.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/relayer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/start.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/tss.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/utils.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/localnet/scripts/start-zetaclientd.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
zetaclient/config/types.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/generate.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/tss/tss_signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (7)
cmd/cosmos.go (1)
1-36: Well-structured implementation that aligns with PR objectives.
The file successfully:
- Centralizes Cosmos-related configuration
- Ensures thread-safe, single initialization
- Maintains clean separation of concerns
cmd/zetaclientd/tss.go (1)
1-16: LGTM! Clean and well-organized imports.
The package structure and import declarations follow Go best practices.
cmd/zetaclientd/initconfig.go (1)
38-69:
Review default values and add validation for production safety.
Several configuration defaults and validations need attention:
- The default value "127.0.0.1" for zetacore-url might not be suitable for production.
- The default keyring-backend "test" should be "file" for production environments.
- Missing validation for critical parameters like chain-id and zetacore-url.
Consider implementing these changes:
f.StringVar(&cfg.chainID, "chain-id", "athens_7001-1", "chain id")
-f.StringVar(&cfg.zetacoreURL, "zetacore-url", "127.0.0.1", "zetacore node URL")
+f.StringVar(&cfg.zetacoreURL, "zetacore-url", "", "zetacore node URL (required)")
-f.StringVar(&cfg.KeyringBackend, "keyring-backend", string(config.KeyringBackendTest), usageKeyring)
+f.StringVar(&cfg.KeyringBackend, "keyring-backend", string(config.KeyringBackendFile), usageKeyring)Let's verify the current usage of these default values across the codebase:
#!/bin/bash
# Search for references to these default values
rg -A 2 "127.0.0.1"
rg -A 2 "KeyringBackendTest"cmd/zetaclientd/inbound.go (1)
33-33: LGTM!
The global variable declaration follows common patterns for cobra-based CLI applications.
zetaclient/tss/tss_signer.go (1)
90-91: Function name change aligns with Go best practices.
The renaming of NewTSS to New follows Go's idiomatic pattern for constructor functions. This change improves consistency with standard Go practices.
changelog.md (1)
Line range hint 1-24: LGTM! The changelog structure is well-organized.
The changelog follows a clear and consistent structure with appropriate categorization of changes (Features, Refactor, Tests, Fixes) and proper version numbering.
cmd/zetaclientd/start.go (1)
46-46: Confirm the necessity of exporting the Start function
Renaming start to Start changes its visibility from unexported to exported. Verify that this change is intentional and that external packages or commands require access to this function.
init()in cmdCloses #2019
Closes #2029
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor