Implement protoreflect based amino JSON encoder

[aaronc]

Background

Currently amino 1) only works on golang structs and 2) doesn't work with the new pulsar protobuf generated code, only gogo because of some hacks. Amino JSON is still needed for ledger signing for the forseeable future so we need to support this.

To enable both the new protobuf generated code to work with Amino and dynamic clients such as lens which may not have generated code for all proto descriptors, we will need a dynamic Amino JSON encoder. Also proto descriptors on their own do not include amino names.

Design

This encoder should:

• work against any message that implements protoreflect.Message and not depend on go-amino at all, just using protoreflect not go struct reflection, and
• use .proto file options to specify things like amino types names (ex: cosmos.msg.legagy_amino.v1.name) and omitempty for fields, ex:

message MsgSend {
  option (cosmos.msg.legacy_amino.v1.name) = "cosmos-sdk/MsgSend";
  ...

  // say we have an optional field that gets added
  string note = 4 [(cosmos.msg.legacy_amino.v1.omit_empty) = true];
}

Once this done, we may be able to get rid of the amino golang dependency. Note that adding these annotations will also help clients such as CosmJS.

Concretely, let's create a type in the codec/amino package:

type AminoJSONEncoder interface {
  Marshal(proto.Message) ([]byte, error)
}

We don't need to use any of the existing codec registration infrastructure as all amino encoding info would be stored in the .proto files using cosmos.msg.v1 proto options. There is no need for RegisterConcrete, RegisterInterface, etc. We don't need any binary encoding/decoding or JSON decoding, just JSON encoding.

Tests should compare encoding of a number of different SDK messages using gogo codegen + go-amino with the same messages using pulsar + the new encoder. We can avoid manual copying in these tests by proto marshaling between gogo and pulsar. Tests should leverage rapid to generate data and must test all custom types (int, dec, timestamps) used in SDK messages.

TODO (in separate PRs)

• write tests that fail against the AminoJSONEncoder interface above (do this first!) - we probably want to leverage Rapid proto message generator #14704 for this
• define options in cosmos.msg.legacy_amino.v1 proto package (name, omit_empty, etc.) feat: Add proto annotations for Amino JSON #13501
• add legacy amino options to all proto messages feat: Add proto annotations for Amino JSON #13501
• implement AminoJSONEncoder based on https://github.com/tendermint/go-amino/blob/master/json-encode.go but without using go reflection, just protoreflect (tests for structs which don't use custom types should pass now)
• handle custom types in AminoJSONEncoder based on cosmos_proto.scalar (some tests will likely fail until this is done)

[tac0turtle]

To add on to this, Tendermint Is deprecating tmjson in 0.36, we should be prepared to include this in 0.46 in this case, or copy over tmjson if needed.

[alexanderbez]

This would be a state machine breaking changes for existing messages, correct @aaronc?

[aaronc]

This would be a state machine breaking changes for existing messages, correct @aaronc?

No state machine nor protocol breaking changes. We're just talking about making amino JSON work for new protobuf generated code. Same proto types, same amino JSON, different code generator.

[alexanderbez]

Ok, I see. If what you say is indeed true, then I think this is pretty high priority. We should speak about this in terms of approach and implementation details.

[aaronc]

We chatted about it in our last framework WG and tried to capture as many details as possible in the issue description

[robert-zaremba]

In AminoJSONEncoder, I think the method name should be MarshalAmino rather than Marshal.
For example, json.Marshaller has MarshalJSON.
Maybe we could rename AminoJSONEncoder to AminoJSONMarshaller or code/amino.JSONMarshaller.

[aaronc]

See cosmos/cosmos-proto#83 for some prior work that may be useful for how to structure this encoder

[kocubinski]

I'm going for feature parity between this implementation and the current/legacy amino.MarshalJSON, however the legacy implementation can sometimes produce bytes which are invalid to google.golang.org/protobuf/encoding/protojson.Unmarshal. One such case is illustrated below.

cosmos-sdk/codec/aminojson/marshal_test.go

Line 45 in e001be7

"zero duration": {msg: &testpb.ABitOfEverything{Duration: durationpb.New(time.Second * 0)}},

The legacy implementation encodes this as {"duration":{}} while protojson expects {"duration":"0s"}.

Another example is the handling of Any types whose marshaling differs like;

// aminojson
{"any":{"type_url":"type.googleapis.com/testpb.NestedMessage","value":"Cg9hbnkgdHlwZSBuZXN0ZWQQCw=="}} 
// protojson
{"any":{"@type":"type.googleapis.com/testpb.NestedMessage","foo":"any type nested","bar":11}}

The full test covering these cases can be seen here.

Is this a problem? If not, is there another unmarshaller I should be using in test instead?

[aaronc]

It should definitely be incompatible with protojson.Unmarshal. It's amino json not proto json. It should be possible to unmarshal with amino unmarshaling to the existing gogo proto types