-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor(x/tx): use dynamicpb instead of type resolver in any marshal #16048
Changes from 38 commits
09d9e6b
82ca71f
3091b43
e7bf7bb
4a367d4
5d45211
f28eda3
53b1522
38c95b8
bbf9a4e
01e92b9
2c120b5
b7e7c22
1370739
a9e188d
a2127f8
3986662
0e4559d
5a92828
ec384c5
3b11f31
b1f4fde
890a927
5815c54
abf3c56
1ba32d7
e95633b
184baab
20517ce
8d6dc66
7209dec
0dbf476
e18817b
58c019f
4636726
90527b4
87cfdda
0a76cb9
70a6870
f16c9e7
f63f47e
0687069
51dca43
ad360d4
ba4a5e7
693c267
73cac56
e9c8c53
93d1362
09e7597
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,20 @@ package simulation | |
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"math/rand" | ||
"time" | ||
|
||
gogoproto "github.com/cosmos/gogoproto/proto" | ||
"google.golang.org/protobuf/proto" | ||
protoreflect "google.golang.org/protobuf/reflect/protoreflect" | ||
"google.golang.org/protobuf/types/dynamicpb" | ||
|
||
"cosmossdk.io/x/tx/signing/aminojson" | ||
"github.com/cosmos/cosmos-sdk/baseapp" | ||
"github.com/cosmos/cosmos-sdk/codec" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/cosmos-sdk/types/kv" | ||
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" | ||
) | ||
|
||
// Deprecated: Use WeightedProposalMsg instead. | ||
|
@@ -95,11 +101,35 @@ func NewOperationMsg(msg sdk.Msg, ok bool, comment string, cdc *codec.ProtoCodec | |
moduleName = msgType | ||
} | ||
|
||
if legacyMsg, okType := msg.(legacytx.LegacyMsg); okType { | ||
return NewOperationMsgBasic(moduleName, msgType, comment, ok, legacyMsg.GetSignBytes()) | ||
// Below is logic added for aminojson.Encoder compatibility with gogoproto messages. | ||
// x/tx/signing/aminojson cannot marshal sdk.Msg (an alias to gogoproto.message) directly since this type is not | ||
// protoreflect enabled like the std library google.golang.org/protobuf/proto.Message is. | ||
// | ||
// So we just convert to a dynamicpb message and marshal that directly | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting adding |
||
|
||
return NewOperationMsgBasic(moduleName, msgType, comment, ok, cdc.MustMarshalJSON(msg)) | ||
return NewOperationMsgBasic(moduleName, msgType, comment, ok, jsonBytes) | ||
} | ||
|
||
// NoOpMsg - create a no-operation message | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ import ( | |
"encoding/json" | ||
"fmt" | ||
|
||
"google.golang.org/protobuf/reflect/protodesc" | ||
"google.golang.org/protobuf/reflect/protoregistry" | ||
|
||
signingv1beta1 "cosmossdk.io/api/cosmos/tx/signing/v1beta1" | ||
|
@@ -16,14 +15,14 @@ import ( | |
|
||
// SignModeHandler implements the SIGN_MODE_LEGACY_AMINO_JSON signing mode. | ||
type SignModeHandler struct { | ||
fileResolver protodesc.Resolver | ||
fileResolver signing.ProtoFileResolver | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just use the file resolver on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right this supports he case where |
||
TypeResolver protoregistry.MessageTypeResolver | ||
Encoder *Encoder | ||
} | ||
|
@@ -42,7 +41,10 @@ func NewSignModeHandler(options SignModeHandlerOptions) *SignModeHandler { | |
h.typeResolver = options.TypeResolver | ||
} | ||
if options.Encoder == nil { | ||
h.encoder = NewEncoder() | ||
h.encoder = NewEncoder(EncoderOptions{ | ||
FileResolver: options.FileResolver, | ||
TypeResolver: options.TypeResolver, | ||
}) | ||
} else { | ||
h.encoder = *options.Encoder | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,34 +4,87 @@ import ( | |
"fmt" | ||
"io" | ||
|
||
"google.golang.org/protobuf/types/dynamicpb" | ||
"google.golang.org/protobuf/types/known/anypb" | ||
|
||
"github.com/pkg/errors" | ||
"google.golang.org/protobuf/proto" | ||
"google.golang.org/protobuf/reflect/protoreflect" | ||
"google.golang.org/protobuf/reflect/protoregistry" | ||
) | ||
|
||
func (enc Encoder) marshalAny(message protoreflect.Message, writer io.Writer) error { | ||
anyMsg := message.Interface().(*anypb.Any) | ||
resolver := protoregistry.GlobalTypes | ||
// when a message contains a nested any field, and the top-level message has been unmarshalled into a dyanmicpb, | ||
// the nested any field will also be a dynamicpb. In this case, we must use the dynamicpb API. | ||
_, ok := message.Interface().(*dynamicpb.Message) | ||
if ok { | ||
return enc.marshalDynamic(message, writer) | ||
} | ||
|
||
typ, err := resolver.FindMessageByURL(anyMsg.TypeUrl) | ||
if err != nil { | ||
return errors.Wrapf(err, "can't resolve type URL %s", anyMsg.TypeUrl) | ||
anyMsg, ok := message.Interface().(*anypb.Any) | ||
if !ok { | ||
return fmt.Errorf("expected *anypb.Any, got %T", message.Interface()) | ||
} | ||
|
||
valueMsg := typ.New() | ||
err = proto.Unmarshal(anyMsg.Value, valueMsg.Interface()) | ||
if err != nil { | ||
return err | ||
// build a message of the correct type | ||
var protoMessage protoreflect.Message | ||
typ, err := enc.typeResolver.FindMessageByURL(anyMsg.TypeUrl) | ||
if err == nil { | ||
// If the type is registered, we can use the proto API to unmarshal into a concrete type. | ||
valueMsg := typ.New() | ||
err = proto.Unmarshal(anyMsg.Value, valueMsg.Interface()) | ||
if err != nil { | ||
return err | ||
} | ||
protoMessage = valueMsg | ||
} else { | ||
// otherwise we use the dynamicpb API to unmarshal into a dynamic message. | ||
desc, err := enc.fileResolver.FindDescriptorByName(protoreflect.FullName(anyMsg.TypeUrl[1:])) | ||
if err != nil { | ||
return errors.Wrapf(err, "can't resolve type URL %s", anyMsg.TypeUrl) | ||
} | ||
|
||
valueMsg := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)).New().Interface() | ||
err = proto.Unmarshal(anyMsg.Value, valueMsg) | ||
if err != nil { | ||
return err | ||
} | ||
protoMessage = valueMsg.ProtoReflect() | ||
} | ||
|
||
_, named := getMessageAminoName(valueMsg) | ||
_, named := getMessageAminoName(protoMessage.Descriptor().Options()) | ||
if !named { | ||
return fmt.Errorf("message %s is packed into an any field, so requires an amino.name annotation", | ||
anyMsg.TypeUrl) | ||
} | ||
|
||
return enc.beginMarshal(valueMsg, writer) | ||
return enc.beginMarshal(protoMessage, writer) | ||
} | ||
|
||
const ( | ||
anyTypeURLFieldName = "type_url" | ||
anyValueFieldName = "value" | ||
) | ||
|
||
func (enc Encoder) marshalDynamic(message protoreflect.Message, writer io.Writer) error { | ||
msgName := message.Get(message.Descriptor().Fields().ByName(anyTypeURLFieldName)).String()[1:] | ||
msgBytes := message.Get(message.Descriptor().Fields().ByName(anyValueFieldName)).Bytes() | ||
|
||
desc, err := enc.fileResolver.FindDescriptorByName(protoreflect.FullName(msgName)) | ||
if err != nil { | ||
return errors.Wrapf(err, "can't resolve type URL %s", msgName) | ||
} | ||
|
||
_, named := getMessageAminoName(desc.Options()) | ||
if !named { | ||
return fmt.Errorf("message %s is packed into an any field, so requires an amino.name annotation", | ||
msgName) | ||
} | ||
|
||
valueMsg := dynamicpb.NewMessageType(desc.(protoreflect.MessageDescriptor)).New().Interface() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should use the type from the type resolver if it resolves first, if not then use dynamicpb |
||
err = proto.Unmarshal(msgBytes, valueMsg) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return enc.beginMarshal(valueMsg.ProtoReflect(), writer) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.