fix: Add support for delayedack middleware to also delay ack and timeout packets #480
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
- Coverage 30.87% 30.71% -0.16%
==========================================
Files 166 166
Lines 25383 25719 +336
==========================================
+ Hits 7836 7900 +64
- Misses 16299 16552 +253
- Partials 1248 1267 +19 ☔ View full report in Codecov by Sentry. |
x/delayedack/hooks.go
Outdated
| @@ -32,24 +32,37 @@ func (im IBCMiddleware) FinalizeRollappPackets(ctx sdk.Context, rollappID string | |||
| // Get the packets for the rollapp until height | |||
| logger.Debug("Finalizing IBC rollapp packets", "rollappID", rollappID, "state end height", stateEndHeight, "num packets", len(rollappPendingPackets)) | |||
| for _, rollappPacket := range rollappPendingPackets { | |||
| logger.Debug("Finalizing IBC rollapp packet", "rollappID", rollappID, "sequence", rollappPacket.Packet.GetSequence(), "destination channel", rollappPacket.Packet.GetDestChannel()) | |||
| logger.Debug("Finalizing IBC rollapp packet", "rollappID", rollappID, "sequence", rollappPacket.Packet.GetSequence(), "destination channel", rollappPacket.Packet.GetDestChannel(), "type", rollappPacket.Type) | |||
| // Update the packet status | |||
| im.keeper.UpdateRollappPacketStatus(ctx, rollappID, rollappPacket, types.RollappPacket_ACCEPTED) | |||
There was a problem hiding this comment.
I think it's better to move it after the switch case, and set according to the error received
// Update the packet status
if err != nil {
im.keeper.UpdateRollappPacketStatus(ctx, rollappID, rollappPacket, types.RollappPacket_REJECTED)
} else {
im.keeper.UpdateRollappPacketStatus(ctx, rollappID, rollappPacket, types.RollappPacket_ACCEPTED)
}
There was a problem hiding this comment.
these are 3 different concepts here:
- Accepted means that its finalized (changed this name in the eibc PR for better name, i.e "FINALIZED").
- err means that there was an error running the underlying ibc method
- REJECTED means that there was a fraud and the packet was not finalized (changed it to REVERTED).
so err doesn't mean rejected as there was no fraud. it just means there an error running the underlying ibc module.
There was a problem hiding this comment.
yeah but do u want to mark it as ACCEPTED?
In normal case, if the app would return err, the relayer would retry
in our case the packet will be marked as accepted and forgotten (which might be fine if by design, but probably need to be marked somehow)
There was a problem hiding this comment.
IMO it should be marked as FINALIZED - as it is finalized.
as I previously wrote I think ACCEPTED is confusing term here.
so as I see it, the packet is finalized, but it could have errors.
as for the relayer - if we're talking about application layer error which are not part of out-of-sync problem (e.g LC headers didn't yet arrive - which is not the case in our example) - by default the relayer will not retry (i.e if you simply run rly tx transfer command). It's true that if you restart it will try to get all non-relayed packets but if the packet itself is maleformed or it's data invalid - retry won't help unless the relayer code/blockchain code or the blockchain state changes.
My point is that as I see it mostly application layer errors, specifically for ics-20 which are mostly around malformed data or lack of balances wouldn't change with relayer retry (which eventually get exhausted and the packet transfer would fail and timeout) so I think it's better to say: "Hi, this packet is finalized, but we couldn't apply it as it was malformed/not enough balance".
re forgotten - I think our UI should reflect any errors in the packet by looking at the error field.
…t malformed packet attacks.
… and update the error.
omritoptix
changed the title
| // Run the underlying app's OnAcknowledgementPacket callback | ||
| // with cache context to avoid state changes and report the acknowledgement result. | ||
| // Only save the packet if the underlying app's callback succeeds. | ||
| cacheCtx, _ := ctx.CacheContext() |
There was a problem hiding this comment.
why u need it? u already run it with cacheTx by the caller
There was a problem hiding this comment.
Yea but than the caller controls it.
after it succeeds with cache context, it will actually run it without - which will make the ack run before the packet is finalized.
I want to force it to always run with cache context by the caller and run it with non-cached context only from the hook.
Maybe there is a better way to do it?
There was a problem hiding this comment.
oh I see what u did here
what about the case err != nil?
we don't want to write the cacheCtx?
There was a problem hiding this comment.
can you elaborate? not sure what you mean by writing the cacheCtx.
|
There is an upgrade hander required for FP because of the packet struct change. opened an issue: #529 |
Description
Closes #XXX
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsFor Reviewer:
After reviewer approval: