fix: json codec to use custom json marshal unmarshal #118
Conversation
beer-1
changed the title
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BridgeConfig
participant JSONPB
Client->>BridgeConfig: Request to marshal BatchInfo_ChainType
BridgeConfig->>JSONPB: Call MarshalJSONPB
JSONPB-->>BridgeConfig: Return marshaled JSONPB
BridgeConfig-->>Client: Return marshaled data
Client->>BridgeConfig: Request to unmarshal JSONPB
BridgeConfig->>JSONPB: Call UnmarshalJSONPB
JSONPB-->>BridgeConfig: Return unmarshaled BatchInfo_ChainType
BridgeConfig-->>Client: Return BatchInfo_ChainType
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 48.61% 49.25% +0.63%
==========================================
Files 57 57
Lines 4229 4233 +4
==========================================
+ Hits 2056 2085 +29
+ Misses 1744 1719 -25
Partials 429 429
|
![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
🧹 Outside diff range and nitpick comments (3)
x/ophost/types/bridge_config.go (1)
108-116: LGTM! Consider enhancing the documentation.The implementation correctly delegates to the existing JSON methods, ensuring consistency between JSON and JSONPB serialization. This is a clean and efficient approach.
Consider adding a brief comment explaining why the Marshaler/Unmarshaler parameters are ignored:
// MarshalJSONPB marshals the BatchInfo_ChainType to JSON -func (cy BatchInfo_ChainType) MarshalJSONPB(_ *jsonpb.Marshaler) ([]byte, error) { +// The jsonpb.Marshaler parameter is ignored as we delegate to the standard JSON marshaling +func (cy BatchInfo_ChainType) MarshalJSONPB(_ *jsonpb.Marshaler) ([]byte, error) { // UnmarshalJSONPB unmarshals the BatchInfo_ChainType from JSON -func (cy *BatchInfo_ChainType) UnmarshalJSONPB(_ *jsonpb.Unmarshaler, inputBz []byte) error { +// The jsonpb.Unmarshaler parameter is ignored as we delegate to the standard JSON unmarshaling +func (cy *BatchInfo_ChainType) UnmarshalJSONPB(_ *jsonpb.Unmarshaler, inputBz []byte) error {x/ophost/types/bridge_config_test.go (2)
73-74: Consider extracting the test JSON into a separate test data file.The hardcoded JSON string makes the test difficult to maintain and read. Consider:
- Moving it to a separate test data file
- Using a more maintainable way to construct the test JSON, such as marshaling a struct
59-85: Enhance test documentation and structure.The test would benefit from:
- Adding comments explaining the test's purpose and what it's validating
- Converting to a table-driven test to cover multiple chain types and error cases
- Breaking down the large JSON string into smaller, more manageable test cases
Example structure:
func TestGoGoProtoJsonPB(t *testing.T) { // Test cases for different chain types and scenarios testCases := []struct { name string chainType BatchInfo_ChainType wantErr bool }{ { name: "valid celestia chain type", chainType: BatchInfo_CHAIN_TYPE_CELESTIA, wantErr: false, }, // Add more test cases } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Test implementation }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/ophost/types/bridge_config.go(2 hunks)x/ophost/types/bridge_config_test.go(2 hunks)
🔇 Additional comments (5)
x/ophost/types/bridge_config.go (2)
11-11: LGTM! Good use of interface assertions.
The interface assertions ensure compile-time verification that BatchInfo_ChainType correctly implements both jsonpb.JSONPBMarshaler and jsonpb.JSONPBUnmarshaler interfaces.
Also applies to: 14-15
14-15: Verify JSONPB implementation completeness.
Let's verify if there are other enum types that might need similar JSONPB support.
Also applies to: 108-116
✅ Verification successful
Let me gather more information about the enum definitions in the proto files to ensure we have a complete picture.
Based on the comprehensive analysis of the proto files and codebase usage, I can now provide the final response:
JSONPB implementation is complete and correctly implemented
The verification shows that:
BatchInfo_ChainTypeis the only enum type that requires JSONPB support as it's the only enum used in JSON serialization contexts (viaBatchSubmissionTargetfield in configs)- The implementation is complete with both
MarshalJSONPBandUnmarshalJSONPBmethods properly delegating to the existing JSON methods - The enum is properly used across the codebase in a consistent manner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other enum types defined in .proto files that might need JSONPB support
echo "Searching for enum definitions in .proto files..."
fd -e proto -x grep -l "enum" {} \;
# Search for existing MarshalJSON/UnmarshalJSON implementations that might need JSONPB counterparts
echo "Searching for existing JSON marshal/unmarshal implementations..."
rg "func.*Marshal(JSON|JSON)" -A 5
rg "func.*Unmarshal(JSON|JSON)" -A 5
Length of output: 3539
Script:
#!/bin/bash
# Check the content of proto files for enum definitions
echo "Content of ./proto/opinit/opchild/v1/types.proto:"
cat ./proto/opinit/opchild/v1/types.proto
echo -e "\nContent of ./proto/opinit/ophost/v1/types.proto:"
cat ./proto/opinit/ophost/v1/types.proto
# Search for any usage of these enums in the codebase
echo -e "\nSearching for enum usages:"
rg "BatchInfo_ChainType" -A 2
Length of output: 19560
x/ophost/types/bridge_config_test.go (3)
5-5: LGTM! Import changes are well-structured.
The new imports appropriately support the added test functionality for JSON/JSONPB marshaling and codec operations.
Also applies to: 10-17
58-58: LGTM! Validation test cases are comprehensive.
The additional validation cases for UNSPECIFIED and unknown chain types strengthen the test coverage and align with the PR's objective of proper enum handling.
74-74: Verify the contents of the encoded metadata.
The test contains base64 encoded metadata in the JSON string. Please verify its contents and consider adding explicit validation of the decoded metadata.
✅ Verification successful
Metadata validation is properly implemented
The codebase has appropriate validation for the metadata:
- There is a defined maximum length limit (
MaxMetadataLength = 1024 * 5) - The
MsgCreateBridge.Validate()method enforces this limit with a clear error message - The same validation is also applied in
MsgUpdateMetadata.Validate() - The test data contains valid JSON metadata for IBC channels configuration
The base64 encoded metadata in the test is used as a valid test fixture and doesn't require additional validation since the length check provides sufficient protection against malicious input.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Decode and verify the metadata content
echo "eyJwZXJtX2NoYW5uZWxzIjpbeyJwb3J0X2lkIjoibmZ0LXRyYW5zZmVyIiwiY2hhbm5lbF9pZCI6ImNoYW5uZWwtMzUzIn0seyJwb3J0X2lkIjoidHJhbnNmZXIiLCJjaGFubmVsX2lkIjoiY2hhbm5lbC0zNTIifV19" | base64 -d
Length of output: 257
Script:
#!/bin/bash
# Search for metadata validation in the test file and related files
rg -A 5 "metadata.*base64" x/ophost/types/
# Look for any metadata validation patterns in the codebase
ast-grep --pattern 'func $_ ($_, $_) {
$$$
base64.Decode$$$($$$)
$$$
}'
# Check if there are any validation functions for bridge config
ast-grep --pattern 'func (m *MsgCreateBridge) Validate() error {
$$$
}'
Length of output: 200
Script:
#!/bin/bash
# Look for metadata validation in bridge config files
rg -l "metadata" x/ophost/types/
# Check the test file content for context
cat x/ophost/types/bridge_config_test.go
# Look for any validation related to bridge config
rg "Validate.*Bridge" x/ophost/types/
Length of output: 4388
Script:
#!/bin/bash
# Check the validation implementation in tx.go
cat x/ophost/types/tx.go
# Look for any metadata-specific validation
rg -A 5 "Config.*Validate" x/ophost/types/
Length of output: 12006
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic with proper test error handling.
Using panic in tests is not recommended. Instead, use require.NoError for consistency with the rest of the test file.
- if err != nil {
- panic(err)
- }
+ require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
Description
Closes: #XXXX
cosmos jsonpb does not check existing
UnmarshalJSONif the type is enum, so we use custom marshal/unmarshal functions which is always execute if exists.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 changeReviewers 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
BatchInfo_ChainTypewith JSONPB support.BatchInfochain types.