feat: add missing migration flow in IBC refund flow #164
Conversation
WalkthroughAdds .gocache to .gitignore; expands migration spec with failure-handling text; adds a refund event constant; extends IBC middleware to accept a codec, parse acknowledgements/timeouts, detect refunded IBC vouchers, and refund/mint OP tokens; updates tests and mocks to cover new flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Relayer
participant IBC as IBCMiddleware
participant App as UnderlyingApp
participant Bank as BankKeeper
participant OP as OPChildKeeper
participant Events as EventBus
Note over IBC: OnAcknowledgementPacket flow (codec-aware)
Relayer->>IBC: Acknowledgement(packet, ackBytes)
IBC->>IBC: isAckError(ackBytes) using cdc
alt Ack success
IBC->>App: OnAcknowledgementPacket(...)
App-->>IBC: result
else Ack error (failed transfer)
IBC->>IBC: lookupPacket(packet) -> (token, ibcDenom)
IBC->>Bank: BalanceBefore(sender, ibcDenom)
IBC->>App: OnAcknowledgementPacket(...) (delegate)
App-->>IBC: result
IBC->>Bank: BalanceAfter(sender, ibcDenom)
IBC->>IBC: delta = after - before
alt delta > 0 (voucher refunded)
IBC->>OP: HandleMigratedTokenRefund(sender, ibcDenom, delta)
OP-->>IBC: burned voucher & minted OP
IBC->>Events: Emit(EventTypeHandleMigratedTokenRefund)
else No refund detected
Note over IBC: no migration action
end
end
sequenceDiagram
autonumber
participant Relayer
participant IBC as IBCMiddleware
participant App as UnderlyingApp
participant Bank as BankKeeper
participant OP as OPChildKeeper
participant Events as EventBus
Note over IBC: OnTimeoutPacket flow
Relayer->>IBC: Timeout(packet)
IBC->>IBC: lookupPacket(packet) -> (token, ibcDenom)
IBC->>Bank: BalanceBefore(sender, ibcDenom)
IBC->>App: OnTimeoutPacket(...)
App-->>IBC: result
IBC->>Bank: BalanceAfter(sender, ibcDenom)
IBC->>IBC: delta = after - before
alt delta > 0 (voucher refunded on timeout)
IBC->>OP: HandleMigratedTokenRefund(sender, ibcDenom, delta)
OP-->>IBC: burned voucher & minted OP
IBC->>Events: Emit(EventTypeHandleMigratedTokenRefund)
else No refund detected
Note over IBC: no migration action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
+ Coverage 51.69% 51.99% +0.30%
==========================================
Files 62 62
Lines 5186 5281 +95
==========================================
+ Hits 2681 2746 +65
- Misses 1977 1996 +19
- Partials 528 539 +11
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/opchild/middleware/migration/ibc_module.go (1)
274-394: Consider refactoring to eliminate duplication between OnAcknowledgementPacket and OnTimeoutPacket.
OnAcknowledgementPacket(lines 274-336) andOnTimeoutPacket(lines 338-394) share nearly identical logic:
- Parse packet with
lookupPacket- Check migration registration
- Measure balance before/after underlying app call
- Burn IBC token and mint L2 token
- Emit refund event
The only difference is the initial error-acknowledgement check in
OnAcknowledgementPacket.Consider extracting the common logic into a private helper method:
func (im IBCMiddleware) handleRefund( ctx sdk.Context, packet channeltypes.Packet, sender sdk.AccAddress, ibcDenom string, data transfertypes.FungibleTokenPacketData, ) error { // get the before balance beforeBalance := im.bankKeeper.GetBalance(ctx, sender, ibcDenom) // call the underlying IBC module (caller passes the appropriate method) // This part would need to be parameterized // if the balance is not changed, do nothing afterBalance := im.bankKeeper.GetBalance(ctx, sender, ibcDenom) if afterBalance.Amount.LTE(beforeBalance.Amount) { return nil } // compute the difference diff := afterBalance.Amount.Sub(beforeBalance.Amount) // burn IBC token and mint L2 token ibcCoin := sdk.NewCoin(ibcDenom, diff) l2Coin, err := im.opChildKeeper.HandleMigratedTokenDeposit(ctx, sender, ibcCoin, "") if err != nil { return err } ctx.EventManager().EmitEvent(sdk.NewEvent( EventTypeHandleMigratedTokenRefund, sdk.NewAttribute(AttributeKeyReceiver, data.Sender), sdk.NewAttribute(AttributeKeyIbcDenom, ibcDenom), sdk.NewAttribute(AttributeKeyAmount, l2Coin.String()), )) return nil }Then call this helper from both methods after their respective checks and before the underlying app call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
specs/migration/readme.md(1 hunks)specs/migration/technical_specification.md(2 hunks)x/opchild/middleware/migration/ibc_module.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/migration/readme.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: spellcheck
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
x/opchild/middleware/migration/ibc_module.go (6)
9-9: LGTM: Codec integration is correct.The addition of
codec.Codecto the middleware is necessary for parsing acknowledgements inisAckErrorand is consistently integrated across import, struct field, constructor parameter, and initialization.Also applies to: 33-33, 56-56, 64-64
221-226: LGTM: Refactoring improves maintainability.The use of
lookupPackethelper eliminates code duplication and makes the intent clearer. The early-return pattern whenok=falseis appropriate.
328-333: Verify attribute key naming and memo usage.Two observations:
Attribute naming inconsistency: Line 330 uses
AttributeKeyReceiverbut sets it todata.Sender. Consider renaming toAttributeKeySenderfor clarity, or update the variable name if "receiver" is intended to represent the refund recipient.Empty memo: Line 323 passes an empty string
""as the memo toHandleMigratedTokenDeposit. Verify this is the intended behavior for refund scenarios, as opposed to preserving the original memo from the failed transfer.
396-425: LGTM: lookupPacket helper correctly handles both receive and send paths.The function properly:
- Parses transfer packet data
- Distinguishes between receive and send scenarios
- Checks token origin using
ReceiverChainIsSource/SenderChainIsSource- Computes the correct prefixed IBC denom
Note: Line 400 silently returns
falseon unmarshal errors, treating non-transfer packets as not needing migration checks. This is appropriate for the middleware pattern.
437-445: LGTM: isAckError correctly detects error acknowledgements.The helper properly uses the codec to unmarshal the acknowledgement and checks the success status. The fallback to
falseon unmarshal error is safe, as it will pass through to the underlying app.
329-329: EventTypeHandleMigratedTokenRefund constant exists; no action required.specs/migration/technical_specification.md (2)
28-30: LGTM: Documentation accurately describes the new functionality.The updated key functions list correctly includes
OnAcknowledgementPacketandOnTimeoutPacket, with clear descriptions of their refund behavior.
132-140: LGTM: Comprehensive documentation of failure handling logic.The new "Failure Handling" section clearly documents:
- Detection of failed acknowledgements and timeouts
- The refund flow using
HandleMigratedTokenDeposit- Event emission with
EventTypeHandleMigratedTokenRefundThis accurately reflects the implementation and provides good technical detail.
1 participant
Description
This is following of PR #159, add missing migration flow in IBC refund flow
Author Checklist
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.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Documentation
Tests
Chores