Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deliver messages directly to Polymer Dispatcher #31
Deliver messages directly to Polymer Dispatcher #31
Changes from 3 commits
b129bd8
9d62152
f90aada
d2c4747
bd6726b
cacb72d
1029a44
5d65d9c
ec9e943
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
_handleAck
reverts,isVerifiedMessaageHash
state wouldn't be mutated. So how to recover the ack? this is related to the question above tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question. What is the execution strictness when delivered from Polymer?
Is it required for applications that the
on....Packet
functions never revert?So we need to call
_handleAck
within a try statement?What happens if
onAcknowledgementPacket
reverts from `Polymer's perspective?Generally,
_handleAck
should never revert if the relayer provides enough gas for the application. It is only if the relayer cheaps out the function can revert.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Polymer side, all packet callback functions
on...Packet
are effectively try-catched, eg.onAcknowledgementPacket
(bool success, bytes memory data) = _callIfContract(. So it's ok if IBC-dApps revert in any callback functions.In your case, since retry is handled by
recoverAck
, I'd recommend you wrap_handleAck
in try-catch, so to record acks even when_handleAck
reverts due to lack of gas.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try-catch implementation in the dispatcher is vulnerable to gas denial of service for large gas spenders. This is because max 63/64 of the gas is forwarded. There should be an gas check, otherwise there is a soft-max gas limit for Polymer applications.
Something like this:
GeneralisedIncentives/src/IncentivizedMessageEscrow.sol
Lines 585 to 587 in e410087
This is not a catch-all solution since reverting by spending all gas will block from the applications side. But at least that will be the applications fault.
Looking further through your implementation, this is not an immediate using it for our testnet purposes.
Our contracts should not revert when packages are properly relayed and as a result, the try-catch in itself is not what we worry about. Furthermore, there is still a way to process the acks since acks are not written (on Polymer) until they execute without failure.