refactor(client/v2)!: remove client.Context #22493
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
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
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (15)
client/v2/autocli/config/toml.go (1)
10-12: Update comment to remove template referenceThe function comment still mentions template rendering, but the implementation now uses direct TOML marshaling.
Apply this diff to fix the documentation:
-// writeConfigFile renders config using the template and writes it to -// configFilePath. +// writeConfigFile marshals the config to TOML format and writes it to +// configFilePath.client/v2/autocli/context/context.go (2)
1-15: Consider renaming the package to avoid confusion with stdlib's contextThe package name
contextmight cause confusion with Go's standard librarycontextpackage. Consider a more specific name likeautoclicontextorclientctxto better reflect its purpose and avoid potential naming conflicts.-package context +package autoclicontext
20-38: Add documentation for struct fieldsWhile the struct itself is documented, each field should have its own documentation comment explaining its purpose and usage. This helps users understand the role of each component.
Example improvement:
type Context struct { gocontext.Context + // Flags contains the command-line flags for the current command Flags *pflag.FlagSet + // AddressCodec handles encoding/decoding of account addresses AddressCodec address.Codec + // ValidatorAddressCodec handles encoding/decoding of validator addresses ValidatorAddressCodec address.ValidatorAddressCodec + // ConsensusAddressCodec handles encoding/decoding of consensus addresses ConsensusAddressCodec address.ConsensusAddressCodec + // Cdc provides methods for serializing and deserializing data Cdc codec.Codec + // Printer handles formatted output Printer *print.Printer + // Keyring manages cryptographic keys Keyring keyring.Keyring + // EnabledSignmodes defines the allowed signing modes EnabledSignmodes []apisigning.SignMode }client/v2/offchain/sign_test.go (3)
16-17: Consider adding negative test cases for address encodingThe initialization of separate codecs for different address types is good practice. However, consider adding test cases that verify the behavior with invalid address prefixes to ensure robust error handling.
25-29: Consider adding test cases for context validationWhile the new context structure is correct, consider adding test cases that verify behavior when context fields are nil or invalid. This would ensure robust error handling in the production code.
// Example additional test cases: { name: "nil address codec", ctx: clientcontext.Context{AddressCodec: nil}, wantErr: true, }, { name: "nil keyring", ctx: clientcontext.Context{Keyring: nil}, wantErr: true, }
60-60: Consider expanding test coverage for Sign functionWhile the current test cases cover the basic signing modes, consider adding:
- Test cases for empty/nil client connection
- Edge cases for various byte inputs (empty, very large)
- Different encoding combinations
client/v2/autocli/keyring/keyring.go (1)
32-46: Consider enhancing error messages for better debuggingThe error messages from flag retrieval could be more specific to help with debugging.
Consider wrapping the errors with more context:
backEnd, err := flagSet.GetString("keyring-backend") if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get keyring-backend flag: %w", err) } keyringDir, err := flagSet.GetString("keyring-dir") if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get keyring-dir flag: %w", err) } if keyringDir == "" { keyringDir, err = flagSet.GetString("home") if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get home flag: %w", err) } }client/v2/offchain/sign.go (1)
33-38: Consider using a configuration struct to simplify the function signatureThe function signature has become quite complex with many parameters. This makes it harder to maintain and use correctly.
Consider introducing a configuration struct:
+type SignConfig struct { + Context clientcontext.Context + RawBytes []byte + Conn gogogrpc.ClientConn + FromName string + Encoding string + SignMode string + Output string +} -func Sign( - ctx clientcontext.Context, - rawBytes []byte, - conn gogogrpc.ClientConn, - fromName, encoding, signMode, output string, -) (string, error) { +func Sign(config SignConfig) (string, error) {This would make the function more maintainable and easier to extend in the future.
client/v2/offchain/cli.go (4)
43-43: Enhance the help text for the Bech32 prefix flag.Consider providing more context in the help text to explain the purpose and impact of the Bech32 prefix.
-cmd.PersistentFlags().String(flagBech32, "cosmos", "address bech32 prefix") +cmd.PersistentFlags().String(flagBech32, "cosmos", "Bech32 prefix for addresses (e.g., 'cosmos' for Cosmos Hub addresses)")
86-87: Improve the comment clarity for CometBFT broadcaster initialization.The current comment is vague. Consider explaining why off-chain operations don't require querying and what the empty string parameter represents.
-// off-chain does not need to query any information +// Initialize CometBFT broadcaster with empty node address as off-chain operations +// don't require querying or broadcasting to the network
92-98: Consider validating context fields.The context initialization could benefit from validation to ensure required fields are properly set.
+func validateContext(ctx clientcontext.Context) error { + if ctx.AddressCodec == nil { + return fmt.Errorf("address codec is required") + } + if ctx.ValidatorAddressCodec == nil { + return fmt.Errorf("validator address codec is required") + } + if ctx.Cdc == nil { + return fmt.Errorf("codec is required") + } + return nil +}Then use it after context initialization:
ctx := clientcontext.Context{ Flags: cmd.Flags(), AddressCodec: ac, ValidatorAddressCodec: address.NewBech32Codec(sdk.GetBech32PrefixValAddr(bech32Prefix)), Cdc: cdc, Keyring: k, } +if err := validateContext(ctx); err != nil { + return err +}
Line range hint
153-157: Enhance the verification success message.The current success message could be more informative by including details about what was verified.
-if err == nil { - cmd.Println("Verification OK!") +if err == nil { + cmd.Printf("File %s verification successful!\n", args[0]) }client/v2/tx/tx.go (2)
Line range hint
179-238: Consider extracting confirmation logic into a separate functionThe transaction confirmation logic in
BroadcastTxis quite lengthy and could be extracted into a separate function for better readability and maintainability.+func confirmTransaction(printer *print.Printer, txBytes []byte) error { + if err := printer.PrintRaw(txBytes); err != nil { + _, _ = fmt.Fprintf(os.Stderr, "error: %v\n%s\n", err, txBytes) + } + + buf := bufio.NewReader(os.Stdin) + ok, err := input.GetConfirmation("confirm transaction before signing and broadcasting", buf, os.Stderr) + if err != nil { + _, _ = fmt.Fprintf(os.Stderr, "error: %v\ncanceled transaction\n", err) + return err + } + if !ok { + _, _ = fmt.Fprintln(os.Stderr, "canceled transaction") + return nil + } + return nil +}
Line range hint
223-238: Add timeout to broadcast contextThe broadcast operation uses a background context without a timeout, which could lead to hanging operations. Consider adding a timeout.
- res, err := broadcaster.Broadcast(context.Background(), txBytes) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + res, err := broadcaster.Broadcast(ctx, txBytes)client/v2/autocli/msg.go (1)
Line range hint
109-110: Plan transition from client.GetClientTxContextThe BuildMsgMethodCommand still uses client.GetClientTxContext. As per the PR objectives and the nolint comment on generateOrBroadcastTxWithV2, this needs to be updated to use the v2 transaction factory.
Consider creating a follow-up issue to track the migration of BuildMsgMethodCommand to use the new v2 transaction handling approach.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (19)
client/v2/autocli/app.go(4 hunks)client/v2/autocli/builder.go(2 hunks)client/v2/autocli/common.go(3 hunks)client/v2/autocli/config/config.go(1 hunks)client/v2/autocli/config/toml.go(1 hunks)client/v2/autocli/context/context.go(1 hunks)client/v2/autocli/flag/address.go(5 hunks)client/v2/autocli/keyring/keyring.go(2 hunks)client/v2/autocli/msg.go(2 hunks)client/v2/autocli/print/printer.go(1 hunks)client/v2/broadcast/comet/clientConn.go(1 hunks)client/v2/offchain/cli.go(5 hunks)client/v2/offchain/sign.go(3 hunks)client/v2/offchain/sign_test.go(2 hunks)client/v2/offchain/verify.go(3 hunks)client/v2/offchain/verify_test.go(2 hunks)client/v2/tx/tx.go(7 hunks)client/v2/tx/types.go(1 hunks)simapp/simd/cmd/root.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- client/v2/autocli/app.go
- client/v2/autocli/builder.go
- client/v2/autocli/config/config.go
- client/v2/autocli/flag/address.go
- client/v2/autocli/print/printer.go
- client/v2/broadcast/comet/clientConn.go
- client/v2/offchain/verify_test.go
- client/v2/tx/types.go
- simapp/simd/cmd/root.go
🧰 Additional context used
📓 Path-based instructions (10)
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/config/toml.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/context/context.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/keyring/keyring.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/msg.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/offchain/cli.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/offchain/sign.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/offchain/sign_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"
client/v2/offchain/verify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (21)
client/v2/autocli/config/toml.go (1)
1-8: LGTM: Clean package structure and imports
The package name and imports are well-organized and follow Go conventions.
client/v2/autocli/context/context.go (1)
17-18: LGTM!
The context key constant is well-defined and properly documented. Using the package name as a prefix in the key value helps prevent collisions.
client/v2/offchain/sign_test.go (2)
8-9: LGTM: Import changes align with PR objectives
The replacement of the old client import with the new clientcontext package correctly implements the removal of client.Context.
22-23: LGTM: Proper test isolation with in-memory keyring
The keyring setup correctly uses an in-memory implementation and integrates with the address codec, providing good test isolation.
client/v2/autocli/keyring/keyring.go (2)
4-12: LGTM: Import changes are well-organized
The new imports are properly organized and necessary for the new functionality.
28-53: LGTM: Well-structured implementation of flag-based keyring creation
The function properly implements the new approach of creating a keyring from flags, aligning with the PR objective of removing client.Context. The error handling is comprehensive and the fallback to "home" flag is a good practice.
client/v2/offchain/sign.go (5)
7-10: LGTM: Import changes align with the refactoring goals
The new imports support the transition from client.Context to clientcontext.Context and the addition of gRPC client connection parameter.
44-50: LGTM: TxConfig creation is more direct
The changes simplify the configuration by directly using the context fields.
54-54: LGTM: Account retriever initialization is more explicit
The new implementation clearly shows its dependencies by using the conn parameter and interface registry from the context.
70-70: LGTM: Transaction factory creation is more explicit
The new implementation clearly shows its dependencies by directly using context fields and the connection parameter.
75-78: LGTM: Keyring usage is straightforward
The public key retrieval is now more direct using the context's keyring.
client/v2/offchain/verify.go (4)
11-13: LGTM: Import changes align with PR objectives.
The new imports correctly support the transition from client.Context to the new context structure.
41-42: Skip: Context propagation issue already noted.
A previous review comment already addresses the context propagation concern in this function.
64-67: LGTM: Proper address codec implementation.
The address conversion is correctly implemented with proper error handling.
Line range hint 21-37: LGTM: Clean transition to new context structure.
The function successfully migrates from client.Context to clientcontext.Context, maintaining proper error handling while using explicit context fields.
✅ Verification successful
Let me gather more information about the usage of this Verify function to ensure all callers are properly updated.
All callers of Verify function are properly updated to use clientcontext.Context
After reviewing the implementation, all three calls to the Verify function (two in tests and one in CLI) are correctly using the new clientcontext.Context. The CLI implementation in verify-file command properly constructs the context with all required fields before calling Verify. The test files also use the correct context structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all callers of the Verify function have been updated to use clientcontext.Context
# Search for function calls to Verify
rg -p "Verify\([^)]*\)" --type go
Length of output: 1463
Script:
#!/bin/bash
# Check the implementation details of the Verify function callers
ast-grep --pattern 'Verify(ctx, $_, $_)'
# Check the content of the CLI file that calls Verify
cat client/v2/offchain/cli.go
Length of output: 4961
client/v2/offchain/cli.go (2)
9-20: LGTM: Import changes are well-organized.
The new imports align with the PR objective of removing client.Context and introducing the new context structure.
26-26: LGTM: Well-defined constant for Bech32 prefix.
The constant follows Go naming conventions and clearly indicates its purpose.
client/v2/tx/tx.go (2)
29-34: Consider grouping parameters into a configuration struct
The function signature could be simplified by grouping related parameters into a configuration struct. This would make the function more maintainable and easier to extend in the future.
65-69: Verify all callers are updated with the new signature
The function signature has changed significantly. Ensure all calling code has been updated to provide the required context and connection parameters.
client/v2/autocli/msg.go (2)
17-17: LGTM: New imports align with PR objectives
The addition of v2tx and transaction packages aligns with the goal of removing client.Context and modernizing the transaction handling approach.
Also applies to: 19-19
233-243: Enhance documentation and verify integration plan
While the implementation looks good, consider:
- Expanding the documentation to explain the benefits of v2 transaction handling
- Adding examples of usage in the documentation
- Documenting the expected timeline for integration with BuildMsgMethodCommand
Let's verify the planned integration points:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
tests/systemtests/mint_test.go (4)
Line range hint
13-35: Consider using constants for inflation values.The test setup is well-structured, but the inflation values ("1.00") could be defined as constants at the package level for better maintainability and reusability.
+const ( + defaultInflation = "1.00" + defaultInflationMax = "1.00" +) func TestMintQueries(t *testing.T) { // ... sut.ModifyGenesisJSON(t, func(genesis []byte) []byte { - state, err := sjson.Set(string(genesis), "app_state.mint.minter.inflation", "1.00") + state, err := sjson.Set(string(genesis), "app_state.mint.minter.inflation", defaultInflation) require.NoError(t, err) return []byte(state) }, func(genesis []byte) []byte { - state, err := sjson.Set(string(genesis), "app_state.mint.params.inflation_max", "1.00") + state, err := sjson.Set(string(genesis), "app_state.mint.params.inflation_max", defaultInflationMax) require.NoError(t, err) return []byte(state) }, )
Line range hint
37-46: Track the TODO comment regarding v1/v2 query differences.The comment references issue #22302 regarding differences in query values between v1 and v2. Let's ensure this is properly tracked.
Would you like me to create a GitHub issue to track this TODO and link it to issue #22302?
Line range hint
48-78: Refactor hardcoded JSON responses to use structured test data.Using hardcoded JSON strings makes the tests brittle and harder to maintain. Consider using structs and marshaling them to JSON.
+type MintParams struct { + Params struct { + MintDenom string `json:"mint_denom"` + InflationRateChange string `json:"inflation_rate_change"` + InflationMax string `json:"inflation_max"` + InflationMin string `json:"inflation_min"` + GoalBonded string `json:"goal_bonded"` + BlocksPerYear string `json:"blocks_per_year"` + MaxSupply string `json:"max_supply"` + } `json:"params"` +} func TestMintQueries(t *testing.T) { // ... + expectedParams := MintParams{ + Params: struct { + MintDenom string `json:"mint_denom"` + InflationRateChange string `json:"inflation_rate_change"` + InflationMax string `json:"inflation_max"` + InflationMin string `json:"inflation_min"` + GoalBonded string `json:"goal_bonded"` + BlocksPerYear string `json:"blocks_per_year"` + MaxSupply string `json:"max_supply"` + }{ + MintDenom: "stake", + InflationRateChange: "0.130000000000000000", + InflationMax: "1.000000000000000000", + // ... other fields + }, + } + paramsResp, err := json.Marshal(expectedParams) + require.NoError(t, err)
Line range hint
80-91: Track the TODO regarding gRPC gateway implementation in v2.The tests are currently skipped for v2 pending gRPC gateway implementation. This needs to be tracked to ensure tests are enabled once implemented.
Would you like me to create a GitHub issue to track enabling these tests once the gRPC gateway is implemented in v2?
client/v2/autocli/common.go (2)
383-384: Address the TODO: Define default behavior when 'grpc-addr' is not setThere is a
TODOcomment indicating that the default is CometBFT whengrpc-addris not specified. It's important to finalize this behavior to avoid confusion and ensure consistent functionality. Consider removing theTODOcomment after confirming the default.Would you like assistance in implementing the default behavior or updating the documentation? I can help generate the required code changes or open a new GitHub issue to track this task.
305-349: Refactor 'setFlagsFromConfig' to eliminate code duplicationThe
setFlagsFromConfigfunction contains repetitive code patterns for setting flags. Refactoring to remove duplication will enhance maintainability and readability.Consider creating a helper function to streamline flag setting:
func setFlagIfNotChanged(cmd *cobra.Command, flagName, value string) error { if cmd.Flags().Lookup(flagName) != nil && !cmd.Flags().Changed(flagName) { if err := cmd.Flags().Set(flagName, value); err != nil { return err } } return nil }Refactor
setFlagsFromConfigusing the helper function:func (b *Builder) setFlagsFromConfig(cmd *cobra.Command) error { conf, err := config.CreateClientConfigFromFlags(cmd.Flags()) if err != nil { return err } - if cmd.Flags().Lookup("chain-id") != nil && !cmd.Flags().Changed("chain-id") { - if err = cmd.Flags().Set("chain-id", conf.ChainID); err != nil { - return err - } - } + if err := setFlagIfNotChanged(cmd, "chain-id", conf.ChainID); err != nil { + return err + } // Repeat for other flags...
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
client/v2/autocli/common.go(3 hunks)client/v2/autocli/config/toml.go(1 hunks)client/v2/autocli/context/context.go(1 hunks)client/v2/go.mod(4 hunks)tests/systemtests/mint_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/v2/autocli/config/toml.go
- client/v2/autocli/context/context.go
- client/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/mint_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
tests/systemtests/mint_test.go (2)
Line range hint 1-11: LGTM! Build constraint and imports are properly structured.
The build tag is correctly placed and the imports are clean.
Line range hint 93-100: Refactor CLI tests to use the same structured test data.
The CLI tests should use the same structured test data suggested earlier for consistency and maintainability.
client/v2/autocli/common.go (1)
279-279:
Fix typo in 'EnabledSignmodes' field name
In the clientCtx struct initialization, the field EnabledSignmodes should be EnabledSignModes to match Go naming conventions and ensure consistency. This typo could lead to issues with struct field mapping.
Apply this diff to correct the field name:
clientCtx := clientcontext.Context{
Flags: cmd.Flags(),
AddressCodec: b.AddressCodec,
ValidatorAddressCodec: b.ValidatorAddressCodec,
ConsensusAddressCodec: b.ConsensusAddressCodec,
Cdc: b.Cdc,
Printer: printer,
Keyring: k,
- EnabledSignmodes: b.EnabledSignModes,
+ EnabledSignModes: b.EnabledSignModes,
}Likely invalid or redundant comment.
| } | ||
|
|
||
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) | ||
| } |
There was a problem hiding this comment.
Correct the use of 'grpc.NewClient' to 'grpc.Dial'
The function grpc.NewClient does not exist in the google.golang.org/grpc package. The correct function to establish a gRPC connection is grpc.Dial.
Apply this diff to fix the issue:
return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...)
+return grpc.Dial(addr, grpc.WithTransportCredentials(creds))Ensure you handle the returned *grpc.ClientConn and associated errors appropriately.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -147,20 +146,18 @@ func (a *consensusAddressValue) Set(s string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // getKeyringFromCtx retrieves the keyring from the provided context. | |||
| // If the context is nil or does not contain a valid client context, | |||
There was a problem hiding this comment.
Shall we fallback and check client.Context v1 of we cannot find an autocli client context?
Just to be sure. Maybe it isn't needed. Your call.
| // NewKeyringFromFlags creates a new Keyring instance based on command-line flags. | ||
| // It retrieves the keyring backend and directory from flags, creates a new keyring, | ||
| // and wraps it with an AutoCLI-compatible interface. | ||
| func NewKeyringFromFlags(flagSet *pflag.FlagSet, ac address.Codec, input io.Reader, cdc codec.Codec, opts ...keyring.Option) (Keyring, error) { |
There was a problem hiding this comment.
This should take the autocli context instead right?
client/v2/autocli/context/context.go
Outdated
|
|
||
| Cdc codec.Codec | ||
|
|
||
| Printer *print.Printer |
There was a problem hiding this comment.
Not super of fan of the printer and as well to have that in the context. Is that really necessary?
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
client/v2/autocli/context/context.go (1)
28-30: Consider consolidating address codec fieldsThe struct currently has three separate address codec fields. Consider consolidating these into a single address codec interface or struct to reduce the number of fields and improve maintainability.
client/v2/internal/print/printer.go (3)
13-16: Consider exporting and documenting output format constantsThese constants represent valid output formats and could be useful to package users. Consider:
- Exporting them (capitalize first letter)
- Adding documentation comments
-const ( - jsonOutput = "json" - textOutput = "text" -) +// OutputFormat represents the supported output formats +const ( + // JSONOutput indicates JSON formatting + JSONOutput = "json" + // TextOutput indicates text (YAML) formatting + TextOutput = "text" +)
18-22: Add field documentation to Printer structConsider adding documentation for the struct fields to improve code clarity.
// Printer handles formatted output of different types of data type Printer struct { + // Output is the destination writer for the formatted output Output io.Writer + // OutputFormat specifies the format type ("json" or "text") OutputFormat string }
24-39: Enhance error messages in NewPrinterConsider providing more descriptive error messages that include the valid format options.
if outputFormat != jsonOutput && outputFormat != textOutput { - return nil, fmt.Errorf("unsupported output format: %s", outputFormat) + return nil, fmt.Errorf("unsupported output format %q: must be either %q or %q", + outputFormat, jsonOutput, textOutput) }client/v2/autocli/app.go (1)
49-50: Add documentation for TxConfigOpts fieldFor consistency with other fields, please add a descriptive comment for the TxConfigOpts field explaining its purpose and usage.
client/v2/tx/tx.go (2)
34-37: Enhance error handling with wrapped errorsConsider wrapping the error with additional context to help with debugging:
- clientCtx, err := clientcontext.ClientContextFromGoContext(ctx) - if err != nil { - return nil, err - } + clientCtx, err := clientcontext.ClientContextFromGoContext(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get client context: %w", err) + }
191-192: Improve error messages throughout the fileSeveral error returns could benefit from more context. Consider wrapping errors with additional information:
- return nil, errors.New("failed to encode transaction: tx json encoder is nil") + return nil, fmt.Errorf("transaction encoding failed: JSON encoder not initialized") - return nil, err + return nil, fmt.Errorf("failed to encode transaction bytes: %w", err)Also applies to: 226-227
client/v2/autocli/config/config.go (4)
83-91: Use a more descriptive variable name instead ofv.The variable
vrepresents a Viper instance. Renaming it to something more descriptive, likeviperInstance, improves code readability.Apply this diff to rename the variable:
-func CreateClientConfigFromFlags(set *pflag.FlagSet) (*Config, error) { - v := viper.New() +func CreateClientConfigFromFlags(set *pflag.FlagSet) (*Config, error) { + viperInstance := viper.New() executableName, err := os.Executable() if err != nil { return nil, err } - v.SetEnvPrefix(path.Base(executableName)) - v.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) - v.AutomaticEnv() + viperInstance.SetEnvPrefix(path.Base(executableName)) + viperInstance.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) + viperInstance.AutomaticEnv() - return CreateClientConfig(homeDir, chainID, v) + return CreateClientConfig(homeDir, chainID, viperInstance)
114-114: Rename parametervto a more descriptive name inreadConfig.The parameter
vin thereadConfigfunction represents a Viper instance. Renaming it toviperInstanceenhances clarity and consistency.Apply this diff to rename the parameter:
-func readConfig(configPath string, v *viper.Viper) (*Config, error) { +func readConfig(configPath string, viperInstance *viper.Viper) (*Config, error) { viperInstance.AddConfigPath(configPath) viperInstance.SetConfigName("client") viperInstance.SetConfigType("toml") - if err := v.ReadInConfig(); err != nil { + if err := viperInstance.ReadInConfig(); err != nil { return nil, err }And update the call in
CreateClientConfig:// Inside CreateClientConfig function - conf, err := readConfig(configPath, v) + conf, err := readConfig(configPath, viperInstance)
32-41: Initialize theGRPCfield inDefaultConfig.The
GRPCfield in theConfigstruct is not initialized inDefaultConfig, which may lead to zero values in the GRPC settings. Initializing it with default values ensures expected behavior.Apply this diff to initialize the
GRPCfield:func DefaultConfig() *Config { return &Config{ ChainID: "", KeyringBackend: "os", KeyringDefaultKeyName: "", Output: "text", Node: "tcp://localhost:26657", BroadcastMode: "sync", + GRPC: GRPCConfig{ + Address: "localhost:9090", + Insecure: false, + }, } }
17-30: Ensure consistent spacing in struct tags.There are inconsistent spaces in the struct tags of
ConfigandGRPCConfig. Removing extra spaces enhances code readability.Apply this diff to fix the spacing:
type Config struct { ChainID string `mapstructure:"chain-id" toml:"chain-id" comment:"The chain ID of the blockchain network"` KeyringBackend string `mapstructure:"keyring-backend" toml:"keyring-backend" comment:"The keyring backend to use (os|file|kwallet|pass|test|memory)"` KeyringDefaultKeyName string `mapstructure:"keyring-default-keyname" toml:"keyring-default-keyname" comment:"The default key name to use for signing transactions"` Output string `mapstructure:"output" toml:"output" comment:"The output format for queries (text|json)"` Node string `mapstructure:"node" toml:"node" comment:"The RPC endpoint URL for the node to connect to"` BroadcastMode string `mapstructure:"broadcast-mode" toml:"broadcast-mode" comment:"How transactions are broadcast to the network (sync|async|block)"` - GRPC GRPCConfig `mapstructure:",squash" comment:"The gRPC client configuration"` + GRPC GRPCConfig `mapstructure:",squash" toml:"" comment:"The gRPC client configuration"` } // GRPCConfig holds the gRPC client configuration. type GRPCConfig struct { - Address string `mapstructure:"grpc-address" toml:"grpc-address" comment:"The gRPC server address to connect to"` - Insecure bool `mapstructure:"grpc-insecure" toml:"grpc-insecure" comment:"Allow gRPC over insecure connections"` + Address string `mapstructure:"grpc-address" toml:"grpc-address" comment:"The gRPC server address to connect to"` + Insecure bool `mapstructure:"grpc-insecure" toml:"grpc-insecure" comment:"Allow gRPC over insecure connections"` }client/v2/broadcast/comet/client_conn.go (4)
32-34: ImplementNewStreammethod or document its absenceThe
NewStreammethod currently returns an error indicating "not implemented". SinceCometBFTBroadcasterimplementsgrpc.ClientConnInterface, which requires this method, consider providing an implementation if streaming support is intended in the future. If streaming is not supported, it would be helpful to document this explicitly for clarity.
38-38: Remove unnecessary named return parametererrinInvokemethodThe named return parameter
errin theInvokemethod is unnecessary since it is not used for deferred error handling or modifying the error before returning. Omitting it improves readability.Apply this diff to remove the named return parameter:
-func (c *CometBFTBroadcaster) Invoke(ctx context.Context, method string, req, reply interface{}, opts ...grpc.CallOption) (err error) { +func (c *CometBFTBroadcaster) Invoke(ctx context.Context, method string, req, reply interface{}, opts ...grpc.CallOption) error {
124-134: Handle additional SDK error codes insdkErrorToGRPCErrorCurrently,
sdkErrorToGRPCErrorhandles a limited set of SDK error codes. Consider mapping more specific SDK errors to appropriate gRPC status codes to provide better error granularity and improve client-side error handling.For example, you might handle
sdkerrors.ErrInsufficientFundsorsdkerrors.ErrTimeout:default: + case sdkerrors.ErrInsufficientFunds.ABCICode(): + return status.Error(codes.ResourceExhausted, resp.Log) + case sdkerrors.ErrTimeout.ABCICode(): + return status.Error(codes.DeadlineExceeded, resp.Log) return status.Error(codes.Unknown, resp.Log)
140-147: SimplifygetRPCCodecby using type assertion only when necessaryIn
getRPCCodec, you perform a type assertion onc.cdcto check if it implementscodec.GRPCCodecProvider. Since the fallback is to create a newProtoCodec, consider simplifying the logic or documenting why this approach is necessary to avoid potential confusion.client/v2/autocli/common.go (2)
301-352: Refactor 'setFlagsFromConfig' to eliminate repetitive codeThe
setFlagsFromConfigfunction contains repetitive code blocks for setting each flag. Consider refactoring to reduce duplication, which will enhance readability and maintainability.Here's how you might refactor the code:
flagSettings := []struct { flagName string configValue string }{ {"chain-id", conf.ChainID}, {"keyring-backend", conf.KeyringBackend}, {"from", conf.KeyringDefaultKeyName}, {"output", conf.Output}, {"node", conf.Node}, {"broadcast-mode", conf.BroadcastMode}, {"grpc-addr", conf.GRPC.Address}, {"grpc-insecure", strconv.FormatBool(conf.GRPC.Insecure)}, } for _, fs := range flagSettings { if cmd.Flags().Lookup(fs.flagName) != nil && !cmd.Flags().Changed(fs.flagName) { if err := cmd.Flags().Set(fs.flagName, fs.configValue); err != nil { return err } } }This refactoring adheres to the DRY (Don't Repeat Yourself) principle.
387-387: Address the TODO: Implement default CometBFT broadcasterThe TODO comment indicates that the default client connection should be CometBFT when
grpc-addris not set. Currently, the implementation might not reflect the intended default behavior.Would you like assistance in implementing the default CometBFT broadcaster to ensure the application behaves as expected?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (12)
client/v2/autocli/app.go(4 hunks)client/v2/autocli/builder.go(2 hunks)client/v2/autocli/common.go(3 hunks)client/v2/autocli/config/config.go(1 hunks)client/v2/autocli/context/context.go(1 hunks)client/v2/autocli/msg.go(2 hunks)client/v2/broadcast/comet/client_conn.go(1 hunks)client/v2/broadcast/comet/comet.go(1 hunks)client/v2/broadcast/comet/comet_test.go(1 hunks)client/v2/internal/print/printer.go(1 hunks)client/v2/offchain/cli.go(5 hunks)client/v2/tx/tx.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/v2/autocli/builder.go
- client/v2/broadcast/comet/comet.go
- client/v2/broadcast/comet/comet_test.go
- client/v2/offchain/cli.go
🧰 Additional context used
📓 Path-based instructions (8)
client/v2/autocli/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/config/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/context/context.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/msg.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/broadcast/comet/client_conn.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/internal/print/printer.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
client/v2/autocli/context/context.go (1)
1-15: Consider relocating package to client/v2/context
As suggested in the previous review, consider moving this package to client/v2/context for better organization and to avoid nesting under autocli.
client/v2/internal/print/printer.go (3)
1-11: LGTM: Clean package structure and imports
The package structure is well-organized with appropriate imports.
41-44: LGTM: Clean and concise implementation
The method correctly delegates to PrintBytes after converting the string to bytes.
46-49: LGTM: Appropriate handling of raw JSON
The method correctly handles raw JSON messages without unnecessary marshaling.
client/v2/autocli/app.go (2)
74-86: LGTM! Clean integration of new fields
The changes properly integrate the new codec and address fields while maintaining the original functionality. The removal of ClientCtx dependencies is handled correctly.
183-184: Missing initialization of required fields
The function still needs to initialize:
- AddressCodec
- ValidatorAddressCodec
- ConsensusAddressCodec
- TxConfigOpts
client/v2/autocli/msg.go (2)
17-17: LGTM: New imports align with v2 transaction handling.
The addition of v2tx and transaction imports supports the transition to v2 transaction handling.
Also applies to: 19-19
233-243: Document the migration timeline for v2 transaction handling.
The function is well-implemented but currently unused. While the nolint directive is properly documented, it would be helpful to:
- Add a TODO comment with the expected timeline for updating BuildMsgMethodCommand
- Document any breaking changes that will occur when this transition happens
client/v2/tx/tx.go
Outdated
| func BroadcastTx( | ||
| ctx context.Context, | ||
| printer *print.Printer, | ||
| txf Factory, | ||
| broadcaster broadcast.Broadcaster, | ||
| skipConfirm bool, | ||
| msgs ...transaction.Msg, | ||
| ) ([]byte, error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing printer dependency
The function currently handles both transaction broadcasting and output printing. Consider separating these concerns by returning the transaction data and letting the caller handle the printing. This would improve modularity and make the function more reusable.
client/v2/tx/tx.go
Outdated
| if !skipConfirm { | ||
| encoder := txf.txConfig.TxJSONEncoder() | ||
| if encoder == nil { | ||
| return errors.New("failed to encode transaction: tx json encoder is nil") | ||
| return nil, errors.New("failed to encode transaction: tx json encoder is nil") | ||
| } | ||
|
|
||
| unsigTx, err := txf.getTx() | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
| txBytes, err := encoder(unsigTx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to encode transaction: %w", err) | ||
| return nil, fmt.Errorf("failed to encode transaction: %w", err) | ||
| } | ||
|
|
||
| if err := clientCtx.PrintRaw(txBytes); err != nil { | ||
| if err := printer.PrintRaw(txBytes); err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\n%s\n", err, txBytes) | ||
| } | ||
|
|
||
| buf := bufio.NewReader(os.Stdin) | ||
| ok, err := input.GetConfirmation("confirm transaction before signing and broadcasting", buf, os.Stderr) | ||
| if err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\ncanceled transaction\n", err) | ||
| return err | ||
| return nil, err | ||
| } | ||
| if !ok { | ||
| _, _ = fmt.Fprintln(os.Stderr, "canceled transaction") | ||
| return nil | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor confirmation logic for better error handling
The confirmation logic contains multiple error paths with inconsistent error handling. Some errors are printed to stderr while others are returned. Consider:
- Consolidating error handling
- Moving confirmation logic to a separate function
- Using a consistent approach for error reporting
+func confirmTransaction(txBytes []byte, stdin *bufio.Reader) error {
+ if err := printer.PrintRaw(txBytes); err != nil {
+ return fmt.Errorf("failed to print transaction: %w", err)
+ }
+
+ ok, err := input.GetConfirmation("confirm transaction before signing and broadcasting", stdin, os.Stderr)
+ if err != nil {
+ return fmt.Errorf("failed to get confirmation: %w", err)
+ }
+ if !ok {
+ return ErrUserDeclined
+ }
+ return nil
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
client/v2/tx/tx.go (5)
188-233: Refactor confirmation and printing logic for improved clarityThe confirmation and printing logic within
BroadcastTxcombines multiple concerns and uses global state (os.Stdin,os.Stdout,os.Stderr). This can make testing and maintenance more challenging. Consider refactoring by:
- Extracting the confirmation prompt into a separate function.
- Passing input/output streams as parameters instead of using global
osvariables.- Separating the printing logic from transaction signing and broadcasting.
This adheres to the Single Responsibility Principle and enhances testability.
Example refactor:
func promptConfirmation(prompt string, in io.Reader, out io.Writer) (bool, error) { buf := bufio.NewReader(in) return input.GetConfirmation(prompt, buf, out) } // Update the BroadcastTx function signature to accept input/output parameters func BroadcastTx( ctx context.Context, txf Factory, broadcaster broadcast.Broadcaster, skipConfirm bool, in io.Reader, out, errOut io.Writer, msgs ...transaction.Msg, ) ([]byte, error) { // ... existing logic ... if !skipConfirm { // Use the provided output writer instead of os.Stdout/os.Stderr clientCtx, err := clientcontext.ClientContextFromGoContext(ctx) if err == nil { // ... existing printing logic ... } else { _, err = out.Write(txBytes) if err != nil { return nil, err } } // Use the promptConfirmation function with provided streams ok, err := promptConfirmation("confirm transaction before signing and broadcasting", in, errOut) if err != nil { _, _ = fmt.Fprintf(errOut, "error: %v\ncanceled transaction\n", err) return nil, err } if !ok { _, _ = fmt.Fprintln(errOut, "canceled transaction") return nil, nil } } // ... existing logic ... }
28-33: Function parameters exceed recommended limitThe
GenerateOrBroadcastTxCLIWithBroadcasterfunction has 5 parameters, which is at the recommended limit according to the Uber Go Style Guide. However, considering future maintainability, grouping related parameters into a struct could improve readability and scalability.Consider defining a struct for related parameters:
type TxCLIParams struct { Context context.Context Conn grpc.ClientConn Broadcaster broadcast.Broadcaster Msgs []transaction.Msg } func GenerateOrBroadcastTxCLIWithBroadcaster(params TxCLIParams) ([]byte, error) { // Use params.Context, params.Conn, etc. }
66-68: Consistent parameter usage across functionsThe
GenerateOrBroadcastTxCLIfunction acceptsconn grpc.ClientConnandmsgs ...transaction.Msg. For consistency and future extensibility, consider aligning this function's parameters with the refactoredGenerateOrBroadcastTxCLIWithBroadcaster, possibly by using a params struct as suggested earlier.
172-174: Consider context cancellation during broadcastingIn the
BroadcastTxfunction, theskipConfirmflag controls user confirmation. However, ifskipConfirmisfalse, and the user takes too long to confirm, it'd be beneficial to honor any cancellation or timeout from the providedctxto avoid hanging processes.Ensure that any blocking operations respect the
ctx.Done()channel and handle context cancellation appropriately.
215-217: Handle errors consistently when printing to stderrIn case of an error during printing, the error is printed to
os.Stderr, and then the error is returned. Make sure that error messages don't leak sensitive information and that the error handling is consistent throughout the function.Review the error messages for consistency and clarity.
client/v2/autocli/common.go (1)
314-360: Refactor to eliminate code duplication in flag settingThe
setFlagsFromConfigfunction contains repetitive code blocks for setting command flags from the configuration. Refactoring this code to iterate over a map of flag names and their corresponding config values can reduce duplication and improve maintainability.You can apply the following diff to refactor the function:
func (b *Builder) setFlagsFromConfig(cmd *cobra.Command) error { conf, err := config.CreateClientConfigFromFlags(cmd.Flags()) if err != nil { return err } + flagsToSet := map[string]string{ + "chain-id": conf.ChainID, + "keyring-backend": conf.KeyringBackend, + "from": conf.KeyringDefaultKeyName, + "output": conf.Output, + "node": conf.Node, + "broadcast-mode": conf.BroadcastMode, + "grpc-addr": conf.GRPC.Address, + "grpc-insecure": strconv.FormatBool(conf.GRPC.Insecure), + } + for flagName, value := range flagsToSet { + if cmd.Flags().Lookup(flagName) != nil && !cmd.Flags().Changed(flagName) { + if err = cmd.Flags().Set(flagName, value); err != nil { + return err + } + } + } return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
client/v2/autocli/common.go(3 hunks)client/v2/autocli/context/context.go(1 hunks)client/v2/go.mod(4 hunks)client/v2/tx/tx.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/v2/autocli/context/context.go
- client/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/tx/tx.go
Outdated
| } | ||
|
|
||
| if err := clientCtx.PrintRaw(txBytes); err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\n%s\n", err, txBytes) | ||
| clientCtx, err := clientcontext.ClientContextFromGoContext(ctx) |
There was a problem hiding this comment.
Yeah so broadcast tx shouldn't print anything as it returns the output. We should let the caller (which is autocli or any other caller) to handle the printing.
There was a problem hiding this comment.
I know this command is somewhat interactive, as it can take an user input, but that should be handled by something else (autocli or client) imho not here.
There was a problem hiding this comment.
We could have an helper for generate, and one for sign and broadcast and kill all that input logic. In autocli we can easily handle the printing + sign+broadcast in two seperate call.
client/v2/autocli/common.go
Outdated
| return err | ||
| } | ||
|
|
||
| if cmd.Flags().Lookup("chain-id") != nil && !cmd.Flags().Changed("chain-id") { |
There was a problem hiding this comment.
nit, can we put all those flags in the internal/flags package with the rest?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
client/v2/internal/flags/flags.go (3)
22-23: Enhance documentation for keyring backend optionsConsider listing all supported keyring backend types in the documentation comment (e.g., "os", "file", "test", etc.) to provide better guidance for users.
- // FlagKeyringBackend is the flag to specify which backend to use for the keyring (e.g. os, file, test). + // FlagKeyringBackend is the flag to specify which backend to use for the keyring. + // Supported backends: os (default), file, test, memory, kwallet, pass, secret-service
31-32: Enhance documentation for broadcast mode optionsConsider listing all supported broadcast modes in the documentation comment to provide better guidance for users.
- // FlagBroadcastMode is the flag to specify the broadcast mode for transactions. + // FlagBroadcastMode is the flag to specify the broadcast mode for transactions. + // Supported modes: sync (default), async, block
36-37: Add security warning for insecure gRPC connectionsConsider adding a security warning in the documentation for the
grpc-insecureflag.- // FlagGrpcInsecure is the flag to allow insecure gRPC connections. + // FlagGrpcInsecure is the flag to allow insecure gRPC connections. + // WARNING: This flag disables transport security. Use only in trusted networks.client/v2/internal/print/printer.go (2)
15-18: Add documentation for constants.Consider adding documentation comments for the constants to explain their purpose and valid values.
const ( + // jsonOutput represents JSON output format jsonOutput = flags.OutputFormatJSON + // textOutput represents text/YAML output format textOutput = flags.OutputFormatText )
20-24: Add documentation for struct fields.Consider adding documentation for the struct fields to improve code clarity.
// Printer handles formatted output of different types of data type Printer struct { + // Output is the writer where formatted data will be written Output io.Writer + // OutputFormat specifies the format (json or text) for the output OutputFormat string }client/v2/tx/tx.go (4)
29-32: Enhance error context in ClientContextFromGoContext error handlingConsider wrapping the error with additional context to help with debugging.
clientCtx, err := clientcontext.ClientContextFromGoContext(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get client context: %w", err) }
63-71: Improve error context in error handling chainConsider wrapping errors with additional context for better debugging.
c, err := clientcontext.ClientContextFromGoContext(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get client context: %w", err) } cometBroadcaster, err := getCometBroadcaster(c.Cdc, c.Flags) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create comet broadcaster: %w", err) }
Line range hint
86-103: Add validation for gRPC connectionThe function should validate that the gRPC connection is not nil before using it.
func newFactory(ctx clientcontext.Context, conn grpc.ClientConn) (Factory, error) { + if conn == nil { + return Factory{}, fmt.Errorf("gRPC connection cannot be nil") + }
129-146: Enhance error context in generateOnly and dryRun functionsAdd more context to errors for better debugging experience.
func generateOnly(txf Factory, msgs ...transaction.Msg) ([]byte, error) { uTx, err := txf.UnsignedTxString(msgs...) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to generate unsigned transaction: %w", err) } return []byte(uTx), nil } func dryRun(txf Factory, msgs ...transaction.Msg) ([]byte, error) { _, gas, err := txf.Simulate(msgs...) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to simulate transaction: %w", err) } return []byte(fmt.Sprintf("%s\n", GasEstimateResponse{GasEstimate: gas})), nil }client/v2/autocli/common.go (2)
295-308: Simplify error handling inpreRunEThe current implementation returns
nilafter checking for an error, which can be simplified for clarity.Apply this diff to simplify the function:
func (b *Builder) preRunE() func(cmd *cobra.Command, args []string) error { return func(cmd *cobra.Command, args []string) error { - err := b.setFlagsFromConfig(cmd) - if err != nil { - return err - } - - return nil + return b.setFlagsFromConfig(cmd) } }This refactor removes unnecessary lines and makes the code more concise.
289-289: Correct field nameEnabledSignmodestoEnabledSignModesThe struct field
EnabledSignmodesshould be renamed toEnabledSignModesto maintain consistent naming conventions and improve code readability.Apply the following diff:
clientCtx := clientcontext.Context{ Flags: cmd.Flags(), AddressCodec: b.AddressCodec, ValidatorAddressCodec: b.ValidatorAddressCodec, ConsensusAddressCodec: b.ConsensusAddressCodec, Cdc: b.Cdc, Keyring: k, - EnabledSignmodes: signModesToApiSignModes(b.EnabledSignModes), + EnabledSignModes: signModesToApiSignModes(b.EnabledSignModes), }Ensure that all references to this field are updated accordingly.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
client/v2/autocli/common.go(3 hunks)client/v2/autocli/context/context.go(1 hunks)client/v2/internal/flags/flags.go(2 hunks)client/v2/internal/print/printer.go(1 hunks)client/v2/offchain/cli.go(5 hunks)client/v2/tx/tx.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/v2/autocli/context/context.go
- client/v2/offchain/cli.go
🧰 Additional context used
📓 Path-based instructions (4)
client/v2/autocli/common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/internal/flags/flags.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/internal/print/printer.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (12)
client/v2/internal/flags/flags.go (2)
5-6: LGTM: Clear documentation and consistent naming
The FlagChainID constant follows the established naming pattern and includes clear documentation.
Line range hint 5-37: Verify complete coverage of client.Context functionality
Let's ensure these flags cover all the necessary functionality previously provided by client.Context.
✅ Verification successful
Let me gather more information about the flags and their usage to ensure complete coverage.
Let me do one final check to understand how the new v2 client handles these configurations.
Based on the gathered information, I can now provide the final response:
Flags adequately cover client.Context functionality with improved organization
The defined flags in flags.go appropriately cover the essential functionality from the original client.Context, with improvements:
- Core connectivity flags:
chain-id,node,grpc-addr,grpc-insecure,broadcast-mode - Keyring configuration:
keyring-dir,keyring-backend - Output control:
output,no-indent,no-prompt - Transaction signing:
from - Special operations:
no-proposal
The new implementation shows a cleaner separation of concerns, with dedicated initialization functions for specific components (e.g., NewFactoryFromFlagSet, NewKeyringFromFlags) that properly utilize these flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all client.Context fields have corresponding flags
# Search for client.Context fields in the codebase
echo "Searching for client.Context fields..."
rg -A 10 "type Context struct" "client/"
# Search for any remaining references to client.Context
echo "Checking for remaining client.Context references..."
rg "client\.Context" "client/v2/"
Length of output: 2267
Script:
#!/bin/bash
# Check how these flags are used and if they cover all client.Context fields
# Search for flag usage in v2
echo "Searching for flag usage in v2..."
rg "AddFlag|MarkFlagRequired" "client/v2/"
# Search for any new configuration or context setup
echo "Searching for context/config setup..."
rg "NewContext|WithContext|Config" "client/v2/internal/"
# Check for any potential missing functionality
echo "Checking client.Context fields usage in v2..."
rg "Client|GRPCClient|ChainID|Codec|InterfaceRegistry|Input|Keyring|KeyringOptions" "client/v2/internal/"
Length of output: 8659
Script:
#!/bin/bash
# Check for client configuration and initialization
# Look for client initialization and configuration
echo "Checking client initialization..."
rg -A 5 "func New|func Init" "client/v2/"
# Look for how flags are bound to configuration
echo "Checking flag binding..."
rg -A 5 "BindFlags|AddFlags" "client/v2/"
# Check for any configuration structs
echo "Checking configuration structures..."
ast-grep --pattern 'type $_ struct {
$$$
}' --lang go client/v2/internal/config/
Length of output: 6307
client/v2/internal/print/printer.go (5)
1-13: LGTM! Well-organized imports and package structure.
The imports are properly grouped and the package name follows Go conventions.
26-41: LGTM! Constructor implementation is sound.
The constructor properly validates the output format and handles errors appropriately.
43-46: LGTM! Clean and concise implementation.
The method provides a convenient way to print strings while reusing PrintBytes logic.
48-51: LGTM! Appropriate handling of raw JSON messages.
The method correctly handles raw JSON messages by delegating to PrintBytes.
53-84: Skip comment as covered by previous review.
A comprehensive refactoring suggestion for this method was provided in a previous review and is still valid.
client/v2/autocli/common.go (5)
258-263: Function outOrStdoutFormat is correctly refactored
The function properly initializes a new printer and uses it to print the output bytes. The error handling is appropriate.
265-294: Function getContext is well-implemented
The getContext function effectively creates a new context with the necessary client information. It initializes the keyring correctly based on the command flags and constructs a clientcontext.Context with all required fields.
309-338: Efficient flag setting in setFlagsFromConfig
The setFlagsFromConfig function effectively sets command flags from the configuration while checking if they have been changed by the user. Error handling is appropriately managed.
380-386: Function signModesToApiSignModes is correctly implemented
The function properly converts a slice of signing.SignMode to a slice of apitxsigning.SignMode. The implementation is efficient and follows best practices.
345-346:
Set explicit server name in TLS configuration
When creating TLS credentials with credentials.NewTLS, the ServerName field in tls.Config is not set. This might cause hostname verification to be skipped, potentially reducing the security of the TLS connection.
Modify the TLS configuration to include the server name derived from the address:
if !insecure {
- creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
+ tlsConfig := &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ ServerName: extractHostname(addr),
+ }
+ creds = credentials.NewTLS(tlsConfig)
}Implement a helper function extractHostname to parse and return the hostname from the addr variable.
Likely invalid or redundant comment.
| func getCometBroadcaster(cdc codec.Codec, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) { | ||
| url, _ := flagSet.GetString(flags.FlagNode) | ||
| mode, _ := flagSet.GetString(flags.FlagBroadcastMode) | ||
| return comet.NewCometBFTBroadcaster(url, mode, cdc) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required flag values
The function should validate that required flag values are present and valid before using them.
func getCometBroadcaster(cdc codec.Codec, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) {
+ if flagSet == nil {
+ return nil, fmt.Errorf("flag set cannot be nil")
+ }
+ if cdc == nil {
+ return nil, fmt.Errorf("codec cannot be nil")
+ }
url, _ := flagSet.GetString(flags.FlagNode)
+ if url == "" {
+ return nil, fmt.Errorf("node URL is required")
+ }
mode, _ := flagSet.GetString(flags.FlagBroadcastMode)
+ if mode == "" {
+ return nil, fmt.Errorf("broadcast mode is required")
+ }
return comet.NewCometBFTBroadcaster(url, mode, cdc)
}📝 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 getCometBroadcaster(cdc codec.Codec, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) { | |
| url, _ := flagSet.GetString(flags.FlagNode) | |
| mode, _ := flagSet.GetString(flags.FlagBroadcastMode) | |
| return comet.NewCometBFTBroadcaster(url, mode, cdc) | |
| func getCometBroadcaster(cdc codec.Codec, flagSet *pflag.FlagSet) (broadcast.Broadcaster, error) { | |
| if flagSet == nil { | |
| return nil, fmt.Errorf("flag set cannot be nil") | |
| } | |
| if cdc == nil { | |
| return nil, fmt.Errorf("codec cannot be nil") | |
| } | |
| url, _ := flagSet.GetString(flags.FlagNode) | |
| if url == "" { | |
| return nil, fmt.Errorf("node URL is required") | |
| } | |
| mode, _ := flagSet.GetString(flags.FlagBroadcastMode) | |
| if mode == "" { | |
| return nil, fmt.Errorf("broadcast mode is required") | |
| } | |
| return comet.NewCometBFTBroadcaster(url, mode, cdc) | |
| } |
| func BroadcastTx(ctx context.Context, txf Factory, broadcaster broadcast.Broadcaster, msgs ...transaction.Msg) ([]byte, error) { | ||
| if txf.simulateAndExecute() { | ||
| err := txf.calculateGas(msgs...) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| err := txf.BuildUnsignedTx(msgs...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !clientCtx.SkipConfirm { | ||
| encoder := txf.txConfig.TxJSONEncoder() | ||
| if encoder == nil { | ||
| return errors.New("failed to encode transaction: tx json encoder is nil") | ||
| } | ||
|
|
||
| unsigTx, err := txf.getTx() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| txBytes, err := encoder(unsigTx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to encode transaction: %w", err) | ||
| } | ||
|
|
||
| if err := clientCtx.PrintRaw(txBytes); err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\n%s\n", err, txBytes) | ||
| } | ||
|
|
||
| buf := bufio.NewReader(os.Stdin) | ||
| ok, err := input.GetConfirmation("confirm transaction before signing and broadcasting", buf, os.Stderr) | ||
| if err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\ncanceled transaction\n", err) | ||
| return err | ||
| } | ||
| if !ok { | ||
| _, _ = fmt.Fprintln(os.Stderr, "canceled transaction") | ||
| return nil | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| signedTx, err := txf.sign(clientCtx.CmdContext, true) | ||
| signedTx, err := txf.sign(ctx, true) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| txBytes, err := txf.txConfig.TxEncoder()(signedTx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| res, err := broadcaster.Broadcast(context.Background(), txBytes) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| return clientCtx.PrintString(string(res)) | ||
| return broadcaster.Broadcast(ctx, txBytes) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and context propagation in BroadcastTx
The function should wrap errors with context and ensure context is properly propagated.
func BroadcastTx(ctx context.Context, txf Factory, broadcaster broadcast.Broadcaster, msgs ...transaction.Msg) ([]byte, error) {
+ if broadcaster == nil {
+ return nil, fmt.Errorf("broadcaster cannot be nil")
+ }
+
if txf.simulateAndExecute() {
err := txf.calculateGas(msgs...)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to calculate gas: %w", err)
}
}
err := txf.BuildUnsignedTx(msgs...)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to build unsigned transaction: %w", err)
}
signedTx, err := txf.sign(ctx, true)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to sign transaction: %w", err)
}
txBytes, err := txf.txConfig.TxEncoder()(signedTx)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to encode transaction: %w", err)
}
return broadcaster.Broadcast(ctx, txBytes)
}📝 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 BroadcastTx(ctx context.Context, txf Factory, broadcaster broadcast.Broadcaster, msgs ...transaction.Msg) ([]byte, error) { | |
| if txf.simulateAndExecute() { | |
| err := txf.calculateGas(msgs...) | |
| if err != nil { | |
| return err | |
| return nil, err | |
| } | |
| } | |
| err := txf.BuildUnsignedTx(msgs...) | |
| if err != nil { | |
| return err | |
| } | |
| if !clientCtx.SkipConfirm { | |
| encoder := txf.txConfig.TxJSONEncoder() | |
| if encoder == nil { | |
| return errors.New("failed to encode transaction: tx json encoder is nil") | |
| } | |
| unsigTx, err := txf.getTx() | |
| if err != nil { | |
| return err | |
| } | |
| txBytes, err := encoder(unsigTx) | |
| if err != nil { | |
| return fmt.Errorf("failed to encode transaction: %w", err) | |
| } | |
| if err := clientCtx.PrintRaw(txBytes); err != nil { | |
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\n%s\n", err, txBytes) | |
| } | |
| buf := bufio.NewReader(os.Stdin) | |
| ok, err := input.GetConfirmation("confirm transaction before signing and broadcasting", buf, os.Stderr) | |
| if err != nil { | |
| _, _ = fmt.Fprintf(os.Stderr, "error: %v\ncanceled transaction\n", err) | |
| return err | |
| } | |
| if !ok { | |
| _, _ = fmt.Fprintln(os.Stderr, "canceled transaction") | |
| return nil | |
| } | |
| return nil, err | |
| } | |
| signedTx, err := txf.sign(clientCtx.CmdContext, true) | |
| signedTx, err := txf.sign(ctx, true) | |
| if err != nil { | |
| return err | |
| return nil, err | |
| } | |
| txBytes, err := txf.txConfig.TxEncoder()(signedTx) | |
| if err != nil { | |
| return err | |
| } | |
| res, err := broadcaster.Broadcast(context.Background(), txBytes) | |
| if err != nil { | |
| return err | |
| return nil, err | |
| } | |
| return clientCtx.PrintString(string(res)) | |
| return broadcaster.Broadcast(ctx, txBytes) | |
| func BroadcastTx(ctx context.Context, txf Factory, broadcaster broadcast.Broadcaster, msgs ...transaction.Msg) ([]byte, error) { | |
| if broadcaster == nil { | |
| return nil, fmt.Errorf("broadcaster cannot be nil") | |
| } | |
| if txf.simulateAndExecute() { | |
| err := txf.calculateGas(msgs...) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to calculate gas: %w", err) | |
| } | |
| } | |
| err := txf.BuildUnsignedTx(msgs...) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build unsigned transaction: %w", err) | |
| } | |
| signedTx, err := txf.sign(ctx, true) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to sign transaction: %w", err) | |
| } | |
| txBytes, err := txf.txConfig.TxEncoder()(signedTx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to encode transaction: %w", err) | |
| } | |
| return broadcaster.Broadcast(ctx, txBytes) | |
| } |
| return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc) | ||
| } | ||
|
|
||
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) |
There was a problem hiding this comment.
Correct the use of grpc.NewClient to grpc.Dial
The function grpc.NewClient does not exist in the google.golang.org/grpc package. The correct function to establish a gRPC connection is grpc.Dial.
Apply this diff to fix the issue:
-return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...)
+return grpc.Dial(addr, grpc.WithTransportCredentials(creds))Ensure you handle the returned *grpc.ClientConn and associated errors appropriately.
📝 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.
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) | |
| return grpc.Dial(addr, grpc.WithTransportCredentials(creds)) |
| return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) { | ||
| var err error | ||
| creds := grpcinsecure.NewCredentials() | ||
|
|
||
| insecure := true | ||
| if cmd.Flags().Lookup(flags.FlagGrpcInsecure) != nil { | ||
| insecure, err = cmd.Flags().GetBool(flags.FlagGrpcInsecure) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| if !insecure { | ||
| creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}) | ||
| } | ||
|
|
||
| var addr string | ||
| if cmd.Flags().Lookup(flags.FlagGrpcAddress) != nil { | ||
| addr, err = cmd.Flags().GetString(flags.FlagGrpcAddress) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| if addr == "" { | ||
| // if grpc-addr has not been set, use the default clientConn | ||
| // TODO: default is comet | ||
| node, err := cmd.Flags().GetString(flags.FlagNode) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc) | ||
| } | ||
|
|
||
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure secure gRPC connections by default
The getQueryClientConn function initializes the insecure variable to true, which means the application defaults to insecure gRPC connections unless the --grpc-insecure flag is explicitly set. This could pose security risks.
Recommend changing the default value of insecure to false to prioritize secure connections by default. Apply this diff:
func getQueryClientConn(cdc codec.Codec) func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) {
var err error
- creds := grpcinsecure.NewCredentials()
-
- insecure := true
+ insecure := false
+ creds := credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})
if cmd.Flags().Lookup(flags.FlagGrpcInsecure) != nil {
insecure, err = cmd.Flags().GetBool(flags.FlagGrpcInsecure)
if err != nil {
return nil, err
}
}
if insecure {
creds = grpcinsecure.NewCredentials()
}This change ensures that the application uses secure gRPC connections by default, enhancing overall security.
📝 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.
| return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) { | |
| var err error | |
| creds := grpcinsecure.NewCredentials() | |
| insecure := true | |
| if cmd.Flags().Lookup(flags.FlagGrpcInsecure) != nil { | |
| insecure, err = cmd.Flags().GetBool(flags.FlagGrpcInsecure) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if !insecure { | |
| creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}) | |
| } | |
| var addr string | |
| if cmd.Flags().Lookup(flags.FlagGrpcAddress) != nil { | |
| addr, err = cmd.Flags().GetString(flags.FlagGrpcAddress) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if addr == "" { | |
| // if grpc-addr has not been set, use the default clientConn | |
| // TODO: default is comet | |
| node, err := cmd.Flags().GetString(flags.FlagNode) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc) | |
| } | |
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) | |
| } | |
| } | |
| return func(cmd *cobra.Command) (grpc.ClientConnInterface, error) { | |
| var err error | |
| insecure := false | |
| creds := credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}) | |
| if cmd.Flags().Lookup(flags.FlagGrpcInsecure) != nil { | |
| insecure, err = cmd.Flags().GetBool(flags.FlagGrpcInsecure) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if insecure { | |
| creds = grpcinsecure.NewCredentials() | |
| } | |
| var addr string | |
| if cmd.Flags().Lookup(flags.FlagGrpcAddress) != nil { | |
| addr, err = cmd.Flags().GetString(flags.FlagGrpcAddress) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if addr == "" { | |
| // if grpc-addr has not been set, use the default clientConn | |
| // TODO: default is comet | |
| node, err := cmd.Flags().GetString(flags.FlagNode) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return comet.NewCometBFTBroadcaster(node, comet.BroadcastSync, cdc) | |
| } | |
| return grpc.NewClient(addr, []grpc.DialOption{grpc.WithTransportCredentials(creds)}...) | |
| } | |
| } |
| } | ||
|
|
||
| signedTx, err := txf.sign(clientCtx.CmdContext, true) | ||
| signedTx, err := txf.sign(ctx, true) |
There was a problem hiding this comment.
This funcition still need to be split in two, otherwise we loose the ability to confirm
Description
Closes:
#22397
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
Release Notes
New Features
Printerfor formatted output handling.CometBFTBroadcasterstruct for improved gRPC communication.Improvements
Bug Fixes
Tests