fix(client): don't marshal zero-ed fields #22723
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files to enhance transaction handling and encoding processes. Key changes include the addition of a custom JSON marshaling method, updates to transaction encoding options to prevent unpopulated fields, and the removal of an interface to simplify the codec structure. These adjustments aim to improve compatibility and error handling within the transaction system while maintaining existing functionalities. 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
|
|
@julienrbrt your pull request is missing a changelog! |
![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 (2)
client/tx/tx.go (1)
137-137: Typo in comment: Change 'accross' to 'across'There's a typo in the comment on line 137; 'accross' should be 'across'.
Apply this diff to correct the typo:
-// It helps the client to be compatible accross SDK versions. +// It helps the client to be compatible across SDK versions.🧰 Tools
🪛 golangci-lint (1.62.2)
137-137:
accrossis a misspelling ofacross(misspell)
client/v2/tx/encoder.go (1)
26-26: Typo in comment: Change 'accross' to 'across'There's a typo in the comment on line 26; 'accross' should be 'across'.
Apply this diff to correct the typo:
-// It helps the client to be compatible accross SDK versions. +// It helps the client to be compatible across SDK versions.🧰 Tools
🪛 golangci-lint (1.62.2)
26-26:
accrossis a misspelling ofacross(misspell)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
client/tx/factory.go(1 hunks)client/tx/tx.go(3 hunks)client/v2/tx/encoder.go(1 hunks)codec/codec.go(1 hunks)codec/json.go(1 hunks)codec/proto_codec.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/tx/factory.go
🧰 Additional context used
📓 Path-based instructions (5)
client/v2/tx/encoder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
codec/codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
codec/proto_codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
codec/json.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/tx/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
client/v2/tx/encoder.go
26-26: accross is a misspelling of across
(misspell)
client/tx/tx.go
137-137: accross is a misspelling of across
(misspell)
🔇 Additional comments (6)
client/tx/tx.go (3)
12-12: LGTM: Necessary imports added
The added imports are appropriate and necessary for the new functionalities introduced in the code.
Also applies to: 17-18, 25-25
136-149: LGTM: Custom encoder implementation to skip unpopulated fields
The introduction of the custom encoder using clientProtoCodec to avoid emitting unpopulated fields enhances compatibility across SDK versions. The fallback mechanism to the default encoder ensures robustness in case of any issues with the custom encoder.
🧰 Tools
🪛 golangci-lint (1.62.2)
137-137: accross is a misspelling of across
(misspell)
456-468: LGTM: Implementation of clientProtoCodec and MarshalJSON method
The clientProtoCodec struct and its MarshalJSON method are correctly implemented to marshal messages while skipping empty fields. This aligns with the goal of improving compatibility and reducing unnecessary data in transaction encoding.
client/v2/tx/encoder.go (1)
25-29: LGTM: Setting EmitUnpopulated to false
Setting EmitUnpopulated: false in jsonMarshalOptions appropriately configures the JSON marshaling to omit unpopulated fields. This change enhances compatibility across SDK versions by preventing the inclusion of zero-value fields.
🧰 Tools
🪛 golangci-lint (1.62.2)
26-26: accross is a misspelling of across
(misspell)
codec/codec.go (1)
44-44: Verify the impact of commenting out mustEmbedCodec()
By commenting out mustEmbedCodec() in the Codec interface, the requirement for implementations to embed an official codec is removed. Please verify that this modification does not affect backward compatibility or lead to unintended side effects in codec implementations.
codec/proto_codec.go (1)
24-24: Confirm the removal of ProtoCodecMarshaler interface
The removal of the ProtoCodecMarshaler interface simplifies the codec structure. Ensure that this change does not impact any existing components that may rely on this interface and that all necessary functionalities are preserved.
| jm := defaultJM | ||
| jm.EmitDefaults = false |
There was a problem hiding this comment.
Avoid modifying the shared defaultJM variable
Modifying the shared defaultJM variable by changing its EmitDefaults field can introduce unintended side effects in other parts of the code that rely on defaultJM. Instead, create a new instance of jsonpb.Marshaler with the desired settings.
Apply this diff to fix the issue:
func ProtoMarshalJSONSkipEmpty(msg proto.Message, resolver jsonpb.AnyResolver) ([]byte, error) {
- jm := defaultJM
- jm.EmitDefaults = false
+ jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: nil}
if resolver != nil {
jm = &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: resolver}
}📝 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.
| jm := defaultJM | |
| jm.EmitDefaults = false | |
| jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: nil} | |
| if resolver != nil { | |
| jm = &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: resolver} | |
| } |
|
Turned out that didn't work. I'll check another way. |
Description
Due to the new additions of two new fields in the tx body: unordered and timeout, v0.52 clients aren't compatible with lower version anymore.
While always checking unknown fields is crucial, the client library of the SDK doesn't need to marshal all fields of the tx.
Meaning, a tx where unordered and/or timeout are set, will fail in 0.50 with a v0.52 client, when you don't make use of that feature, you should still be able to use the v0.52 client.
This PR fixes that by overriding the json encoder.
HOWEVER, there's two way to do it, and we need to pick one:
This PR took option 1, but maybe option 2 is better?
When we migrate to client/v2/tx that won't be required, as we use the other proto marshaller.
It should however be tested by the IBC team first (cc @chatton @damiannolan). I have cherry-picked this for 0.52 in this branch: https://github.com/cosmos/cosmos-sdk/tree/julien/loosen-codec-052
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
Improvements
Bug Fixes
Documentation