improve(clients): track upcoming refunds by exact repayment token - #3598
improve(clients): track upcoming refunds by exact repayment token#3598droplet-rl wants to merge 5 commits into
Conversation
BundleDataApproxClient previously aggregated upcoming refunds per (chain, l1Token, relayer), with no record of which L2 token the refund leaf will pay out in. Consumers that need per-token attribution (e.g. the monitor's balance report) had to guess. Mirror the dataworker's repayment token resolution (getRefundInformationFromFill in the SDK): origin-chain repayments pay the deposit's input token; all other repayments pay the l1 token's pool-rebalance-route token on the repayment chain. Refunds are now keyed internally by that payout token and getUpcomingRefunds accepts an optional repaymentToken filter. Chain-level queries are unchanged. The serialized BundleDataState keeps its flat per-relayer shape: export flattens the breakdown and import keys chain-level sums under a sentinel token, so Redis handover state stays compatible across versions in both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: e3465d9bc1
ℹ️ 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 repaymentToken = | ||
| repaymentChainId === originChainId ? inputToken : this.getRepaymentTokenForChain(l1Token, repaymentChainId); | ||
| if (!isDefined(repaymentToken)) { | ||
| // No pool rebalance route to the repayment chain: the dataworker cannot repay there either. | ||
| return; |
There was a problem hiding this comment.
For fills whose requested repayment chain is invalid (for example, the input token lacks a pool-rebalance route from its origin, or the selected repayment chain has no route), the SDK path referenced here falls back to origin-chain repayment in inputToken via _getRepaymentChainId; this logic instead trusts the raw repaymentChainId, so it either buckets the pending refund under the wrong remote token or drops it entirely when repaymentToken is undefined. In that scenario the new per-token filter, and in the no-route case the existing chain-level sums, disagree with the refunds the dataworker will actually include.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in d1edf77.
getApproximateRefundsForToken now resolves the effective repayment chain/token through a new resolveRefundInformation() helper that mirrors the SDK's _getRepaymentChainId()/getRefundInformationFromFill(): the requested repayment chain is honored only when the input token has a pool rebalance route on its origin chain, that route's L1 token is also routed to the repayment chain, and the repayment chain is not disabled. Otherwise the refund falls back to the origin chain in the input token instead of being bucketed under an invalid chain (pre-existing chain-level behavior) or dropped (the new per-token filter).
Two related corrections that fell out of this:
- The
fromBlocksexecution-state filter now uses the resolved repayment chain, since that's where the refund leaf will actually execute. - Slow fills are skipped explicitly (they pay the recipient out of a slow fill leaf and produce no relayer refund); previously they were only invisible by accident because their hardcoded
repaymentChainId = 0bucket was never queried, and the origin fallback would otherwise have started mis-counting them.
Added regression tests for all three fallback triggers (no route to repayment chain, input token without a route, disabled repayment chain) plus slow-fill exclusion.
…unds Mirror the SDK's _getRepaymentChainId()/getRefundInformationFromFill() resolution when counting upcoming refunds: a fill's requested repayment chain is only honored when the input token has a pool rebalance route on its origin chain, that route's L1 token is also routed to the repayment chain, and the repayment chain is not disabled. Otherwise the refund is attributed to the origin chain in the input token, matching what the dataworker will actually do, instead of being bucketed under an invalid chain or dropped. Slow fills are now skipped explicitly since they pay the recipient out of a slow fill leaf and produce no relayer refund. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the Codex review feedback in d1edf77. The one inline finding (P2: refund bucketing trusted the raw @codex please re-review the updated diff. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3465d9bc1
ℹ️ 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 repaymentToken = | ||
| repaymentChainId === originChainId ? inputToken : this.getRepaymentTokenForChain(l1Token, repaymentChainId); |
There was a problem hiding this comment.
When a fast fill requests a repayment chain that the dataworker rejects (for example the input token lacks a pool-rebalance route, the requested chain lacks the route, or that chain is disabled), the SDK's _getRepaymentChainId() falls back to repaying on the origin chain in the input token. This code derives the repayment token from the raw repaymentChainId and later buckets/filters with that same raw chain, so those fills are either skipped when getRepaymentTokenForChain() returns undefined or attributed to the invalid chain/token instead of the actual origin-chain refund.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a duplicate of the finding in review #4731086224 — both reviews ran against the pre-fix commit e3465d9bc1. It's already addressed in d1edf77: resolveRefundInformation() mirrors the SDK's _getRepaymentChainId() fallback for all three triggers named here (input token without a pool rebalance route, no route to the requested repayment chain, disabled repayment chain), attributing the refund to the origin chain in the input token. The fromBlocks filter also uses the resolved chain, and slow fills are skipped explicitly. Regression tests cover each case.
|
This second review is a duplicate of the first — both ran against the pre-fix commit |
| * routed to the repayment chain, and the repayment chain is not disabled. Otherwise the dataworker | ||
| * falls back to repaying on the origin chain in the input token. | ||
| */ | ||
| protected resolveRefundInformation(fill: FillWithBlock): { repaymentChainId: number; repaymentToken: Address } { |
There was a problem hiding this comment.
s/refund/repayment. Refunds is a misnomer in the context of relayers being repaid by the protocol for valid fills. Refunds is relevant in the context of a depositor being refunded for an unfilled deposit.
There was a problem hiding this comment.
Renamed in b1ba65e: resolveRefundInformation → resolveRepaymentInformation, plus the new comments/locals this PR introduced now say repayment when describing relayers being repaid for fills. I left the pre-existing exported surface (getUpcomingRefunds/upcomingRefunds/getApproximateRefundsForToken) and contract-level terms like "refund leaf" (RelayerRefundLeaf/relayerRefundRoot) as-is to avoid churning the API in this PR — happy to do that rename in a follow-up if you'd like it applied across the client.
Rename resolveRefundInformation to resolveRepaymentInformation and adjust new comments: relayers are repaid by the protocol for valid fills, while "refund" refers to a depositor being refunded for an unfilled deposit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in b1ba65e — renamed the new helper to |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Motivation
Precursor to #3592, split out per Paul in Slack so the formatting PR stays formatting-only.
The monitor's balance report is gaining a per-row
Pending Repaymentline, which requires knowing which L2 token each upcoming refund will actually be paid in.BundleDataApproxClientpreviously aggregated upcoming refunds per (chain, l1Token, relayer) with no payout-token breakdown, so per-token consumers had to guess — and guessing the canonical token is wrong for origin-chain repayments of non-canonical inputs (e.g. a USDC.e-input deposit repaid on its origin chain pays the relayer in USDC.e).Changes
getRefundInformationFromFillin the SDK): origin-chain repayments pay the deposit's input token; all other repayments pay the l1 token's pool-rebalance-route token on the repayment chain (hubPoolClient.getL2TokenForL1TokenAtBlock).getUpcomingRefundsaccepts an optionalrepaymentTokenfilter. Omitted, it returns the same chain-level sums as before —InventoryClientandRunningBalanceUtilsbehavior is unchanged.BundleDataStateshape is deliberately unchanged:export()flattens the per-token breakdown to the historical per-relayer sums, andimport()keys chain-level sums under a sentinel token. Redis handover state stays compatible with older/newer relayer versions in both directions; the breakdown is simply unavailable on imported state (its only consumers are chain-level).No README/AGENTS updates needed: internal client behavior only; the single interface change is an optional parameter, and the serialized state shape is unchanged.
Testing
yarn hardhat test test/BundleDataApproxClient.ts test/InventoryClient.InventoryRebalance.ts test/InventoryClient.RefundChain.ts test/Monitor.ts— 74 passingyarn build, eslint + prettier clean on touched files🤖 Generated with Claude Code