Routing module has to deliver separate store to secondary module and have the writing logic.

[Thunnini]

I'm not sure that this issue was discussed, but I looked around but couldn't find this issue.
In ics-26 routing module, IBC module will route packets, handshakes, and datagram to secondary module's handler. However, I found the lack of feature that delivers a separate store for the secondary module and writes this store if handler returns success.
In the current implementation in cosmos-sdk, cosmos-sdk have CacheKVStore and actually this store saves the values in memory and writes these values to the storage if the transaction succeeds. I think that the routing module has to support this feature within their logic. Without this, though the secondary module's handler returns failure, the routing module should deal with a relayed IBC transaction as a success because the receiving sequence should be increased. But in this case, even though the secondary module's handler returns failure, the values saved to the store before returning failure would remain in storage because transaction actually succeeds, and this leftover would make hard to predict errors and fundamentally this is not expected behavior for software developer. If relayed IBC transaction is handled as a failure when the secondary module's handler returns failure, the routing module would stop because the receiving sequence couldn't be increased and this is never recovered without manual chain upgrade. And If a panic occurs in the secondary module's handler, the same case occurs.
So, I suggest adding abstraction for CacheStore and the logic that writes values in the store to storage when the secondary module's handler returns success.

[mossid]

The sdk implementation uses antehandler in order to utilize baseapp's two-step commit process, one done after the antehandler execution and one after the handler execution. The packet merkle proof verification and sequence increment logic is done in the ante phase(see function Manager.Receive https://github.com/cosmos/cosmos-sdk/pull/5124/files#diff-0905361c7df7772ca92457ec2ebaea4bR262, which is called by the antehandler), so even if the internal handler return error, all the handler state modification will be reverted but not the IBC related changes.

[Thunnini]

Modular ante handler seems to be a very cool feature!
It solved this problem in very clean way. But, how do you think of adding our suggestion to ICS spec?

[Thunnini]

@mossid On second thought, I don't agree that this problem fully solved by the ante handler. Increasing the receiving sequence regardless of the result of the secondary module's handler can be solved by a modular ante handler. However, we should send an acknowledgement packet to sending chain, and we can get acknowledgement bytes after executing the secondary module's handler. So, I think this problem is not solved yet.

[mossid]

The fundamental solution for this problem will be adding the opposite function from AnteHandler, which may be named FoldHandler, which is executed after the normal handler execution and takes the sdk.Result as its argument(as opposed to sdk.Tx).

When the handler does not revert:

ibc.AnteHandler: verifies merkle proof and increases sequence(committed)
xxx.Handler: handles the packet and returns empty result(committed)
ibc.FoldHandler: does not write acknowledge bytes(or write empty one if unordered) (committed)

When the handler reverts:

ibc.AnteHandler: verifies merkle proof and increases sequence(committed)
xxx.Handler: fail to handle the packet and returns error result(reverted)
ibc.FoldHandler: writes acknowledge bytes according to the result(committed)

This way we can preserve the state changes related to IBC logic to be committed to the state even the handler state change gets reverted.

I think abortTransactionUnless between onRecvPacket and other IBC logics should be distinguished.

[cwgoes]

(see cosmos/cosmos-sdk#5230)

[cwgoes]

I think abortTransactionUnless between onRecvPacket and other IBC logics should be distinguished.

Possibly... or we can leave it up to the handler to call CacheContext and write() or not as necessary.

Do you think it's better to define that logic in the spec or leave it up to the module?

[cwgoes]

Closing as out-of-date; this was resolved in ADR 15 discussions.