Skip to content

fix(bundle): resolve fill repayment against the resolved chain and bundle-end block - #1523

Open
droplet-rl wants to merge 2 commits into
masterfrom
droplet/fill-repayment-chain
Open

fix(bundle): resolve fill repayment against the resolved chain and bundle-end block#1523
droplet-rl wants to merge 2 commits into
masterfrom
droplet/fill-repayment-chain

Conversation

@droplet-rl

Copy link
Copy Markdown
Contributor

Two block-height / repayment-chain consistency fixes in verifyFillRepayment (BundleDataClient/utils/FillUtils.ts):

  1. Relayer validity checked against the requested, not resolved, repayment chain. _getRepaymentChainId resolves the repayment chain (falling back to the origin chain for lite chains, or for a disabled/invalid repayment route), but _repaymentAddressNeedsToBeOverwritten evaluated !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.repaymentChainId was set to the incompatible resolved chain, and the fill was returned as repayable — then silently discarded in updateBundleFillsV3 (isValidOn(repaymentChainId) guard) with no refund and no entry in bundleUnrepayableFillsV3. Now validated against the resolved repaymentChainId, so such fills are correctly routed to the unrepayable set.

  2. Token equivalence read at the wrong block. areTokensEquivalent(...) was called without the optional hub-block argument, so it resolved at the client's latestHeightSearched rather than the bundle's mainnet end block. A SetPoolRebalanceRoute landing 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 passes bundleEndBlockForMainnet, consistent with the rest of the file.

Typecheck (tsc --project tsconfig.build.json), prettier and eslint pass.

…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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 undefined rather 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>
@droplet-rl

Copy link
Copy Markdown
Contributor Author

Addressed the Codex review — the P1 was real, fixed in 89c427d.

The finding: verifyFillRepayment's destination-chain switch was gated only on areTokensEquivalent(), which inspects PoolRebalanceRoutes and nothing else. The requested repayment chain goes through an additional getDisabledChainsForBlock gate inside _getRepaymentChainId, but the destination fallback skipped it — so the switch could land on a chain disabled at the bundle end block.

That switch then gets undone downstream: getRefundInformationFromFill re-runs _getRepaymentChainId, sees the disabled destination and re-resolves to the origin chain. The msg.sender assigned during the switch is only guaranteed valid on the destination chain, so when origin is SVM the now-EVM relayer trips updateBundleFillsV3's isValidOn guard (BundleDataClient.ts:102) and the fill is silently dropped — no refund, no bundleUnrepayableFillsV3 entry. Same silent drop this PR set out to fix, reached by a different path.

The fix: apply the disabled check to the destination switch too. When it fails we fall through to the existing !chainIsEvm(repaymentChainId) branch, which returns undefined and correctly records the fill as unrepayable. Also swapped the inlined configStoreClient lookup for the existing isChainDisabledAtBlock() helper, now used at both sites.

One thing worth recording: the disabled gate was the only missing piece. areTokensEquivalent already implies _repaymentChainTokenIsValid for the destination chain — getL2TokenForL1TokenAtBlock returns undefined for a zeroed mapping, which is exactly what l2TokenEnabledForL1TokenAtBlock checks. So the resolved repayment chain is now idempotent under downstream re-resolution in every branch, which is the invariant this PR is really about.

Verification:

  • 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 with an EVM resolved chain (repayment stays put) and with an SVM resolved chain (returns undefined).
  • test/FillUtils.ts 18 passing; BundleDataClient.cache / ConfigStoreClient / HubPoolClient.DepositToDestinationToken / SpokePoolClient.ValidateFill 36 passing; full build and lint-check clean.

Posted the detail on the inline thread and tagged @codex for a re-review of the updated diff.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Pushed 89c427d addressing the P1 above (disabled destination chain bypassing the repayment-chain gate), plus two regression tests that fail on the parent commit.

@codex review

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

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".

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Heads up that this verdict is against a stale commit — it names 2e2e0fd071, which is the parent, not the fix.

My @codex review at 00:45:50Z raced the checkpoint that pushed 89c427d, so Codex snapshotted the pre-fix head. 89c427d is now confirmed on the remote branch head and contains the fix for the P1 (destination-chain switch bypassing the disabled-chain gate). Worth noting the two verdicts are on the same commit 2e2e0fd071 — the earlier P1 and this "no major issues" — so this one should not be read as retracting that finding either way.

Nothing new to change; just re-running against the right diff.

@codex review

@droplet-rl

Copy link
Copy Markdown
Contributor Author

This "no major issues" verdict doesn't actually clear the fix — it's against a stale commit.

Codex names 2e2e0fd071, which is the parent commit. My fix is 89c427d. Verified:

  • PR head is 89c427ddc0fd57e842ff83e048d9c3ea5aa0c554 and remote droplet/fill-repayment-chain matches it.
  • Fetched FillUtils.ts at that remote SHA — the fix is there (isChainDisabledAtBlock import plus both call sites, lines 16/144/205). So the push landed fine.

What happened: I posted @codex review at 00:45:50Z, but the connector only pushes HEAD on checkpoint, which ran after my turn exited. Codex snapshotted the PR head at trigger time, when it was still 2e2e0fd071. It then reported at 00:48:02Z on that pre-fix diff. My mistake — I tagged for re-review before the commit was actually on the remote.

Two things worth flagging rather than glossing over:

  1. Codex has never seen 89c427d. Its P1 remains addressed in my judgment and by the regression tests (both fail on the parent, pass on the fix), but that's my verification, not an independent re-review.
  2. Both Codex verdicts — the original P1 and this "no major issues" — are on the same commit 2e2e0fd071. So the second doesn't retract the first; it's just non-deterministic output on an identical diff. Nobody should read it as Codex withdrawing the finding.

No code changes needed. I re-tagged @codex review now that 89c427d is confirmed on the remote head, so the next pass should be against the real diff.

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.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 89c427ddc0

ℹ️ 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".

@droplet-rl

Copy link
Copy Markdown
Contributor Author

Codex re-reviewed the right commit this time — 89c427ddc0 — and found no major issues. That closes the loop on the P1.

State of the PR:

  • Codex: P1 (disabled destination chain bypassing the repayment-chain gate) reported on 2e2e0fd07, fixed in 89c427d, re-reviewed clean against that commit.
  • CI: Builds, Lint, Test, dependency-review all passing on 89c427d.
  • Branch: local and remote both at 89c427ddc0, working tree clean — nothing unpushed.
  • Mergeable, awaiting human review (REVIEW_REQUIRED).

Not tagging @codex again, since there are no new changes for it to look at.

Ready for a human reviewer. I won't merge it.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 89c427ddc0

ℹ️ 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".

@droplet-rl

Copy link
Copy Markdown
Contributor Author

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 89c427ddc0:

time commit verdict
00:48:02Z 2e2e0fd071 (parent) no issues — stale, raced the push
00:51:53Z 89c427ddc0 no issues
00:54:10Z 89c427ddc0 no issues (this one)

Same commit and same result as 00:51:53Z. Most likely my first @codex review — the one that raced the checkpoint push — got queued and re-ran against the new head, landing alongside the explicit re-tag I sent afterward. Two tags, so two runs on the final commit.

PR state is unchanged from my last update: head 89c427ddc0, all four checks green, mergeable, REVIEW_REQUIRED. Still open for a human reviewer; I won't merge it.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

@chatgpt-codex-connector

Copy link
Copy Markdown

No action taken, as requested. The PR remains at 89c427ddc0fd57e842ff83e048d9c3ea5aa0c554; no commit or follow-up pull request was created.

Checks

  • git rev-parse HEAD
  • ⚠️ git status --short --branch (the working tree already contains unrelated modifications to package.json, yarn.lock, .yarn/, and .yarnrc.yml; they were left untouched)

View task →

@droplet-rl

Copy link
Copy Markdown
Contributor Author

One clarification worth leaving on the record, then I'm done commenting here.

The package.json / yarn.lock / .yarn/ / .yarnrc.yml modifications noted above are in Codex's own sandbox working tree, not in this PR. Verified from my side:

  • PR changes exactly two files: src/clients/BundleDataClient/utils/FillUtils.ts (+28/-9) and test/FillUtils.ts (+47/-0).
  • git diff --name-only 3792b94..HEAD matches those two files — no packaging or lockfile changes in either commit.
  • My working tree is clean at 89c427ddc0.

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 89c427ddc0 — they carry no new information. Final state: two files changed, all four checks green, mergeable, awaiting human review. I won't merge it.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant