feat: protoreflect based amino json encoder #14877
Conversation
| return jsonMarshal(w, "0") | ||
| } | ||
| return jsonMarshal(w, val) | ||
| default: |
There was a problem hiding this comment.
Same as Dec, I think cosmos.Int could be bytes too: https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/staking/v1beta1/genesis.proto#L18.
In practice I doubt that message will ever be marshalled with Amino, but I would still add it for completeness.
There was a problem hiding this comment.
I think this is the only instance in of types.Int stored as bytes in a proto message. I'll make a test case to support it though.
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
…into kocubinski/amino-json
| @@ -0,0 +1 @@ | |||
| testdata | |||
x/tx/aminojson/json_marshal.go
Outdated
| type FieldEncoder func(AminoJSON, protoreflect.Value, io.Writer) error | ||
|
|
||
| // AminoJSON is a JSON encoder that uses the Amino JSON encoding rules. | ||
| type AminoJSON struct { |
There was a problem hiding this comment.
| type AminoJSON struct { | |
| type Encoder struct { |
x/tx/aminojson/json_marshal.go
Outdated
| } | ||
|
|
||
| // NewAminoJSON returns a new AminoJSON encoder. | ||
| func NewAminoJSON() AminoJSON { |
There was a problem hiding this comment.
| func NewAminoJSON() AminoJSON { | |
| func NewEncoder() Encoder { |
x/tx/aminojson/json_marshal.go
Outdated
| } | ||
|
|
||
| // MarshalAmino serializes a protobuf message to JSON. | ||
| func (aj AminoJSON) MarshalAmino(message proto.Message) ([]byte, error) { |
There was a problem hiding this comment.
| func (aj AminoJSON) MarshalAmino(message proto.Message) ([]byte, error) { | |
| func (aj AminoJSON) Marshal(message proto.Message) ([]byte, error) { |
x/tx/aminojson/encoder.go
Outdated
| aj := Encoder{}.DefineMessageEncoding("key_field", keyFieldEncoder) | ||
| err = aj.marshalList(pubkeys, w) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Re-looking at this PR, i'm wondering if it's worth changing the message encoder func signature to:
- type MessageEncoder func(protoreflect.Message, io.Writer) error
+ type MessageEncoder func(Encoder, protoreflect.Message, io.Writer) errorthis way we don't need to create a new instance of Encoder{} here.
There was a problem hiding this comment.
It should be *Encoder in that case I think
There was a problem hiding this comment.
I'm fine with either. thresholdStringEncoder needs an Encoder instance to handle the Any types in the message.
There was a problem hiding this comment.
It should be *Encoder in that case I think
Why do you say so?
There was a problem hiding this comment.
Efficiency, so there's less copying
Description
Closes: #10993
Follows: #15019
This patch implements an Amino JSON encoder for
proto.Messagetypes using the protoreflect API. Therefore, protoreflect capable protoc generated types ("pulsar" types) can be serialized to bytes identical to an equivalent gogo generated type serialized through the legacy amino codec in go-amino.For example, given the IDL specification for
auth.ModuleAccount, the gogo type serialized through go-amino and the pulsar type serialized through the encoder in this patch produce the same bytes.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...
!to 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...
!in the type prefix if API or client breaking change