Deliver messages directly to Polymer Dispatcher #31
Conversation
reednaa
changed the title
|
I have updated the implementation with the newest ABI inspired by Mars.sol. Let me know if the implementation is good and secure. |
| // Set a verificaiton context so we can recover the ack. | ||
| isVerifiedMessageHash[keccak256(rawMessage)] = VerifiedMessageHashContext({ | ||
| chainIdentifier: packet.src.channelId, | ||
| implementationIdentifier: destinationImplementationIdentifier | ||
| }); | ||
| _handleAck(packet.src.channelId, destinationImplementationIdentifier, rawMessage, feeRecipitent, gasLimit); |
There was a problem hiding this comment.
If _handleAck reverts, isVerifiedMessaageHash state wouldn't be mutated. So how to recover the ack? this is related to the question above too
There was a problem hiding this comment.
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.
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.
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.
Connect directly with the Polymer dispatcher for simpler off-chain indexing of the packages. This allows Generalised Incentives to have full control over the package flow.