Update ibc handling to accommodate ibc v3

[ethanfrey]

With the current ibc implementation, we take great care a contract never errors on ibc_recv_packet. It must also take care to roll back any state internally on error (using submessage to self if needed). Any error it returns would revert the entire transaction, rather than signal a failure in the acknowledgement.

In ibcv2, we have 4 cases to cover:

1. success
2. application failure and NO rollback
3. application error and rollback app state
4. tx error and rollback ibc state as well

(1 and 2 we handled by the current version, only difference is the interpretation of bytes in the ack data)

In https://github.com/CosmWasm/wasmvm/blob/main/lib.go#L540-L547 we unmarshall the response from the contract and consider a Result::Err from the contract the same as a VM error and convert both to a Go error. (VM error is "out of gas", "panic", "no ibc entry point" or other hard abort)

One step suggested by @alpe would be returning the complete IBCReceiveResult rather than unmarshalling it in wasmvm. That will provide more info to the caller. However, how do we handle these two cases:

3. contract returned error
4. vm returned error

Until wasmd 0.21 (on cosmos-sdk 0.42) both are handled the same and abort the transaction, which means the ibc packet is never recorded as received, so no ack can be returned to the sender. Can we do something better now?

[ethanfrey]

Personally, I would like to take advantage of the new interface. Here are the conditions we want to ensure keep working:

• If a contract returns success with []byte("xyzzy") as ack data, those exact bytes will be received by the counterpart chain to give them full control
• This should support both utf-8 (JSON) data as well as binary (protobuf) data formats to match any native protocol
• On out of gas error we should abort the transaction (return go error), so it can be retried (not recorded like the app called it a failure)

Here are some open cases to cover.

• The contract returns an error result (eg "InsufficientBalance"). It would be nice to let it return an error and have that be passed to the other side as an error. And rollback all state changes.
• The contract panics. Maybe we should consider that as above (application error) or maybe we should handle that like out of gas (abort tx)

Why do I say this? Look at the hoops we must do for a very simple handler:

• On recv, make a special function and handle all errors separately
• Ensure we catch the error case for any submessages
• And manually set the ack packet on submessage failure

And this is a simple case. If we actually changed state of the main contract in ibc_packet_recv we must then somehow roll that back in reply for the error case, as the app framework doesn't help us here.

[alpe]

I have looked at the code in wasmvm again and we actually handle 1+3. There is currently no way for a contract to revert state AND confirm the ACK. A contract failure IBCReceiveResult is mapped into an error that reverts state and aborts the protocol ack.

[ethanfrey]

I think we have different meanings for 3.
I pulled it out to 4 cases.

We currently handle 1, 2, and 4.

1 and 2 inside the contract (look the same to wasmd) and 4 is just returning a go error.

3 is not possible now, but is made possible by the new Acknowledgement interface

[ethanfrey]

The contract returns an error result (eg "InsufficientBalance"). It would be nice to let it return an error and have that be passed to the other side as an error. And rollback all state changes.

To accomplish this, we would need to define some standard way to encode unchecked errors. A contract can return errors and make use of that, or they be careful not to return errors ever, fall into case 2 and use anything they want.

ics20 and the new ics27 and using the same Acknowledgement type here. Let's just do this

[alpe]

When I think about the possible scenarios then I have the following in mind:

number	scenario	contract issue	state commit	tx successful	description
1	success	❌	✅	✅	all good
2	application failure	✅	✅	✅	issues that the contract can handle and requires modifying state: escrow paid to late then remove from list and abort process
3	application error	✅	❌	✅	issues that the contract can handle but requires reverting state: InsufficientBalance then abort process, no state update
4	other error	✅	❌	❌	unknown or not handleable issues: out of gas, invalid data

I would consider the first 3 application (contract) level scenarios. The contract is in full control and needs to modify state only. No Go errors returned.
Scenario 4 though is the "exception" case for me where we need to abort the TX.
The IBCReceiveResult object is not good enough yet as we need to distinguish between 2 and 3) without reverting the TX

