fix(bundle): resolve fill repayment against the resolved chain and bundle-end block - #1523
fix(bundle): resolve fill repayment against the resolved chain and bundle-end block#1523droplet-rl wants to merge 2 commits into
Conversation
…ndle-end block verifyFillRepayment had two block-height/repayment-chain inconsistencies: - _repaymentAddressNeedsToBeOverwritten checked relayer validity against the fill's originally-requested repaymentChainId instead of the resolved repaymentChainId returned by _getRepaymentChainId (which falls back to the origin chain for lite chains or a disabled/invalid repayment route). A relayer whose address family matched the requested chain but not the resolved fallback skipped the overwrite and was returned as repayable, then silently dropped downstream in updateBundleFillsV3 with no refund and no entry in bundleUnrepayableFillsV3. It now validates against the resolved chain. - areTokensEquivalent was called without a hub block, so it resolved at the client's current sync head rather than the bundle's mainnet end block. A SetPoolRebalanceRoute landing after the bundle end block could flip the answer, making the resolved repayment chain non-deterministic across validators for the same fill in the same bundle. It now passes bundleEndBlockForMainnet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e2e0fd071
ℹ️ 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".
| // origin chain (lite chain, or a disabled/invalid repayment route), and a relayer address whose family | ||
| // matches the requested chain but not the resolved fallback would otherwise skip the overwrite and be | ||
| // returned as repayable, only to be silently dropped later in updateBundleFillsV3. | ||
| if (_repaymentAddressNeedsToBeOverwritten(fill, repaymentChainId)) { |
There was a problem hiding this comment.
Honor destination disablement before switching repayment
When the destination chain becomes disabled between the bundle start and end blocks, and an address is valid on that EVM destination but not on the resolved SVM origin, this new resolved-chain check enters the overwrite path; areTokensEquivalent can then switch repaymentChainId back to the disabled destination because it checks only token routes. Later, getRefundInformationFromFill resolves the disabled chain back to the origin, and updateBundleFillsV3 silently drops the now-EVM relayer as invalid on that origin instead of recording the fill as unrepayable. The destination switch must also honor getDisabledChainsForBlock(bundleEndBlockForMainnet) or revalidate the final resolved chain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 89c427d.
Traced it through: areTokensEquivalent only inspects PoolRebalanceRoutes, so the destination switch skipped the getDisabledChainsForBlock gate that _getRepaymentChainId applies to the requested chain. Downstream, getRefundInformationFromFill re-runs _getRepaymentChainId, sees the disabled destination and re-resolves to the origin chain — and since the msg.sender assigned during the switch is only guaranteed valid on the destination chain, an SVM origin means updateBundleFillsV3's isValidOn guard at BundleDataClient.ts:102 drops the fill with no refund and no bundleUnrepayableFillsV3 entry. Same silent drop this PR set out to fix, reached by a different path.
Fix adds the disabled check to the destination switch. When it fails we fall through to the existing !chainIsEvm(repaymentChainId) branch, which returns undefined and records the fill as unrepayable — the correct outcome.
Worth noting for anyone reading later: the disabled gate was the only missing piece. areTokensEquivalent already implies _repaymentChainTokenIsValid for the destination chain, because getL2TokenForL1TokenAtBlock returns undefined for a zeroed mapping, which is exactly what l2TokenEnabledForL1TokenAtBlock checks. So with this the resolved chain is now idempotent under downstream re-resolution in every branch.
Two regression tests added in test/FillUtils.ts; both fail on the parent commit (16 passing, 2 failing) and pass with the fix:
- destination disabled + EVM resolved chain → repayment stays on the resolved chain instead of moving to the disabled destination
- destination disabled + SVM resolved chain → returns
undefinedrather than a fill that gets dropped downstream
Codex review catch. verifyFillRepayment's destination-chain switch was gated only on areTokensEquivalent(), which inspects PoolRebalanceRoutes and nothing else. The requested repayment chain goes through an additional disabled-chain gate in _getRepaymentChainId, but the destination fallback skipped it, so the switch could land on a chain that was disabled at the bundle end block. That switch is then undone downstream: getRefundInformationFromFill re-runs _getRepaymentChainId, sees the disabled chain and re-resolves it to the origin chain. The msg.sender assigned during the switch is only guaranteed valid on the destination chain, so when the origin chain is SVM the now-EVM relayer fails updateBundleFillsV3's isValidOn() guard and the fill is silently dropped -- no refund, and no entry in bundleUnrepayableFillsV3. This is the same drop the parent commit set out to fix, reached by a different path. Apply the disabled check to the destination chain as well. When it fails we fall through to the existing !chainIsEvm(repaymentChainId) branch, which returns undefined and correctly records the fill as unrepayable. areTokensEquivalent() already implies _repaymentChainTokenIsValid() for the destination chain (getL2TokenForL1TokenAtBlock returns undefined for a zeroed mapping), so the disabled gate was the only piece missing to make the resolved chain idempotent under downstream re-resolution. Also reuse the existing isChainDisabledAtBlock() helper rather than inlining the configStoreClient lookup twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the Codex review — the P1 was real, fixed in 89c427d. The finding: That switch then gets undone downstream: The fix: apply the disabled check to the destination switch too. When it fails we fall through to the existing One thing worth recording: the disabled gate was the only missing piece. Verification:
Posted the detail on the inline thread and tagged |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Heads up that this verdict is against a stale commit — it names My Nothing new to change; just re-running against the right diff. @codex review |
|
This "no major issues" verdict doesn't actually clear the fix — it's against a stale commit. Codex names
What happened: I posted Two things worth flagging rather than glossing over:
No code changes needed. I re-tagged Practical note for future runs: tagging an automated reviewer in the same turn that produces the commit will always race the checkpoint push. The tag needs to come from a turn after the push lands. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex re-reviewed the right commit this time — State of the PR:
Not tagging Ready for a human reviewer. I won't merge it. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Duplicate run — no action needed, and I won't restate the summary already posted above. This is the third Codex verdict on the PR and the second on
Same commit and same result as 00:51:53Z. Most likely my first PR state is unchanged from my last update: head |
|
No action taken, as requested. The PR remains at Checks
|
|
One clarification worth leaving on the record, then I'm done commenting here. The
So no stray files reached the branch, and nothing needs cleaning up. Flagging it only so the warning above isn't read as this PR carrying accidental dependency changes. Closing the loop on my side: the last several comments have been bot round-trips on an unchanged commit, and my replies appear to be triggering further Codex runs. I won't reply to additional automated verdicts on |
Two block-height / repayment-chain consistency fixes in
verifyFillRepayment(BundleDataClient/utils/FillUtils.ts):Relayer validity checked against the requested, not resolved, repayment chain.
_getRepaymentChainIdresolves the repayment chain (falling back to the origin chain for lite chains, or for a disabled/invalid repayment route), but_repaymentAddressNeedsToBeOverwrittenevaluated!fill.relayer.isValidOn(fill.repaymentChainId)against the fill's originally-requested chain. When the relayer address family matches the requested chain but not the resolved fallback (e.g. an EVM relayer on a lite-chain deposit that resolves back to a Solana origin), the overwrite was skipped,fill.repaymentChainIdwas set to the incompatible resolved chain, and the fill was returned as repayable — then silently discarded inupdateBundleFillsV3(isValidOn(repaymentChainId)guard) with no refund and no entry inbundleUnrepayableFillsV3. Now validated against the resolvedrepaymentChainId, so such fills are correctly routed to the unrepayable set.Token equivalence read at the wrong block.
areTokensEquivalent(...)was called without the optional hub-block argument, so it resolved at the client'slatestHeightSearchedrather than the bundle's mainnet end block. ASetPoolRebalanceRoutelanding between the bundle end block and a validator's head could flip equivalence, making the resolved repayment chain differ across validators for the same fill in the same bundle. Now passesbundleEndBlockForMainnet, consistent with the rest of the file.Typecheck (
tsc --project tsconfig.build.json), prettier and eslint pass.