refactor(x/tx): DecodedTx implements core/transaction.Tx #20424
Conversation
WalkthroughWalkthroughThe updates primarily focus on refactoring and enhancing the transaction decoding and building processes within the Cosmos SDK. Key changes include modifying the assignment of messages in transaction builders, simplifying the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Builder
participant Decoder
participant DecodedTx
User ->> Builder: Call newBuilderFromDecodedTx
Builder ->> Decoder: Decode transaction
Decoder ->> DecodedTx: Populate decodedTx fields
DecodedTx ->> Builder: Return decodedTx with Messages
Builder ->> User: Return Builder with msgs: decoded.decodedTx.Messages
sequenceDiagram
participant User
participant gogoTxWrapper
participant DecodedTx
User ->> gogoTxWrapper: Call newWrapperFromDecodedTx
gogoTxWrapper ->> DecodedTx: Populate fields directly
DecodedTx ->> gogoTxWrapper: Return fields including fees
gogoTxWrapper ->> User: Return gogoTxWrapper with updated methods
These diagrams illustrate the high-level interactions and flow of the refactored transaction building and decoding processes. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (2)
x/tx/decode/decode.go (2)
23-33: Ensure proper documentation for new fields inDecodedTx.The new fields
ReflectMessages,cachedHash,cachedBytes, andcachedHashedare added to theDecodedTxstruct. It would be beneficial to add comments describing the purpose and usage of these fields to maintain code readability and ease future maintenance.
Line range hint
123-173: Review and optimize theDecodemethod for error handling and efficiency.The
Decodemethod has several new lines of code handling different aspects of transaction decoding. Consider refactoring this method to separate concerns more clearly, possibly by breaking it down into smaller helper functions. This can improve readability and make the code easier to maintain.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/tx/go.modis excluded by!**/*.modx/tx/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- x/tx/decode/decode.go (3 hunks)
Additional Context Used
Path-based Instructions (1)
x/tx/decode/decode.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
x/tx/decode/decode.go (1)
40-40: Good use of type assertion to ensure interface implementation.The type assertion
var _ transaction.Tx = (*DecodedTx)(nil)is a good practice to ensure at compile time thatDecodedTximplements thetransaction.Txinterface.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/tx/go.modis excluded by!**/*.mod
Files selected for processing (5)
- x/auth/tx/builder.go (1 hunks)
- x/auth/tx/gogotx.go (4 hunks)
- x/tx/decode/decode.go (4 hunks)
- x/tx/decode/decode_test.go (4 hunks)
- x/tx/decode/fuzz_test.go (3 hunks)
Additional Context Used
GitHub Check Runs (1)
dependency-review failure (7)
x/auth/tx/builder.go: [failure] 59-59:
cannot use decoded.decodedTx.Messages (variable of type []protoreflect.ProtoMessage) as []"github.com/cosmos/gogoproto/proto".Message value in struct literal
x/auth/tx/gogotx.go: [failure] 107-107:
cannot use w.decodedTx.Messages (variable of type []protoreflect.ProtoMessage) as []"github.com/cosmos/gogoproto/proto".Message value in return statement
x/auth/tx/gogotx.go: [failure] 111-111:
w.decodedTx.ReflectMessages undefined (type *decode.DecodedTx has no field or method ReflectMessages)
Path-based Instructions (5)
x/tx/decode/fuzz_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/tx/decode/decode_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/tx/decode/decode.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/gogotx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
x/tx/decode/fuzz_test.go (2)
Line range hint
1-1: The functiongenerateAndAddSeedsFromTxis well-implemented for fuzz testing purposes.
Line range hint
92-117: The functionFuzzDecodeis correctly set up for fuzzing the decoding process of transactions.x/tx/decode/decode_test.go (2)
Line range hint
26-57: The functionTestDecodeeffectively covers both successful and error scenarios in decoding transactions.
143-143: The functionTestDecodeTxBodyPaniccorrectly tests for error handling in the decoding process.x/tx/decode/decode.go (2)
22-33: TheDecodedTxtype is well-structured and includes necessary methods for transaction handling.
35-62: TheDecodertype is correctly implemented for decoding transactions and handles various error scenarios effectively.x/auth/tx/gogotx.go (1)
Line range hint
30-77: ThegogoTxWrappertype is well-implemented for wrapping decoded transactions and includes necessary methods for transaction handling.x/auth/tx/builder.go (1)
Line range hint
1-1: Thebuildertype is correctly implemented for building transactions and includes comprehensive methods for setting transaction details.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kocubinski
force-pushed
the
kocu/x-tx-core-tx
branch
from
40aa8f2
to
9f5c5fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
x/tx/CHANGELOG.md (2)
Line range hint
157-157: When referring to system architectures, use a hyphen: change "32 bit" to "32-bit".- Fix int64 usage for 32 bit platforms. + Fix int64 usage for 32-bit platforms.
Line range hint
142-153: The indentation for the unordered list items should be consistent. Adjust to match the expected indentation.- * [#15871](https://github.com/cosmos/cosmos-sdk/pull/15871) - * `HandlerMap` now has a `DefaultMode()` getter method - * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files` + * [#15871](https://github.com/cosmos/cosmos-sdk/pull/15871) + * `HandlerMap` now has a `DefaultMode()` getter method + * Textual types use `signing.ProtoFileResolver` instead of `protoregistry.Files`
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/decode/decode.go (4 hunks)
- x/tx/decode/decode_test.go (5 hunks)
- x/tx/decode/fuzz_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/tx/decode/decode_test.go
- x/tx/decode/fuzz_test.go
Additional Context Used
LanguageTool (1)
x/tx/CHANGELOG.md (1)
Near line 157: When ‘32-bit’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...mos-sdk/pull/15849) Fix int64 usage for 32 bit platforms. ## v0.5.1 ### Features * ...
Path-based Instructions (2)
x/tx/decode/decode.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/tx/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (8)
x/tx/decode/decode.go (7)
21-22: The update toDecodedTxstruct to includeReflectMessagesaligns with the new interface requirements.
29-31: Caching mechanism for hash and bytes is a good performance optimization.
35-37: ThegogoProtoCodecinterface might need additional methods likeMarshalto be fully functional.
46-47: Ensure that the new fieldscodecinDecoderandProtoCodecinOptionsare properly handled to avoid nil references.Also applies to: 53-54, 71-72
62-67: Proper error handling for nil checks on new codec-related fields inDecoderconstruction.
187-192: Optimize theHashmethod by checkingcachedHashedat the start to skip unnecessary computations.
226-235: Replace the use ofpanicincomputeHashAndByteswith more graceful error handling to improve robustness.x/tx/CHANGELOG.md (1)
38-39: The changelog entry clearly communicates the breaking changes related todecode.Optionsrequiring a proto codec.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/tx/decode/decode_test.go (5 hunks)
Additional Context Used
Path-based Instructions (1)
x/tx/decode/decode_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"
Additional comments not posted (6)
x/tx/decode/decode_test.go (6)
10-13: The new imports are correctly added to support the updated test implementations.
25-39: The new type definitions fortestGogoCodecandresolverare well-defined and crucial for the updated testing strategy.
41-48: TheUnmarshalmethod intestGogoCodecis correctly implemented to handle both V1 and V2 message types, aligning with the newProtoCodecrequirements.
75-76: The setup ofProtoCodecandAnyResolverusingtestGogoCodecandresolveris correctly implemented to align with the new decoding logic.
87-87: The addition of theSimpleSignertest case is appropriate to ensure coverage of new message types in the decoding process.
162-163: The consistent setup ofProtoCodecandAnyResolverinTestDecodeTxBodyPanicensures uniform testing conditions across test functions.
| @@ -33,6 +33,10 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### API Breaking | |||
|
|
|||
| * [#20424](https://github.com/cosmos/cosmos-sdk/pull/20424) `decode.Options` now requires a proto codec capable of unmarshalling and resolving types. | |||
There was a problem hiding this comment.
Is it used in v0.50? I don't think it is, we if so we should add an example and make sure v0.50 is updated. Otherwise we'll need a compatibility matrix for x/tx and we don't want that.
There was a problem hiding this comment.
x/tx/decode is used in v0.50, or which did you mean?
There was a problem hiding this comment.
Then I missed it (on my phone - and still am). What does it mean for people on v0.50 then? Do we need a patch release that supports that behavior change then?
There was a problem hiding this comment.
sorry, you are correct, it is unused in v0.50.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/epochs/go.modis excluded by!**/*.modx/epochs/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.work.example (1 hunks)
Files skipped from review due to trivial changes (1)
- go.work.example
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/epochs/go.modis excluded by!**/*.modx/epochs/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.work.example (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- go.work.example
Description
Originally a part of #20412, this PR implements
core/transaction.TxforDecodedTx. #20412 implementscore/transaction.Txon the gogo wrapper. Transaction representation can be simplified now that we are committed to gogo types, the gogo wrapper can likely be deleted.This PR needs more work.
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.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
go.work.examplefor better structure.Documentation