improve(InventoryClient): Broaden repayment eligibility for hub-origin deposits#3528
improve(InventoryClient): Broaden repayment eligibility for hub-origin deposits#3528droplet-rl wants to merge 2 commits into
Conversation
…n deposits For a deposit originating on the hub chain, the input liquidity lands on the hub, so the relayer can be repaid on any chain with a pool rebalance route for the token and have the hub source the rebalance cheaply. Enumerate those chains and add them to the repayment-eligibility evaluation; the existing per-chain allocation check keeps only the under-allocated ones, and LP fee differences are priced into downstream profitability selection. Previously, hub-origin deposits could only be repaid on the origin (hub) or destination chain. When the destination was over-allocated and the hub hit origin-commitment limits, such deposits were left unfilled even though the relayer was under-allocated elsewhere. This reuses the cheap in-memory allocation check rather than the running-balance computation removed in #3520. The guard is simply `originChainId === hubChainId`: forceOriginRepayment cannot be set at that point because a hub origin is always an unmetered fast-rebalance source, so the forced-origin path short-circuits earlier. Also fixes a stale batchComputeLpFees comment referencing the removed slow-withdrawal chains. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9eebb5ce3
ℹ️ 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".
| const broadenedCandidateChains = | ||
| originChainId === hubChainId | ||
| ? this.getRepaymentChainsForL1Token(l1Token).filter((chainId) => chainId !== destinationChainId) | ||
| : []; |
There was a problem hiding this comment.
Include destination in hub-origin swap broadening
For supported swap deposits, the standard destination path does not handle the destination chain because canTakeDestinationChainRepayment() only returns true when the input and output tokens are equivalent. This unconditional chainId !== destinationChainId filter therefore drops the destination from the broadened hub-origin candidates, so a Mainnet USDT -> Base USDC-style deposit will not evaluate Base as a USDT repayment route even when Base has a PoolRebalanceRoute and is the only under-allocated/profitable chain, causing the fill to fall back to other chains or be skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
canTakeDestinationChainRepayment() does not gate on input/output equivalence — it returns true whenever the input token's L1 mapping has a pool-rebalance route on the destination chain (l2TokenEnabledForL1TokenAtBlock(l1Token, destinationChainId)), regardless of the output token. So for Mainnet USDT -> Base USDC where Base has a USDT PoolRebalanceRoute, canTakeDestinationChainRepayment is true and the standard canEvaluateDestination path (canTakeDestinationChainRepayment && _l1TokenEnabledForChain(l1Token, dest)) already evaluates Base.
That standard predicate (config AND route on the destination) is exactly the predicate getRepaymentChainsForL1Token() uses for membership, so the destination is never a member of the broadened set that the standard path skips. The chainId !== destinationChainId filter only avoids double-counting the destination in the candidate list; it cannot silently drop an otherwise-eligible route. (Conversely, if Base lacked a USDT route or config, it would be excluded from both the standard and broadened sets — correctly, since the relayer can't manage USDT inventory there.) Leaving this as-is.
| // For a hub-origin deposit the input liquidity lands on the hub, so the relayer can be repaid on any chain with | ||
| // a pool rebalance route for this token and have the hub source the rebalance cheaply. Surface them all here so | ||
| // their LP fees are precomputed; determineRefundChainId() applies the allocation check. forceOriginRepayment is | ||
| // necessarily false at this point (it returns above). | ||
| if (originChainId === this.hubPoolClient.chainId) { |
There was a problem hiding this comment.
Update repayment docs for broadened hub-origin chains
AGENTS.md requires relevant AGENTS.md/README.md files to be updated in the same change whenever runtime behavior changes. This new hub-origin branch changes the repayment eligibility/runtime flow, but the repository docs remain stale (for example, docs/repayment-eligibility.md still describes possible chains as only origin/destination/override/hub and candidate evaluation as origin/destination), so operators debugging these fills will be following incorrect rules; please update the relevant README/docs alongside this behavior change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 5f81d4a. Updated docs/repayment-eligibility.md to document the broadened hub-origin candidate set: the new getRepaymentChainsForL1Token() bullet under getPossibleRepaymentChainIds(), the candidate-build step in the determineRefundChainId() walkthrough, and a dedicated "hub-origin broadening" entry under Subtle behaviors covering the rationale, the allocation-check guard, and the LP-fee mirroring.
| const repaymentToken = this.getRemoteTokenForL1Token(l1Token, chainId); | ||
| return isDefined(repaymentToken) && this.hubPoolClient.l2TokenHasPoolRebalanceRoute(repaymentToken, chainId); |
There was a problem hiding this comment.
Handle alias repayment tokens when broadening
When the inventory config for an L1 token is an alias config, the pool-rebalance route may be on a configured alias token (for example USDC.e/USDbC) rather than the single canonical token returned here. Because this helper probes only getRemoteTokenForL1Token(), hub-origin broadening silently omits otherwise eligible under-allocated chains for those alias-token routes and their LP fees are never precomputed; this recreates the no-eligible-chain behavior for common USDC-style configurations even though getRemoteTokensForL1Token() can enumerate the configured repayment tokens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The helper deliberately mirrors the canonical-token resolution that the downstream allocation loop in determineRefundChainId() already uses: for each ranked non-origin chain the loop resolves repaymentToken = getRemoteTokenForL1Token(l1Token, chainId) (the same singular/canonical lookup) and then asserts l2TokenHasPoolRebalanceRoute(repaymentToken, chainId).
If getRepaymentChainsForL1Token() instead enumerated alias tokens via getRemoteTokensForL1Token() and admitted a chain whose route lives only on a non-canonical alias (e.g. USDC.e/USDbC), that chain would enter the loop, the loop would still resolve the canonical token (which has no route), and the assert would throw — turning a silent omission into a hard crash. So the suggested change is unsafe unless the loop's repayment-token resolution is reworked to pick the alias that actually carries the route.
That canonical-only resolution is pre-existing across the entire origin/destination allocation path, not introduced by this PR, so broadening alias-token handling is a separate change with its own test surface. I'm keeping the helper aligned with the loop here, and have noted the alias limitation for a follow-up.
Update the InventoryClient repayment-eligibility reference to reflect the broadened candidate set for hub-origin deposits, per the documentation maintenance requirement in AGENTS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @codex — reviewed all three suggestions. Summary: 1. Update repayment docs (P2) — fixed in 5f81d4a. 2. Include destination in hub-origin swap broadening (P2) — declined (false positive). 3. Handle alias repayment tokens (P2) — declined for this PR. The helper intentionally mirrors the canonical-token resolution the downstream allocation loop already uses ( Per-thread replies left inline. The only diff change is the docs commit (no logic change), so the broadening behavior and existing tests are unaffected. |
Summary
Testing
|
| if (originChainId === this.hubPoolClient.chainId) { | ||
| const l1Token = this.getL1TokenAddress(inputToken, originChainId); | ||
| if (isDefined(l1Token)) { | ||
| this.getRepaymentChainsForL1Token(l1Token).forEach((chainId) => chainIds.add(chainId)); |
There was a problem hiding this comment.
@droplet-rl Do we subsequently dedup here? It'd be nice to avoid evaluating the same chainIds repeatedly.
|
This docs update is already part of this PR — no separate change is needed. I addressed the original "update repayment docs" review comment in the prior round with commit For the record, the separate commit referenced above ( |
| // their LP fees are precomputed; determineRefundChainId() applies the allocation check. forceOriginRepayment is | ||
| // necessarily false at this point (it returns above). | ||
| if (originChainId === this.hubPoolClient.chainId) { | ||
| const l1Token = this.getL1TokenAddress(inputToken, originChainId); |
There was a problem hiding this comment.
Given that the originChainId is known to be the hubPool chain, we already know that inputToken is equal to l1Token. Therefore, l1Token lookup is redundant. Agree?
|
Thanks for the review @pxrl. I replied inline on the Short version: none of the four methods throw on real inputs. No code change for now; I'd lean against a try/catch here since it'd be inconsistent with the surrounding unguarded calls and could swallow a real misconfig — but happy to add one if you'd prefer. |
Motivation
For a deposit originating on the hub chain, the input liquidity lands on the hub — the rebalancing center. The relayer can therefore be repaid on any chain with a pool rebalance route for the token and have the hub source that rebalance cheaply. Today, hub-origin deposits are only evaluated for repayment on the origin (hub) and destination chains.
This surfaced on a USDT (Mainnet) → USDC (Base) deposit that went unfilled: the destination (Base) was over-allocated and the hub hit origin-commitment limits, so there was no acceptable repayment chain — even though the relayer was under-allocated on other chains it could have rebalanced into.
Change
InventoryClient.getRepaymentChainsForL1Token()— new helper enumerating every enabled L2 chain that has both inventory token config and a pool rebalance route for the L1 token.determineRefundChainId()— for hub-origin deposits, append those chains to the candidate set. The existing per-chain allocation check (expectedPostRelayAllocation <= effectiveTargetPct) keeps only under-allocated chains, so broadening can only add safe targets. Destination stays on the standard path; the hub remains the fallback.getPossibleRepaymentChainIds()— mirror the broadened set sobatchComputeLpFeesprecomputes an LP fee per candidate. LP-fee differences (i.e. the cost of rebalancing to a third chain) are therefore priced into downstream profitability selection — an unprofitable third-chain repayment is filtered out.Notes
originChainId === hubChainId.forceOriginRepaymentcannot be set at that point — a hub origin is always an unmetered fast-rebalance source, soisUnmeteredFastRebalance()is true and the forced-origin path short-circuits earlier indetermineRefundChainId().batchComputeLpFeescomment that still referenced the removed slow-withdrawal chains.canFastRebalanceFillrule. Since the hub is typically absent fromtokenConfig, that short-circuit doesn't fire and broadening runs (the real-world case here). If we later want broadening to also override an explicit hub token config, that's a follow-up.Testing
yarn build+ lint clean. New cases intest/InventoryClient.RefundChain.ts(all 46 in the file pass, no regressions):Worth a sanity check on utilisation after enabling, in the same spirit as #3520.
🤖 Generated with Claude Code