Skip to content
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

Investigate different channel types for ibc_channel_open #888

Closed
ethanfrey opened this issue Apr 19, 2021 · 33 comments
Closed

Investigate different channel types for ibc_channel_open #888

ethanfrey opened this issue Apr 19, 2021 · 33 comments
Assignees
Milestone

Comments

@ethanfrey
Copy link
Member

Follow up from #884 (comment)

In ChannelOpenInit both counterparty_version and counterparty.channel_id are unset. We handle the first with an Option, the second ends up as a blank string.

counterparty_version is only set in ChannelOpenTry and ChannelOpenAck callbacks, and unset everywhere else (including queries), so an Option makes good sense there.

counterparty.channel_id is only unset in ChannelOpenInit (half of our ibc_connect_open) and making it an Option would complicate a lot. One idea was a special type used only for ibc_connect_open that has Option on the counterparty.channel_id. Another was custom entry points for the 4 phases in the handshake (which leads to more clarity, but a lot more code duplication and posibility of errors in contract code).

Other ideas?

@ethanfrey ethanfrey mentioned this issue Apr 19, 2021
@ethanfrey ethanfrey modified the milestones: 0.14.0, 1.0.0 Apr 19, 2021
@webmaster128
Copy link
Member

So the entry point ibc_channel_open is called for any of the two steps ChanOpenInit, ChanOpenTry, right?

In this case I suggest an enum argument to ibc_channel_open can can have things like IbcChannel or IbcEstablishingChannel depending on where we are. This also allows the contract developer to better reason about where which pase to expect. It could then decide to error for IbcChanOpen::Init and only support IbcChanOpen::Try.

Once you have such enums, you also also think about merging all 4 into one entry point.

@ethanfrey
Copy link
Member Author

Once you have such enums, you also also think about merging all 4 into one entry point.

The second ones can trigger external actions, the first step not.
It is also confusing, currently you get each one called once in the handshake.

In this case I suggest an enum argument to ibc_channel_open can can have things like IbcChannel or IbcEstablishingChannel depending on where we are. This also allows the contract developer to better reason about where which pase to expect. It could then decide to error for IbcChanOpen::Init and only support IbcChanOpen::Try.

This is a nice idea.

@webmaster128 webmaster128 self-assigned this Jun 23, 2021
@webmaster128 webmaster128 removed their assignment Jul 13, 2021
@ethanfrey
Copy link
Member Author

Gathering data here.

In CosmWasm, We use IbcChannel as input to ibc_channel_open, ibc_channel_connect, ibc_channel_close, which are part of the channel lifecycle. We also return it from queries of IbcQuery::ListChanels and IbcQuery::Channel.

However, while the same Go/Protobuf struct is used in all these places, it seems there are some subtle differences of null-ability. In one place, you are guaranteed a field to be present, in another guaranteed to be absent. This is trickier with Go, as they will pass "" for a missing identifier, not nil / None.

There are 2 questions:

  • Do we need to make the existence explicit (Option<String>) or do we just use String and assume the user will use .is_empty() to check if this is actual data?
  • Do we need different data types for the different calls representing the fields we are sure are Some/None?

I will follow up with the matrix of what data is defined where to help answer those questions

@ethanfrey
Copy link
Member Author

ethanfrey commented Jul 14, 2021

Channel Lifecycle:

CosmWasm has simplified the ChannelLifecycle callbacks from 6 to 3. You can see them in the full ICS04 spec. Our simplification:

  • ibc_channel_open: ChanOpenInit, ChanOpenTry
  • ibc_channel_connect: ChanOpenAck, ChanOpenConfirm
  • ibc_channel_close: ChanCloseInit, ChanCloseConfirm

Each side of the connection will receive exactly 2 calls on creation and 1 on closing (corresponsing to the cosmwasm entry points). The difference between the 2 ICS calls is the order (is this side A or side B of the connection doing the first step of the handshake, for example).

ICS Call l CounterParty ChannelID Version CounterParty Version
OpenInit None Some None
OpenTry Some Some Some
OpenAck Some Some Some
OpenConfirm Some Some None
CloseInit Some Some None
CloseConfirm Some Some None
Query Some Some None

(from https://github.com/cosmos/cosmos-sdk/blob/v0.42.7/x/ibc/core/04-channel/keeper/handshake.go)

Note that after the handshake, the recorded version on both sides is the version set in OpenTry (shown as counterparty_version in OpenAck). Thus, after OpenConfirm and in all queries, there is no meaning to this field.

counterparty.channel_id is present everywhere except the very first call.

@ethanfrey
Copy link
Member Author

My proposal is this:

  1. counterparty.channel_id remains String. We can advise in docs to use .is_empty() before checking it in ibc_channel_open. The reality is the use cases I have seen only reference this in ibc_channel_connect, once we are sure the channel is created.
  2. Remove pub counterparty_version: Option<String> from IbcChannel.
  3. Add pub counterparty_version: Option<String> as an argument to IbcChannelOpenMsg and IbcChannelConnectMsg. The channel only has one version after handshake, so other uses (saving, in close, in queries) do not need this extra field).

In addition, this is an optional suggestion to get feedback:

  1. Add one more field to IbcChannelOpenMsg, IbcChannelConnectMsg, and IbcChannelCloseMsg. pub initiator: bool. This can be used to determine if the contract is side A or B. This is useful if it happens to need to differentiate between OpenInit and OpenTry for example. If we do this, we can recommend checking this rather than counterparty.channel_id.is_empty() in ibc_channel_open.

These updates should be made to the IBC.md docs as well as the code itself and the contracts.

@ethanfrey
Copy link
Member Author

@webmaster128 I would love your feedback here

@uint uint self-assigned this Jul 14, 2021
@webmaster128 webmaster128 changed the title Investigate different channel types for ibc_connect_open Investigate different channel types for ibc_channel_open Jul 14, 2021
@webmaster128
Copy link
Member

webmaster128 commented Jul 14, 2021

Sounds like a great start. Not sure to what degree #888 (comment) is still useful then.

Add pub counterparty_version: Option<String> as an argument to IbcChannelOpenMsg and IbcChannelConnectMsg. The channel only has one version after handshake, so other uses (saving, in close, in queries) do not need this extra field).

Great to free the IbcChannel from details that are only relevant during the handshake. Does this apply to both counterparty_version and counterparty_endpoint?

@uint
Copy link
Contributor

uint commented Jul 15, 2021

  1. Add one more field to IbcChannelOpenMsg, IbcChannelConnectMsg, and IbcChannelCloseMsg. pub initiator: bool. This can be used to determine if the contract is side A or B. This is useful if it happens to need to differentiate between OpenInit and OpenTry for example. If we do this, we can recommend checking this rather than counterparty.channel_id.is_empty() in ibc_channel_open.

I'm not sure if I wouldn't prefer the initiator/non-initiator distinction to be about separate types or enum variants somehow. But I like the idea of checking for something meaningful like initiator rather than whether the channel_id is empty.

@uint
Copy link
Contributor

uint commented Jul 15, 2021

Something like this?

pub struct IbcChannel {
    // other fields
    pub connection_id: ConnectionID,
}

impl IbcChannel {
    pub fn initialized(&self) -> bool {
        match self.connection_id {
            ConnectionID::Uninit => false,
            _ => true,
        }
    }
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub enum ConnectionID {
    Uninit,
    ID(String),
}

@webmaster128
Copy link
Member

webmaster128 commented Jul 15, 2021

@uint isnt this basically the same as just

impl IbcChannel {
    pub fn initialized(&self) -> bool {
        !self.counterparty_endpoint.channel_id.is_empty()
    }
}

ConnectionID above seems to be equivalent to Option<String> and the question remains of it is worth encoding the empty string from Go as a None Option<String> or just avoid trying to be clever and use whatever Go provides us.

@webmaster128
Copy link
Member

Does this apply to both counterparty_version and counterparty_endpoint?

Looking at the Go code, I get the feeling that IbcChannel::counterparty_endpoint is an important, long term information for the channel and should remain where it is. Version seems to be the same one on both sides after the handshake, such that IbcChannel::version is sufficient.

@uint
Copy link
Contributor

uint commented Jul 15, 2021

@uint isnt this basically the same as just

impl IbcChannel {
    pub fn initialized(&self) -> bool {
        !self.counterparty_endpoint.channel_id.is_empty()
    }
}

ConnectionID above seems to be equivalent to Option<String> and the question remains of it is worth encoding the empty string from Go as a None Option<String> or just avoid trying to be clever and use whatever Go provides us.

The difference is ConnectionID has some specific meaning and isn't generic, but I could see Option being used here for convenience. I think encoding those things in idiomatic Rust types makes the API nicer for Rust programmers. I feel really weird about equating the fact a string is empty with what type of data we're dealing with.

@uint
Copy link
Contributor

uint commented Jul 15, 2021

The difference is ConnectionID has some specific meaning and isn't generic, but I could see Option being used here for convenience. I think encoding those things in idiomatic Rust types makes the API nicer for Rust programmers. I feel really weird about equating the fact a string is empty with what type of data we're dealing with.

Then again, I guess if it's empty strings from Go anyway... yeah, it's not such a strong argument.

@webmaster128
Copy link
Member

The difference is ConnectionID has some specific meaning and isn't generic, but I could see Option being used here for convenience.

The advantage of Option<String> is a much simpler, compact JSON representation as null | string. ConnectionID would end up as a nested object with Uninit/ID becoming object fields.

@ethanfrey
Copy link
Member Author

I think this boolean would be an alternate version of the enum arg you propose. And simpler.

Great to free the IbcChannel from details that are only relevant during the handshake. Does this apply to both counterparty_version and counterparty_endpoint?

counterparty_version is just in the handshake.
counterparty_endpoint is always available, except in the very first step (ibc_connect_open for the initiator, where there is no knowledge of the other side yet)

@ethanfrey
Copy link
Member Author

ethanfrey commented Jul 15, 2021

The fact that 99% of the usage of the counterparty_endpoint.channel_id it is Some(x) to me says making it Option<String> would just be annoying. Adding someway to see if it is valid is nice.

impl IbcChannel {
    pub fn initialized(&self) -> bool {
        !self.counterparty_endpoint.channel_id.is_empty()
    }
}

Is one idea and not bad. The initiator field can be useful for other steps as well in the handshake where we don't have the field differentiate between A and B. Happy to use some enum there rather than a bool:

pub struct IbcChannelOpenMsg {
  pub channel: IbcChannel,
  pub side: HandshakeSide,
}

pub enum HandshakeSide {
  A,
  B,
}

If you find this cleaner than a boolean

@uint
Copy link
Contributor

uint commented Jul 15, 2021

I think this boolean would be an alternate version of the enum arg you propose. And simpler.

My issue is we then have a situation similar to how Go handles errors - a string always exists, but the user has to remember to check another value to even know if the string is meaningful. The enum is nicer since if the thing is None/Uninit, there is no string to try using in the first place.

@ethanfrey
Copy link
Member Author

The enum is nicer since if the thing is None/Uninit, there is no string to try using in the first place.

"Nicer" should be judged by the users (ibc contracts). I never once try to use this field in ibc_connect_open. If we make it an Option<String>, that is set everywhere, how do we handle in code?

channel.counterparty_endpoint.channel_id.unwrap()

channel.counterparty_endpoint.channel_id.ok_or_else(|| StdError::generic_err("counterparty channel_id not defined"))?

@uint
Copy link
Contributor

uint commented Jul 15, 2021

"Nicer" should be judged by the users (ibc contracts). I never once try to use this field in ibc_connect_open. If we make it an Option<String>, that is set everywhere, how do we handle in code?

channel.counterparty_endpoint.channel_id.unwrap()

channel.counterparty_endpoint.channel_id.ok_or_else(|| StdError::generic_err("counterparty channel_id not defined"))?

Possibly a getter that returns an StdResult, used like channel.channel_id()?.

I was wondering about creating a separate channel type called IbcMaybeUninitChannel that's only used for ibc_channel_open. This one would have an Option<String> while IbcChannel would have a String. I think that could keep things from getting cumbersome in a lot of cases.

@ethanfrey
Copy link
Member Author

I was wondering about creating a separate channel type called IbcMaybeUninitChannel that's only used for ibc_channel_open. This one would have an Option while IbcChannel would have a String. I think that could keep things from getting cumbersome in a lot of cases.

I would definitely prefer this to making the Option everywhere IbcChannel is used.
Maybe it is overkill, but if you both feel it is important to make this an option, let's just use a new type for the IbcChannelOpenMsg to hold the Option.

@ethanfrey
Copy link
Member Author

At that point, I wouldn't worry about making it "easy" to unwrap. People will actually want to switch on both cases explicitly

@webmaster128
Copy link
Member

but if you both feel it is important to make this an option

I don't. I think we already lost when trying to model 6 different cases in 3 calls. From there, I don't see how we can get back the type safety that is desired in Rust. In #888 (comment) I propose getting back the 6 distinct cases (through 3 entry points), but I think it is fair to consider this overkill. Since I don't have any experience using this, I don't know what is right or wrong. Also no strong oponion how we continue from here.

@ethanfrey
Copy link
Member Author

I propose getting back the 6 distinct cases (through 3 entry points)

I agree with this. I would use initiator or side field rather than different args. But happy for other ideas. I would only use Option in calls where it really is Option.

Beyond this minor point, I think some people will care to differentiate between the two calls that we merge into one and adding some bool / enum should be included somehow

@uint
Copy link
Contributor

uint commented Jul 15, 2021

I don't. I think we already lost when trying to model 6 different cases in 3 calls. From there, I don't see how we can get back the type safety that is desired in Rust. In #888 (comment) I propose getting back the 6 distinct cases (through 3 entry points), but I think it is fair to consider this overkill.

Looking back at that, that sounds like a great idea. But yeah, I know little and don't write those contracts.

@ethanfrey
Copy link
Member Author

I would like to re-iterate my concrete proposal: #888 (comment)

There has been a lot of discussion since then, but I am unclear if those points are accepted? Let's make clear what is agreed and what is open.

It seems (2) and (3) are agreed up.
I got no direct feedback on (4) but I do like it and would like a 👍 / 👎 here.
All discussions are around (1), right?

If so, I would make a new discussion about that point, and then we can focus on those changes we do agree on (2), (3), (4?)

@webmaster128
Copy link
Member

webmaster128 commented Jul 15, 2021

My idea is this (example): ibc_channel_open is the entry point to handle both ChanOpenInit and ChanOpenTry. Now IbcChannelOpenMsg can become an enum (same as we do with the custom execute messages) like this:

enum IbcChannelOpenMsg {
    Init {
        channel: IbcChannelInit,
    },
    Try {
        channel: IbcChannel,
        counterparty_version: String,
    }
}

/// Like IbcChannel but without counterparty_endpoint.channel_id
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct IbcChannelInit {
    pub endpoint: IbcEndpoint,
    pub counterparty_endpoint: IbcEndpointInit, // Different type here. But do we even have/need port_id in the init case?
    pub order: IbcOrder,
    pub version: String,
    /// The connection upon which this channel was created. If this is a multi-hop
    /// channel, we only expose the first hop.
    pub connection_id: String,
}

/// Like IbcEndpoint but without channel_id
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct IbcEndpointInit {
    pub port_id: String,
    // no channel_id
}

With that the contract developer switches over msg: IbcChannelOpenMsg once and has all type safety from there.

@uint
Copy link
Contributor

uint commented Jul 15, 2021

My idea is this (example): ibc_channel_open is the entry point to handle both ChanOpenInit and ChanOpenTry. Now IbcChannelOpenMsg can become an enum (same as we do with the custom execute messages) like this:

I'm strongly in favor of something like that. It's clean and we have the notion of "what sort of operation this is" encoded in the type system rather than checked purely at runtime - that's very much Rust's philosophy. For each type of operation the user only gets the data that's meaningful for it - less room for error and clutter.

It also looks like it maps more directly and explicitly to concepts users might know from the IBC spec (not that I've read it) - as in different types of operations. Right?

@uint
Copy link
Contributor

uint commented Jul 15, 2021

All discussions are around (1), right?

I feel like we're discussing the direction of the whole thing more broadly at this point, and @webmaster128's proposal affects both 1 and 4.

@uint
Copy link
Contributor

uint commented Jul 15, 2021

My idea is this (example): ibc_channel_open is the entry point to handle both ChanOpenInit and ChanOpenTry. Now IbcChannelOpenMsg can become an enum (same as we do with the custom execute messages) like this:

Ah, I guess one annoyance here would be if you want to create an entrypoint that doesn't distinguish between init and try, you'd still have to match and write double the boilerplate to extract fields that interest you.

pub fn ibc_channel_open(deps: DepsMut, env: Env, msg: IbcChannelOpenMsg) -> StdResult<()> {
    let endpoint;
    let version;
    
    // yuck!
    match msg {
        IbcChannelOpenMsg::Init(channel) => {
            endpoint = channel.endpoint;
            version = channel.version;
        }
        IbcChannelOpenMsg::Try(channel, _) => {
            endpoint = channel.endpoint;
            version = channel.version;
        }
    }

    // real code starts here
}

@ethanfrey
Copy link
Member Author

Ah, I guess one annoyance here would be if you want to create an entrypoint that doesn't distinguish between init and try, you'd still have to match and write double the boilerplate to extract fields that interest you.

I think we need to look at the 3 actual example usages here, as you are making an API that is expressive and safe, but I feel difficult to use.

pub fn ibc_channel_open(_deps: DepsMut, _env: Env, msg: IbcChannelOpenMsg) -> StdResult<()> {
    let channel = msg.channel;

    if channel.order != IbcOrder::Ordered {
        return Err(StdError::generic_err("Only supports ordered channels"));
    }
    if channel.version.as_str() != IBC_VERSION {
        return Err(StdError::generic_err(format!(
            "Must set version to `{}`",
            IBC_VERSION
        )));
    }
    // TODO: do we need to check counterparty version as well?
    // This flow needs to be well documented
    if let Some(counter_version) = channel.counterparty_version {
        if counter_version.as_str() != IBC_VERSION {
            return Err(StdError::generic_err(format!(
                "Counterparty version must be `{}`",
                IBC_VERSION
            )));
        }
    }

    Ok(())
}

https://github.com/CosmWasm/cosmwasm/blob/main/contracts/ibc-reflect/src/contract.rs#L127-L153

pub fn ibc_channel_open(_deps: DepsMut, _env: Env, msg: IbcChannelOpenMsg) -> StdResult<()> {
    let channel = msg.channel;

    if channel.order != IbcOrder::Ordered {
        return Err(StdError::generic_err("Only supports ordered channels"));
    }
    if channel.version.as_str() != IBC_VERSION {
        return Err(StdError::generic_err(format!(
            "Must set version to `{}`",
            IBC_VERSION
        )));
    }
    // TODO: do we need to check counterparty version as well?
    // This flow needs to be well documented
    if let Some(counter_version) = channel.counterparty_version {
        if counter_version.as_str() != IBC_VERSION {
            return Err(StdError::generic_err(format!(
                "Counterparty version must be `{}`",
                IBC_VERSION
            )));
        }
    }

    Ok(())
}

https://github.com/CosmWasm/cosmwasm/blob/main/contracts/ibc-reflect-send/src/ibc.rs#L20-L44

pub fn ibc_channel_open(
    _deps: DepsMut,
    _env: Env,
    channel: IbcChannel,
) -> Result<(), ContractError> {
    enforce_order_and_version(&channel)?;
    Ok(())
}

fn enforce_order_and_version(channel: &IbcChannel) -> Result<(), ContractError> {
    if channel.version != ICS20_VERSION {
        return Err(ContractError::InvalidIbcVersion {
            version: channel.version.clone(),
        });
    }
    if let Some(version) = &channel.counterparty_version {
        if version != ICS20_VERSION {
            return Err(ContractError::InvalidIbcVersion {
                version: version.clone(),
            });
        }
    }
    if channel.order != ICS20_ORDERING {
        return Err(ContractError::OnlyOrderedChannel {});
    }
    Ok(())
}

https://github.com/CosmWasm/cosmwasm-plus/blob/main/contracts/cw20-ics20/src/ibc.rs#L76-L83

While there is always an if around counterparty_version (which we will keep a match or so for), there is a lot of code that uses IbcChannel and will be annoying to copy. Note than none refer to the counterparty_endpoint.channel_id

@ethanfrey
Copy link
Member Author

Also, my (4) is not just about ibc_channel_open, but about the other entry points. Which actually have the same args on both sides, and we can represent the Ack/Confirm distinction without needing such a heavy enum.

I also want to mention that a large amount of code size and gas cost is JSON structs. And the struct here: #888 (comment) will lead to lots of (almost) duplicate generated code.

@webmaster128
Copy link
Member

Yeah, but there are ways to make this nicer:

pub fn ibc_channel_open(deps: DepsMut, env: Env, msg: IbcChannelOpenMsg) -> StdResult<()> {
    let (endpoint, version) = match msg {
        IbcChannelOpenMsg::Init { channel } => {
            (channel.endpoint, channel.version)
        }
        IbcChannelOpenMsg::Try { channel, ..  } => {
            (channel.endpoint, channel.version)
        }
    }

    // real code starts here
}

@ethanfrey
Copy link
Member Author

We decided NOT to worry about this issue and stick with the empty string in that one location.

All the improvements mentioned here that we do want have been extracted to #1010 along with discussion how to acheive that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants