chore: move generic txdecoder #22653
Conversation
tac0turtle
requested review from
kocubinski,
julienrbrt,
a team and
JulianToledano
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications to transaction decoding mechanisms within the codebase. A new generic transaction decoder type, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 and nitpick comments (3)
client/tx_config.go (3)
74-74: Use a nil pointer in interface assertion for clarityIn line 74, when asserting that
DefaultTxDecoder[transaction.Tx]implements thetransaction.Codec[transaction.Tx]interface, it's conventional to use a nil pointer for the implementation side. This improves readability and avoids unnecessary allocations.Apply this change:
-var _ transaction.Codec[transaction.Tx] = &DefaultTxDecoder[transaction.Tx]{} +var _ transaction.Codec[transaction.Tx] = (*DefaultTxDecoder[transaction.Tx])(nil)
81-95: Simplify type assertion and error handling inDecodemethodThe type assertion logic in the
Decodemethod can be streamlined for clarity. Additionally, ensure consistent handling of zero values for the generic typeTwhen returning errors.Refactor the method as follows:
func (t *DefaultTxDecoder[T]) Decode(bz []byte) (T, error) { - var out T tx, err := t.TxConfig.TxDecoder()(bz) if err != nil { - return out, err + var zero T + return zero, err } - var ok bool - out, ok = tx.(T) + out, ok := tx.(T) if !ok { - return out, errors.New("unexpected Tx type") + var zero T + return zero, errors.New("unexpected Tx type") } return out, nil }
97-112: Eliminate code duplication betweenDecodeandDecodeJSONmethodsThe
DecodeandDecodeJSONmethods contain duplicated logic. Refactoring to use a shared helper function promotes the DRY (Don't Repeat Yourself) principle.Introduce a helper function:
func (t *DefaultTxDecoder[T]) decode(bz []byte, decoder sdk.TxDecoder) (T, error) { var zero T tx, err := decoder(bz) if err != nil { return zero, err } out, ok := tx.(T) if !ok { return zero, errors.New("unexpected Tx type") } return out, nil } func (t *DefaultTxDecoder[T]) Decode(bz []byte) (T, error) { return t.decode(bz, t.TxConfig.TxDecoder()) } func (t *DefaultTxDecoder[T]) DecodeJSON(bz []byte) (T, error) { return t.decode(bz, t.TxConfig.TxJSONDecoder()) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
client/tx_config.go(2 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
client/tx_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
110-110: LGTM: Successfully migrated to DefaultTxDecoder
The change aligns with the PR objective of moving the generic transaction decoder. Using DefaultTxDecoder[T] simplifies the codebase by leveraging a standard implementation.
Let's verify that the old decoder is completely removed and no references remain:
✅ Verification successful
Successfully verified DefaultTxDecoder migration
The search results confirm that:
- No references to
genericTxDecoderremain in the codebase DefaultTxDecoderis properly implemented inclient/tx_config.goand correctly used insimapp/v2/simdv2/cmd/commands.go- The implementation follows a consistent pattern across the codebase with proper interface compliance (
var _ transaction.Codec[transaction.Tx] = &DefaultTxDecoder[transaction.Tx]{}).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete removal of genericTxDecoder
# and consistent usage of DefaultTxDecoder
# Check for any remaining references to genericTxDecoder
rg "genericTxDecoder"
# Verify DefaultTxDecoder usage pattern
ast-grep --pattern 'DefaultTxDecoder[T]{TxConfig: $_}'
Length of output: 78
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# Search for any references to genericTxDecoder with case insensitivity
rg -i "generictxdecoder"
# Search for DefaultTxDecoder usage with surrounding context
rg -A 2 "DefaultTxDecoder"
# Look for any decoder-related type definitions or interfaces
ast-grep --pattern 'type $_ struct {
$$$
TxConfig $_
$$$
}'
# Check for any decoder-related imports
rg "\"github.com/.*client.*decoder\""
Length of output: 11103
| type DefaultTxDecoder[T transaction.Tx] struct { | ||
| TxConfig TxConfig | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for exported type DefaultTxDecoder
The exported type DefaultTxDecoder lacks a comment. According to the Uber Go Style Guide, all exported types should have a comment explaining their purpose.
Add a comment like:
+// DefaultTxDecoder is a generic transaction decoder that implements the transaction.Codec interface.
type DefaultTxDecoder[T transaction.Tx] struct {
TxConfig TxConfig
}📝 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.
| type DefaultTxDecoder[T transaction.Tx] struct { | |
| TxConfig TxConfig | |
| } | |
| // DefaultTxDecoder is a generic transaction decoder that implements the transaction.Codec interface. | |
| type DefaultTxDecoder[T transaction.Tx] struct { | |
| TxConfig TxConfig | |
| } |
There was a problem hiding this comment.
Yes, let's add this please @tac0turtle
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!
client/tx_config.go
Outdated
| // DecodeJSON implements transaction.Codec. | ||
| func (t *DefaultTxDecoder[T]) DecodeJSON(bz []byte) (T, error) { | ||
| var out T | ||
| tx, err := t.TxConfig.TxJSONDecoder()(bz) | ||
| if err != nil { | ||
| return out, err | ||
| } | ||
|
|
||
| var ok bool | ||
| out, ok = tx.(T) | ||
| if !ok { | ||
| return out, errors.New("unexpected Tx type") | ||
| } | ||
|
|
||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comments for exported methods Decode and DecodeJSON
The exported methods Decode and DecodeJSON lack documentation. Per the Uber Go Style Guide, all exported functions and methods should have comments.
Add comments like:
+// Decode decodes a binary transaction into type T using the TxConfig's TxDecoder.
func (t *DefaultTxDecoder[T]) Decode(bz []byte) (T, error) {
// existing code
}
+// DecodeJSON decodes a JSON transaction into type T using the TxConfig's TxJSONDecoder.
func (t *DefaultTxDecoder[T]) DecodeJSON(bz []byte) (T, error) {
// existing code
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@julienrbrt, understood. In addition to Decode and DecodeJSON, I noticed that the exported interface methods in TxEncodingConfig and TxConfig also lack documentation comments. Per the Uber Go Style Guide, all exported functions and methods should have comments.
I recommend adding comments like:
// TxEncoder returns the transaction encoder.
TxEncoder() sdk.TxEncoder
// TxDecoder returns the transaction decoder.
TxDecoder() sdk.TxDecoder
// TxJSONEncoder returns the JSON transaction encoder.
TxJSONEncoder() sdk.TxEncoder
// TxJSONDecoder returns the JSON transaction decoder.
TxJSONDecoder() sdk.TxDecoder
// MarshalSignatureJSON marshals the signatures to JSON.
MarshalSignatureJSON([]signingtypes.SignatureV2) ([]byte, error)
// UnmarshalSignatureJSON unmarshals the signatures from JSON.
UnmarshalSignatureJSON([]byte) ([]signingtypes.SignatureV2, error)This will help maintain consistency and improve code readability.
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/tx_config.go (1)
81-96: Enhance error messages for better debuggingThe error messages in type assertions could be more descriptive by including the actual type information.
Consider updating the error messages:
- return out, errors.New("unexpected Tx type") + return out, fmt.Errorf("unexpected Tx type: got %T, want %T", tx, out)This change would require adding
"fmt"to the imports.Also applies to: 98-113
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
client/tx_config.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
client/tx_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
client/tx_config.go (3)
4-4: LGTM: Import statements are properly used
The new imports are correctly added and utilized in the implementation.
Also applies to: 7-7
74-74: LGTM: Proper interface compliance check
The compile-time interface assertion ensures that DefaultTxDecoder implements the transaction.Codec interface.
76-79: LGTM: Well-documented type definition
The type definition is properly documented and follows Go conventions.
(cherry picked from commit f153426)
Description
Closes: #XXXX
move default tx decoder to
client/tx_config.goAuthor 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