Cosmos-sdk: Review work on on ADR 15

[cwgoes]

From this comment onwards - https://github.com/cosmos/cosmos-sdk/pull/5401#issuecomment-573099205

[mossid]

https://github.com/cosmos/cosmos-sdk/pull/5401#issuecomment-573167159

Sequence increasing is now happening inside PacketExecuted which is called by the handlers

[mossid]

https://github.com/cosmos/cosmos-sdk/pull/5401#issuecomment-573169135

It should be called by the receiving side of the connection(in the ICS20 handler), will check it once more

WriteAcknowledgement should be called by the receipt handler

[mossid]

https://github.com/cosmos/cosmos-sdk/pull/5401#issuecomment-573179240

That seems like a mistake.... can be fixed by using a cachecontext inside the antehandler, keep increasing sequence, but not writing back to the underlying store...

[cwgoes]

#5401 (comment)

That seems like a mistake.... can be fixed by using a cachecontext inside the antehandler, keep increasing sequence, but not writing back to the underlying store...

OK, I am not entirely sure what you mean, can you write it in a PR perhaps? (ditto for the above)

[fedekunze]

WriteAcknowledgement should be called by the receipt handler

what is the receipt handler?

[fedekunze]

@cwgoes:

I am trying to figure out what is intended here. It looks like you want the ante handler to check the sequence @mossid, but if we write the sequence only when packets are executed that won't work for subsequent packets in the same block (since the ante handler will be executed first). What is intended?
For now, I added the check in RecvPacket() and the write in PacketExecuted() - this will work for the test cases & in simple cases, but I don't think this is the right solution long-term.

in the case of multiple packet executions, are all of them atomic, i.e should we revert any change if one of those executions fail?

[cwgoes]

in the case of multiple packet executions, are all of them atomic, i.e should we revert any change if one of those executions fail?

For now, I think we should maintain the atomicity semantics of multi-Msg transactions, but subject to separate acknowledgement/failure handling as defined by the IBC spec (so a "ack of failure" would cause remaining packets to be processed, but an invalid sequence would cause remaining packets to fail & all state changes to be reverted). Does that make sense?

[cwgoes]

I don't think there are any remaining actionables here. @mossid if there are please open issues.