Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(x/tx): Rename custom Amino JSON encoder to "inline_json" #19919

Merged
merged 6 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions docs/build/building-modules/05-protobuf-annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,21 @@ Encoding instructs the amino json marshaler how to encode certain fields that ma
https://github.com/cosmos/cosmos-sdk/blob/e8f28bf5db18b8d6b7e0d94b542ce4cf48fed9d6/proto/cosmos/bank/v1beta1/genesis.proto#L23
```

Another example is how `bytes` is encoded when using the amino json encoding format. The `bytes_as_string` option tells the json marshaler [how to encode bytes as string](https://github.com/pinosu/cosmos-sdk/blob/9879ece09c58068402782fa2096199dc89a23d13/x/tx/signing/aminojson/json_marshal.go#L75).
Another example is a protobuf `bytes` that contains a valid JSON document.
The `inline_json` option tells the json marshaler to embed the JSON bytes into the wrapping document without escaping.

```proto
(amino.encoding) = "bytes_as_string",
```
(amino.encoding) = "inline_json",
```

E.g. the bytes containing `{"foo":123}` in the `envelope` field would lead to the following JSON:

```json
{
"envelope": {
"foo": 123
}
}
```

If the bytes are not valid JSON, this leads to JSON broken documents. Thus a JSON validity check needs to be in place at some point of the process.
2 changes: 1 addition & 1 deletion x/tx/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* [#19786](https://github.com/cosmos/cosmos-sdk/pull/19786) Add bytes as string option to encoder.
* [#19786](https://github.com/cosmos/cosmos-sdk/pull/19786)/[#19919](https://github.com/cosmos/cosmos-sdk/pull/19919) Add "inline_json" option to Amino JSON encoder.

### Improvements

Expand Down
17 changes: 11 additions & 6 deletions x/tx/signing/aminojson/encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,21 @@ func nullSliceAsEmptyEncoder(enc *Encoder, v protoreflect.Value, w io.Writer) er
}
}

// cosmosBytesAsString replicates the behavior at:
// cosmosInlineJSON takes bytes and inlines them into a JSON document.
//
// This requires the bytes contain valid JSON since otherwise the resulting document would be invalid.
// Invalid JSON will result in an error.
//
// This replicates the behavior of JSON messages embedded in protobuf bytes
// required for CosmWasm, e.g.:
// https://github.com/CosmWasm/wasmd/blob/08567ff20e372e4f4204a91ca64a371538742bed/x/wasm/types/tx.go#L20-L22
func cosmosBytesAsString(_ *Encoder, v protoreflect.Value, w io.Writer) error {
func cosmosInlineJSON(_ *Encoder, v protoreflect.Value, w io.Writer) error {
switch bz := v.Interface().(type) {
case []byte:
blob, err := json.RawMessage(bz).MarshalJSON()
if err != nil {
return err
if !json.Valid(bz) {
return errors.New("invalid JSON bytes")
}
_, err = w.Write(blob)
_, err := w.Write(bz)
return err
default:
return fmt.Errorf("unsupported type %T", bz)
Expand Down
55 changes: 49 additions & 6 deletions x/tx/signing/aminojson/encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,64 @@ import (
"gotest.tools/v3/assert"
)

func TestCosmosBytesAsString(t *testing.T) {
func TestCosmosInlineJSON(t *testing.T) {
cases := map[string]struct {
value protoreflect.Value
wantErr bool
wantOutput string
}{
"valid bytes - json": {
"supported type - valid JSON object": {
value: protoreflect.ValueOfBytes([]byte(`{"test":"value"}`)),
wantErr: false,
wantOutput: `{"test":"value"}`,
},
"valid bytes - string": {
value: protoreflect.ValueOfBytes([]byte(`foo`)),
"supported type - valid JSON array": {
// spaces are normalized away
value: protoreflect.ValueOfBytes([]byte(`[1,2,3]`)),
wantErr: false,
wantOutput: `foo`,
wantOutput: `[1,2,3]`,
},
"supported type - valid JSON is not normalized": {
value: protoreflect.ValueOfBytes([]byte(`[1, 2, 3]`)),
wantErr: false,
wantOutput: `[1, 2, 3]`,
},
"supported type - valid JSON array (empty)": {
value: protoreflect.ValueOfBytes([]byte(`[]`)),
wantErr: false,
wantOutput: `[]`,
},
"supported type - valid JSON number": {
value: protoreflect.ValueOfBytes([]byte(`43.72`)),
wantErr: false,
wantOutput: `43.72`,
},
"supported type - valid JSON boolean": {
value: protoreflect.ValueOfBytes([]byte(`true`)),
wantErr: false,
wantOutput: `true`,
},
"supported type - valid JSON null": {
value: protoreflect.ValueOfBytes([]byte(`null`)),
wantErr: false,
wantOutput: `null`,
},
"supported type - valid JSON string": {
value: protoreflect.ValueOfBytes([]byte(`"hey yo"`)),
wantErr: false,
wantOutput: `"hey yo"`,
},
"supported type - invalid JSON": {
Copy link
Member

@kocubinski kocubinski Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value: protoreflect.ValueOfBytes([]byte(`foo`)),
wantErr: true,
},
"supported type - invalid JSON (empty)": {
value: protoreflect.ValueOfBytes([]byte(``)),
wantErr: true,
},
"supported type - invalid JSON (nil bytes)": {
value: protoreflect.ValueOfBytes(nil),
wantErr: true,
},
"unsupported type - bool": {
value: protoreflect.ValueOfBool(true),
Expand All @@ -38,7 +81,7 @@ func TestCosmosBytesAsString(t *testing.T) {
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
var buf bytes.Buffer
err := cosmosBytesAsString(nil, tc.value, &buf)
err := cosmosInlineJSON(nil, tc.value, &buf)

if tc.wantErr {
require.Error(t, err)
Expand Down
4 changes: 4 additions & 0 deletions x/tx/signing/aminojson/internal/testpb/test.proto
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ message WithAList {
repeated string list = 2;
}

message WithAJson {
bytes field1 = 1 [(amino.encoding) = "inline_json"];
}

message ABitOfEverything {
option (amino.name) = "ABitOfEverything";

Expand Down
Loading
Loading