Remove non_exhaustive from IBC message types.

[0xekez]

IBC message types being non exhaustive means that contract authors have to include a wildcard arm in any match expression deailing with them. IMO, this is actually quite undesirable. I my contract to fail to compile if a message is added!

[webmaster128]

I my contract to fail to compile if a message is added!

That's a noble approach. Better safe than sorry. However, it would require a major version bump of cosmwasm-std just because a new enum case is added. This is what we want to avoid with all the non_exhaustives.

[0xekez]

@webmaster128: However, it would require a major version bump of cosmwasm-std just because a new enum case is added.

I trust that you have more experience with this stuff than I do. :) I do wonder what the odds of a new message being added to the channel establishment or teardown process is? my guess is, that considered, the expected value of this may be positive. also, adding a new case would break all existing IBC contracts that are deployed. perhaps that is worth a major version bump?

if we do decide to not merge this it's not a big deal. just need to be very careful bumping CosmWasm versions in IBC contracts. :)

[webmaster128]

@ethanfrey what do you think about removing non_exhaustive from those two places? Do we expect new cases here any time soon?

[ethanfrey]

I agree completely with removing the non_exhaustive flags here. I think there was a bit over eager tagging.

I would also remove it here IbcChannelConnectMsg

That said, I think CosmosMsg::Ibc(IbcMsg) type will.need non exhaustive and is very likely to receive additional types in 1.x

[webmaster128]

Good, thanks. Yeah, the issue with non_exhaustive are the cases where the value is coming from the environment and the contract dev has to process them. For all messages, the contract dev creates them and there is no reason to match them.

@0xekez I tried the change in an IBC contract it seems like this is non breaking as it would just create a compiler warning for the fallback matche arm:

warning: unreachable pattern
   --> contracts/nois-proxy/src/contract.rs:193:9
    |
193 |         _ => panic!("What should I do?"),
    |         ^
    |
    = note: `#[warn(unreachable_patterns)]` on by default

Out of curiosity, I wonder why you need the match at all. With msg.channel() you can get the channel and msg.counterparty_version() gives you the counterparty. Is the actual enum case important in your code? Do you have code to share?

[0xekez]

Here's a specific example of where #[non_exhaustive] is trouble:

#[cfg_attr(not(feature = "library"), entry_point)]
pub fn ibc_channel_close(
    _deps: DepsMut,
    _env: Env,
    msg: IbcChannelCloseMsg,
) -> Result<IbcBasicResponse, ContractError> {
    match msg {
        // Error any TX that would cause the channel to close that is
        // coming from the local chain.
        IbcChannelCloseMsg::CloseInit { channel: _ } => Err(ContractError::CantCloseChannel {}),
        // If the close is coming from the other chain, we must allow
        // the TX to pass. Otherwise, our chain will believe the
        // channel to be open, while our counterparty will believe the
        // channel to be closed.
        IbcChannelCloseMsg::CloseConfirm { channel: _ } => Ok(IbcBasicResponse::default()),
        _ => unreachable!("channel can not be closed"),
    }
}

We need to match on the close message type as the contract should do something different depending on if this is a "request to close" from the local chain, or a "statement that the channel has been closed" from the counterparty chain. Channel closing is a really dangerous part of these contracts because if you fail the TX which confirms the close on your side, the chains will be out of sync where one side believes the channel closed and the other does not.

Here's a test that explores some of the complexities of channels closing.

[0xekez]

Rebased and removed #[non_exhaustive] from IbcChannelConnectMsg.

[webmaster128]

Merci, thanks for sharing. Will merge next week.

[webmaster128]

Shipped as part of 1.1.5