Revert application state changes on failed acknowledgements #107
Conversation
|
I just need to update developer docs to make this functionality extra clear |
There was a problem hiding this comment.
Looks good, just document requirement that async acks need to handle packet processing more carefully.
Great work @colin-axner!
| The IBC handler will then commit this acknowledgement of the packet so that a relayer may relay the | ||
| acknowledgement back to the sender module. | ||
|
|
||
| The state changes that occurred during this callback will only be written if: | ||
| - the acknowledgement was successful as indicated by the `Success()` function of the acknowledgement | ||
| - if the acknowledgement returned is nil indicating that an asynchronous process is occurring |
There was a problem hiding this comment.
So anyone using async acks will have to take care to revert state themselves. This should be documented
There was a problem hiding this comment.
Added a note. Going to merge this pr, feel free to open an issue or comment here if you think it could use more information
| // state changes should be written via the `Success()` function. Application state | ||
| // changes are only written if the acknowledgement is successful or the acknowledgement | ||
| // returned is nil indicating that an asynchronous acknowledgement will occur. | ||
| ack := processPacket(ctx, packet, packetData) |
There was a problem hiding this comment.
| ack := processPacket(ctx, packet, packetData) | |
| ack := processPacket(ctx, packet) |
It's a bit confusing to have both packet and packetData here. For documentation purposes, we can do keep it as above. processPacket is responsible for decoding packetdata
There was a problem hiding this comment.
I think it is useful to show the app developer they need to decode the packet data, as shown by the line above this one
Going to leave as is since it matches ICS 20
|
|
||
| ## IBC callback changes | ||
|
|
||
| ### OnRecvPacket |
There was a problem hiding this comment.
Me too! We should continue this for future major version releases
Add docs on (un)supported systems
Fix typo in demo instructions
Description
Thanks @AdityaSripal for the first commits
closes: #91
closes: #68
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes