Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch errors in App callbacks #112

Open
AdityaSripal opened this issue Nov 19, 2024 · 1 comment · May be fixed by #115
Open

Catch errors in App callbacks #112

AdityaSripal opened this issue Nov 19, 2024 · 1 comment · May be fixed by #115

Comments

@AdityaSripal
Copy link
Member

Currently any revert in the external contract call of the application callback will revert the entire transaction. This will lead to suboptimal behavior and even errors in our transaction callbacks.

  1. In OnSendPacket, the behavior is correct. If a revert happens in the callback here, we don't want to send the packet at all so we should revert everything.
  2. In OnRecvPacket, any revert inside the app callback will revert the entire tx. This isn't incorrect but suboptimal. Here we will not have an error ack but instead will have to rely entirely on timeouts which will extend the time taken before tokens are reimbursed in the case of ICS20. Moreover, it will allow relayers to continue submitting a recvPacket tx even though it will never succeed. In particular, the app developers are highly prone to error since they must be careful to not create partial state changes and then return an error ack.
  3. In OnAck/OnTimeoutPacket, any revert inside the app callback will revert the entire tx. This is suboptimal in the single payload case and wrong in the multipayload case since a single revert in a single app will prevent packet lifecycle completion for every other app. Moreover, we again have inefficiencies with the relayer since they may continue resubmission.

Solution:

Use try/catch for the app callbacks. For OnAck/OnTimeout callbacks, we just ignore the app error and revert the app-level changes while still committting the overall tx. This fixes the relayer issue, and in multipayload we will correctly execute all app callbacks regardless in one app errors.

For OnRecvPacket, we will also wrap in a try/catch. Any error that occurs we will convert into an error ack and add to our acknowledgement list. This correctly reverts the app state if an error is returned while still writing an error ack. This makes writing applications correctly much easier since you do not have to worry about reverting state changes yourself. However, in order to do this with Solidity I needed to create an error acknowledgment in the core handler.
This was difficult to do with the current design since the acknowledgements are completely up to the application. So there was no way that IBC core handler could construct an error ack from an error message since there was no standardized way to describe an error acknowledgment.

This led to prefixing the error coming from the app callback with the boolean false to create a standardized error acknowledgment. Thus the success acknowledgment is prefixed with true.

The Acknowledgement callback can thus be modified to pass in a success boolean value. Making this success value global will make it trivially support multipayload atomic acknowledgments

@AdityaSripal AdityaSripal linked a pull request Nov 21, 2024 that will close this issue
6 tasks
@srdtrk
Copy link
Member

srdtrk commented Nov 22, 2024

I was thinking that we could instead implement a multipass function (rather than multicall) where it still allows multiple functions to be called but doesn't revert the entire tx if one fails. What do you think about this @AdityaSripal ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants