Clamp L1->L2 OFT rebalances to the path's remaining capacity - #3765
Clamp L1->L2 OFT rebalances to the path's remaining capacity#3765droplet-rl wants to merge 3 commits into
Conversation
Metered OFT paths (Stargate pools, legacy-mesh adapters) revert rather than size down when a send exceeds the destination path's available capacity, so an over-capacity rebalance fails to construct instead of moving what the path can take. Implement the existing getMaxL1ToL2TransferAmount hook on OFTBridge, reading capacity from quoteOFT's oftLimit.maxAmountLD - the only field that reports it on every path type, since Stargate pools also cap oftReceipt.amountSentLD but legacy-mesh adapters echo the requested amount there, and vanilla OFTs report type(uint64).max to mean no limit. The rebalance loop already clamps to this hook, so an over-capacity deficit is chunked across runs. Also skip a rebalance clamped to zero rather than initiating a transfer that pays full per-message costs to move nothing. Co-Authored-By: Claude <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c033bc4a2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| OFT.buildSimpleSendParamEvm(recipient, this.l2ChainEid, bnZero) | ||
| ); | ||
| const maxAmountLD = BigNumber.from(oftLimit.maxAmountLD); | ||
| return maxAmountLD.gte(OFT_UNLIMITED_CAPACITY) ? undefined : maxAmountLD; |
There was a problem hiding this comment.
When a metered 18-decimal path reports legitimate capacity above 2^64 - 1 local units, this gte check misclassifies it as unlimited. For example, Plasma's OFTWethBridge inherits this method, and any available capacity above roughly 18.45 WETH returns undefined; if a requested rebalance exceeds that capacity, the InventoryClient does not clamp it and quoteSend still reverts, defeating this change for larger WETH capacities. Compare the value for equality with the vanilla-OFT sentinel instead.
Useful? React with 👍 / 👎.
| const maxTransferAmount = await this.adapterManager | ||
| .getMaxL1ToL2TransferAmount(rebalance.chainId, rebalance.l1Token) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
When one run plans multiple rebalances for the same token and destination path, each iteration reads the same pre-transfer capacity and clamps independently. This occurs, for example, when rebalanceShortfalls adds a shortfall rebalance and _getPossibleInventoryRebalances then adds an ordinary rebalance for the same route: with capacity 100 and planned amounts 60 and 80, both pass planning, but after the first confirmed send leaves capacity 40, construction of the second send still uses 80 and reverts. Track and subtract capacity already allocated to earlier same-path rebalances, or re-query immediately before each send.
Useful? React with 👍 / 👎.
|
I've used my 1 automated round on this PR, so I'm ignoring further automated-reviewer activity. To re-activate me: have a human @-mention |
`maxAmountLD` reports `type(uint64).max` to mean "no limit", but not always verbatim: the mainnet WETH messenger scales it by the shared -> local decimal conversion rate. Treating anything at or above the unscaled value as uncapped therefore misread genuine capacity above `type(uint64).max` local units as unlimited and skipped the clamp -- on an 18-decimal path that starts at only ~18.45 tokens. Match the placeholder exactly, in both encodings. Also documents that capacity is not reserved across rebalances planned in the same run, since a per-transfer ceiling and a shared pool cannot be decremented alike behind one hook. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both findings were real. P1 is fixed in 219af1b; P2 I've documented rather than fixed, reasoning below. P1 — uncapped sentinel (fixed, but not the suggested way) Correct diagnosis, and I'd independently hit the same thing while verifying this branch against mainnet. The suggested fix — compare for equality with the vanilla-OFT sentinel — is incomplete though, because
The WETH messenger scales the sentinel by the shared → local conversion rate. A strict Worth noting the bug was slightly narrower than described: Added a regression test at the misread threshold (50 WETH capacity, above the ~18.45 unscaled limit); confirmed it fails against the old P2 — reserving capacity across same-path rebalances (documented, not fixed) Also real: I didn't implement reservation because the naive version breaks the other caller. Verification: That's my one automated round used, so I won't re-summon Codex. If you'd like P2 handled in this PR rather than a follow-up, assign me and I'll pick a direction. |
There was a problem hiding this comment.
Do we have duplicate implementation here? I think I've seen very similar logic (for rounding) previously on the withdrawal path.
There was a problem hiding this comment.
Yes — mine to fix. roundAmountToSend already owned that arithmetic; extracted it as OFT.getDecimalConversionRate and pointed both at it (ee09eda), plus folded OFTBridge's two sharedDecimals fetches into one accessor. Added tests on both branches — the rounding path had none.
Left alone: the OFTBridge/OFTL2Bridge roundAmountToSend wrappers (different hierarchies, needs a mixin — pre-existing, happy to do it here or later). Capacity reading is deliberately not shared: withdrawals size down from amountSentLD, which legacy-mesh adapters don't report capacity in, hence maxAmountLD here.
The placeholder scaling added in the previous commit recomputed 10 ** (tokenDecimals - sharedDecimals), which OFT.roundAmountToSend already owned. Extract it as getDecimalConversionRate and have both use it, and fold OFTBridge's two copies of the sharedDecimals fetch-and-cache into one accessor. Behaviour-preserving: the helper returns 1 rather than a sub-unit divisor when the token carries no more precision than the path, so rounding leaves such amounts untouched as before. Covered by tests on both branches, since the rounding path (withdrawals, dataworker refund leaves, rebalancer) had none. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixed in |
2 participants
Metered OFT paths (Stargate pools, legacy-mesh adapters) revert rather than size down when a send exceeds the destination path's available capacity, so an over-capacity rebalance fails to construct instead of moving what the path can take.
OFTBridgenow implements the existinggetMaxL1ToL2TransferAmounthook, reading capacity fromquoteOFT'softLimit.maxAmountLD. That's the only field reporting capacity on every path type: Stargate pools also capoftReceipt.amountSentLD, but legacy-mesh adapters echo the requested amount there, and vanilla OFTs reporttype(uint64).maxto mean "no limit". The rebalance loop already clamps to this hook, so an over-capacity deficit is chunked across runs.Also skips a rebalance clamped to zero, rather than initiating a transfer that pays full per-message costs to move nothing.
Verified against mainnet: on a metered USDT path
oftLimit.maxAmountLDtracks the adapter's per-destination credit exactly, andquoteSendsucceeds at that amount and reverts just above it; vanilla paths reporttype(uint64).max.🤖 Generated with Claude Code