feat(core): add a new codec to core #22326
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new codec package that defines interfaces for encoding and decoding data in binary and JSON formats. The Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Outside diff range and nitpick comments (3)
core/codec/codec.go (3)
5-9: LGTM: Codec interface is well-designed.The
Codecinterface follows good Go practices by composingBinaryCodecandJSONCodec. The naming convention is correct for an exported interface.Consider slightly modifying the comment to be more idiomatic:
-// Codec defines a Binary Codec and JSON Codec for modules to encode and decode data +// Codec combines Binary and JSON encoding/decoding capabilities for modules
11-16: LGTM: BinaryCodec interface is well-defined.The
BinaryCodecinterface is focused and its methods follow Go conventions. The comment provides useful information about the encoding method.Consider using a more generic type for the message parameter:
type BinaryCodec interface { - Marshal(transaction.Msg) ([]byte, error) - Unmarshal([]byte, transaction.Msg) error + Marshal(proto.Message) ([]byte, error) + Unmarshal([]byte, proto.Message) error }This change would make the interface more flexible and reusable for different types of protobuf messages, not just transaction messages.
18-22: LGTM: JSONCodec interface is well-defined, with minor suggestions.The
JSONCodecinterface is focused and its methods follow Go conventions for JSON encoding/decoding.Consider the following improvements:
- Enhance the comment to provide more context:
-// JSONCodec defines a JSON encoding and decoding interface for modules to encode and decode data +// JSONCodec defines an interface for JSON encoding and decoding of transaction messages
- Use a more generic type for the message parameter, similar to the suggestion for
BinaryCodec:type JSONCodec interface { - MarshalJSON(transaction.Msg) ([]byte, error) - UnmarshalJSON([]byte, transaction.Msg) error + MarshalJSON(proto.Message) ([]byte, error) + UnmarshalJSON([]byte, proto.Message) error }These changes would improve clarity and make the interface more flexible for different types of messages.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- core/codec/codec.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
core/codec/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
core/codec/codec.go (1)
1-3: LGTM: Package declaration and import statement are correct.The package name 'codec' is appropriate for the content, and the import statement is correctly formatted and necessary for the
transaction.Msgtype used in the interfaces.
tac0turtle
requested review from
testinginprod and
facundomedica
as code owners
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
collections/protocodec/collections_test.go (3)
19-39: LGTM: Comprehensive test coverage for CollValueV2 codec.The test thoroughly covers both binary and JSON encoding/decoding operations for
CollValueV2. It appropriately usesprotocmp.Transform()to handle protobuf version differences in comparisons. The test checks for successful operations, value equality after encoding and decoding, and ensures the encoder's value type is not empty.Consider adding test cases for error scenarios, such as decoding invalid input, to ensure robust error handling.
41-56: LGTM: Thorough testing of BoolValue codec with custom assertions.The test effectively covers both true and false values for the
BoolValuecodec. The use ofcolltest.TestValueCodecdemonstrates good reuse of testing utilities. The customvalueAssertfunction provides an additional layer of verification by comparing the encoded bytes withgogotypes.BoolValue.Consider adding a brief comment explaining the purpose of the
valueAssertfunction to improve code readability and maintainability.
1-57: Overall: Well-structured and comprehensive test suite for codec functionality.This test file demonstrates good testing practices for the codec implementations. It covers both
CollValueV2andBoolValuecodecs, including binary and JSON operations. The tests are well-organized, use appropriate testing utilities, and show attention to detail in handling protobuf version differences.To further enhance the test suite:
- Consider adding more error case scenarios to ensure robust error handling.
- Add brief comments for custom functions like
valueAssertto improve code readability.- If applicable, consider testing with edge cases such as empty or very large values to ensure codec stability across a wide range of inputs.
collections/protocodec/collections.go (1)
90-92: Provide More DescriptiveValueTypeIdentifiersIn the
ValueTypemethods of bothcollValueandcollValue2, consider returning more precise type information by including the full package path and type name. This improves clarity when debugging or logging type information.Also applies to: 135-137
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
collections/protocodec/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- codec/collections.go (1 hunks)
- collections/protocodec/collections.go (1 hunks)
- collections/protocodec/collections_test.go (1 hunks)
- collections/protocodec/go.mod (1 hunks)
- go.mod (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- collections/protocodec/go.mod
🧰 Additional context used
📓 Path-based instructions (3)
codec/collections.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/protocodec/collections.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/protocodec/collections_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 Check: split-test-files
codec/collections.go
[failure] 12-12:
module cosmossdk.io/collections/protocodec provides package cosmossdk.io/collections/protocodec and is replaced but not required; to add it:
🪛 GitHub Check: dependency-review
codec/collections.go
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
module cosmossdk.io/collections/protocodec provides package cosmossdk.io/collections/protocodec and is replaced but not required; to add it:
[failure] 12-12:
could not import cosmossdk.io/collections/protocodec (invalid package name: "")
🔇 Additional comments (5)
collections/protocodec/collections_test.go (1)
3-15: LGTM: Import statements are well-organized and follow best practices.The import statements are grouped correctly, separating standard library imports from third-party and internal project imports. This organization aligns with the Uber Go Style Guide recommendations.
go.mod (1)
188-188: New module added: collections/protocodecThe addition of
cosmossdk.io/collections/protocodec => ./collections/protocodecintroduces a new local module to the project. This aligns with the PR objective of adding a new codec to the core.Please ensure:
- The
./collections/protocodecdirectory exists and contains the necessary Go files.- The module is correctly implemented and integrated with the rest of the project.
Consider updating the project documentation to reflect this new module and its purpose within the SDK.
To verify the module's existence and basic structure, you can run:
✅ Verification successful
Module
collections/protocodecVerified SuccessfullyThe
cosmossdk.io/collections/protocodecdirectory exists, contains the necessary Go files, and includes a correctly configuredgo.modfile.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the directory exists and contains Go files if [ -d "./collections/protocodec" ] && [ "$(find ./collections/protocodec -name '*.go' | wc -l)" -gt 0 ]; then echo "collections/protocodec directory exists and contains Go files." ls -R ./collections/protocodec else echo "Error: collections/protocodec directory is missing or doesn't contain Go files." fi # Check if there's a go.mod file for the new module if [ -f "./collections/protocodec/go.mod" ]; then echo "go.mod file exists for collections/protocodec:" cat ./collections/protocodec/go.mod else echo "Warning: No go.mod file found for collections/protocodec." fiLength of output: 2815
codec/collections.go (1)
15-17: Simplify code by aliasing 'BoolValue' to 'protocodec.BoolValue'Replacing the custom
BoolValueimplementation with an alias toprotocodec.BoolValueenhances code maintainability by reusing existing functionality and reducing redundancy.collections/protocodec/collections.go (2)
16-19: Implementation ofBoolValueCodec is CorrectThe
BoolValuevariable properly provides acollcodec.ValueCodec[bool]implementation, ensuring backward compatibility withprototypes.BoolValue.
23-31: Methods ofboolValueStruct are Correctly ImplementedThe
EncodeandDecodemethods forboolValuecorrectly handle the marshaling and unmarshaling of boolean values usinggogotypes.BoolValue.
codec/collections.go
Outdated
| collcodec "cosmossdk.io/collections/codec" | ||
| "cosmossdk.io/collections/protocodec" |
There was a problem hiding this comment.
Fix the import path for 'protocodec' package
The import statement for "cosmossdk.io/collections/protocodec" may cause build failures due to missing module requirements. According to the static analysis hints, the module is not properly included in your go.mod file.
To resolve this issue, ensure that the cosmossdk.io/collections module is required in your go.mod file. You can add it by running:
go get cosmossdk.io/collections@latestThis command will fetch the latest version of the module and update your go.mod file accordingly.
🧰 Tools
🪛 GitHub Check: split-test-files
[failure] 12-12:
module cosmossdk.io/collections/protocodec provides package cosmossdk.io/collections/protocodec and is replaced but not required; to add it:
🪛 GitHub Check: dependency-review
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/collections/protocodec; to add it:
[failure] 12-12:
module cosmossdk.io/collections/protocodec provides package cosmossdk.io/collections/protocodec and is replaced but not required; to add it:
[failure] 12-12:
could not import cosmossdk.io/collections/protocodec (invalid package name: "")
| type collValue[T any, PT protoMessage[T]] struct { | ||
| cdc corecodec.Codec | ||
| messageName string | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) Encode(value T) ([]byte, error) { | ||
| return c.cdc.Marshal(PT(&value)) | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) Decode(b []byte) (value T, err error) { | ||
| err = c.cdc.Unmarshal(b, PT(&value)) | ||
| return value, err | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) EncodeJSON(value T) ([]byte, error) { | ||
| return c.cdc.MarshalJSON(PT(&value)) | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) DecodeJSON(b []byte) (value T, err error) { | ||
| err = c.cdc.UnmarshalJSON(b, PT(&value)) | ||
| return | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) Stringify(value T) string { | ||
| return PT(&value).String() | ||
| } | ||
|
|
||
| func (c collValue[T, PT]) ValueType() string { | ||
| return "github.com/cosmos/gogoproto/" + c.messageName | ||
| } | ||
|
|
||
| type protoMessageV2[T any] interface { | ||
| *T | ||
| protov2.Message | ||
| } | ||
|
|
||
| // CollValueV2 is used for protobuf values of the newest google.golang.org/protobuf API. | ||
| func CollValueV2[T any, PT protoMessageV2[T]]() collcodec.ValueCodec[PT] { | ||
| return &collValue2[T, PT]{ | ||
| messageName: string(PT(new(T)).ProtoReflect().Descriptor().FullName()), | ||
| } | ||
| } | ||
|
|
||
| type collValue2[T any, PT protoMessageV2[T]] struct { | ||
| messageName string | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) Encode(value PT) ([]byte, error) { | ||
| protov2MarshalOpts := protov2.MarshalOptions{Deterministic: true} | ||
| return protov2MarshalOpts.Marshal(value) | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) Decode(b []byte) (PT, error) { | ||
| var value T | ||
| err := protov2.Unmarshal(b, PT(&value)) | ||
| return &value, err | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) EncodeJSON(value PT) ([]byte, error) { | ||
| return protojson.Marshal(value) | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) DecodeJSON(b []byte) (PT, error) { | ||
| var value T | ||
| err := protojson.Unmarshal(b, PT(&value)) | ||
| return &value, err | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) Stringify(value PT) string { | ||
| return fmt.Sprintf("%v", value) | ||
| } | ||
|
|
||
| func (c collValue2[T, PT]) ValueType() string { | ||
| return "google.golang.org/protobuf/" + c.messageName | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to Eliminate Code Duplication Between collValue and collValue2
The structs collValue and collValue2, along with their methods, share similar logic. Consider refactoring to reduce code duplication by abstracting the common functionality into a single generic struct or interface. This adheres to the DRY (Don't Repeat Yourself) principle and improves maintainability.
| func CollValue[T any, PT protoMessage[T]](cdc interface { | ||
| Marshal(proto.Message) ([]byte, error) | ||
| Unmarshal([]byte, proto.Message) error | ||
| }, | ||
| ) collcodec.ValueCodec[T] { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a Specific Interface Instead of Using an Anonymous Interface for cdc
Using an anonymous interface for the cdc parameter reduces clarity and type safety. Consider defining a specific interface or using an existing one that includes the required methods (Marshal and Unmarshal). This enhances readability and ensures better compile-time checks.
| func CollValue[T any, PT protoMessage[T]](cdc interface { | ||
| Marshal(proto.Message) ([]byte, error) | ||
| Unmarshal([]byte, proto.Message) error | ||
| }, | ||
| ) collcodec.ValueCodec[T] { | ||
| return &collValue[T, PT]{cdc.(corecodec.Codec), proto.MessageName(PT(new(T)))} | ||
| } |
There was a problem hiding this comment.
Handle Type Assertion Safely to Prevent Potential Panic
In the CollValue function, the type assertion cdc.(corecodec.Codec) may cause a panic if cdc does not implement corecodec.Codec. It's safer to perform a checked type assertion to prevent runtime errors.
Apply this diff to safely handle the type assertion:
func CollValue[T any, PT protoMessage[T]](cdc interface {
Marshal(proto.Message) ([]byte, error)
Unmarshal([]byte, proto.Message) error
},
) collcodec.ValueCodec[T] {
- return &collValue[T, PT]{cdc.(corecodec.Codec), proto.MessageName(PT(new(T)))}
+ coreCdc, ok := cdc.(corecodec.Codec)
+ if !ok {
+ // Handle the error appropriately, perhaps return an error
+ return nil // or return an error
+ }
+ return &collValue[T, PT]{coreCdc, proto.MessageName(PT(new(T)))}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CollValue[T any, PT protoMessage[T]](cdc interface { | |
| Marshal(proto.Message) ([]byte, error) | |
| Unmarshal([]byte, proto.Message) error | |
| }, | |
| ) collcodec.ValueCodec[T] { | |
| return &collValue[T, PT]{cdc.(corecodec.Codec), proto.MessageName(PT(new(T)))} | |
| } | |
| func CollValue[T any, PT protoMessage[T]](cdc interface { | |
| Marshal(proto.Message) ([]byte, error) | |
| Unmarshal([]byte, proto.Message) error | |
| }, | |
| ) collcodec.ValueCodec[T] { | |
| coreCdc, ok := cdc.(corecodec.Codec) | |
| if !ok { | |
| // Handle the error appropriately, perhaps return an error | |
| return nil // or return an error | |
| } | |
| return &collValue[T, PT]{coreCdc, proto.MessageName(PT(new(T)))} | |
| } |
julienrbrt
changed the title
|
@tac0turtle your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
go.work.example (1)
8-8: LGTM! Consider alphabetical ordering for consistency.The addition of the
./collections/protocodecmodule path is correct and aligns with the PR objectives. This change appropriately includes the new codec package in the workspace.For improved readability and consistency, consider ordering the
collectionsrelated module paths alphabetically:use ( . ./api ./client/v2 ./collections - ./collections/protocodec + ./collections/protocodec ./core ./core/testing ./depinject.github/pr_labeler.yml (1)
21-22: LGTM! Consider adding a comment for clarity.The addition of the "C:collections/protocodec" label category is appropriate and aligns with the PR objectives. This will help in automatically categorizing future PRs related to the new codec implementation.
Consider adding a brief comment above this new entry to explain its purpose, for example:
# Label for the new codec implementation in collections "C:collections/protocodec": - collections/protocodec/*This would improve maintainability and provide context for other developers.
.github/dependabot.yml (1)
181-189: LGTM with a minor suggestion.The new Dependabot entry for
/collections/protocodecis correctly structured and consistent with existing entries. It aligns well with the PR objectives of introducing a new codec to the core.Consider adding a comment above this entry to explain its purpose, similar to other sections in the file. For example:
# Dependency updates for the new codec in collections - package-ecosystem: gomod directory: "/collections/protocodec" ...This would improve readability and maintainability of the configuration file.
.github/workflows/test.yml (1)
638-668: Consider adding race detection to the test command.The new
test-collections-protocodecjob looks good and follows the structure of other testing jobs in the workflow. However, to maintain consistency with other test jobs and potentially catch race conditions, consider adding the-raceflag to the test command.Update the test command in line 659 as follows:
- go test -mod=readonly -timeout 30m -coverprofile=coverage.out -covermode=atomic -tags='norace ledger test_ledger_mock' ./... + go test -mod=readonly -race -timeout 30m -coverprofile=coverage.out -covermode=atomic -tags='norace ledger test_ledger_mock' ./...This change will align the test command with other similar jobs in the workflow and enable race detection during testing.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- .github/dependabot.yml (1 hunks)
- .github/pr_labeler.yml (1 hunks)
- .github/workflows/test.yml (1 hunks)
- go.work.example (1 hunks)
🧰 Additional context used
Description
this pr adds simple codec to core to avoid importing the sdk. This is the second to final step to have sdk modules that do not depend on the sdk
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores