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

Closed
AdityaSripal opened this issue Nov 19, 2024 · 1 comment · Fixed by #259
Closed

Catch errors in App callbacks #112

AdityaSripal opened this issue Nov 19, 2024 · 1 comment · Fixed by #259
Assignees
Labels
enhancement Improvements security This issue impacts security assumptions

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

@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 ?

@srdtrk srdtrk added the enhancement Improvements label Dec 4, 2024
@srdtrk srdtrk added the security This issue impacts security assumptions label Jan 21, 2025
@github-project-automation github-project-automation bot moved this to Backlog in IBC Jan 21, 2025
@gjermundgaraba gjermundgaraba moved this from Backlog to In progress in IBC Feb 6, 2025
@gjermundgaraba gjermundgaraba moved this from In progress to In review in IBC Feb 6, 2025
@github-project-automation github-project-automation bot moved this from In review to Done in IBC Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements security This issue impacts security assumptions
Projects
Status: Done
2 participants