refactor(client/v2): offchain uses client/v2/factory #22344
Conversation
JulianToledano
requested review from
auricom,
julienrbrt,
tac0turtle and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (14)
client/v2/offchain/encode.go (1)
46-46: Consider adding a function comment for documentation.Following Go conventions, consider adding a comment that describes the function's purpose and parameters.
+// encodeDigest encodes the given digest using the specified encoding format. +// Returns the encoded string or an error if the encoding format is unknown. func encodeDigest(encodingFormat string, digest []byte) (string, error) {client/v2/offchain/sign_test.go (2)
26-52: Consider adding more edge cases to the test suite.While the current test cases cover the basic scenarios, consider adding tests for:
- Empty raw bytes
- Very large raw bytes
- Invalid account name
- Empty encoding
Would you like me to provide example test cases for these scenarios?
55-61: Document or parameterize the hardcoded "json" argument.The
Signfunction call includes a hardcoded "json" parameter. Consider either:
- Adding it to the test case struct if it's meant to be variable
- Adding a comment explaining why it's hardcoded to "json"
type struct { name string rawBytes []byte encoding string signMode string + format string // output format (e.g., "json") wantErr bool }client/v2/offchain/cli.go (3)
85-88: Command usage description could be more detailed.The command's usage description should provide more context about the expected file format and structure.
- Use: "verify-file <signedFileName>", - Short: "Verify a file.", - Long: "Verify a previously signed file with the given key.", + Use: "verify-file <signedFileName>", + Short: "Verify a signed file.", + Long: `Verify a previously signed file. + +The signed file should be in the format specified by --file-format flag (json|text). +Example: cosmos-sdk off-chain verify-file signed_tx.json --file-format=json`,
Line range hint
95-103: Enhance error handling and success message.The verification result could provide more detailed information, and error handling for flag retrieval should be added.
- bz, err := os.ReadFile(args[0]) + fileFormat, err := cmd.Flags().GetString(flagFileFormat) + if err != nil { + return err + } + if fileFormat != "json" && fileFormat != "text" { + return fmt.Errorf("invalid file format %q, expected 'json' or 'text'", fileFormat) + } + + bz, err := os.ReadFile(args[0]) if err != nil { return err } - fileFormat, _ := cmd.Flags().GetString(flagFileFormat) - err = Verify(clientCtx, bz, fileFormat) if err == nil { - cmd.Println("Verification OK!") + cmd.Printf("✅ Signature verification successful for file: %s\n", args[0]) }
Line range hint
1-110: Consider implementing a factory pattern for command creation.Given the PR objective to use client/v2/factory, consider refactoring the command creation logic into a factory pattern. This would improve testability and make it easier to create different command variations.
The factory pattern could be implemented as follows:
// Example factory implementation type CommandFactory interface { CreateSignCommand() *cobra.Command CreateVerifyCommand() *cobra.Command } type DefaultCommandFactory struct { // dependencies } func NewDefaultCommandFactory() CommandFactory { return &DefaultCommandFactory{} }client/v2/tx/wrapper.go (1)
100-108: Add documentation for the new methodConsider adding a doc comment explaining the purpose of this method and its relationship to the signing process.
+// GetSigningTxData returns the transaction data required for signing. +// It includes the transaction body, authentication info, and their raw byte representations. func (w wrappedTx) GetSigningTxData() (signing.TxData, error) {client/v2/offchain/verify_test.go (2)
34-34: Consider adding more test cases for robustnessWhile the current test cases cover basic positive and negative scenarios, consider adding tests for:
- Malformed JSON/text input
- Empty input
- Invalid signature format
- Missing required fields
This would improve the robustness of the verification testing.
Also applies to: 40-40, 47-47, 53-53
117-120: Enhance unmarshal test validationWhile the basic unmarshaling is tested, consider:
- Adding assertions to validate the unmarshaled content
- Including negative test cases (malformed input)
- Testing boundary conditions
Example:
got, err := unmarshal(tt.fileFormat, tt.digest, txConfig) require.NoError(t, err) require.NotNil(t, got) // Add specific field validations require.Equal(t, tt.expectedSigner, got.GetSigners()[0].String()) require.Equal(t, tt.expectedAppDomain, got.GetMsgs()[0].(*MsgSignArbitraryData).AppDomain)client/v2/tx/types.go (1)
45-53: Update field documentation to match Go conventionsThe field comments should be updated to follow Go conventions for exported fields. Each comment should start with the field name.
Apply these documentation changes:
- // accountNumber is the unique identifier for the account. + // AccountNumber is the unique identifier for the account. AccountNumber uint64 - // sequence is the sequence number of the transaction. + // Sequence is the sequence number of the transaction. Sequence uint64 - // fromName is the name of the account sending the transaction. + // FromName is the name of the account sending the transaction. FromName string - // fromAddress is the address of the account sending the transaction. + // FromAddress is the address of the account sending the transaction. FromAddress string - // address is the byte representation of the account address. + // Address is the byte representation of the account address. Address []byteclient/v2/tx/factory_test.go (2)
Line range hint
246-262: Consider adding edge cases to improve test coverage.The test suite would benefit from additional test cases:
- Invalid signature scenarios in
TestFactory_Sign- Different gas adjustment values in
TestFactory_calculateGas- Error cases for invalid chain IDs
Line range hint
443-453: Consider extracting common test setup into helper functions.Multiple test functions share similar setup code for creating a new Factory instance. Consider extracting this into a helper function to improve maintainability and reduce duplication:
func setupTestFactory(t *testing.T, params TxParameters) Factory { f, err := NewFactory(keybase, cdc, mockAccountRetriever{}, txConf, ac, mockClientConn{}, params) require.NoError(t, err) require.NotNil(t, f) return f }Also applies to: 497-507, 563-573, 640-650, 701-711
client/v2/offchain/verify.go (1)
Line range hint
119-125: Consider handling additional signature data typesCurrently, the
verifySignaturefunction only handles*clitx.SingleSignatureData. If other signature data types (e.g., multisignatures) may be encountered, consider adding cases to handle them or document why they are not required.client/v2/offchain/sign.go (1)
103-112: Consider renaming theencodefunction for clarityThe function
encodecould be renamed toencodeTxto more clearly indicate that it encodes a transaction. This improves code readability and aligns with Go naming conventions.Apply this diff to rename the function and update its references:
-func encode(output string, tx clitx.Tx, config clitx.TxConfig) ([]byte, error) { +func encodeTx(output string, tx clitx.Tx, config clitx.TxConfig) ([]byte, error) {Update the function call in the
Signmethod:- bz, err := encode(output, signedTx, txConfig) + bz, err := encodeTx(output, signedTx, txConfig)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
client/v2/internal/testpb/msg_grpc.pb.gois excluded by!**/*.pb.goclient/v2/internal/testpb/query_grpc.pb.gois excluded by!**/*.pb.goclient/v2/offchain/msgSignArbitraryData.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
- client/v2/Makefile (1 hunks)
- client/v2/internal/buf.gen.gogo.yaml (1 hunks)
- client/v2/internal/offchain/msgSignArbitraryData.proto (1 hunks)
- client/v2/internal/testpb/msg.pulsar.go (1 hunks)
- client/v2/offchain/builder.go (0 hunks)
- client/v2/offchain/cli.go (3 hunks)
- client/v2/offchain/encode.go (1 hunks)
- client/v2/offchain/marshal.go (0 hunks)
- client/v2/offchain/sign.go (3 hunks)
- client/v2/offchain/sign_test.go (1 hunks)
- client/v2/offchain/signature.go (0 hunks)
- client/v2/offchain/verify.go (4 hunks)
- client/v2/offchain/verify_test.go (5 hunks)
- client/v2/tx/config.go (2 hunks)
- client/v2/tx/encoder.go (3 hunks)
- client/v2/tx/factory.go (10 hunks)
- client/v2/tx/factory_test.go (11 hunks)
- client/v2/tx/types.go (5 hunks)
- client/v2/tx/wrapper.go (2 hunks)
💤 Files with no reviewable changes (3)
- client/v2/offchain/builder.go
- client/v2/offchain/marshal.go
- client/v2/offchain/signature.go
✅ Files skipped from review due to trivial changes (2)
- client/v2/internal/buf.gen.gogo.yaml
- client/v2/internal/offchain/msgSignArbitraryData.proto
🧰 Additional context used
📓 Path-based instructions (13)
client/v2/internal/testpb/msg.pulsar.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/encode.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/offchain/verify_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/tx/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/tx/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/tx/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/tx/factory_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/tx/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/tx/wrapper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (24)
client/v2/Makefile (1)
1-2: Verify the existence of the Pulsar template file.The command now uses a specific template for code generation. Let's verify that the template file exists and is properly configured.
✅ Verification successful
Template file exists and is properly configured for Pulsar code generation
The verification confirms that:
- The template file exists at the expected location
client/v2/internal/buf.gen.pulsar.yaml- It's a valid YAML file with proper configuration for:
- Go package prefix settings
- Pulsar and gRPC plugin configurations
- Source-relative path options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the Pulsar template file # Test 1: Check if the template file exists if [ -f "client/v2/internal/buf.gen.pulsar.yaml" ]; then echo "Template file exists" # Test 2: Check if it's a valid YAML file with basic structure cat "client/v2/internal/buf.gen.pulsar.yaml" else echo "Template file not found at expected location" fiLength of output: 513
client/v2/offchain/encode.go (1)
46-53: LGTM! Clean and well-structured implementation.The new
encodeDigestfunction is well-implemented with proper error handling and good reuse of existing functionality.client/v2/offchain/sign_test.go (2)
14-17: Function name change follows Go conventions.The renaming from
Test_signtoTestSignbetter aligns with Go's naming conventions for test functions.
16-17: Verify mnemonic variable availability.The test uses an undefined
mnemonicvariable. Ensure this variable is defined elsewhere in the test file or consider making it explicit in the test.✅ Verification successful
The
mnemonicvariable is properly defined in the test packageThe
mnemonicvariable is defined as a package-level variable incommon_test.gowithin the same package, making it accessible to all test files in theoffchainpackage. The value is properly set to a valid mnemonic phrase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mnemonic definition in the test file rg -A 2 'mnemonic\s*:?=' client/v2/offchain/Length of output: 339
client/v2/offchain/cli.go (1)
16-17: LGTM! Constants and package structure are well-organized.The constants follow Go naming conventions and imports are properly organized.
client/v2/tx/wrapper.go (1)
13-13: LGTM: Import follows Go conventionsThe new signing package import is correctly placed and necessary for the new functionality.
client/v2/tx/encoder.go (2)
7-7: LGTM: Marshal options are consistent with existing patterns.The new
textMarshalOptionsfollows the same configuration pattern as the existingjsonMarshalOptions, maintaining consistency in the codebase.Also applies to: 27-30
109-115:⚠️ Potential issueFix function name to follow Go naming conventions.
The function name should be
encodeTextTxto maintain consistency withencodeJsonTx. In Go, acronyms should be consistently cased.Apply this change:
-func encodeTextTx(tx Tx) ([]byte, error) { +func encodeTextTx(tx Tx) ([]byte, error) {Likely invalid or redundant comment.
client/v2/offchain/verify_test.go (2)
9-9: LGTM: Context setup is well-structuredThe addition of ValidatorAddressCodec and its initialization follows consistent patterns with existing code. The context setup is comprehensive and properly initialized.
Also applies to: 19-22
92-98: LGTM: Well-structured configuration setupThe txConfig initialization is comprehensive and properly configured with all necessary options.
client/v2/tx/types.go (3)
17-17: LGTM: Clean interface extensionThe addition of
GetSigningTxDatamethod to theTxinterface is well-structured and follows Go interface design principles.Also applies to: 145-146
198-206: LGTM: Consistent field usageThe struct initialization correctly uses the newly exported field names while maintaining the original functionality.
32-32: Verify field visibility consistencyThe struct has a mix of exported (
ChainID,SignMode) and unexported fields (timeoutTimestamp,memo). Please verify if this mixed visibility pattern is intentional, as it might indicate inconsistent access patterns.Also applies to: 34-34
✅ Verification successful
Let me try a different approach to gather information about the usage patterns and visibility.
This will help us understand:
- The complete struct definition to see all fields
- Where this type is being imported and potentially used
Let me search for direct references to TxParameters in the codebase to understand its usage pattern.
This will help us understand how TxParameters and its fields are being used across the codebase.
Based on the gathered information, I can now provide a final response about the field visibility pattern.
Field visibility pattern is intentional and correct
The mixed visibility pattern in
TxParametersis intentional and consistent with the usage patterns in the codebase:
ChainIDandSignModeare exported because they are used across package boundaries, particularly in:
- External signing operations (
client/v2/offchain/sign.go)- Transaction factory operations requiring chain validation
- Direct mode signing and verification flows
timeoutTimestampandmemoremain unexported as they are only used internally within the tx package for:
- Transaction building (
factory.go)- Local validation
- Transaction encoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of TxParameters fields to verify if the visibility changes are consistent with usage patterns rg -A 3 "TxParameters\."Length of output: 26
Script:
#!/bin/bash # First, let's find the file containing TxParameters struct definition ast-grep --pattern 'type TxParameters struct { $$$ }' # Then, let's search for any imports of the package containing TxParameters rg -l "tx\"$|tx/types|tx\""Length of output: 6630
Script:
#!/bin/bash # Search for TxParameters usage in the codebase rg -A 5 "TxParameters" # Also search for any method that might be accessing these fields rg -A 5 "\.ChainID|\.SignMode|\.timeoutTimestamp|\.memo" client/v2/tx/Length of output: 33787
client/v2/tx/config.go (2)
54-57: LGTM: Interface extension follows consistent patternsThe new text encoding methods are well-documented and maintain consistency with existing encoder/decoder patterns.
175-183: Verify referenced text encoding functions existThe implementation looks correct, but we need to verify the existence of the referenced
encodeTextTxanddecodeTextTxfunctions.✅ Verification successful
Since I found both functions in
client/v2/tx/encoder.go, let me verify their implementations to ensure they match the expected usage pattern.
Functions exist and are properly implemented
The referenced functions
encodeTextTxanddecodeTextTxexist inclient/v2/tx/encoder.gowith proper implementations:
encodeTextTxhandles text marshaling of the wrapped transactiondecodeTextTxuses prototext unmarshaling with appropriate decoder and codec parameters🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of text encoding/decoding functions # Expected: Find definitions of encodeTextTx and decodeTextTx functions echo "Searching for encodeTextTx function..." rg "func.*encodeTextTx" --type go echo "Searching for decodeTextTx function..." rg "func.*decodeTextTx" --type goLength of output: 406
Script:
#!/bin/bash # Get the implementation details of both functions with context echo "Checking implementation of text encoding functions..." rg "func (.*encodeTextTx|.*decodeTextTx)" -A 10 --type go client/v2/tx/encoder.goLength of output: 778
client/v2/tx/factory_test.go (1)
38-38: LGTM! Field naming follows Go conventions.The renaming of struct fields to use uppercase (e.g.,
Address,ChainID,FromName, etc.) follows Go's convention for exported fields, improving consistency and API visibility.Also applies to: 69-69, 71-71, 84-84, 86-86, 136-136, 138-138, 178-178, 180-180, 222-222, 224-224, 254-254, 256-257, 302-305, 312-312, 314-314, 418-418, 425-425, 432-432, 439-439
client/v2/tx/factory.go (3)
Line range hint
331-348: Verify chain ID validation before signing.The signing process uses the chain ID directly without explicit validation. While the chain ID is validated in
validateFlagSet, it's crucial to ensure it matches the expected format here as well.#!/bin/bash # Search for chain ID validation patterns ast-grep --pattern 'if $chain_id == "" { return $err }'
519-519: Consider adding validation for sign mode.The sign mode is used without validation in the simulation context.
127-142: Enhance error handling for account retrieval.The error message "missing 'from address' field" could be more descriptive. Consider including the expected format or requirements for the address field.
- return parameters, errors.New("missing 'from address' field") + return parameters, errors.New("missing 'from address' field: address must be a valid bech32 encoded string")Additionally, consider adding validation for the retrieved account number and sequence to ensure they are within expected ranges.
client/v2/internal/testpb/msg.pulsar.go (2)
Line range hint
1-4310: Generated code structure looks good.The protobuf generated code follows best practices:
- Proper null checks and initialization
- Comprehensive field descriptors and reflection support
- Robust error handling for protocol buffer operations
4290-4301:⚠️ Potential issueVersion inconsistency detected in service definitions.
The service methods are using different cosmos-sdk versions:
- "Send" method uses v0.50.0
- "Clawback" method uses v0.53.0
This inconsistency could lead to compatibility issues and should be unified.
client/v2/offchain/verify.go (2)
98-105: Unmarshal function handles formats correctlyThe
unmarshalfunction appropriately decodes the transaction based on the provided format (jsonortext) and returns an error for unsupported formats. Error handling is correctly implemented.
21-26: Verify the definition ofenabledSignModesThe variable
enabledSignModesis used in theclitx.NewTxConfigcall, but its definition is not shown in the provided code. Ensure thatenabledSignModesis properly defined and initialized with all the necessary sign modes required for transaction processing.Run the following script to check for the definition of
enabledSignModes:✅ Verification successful
Let me gather more context about the
enabledSignModesdefinition in the relevant file.
Let me try again with a corrected regex pattern.
Let me try one more time with a simpler approach.
enabledSignModesis properly defined and used correctlyThe variable
enabledSignModesis properly defined in the same package (offchain) as a package-level variable that supports two sign modes:
SignMode_SIGN_MODE_DIRECTSignMode_SIGN_MODE_LEGACY_AMINO_JSONThis definition aligns with its usage in the
NewTxConfigcall and provides the necessary sign modes for transaction processing in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of 'enabledSignModes' in the codebase. # Expectation: 'enabledSignModes' should be defined in the Go code. rg 'var enabledSignModes|enabledSignModes\s*:=' --type goLength of output: 802
Script:
#!/bin/bash # Get the full definition of enabledSignModes in client/v2/offchain/sign.go rg -A 5 'var enabledSignModes = \[\]apisigning\.SignMode{' client/v2/offchain/sign.goLength of output: 247
Script:
#!/bin/bash # Get the full definition of enabledSignModes in client/v2/offchain/sign.go rg -A 5 "var enabledSignModes = .*SignMode{" client/v2/offchain/sign.goLength of output: 205
Script:
#!/bin/bash # Get the content of the file to see the definition cat client/v2/offchain/sign.goLength of output: 3240
client/v2/offchain/sign.go (1)
26-28: LGTM!The declaration of
enabledSignModesis clear and correctly lists the supported sign modes.
| func (w wrappedTx) GetSigningTxData() (signing.TxData, error) { | ||
| return signing.TxData{ | ||
| Body: w.Tx.Body, | ||
| AuthInfo: w.Tx.AuthInfo, | ||
| BodyBytes: w.TxRaw.BodyBytes, | ||
| AuthInfoBytes: w.TxRaw.AuthInfoBytes, | ||
| BodyHasUnknownNonCriticals: w.TxBodyHasUnknownNonCriticals, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the return signature
The method never returns an error but includes it in the return signature. Consider either:
- Removing the error return since no error conditions exist, or
- Adding error handling if there are potential error cases.
-func (w wrappedTx) GetSigningTxData() (signing.TxData, error) {
+func (w wrappedTx) GetSigningTxData() signing.TxData {
- return signing.TxData{
+ return signing.TxData{
Body: w.Tx.Body,
AuthInfo: w.Tx.AuthInfo,
BodyBytes: w.TxRaw.BodyBytes,
AuthInfoBytes: w.TxRaw.AuthInfoBytes,
BodyHasUnknownNonCriticals: w.TxBodyHasUnknownNonCriticals,
- }, nil
+ }
}📝 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 (w wrappedTx) GetSigningTxData() (signing.TxData, error) { | |
| return signing.TxData{ | |
| Body: w.Tx.Body, | |
| AuthInfo: w.Tx.AuthInfo, | |
| BodyBytes: w.TxRaw.BodyBytes, | |
| AuthInfoBytes: w.TxRaw.AuthInfoBytes, | |
| BodyHasUnknownNonCriticals: w.TxBodyHasUnknownNonCriticals, | |
| }, nil | |
| } | |
| func (w wrappedTx) GetSigningTxData() signing.TxData { | |
| return signing.TxData{ | |
| Body: w.Tx.Body, | |
| AuthInfo: w.Tx.AuthInfo, | |
| BodyBytes: w.TxRaw.BodyBytes, | |
| AuthInfoBytes: w.TxRaw.AuthInfoBytes, | |
| BodyHasUnknownNonCriticals: w.TxBodyHasUnknownNonCriticals, | |
| } | |
| } |
| func encodeTextTx(tx Tx) ([]byte, error) { | ||
| wTx, ok := tx.(*wrappedTx) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected tx type: %T", tx) | ||
| } | ||
| return textMarshalOptions.Marshal(wTx.Tx) | ||
| } | ||
|
|
||
| // decodeJsonTx decodes transaction bytes into an apitx.Tx structure using JSON format. | ||
| func decodeTextTx(cdc codec.BinaryCodec, decoder Decoder) txDecoder { | ||
| return func(txBytes []byte) (Tx, error) { | ||
| jsonTx := new(txv1beta1.Tx) | ||
| err := prototext.UnmarshalOptions{ | ||
| AllowPartial: false, | ||
| DiscardUnknown: false, | ||
| }.Unmarshal(txBytes, jsonTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| pTxBytes, err := protoTxBytes(jsonTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| decodedTx, err := decoder.Decode(pTxBytes) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return newWrapperTx(cdc, decodedTx), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication.
The encoder/decoder implementations follow similar patterns across different formats (binary, JSON, text). Consider introducing a generic encoder/decoder factory that takes format-specific marshal options as parameters, reducing code duplication and making it easier to add new formats in the future.
Example approach:
type marshalConfig struct {
marshal protov2.MarshalOptions
unmarshal interface{} // protov2.UnmarshalOptions or similar
}
func newEncoder(config marshalConfig) txEncoder {
return func(tx Tx) ([]byte, error) {
wTx, ok := tx.(*wrappedTx)
if !ok {
return nil, fmt.Errorf("unexpected tx type: %T", tx)
}
return config.marshal.Marshal(wTx.Tx)
}
}| f.txParams.Sequence = sequence | ||
| } | ||
|
|
||
| // WithAccountNumber returns a copy of the Factory with an updated account number. | ||
| func (f *Factory) WithAccountNumber(accnum uint64) { | ||
| f.txParams.accountNumber = accnum | ||
| f.txParams.AccountNumber = accnum |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for sequence and account number.
The WithSequence and WithAccountNumber methods directly set values without validation. Consider adding checks to prevent setting invalid values.
func (f *Factory) WithSequence(sequence uint64) {
+ if sequence == 0 {
+ panic("sequence number cannot be zero")
+ }
f.txParams.Sequence = sequence
}
func (f *Factory) WithAccountNumber(accnum uint64) {
+ if accnum == 0 {
+ panic("account number cannot be zero")
+ }
f.txParams.AccountNumber = accnum
}Committable suggestion was skipped due to low confidence.
client/v2/offchain/sign.go
Outdated
| func Sign(ctx client.Context, rawBytes []byte, fromName, encoding, signMode, output string) (string, error) { | ||
| digest, err := encodeDigest(encoding, rawBytes) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| digest, err := encoder(rawBytes) | ||
| keybase, err := keyring.NewAutoCLIKeyring(ctx.Keyring, ctx.AddressCodec) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| tx, err := sign(ctx, fromName, digest) | ||
| txConfig, err := clitx.NewTxConfig(clitx.ConfigOptions{ | ||
| AddressCodec: ctx.AddressCodec, | ||
| Cdc: ctx.Codec, | ||
| ValidatorAddressCodec: ctx.ValidatorAddressCodec, | ||
| EnablesSignModes: enabledSignModes, | ||
| }) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| txMarshaller, err := getMarshaller(output, indent, emitUnpopulated) | ||
| accRetriever := account.NewAccountRetriever(ctx.AddressCodec, ctx, ctx.InterfaceRegistry) | ||
|
|
||
| sm, err := getSignMode(signMode) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| params := clitx.TxParameters{ | ||
| ChainID: ExpectedChainID, | ||
| SignMode: sm, | ||
| AccountConfig: clitx.AccountConfig{ | ||
| AccountNumber: ExpectedAccountNumber, | ||
| Sequence: ExpectedSequence, | ||
| FromName: fromName, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Sign function parameters
The Sign function has multiple parameters, which can make it harder to maintain and use. Consider grouping related parameters into a struct or using functional options to improve readability and flexibility.
| signedTx, err := txf.BuildsSignedTx(context.Background(), msg) | ||
| if err != nil { | ||
| return nil, err | ||
| return "", err |
There was a problem hiding this comment.
Possible typo in method name BuildsSignedTx
The method BuildsSignedTx may be incorrectly named. In Go, method names typically follow the VerbNoun convention. Consider renaming it to BuildSignedTx if that is the correct method.
Apply this diff to correct the method name:
- signedTx, err := txf.BuildsSignedTx(context.Background(), msg)
+ signedTx, err := txf.BuildSignedTx(context.Background(), msg)Committable suggestion was skipped due to low confidence.
client/v2/offchain/sign.go
Outdated
| txConfig, err := clitx.NewTxConfig(clitx.ConfigOptions{ | ||
| AddressCodec: ctx.AddressCodec, | ||
| Cdc: ctx.Codec, | ||
| ValidatorAddressCodec: ctx.ValidatorAddressCodec, | ||
| EnablesSignModes: enabledSignModes, | ||
| }) |
There was a problem hiding this comment.
Typo in field name EnablesSignModes
The field name EnablesSignModes in clitx.ConfigOptions appears to be misspelled. It should likely be EnabledSignModes to correctly represent the enabled sign modes.
Apply this diff to correct the field name:
- EnablesSignModes: enabledSignModes,
+ EnabledSignModes: enabledSignModes,
There was a problem hiding this comment.
that's a good point, shall we rename it? This is better cc @JulianToledano
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
yes better rename it!! 😅
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
client/v2/offchain/verify.go (1)
98-105: Add documentation for supported formats.Consider adding a comment to document the supported formats ("json" and "text") and their use cases. This would improve API clarity.
// unmarshal unmarshalls a digest to a Tx using protobuf protojson. +// Supported formats: +// - "json": JSON format using protojson marshaling +// - "text": Text format for human-readable representation func unmarshal(format string, bz []byte, config clitx.TxConfig) (clitx.Tx, error) {client/v2/offchain/sign.go (1)
32-33: Add parameter documentation.While the function signature is cleaner, consider adding documentation for each parameter to improve maintainability.
// Sign signs given bytes using the specified encoder and SignMode. +// Parameters: +// - ctx: The client context +// - rawBytes: The raw bytes to sign +// - fromName: The name of the signing key +// - encoding: The encoding format for the digest +// - signMode: The signing mode ("direct" or "amino-json") +// - output: The output format ("json" or "text") func Sign(ctx client.Context, rawBytes []byte, fromName, encoding, signMode, output string) (string, error) {client/v2/tx/config.go (1)
175-183: Add error handling documentationThe new text encoding/decoding methods should document their error handling behavior to maintain consistency with other methods in the interface.
-// TxTextEncoder returns the default text transaction encoder. +// TxTextEncoder returns the default text transaction encoder. +// It returns an error if the transaction cannot be encoded or if the encoding format is invalid. func (t defaultEncodingConfig) TxTextEncoder() txEncoder { return encodeTextTx } -// TxTextDecoder returns the default text transaction decoder. +// TxTextDecoder returns the default text transaction decoder. +// It returns an error if the transaction cannot be decoded or if the input format is invalid. func (t defaultEncodingConfig) TxTextDecoder() txDecoder { return decodeTextTx(t.cdc, t.decoder) }client/v2/offchain/verify_test.go (1)
19-22: Avoid horizontal alignment in struct initializationsAccording to the Uber Go Style Guide, aligning struct fields with extra spaces is discouraged as it can lead to unnecessary diffs and reduce readability. It's recommended to use standard indentation without extra spaces for alignment.
Apply the following changes to remove the extra spaces:
In
Test_Verify:ctx := client.Context{ - TxConfig: newTestConfig(t), - Codec: getCodec(), - AddressCodec: address.NewBech32Codec("cosmos"), - ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), + TxConfig: newTestConfig(t), + Codec: getCodec(), + AddressCodec: address.NewBech32Codec("cosmos"), + ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), }In
Test_SignVerify:ctx := client.Context{ - TxConfig: newTestConfig(t), - Codec: getCodec(), - AddressCodec: address.NewBech32Codec("cosmos"), - ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), - Keyring: k, + TxConfig: newTestConfig(t), + Codec: getCodec(), + AddressCodec: address.NewBech32Codec("cosmos"), + ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), + Keyring: k, }In
Test_unmarshal:txConfig, err := clitx.NewTxConfig(clitx.ConfigOptions{ - AddressCodec: address.NewBech32Codec("cosmos"), - Cdc: getCodec(), - ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), - EnabledSignModes: enabledSignModes, + AddressCodec: address.NewBech32Codec("cosmos"), + Cdc: getCodec(), + ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), + EnabledSignModes: enabledSignModes, })Also applies to: 77-81, 92-97
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- client/v2/offchain/sign.go (3 hunks)
- client/v2/offchain/verify.go (4 hunks)
- client/v2/offchain/verify_test.go (5 hunks)
- client/v2/tx/config.go (5 hunks)
- client/v2/tx/config_test.go (2 hunks)
- client/v2/tx/tx.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/verify.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/offchain/verify_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/tx/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/tx/config_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/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/offchain/verify.go (4)
11-12: LGTM: Import changes align with refactoring goals.The new imports correctly introduce the client/v2/tx package with appropriate aliases.
40-40: LGTM: Proper error handling and type usage.The changes correctly implement error handling for the new transaction type methods and maintain good separation of concerns.
Also applies to: 43-48, 84-87
114-114: LGTM: Proper signature verification implementation.The signature verification logic is well-implemented with proper type assertions and error handling.
Also applies to: 119-129
21-26: Verify the enabledSignModes variable definition.The code uses
enabledSignModesin the configuration but its definition is not visible. Please ensure it's properly defined and includes all necessary sign modes.✅ Verification successful
The verification is complete as I can see the definition of
enabledSignModesin both the main code and test files. I'll generate the final response:
enabledSignModesis properly defined with supported sign modesThe variable is correctly defined in
client/v2/offchain/sign.gowith support for two sign modes:
SIGN_MODE_DIRECTSIGN_MODE_LEGACY_AMINO_JSON🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for enabledSignModes definition rg -A 5 "var\s+enabledSignModes|enabledSignModes\s+:=" client/v2/offchain/Length of output: 884
client/v2/offchain/sign.go (2)
26-28: LGTM! Well-defined sign modes.The enabled sign modes are clearly defined and include the standard Cosmos SDK signing modes.
43-48: LGTM! Clean configuration setup.The transaction configuration is well-structured with all necessary codecs and sign modes.
client/v2/tx/tx.go (1)
59-59: LGTM! Improved naming convention.The rename from
EnablesSignModestoEnabledSignModesis a good change as it:
- Uses a more appropriate adjective form for a configuration option
- Maintains consistency with related configuration structs
client/v2/tx/config_test.go (2)
114-114: LGTM: Field name change improves clarity.The rename from
EnablesSignModestoEnabledSignModesis a grammatical improvement that better reflects the field's purpose as a list of enabled sign modes.
139-139: Verify test coverage for sign mode combinations.While the assertion correctly verifies the total number of supported modes, consider adding test cases that validate specific combinations of
EnabledSignModesandCustomSignModesto ensure proper handler registration.Consider adding a test case that combines both
EnabledSignModesandCustomSignModesto verify their interaction:{ name: "handler map with both enabled and custom sign modes", opts: ConfigOptions{ AddressCodec: address.NewBech32Codec("cosmos"), Decoder: decoder, Cdc: cdc, ValidatorAddressCodec: address.NewBech32Codec("cosmosvaloper"), EnabledSignModes: []apitxsigning.SignMode{apitxsigning.SignMode_SIGN_MODE_DIRECT}, CustomSignModes: []signing.SignModeHandler{mockModeHandler{}}, }, },client/v2/tx/config.go (2)
54-57: LGTM: Well-documented interface additionsThe new text encoding/decoding methods follow consistent naming patterns and are properly documented.
86-86: LGTM: Improved field namingThe rename from
EnablesSignModestoEnabledSignModesbetter reflects the field's purpose - it represents the list of sign modes that are enabled.client/v2/offchain/verify_test.go (1)
84-87: Previous suggestion still applicableThe earlier suggestion to enhance
Test_SignVerifywith more test scenarios remains valid. It is recommended to add additional test cases to cover different encoding options, signing modes, error cases, and verification of the actual signature content.
| // getSignMode returns the corresponding apisigning.SignMode based on the provided mode string. | ||
| func getSignMode(mode string) (apisigning.SignMode, error) { | ||
| switch mode { | ||
| case "direct": | ||
| return apisigning.SignMode_SIGN_MODE_DIRECT, nil | ||
| case "amino-json": | ||
| return apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, nil | ||
| } | ||
|
|
||
| return handlerMap.GetSignBytes(ctx, signMode, txSignerData, txData) | ||
| return apisigning.SignMode_SIGN_MODE_UNSPECIFIED, fmt.Errorf("unsupported sign mode: %s", mode) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constants for sign modes.
To improve maintainability and prevent typos, consider defining constants for the sign modes.
+const (
+ SignModeDirect = "direct"
+ SignModeAminoJSON = "amino-json"
+)
func getSignMode(mode string) (apisigning.SignMode, error) {
switch mode {
- case "direct":
+ case SignModeDirect:
return apisigning.SignMode_SIGN_MODE_DIRECT, nil
- case "amino-json":
+ case SignModeAminoJSON:
return apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, nil
}
return apisigning.SignMode_SIGN_MODE_UNSPECIFIED, fmt.Errorf("unsupported sign mode: %s", mode)
}📝 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.
| // getSignMode returns the corresponding apisigning.SignMode based on the provided mode string. | |
| func getSignMode(mode string) (apisigning.SignMode, error) { | |
| switch mode { | |
| case "direct": | |
| return apisigning.SignMode_SIGN_MODE_DIRECT, nil | |
| case "amino-json": | |
| return apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, nil | |
| } | |
| return handlerMap.GetSignBytes(ctx, signMode, txSignerData, txData) | |
| return apisigning.SignMode_SIGN_MODE_UNSPECIFIED, fmt.Errorf("unsupported sign mode: %s", mode) | |
| const ( | |
| SignModeDirect = "direct" | |
| SignModeAminoJSON = "amino-json" | |
| ) | |
| // getSignMode returns the corresponding apisigning.SignMode based on the provided mode string. | |
| func getSignMode(mode string) (apisigning.SignMode, error) { | |
| switch mode { | |
| case SignModeDirect: | |
| return apisigning.SignMode_SIGN_MODE_DIRECT, nil | |
| case SignModeAminoJSON: | |
| return apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, nil | |
| } | |
| return apisigning.SignMode_SIGN_MODE_UNSPECIFIED, fmt.Errorf("unsupported sign mode: %s", mode) |
| func encode(output string, tx clitx.Tx, config clitx.TxConfig) ([]byte, error) { | ||
| switch output { | ||
| case "json": | ||
| return config.TxJSONEncoder()(tx) | ||
| case "text": | ||
| return config.TxTextEncoder()(tx) | ||
| default: | ||
| return nil, fmt.Errorf("unsupported output type: %s", output) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constants for output formats.
To improve maintainability and prevent typos, consider defining constants for the output formats.
+const (
+ OutputFormatJSON = "json"
+ OutputFormatText = "text"
+)
func encode(output string, tx clitx.Tx, config clitx.TxConfig) ([]byte, error) {
switch output {
- case "json":
+ case OutputFormatJSON:
return config.TxJSONEncoder()(tx)
- case "text":
+ case OutputFormatText:
return config.TxTextEncoder()(tx)
default:
return nil, fmt.Errorf("unsupported output type: %s", output)
}
}Committable suggestion was skipped due to low confidence.
| if len(c.EnabledSignModes) == 0 { | ||
| c.EnabledSignModes = defaultEnabledSignModes | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for sign modes
While the current validation ensures non-empty sign modes by setting defaults, consider adding validation to ensure that:
- No duplicate sign modes are provided
- All provided sign modes are valid/supported
if len(c.EnabledSignModes) == 0 {
c.EnabledSignModes = defaultEnabledSignModes
+} else {
+ // Create a map to check for duplicates
+ seen := make(map[apitxsigning.SignMode]bool)
+ for _, mode := range c.EnabledSignModes {
+ if seen[mode] {
+ return fmt.Errorf("duplicate sign mode: %v", mode)
+ }
+ seen[mode] = true
+
+ // Validate that the mode is supported
+ supported := false
+ for _, defaultMode := range defaultEnabledSignModes {
+ if mode == defaultMode {
+ supported = true
+ break
+ }
+ }
+ if !supported {
+ return fmt.Errorf("unsupported sign mode: %v", mode)
+ }
+ }
}Committable suggestion was skipped due to low confidence.
| lenSignModes := len(opts.EnabledSignModes) | ||
| handlers := make([]signing.SignModeHandler, lenSignModes+len(opts.CustomSignModes)) | ||
|
|
||
| for i, m := range opts.EnablesSignModes { | ||
| for i, m := range opts.EnabledSignModes { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing handler initialization
The current implementation pre-allocates the handlers slice and then fills it sequentially. Consider using a more efficient approach:
-lenSignModes := len(opts.EnabledSignModes)
-handlers := make([]signing.SignModeHandler, lenSignModes+len(opts.CustomSignModes))
+// Pre-calculate capacity to avoid reallocations
+handlers := make([]signing.SignModeHandler, 0, len(opts.EnabledSignModes)+len(opts.CustomSignModes))
-for i, m := range opts.EnabledSignModes {
+for _, m := range opts.EnabledSignModes {
var err error
+ var handler signing.SignModeHandler
switch m {
case apitxsigning.SignMode_SIGN_MODE_DIRECT:
- handlers[i] = &direct.SignModeHandler{}
+ handler = &direct.SignModeHandler{}
// ... rest of the cases ...
}
+ if handler != nil {
+ handlers = append(handlers, handler)
+ }
}Committable suggestion was skipped due to low confidence.
julienrbrt
added
the
backport/v0.52.x
|
@sontrinh16 / @hieuvubk could you review this? |
Co-authored-by: Julien Robert <[email protected]> (cherry picked from commit ed46a4c) # Conflicts: # client/v2/internal/testpb/msg.pulsar.go
#22437) Co-authored-by: Julián Toledano <[email protected]> Co-authored-by: Julien Robert <[email protected]>
* main: (24 commits) build(deps): upgrade to [email protected] (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Closes: #XXXX
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
New Features
Improvements
Bug Fixes
Documentation