Implement additional callbacks as per ICS 26

[cwgoes]

Handle additional callbacks, ref https://github.com/cosmos/cosmos-sdk/pull/5401/files#diff-de4dba43d2cba47382cdb1c05d1f8c3cR3

[jackzampolin]

Ok, it appears that there are some substantial remaining issues to be resolved here that will block the demo. It does not appear that packet receipt is hooked up or working in the current code.

[cwgoes]

Ok, it appears that there are some substantial remaining issues to be resolved here that will block the demo. It does not appear that packet receipt is hooked up or working in the current code.

None of these points should be related to packet receipt. As far as I can tell from a quick read of the code, packet receipt is hooked up (see handler.go and handlePacketDataTransfer), and it's tested in keeper/relay_test.go (though the tests may not be complete, and it looks like handler_test.go should also include some tests for the full handler switch-case logic). Is that what you are referring to, or if not can you be more specific?

[mossid]

Packet receipts are only sent back when the token sending fails due to the invalid packet data or some receiving side logic, which we don't have in the current version of ICS20 spec/impl afaik

[cwgoes]

Packet receipts are only sent back when the token sending fails due to the invalid packet data or some receiving side logic, which we don't have in the current version of ICS20 spec/impl afaik

Can bankKeeper.AddCoins fail? If so, we'll need to handle this case.

[mossid]

Assuming the amount is correct, the only case that AddCoins fails is when the (oldCoins+amt) < 0, which I don't quite understand.

[fedekunze]

Packet receipts are only sent back when the token sending fails due to the invalid packet data or some receiving side logic

what is a packet receipt and where is this implemented?

[fedekunze]

Handle additional callbacks

can you point me to this item? which are the additional callbacks that need to be handled?

[jackzampolin]

Packet Receipt is the PacketAcknowledge afaik.

And the callbacks, according to the linked comment are:

// OnChanOpenInit, OnChanOpenTry, OnChanOpenAck, OnChanOpenConfirm, OnChanCLoseConfirm
// will be implemented according to ADR15 in the future PRs. Code left for reference.
//
// OnRecvPacket, OnAcknowledgementPacket, OnTimeoutPacket has been implemented according
// to ADR15.

[cwgoes]

Assuming the amount is correct, the only case that AddCoins fails is when the (oldCoins+amt) < 0, which I don't quite understand.

It seems to me like we should probably panic in this case, but I think it's worth building out the acknowledgement success/failure logic anyways, just to see how it works.

[cwgoes]

Cleanup ICS 20, ref https://github.com/cosmos/cosmos-sdk/pull/5401/files#diff-7618ad92b3cc2b6f88de792912599dbfR3

I can't find this comment anymore, does anyone remember what it was referencing?

[cwgoes]

Why does PacketDataI have a .Type() field? This shouldn't be used for routing anywhere.

[cwgoes]

How precisely do we want to deal with the requisite callbacks (channel opening, closing)? The ADR defines a CheckOpen method - https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-015-ibc-packet-receiver.md#decision - but I can't find that implemented anywhere, nor the ChannelChecker (what is the ChannelChecker?).

I think it would be preferable to handle these callbacks in the same way as incoming packets and timeouts, instead of creating another binding & callback system, if possible. Are there any impediments there?

Thoughts @mossid @fedekunze?

[fedekunze]

Why does PacketDataI have a .Type() field? This shouldn't be used for routing anywhere.

Agree we should delete it

[fedekunze]

I think it would be preferable to handle these callbacks in the same way as incoming packets and timeouts, instead of creating another binding & callback system, if possible.

👍

The ADR defines a CheckOpen method - https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-015-ibc-packet-receiver.md#decision - but I can't find that implemented anywhere, nor the ChannelChecker (what is the ChannelChecker?).

I also noticed there are a lot of concepts and functions that were not implemented. @mossid, care to comment regarding that?

[cwgoes]

Initially, I think we can just implement the callbacks identically to how packet receipt, timeouts, etc. are currently handled within the ante handler.

[jackzampolin]

This sounds like a great call @cwgoes

[cwgoes]

@AdityaSripal suggests a second router used by the IBC handler. Note that it must be the same as the capability ownership table.

[cwgoes]

Agreed that we need a second router with dynamic routing support, I will work on this.

I'll also update the ADR accordingly.

[jackzampolin]

Where are we with this issue? cc @cwgoes

[cwgoes]

Where are we with this issue? cc @cwgoes

1. The functionality fixes for timeouts & changes to interfaces & other misc minor stuff are done.
2. The second router to route handshake callbacks is not done.
3. The ICS 20 code needs to be peer-reviewed, this is scheduled for Thursday.
4. I am still working on some minor ICS 20 spec updates here which we should integrate.

The current state should be fine to get the demo working (w.r.t. this issue) but 2/3/4 block IBC 1.0.

[jackzampolin]

Is there an issue/design for the second router?

[jackzampolin]

Are there issues open for the remaining work here that make what needs to be done a bit more clear? Looks like the spec update has been done and some of the stuff that wasn't done a month ago has moved forward a bit.

[cwgoes]

This is blocked on #5542.

[cwgoes]

Closed by #5888.