Add a section on encoding IBCReceive errors #885
Conversation
ethanfrey
force-pushed
the
on-packet-recv-error-handling
branch
from
d4daf80
to
69b259a
Compare
|
I'd be happy for a review on this one. Especially from @alpe who has seen the sdk bindings. I hope this provides a good solution for rollback on receive |
|
Open question: do we want to do something similar with acknowledgement and timeout? |
ethanfrey
force-pushed
the
on-packet-recv-error-handling
branch
from
69b259a
to
e8a1635
Compare
IBC.md
Outdated
| 4. `receiveResult` is `Err(String)` if any of the above errored, or `Ok(Binary)` | ||
| with the original acknowledgement bytes if we have the success case. | ||
| 5. Call `ibc_encode_acknowledgement` with the `receiveResult`. The return value | ||
| is the acknowledgement packet to send to the calling chain. This should never |
There was a problem hiding this comment.
What is the DB transaction scope of this call? Is it the same as ibc_receive_packet so that it would be reverted on the error before or a new one that is independent?
I don't have a concrete use case but it feels useful to persist data with a failure ACK as well. If we want this then an error return value makes sense. Though the process would end without an ACK to the packet sender.
There was a problem hiding this comment.
This is not my invention, I am following the ibc design (which will come out in 0.43 or 0.44). Check the links at the top of https://github.com/CosmWasm/cosmwasm/blob/on-packet-recv-error-handling/IBC.md#acknowledgement-processing
Especially read the issue description here: cosmos/ibc-go#91 (skim the comments if you wish, the description has the intention, the rest is discussing details). And if you want to dig into the code to understand one part, you can see cosmos/ibc-go#107 (but that is not necessary to get the design)
IBC.md
Outdated
| is the acknowledgement packet to send to the calling chain. This should never | ||
| error. | ||
| 6. If `ReceiveResult` is `Ok`, commit this "storage cache". If it is `Err`, | ||
| revert those changes. |
There was a problem hiding this comment.
🤔 Do I understand this correct that this step let the contract handle the error case on messages and control the TX scope? The process I was assuming would revert the TX on error in 3) but can persist new state in 5).
There was a problem hiding this comment.
The future IBC design (linked above, here again: cosmos/ibc-go#91) explicitly makes the entire app execution (x/wasm) atomic - to be rolled back when app error. But not touching the IBC part of the transaction and successfully committing a "error acknowledgement" in the IBC Keeper.
I feel it is pointless to design something other than what they are doing, unless you want to re-open that issue in the ibc repo. As in the mid-term, we will just use those semantics in x/wasm and all other modules will use them as well.
ethanfrey
force-pushed
the
on-packet-recv-error-handling
branch
from
6b32c6e
to
e00a340
Compare
ethanfrey
force-pushed
the
on-packet-recv-error-handling
branch
from
e00a340
to
cd784cd
Compare
|
Okay, this got a complete overhaul since I worked out the new I also modified |
IBC.md
Outdated
| attributes: vec![], | ||
| }) | ||
| }) | ||
| ``` |
There was a problem hiding this comment.
We could even enforece this by changing the signature of the entry point to IbcReceiveResponse, i.e. no result around it.
There was a problem hiding this comment.
Nice idea. This will push devs to proper usage
|
I addressed all the comments except for changing the return type. If we want to do that, I would like to do that in a different PR (this one is mainly docs and making example code). Can we merge this? |
| Note that it leaves the actual success response as app-specific bytes where you | ||
| can place anything, but does provide a standard way for an observer to check | ||
| success-or-error. If you are designing a new protocol, I encourage you to use | ||
| this struct in either of the encodings as the acknowledgement envelope. |
There was a problem hiding this comment.
👍 good to have this as recommendation
Builds on #898 (merge that first)
Closes #762