Skip to content

fix(deploy): check escrow settler is a contract; demote off core - #2183

Closed
gvladika wants to merge 5 commits into
mainfrom
feature/exsc-740-deploylifiintentescrowfacetv2-does-not-verify-the-settler
Closed

fix(deploy): check escrow settler is a contract; demote off core#2183
gvladika wants to merge 5 commits into
mainfrom
feature/exsc-740-deploylifiintentescrowfacetv2-does-not-verify-the-settler

Conversation

@gvladika

@gvladika gvladika commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Which Linear task belongs to this PR?

Fixes EXSC-740

Why did I implement it this way?

DeployLiFiIntentEscrowFacetV2.s.sol read its settler constructor arg with a bare json.readAddress(".lifiEscrowInputSettler"), with no check that the address holds code. On chains where the settler was not deployed the facet deployed anyway and bound its immutable LIFI_INTENT_ESCROW_SETTLER_V2 to a codeless address; because the binding is immutable the only remedy is a redeploy. The fix swaps the bare read for the inherited ScriptBase._getConfigContractAddress helper — the same checked read DeployReceiverOIF.s.sol already uses — which reverts "<key> in file <path> is not a contract" when the address has no code, failing the deploy loudly at the right moment.

That check surfaced a deeper mismatch. LiFiIntentEscrowFacetV2 was listed in config/global.jsoncoreFacets, so deployCoreFacets.sh deploys it unconditionally on every network — but its constructor requires an external settler contract that is deployed on only 12 chains. That mismatch is exactly how the facet came to be deployed against a codeless settler on chains that lack it (e.g. jovay, whose own CORE_FACET_EXEMPTIONS entry documented "Intent escrow settlers are not deployed on Jovay"). The new check turns that latent over-deploy into a hard failure — first caught by deploy-smoke-test, which runs a full core-facet deploy on a throwaway anvil where no settler can exist.

The right home for a facet that can only deploy where its external prerequisite exists is the per-network _targetState.json path, not the deploy-everywhere core path. The facet is already pinned in _targetState.json on exactly the 12 chains where the settler is deployed, so moving it off coreFacets changes nothing on those chains: the standard non-core deploy-and-cut flow (its existing UpdateLiFiIntentEscrowFacetV2.s.sol + target state) maintains it there, identical to every other prerequisite-gated facet (AcrossFacetV4, StargateFacetV2, ReceiverOIF, …). localanvil and the previously grandfathered chains simply never deploy it. Concretely this PR removes the facet from coreFacets and removes the two now-obsolete CORE_FACET_EXEMPTIONS entries — those existed only to suppress the core health check on chains without the settler, and healthCheckInvariants.test.ts enforces that every exemption targets a facet that is in coreFacets, so they are removed together. Health-check coverage on the 12 production chains is unchanged: the facet is now verified as a non-core facet via target state.

This reverses the "core going forward" designation from V2-227/#1997: a new intent chain must now be added to _targetState.json explicitly rather than picking the facet up automatically — the correct model for a facet whose constructor can only bind where its settler is deployed. Two things intentionally left out of scope: jovay's target state still lists the facet even though its settler is codeless there (pre-existing data from the original over-deploy — a separate backfill/cleanup), and the audit of other script/deploy/facets/ scripts for the same unchecked-readAddress pattern found the escrow facet was the only offender (the remaining bare reads resolve EOAs or sentinel addresses, not contracts).

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

@lifi-action-bot
lifi-action-bot marked this pull request as draft August 7, 2026 07:09
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The deployment script now uses shared configuration lookup for the settler address. LiFiIntentEscrowFacetV2 is removed from core-facet registration and health-check exemptions.

Changes

LiFi escrow deployment

Layer / File(s) Summary
Use shared settler lookup
script/deploy/facets/DeployLiFiIntentEscrowFacetV2.s.sol
The deployment script removes direct JSON parsing and uses _getConfigContractAddress to resolve the settler address.
Remove facet registration and exemptions
config/global.json, script/deploy/healthCheckInvariants.ts
LiFiIntentEscrowFacetV2 is removed from coreFacets and CORE_FACET_EXEMPTIONS.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the settler contract validation fix and the facet's removal from the core deployment path.
Description check ✅ Passed The description includes the Linear task, implementation rationale, scope, deployment impact, and completed review checklist items.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/exsc-740-deploylifiintentescrowfacetv2-does-not-verify-the-settler

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gvladika
gvladika marked this pull request as ready for review August 7, 2026 07:12
gvladika and others added 4 commits August 7, 2026 09:28
…oylifiintentescrowfacetv2-does-not-verify-the-settler
Make lifiEscrowInputSettler a per-network key so networks without a configured settler (incl. the smoke test's localanvil) are skipped by the deploy-requirements gate, matching how GasZipFacet and other core facets with external deps are handled. Replaces the anvil_setCode smoke shim, which forge's deploy fork did not observe. The _getConfigContractAddress check remains as the settler-has-code guard on the configured networks.
Discard the earlier smoke-test workarounds (anvil etch, per-network settler
config) and instead move LiFiIntentEscrowFacetV2 off coreFacets onto the
per-network target-state path, keeping the EXSC-740 settler-is-a-contract
check. The facet's constructor needs an external settler deployed on only 12
chains, so it cannot satisfy the core contract of deploying on every network;
deploying it everywhere is what bound the immutable settler to a codeless
address (e.g. jovay).

- config/global.json: remove LiFiIntentEscrowFacetV2 from coreFacets
- healthCheckInvariants.ts: drop the two now-obsolete CORE_FACET_EXEMPTIONS
  entries (test requires every exemption to target a coreFacets member)
- DeployLiFiIntentEscrowFacetV2.s.sol: checked _getConfigContractAddress read
- revert per-network lifiintentescrow.json + deployRequirements.json to main

Net diff vs main: the settler check plus the demotion. The facet stays
pinned in _targetState.json on its 12 chains, so the non-core deploy-and-cut
flow maintains it there unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@gvladika gvladika changed the title fix(deploy): verify settler is a contract in escrow facet v2 deploy fix(deploy): check escrow settler is a contract; demote off core Aug 7, 2026
@gvladika
gvladika enabled auto-merge (squash) August 7, 2026 09:36
@gvladika
gvladika disabled auto-merge August 7, 2026 10:51
@gvladika

gvladika commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Closing — superseded by the actual deployment model. The intent settlers are reserved, deterministically-deployed vanity addresses (same address on every chain), so deploying LiFiIntentEscrowFacetV2 against a not-yet-deployed (codeless) ref is the intended pre-deploy workflow, not a bug (see the 'new LI.FI Intent Contracts' thread in #dev-sc-review). EXSC-740's check would block that workflow, and the demotion here was only introduced to make the smoke test pass after the check broke it. EXSC-740 marked Cancelled; a follow-up ticket will track the inverse (allow deploying against codeless reserved refs).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants