Support json tx encoding for interchain accounts

womensrights commented 1 year ago

Summary

Currently interchain accounts only has protobuf encoding, json encoding would give more flexibility to using the feature, particularly for CosmWasm and Solidity smart contracts.

Problem Definition

Adding json encoding would improve ICA composability, the feature is currently limited to only protobuf encoding

Proposal

Add json encoding for ICA messages

For Admin Use

[ ] Not duplicate issue
[ ] Appropriate labels applied
[ ] Appropriate contributors tagged/assigned

colin-axner commented 1 year ago

Is there an example user flow? Trying to understand whether this is a helper function or for decoding on the host? I'm not sure I follow when this would need to be rlp encoded on the host side within ibc-go

colin-axner commented 1 year ago

Revisiting, here's how to implement this issue for json. Please implement json and rlp support in separate prs.

User flow

json_encode(tx) -> give to controller -> ibc send, relay -> host recv -> decode_json(tx) into []sdk.Msg -> execute tx

json_encoding code will be external to ibc-go (potentially a wasm contract)

Implementation

Add EncodingJSON to metadata.go in the same format as EncodingProtobuf

Add EncodingJSON to the list of supported encodings

Modify SerializeCosmosTx/DeserializeCosmosTx to add an extra parameter for the encoding type (this is an API breaking change; we'll need to deal with it in the backport to v7.1.x by keeping the signature and adding an extra function). It should switch on the encoding type, something similar to:

// DeserializeCosmosTx, pass in full packetData
// *codec.ProtoCodec implements the JSONCodec interface

if _, ok := cdc.(*codec.ProtoCodec); !ok {
        return nil, errorsmod.Wrap(ErrInvalidCodec, "provided codec is not of type %T", codec.ProtoCodec{})
}

var cosmosTx CosmosTx

switch encoding // encoding is a parameter passed to the function
case EncodingProtobuf:
    if err := cdc.Unmarshal(data, &cosmosTx); err != nil {
        return nil, err
    }

case EncodingJSON:
    if err := cdc.UnmarshalJSON(data, &cosmosTx); err != nil {
        return nil, err
    }

default:
    return nil, errorsmod.Wrap(ErrUnsupportedEncoding, "encoding type %s is not supported", packetData.Encoding)
}

Similar changes can be made to SerializeCosmosTx

Testing

A test should be added to the host submodule which creates a channel using the JSON encoding. It should send a successful json encoded transaction (succeeds) and it should send a proto encoded transaction (fails). An e2e is not necessary in my opinion.

It would be greatly appreciated if someone could test this integration with wasm :pray:

I may have missed something so please ask questions if you run into any issues

crodriguezvega commented 1 year ago

Thanks for the write-up, @colin-axner! I would like to add a couple of things and hear your opinion.

The InterchainAccountPacketData does not contain at the moment any field with the encoding type. Is your proposal to add it to the proto definition of the packet data? If that was the case, then each message sent to the host could potentially be encoded either in JSON or proto, and then the encoding type (which is part of the version string) agreed during the channel handshake would not be really useful (I guess could be marked deprecated). But my question would be: is reasonable to expect that messages will be encoded differently over one given channel? UPDATE: this is not what Colin meant.

If we don't add an extra field to the InterchainAccountPacketData then we could pass to SerializeCosmosTx/DeserializeCosmosTx, instead of the full packet data, an extra argument with the type of encoding. Then in OnRecvPacket on the host submodule, when the messages need to be decoded, we will need to get the encoding type somehow. Maybe there are, at least, two options:

To retrieve the version of the channel (using GetAppVersion), un-marshal it to the Metadata type and read the Encoding field. Pro is that we already have the information in state; cons is that we need to do JSON deserialisation before executing every packet data to figure out how to decode the data. UPDATE: this is the preferred option.
To store in state the encoding for each ICA channel and retrieve it in OnRecvPacket. Pro is that we don't need to do JSON deserialisation of the channel version in every ICA packet; cons is that it adds extra state.

However, SerializeCosmosTx/DeserializeCosmosTx are both public functions and changing their signature (either to take the full packet data or to add an extra argument for encoding type) will break an API-breaking change, right? Unless we do a work around and create new functions with the new signature and leave these as is.

Maybe we can also make the encoding type an enum, at least in the Go code, but still use a string in the Metadata type, so that we don't break the API here as well.

We also have this helper CLI command on the host submodule to generate the packet data that I think we should also adapt so that it takes an extra parameter for the encoding type. UPDATE: instead of adding an extra argument to the CLI, it would be possible to retrieve the encoding from the version of the channel end.

webmaster128 commented 1 year ago

Please note that there is no specification for how Cosmos SDK transactions are serialized to JSON. It is defined by the Cosmos SDK implementation based on the protobuf JSON specs plus a bunch of extra rules that may or may not be consistent. I.e. the JSON serialization can change any time. I would not recommend to assume chain 1 and chain 2 use the same JSON codec as long as such a spec does not exist.

aaronc commented 1 year ago

Please note that there is no specification for how Cosmos SDK transactions are serialized to JSON. It is defined by the Cosmos SDK implementation based on the protobuf JSON specs plus a bunch of extra rules that may or may not be consistent. I.e. the JSON serialization can change any time. I would not recommend to assume chain 1 and chain 2 use the same JSON codec as long as such a spec does not exist.

The SDK does not apply any extra rules to proto JSON serialization. The only deviation that might be present relates to how some legacy amino types marshal json which (if they still exist) I would basically consider bugs at this point. We attempt to follow the official proto3 spec to the letter, but of course that spec could change. Also in the future we will use the official protoreflect based JSON serializer so there should be no legacy amino glitches with that. Note, however, that the official serializer is intentionally non-deterministic because they want to reserve the right to make breaking changes... So yes, it's generally not a great choice

webmaster128 commented 1 year ago

I would love to see see this become a reality. It would help independent implementations so much. But right now I often see JSON seralizations that differ from proto. E.g. the Dec cases which are integer string on the protobuf level but in JSON suddenly have a decimal point. Also embedded CosmWasm messages are bytes at protobuf level but nested objects in JSON. Is this something you consider bugs and everything should be the original protobuf JSON? What about the protobuf option system? Should it allow changes to the JSON representation or should it all become the standard once we don't need Amino JSON anymore?

0xekez commented 1 year ago

Thanks for considering adding JSON support to ICA! This would make using interchain accounts from smart contracts 100x as easy.

For the JSON encoding to use, in CosmWasm we use the one defined here (also, you can find the JSON schema for this here). From my perspective, using that would be AMAZING as we could start using ICA natively (without a wrapper) from CosmWasm.

colin-axner commented 1 year ago

Thanks everyone for the context. I have some clarifying questions just to make sure I follow the concerns being raised here. Please let me know if I have misunderstood something

As @webmaster128 points out in the first https://github.com/cosmos/ibc-go/issues/3246#issuecomment-1503171199, the SDK does not contain json schema's for every sdk.Msg nor does it enforce this practice. Instead it relies on the protobuf json mapping which as @aaronc mentions could technically change from under our feet (the official serializer reserving the right to making breaking changes).

I'm not sure I follow the followup https://github.com/cosmos/ibc-go/issues/3246#issuecomment-1509318456 though? This seems to be pointing out differences between proto and JSON, not proto JSON vs JSON?

With ICA, the primary concern is ensuring that we can decode a msg type using the encoding type negotiated at channel opening (in addition to support encoding types that connecting controllers have access to).

What is the exact concern with the lack of a specification for encoding sdk.Msgs? Is it a concern that the proto json mapping could potentially change (ie future proofing)? Is it a concern that without json schema's attached to sdk.Msg type, it is difficult for smart contract users to correctly encode a specific sdk.Msg? Please note that ICA will be expecting an Any which references to specific sdk.Msg implementation.

While no json schema explicitly exists, it does implicitly exist by referencing the proto definition + using proto json.

colin-axner commented 1 year ago

Hi @aaronc and @webmaster128, there are two proposed implementations for this issue, #3725 and #3796. If you have any further comments, they would be very much appreciated!

In further consideration, I'm not so sure we need to worry about proto JSON future compatibility only with respect to ICA transaction encode/decode. IBC packet data (such as for transfer and ICA) is already encoded/decoded using proto JSON and will be required to maintain backwards compatibility in the absence of a coordinated upgrade between both channel ends. Thus in such a world where this compatibility would break, we would need to go the extra mile regardless to ensure we maintain compatibility

