refactor: simplify sign mode (2/2) #23476
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 WalkthroughWalkthroughThis pull request focuses on simplifying the handling of sign modes across the Cosmos SDK. The primary changes involve removing 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)
⏰ Context from checks skipped due to timeout of 90000ms (11)
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: 1
🧹 Nitpick comments (2)
CHANGELOG.md (2)
Line range hint
14-20: Consider adding more context to the guiding principles section.While the guiding principles section provides good high-level guidance, it could benefit from brief explanations of why each principle is important. For example, explain why changelogs are "for humans" and what makes a good changelog entry.
Line range hint
22-29: Usage section could be clearer about required format.The usage section should provide a more explicit example of the expected format for changelog entries, including:
- Exact formatting of the issue reference
- Required components of a changelog entry
- Examples of good vs bad entries
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)client/tx/aux_builder.go(1 hunks)client/tx/aux_builder_test.go(1 hunks)types/tx/direct_aux.go(2 hunks)types/tx/direct_aux_test.go(2 hunks)types/tx/signing/signature.go(3 hunks)types/tx/signing/types.go(0 hunks)x/auth/tx/sigs.go(1 hunks)
💤 Files with no reviewable changes (1)
- types/tx/signing/types.go
🧰 Additional context used
📓 Path-based instructions (7)
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"
types/tx/direct_aux.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/sigs.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"
client/tx/aux_builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/tx/signing/signature.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 GitHub Actions: Lint
types/tx/signing/signature.go
[error] 6-8: File is not properly formatted according to gci standards. Import order should follow: --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-simapp-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (9)
types/tx/direct_aux.go (2)
6-6: LGTM! Import addition for direct sign mode usage.The addition of the apisigning import aligns with the PR's objective of simplifying sign mode handling.
58-58: LGTM! Direct sign mode assignment.The direct usage of
apisigning.SignMode(a.Mode)simplifies the code by removing unnecessary conversion steps.types/tx/direct_aux_test.go (2)
8-8: LGTM! Import addition for test assertions.The addition of the apisigning import enables direct sign mode comparisons in tests.
81-81: LGTM! Updated test assertion.The test assertion now correctly uses direct sign mode comparison, maintaining test coverage while aligning with the simplified approach.
types/tx/signing/signature.go (1)
39-39: LGTM! Consistent sign mode handling.The direct usage of SignMode in both conversion functions maintains consistency and simplifies the codebase.
Also applies to: 71-71
x/auth/tx/sigs.go (1)
26-26: LGTM! Simplified sign mode conversion.The direct casting to
signing.SignModealigns with the PR's objective of removing unnecessary conversion helpers.client/tx/aux_builder_test.go (1)
251-251: LGTM! Sign mode comparison simplified.The direct type conversion
apisigning.SignMode(auxSignerData.Mode)aligns with the PR's goal of removing unnecessary conversion helpers while maintaining the same validation logic.client/tx/aux_builder.go (1)
139-139: LGTM! Sign mode assignment simplified.The direct type conversion
signing.SignMode(mode)aligns with the PR's goal of removing unnecessary conversion helpers. The change maintains correctness as the validation of allowed sign modes is still performed in the switch statement above.CHANGELOG.md (1)
Line range hint
1-1: LGTM! The CHANGELOG follows the standard Keep a Changelog format.The file properly documents changes across versions in a clear and organized way, with changes appropriately categorized into sections like Features, Improvements, Bug Fixes, etc.
types/tx/signing/signature.go
Outdated
| signingv1beta1 "cosmossdk.io/api/cosmos/tx/signing/v1beta1" | ||
| gogoprotoany "github.com/cosmos/gogoproto/types/any" | ||
|
|
There was a problem hiding this comment.
Fix import ordering according to gci standards.
The imports should follow this order:
- Standard library
- Default packages
- Packages with prefix "cosmossdk.io"
- Packages with prefix "github.com/cosmos/cosmos-sdk"
Apply this diff to fix the import ordering:
import (
"fmt"
- signingv1beta1 "cosmossdk.io/api/cosmos/tx/signing/v1beta1"
- gogoprotoany "github.com/cosmos/gogoproto/types/any"
+ signingv1beta1 "cosmossdk.io/api/cosmos/tx/signing/v1beta1"
+ gogoprotoany "github.com/cosmos/gogoproto/types/any"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 6-8: File is not properly formatted according to gci standards. Import order should follow: --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(cherry picked from commit 39b0244) # Conflicts: # CHANGELOG.md
Description
we actually do not need to have those converting helpers at all.
Especially internally when someone can extend the sign mode
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
Removed Functionality
APISignModeToInternalandInternalSignModeToAPIconversion methodsTechnical Updates
These changes simplify the SDK's signing process, making it more straightforward for developers to work with transaction signatures.