feat(x/tx): Rename custom Amino JSON encoder to "inline_json" #19919
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe recent changes revolve around optimizing byte encoding into JSON documents in a specific codebase. A key update is the renaming of Changes
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 (
|
webmaster128
changed the title
![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
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (4)
- docs/build/building-modules/05-protobuf-annotations.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (1 hunks)
- x/tx/signing/aminojson/encoder_test.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (1 hunks)
Additional Context Used
Path-based Instructions (4)
x/tx/signing/aminojson/encoder_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"docs/build/building-modules/05-protobuf-annotations.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/tx/signing/aminojson/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/tx/signing/aminojson/json_marshal.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
x/tx/signing/aminojson/encoder_test.go (1)
64-64: The comment regarding undefined behavior for invalid JSON inputs suggests a potential improvement area. Consider implementing JSON validity checks withincosmosInlineJSONto ensure only valid JSON is processed.Implementing a JSON validity check could prevent the generation of nonsensical documents and improve the robustness of the encoder.
docs/build/building-modules/05-protobuf-annotations.md (1)
129-146: Ensure the documentation clearly states that theinline_jsonoption should only be used when the bytes are guaranteed to contain valid JSON, to prevent the creation of malformed JSON documents.Clarifying this requirement can help developers understand the importance of validating JSON before using the
inline_jsonoption.x/tx/signing/aminojson/json_marshal.go (1)
75-76: The introduction of the"inline_json"field encoder aligns with the PR's objectives to handle JSON bytes more accurately. Ensure that all uses of this encoder are accompanied by appropriate JSON validity checks upstream.Consider verifying through the codebase that JSON validity checks are in place wherever the
"inline_json"encoder is used to prevent the creation of malformed JSON documents.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/tx/CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
x/tx/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
x/tx/CHANGELOG.md (1)
36-36: Consider adding a brief explanation of the "inline_json" option.While the entry for the "inline_json" option in the Amino JSON encoder is clear, it could benefit from a brief explanation or context. This would help users understand the significance of the change and how it might affect their use of the SDK.
|
Unfortunately this test is now failing: func TestRapid(t *testing.T) {
gen := rapidproto.MessageGenerator(&testpb.ABitOfEverything{}, rapidproto.GeneratorOptions{})
rapid.Check(t, func(t *rapid.T) {
msg := gen.Draw(t, "msg")
bz, err := aminojson.NewEncoder(aminojson.EncoderOptions{DoNotSortFields: true}).Marshal(msg)
assert.NilError(t, err)
checkInvariants(t, msg, bz)
})
}It seems like at some point a |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/tx/signing/aminojson/internal/testpb/test.proto (1 hunks)
- x/tx/signing/aminojson/internal/testpb/test.pulsar.go (16 hunks)
- x/tx/signing/aminojson/json_marshal_test.go (2 hunks)
Additional Context Used
Path-based Instructions (2)
x/tx/signing/aminojson/json_marshal_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/signing/aminojson/internal/testpb/test.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
x/tx/signing/aminojson/internal/testpb/test.proto (1)
44-44: The addition of thejsonfield with custom encoding attribute(amino.encoding) = "inline_json"is well-aligned with the PR's objectives and follows protobuf conventions.x/tx/signing/aminojson/json_marshal_test.go (1)
249-259: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [236-256]
The addition of the
Jsonfield in theTestIndentfunction, along with the corresponding test assertion, is correctly implemented and effectively tests theinline_jsonencoder's functionality.
|
Is there an AI tool to mute the rabbit? |
|
@coderabbitai pause |
|
Confused, the PR description states:
However the encoding function includes this comment: It is clear to me that bytes need not be valid JSON |
|
Semantically The question is just at which point the JSON validation is happening. In wasmd, this is done in ValidateBasic() of the relevant messages. But I am also happy to add a JSON validity check here to make it harder to misuse this encoder. |
This test failure doesn't seem to be due to invalid JSON, rather this code block asserts legacy amino json bytes from the codec in I would adjust the tests to keep it out of that code path. |
If that annotation is named // RawMessage is a raw encoded JSON value.
// It implements [Marshaler] and [Unmarshaler] and can
// be used to delay JSON decoding or precompute a JSON encoding.
type RawMessage []byte
// MarshalJSON returns m as the JSON encoding of m.
func (m RawMessage) MarshalJSON() ([]byte, error) {
if m == nil {
return []byte("null"), nil
}
return m, nil
} |
|
The latest commit adds a JSON validity check. I agree, it's good to have it here. Opening up the encoder for arbitrary bytes would just lead to invalid signdocs and issues further down the road.
Could you elaborate what you mean here? Should I avoid the I think it would be nice to have a test that shows the encoder in action. But yeah, comparing it with the old encoder is not needed. |
webmaster128
force-pushed
the
rename-to-inline_json
branch
from
2119604
to
00bf1f0
Compare
|
Good morning! I re-write to embedding test to use a separate proto message for it. The x/tx tests are now all passing. Unfortunately there seem to be a bunch of unrelated CI fails. From my point of view, this is ready for review. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| wantErr: false, | ||
| wantOutput: `"hey yo"`, | ||
| }, | ||
| "supported type - invalid JSON": { |
There was a problem hiding this comment.
since you're expected an error here, should the case include the name "supported type"?
suggest renaming to just "invalid JSON" (ditto for the negative err tests below also).
There was a problem hiding this comment.
My understanding is this: The term "type" here refers to the data type. "ValueOfBytes" is a supported type, but bool and int64 are not. This highlights two levels of errors: one is a wrong type and the other one is wrong contents of the right type.
However, we can also rephrase those to failure reasons only. No strong opinion what is better.
* main: (25 commits) fix: Implement gogoproto customtype to secp256r1 keys (cosmos#20027) fix(x/epochs): avoid invalid epoch duration in simulation (cosmos#20030) fix(x/bank): align query with multi denoms for send-enabled (cosmos#20028) refactor(x/slashing)!: remove Accounts String (cosmos#20026) refactor(x/evidence)!: remove Address.String() (cosmos#20016) chore: make telemetry consistent (cosmos#20025) chore: prepare x/tx changelog (cosmos#20015) build(deps): Bump actions/add-to-project from 1.0.0 to 1.0.1 (cosmos#20018) feat(x/bank): support depinject for send restrictions (cosmos#20014) feat: Conditionally emit metrics based on enablement (cosmos#19903) fix(store): fix the typo (cosmos#20011) docs(x/feegrant): fix allowance typo (cosmos#20000) chore(confix): update latest config value (cosmos#20012) refactor(x/auth): auth module can recognize x/accounts account (cosmos#20002) fix(mempool): use no-op mempool as default (cosmos#19970) fix(simapp): add epoch store to upgrade (cosmos#20007) test(kv): add unit tests for the helpers functions kv.AssertKeyAtLeas… (cosmos#19965) feat(x/tx): Rename custom Amino JSON encoder to "inline_json" (cosmos#19919) refactor(x/auth): use transaction service (cosmos#19967) fix(client/v2): add encoder for `cosmos.base.v1beta1.DecCoin` (cosmos#19976) ...
This is a follow-up to #19786 as discussed.
The important clarifications here are:
bytesmust contain valid JSON. Otherwise this produces nonsense documents.Open questions/tasks
Summary by CodeRabbit