[codec] Calling proto.MessageName on sdk.Msgs after decoding a TX returns an empty string

[fdymylja]

Summary of Bug

As per title, calling proto.MessageName on an sdk.Msg contained in a decoded transaction (decoded via the sdk.TxDecoder) returns an empty string.

This happens because sdk.Msgs are wrapped into sdk.ServiceMsg which does not fully and correctly implement the proto.Message interface.

We could make sdk.ServiceMsg return the sdk.Msg implementation (which service msg wraps). But this could bring some recursivity issues as sdk.ServiceMsg implements sdk.MsgRequest too, so I could wrap an sdk.ServiceMsg as the sdk.MsgRequest.

Probably a better solution would be to get rid of sdk.ServiceMsg, and use plain sdk.Msgs.

From what I've seen, sdk.ServiceMsg is used only to provide a route for a given sdk.Msg, most likely we could map routes using the msg (proto) name itself, instead of the method.

If I'm missing some context, please feel free to correct me.

Version

Steps to Reproduce

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[jgimeno]

@aaronc @alessio

[clevinson]

@fdymylja when you say decoding a transaction, do you mean using the /decode transaction endpoint?

@AmauryM i think should have some context as to the necessity of ServiceMsg, maybe he can speak to if dropping is possible?

[fdymylja]

@fdymylja when you say decoding a transaction, do you mean using the /decode transaction endpoint?

I meant using the tx decoder. I will update the description.

[amaury1093]

ServiceMsg is not a proto.Message. The underlying question is: What is the use case of calling proto.MessageName on sdk.Msg?

[fdymylja]

ServiceMsg is not a proto.Message. The underlying question is: What is the use case of calling proto.MessageName on sdk.Msg?

I see, but it is implemented as proto.Message, in rosetta we use the proto messages utilities to facilitate construction and parsing of sdk messages in transactions, which needs to be treated in a generalized way.

Plus, since sdk.Msg implements proto.Message, I think it's expected for it to behave as such. WDYT?

[amaury1093]

Plus, since sdk.Msg implements proto.Message, I think it's expected for it to behave as such. WDYT?

Ah yeah, we did add proto.Message implementation, as a hack for legacy Msg compatibility, it will be removed once we remove legacy support. IMO, now we should panic on ProtoMessage(), because it really should never be called.

In Rosetta, I think the method you are looking for is Route():

• for service msgs, it returns the service Msg FQ name, e.g. /cosmos.bank.v1beta1.Msg/Send
• for legacy proto msgs, it returns the proto Msg FQ name, e.g. /cosmos.bank.v1beta1.MsgSend (no additional slash)
• for legacy amiino, it returns old amino route, e.g. cosmos-sdk/MsgSend

[fdymylja]

Ah yeah, we did add proto.Message implementation, as a hack for legacy Msg compatibility, it will be removed once we remove legacy support. IMO, now we should panic on ProtoMessage(), because it really should never be called.

You mean on ServiceMsg?

In Rosetta, I think the method you are looking for is Route():

• for service msgs, it returns the service Msg FQ name, e.g. /cosmos.bank.v1beta1.Msg/Send
• for legacy proto msgs, it returns the proto Msg FQ name, e.g. /cosmos.bank.v1beta1.MsgSend (no additional slash)
• for legacy amiino, it returns old amino route, e.g. cosmos-sdk/MsgSend

No, I need the msg type so I can resolve it via interface registry, and then fill its fields.

[amaury1093]

You mean on ServiceMsg?

Yes, panic here (these 3 implementation methods are just hacky...).

No, I need the msg type so I can resolve it via interface registry, and then fill its fields.

I believe this is a XY problem. If you have a PR/branch, I'm happy to take a look and help, but I'm pretty sure calling proto.MessageName() on sdk.Msg is not the correct solution.

[aaronc]

With the current design,ServiceMsg should eventually supplant Msg. The short term solution here is probably removing proto.Message from the sdk.Msg interface to make things clear and use Route instead.

[fdymylja]

and use Route instead

But calling Route() on an sdk.Msg returns the module key, or am I missing something?

If you have a PR/branch, I'm happy to take a look and help, but I'm pretty sure calling proto.MessageName() on sdk.Msg is not the correct solution.

BTW, maybe there's some confusion on the use case, but here is how in rosetta thanks to interface registry and the message name, we can allow users to build any kind of transaction (multiple messages, multiple signers, etc) in a generalized way.

https://github.com/cosmos/cosmos-sdk/blob/frojdi/rosetta-balance-tracking/server/rosetta/convert/converter.go#L89
https://github.com/cosmos/cosmos-sdk/blob/frojdi/rosetta-balance-tracking/server/rosetta/convert/converter.go#L182

With the current design,ServiceMsg should eventually supplant Msg

I would think about this, like building transactions with the tx builder by simply settings messages is convenient, and having it being wrapped in a service msg I think adds unnecessary complexity. Also in this case encoding sdk.Msgs and having decoded back sdk.ServiceMsg is inconsistent and should at some point be addressed.

The short term solution here is probably removing proto.Message from the sdk.Msg interface to make things clear and use Route instead

I hope this does not mean we want to get rid of the proto implementations on messages, I think with the implementation of protov2 is gonna open up a lot of possibilities and cool tooling (ex: a generalized query and msg command builder)

[amaury1093]

