feat: add same-chain swap rebalancing bridge#8618
Conversation
nambrot-agent
changed the title
📝 WalkthroughWalkthroughAdds SwapRebalancingBridge: interface and contract enabling same-chain exact-input swap rebalances with owner-managed rebalancer/target allowlists, single in-flight pending state, swap-call execution with temporary allowances, required-output computation, and comprehensive unit and fork tests plus a changeset. ChangesSwap Rebalancing Bridge Feature
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8618 +/- ##
==========================================
- Coverage 79.33% 79.28% -0.06%
==========================================
Files 143 144 +1
Lines 4278 4407 +129
Branches 436 465 +29
==========================================
+ Hits 3394 3494 +100
- Misses 855 884 +29
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| token: pending.inputToken, | ||
| amount: pending.amountIn | ||
| }); | ||
| quotes[2] = Quote({token: pending.inputToken, amount: 0}); |
There was a problem hiding this comment.
🟡 quoteTransferRemote returns duplicate token addresses violating ITokenBridge interface contract
quoteTransferRemote returns quotes[1] and quotes[2] both with token: pending.inputToken, violating the ITokenBridge interface documentation at solidity/contracts/interfaces/ITokenBridge.sol:18 which states: "There should not be duplicate token addresses in the returned quotes."
The current caller (MovableCollateralRouter.rebalance at solidity/contracts/token/libs/MovableCollateralRouter.sol:143) uses Quotes.extract() which sums amounts for duplicate tokens, so the duplicate with amount: 0 on quotes[2] doesn't cause a runtime issue today. However, other callers that process quotes positionally or build a map (overwriting one entry with the other) would misinterpret the fee structure. The likely intended token for quotes[2] is pending.outputToken (the destination token being credited), not pending.inputToken.
| quotes[2] = Quote({token: pending.inputToken, amount: 0}); | |
| quotes[2] = Quote({token: pending.outputToken, amount: 0}); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| function _clearPending() internal { | ||
| delete pending; | ||
| delete pendingSwapCalls; | ||
| } |
There was a problem hiding this comment.
🚩 No recovery mechanism for stuck pending rebalance state
If source.rebalance() at line 221 returns successfully without calling transferRemote(), the pending state is committed to storage and RebalanceAlreadyPending (line 161-162) permanently blocks all future rebalances. There is no owner escape hatch or timeout-based cleanup, despite pending.deadline being stored. The real MovableCollateralRouter.rebalance (solidity/contracts/token/libs/MovableCollateralRouter.sol:156) always calls bridge.transferRemote(...) unconditionally—if it reverts, the whole transaction reverts and pending is never committed. So this only manifests with a buggy or non-standard source router implementation. Given the trust model (owner sets authorized rebalancers, routers must be enrolled contracts), the risk is low, but adding an onlyOwner clearPending() function would be a cheap safety net.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
solidity/contracts/token/SwapRebalancingBridge.sol (1)
349-381: 💤 Low valueAdd a brief comment documenting rounding behavior.
The double
Math.Rounding.Downusage means therequiredOutmight be slightly less than a mathematically perfect conversion. A quick comment noting which party this favors (initiator, since less output is required) would help future readers understand the design choice. Not a showstopper by any means, just good swamp hygiene.+ // Note: Double rounding down minimally favors the initiator (slightly lower requiredOut). function _requiredOut( IMovableCollateralRouterLike sourceRouter,As per coding guidelines: "Document precision loss and which actors benefit/suffer from it in Solidity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/contracts/token/SwapRebalancingBridge.sol` around lines 349 - 381, Add a short inline NatSpec or code comment inside the _requiredOut function explaining that both Math.mulDiv calls use Math.Rounding.Down, which causes potential precision loss resulting in requiredOut being rounded down (i.e., possibly slightly less than exact conversion) and therefore favors the initiator/sender (they need to supply less output than exact math); reference the two Math.Rounding.Down usages and note this is an intentional design choice about who benefits from rounding.solidity/contracts/token/interfaces/ISwapRebalancingBridge.sol (1)
12-23: 💤 Low valueConsider packing
localDomainwith an adjacent address field for gas savings.The
PendingRebalancestruct has auint32 localDomainsitting alone in its own storage slot. Moving it next to one of the address fields (e.g.,outputToken) would pack them together and save a storage slot. Not the end of the swamp, but every bit of gas counts when you're storing this in state.struct PendingRebalance { address initiator; address sourceRouter; address destinationRouter; address inputToken; - address outputToken; - uint32 localDomain; + address outputToken; // 20 bytes + uint32 localDomain; // 4 bytes - packs with outputToken in same slot uint256 amountIn; uint256 minAmountOut; uint256 requiredOut; uint256 deadline; }Note: Adjacent placement in struct definition enables packing. Verify Solidity version behavior, but this should work in 0.8.x.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/contracts/token/interfaces/ISwapRebalancingBridge.sol` around lines 12 - 23, The PendingRebalance struct currently has uint32 localDomain isolated in its own slot; to enable storage packing and save gas, reorder the struct so localDomain is declared directly adjacent to an address-sized field (for example move localDomain next to outputToken or inputToken), ensuring the resulting field order keeps addresses aligned before smaller uints; update any code that constructs or reads PendingRebalance to match the new field order (references to PendingRebalance, localDomain, outputToken, inputToken, and any constructors or assignment sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@solidity/contracts/token/interfaces/ISwapRebalancingBridge.sol`:
- Around line 12-23: The PendingRebalance struct currently has uint32
localDomain isolated in its own slot; to enable storage packing and save gas,
reorder the struct so localDomain is declared directly adjacent to an
address-sized field (for example move localDomain next to outputToken or
inputToken), ensuring the resulting field order keeps addresses aligned before
smaller uints; update any code that constructs or reads PendingRebalance to
match the new field order (references to PendingRebalance, localDomain,
outputToken, inputToken, and any constructors or assignment sites).
In `@solidity/contracts/token/SwapRebalancingBridge.sol`:
- Around line 349-381: Add a short inline NatSpec or code comment inside the
_requiredOut function explaining that both Math.mulDiv calls use
Math.Rounding.Down, which causes potential precision loss resulting in
requiredOut being rounded down (i.e., possibly slightly less than exact
conversion) and therefore favors the initiator/sender (they need to supply less
output than exact math); reference the two Math.Rounding.Down usages and note
this is an intentional design choice about who benefits from rounding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1723460a-de35-4dd9-8444-594b399f5e9f
📒 Files selected for processing (6)
.changeset/swap-rebalancing-bridge.mdsolidity/contracts/test/TestSwapTarget.solsolidity/contracts/token/SwapRebalancingBridge.solsolidity/contracts/token/interfaces/ISwapRebalancingBridge.solsolidity/test/token/SwapRebalancingBridge.fork.t.solsolidity/test/token/SwapRebalancingBridge.t.sol
1 participant
Summary
Adds
SwapRebalancingBridge, a same-chain rebalance bridge that usesMovableCollateralRouter.rebalance(...)only as a source-collateral release primitive, then executes exact-input swap calls and settles an enrolled destination router with exact nominal output.Intended usage
targetsallowanceTargetsexecuteRebalance(sourceRouter, destinationRouter, amountIn, minAmountOut, deadline, swapCalls).routers(...)orcrossCollateralRouters(...)localDomainsourceRouter.rebalance(localDomain, amountIn, this).quoteTransferRemote(...)andtransferRemote(...).amountInfrom the source router, executes exact-input swap calls, then:actualOut < requiredOut, pulls the shortfall from the rebalancer inoutputTokenrequiredOutto the destination routerAccepted tradeoffs
TokenRouterscaleNumerator/scaleDenominatormath.recipientis intentionally ignored; the enrolleddestinationRouterpassed by the rebalancer is authoritative.Ergonomics
pendingRebalance()isEnrolledDestination(...)requiredOut(...)Trust assumptions
target/allowanceTargetallowlists.rebalance(localDomain, amountIn, bridge)is allowed to execute.Security / accounting properties
requiredOutor the call reverts.outputToken.outputToken.pending.amountIn.inputToken != outputToken.Tests
Local:
forge test --match-path test/token/SwapRebalancingBridge.t.solforge test --match-path test/token/SwapRebalancingBridge.fork.t.solCoverage includes:
recipientminAmountOutrevert