[ethanfrey]

The IBCReceiveResult object is not good enough yet as we need to distinguish between 2 and 3) without reverting the TX

You mean IbcReceiveResponse?

We currently handle (1) and (2) with IbcReceiveResult::Ok(IbcReceiveResponse{..})
And we combine IbcReceiveResult::Err(_) with a "Go"/VM error.

If we implement the requested changes to wasmvm, we should have all info needed to distinguish between 3 and 4.

[ethanfrey]

PR to update wasmvm is made

[alpe]

ics20 and the new ics27 and using the same Acknowledgement type here. Let's just do this

Both protocols have a very limited scope use cases. For dynamic IBC we target a much greater audience and therefore can not make any assumption on the transported data nor what is considered success or failure on the application level.
I support the effort to standardize a success/error frame that would make monitoring and building frameworks easier but I don't think we are there yet. Instead we should support the lowest layer in wasmd which is binary.

We currently handle (1) and (2) with IbcReceiveResult::Ok(IbcReceiveResponse{..})
And we combine IbcReceiveResult::Err(_) with a "Go"/VM error.

Thanks for for the example. I have overseen that IbcReceiveResponse is used within OK()

When I try to map the IBCPacketReceive method return data from the PR into our cases, then I have still problems to distinguish between 2 and 3). Do I understand the cases correct:
1: IBCReceiveResult.OK()
2: IBCReceiveResult.OK() // as this is app level
3: IBCReceiveResult.Err
4: returned Go error

This would allow all 4 cases.

There is a little smell with IBCReceiveResult.Err being a string type that could be abused to transport json. For me a "cleaner" approach would be:

type IBCPacketReceiveResponse struct {
  Data []byte
  RollbackStore bool // default is commit
}

I had updated this post later.

[ethanfrey]

1 and 2 are both Resp.OK

They are both implemented now like that

[ethanfrey]

Current:

1. Resp.Ok
2. Resp.Ok
3. (not used)
4. Resp.Err, Go error

proposed:

1. Resp.Ok
2. Resp.Ok
3. Resp.Err
4. Go error

[ethanfrey]

Both protocols have a very limited scope. For dynamic IBC we can not make any assumption on the transported data nor what is considered success or failure on the application level.

Let me be blunt. Most go ibc code is coming from the core ibc interchain team. They wrote Ibc-Go v2. Everyone else writing such protocols (Desmos) will most likely follow their idioms. These are now using the Acknowledgement type.

Existing CosmWasm code (protocols) will have to use the same format when we want to intercommunicate (cw20-ics20).

New CosmWasm-only protocols can pick any encoding they want, so why not the standard.

By allowing (2) We allow someone to take extra care not to return errors and in return get full control over their encoding. (This is possible). By using a standard for (3) we make it much easier to interoperate with this standard.

I agree to your point that we should not limit the potential (and thus not wrap the success case in (1)). And I appreciate pushing me to make this work as it does now, so we know full customisation is possible.

But I do not understand the resistance to making the normal case much easier. I have written these contracts, (3) would be a huge simplification for coding. In fact, I could make 2 versions of cw20-ics20 using both approaches and add ts-relayer tests for them, to ensure they work with the "standard" or with full control of error encoding.

[alpe]

Sorry, I modified my comment not aware that you answered already.
I glad you confirmed the cases as I understand them now. 😅

the resistance to making the normal case much easier

As I wrote later standardization is a good thing. But I have more the view of a network protocol with data + rollback in wasmd than application payload + error. I definitively agree that the contract developer should not have to care about these details.
It is more where it belongs: contract stdlib or wasmd.

[ethanfrey]

It is more where it belongs: contract stdlib or wasmd.

We cannot handle any rollback over multiple submessages in cosmwasm stdlib. This can only be handled in x/wasm level.

[ethanfrey]

I think we have addressed this already as part of the v0.45 upgrade. We have much nicer rollback semantics now.

@alpe can I close this?