DRAFT: Convert error to Acknowledgements #115
Conversation
src/ICS26Router.sol
Outdated
| } catch (bytes memory errorData) { | ||
| recvSuccess = false; | ||
| acks[0] = abi.encodePacked( | ||
| errorData | ||
| ); |
There was a problem hiding this comment.
I don't get this. We are returning an error ack if callback fails, couldn't this happen due to gas reasons?
There was a problem hiding this comment.
AHh yes, i forgot about gas being an issue, in this case entire tx should revert. I basically want to return an error ack if the error is unrecoverable (i.e. the callback itself is returning an error)
There was a problem hiding this comment.
Though it would be strange if you can try/catch a gas error right? Seems like a loophole that wouldn't exist 😄
Trying to read through the docs to see what the behavior of gas panics in EVM is
There was a problem hiding this comment.
I think the gas issue is only relevant if we call the external contract with a specific amount of gas. With a normal call we would forward all the gas the tx has, which means that we would be out of gas in the catch (therefore never get to do anything anyway). If we had limited the call to some amount of gas, then we could in theory catch it here and handle it (if we had gas left).
| ) {} catch { | ||
| // no-op | ||
| } |
There was a problem hiding this comment.
we shouldn't actually be acking a packet if the callback cannot be made right? This feels wrong
There was a problem hiding this comment.
If the callback fails permanently then there's no point in resubmitting. This is for the case where ack just errors in an unrecoverable way. Resubmission won't fix anything here, so we may as well do cleanup.
| ) {} catch { | ||
| // no-op | ||
| } |
There was a problem hiding this comment.
Again, we need the timeout callback to succeed to unescrow funds
There was a problem hiding this comment.
Ditto: What if the timeout callback can never succeed?
There was a problem hiding this comment.
Then I think the packet should just be stuck?
|
We might want to compare this pr to #138 |
Description
closes: #112
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.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.