colin-axner commented 1 year ago

Cool so I looked into this some more, documenting my findings here.

The purpose of this feature is to support:

wasm contracts to json encode SDK msgs -> which are relayed to another chain -> and decoded by ibc-go ica host module into sdk.Msg and then the msgs are executed via baseapp

One issue I don't see how we can get around is usage of Anys. This is because the wasm contract can send the ibc-go ica host any sdk.Msg, thus when the ica host module receives a Tx, it needs to figure out how to decode the msgs. Thus, the encoded bytes need to include self referential information (type url) so that it can look up how to decode this type so that it can be executed and routed to the correct handler. This is the purpose of the Any type, so while you could do some work around to replicate the same behaviour via the standard json encoding library, in the end you are performing the same tasks as a proto3 library

From what I've been able to tell, proto3 json differs from the standard json library in a few ways:

int64/uint64 are encoded as strings
enums are encoded as strings
handles Any type (see below quote)

If the Any contains a value that has a special JSON mapping, it will be converted as follows: {"@type": xxx, "value": yyy}. Otherwise, the value will be converted into a JSON object, and the "@type" field will be inserted to indicate the actual data type.

proto3 json spec ref

impl ref

I did some testing and it appears to me the pbjson impl can successfully unmarshal any bytes encoded by the standard json library. The reverse is not true. If you encode using proto3 json (say by using a string for a int64) the standard library will return an error

Based on this understanding, if a new encoding type is added for Proto3JSON, what this is specifying in terms of requirements for contracts, is that they encode their Anys using the format defined in the proto3 spec. They should also encode the other types (int64, uint64, enums) in the same way, but my investigation suggests that it should not cause an error if the equivalent of the standard json library is used.

The requirements set by the proto3 json spec for Anys seems reasonable to me. It clearly differentiates it as a special type and as @srdtrk has shown, there is not much overhead in the contract code to give it the special "@type" tag. In my opinion, if the handling of the Any changed, it would be the equivalent of a new EncodingType from the interchain account perspective and thus channel upgradeablility could be used to change the encoding expectations, but I would be very surprised if that standard ever changed

As a side note:

transfer uses json encoding for packet data
ica uses proto3 json (due to the enum type)

From my understanding based on @srdtrk research, the rust json library provides good flexibility in determining exactly how to encode a custom type. As shown in some code @srdtrk sent me, meeting the proto3 requirement is quite simple with the standard rust library:

/// This enum does not derive Deserialize, see issue [#1443](https://github.com/CosmWasm/cosmwasm/issues/1443)
#[derive(Serialize, Clone, Debug)]
#[serde(tag = "@type")]
pub enum CosmosMessages {
    #[serde(rename = "/cosmos.bank.v1beta1.MsgSend")]
    MsgSend {
        /// Sender's address.
        from_address: String,
        /// Recipient's address.
        to_address: String,
        /// Amount to send
        amount: Vec<Coin>,
    },
    #[serde(rename = "/cosmos.gov.v1beta1.MsgSubmitProposal")]
    MsgSubmitProposalLegacy {
        content: Box<CosmosMessages>,
        initial_deposit: Vec<Coin>,
        proposer: String,
    },
    #[serde(rename = "/cosmos.gov.v1beta1.TextProposal")]
    TextProposal { title: String, description: String },
}

#[derive(Serialize, Debug)]
pub struct CosmosTx {
    pub messages: Vec<CosmosMessages>,
}

Based on these findings, adding a Proto3JSON encoding type seems sufficiently safe in terms of future compatibility and meets to need for contract developers to easily encode ICA transactions with json. In addition it allows us to use well established proto3 libraries instead of implementing custom handling for types the standard json encoder cannot handle (Any in this case)

Please let us know if you have any concerns or comments, we will likely be moving forward with #3796 (excellent work @srdtrk)

srdtrk commented 1 year ago

I'll note that PR #3796 is end to end tested using this CosmWasm implementation of the ica controller and the e2e testing package in that repo. I've verified that the json codec we used in this PR and cosmwasm communicate as intended.

cosmos / ibc-go