Add IbcSend message type to stargate code #710

ethanfrey · 2021-01-12T19:26:12Z

Expose simple message to allow contracts to send native tokens to other chains

webmaster128 · 2021-01-13T09:25:27Z

packages/std/src/results/cosmos_msg.rs

    /// A Stargate message encoded the same way as a protobof [Any](https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/any.proto).
    /// This is the same structure as messages in `TxBody` from [ADR-020](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-020-protobuf-transaction-encoding.md)
    #[cfg(feature = "stargate")]
    Stargate {
        type_url: String,
        data: Binary,
    },
-    Wasm(WasmMsg),
+    /// Sends *native tokens* owned by the contract to the given address on another chain.


What mean "native token" exactly? A bank token of the contract's chain?

good 👁️ . This can be a bank token but also a ibc voucher or anything else that got minted on the sdk mint module.

Yes. As opposed to a cw20 token.

I will use the term bank token (others in cosmos used native token)

webmaster128 · 2021-01-13T09:26:03Z

packages/std/src/results/cosmos_msg.rs

+    /// We cannot select the port_id, this is whatever the local chain has bound the ibctransfer
+    /// module to.
+    #[cfg(feature = "stargate")]
+    IbcSend {


Can we pull this out into a IbcMsg enum, which groups all IBC messages?

Can we rename this to Ics20Transfer and link https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer?

Sure, I will rebase on another PR that adds IBC message types.

My issue was that the other ones (SendPacket and CloseChannel) only are valid for contracts that actively participate in ibc channels (and have 6 entry points for that). This one could be used by any contract.

I guess I can make that distinction with docstrings as well

agree that is a transfer is a better naming. it is used in the sdk module as well as on the cli.

My issue was that the other ones (SendPacket and CloseChannel) only are valid for contracts that actively participate in ibc channels (and have 6 entry points for that). This one could be used by any contract.

I guess I can make that distinction with docstrings as well

Yeah, I think this way would be better.

I guess this would be reflected in the necessary fiels as well (the other messages would require more fields that include all this channel state information)?

webmaster128 · 2021-01-13T09:29:44Z

packages/std/src/query.rs

@@ -24,7 +25,6 @@ pub enum QueryRequest<C: CustomQuery> {
        /// this is the expected protobuf message type (not any), binary encoded
        data: Binary,
    },
-    Wasm(WasmQuery),


What's the idea behind this move? Before it was alphabetically. I'm happy either way but I'd like to understand how you want to order this.

I left it alphabetic for the standard ones, then all the stargate ones below (under feature flag). So sorting (is_stargate, name).

Maybe that is not very good idea, as staking is also under a different feature flag. I can make it alphabetical again

I'd either use plain alphabetic order or introduce ordered groups that are separated by an empty line

alpe · 2021-01-13T10:53:58Z

packages/std/src/results/cosmos_msg.rs

    /// A Stargate message encoded the same way as a protobof [Any](https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/any.proto).
    /// This is the same structure as messages in `TxBody` from [ADR-020](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-020-protobuf-transaction-encoding.md)
    #[cfg(feature = "stargate")]
    Stargate {
        type_url: String,
        data: Binary,
    },
-    Wasm(WasmMsg),
+    /// Sends *native tokens* owned by the contract to the given address on another chain.


good 👁️ . This can be a bank token but also a ibc voucher or anything else that got minted on the sdk mint module.

alpe · 2021-01-13T10:57:04Z

packages/std/src/results/cosmos_msg.rs

+    /// We cannot select the port_id, this is whatever the local chain has bound the ibctransfer
+    /// module to.
+    #[cfg(feature = "stargate")]
+    IbcSend {


agree that is a transfer is a better naming. it is used in the sdk module as well as on the cli.

alpe · 2021-01-13T11:03:19Z

packages/std/src/results/cosmos_msg.rs

+        /// packet data only supports one coin
+        /// https://github.com/cosmos/cosmos-sdk/blob/master/proto/ibc/applications/transfer/v1/transfer.proto#L11-L20
+        amount: Coin,
+    },


Can we have an optional field port_id:String?
The default should be transfer but this can be set in the genesis for every chain

Optinal fields are not super convenient in Rust (you cannot omit them like in JavaScript/Go). Instead you always have to write them either port: Some(value) or port: None.

Would it be an option to store this as a constant in the chain code, such that it is added when the Rust type is translated to a Cosmos SDK type?

Can we have an optional field port_id:String?
The default should be transfer but this can be set in the genesis for every chain

This detail is super important for the Go module handling the transfers. There should be one official IbcTransfer port for the application, which may be name transfer or ibctransfer or foobar or wasm12334556775fg4g43tuhti44tt4yt74yt47.

The contract should run the same on any chain, regardless of how genesis was set up. By just sending a token, we leave the port naming up to the wasmd-based app. The channel is needed to differentiate various chains and should be set by the calling user.

Does that make sense? Setting a port to anything other than what the ibctransfer module was bound to will lead to an error, so let's just set it for them, as there is only one "right" value

ethanfrey · 2021-01-13T12:35:11Z

Okay, I rebased this on #711 and then simplified it from the comments.
Can you please re-review?

webmaster128

LGTM

ethanfrey requested a review from webmaster128 January 13, 2021 09:20

webmaster128 reviewed Jan 13, 2021

View reviewed changes

alpe reviewed Jan 13, 2021

View reviewed changes

Add Ics20Transfer to IbcMsg

b8acfed

ethanfrey force-pushed the ibc-transfer-msg branch from 58d33dd to b8acfed Compare January 13, 2021 12:34

ethanfrey changed the base branch from master to ibc-entry-points January 13, 2021 12:34

Ensure stargate code is tested in the CI

baf6f9c

webmaster128 approved these changes Jan 13, 2021

View reviewed changes

ethanfrey merged commit 942f389 into ibc-entry-points Jan 13, 2021

ethanfrey deleted the ibc-transfer-msg branch January 13, 2021 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IbcSend message type to stargate code #710

Add IbcSend message type to stargate code #710

ethanfrey commented Jan 12, 2021

webmaster128 Jan 13, 2021

alpe Jan 13, 2021

ethanfrey Jan 13, 2021

webmaster128 Jan 13, 2021

ethanfrey Jan 13, 2021

alpe Jan 13, 2021

webmaster128 Jan 13, 2021

webmaster128 Jan 13, 2021

ethanfrey Jan 13, 2021

webmaster128 Jan 13, 2021

alpe Jan 13, 2021

alpe Jan 13, 2021

alpe Jan 13, 2021

webmaster128 Jan 13, 2021

ethanfrey Jan 13, 2021

ethanfrey commented Jan 13, 2021

webmaster128 left a comment

Add IbcSend message type to stargate code #710

Add IbcSend message type to stargate code #710

Conversation

ethanfrey commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Jan 13, 2021

webmaster128 left a comment

Choose a reason for hiding this comment