feat(x/tx): add custom type encoder #19786
Conversation
WalkthroughThe update introduces new functionalities in the Cosmos SDK to handle JSON marshaling for Changes
Assessment against linked issues
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 65ab253 and 47199e373767e0ee411292ad3c1c8bc5bc6ad1a4.Files selected for processing (2)
- x/tx/signing/aminojson/encoder.go (1 hunks)
- x/tx/signing/aminojson/json_marshal.go (1 hunks)
Additional comments: 2
x/tx/signing/aminojson/encoder.go (1)
- 84-93: The implementation of
cosmosInlineJsonappears correct and aligns with the PR's objectives to handle JSON marshaling forprotoreflect.Value. It would be beneficial to add a comment detailing the specific use case or scenario where this function is expected to be utilized, enhancing clarity and maintainability.x/tx/signing/aminojson/json_marshal.go (1)
- 75-75: The addition of the
inline_jsonkey to theaminoFieldEncodersmap, utilizing thecosmosInlineJsonfunction, is a crucial change for addressing the PR's objectives. It's important to verify the integration and functionality of thecosmosInlineJsonfunction within the broader context of the file and the PR's objectives to ensure proper serialization and deserialization of custom types.
x/tx/signing/aminojson/encoder.go
Outdated
| // cosmosInlineJson replicates the behavior at: | ||
| // https://github.com/CosmWasm/wasmd/blob/08567ff20e372e4f4204a91ca64a371538742bed/x/wasm/types/tx.go#L20-L22 | ||
| func cosmosInlineJson(_ *Encoder, v protoreflect.Value, w io.Writer) error { | ||
| blob, err := json.RawMessage(v.Bytes()).MarshalJSON() |
There was a problem hiding this comment.
this is a lot of machinery to just print a pre-rendered string, v.Bytes() doesn't even need to be valid json.
raw := json.RawMessage("foo")
bz, err := raw.MarshalJSON()
fmt.Println(string(bz))
=> "foo"
Given this, could this encoder just be called bytes_as_string and include a type check on v protoreflect.Value (it should be bytes) similar to nullSliceAsEmptyEncoder checks that v is a List?
There was a problem hiding this comment.
All of this PR is specific to Amino JSON and embedded somewhere in a JSON document. So I wonder why the emphesis on arbitrary bytes here is useful.
I don't think the naming choice "bytes_as_string" is great, because to me it sounds like adding a JSON string to the document instead of a nested JSON object. I.e. from the name it could easily mean that the bytes {"foo":123} becomes
{
"envelope":"{\"foo\":123}"
}instead of
{
"envelope": {
"foo": 123
}
}We need the later here, which is why I suggested inline JSON.
There was a problem hiding this comment.
Are you willing to open a PR with your suggestion? So it doesn't get lost here.
There was a problem hiding this comment.
Yeah, I can do, no probem. Just wanted to check if there is alignment on my proposal/thinking first.
There was a problem hiding this comment.
I see what you mean, and if there is a possible ambiguity, let's indeed reword it.
There was a problem hiding this comment.
Great, will take a look.
Do you have an idea how I can create a simple test that shows how the field is embedded in a larger structure (similar to shown in my comment)?
There was a problem hiding this comment.
There was a problem hiding this comment.
Great, thank you. Will do that during or after Easter depending on family mood
There was a problem hiding this comment.
Thank again! Here we go: #19919
x/tx/signing/aminojson/encoder.go
Outdated
| @@ -81,6 +81,17 @@ func nullSliceAsEmptyEncoder(enc *Encoder, v protoreflect.Value, w io.Writer) er | |||
| } | |||
| } | |||
|
|
|||
| // cosmosInlineJson replicates the behavior at: | |||
| // https://github.com/CosmWasm/wasmd/blob/08567ff20e372e4f4204a91ca64a371538742bed/x/wasm/types/tx.go#L20-L22 | |||
| func cosmosInlineJson(_ *Encoder, v protoreflect.Value, w io.Writer) error { | |||
There was a problem hiding this comment.
Can we add a test for this + changelog in x/tx ?
|
Could you as well add a mention of this new annotation on this page: https://docs.cosmos.network/v0.50/build/building-modules/protobuf-annotations (https://github.com/cosmos/cosmos-sdk/blob/main/docs/build/building-modules/05-protobuf-annotations.md) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 47199e373767e0ee411292ad3c1c8bc5bc6ad1a4 and 9879ece09c58068402782fa2096199dc89a23d13.Files selected for processing (4)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (1 hunks)
- x/tx/signing/aminojson/encoder_test.go (1 hunks)
- x/tx/signing/aminojson/json_marshal.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/tx/signing/aminojson/encoder.go
- x/tx/signing/aminojson/json_marshal.go
Additional comments: 2
x/tx/signing/aminojson/encoder_test.go (1)
- 3-49: Consider using a single assertion library (
requireorassert) throughout your tests for consistency. Mixingrequireandassertcan make the test code less coherent and slightly harder to read.x/tx/CHANGELOG.md (1)
- 34-37: The changelog entry for the added feature is clear and correctly formatted. Great job on maintaining the changelog's readability and usefulness.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 9879ece09c58068402782fa2096199dc89a23d13 and bf52ad80c31083c3f822ef9bd82f6fee3c77cca0.Files selected for processing (1)
- docs/build/building-modules/05-protobuf-annotations.md (1 hunks)
Additional comments: 2
docs/build/building-modules/05-protobuf-annotations.md (2)
- 128-133: The addition of encoding instructions for
bytesas strings using thebytes_as_stringoption is a valuable update for developers working with amino JSON encoding. This change clearly demonstrates how to apply this encoding option within protobuf annotations, enhancing the documentation's utility and clarity.- 125-133: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-133]
Ensure that the documentation is updated to reflect the new annotation for encoding
bytesas strings in the amino JSON marshaler. This addition is crucial for developers to understand how to use this specific encoding option effectively. The documentation appears to be well-structured and informative, providing valuable insights into protobuf annotations within the Cosmos SDK.
| // cosmosBytesAsString replicates the behavior at: | ||
| // https://github.com/CosmWasm/wasmd/blob/08567ff20e372e4f4204a91ca64a371538742bed/x/wasm/types/tx.go#L20-L22 | ||
| func cosmosBytesAsString(_ *Encoder, v protoreflect.Value, w io.Writer) error { | ||
| switch bz := v.Interface().(type) { |
There was a problem hiding this comment.
can this entire block be replaced with
_, err = w.Write(string(bz))
return errThere was a problem hiding this comment.
Write does not accept string , only []byte . I tried to remove the cast and it seems to work fine in wasmd ( msg is not base64 encoded)
then the function would look like this:
func cosmosBytesAsString(_ *Encoder, v protoreflect.Value, w io.Writer) error {
switch bz := v.Interface().(type) {
case []byte:
_, err := w.Write(bz)
return err
default:
return fmt.Errorf("unsupported type %T", bz)
}
}
Do you agree?
There was a problem hiding this comment.
@kocubinski @julienrbrt I did some tests and the outputs of the two implementations are the same.
Personally I would prefer the first implementation because it is more clear and the implementation matches the one in wasmd. On the other side, the second option is much shorter so it is nice as well.
Please let me know how to proceed so I will do the needed changes. Thank you!
There was a problem hiding this comment.
Sorry for the delay, please feel free to ping me in Slack if I'm not responsive on this PR.
I find below (the first implementation) confusing:
blob, err := json.RawMessage(bz).MarshalJSON()
if err != nil {
return err
}
_, err = w.Write(blob)There is mention of json, but bz need not even be valid json! the effect of this code is just to write bytes as a literal, with the exception that when the byte slice is nil the string literal "null" is printed.
// 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
}I don't want to be overly pedantic here either, since as you say either will produce the same results. Given that I'll approve as is.
There was a problem hiding this comment.
There is mention of json, but bz need not even be valid json! the effect of this code is just to write bytes as a literal
The code you are referencing requires bytes to be valid JSON which is enforced in a different part of the code:
func (r *RawContractMessage) ValidateBasic() error {
if r == nil {
return ErrEmpty
}
if !json.Valid(*r) {
return ErrInvalid
}
return nil
}So yeah, it is technically possible for a RawContractMessage instance to contain invalid JSON. But every valid usage of that type must perform this check and can then make assumptions on JSON validity later on. I with this was guaranteed at type level instead of app logic levels but this is not how json.RawMessage work and creating a custom "valid JSON" type seemed to be a bit overkill and not idiomatic.
pinosu
force-pushed
the
pinosu/inline-json-fix
branch
from
bf52ad8 to
1e3a6f6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- docs/build/building-modules/05-protobuf-annotations.md (1 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (1 hunks)
- x/tx/signing/aminojson/encoder_test.go (1 hunks)
- x/tx/signing/aminojson/json_marshal.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/tx/signing/aminojson/encoder.go
- x/tx/signing/aminojson/encoder_test.go
- x/tx/signing/aminojson/json_marshal.go
Additional comments: 2
docs/build/building-modules/05-protobuf-annotations.md (1)
- 128-133: The addition of encoding instructions for
bytesas strings using thebytes_as_stringoption is a valuable update for users needing this specific encoding behavior. However, it would enhance clarity to include a brief explanation or example of when this encoding might be necessary or beneficial.x/tx/CHANGELOG.md (1)
- 36-36: The changelog entry for adding the option to encode bytes as strings is clear and includes the necessary pull request link. It's important to ensure that this entry follows the project's standards for changelog entries, including the level of detail and formatting. If there are any additional impacts or considerations related to this change that could affect users, it might be helpful to include those details here for completeness.
| @@ -31,6 +31,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Features | |||
There was a problem hiding this comment.
Shall we prepare x/tx v0.13.2 / v0.14 now or do we have anything related to protov2 / gogo to get in before?
cc @kocubinski
Description
Closes: #19785
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
protoreflect.Valueand[]bytevalues more efficiently.