[types.TxMsgData, types.MsgData]: consider using google.Protobuf.Any instead of sdk.MsgData

[fdymylja]

In the SDK, after we finalise executing a transaction, we marshal into ResponseDeliverTx.Data the types.TxMsgData https://github.com/cosmos/cosmos-sdk/blob/master/types/abci.pb.go#L462.

This object contains a slice of types.MsgData https://github.com/cosmos/cosmos-sdk/blob/master/types/abci.pb.go#L408.

types.MsgData in the end is an object with a type URL and a value, and hence could be represented as a google.Protobuf.Any.

Reasons for which this change should be made: google.Protobuf.Any offers a simple API for resolving typeURLs into concrete messages, types.MsgData, aside from being an alias object, offers no such API and makes it more difficult for external consumers to understand how to parse the response correctly.

Even at a cross-language level, understanding an Any is easier than understanding a custom type.

[aaronc]

I'm actually not sure how we ended up with this design at all for the result and honestly it seems way too heavy handed.

We don't need to include the msg type URL at all because we know that from the tx. I think we could get away with just serializing each response item with no type URL at all and just length prefixing each one. We know the response type from the proto files, no need to be redundant.

Also side note, I'm not sure who is even using this response data but I think we should provide an easy way to retrieve it and we don't.

[jgimeno]

If I am not wrong an example usage of it is the response of the proposal ID when creating a governance proposal.

[fdymylja]

Also, the type url refers to the MsgRequest and not MsgResponse, which is a further deviation from standard behaviour, IDK if intentional, but it is misleading: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/run_msgs.go#L131 <- we call type URL on the request and not response.

[aaronc]

If I am not wrong an example usage of it is the response of the proposal ID when creating a governance proposal.

Yep. We should have a way to get that.

Also, the type url refers to the MsgRequest and not MsgResponse, which is a further deviation from standard behaviour, IDK if intentional, but it is misleading: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/run_msgs.go#L131 <- we call type URL on the request and not response.

I know. It should be the MsgResponse if anything, but the type URL isn't really required for either...

[aaronc]

As an example of how I think this could work for clients without a lot of extra codegen, we could have the response types get populated after the tx completes (in block mode) using some hypothetical TxClient. Because they're pointers I think that's sort of okay. It's not a very functional approach so you could call it hacky, but also we are in an imperative language. Ex:

txClient := NewTxClient(...)

txClient.StartTx()
govMsgClient := gov.NewMsgClient(txClient)
res, err := govMsgClient.SubmitProposal(...)
if err != nil { ... }
err = txClient.ExecuteBlock()
if err != nil { ... }

// now res has been unmarshaled to MsgSubmitProposalResponse because we executed in block mode
// and we can do something with the proposal ID
txClient.StartTx()
govMsgClient.Deposit(MsgDeposit{ProposalId: res.ProposalId, ...})
...

We could do some special codegen for this to have real async MsgClients. I started looking into that a while ago. But maybe even this approach is sufficient for now? What do you think?

@AmauryM btw it would be useful to start thinking about some of this as we are working on TxBuilder improvements. Maybe we can create some sort of TxClient that encapsulates what's in TxBuilder + a signing key + an RPC connection for a smoother client tx workflow.

[amaury1093]

We don't need to include the msg type URL at all because we know that from the tx. I think we could get away with just serializing each response item with no type URL at all and just length prefixing each one. We know the response type from the proto files, no need to be redundant.

I think I still prefer to use an Any though: it would be less magic, ie. just pass an interface registry and proto decodes everything, no need to do micro-format deserialization.

For users querying txs, having the typeURL in proto JSON would also help (no need to count indices in msgs and responses arrays).

Also side note, I'm not sure who is even using this response data but I think we should provide an easy way to retrieve it and we don't.

When querying for txs, we return the TxResponse type, which just shows data as a string, representing the concantenated response bytes of all msgs:

cosmos-sdk/proto/cosmos/base/abci/v1beta1/abci.proto

Line 24 in 3f2570f

string data = 5;

IMO we could replace that line with:

    TxMsgData data = 5;

where

message TxMsgData {
  repeated google.protobuf.Any msg_responses = 1;
}

If we switch to Anys, we should incorporate that in the middleware interfaces: #10484

[aaronc]

I think I still prefer to use an Any though: it would be less magic, ie. just pass an interface registry and proto decodes everything, no need to do micro-format deserialization.

For users querying txs, having the typeURL in proto JSON would also help (no need to count indices in msgs and responses arrays).

I guess the main downside is more storage space gets used up... but maybe that's not too big a deal?

[amaury1093]

Currently the way we're working towards in #10527 is to have the ABCI data field be the proto-bytes of:

message TxMsgData {
  repeated google.protobuf.Any msg_responses = 2; // field 1 is deprecated and never populated in the SDK.
}

However, if we want to decode this, then this Any needs to have its own interface, so that the registry can call RegisterInterface. Module developers also need to call RegisterImplementations on all their Msg*Response types with this interface.

Any thoughts on this? Is that too much burden for module devs? I propose just an empty tx.MsgResponse interface inside types/tx, which is:

type MsgResponse interface {
  proto.Message
}

The alternative is to introduce some micro-format serialization.

[nickray]

Also, the type url refers to the MsgRequest and not MsgResponse, which is a further deviation from standard behaviour, IDK if intentional, but it is misleading: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/run_msgs.go#L131 <- we call type URL on the request and not response.

I just ran into this, and it's rather annoying. It means a generic response decoder (in my case, a Typescript decoder in a static web app using gRPC-Web) can't rely on the decoded MsgData to decode the response data, but has to separately keep track of the mapping of request->response types.

We don't need to include the msg type URL at all because we know that from the tx.

I consider sending the type of the request with the data of the response a bug, no?

[amaury1093]

Hey! This is actually fixed since #10527.

So now the data field contains a struct containing an array of MsgResponses as Anys:

cosmos-sdk/baseapp/abci.go

Lines 877 to 880 in 6d1525f

	// makeABCIData generates the Data field to be sent to ABCI Check/DeliverTx.
	func makeABCIData(txRes tx.Response) ([]byte, error) {
	return proto.Marshal(&sdk.TxMsgData{MsgResponses: txRes.MsgResponses})
	}

cosmos-sdk/proto/cosmos/base/abci/v1beta1/abci.proto

Lines 128 to 140 in 6d1525f

	// TxMsgData defines a list of MsgData. A transaction will have a MsgData object
	// for each message.
	message TxMsgData {
	option (gogoproto.stringer) = true;
	// data field is deprecated and not populated.
	repeated MsgData data = 1 [deprecated = true];
	// msg_responses contains the Msg handler responses packed into Anys.
	//
	// Since: cosmos-sdk 0.46
	repeated google.protobuf.Any msg_responses = 2;
	}

Your TS implementation can just simply decode the TxMsgData using protobuf and inside you should see MsgResponses.

I'll close this, LMK if you still have questions.