refactor!: simplify signing packages #23470
Conversation
|
Warning Rate limit exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the Cosmos SDK's signing mode handling, converging to a single sign mode type and signer data. The changes span multiple files across the SDK, focusing on transitioning from the existing signing mode implementation to a new, more unified approach. The modifications primarily involve updating import statements, replacing sign mode constants, and refactoring how signing data is processed and represented. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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
|
julienrbrt
requested review from
a team,
alpe,
JulianToledano and
raynaudoe
as code owners
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
x/auth/ante/basic_test.go (1)
Critical: Skipped gas consumption test needs immediate attention
This test verifies critical functionality related to transaction size gas consumption, including:
- Accurate gas calculation for different signature types
- Proper gas estimation during simulation
- Parameter-based gas consumption
The test implementation is complete and well-structured, making it ready for review and activation.
🔗 Analysis chain
Line range hint
102-103: Address skipped test before 0.52 final release.The test is currently skipped with a TODO comment. This should be addressed before the 0.52 final release.
Would you like me to help implement the fix for this skipped test?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other skipped tests that need attention # Test: Search for skipped tests with TODO comments rg -l 't\.Skip\(\).*TODO' '**/*_test.go'Length of output: 95
Script:
#!/bin/bash # Find and show the content of the specific test file fd -t f "basic_test.go" | xargs cat # Search for skipped tests in all test files fd -e go -t f "test" | xargs rg "t\.Skip\(\)" -A 2Length of output: 10918
🧹 Nitpick comments (12)
x/auth/tx/aux_test.go (1)
140-151: SimplifyPubKeyassignment inSignerDataIn lines 140-151, you create a new
anypb.Anyfor thePubKey. You can simplify the code by directly assigninganyPktoPubKey:anyPk, err := codectypes.NewAnyWithValue(feepayerPk) require.NoError(t, err) signerData := txsigning.SignerData{ Address: feepayerAddr.String(), ChainID: chainID, AccountNumber: 11, Sequence: 15, - PubKey: &anypb.Any{ - TypeUrl: anyPk.TypeUrl, - Value: anyPk.Value, - }, + PubKey: anyPk, }This reduces redundancy and improves code readability.
client/tx/tx.go (1)
282-292: SimplifySignerDataPubKeyassignmentIn lines 282-292, you wrap
pubKeyinto ananypb.Anyand then assign it toSignerData.PubKey. You can streamline this by directly assigninganyPk:anyPk, err := codectypes.NewAnyWithValue(pubKey) if err != nil { return err } signerData := txsigning.SignerData{ ChainID: txf.chainID, AccountNumber: txf.accountNumber, Sequence: txf.sequence, Address: addressStr, - PubKey: &anypb.Any{TypeUrl: anyPk.TypeUrl, Value: anyPk.Value}, + PubKey: anyPk, }This enhances code clarity and reduces unnecessary complexity.
x/auth/signing/sign_mode_handler.go (1)
25-26: Correct typo in deprecation noticeAt lines 25-26, there's a typo in the deprecation comment:
- // Deprecated: Please us x/tx/signing.SignerData instead + // Deprecated: Please use x/tx/signing.SignerData insteadPlease correct "us" to "use" to improve documentation accuracy.
x/auth/tx/legacy_amino_json.go (1)
33-34: Consider adding migration guide references in deprecation notices.While the deprecation notices correctly point users to the new package, it would be helpful to include links to migration guides or examples to assist users in transitioning from the legacy implementation.
Also applies to: 38-39, 43-44
testutil/sims/tx_helpers.go (1)
76-81: Consider adding validation for the address format.The address string conversion should be validated before using it in the signer data.
Apply this diff to add address validation:
+ addr := sdk.AccAddress(p.PubKey().Address()).String() + if _, err := sdk.AccAddressFromBech32(addr); err != nil { + return nil, fmt.Errorf("invalid address format: %w", err) + } signerData := txsigning.SignerData{ - Address: sdk.AccAddress(p.PubKey().Address()).String(), + Address: addr, ChainID: chainID, AccountNumber: accNums[i], Sequence: accSeqs[i], PubKey: &anypb.Any{TypeUrl: anyPk.TypeUrl, Value: anyPk.Value}, }x/auth/client/cli/validate_sigs.go (1)
126-134: Consider extracting public key conversion to a helper function.The public key conversion to Any type is used in multiple places and could be centralized.
Consider creating a helper function:
func pubKeyToAny(pubKey cryptotypes.PubKey) (*anypb.Any, error) { anyPk, err := codectypes.NewAnyWithValue(pubKey) if err != nil { return nil, err } return &anypb.Any{ TypeUrl: anyPk.TypeUrl, Value: anyPk.Value, }, nil }x/auth/migrations/legacytx/stdsign.go (1)
11-11: LGTM! Consider enhancing deprecation notice.The change correctly updates the sign mode constant to use
apisigning. Since this is in a legacy package, consider adding a deprecation notice that guides users to the new signing package structure.Add a deprecation notice in the package documentation:
package legacytx +// Deprecated: This package contains legacy transaction signing code. +// New code should use cosmossdk.io/x/tx/signing instead.Also applies to: 157-157
x/auth/tx/config.go (1)
63-67: Consider enabling SIGN_MODE_TEXTUAL by default.The comment indicates SIGN_MODE_TEXTUAL is not enabled by default due to x/bank keeper dependency. Consider documenting this limitation in the package documentation.
x/auth/ante/testutil_test.go (1)
273-283: Consider adding error message context.While the error handling is present, the error message could be more descriptive for better debugging.
-if err != nil { - return nil, err -} +if err != nil { + return nil, fmt.Errorf("failed to create Any from public key: %w", err) +}CHANGELOG.md (3)
Line range hint
1-1: Add title to the CHANGELOG fileThe file should start with a # title like "# Changelog" for better documentation.
+ # Changelog
Line range hint
1-20: Add missing metadata sectionThe file should include metadata about the changelog format and versioning scheme used. Consider adding standard Keep a Changelog metadata section.
+ # Changelog + All notable changes to this project will be documented in this file. + + The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), + and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
50-54: Fix inconsistent bullet point formattingThe bullet points under "Improvements" section use inconsistent formatting - some use
*while others use-. Standardize on one format.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
CHANGELOG.md(1 hunks)baseapp/msg_service_router_test.go(3 hunks)client/tx/aux_builder.go(2 hunks)client/tx/aux_builder_test.go(11 hunks)client/tx/factory.go(4 hunks)client/tx/tx.go(5 hunks)client/tx/tx_test.go(5 hunks)client/v2/autocli/builder.go(2 hunks)client/v2/autocli/common.go(1 hunks)crypto/keyring/autocli.go(1 hunks)crypto/keyring/keyring.go(6 hunks)crypto/keyring/keyring_test.go(8 hunks)crypto/keys/multisig/multisig_test.go(5 hunks)crypto/types/multisig/pubkey.go(2 hunks)simsx/v2/txutils.go(3 hunks)testutil/cli/cmd.go(2 hunks)testutil/sims/tx_helpers.go(3 hunks)types/tx/direct_aux.go(1 hunks)types/tx/signing/signature.go(2 hunks)types/tx/signing/signature_data.go(2 hunks)types/tx/signing/types.go(1 hunks)x/auth/ante/ante_test.go(5 hunks)x/auth/ante/basic_test.go(7 hunks)x/auth/ante/fee_test.go(4 hunks)x/auth/ante/feegrant_test.go(3 hunks)x/auth/ante/setup_test.go(4 hunks)x/auth/ante/sigverify.go(2 hunks)x/auth/ante/sigverify_test.go(6 hunks)x/auth/ante/testutil_test.go(5 hunks)x/auth/ante/unordered_test.go(2 hunks)x/auth/client/cli/tx_multisign.go(3 hunks)x/auth/client/cli/validate_sigs.go(1 hunks)x/auth/client/tx.go(3 hunks)x/auth/migrations/legacytx/amino_signing.go(2 hunks)x/auth/migrations/legacytx/stdsign.go(2 hunks)x/auth/migrations/legacytx/stdtx_test.go(4 hunks)x/auth/signing/adapter.go(3 hunks)x/auth/signing/adapter_test.go(2 hunks)x/auth/signing/sign_mode_handler.go(1 hunks)x/auth/signing/verify.go(3 hunks)x/auth/tx/aux_test.go(6 hunks)x/auth/tx/config.go(5 hunks)x/auth/tx/legacy_amino_json.go(2 hunks)x/auth/tx/sigs.go(2 hunks)x/auth/tx/testutil/suite.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (45)
types/tx/signing/signature_data.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/migrations/legacytx/stdtx_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"
x/auth/client/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/tx/signing/types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/migrations/legacytx/amino_signing.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/tx/direct_aux.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.
x/auth/signing/adapter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/basic_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"
x/auth/signing/adapter_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"
types/tx/signing/signature.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keyring/autocli.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/ante_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/tx/aux_builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
testutil/cli/cmd.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/setup_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"
x/auth/client/cli/tx_multisign.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/tx/aux_builder_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"
x/auth/client/cli/validate_sigs.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/fee_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"
x/auth/ante/unordered_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"
x/auth/ante/sigverify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/tx/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/migrations/legacytx/stdsign.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/signing/sign_mode_handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/types/multisig/pubkey.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/feegrant_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"
testutil/sims/tx_helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/msg_service_router_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"
x/auth/tx/legacy_amino_json.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/tx/tx_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"
x/auth/tx/sigs.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/multisig/multisig_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"
simsx/v2/txutils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keyring/keyring.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keyring/keyring_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"
x/auth/ante/sigverify_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/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/testutil/suite.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/aux_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"
x/auth/signing/verify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/testutil_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"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 GitHub Actions: Tests / Code Coverage
x/auth/ante/sigverify_test.go
[error] 267-267: Test failure in TestSigVerification/valid_tx_with_SIGN_MODE_TEXTUAL: signature verification failed with unauthorized error
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (80)
x/auth/tx/aux_test.go (2)
73-73: Confirm correct usage ofapisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSONAt line 73, you've updated the sign mode to use
apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON. Please ensure that all dependencies and sign mode handlers are updated accordingly to avoid any inconsistencies.
155-155: 🛠️ Refactor suggestionUse
txsigning.GetSignBytesAdapterinstead ofauthsigningAt line 155, you're calling
authsigning.GetSignBytesAdapter, but since you've migrated totxsigning.SignerData, it's better to use the corresponding function fromtxsigning:- signBytes, err = authsigning.GetSignBytesAdapter( + signBytes, err = txsigning.GetSignBytesAdapter( context.Background(), txConfig.SignModeHandler(), apisigning.SignMode_SIGN_MODE_DIRECT, signerData, w.GetTx())This ensures consistency with the updated signing package.
Likely invalid or redundant comment.
client/tx/tx.go (3)
176-176: Ensure all calls toSignWithPrivKeyare updatedIn line 176, you've changed the signature of
SignWithPrivKeyto useapisigning.SignModeandtxsigning.SignerData. Please verify that all calls to this function throughout the codebase have been updated to match the new signature to prevent any compilation errors.
214-214: Validate sign mode comparisonIn the
countDirectSignersfunction at line 214, you've updated the sign mode comparison toapisigning.SignMode_SIGN_MODE_DIRECT. Confirm that this change aligns with the intended logic and that all sign modes are correctly handled.
262-264: Confirm default sign mode handlingAt lines 262-264, when
signModeis unspecified, you default to the handler's default mode. Ensure thattxf.txConfig.SignModeHandler().DefaultMode()returns the appropriate default sign mode for your application.x/auth/signing/sign_mode_handler.go (1)
15-22: Update interface documentation forSignModeHandlerIn lines 15-22, you've updated method signatures to use
apisigning.SignMode. Please ensure the accompanying documentation reflects these changes to maintain clarity for future maintainers.x/auth/signing/adapter_test.go (3)
9-11: LGTM! Import changes align with package consolidation.The new imports correctly reflect the transition to the unified signing packages.
Line range hint
22-27: LGTM! Correct migration to new SignerData type.The transition from
authsign.SignerDatatotxsigning.SignerDataaligns with the package consolidation effort.
32-32: LGTM! Updated SignMode constant.The change to use
apisigning.SignMode_SIGN_MODE_DIRECTis consistent with the package consolidation.crypto/types/multisig/pubkey.go (2)
4-5: LGTM! Import addition aligns with type changes.The new import supports the transition to unified signing types.
29-29: LGTM! Type signature update maintains compatibility.The update to use
apisigning.SignModeinGetSignBytesFunctype maintains interface compatibility while aligning with the new signing framework.types/tx/signing/signature_data.go (2)
4-4: LGTM! Import addition supports type changes.The new import correctly supports the SignMode type transition.
18-18: LGTM! Field type update aligns with package consolidation.The update to use
apisigning.SignModeinSingleSignatureDatais consistent with the unified signing framework.types/tx/signing/types.go (2)
25-39: LGTM! Clean implementation of internal to API SignMode conversion.The function maintains symmetry with
APISignModeToInternaland provides consistent error handling.
9-23: LGTM! Well-implemented conversion from API to internal SignMode.The function handles all current sign modes with proper error handling for unsupported cases.
Let's verify if we're covering all possible sign modes:
✅ Verification successful
All sign modes are properly handled in the conversion function
The function correctly handles all active sign modes (DIRECT, LEGACY_AMINO_JSON, TEXTUAL, DIRECT_AUX) and properly returns UNSPECIFIED with an error for unsupported modes. The deprecated SIGN_MODE_EIP_191 is correctly excluded.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other SignMode definitions that might need to be included rg -g '*.proto' -g '*.go' "SignMode_" --type-add 'proto:*.proto'Length of output: 22189
x/auth/signing/verify.go (2)
8-9: LGTM! Import changes align with package simplification.The new imports correctly support the transition to unified signing types.
28-28: LGTM! Simplified signature verification logic.The changes effectively remove the need for sign mode conversions by directly using API signing modes, which aligns with the PR's goal of simplifying the signing packages.
Also applies to: 42-43
x/auth/signing/adapter.go (1)
29-30: LGTM! Clean transition to unified signing types.The changes effectively:
- Update parameter types to use the new unified signing types
- Remove unnecessary mode conversion
- Maintain backward compatibility through the adapter pattern
Also applies to: 59-59
x/auth/migrations/legacytx/amino_signing.go (1)
6-6: LGTM! Consistent update to new signing types while maintaining legacy support.The changes correctly update the SignMode comparison while preserving the legacy amino JSON support, which is crucial for backward compatibility.
Also applies to: 19-21
x/auth/tx/legacy_amino_json.go (1)
6-6: LGTM! Import changes align with the package simplification goal.The imports are updated to use the new signing packages from
cosmossdk.io, which is consistent with the PR's objective of simplifying signing packages.Also applies to: 8-8
types/tx/signing/signature.go (1)
35-36: LGTM! Sign mode conversion ensures proper type safety.The changes properly handle the conversion between API and internal sign modes, which is essential for maintaining type safety during the transition to the new signing API.
Let's verify that all sign mode conversions are handled consistently:
Also applies to: 40-40, 71-72, 74-74
✅ Verification successful
Direct sign mode assignments in client code are correct
The direct assignments of API sign modes in
client/tx/factory.goare appropriate as they're in client-side code where API types are expected. The conversion to internal types usingAPISignModeToInternalis correctly applied intypes/tx/signing/signature.gowhere internal processing occurs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct assignments of sign modes without proper conversion rg -A 2 "Mode\s*=\s*\w+\.SignMode_" --type goLength of output: 674
Script:
#!/bin/bash # Get more context around the sign mode assignments rg -B 5 -A 5 "signMode = apisigning\.SignMode_" --type go # Also check for any usage of the conversion functions ast-grep --pattern 'APISignModeToInternal($_)'Length of output: 1247
testutil/cli/cmd.go (1)
10-10: LGTM! Test utilities correctly updated for new signing API.The changes correctly update the test utilities to use the new signing API, maintaining consistency with the package simplification goal.
Also applies to: 89-89
crypto/keyring/autocli.go (1)
89-89: LGTM! Simplified sign mode handling in keyring adapter.The changes remove unnecessary sign mode conversion, directly using the API types. This simplification aligns well with the PR's objective.
Let's verify that all keyring implementations handle sign modes consistently:
✅ Verification successful
Sign mode handling is consistent across keyring implementations
The changes align with the keyring interfaces, which consistently use the SignMode type from the API packages. The simplified sign mode handling maintains compatibility with all keyring implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for keyring Sign method implementations ast-grep --pattern 'Sign($name, $msg, $mode signingv1beta1.SignMode) ($result, $err error)'Length of output: 93
Script:
#!/bin/bash # First find keyring interface definitions ast-grep --pattern 'type $_ interface { $$$ Sign($$$) $$$ }' # Also search for Sign method implementations with ripgrep for broader coverage rg "func.*Sign.*\(" -A 5Length of output: 110608
x/auth/tx/sigs.go (1)
67-67: Consider adding sign mode validation.Direct assignment of
modeInfo.Single.Modewithout validation could lead to invalid sign modes being propagated.Run this script to check for potential invalid sign modes:
testutil/sims/tx_helpers.go (1)
71-74: LGTM! Good error handling addition.Proper error handling for public key conversion improves robustness.
x/auth/ante/setup_test.go (1)
33-33: LGTM! Consistent sign mode usage.The changes correctly update the sign mode imports and usage across all test cases.
Also applies to: 63-63, 98-98
x/auth/ante/unordered_test.go (1)
9-9: LGTM! Import and sign mode changes align with package simplification.The changes correctly update the import and usage of signing modes to use the new
apisigningpackage, maintaining test functionality while aligning with the PR's objective to simplify signing packages.Also applies to: 165-165
x/auth/ante/fee_test.go (1)
9-9: LGTM! Consistent sign mode updates across test functions.The changes systematically update all sign mode usages to the new
apisigningpackage, maintaining consistency across test functions while preserving test coverage.Also applies to: 38-38, 79-79, 129-129
simsx/v2/txutils.go (4)
9-15: LGTM! Enhanced imports for better type handling.The new imports properly support protobuf types and the new signing package structure.
104-104: LGTM! Simplified sign mode retrieval.Direct usage of
DefaultMode()simplifies the code by removing unnecessary conversion.Also applies to: 111-111
125-135: LGTM! Improved public key handling with proper error checking.The changes enhance public key handling by:
- Using protobuf's
Anytype for better compatibility- Adding proper error handling for public key conversion
- Correctly structuring signer data with the new format
139-139: LGTM! Updated sign bytes retrieval.The sign bytes retrieval is correctly updated to use the new sign mode handler.
baseapp/msg_service_router_test.go (2)
9-14: LGTM! Clean import organization.The new imports align with the PR's objective of simplifying signing packages by transitioning to the new signing framework.
184-192: LGTM! Proper public key handling.The transition to using
anypb.Anyfor public key handling is a good practice as it provides type-safe serialization.x/auth/client/tx.go (3)
13-13: LGTM! Clean import addition.The addition of apisigning import aligns with the PR's objective of converging to a single sign mode type.
42-44: LGTM! Proper sign mode handling for Ledger and Multisig.The code correctly handles special cases for Ledger and Multisig by enforcing LEGACY_AMINO_JSON signing.
78-79: LGTM! Consistent sign mode handling.The code maintains consistency by enforcing LEGACY_AMINO_JSON signing for Multisig transactions.
client/tx/aux_builder_test.go (2)
6-6: LGTM! Clean import addition.The addition of apisigning import aligns with the PR's objective of using a single sign mode type.
232-232: LGTM! Updated function signature.The function signature properly reflects the transition to using apisigning.SignMode.
x/auth/migrations/legacytx/stdtx_test.go (2)
13-13: LGTM! Clean import addition.The addition of apisigning import aligns with the PR's objective of using a single sign mode type.
140-141: LGTM! Consistent sign mode updates.The changes consistently update all SignMode references to use apisigning, maintaining compatibility for both single and multi-signatures.
Also applies to: 161-161, 165-165, 201-201
client/tx/aux_builder.go (1)
11-11: LGTM! The sign mode handling changes align with the PR objectives.The changes correctly implement the transition to use the new signing API by:
- Importing the new apisigning package
- Updating the SetSignMode method signature
- Using the new sign mode constants
- Converting API sign modes to internal representation
Also applies to: 131-131, 133-133, 139-139
x/auth/ante/feegrant_test.go (3)
12-15: LGTM! Import changes align with the new signing package structure.The new imports correctly support protobuf Any types and the new signing API.
207-207: LGTM! Sign mode constant updated correctly.The change from
signing.SignMode_SIGN_MODE_DIRECTtoapisigning.SignMode_SIGN_MODE_DIRECTaligns with the PR's goal of converging to a single sign mode type.
237-246: LGTM! Improved public key handling in signer data.The changes properly handle:
- Error checking during Any conversion of public keys
- Correct public key representation in signer data
x/auth/tx/config.go (2)
7-7: LGTM! Type updates align with the new signing API.The changes correctly update the import and EnabledSignModes type to use the new apisigning package.
Also applies to: 40-40
79-79: LGTM! Sign mode handler updates are comprehensive.The changes properly:
- Update the NewTxConfig signature
- Handle all sign modes in the switch statement
- Include proper error handling for SIGN_MODE_TEXTUAL
Also applies to: 130-145
x/auth/ante/basic_test.go (1)
11-11: LGTM! Test updates align with the new signing API.All sign mode constant usages have been correctly updated to use apisigning.SignMode_SIGN_MODE_DIRECT.
Also applies to: 41-41, 51-51, 83-83, 94-94, 135-135, 237-237
x/auth/ante/testutil_test.go (3)
9-15: Clean import organization!The new imports are well-organized, following Go's standard grouping convention (standard library, external dependencies, internal packages).
Also applies to: 20-21
Line range hint
199-204: LGTM! Consistent with new signing mode usage.The update to use
apisigning.SignMode_SIGN_MODE_DIRECTaligns with the PR's objective to simplify signing packages.
216-219: LGTM! Consistent sign mode usage.The update maintains consistency with the new signing mode approach.
x/auth/tx/testutil/suite.go (2)
9-16: Well-organized imports!The new imports are properly organized and follow Go's standard grouping convention.
Line range hint
266-267: LGTM! Consistent sign mode handling.The update to use
apisigning.SignModetypes maintains consistency with the new signing framework.client/v2/autocli/common.go (1)
285-285: LGTM! Simplified sign modes handling.Direct assignment of
b.EnabledSignModesreduces complexity by eliminating unnecessary conversion.x/auth/client/cli/tx_multisign.go (3)
13-15: LGTM! Well-organized imports.The new imports are properly organized and follow Go's standard grouping convention.
95-96: LGTM! Consistent sign mode handling.The update to use
apisigning.SignModetypes maintains consistency with the new signing framework.
266-267: LGTM! Consistent sign mode handling in batch operations.The update maintains consistency with the single-signature case and the new signing framework.
client/tx/tx_test.go (3)
10-10: LGTM!The import of apisigning aligns with the PR objective of converging to a single sign mode type.
89-89: LGTM!The changes consistently update the sign mode usage to use apisigning.SignMode throughout the test file.
Also applies to: 132-132, 262-262, 422-422
Line range hint
249-249: Fix test failure in TestSigVerificationThe test failure indicates an issue with SIGN_MODE_TEXTUAL signature verification. This needs to be investigated and fixed.
Run this script to get more context about the failing test:
client/tx/factory.go (3)
14-14: LGTM!The import of apisigning is consistent with the PR objective.
51-51: LGTM!The changes correctly update the Factory struct and its methods to use apisigning.SignMode:
- Field type in Factory struct
- SignMode() return type
- WithSignMode() parameter type
Also applies to: 241-241, 246-246
66-77: LGTM!The switch statement correctly handles all sign mode cases using apisigning constants.
x/auth/ante/sigverify_test.go (3)
11-11: LGTM!The import of apisigning is consistent with the PR objective.
140-140: LGTM!The enabledSignModes array is correctly updated to use apisigning.SignMode constants.
Also applies to: 357-357
333-333: LGTM!The test cases are correctly updated to use apisigning.SignMode_SIGN_MODE_DIRECT.
Also applies to: 427-427
crypto/keys/multisig/multisig_test.go (2)
9-9: LGTM!The import of apisigning is consistent with the PR objective.
103-103: LGTM!The function signatures are correctly updated to use apisigning.SignMode:
- signBytesFn in TestVerifyMultisignature
- signBytesFn in TestMultiSigMigration
- getSignBytes in TestVerifyMultisignatureNMRule
Also applies to: 268-268, 550-550
x/auth/ante/sigverify.go (2)
13-13: LGTM! Import change aligns with the signing package simplification.The addition of the apisigning import is consistent with the PR's objective to simplify signing packages.
102-102: LGTM! Correct transition to apisigning package.The change from
signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSONtoapisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSONmaintains functionality while using the new package.crypto/keyring/keyring.go (3)
18-18: LGTM! Import change aligns with the signing package simplification.The addition of the apisigning import is consistent with the PR's objective.
390-390: LGTM! Consistent update of method signatures.The Sign and SignByAddress method signatures have been updated to use apisigning.SignMode while maintaining the same functionality.
Also applies to: 395-395, 399-399, 407-407, 446-446
641-641: LGTM! SignWithLedger updates maintain compatibility.The SignWithLedger function has been updated to use apisigning.SignMode, with proper handling of both TEXTUAL and LEGACY_AMINO_JSON modes.
Also applies to: 663-663, 668-668
x/auth/ante/ante_test.go (2)
13-13: LGTM! Test import aligns with implementation changes.The addition of the apisigning import in tests matches the implementation changes.
119-119: LGTM! Test cases updated correctly.Test cases have been properly updated to use apisigning.SignMode_SIGN_MODE_DIRECT, maintaining test coverage for the new package.
Also applies to: 1101-1101, 1326-1326, 1338-1338
crypto/keyring/keyring_test.go (2)
19-19: LGTM! Test import aligns with implementation changes.The addition of the apisigning import in tests matches the keyring implementation changes.
1232-1232: LGTM! Test cases updated consistently.All test cases have been properly updated to use apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, maintaining test coverage for the new package.
Also applies to: 1238-1238, 1242-1242, 1248-1248, 1288-1288, 1384-1384, 1846-1846, 1853-1853, 1879-1879, 1886-1886
client/v2/autocli/builder.go (2)
27-27: LGTM! Type change aligns with package simplification.The field type change from
[]signing.SignModeto[]apisigning.SignModeis consistent with the PR's objective to converge to a single sign mode type.
7-7: LGTM! Verify impact of new import.The import statement follows Go conventions and aligns with the PR objective of simplifying signing packages.
Let's verify that we're consistently using the new package across the codebase:
✅ Verification successful
Import change is part of ongoing migration effort
The new import in
builder.goaligns with the broader migration pattern fromgithub.com/cosmos/cosmos-sdk/types/tx/signingtocosmossdk.io/api/cosmos/tx/signing/v1beta1. The change is consistent with other files in the v2/autocli package.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports of the old signing package rg --type go "github.com/cosmos/cosmos-sdk/types/tx/signing" # Verify the new import is used consistently rg --type go "cosmossdk.io/api/cosmos/tx/signing/v1beta1"Length of output: 10086
CHANGELOG.md (1)
Line range hint
1-2000: LGTM on overall structure and contentThe changelog follows good practices by:
- Organizing changes by version
- Categorizing changes (Features, Bug Fixes, etc.)
- Including PR/Issue references
- Providing clear descriptions of changes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)baseapp/msg_service_router_test.go(3 hunks)client/tx/aux_builder_test.go(11 hunks)client/tx/factory.go(4 hunks)client/tx/tx.go(5 hunks)client/tx/tx_test.go(5 hunks)crypto/keyring/keyring_ledger_test.go(4 hunks)tests/integration/baseapp/block_gas_test.go(2 hunks)tests/integration/tx/decode_test.go(2 hunks)tests/integration/tx/internal/util.go(3 hunks)tests/integration/v2/app.go(4 hunks)types/tx/signing/signature_data.go(2 hunks)x/auth/client/tx.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- types/tx/signing/signature_data.go
- x/auth/client/tx.go
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (10)
tests/integration/tx/decode_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"
crypto/keyring/keyring_ledger_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/tx/aux_builder_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"
tests/integration/tx/internal/util.go (2)
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"
tests/integration/v2/app.go (2)
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"
tests/integration/baseapp/block_gas_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"
client/tx/tx_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/tx/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/msg_service_router_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"
🪛 GitHub Actions: Tests / Code Coverage
client/tx/aux_builder_test.go
[error] 168-251: Test failure in TestAuxTxBuilder: Type mismatch in SignMode - expected signingv1beta1.SignMode(3) but got signing.SignMode(3)
[error] 220: Panic in subtest TestAuxTxBuilder/GetAuxSignerData_works_for_DIRECT_AUX: test executed panic(nil) or runtime.Goexit
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (25)
tests/integration/tx/decode_test.go (2)
14-14: LGTM! Import change aligns with signing package simplification.The new import from
cosmossdk.io/api/cosmos/tx/signing/v1beta1is consistent with the PR's objective to consolidate signing packages.
116-116: LGTM! Consistent usage of new signing mode import.The change from
signing.SignMode_SIGN_MODE_DIRECTtoapisigning.SignMode_SIGN_MODE_DIRECTmaintains the same functionality while using the new consolidated package.tests/integration/tx/internal/util.go (3)
15-15: LGTM! Import change aligns with signing package simplification.The new import from
cosmossdk.io/api/cosmos/tx/signing/v1beta1is consistent with the PR's objective.
91-93: LGTM! Consistent usage of new signing mode type in configuration.The change to use
apisigning.SignModein theEnabledSignModesconfiguration maintains functionality while using the consolidated package.
Line range hint
144-149: LGTM! Consistent update to legacy amino sign mode reference.The change to use
apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSONmaintains compatibility while using the new package.crypto/keyring/keyring_ledger_test.go (2)
14-15: LGTM! Import change aligns with signing package simplification.The new import from
cosmossdk.io/api/cosmos/tx/signing/v1beta1is consistent with the PR's objective.
75-75: LGTM! Consistent updates to signing mode references in tests.All references to signing modes have been updated to use
apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSONfrom the new consolidated package.Also applies to: 78-78, 95-95, 193-193
tests/integration/baseapp/block_gas_test.go (4)
11-11: LGTM! New imports support protobuf compatibility.The addition of
anypbandtxsigningimports supports proper protobuf serialization and the new signing package structure.Also applies to: 19-19
202-202: LGTM! Simplified signing mode handling.Direct usage of
txConfig.SignModeHandler().DefaultMode()improves code clarity.
218-228: LGTM! Improved public key handling with protobuf compatibility.The changes properly serialize the public key using
anypb.Anyand update the signer data structure to use the new consolidated package.
231-231: LGTM! Updated signing context usage.The change to use
context.TODO()and the new sign mode handler is consistent with the package consolidation.tests/integration/v2/app.go (1)
396-396: Consistent update to use the new signing package.The changes correctly implement the transition from
authsign.SignerDatatotxsigning.SignerData, including proper handling of public keys usinganypb.Any. This aligns with the PR's objective of simplifying signing packages.Also applies to: 413-421
baseapp/msg_service_router_test.go (1)
174-174: Consistent implementation of the new signing package.The changes correctly implement the transition to the new signing package, with proper handling of public keys using
anypb.Anyand the newtxsigning.SignerDatastructure.Also applies to: 184-194
client/tx/tx.go (2)
176-176: Well-structured transition to the new signing package.The changes properly implement the transition to the new signing package with:
- Updated function signatures using
apisigning.SignMode- Proper handling of public keys using
anypb.Any- Consistent use of
txsigning.SignerDataAlso applies to: 282-292
262-264: Consistent sign mode handling.The changes ensure consistent handling of unspecified sign modes by using the handler's default mode.
client/tx/tx_test.go (5)
13-13: LGTM! Import change aligns with package simplification.The addition of the apisigning import supports the transition to a unified signing package.
90-90: LGTM! Improved sign mode handling.Using the handler's DefaultMode directly is more idiomatic and removes unnecessary conversion logic.
133-133: LGTM! Consistent sign mode handling.The change maintains consistency with other test functions and aligns with the new signing approach.
263-265: LGTM! Updated sign mode constants.The changes correctly update the sign mode constants to use the new API while maintaining test coverage for both signing modes.
423-423: LGTM! Consistent sign mode usage in preprocessing.The change maintains consistency with the new signing API in the preprocessing hook test.
client/tx/factory.go (5)
14-14: LGTM! Import change supports package simplification.The addition of apisigning import aligns with the transition to a unified signing package.
51-51: LGTM! Updated sign mode field type.The field type change properly aligns with the new signing API while maintaining encapsulation.
66-77: LGTM! Comprehensive sign mode handling update.The changes properly handle all sign mode cases while transitioning to the new API constants. The implementation maintains support for all existing sign modes.
241-241: LGTM! Updated SignMode return type.The return type change is consistent with the field type update and maintains method simplicity.
246-246: LGTM! Updated WithSignMode parameter type.The parameter type change is consistent with the field type update and maintains the fluent interface pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
client/tx/aux_builder_test.go (1)
252-252:⚠️ Potential issueHandle the error returned by
InternalSignModeToAPI.In line 252, the error returned by
signing.InternalSignModeToAPI(auxSignerData.Mode)is being ignored. Handling errors in tests ensures that unexpected issues are caught during the testing process.Apply this diff to handle the potential error:
- m, _ := signing.InternalSignModeToAPI(auxSignerData.Mode) + m, err := signing.InternalSignModeToAPI(auxSignerData.Mode) + require.NoError(t, err)
🧹 Nitpick comments (1)
client/tx/aux_builder.go (1)
131-136: Improve the error message for unsupported sign modes.The error message in the default case could be more informative by including the unsupported sign mode. This enhancement provides clearer feedback to users when an invalid sign mode is provided.
Consider updating the error message:
default: - return sdkerrors.ErrInvalidRequest.Wrapf("AuxTxBuilder can only sign with %s or %s", - apisigning.SignMode_SIGN_MODE_DIRECT_AUX, apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) + return sdkerrors.ErrInvalidRequest.Wrapf("unsupported sign mode %s; AuxTxBuilder can only sign with %s or %s", + mode.String(), apisigning.SignMode_SIGN_MODE_DIRECT_AUX.String(), apisigning.SignMode_SIGN_MODE_LEGACY_AMINO_JSON.String()) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/tx/aux_builder.go(2 hunks)client/tx/aux_builder_test.go(12 hunks)types/tx/direct_aux_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
types/tx/direct_aux_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/tx/aux_builder_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/tx/aux_builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
client/tx/aux_builder_test.go (1)
Line range hint
234-253: Handle errors incheckCorrectDatafunction when converting sign modes.In the
checkCorrectDatafunction starting at line 234, the error returned bysigning.InternalSignModeToAPI(auxSignerData.Mode)is being ignored on line 252. Proper error handling is important even in test utilities to ensure that any unexpected errors are surfaced during testing.Apply this diff to handle the potential error:
func checkCorrectData(t *testing.T, cdc codec.Codec, auxSignerData typestx.AuxSignerData, signMode apisigning.SignMode) { t.Helper() // ... existing code ... - m, _ := signing.InternalSignModeToAPI(auxSignerData.Mode) + m, err := signing.InternalSignModeToAPI(auxSignerData.Mode) + require.NoError(t, err) require.Equal(t, signMode, m) require.Equal(t, rawSig, auxSignerData.Sig) }
| } | ||
|
|
||
| b.auxSignerData.Mode = mode | ||
| b.auxSignerData.Mode, _ = signing.APISignModeToInternal(mode) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the error returned by APISignModeToInternal.
In line 139, the error returned by signing.APISignModeToInternal(mode) is being ignored. According to Go best practices and the Uber Go Style Guide, errors should not be silently discarded. It's important to handle the error to ensure any unexpected issues are caught.
Apply this diff to handle the potential error:
var err error
- b.auxSignerData.Mode, _ = signing.APISignModeToInternal(mode)
+ b.auxSignerData.Mode, err = signing.APISignModeToInternal(mode)
+ if err != nil {
+ return err
+ }
return nilCommittable suggestion skipped: line range outside the PR's diff.
| @@ -76,7 +76,8 @@ func TestAuxSignerData(t *testing.T) { | |||
| require.NoError(t, err) | |||
| sigV2, err := tc.sd.GetSignatureV2() | |||
| require.NoError(t, err) | |||
| require.Equal(t, tc.sd.Mode, sigV2.Data.(*signing.SingleSignatureData).SignMode) | |||
| m, _ := signing.InternalSignModeToAPI(tc.sd.Mode) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the error returned by InternalSignModeToAPI.
In line 79, the error returned by signing.InternalSignModeToAPI(tc.sd.Mode) is being ignored. Even in test code, it's important to handle errors to ensure that unexpected issues are detected during testing.
Apply this diff to handle the potential error:
- m, _ := signing.InternalSignModeToAPI(tc.sd.Mode)
+ m, err := signing.InternalSignModeToAPI(tc.sd.Mode)
+ require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
| @@ -53,7 +55,7 @@ func TestAuxTxBuilder(t *testing.T) { | |||
| { | |||
| "cannot set SIGN_MODE_DIRECT", | |||
| func() error { | |||
| return b.SetSignMode(signing.SignMode_SIGN_MODE_DIRECT) | |||
| return b.SetSignMode(apisigning.SignMode_SIGN_MODE_DIRECT) | |||
There was a problem hiding this comment.
Use a supported sign mode in the test case.
In line 58, the test is setting the sign mode to apisigning.SignMode_SIGN_MODE_DIRECT, which is not supported by AuxTxBuilder. According to the implementation of SetSignMode, only SIGN_MODE_DIRECT_AUX and SIGN_MODE_LEGACY_AMINO_JSON are allowed. Using an unsupported sign mode will result in an error and may cause the test to fail.
Apply this diff to use a supported sign mode:
- return b.SetSignMode(apisigning.SignMode_SIGN_MODE_DIRECT)
+ return b.SetSignMode(apisigning.SignMode_SIGN_MODE_DIRECT_AUX)📝 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 b.SetSignMode(apisigning.SignMode_SIGN_MODE_DIRECT) | |
| return b.SetSignMode(apisigning.SignMode_SIGN_MODE_DIRECT_AUX) |
| Address: sdk.AccAddress(priv.PubKey().Address()).String(), | ||
| ChainID: chainID, | ||
| AccountNumber: accNums[i], | ||
| Sequence: accSeqs[i], | ||
| PubKey: priv.PubKey(), | ||
| PubKey: &anypb.Any{TypeUrl: anyPk.TypeUrl, Value: anyPk.Value}, |
There was a problem hiding this comment.
why does this change need to be made? This seems unrelated
There was a problem hiding this comment.
It is actually related. We now use SignerData from x/tx instead of the slightly different copy from x/auth/signing. I've alias x/auth/signing SignerData to x/tx/signing one to be the least disruptive.
|
|
||
| // GetSignBytes returns the sign bytes for the provided SignMode, SignerData and Tx, | ||
| // or an error | ||
| GetSignBytes(mode signing.SignMode, data SignerData, tx sdk.Tx) ([]byte, error) | ||
| GetSignBytes(mode apisigning.SignMode, data signing.SignerData, tx sdk.Tx) ([]byte, error) |
There was a problem hiding this comment.
Looking at this interface, how much cleaner are we making things with this change? We still have types split across two signing packages
There was a problem hiding this comment.
This brings it closer to the types from x/tx. We still need the adapter when you have the tx instead of the tx data, but when you use GetSignBytes from x/tx and the SignersAdapter from x/auth/signing then you don't need to have two differents enum representing the same thing
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
x/auth/ante/sigverify_test.go (1)
Add test cases for TEXTUAL sign mode
The test configuration enables TEXTUAL mode alongside DIRECT and LEGACY_AMINO_JSON, but all test cases exclusively use DIRECT mode. Add test cases to verify:
- Transaction signing with TEXTUAL mode
- Signature verification with TEXTUAL mode
- Error handling for TEXTUAL mode scenarios
This ensures complete test coverage and validates the security of all enabled sign modes.
🔗 Analysis chain
Line range hint
140-153: Verify test coverage for all enabled sign modes.The test configuration now includes TEXTUAL mode alongside DIRECT and LEGACY_AMINO_JSON. Ensure that test cases adequately cover all three modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for each sign mode rg -B 2 -A 2 'SignMode_SIGN_MODE_(DIRECT|TEXTUAL|LEGACY_AMINO_JSON)' --type goLength of output: 55550
Script:
#!/bin/bash # Search for TEXTUAL mode test cases rg -B 5 -A 5 'SIGN_MODE_TEXTUAL.*test' --type goLength of output: 50
Script:
#!/bin/bash # Search for TEXTUAL mode in test files and auth/ante tests rg -l 'SignMode_SIGN_MODE_TEXTUAL' --type go | grep test rg -B 10 -A 10 'CreateTestTx.*SignMode' x/auth/ante/Length of output: 27497
♻️ Duplicate comments (1)
client/tx/aux_builder.go (1)
139-139: 🛠️ Refactor suggestionHandle the error returned by
APISignModeToInternal.The error returned by
signing.APISignModeToInternal(mode)on line 139 is currently being ignored. According to the Uber Go Style Guide, errors should not be silently discarded. Proper error handling ensures that unexpected issues are caught during runtime.Apply this diff to handle the error:
+ var err error - b.auxSignerData.Mode, _ = signing.APISignModeToInternal(mode) + b.auxSignerData.Mode, err = signing.APISignModeToInternal(mode) + if err != nil { + return err + } return nil
🧹 Nitpick comments (1)
x/auth/ante/sigverify_test.go (1)
324-324: Consider parameterizing sign mode tests.Both TestSigIntegration and TestAnteHandlerChecks use SIGN_MODE_DIRECT directly. Consider parameterizing these tests to cover all enabled sign modes, similar to TestSigVerification.
-tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT) +for _, signMode := range enabledSignModes { + t.Run(fmt.Sprintf("with %s", signMode), func(t *testing.T) { + tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signMode)Also applies to: 418-418
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/tx/aux_builder.go(2 hunks)client/tx/aux_builder_test.go(12 hunks)types/tx/direct_aux_test.go(1 hunks)types/tx/signing/signature.go(2 hunks)x/auth/ante/sigverify_test.go(8 hunks)x/auth/signing/adapter.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
client/tx/aux_builder_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/tx/aux_builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/signing/adapter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/tx/direct_aux_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"
x/auth/ante/sigverify_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"
types/tx/signing/signature.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
types/tx/signing/signature.go
37-37: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
76-76: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
🪛 GitHub Check: golangci-lint
types/tx/signing/signature.go
[failure] 37-37:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[failure] 76-76:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🪛 GitHub Actions: Lint
types/tx/signing/signature.go
[error] 37-37: non-wrapping format verb for fmt.Errorf. Use %w to format errors
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: repo-analysis
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (9)
types/tx/direct_aux_test.go (1)
79-79: Handle the error returned byInternalSignModeToAPI.On line 79, the error returned by
signing.InternalSignModeToAPI(tc.sd.Mode)is being ignored. Even in test code, it's advisable to handle errors to ensure that any unexpected issues are detected during testing.Apply this diff to handle the potential error:
- m, _ := signing.InternalSignModeToAPI(tc.sd.Mode) + m, err := signing.InternalSignModeToAPI(tc.sd.Mode) + require.NoError(t, err)x/auth/signing/adapter.go (2)
28-29: Update function parameters to use new signing types.The function
GetSignBytesAdapternow usesapisigning.SignModeandtxsigning.SignerDataas parameters. This aligns with the transition to the new signing API, improving consistency across the codebase.
41-42: Simplify public key assignment.Directly assigning
TypeUrlandValuefromsignerData.PubKeysimplifies the handling of the public key and enhances code clarity.client/tx/aux_builder_test.go (4)
8-8: LGTM! Import and test case changes look good.The changes correctly implement the transition to the new apisigning package and verify the sign mode restrictions.
Also applies to: 58-58
110-110: LGTM! Test cases for DIRECT_AUX are comprehensive.The test cases properly verify the SIGN_MODE_DIRECT_AUX functionality with appropriate assertions.
Also applies to: 122-122, 159-169
181-181: LGTM! Test cases for LEGACY_AMINO_JSON are well-structured.The test cases properly verify the SIGN_MODE_LEGACY_AMINO_JSON functionality with appropriate assertions.
Also applies to: 200-210
234-234: LGTM! Helper function properly updated for new sign mode type.The function signature and internal logic have been correctly updated to handle the new apisigning.SignMode type.
Also applies to: 252-253
x/auth/ante/sigverify_test.go (2)
11-11: LGTM! Import aligns with PR objectives.The import change from
types/tx/signingtoapi/cosmos/tx/signing/v1beta1aligns with the PR's goal of simplifying signing packages.
181-182: LGTM! Proper test configuration override.The metadata query function override ensures tests use the mock bank keeper, which is the correct testing approach.
types/tx/signing/signature.go
Outdated
| signMode, err := APISignModeToInternal(data.SignMode) | ||
| if err != nil { | ||
| panic(fmt.Errorf("invalid sign mode: %v", err)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use %w verb for error wrapping in fmt.Errorf.
The error from APISignModeToInternal should be properly wrapped to maintain the error chain.
- panic(fmt.Errorf("invalid sign mode: %v", err))
+ panic(fmt.Errorf("invalid sign mode: %w", err))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signMode, err := APISignModeToInternal(data.SignMode) | |
| if err != nil { | |
| panic(fmt.Errorf("invalid sign mode: %v", err)) | |
| } | |
| signMode, err := APISignModeToInternal(data.SignMode) | |
| if err != nil { | |
| panic(fmt.Errorf("invalid sign mode: %w", err)) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
37-37: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
🪛 GitHub Check: golangci-lint
[failure] 37-37:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🪛 GitHub Actions: Lint
[error] 37-37: non-wrapping format verb for fmt.Errorf. Use %w to format errors
types/tx/signing/signature.go
Outdated
| signMode, err := InternalSignModeToAPI(descData.Single.Mode) | ||
| if err != nil { | ||
| panic(fmt.Errorf("invalid sign mode: %v", err)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use %w verb for error wrapping in fmt.Errorf.
The error from InternalSignModeToAPI should be properly wrapped to maintain the error chain.
- panic(fmt.Errorf("invalid sign mode: %v", err))
+ panic(fmt.Errorf("invalid sign mode: %w", err))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signMode, err := InternalSignModeToAPI(descData.Single.Mode) | |
| if err != nil { | |
| panic(fmt.Errorf("invalid sign mode: %v", err)) | |
| } | |
| signMode, err := InternalSignModeToAPI(descData.Single.Mode) | |
| if err != nil { | |
| panic(fmt.Errorf("invalid sign mode: %w", err)) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
76-76: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
🪛 GitHub Check: golangci-lint
[failure] 76-76:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
(cherry picked from commit e441778) # Conflicts: # CHANGELOG.md # tests/integration/v2/app.go # x/auth/ante/feegrant_test.go # x/auth/ante/testutil_test.go
Co-authored-by: Julien Robert <[email protected]>
Description
Before we go to LTS, it would be good to push forward the simplification of the signing packages.
Currently we have:
cosmossdk.io/x/tx/signing & cosmossdk.io/api/cosmos/tx/signing
github.com/cosmos/cosmos-sdk/x/auth/signing & github.com/cosmos/cosmos-sdk/types/tx/signing
There were some plan of completely killing x/auth/tx in favor of x/tx directly, which hopefully will still be the case long term: #18446. However, in the meantime we can clear the API inconsistency wherever it is possible and let the conversion happen internally so users do not need to deal with two sets of sign mode enums for instance.
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
Improvements
authsigning.SignerDatain favor oftxsigning.SignerData.Breaking Changes
txsigningpackage.Migration Guidance
authsigning.SignerDatawithtxsigning.SignerData.apisigningpackage.anypb.Anytype.These changes enhance the SDK's transaction signing mechanism, providing a more consistent and type-safe approach to handling signatures across different components.