refactor(x/tx): use dynamicpb instead of type resolver in any marshal #16048
Conversation
could it be this easy?
aminojson.marhsalAny now uses a fileResolver and dynamicpb instead of a typeResolver
| @@ -17,17 +20,26 @@ type MessageEncoder func(*Encoder, protoreflect.Message, io.Writer) error | |||
| // FieldEncoder is a function that can encode a protobuf protoreflect.Value to JSON. | |||
| type FieldEncoder func(*Encoder, protoreflect.Value, io.Writer) error | |||
|
|
|||
| // EncoderOptions are options for creating a new Encoder. | |||
| type EncoderOptions struct { | |||
| FileResolver signing.ProtoFileResolver | |||
There was a problem hiding this comment.
I think there should be a TypeResolver option too
| msgName) | ||
| } | ||
|
|
||
| valueMsg := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)).New().Interface() |
There was a problem hiding this comment.
I think we should use the type from the type resolver if it resolves first, if not then use dynamicpb
| @@ -95,11 +101,36 @@ func NewOperationMsg(msg sdk.Msg, ok bool, comment string, cdc *codec.ProtoCodec | |||
| moduleName = msgType | |||
| } | |||
|
|
|||
| if legacyMsg, okType := msg.(legacytx.LegacyMsg); okType { | |||
There was a problem hiding this comment.
Another option I explored was packing sdk.Msg into an any message and then marshaling that, but if we do
the encoder will reject messages that are not tagged with an amino.name. Strictly speaking we don't need
an amino.name on messages which aren't expected to be packed into any fields except for this one case.
types/simulation/types.go
Outdated
| resolver := gogoproto.HybridResolver | ||
| desc, err := resolver.FindDescriptorByName(protoreflect.FullName(gogoproto.MessageName(msg))) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to find proto descriptor for %s: %w", msgType, err)) | ||
| } | ||
|
|
||
| dynamicMsgType := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)) | ||
| dynamicMsg := dynamicMsgType.New().Interface() | ||
| gogoBytes, err := gogoproto.Marshal(msg) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to marshal msg: %w", err)) | ||
| } | ||
| err = proto.Unmarshal(gogoBytes, dynamicMsg) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to unmarshal msg: %w", err)) | ||
| } | ||
|
|
||
| encoder := aminojson.NewEncoder(aminojson.EncoderOptions{FileResolver: resolver}) | ||
| jsonBytes, err := encoder.Marshal(dynamicMsg) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to marshal msg: %w", err)) | ||
| } |
There was a problem hiding this comment.
Since this will be pretty common let's create some helpers for it. I would even go as far as saying that we can add a MarshalAminoJSON method to codec.Codec which does exactly this - it already has the proper file resolver from InterfaceRegistry
There was a problem hiding this comment.
Are you suggesting adding MarshalAminoJSON to JSONCodec?
…into kocubinski/x-tx-0.7.0
codec/proto_codec.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| dynamicMsgType := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)) |
There was a problem hiding this comment.
Should we try to find a non-dynamic version in protoregistry.GlobalTypes first? That might be slightly more performant than dynamicpb
| typeResolver protoregistry.MessageTypeResolver | ||
| encoder Encoder | ||
| } | ||
|
|
||
| // SignModeHandlerOptions are the options for the SignModeHandler. | ||
| type SignModeHandlerOptions struct { | ||
| FileResolver protodesc.Resolver | ||
| FileResolver signing.ProtoFileResolver |
There was a problem hiding this comment.
Can't we just use the file resolver on the Encoder? Or is this so that Encoder can be left nil?
There was a problem hiding this comment.
Right this supports he case where Encoder is nil
Co-authored-by: Aaron Craelius <aaron@regen.network>
…into kocubinski/x-tx-0.7.0
…cosmos#16048) Co-authored-by: Aaron Craelius <aaron@regen.network>
Description
Closes: #16024
legacytx.GetSignBytesin the SDK from the simulation package.aminojson.Encoderso that the dynamicpb API be used with the HybridResolver in gogoproto. This breaks a hard, indirect, dependency on cosmossdk.io/api which was needed for handling any types. When the type isn't loaded or found we now fall back to dynamicpb.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