The short term solution here is probably removing proto.Message from the sdk.Msg interface to make things clear

iirc we wanted that, but our Any packing of Msgs requires them to be proto.Messages. Panicking in the ServiceMsg proto.Message stubs is the next best thing i can think of.

[aaronc]

and use Route instead

But calling Route() on an sdk.Msg returns the module key, or am I missing something?

If you have a PR/branch, I'm happy to take a look and help, but I'm pretty sure calling proto.MessageName() on sdk.Msg is not the correct solution.

BTW, maybe there's some confusion on the use case, but here is how in rosetta thanks to interface registry and the message name, we can allow users to build any kind of transaction (multiple messages, multiple signers, etc) in a generalized way.

https://github.com/cosmos/cosmos-sdk/blob/frojdi/rosetta-balance-tracking/server/rosetta/convert/converter.go#L89

https://github.com/cosmos/cosmos-sdk/blob/frojdi/rosetta-balance-tracking/server/rosetta/convert/converter.go#L182

With the current design,ServiceMsg should eventually supplant Msg

I would think about this, like building transactions with the tx builder by simply settings messages is convenient, and having it being wrapped in a service msg I think adds unnecessary complexity. Also in this case encoding sdk.Msgs and having decoded back sdk.ServiceMsg is inconsistent and should at some point be addressed.

The short term solution here is probably removing proto.Message from the sdk.Msg interface to make things clear and use Route instead

I hope this does not mean we want to get rid of the proto implementations on messages, I think with the implementation of protov2 is gonna open up a lot of possibilities and cool tooling (ex: a generalized query and msg command builder)

ServiceMsg is the protobuf way of encoding Msgs now. sdk.Msg should be considered a legacy holdover. See ADR 031 for context. The default is to create a service definition now for request and response types - that maps to ServiceMsg. Does that make sense?

[fdymylja]

ServiceMsg is the protobuf way of encoding Msgs now. sdk.Msg should be considered a legacy holdover. See ADR 031 for context. The default is to create a service definition now for request and response types - that maps to ServiceMsg. Does that make sense?

So, for example, when using the tx builder we should set msgs by wrapping them with ServiceMsg?

And if I wanted to go back, let's say I'm given the route then I want to get the request (sdk.Msg) to use for that route, how should I go about that? With rosetta ATM we map requests using messages name, and use the interface registry to build messages, which seems a lot more easier than doing all the wrapping.

[aaronc]

ServiceMsg is the protobuf way of encoding Msgs now. sdk.Msg should be considered a legacy holdover. See ADR 031 for context. The default is to create a service definition now for request and response types - that maps to ServiceMsg. Does that make sense?

So, for example, when using the tx builder we should set msgs by wrapping them with ServiceMsg?

More or less. We should probably add a SetServiceMsgs method to TxBuilder and mark SetMsgs as deprecated.

And if I wanted to go back, let's say I'm given the route then I want to get the request (sdk.Msg) to use for that route, how should I go about that? With rosetta ATM we map requests using messages name, and use the interface registry to build messages, which seems a lot more easier than doing all the wrapping.

For a new ADR 031 only module, you should be able to use the interface registry to get the request type for a service method. Say for the method /cosmos.authz.v1beta1.Msg/GrantAuthorization, calling InterfaceRegistry.Resolve should return MsgGrantAuthorizationRequest. Does that help?

We can chat about this on the Friday call if you'd like.

[fdymylja]

For a new ADR 031 only module, you should be able to use the interface registry to get the request type for a service method. Say for the method /cosmos.authz.v1beta1.Msg/GrantAuthorization, calling InterfaceRegistry.Resolve should return MsgGrantAuthorizationRequest. Does that help?

Thank you. I was not aware that the registry also mapped gRPC methods to request messages.

Perhaps the only thing it'd be worth to talk about is how to cleanup some APIs to reflect the 031 spec? IMHO, some of them are a little confusing. Maybe we could open an issue and start listing them

[amaury1093]

Perhaps the only thing it'd be worth to talk about is how to cleanup some APIs to reflect the 031 spec? IMHO, some of them are a little confusing. Maybe we could open an issue and start listing them

I think that's a great idea 👍 , we need an issue about txBuilder UX with ADR-031. Right now it's really not ideal.

IMO it belongs to a bigger story, could you make sure to link relevant issues (#8138, #8270, #7541)?

[aaronc]

Perhaps the only thing it'd be worth to talk about is how to cleanup some APIs to reflect the 031 spec? IMHO, some of them are a little confusing. Maybe we could open an issue and start listing them

I think that's a great idea 👍 , we need an issue about txBuilder UX with ADR-031. Right now it's really not ideal.

IMO it belongs to a bigger story, could you make sure to link relevant issues (#8138, #8270, #7541)?

I think #8138 suffices for now. Let's just add more context there.

[clevinson]

Should we close this issue then if we have a clear action item tracked elsewhere? Or is there still something specific that we need to tackle from this?

[fdymylja]

Should we close this issue then if we have a clear action item tracked elsewhere? Or is there still something specific that we need to tackle from this?

The only doubt I still have is understanding the cases in which tx encoding and decoding produces a tx in which msgs are wrapped as service msgs or not, just to avoid having to cast it in order to access functionalities such as Route() (which in unwrapped messages return the module key and not the gRPC methods)

[technicallyty]

closing as service msgs are no longer in the sdk #9139