feat(healthcheck): couple facets to their companion periphery contracts - #2125
feat(healthcheck): couple facets to their companion periphery contracts#21250xDEnYO wants to merge 26 commits into
Conversation
A bridge facet only covers the source side; destination calls need its companion Receiver on the same chain. Nothing tied the two together, so a facet could be rolled out to a new chain while its Receiver was silently forgotten - which disabled Across destination calls on Robinhood. Receivers had no "must exist" coverage at any tier: non-core-facets-deployed filters on names containing "Facet", periphery-registered only checks corePeriphery/whitelistPeripheryFunctions, and receiver-executor-binding skips a Receiver whose address is absent. Declare the couplings in config/global.json -> facetPeripheryCouplings and enforce them in two places: - facet-required-periphery health-check invariant (daily sweep + new-network CI): for every facet REGISTERED ON CHAIN, assert one of its requiresAnyOf contracts is registered in the PeripheryRegistry. Triggering on on-chain facets rather than target state matters - target state was itself missing the Receiver in the Robinhood incident. - facetCompanionReminder.ts: non-fatal deploy-time nudge when the companion is absent from the network's deploy log. Non-fatal because deploying the facet before its Receiver is the normal order. requiresAnyOf (not a single name) because Across kept the handleV3AcrossMessage callback in V4, so either Receiver can service a V4 destination call. notRequiredYet records a coupling that is not active yet (OIF destination execution is unsupported today) so nobody has to rediscover it; notRequiredOn carves out individual chains, both with a mandatory reason that is printed when the check is skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds facet-periphery coupling configuration, live-facet selector resolution, expanded health-check invariants, immutable-binding validation, non-fatal deployment reminders, deploy-log helpers, and ReceiverOIF deployment metadata. ChangesFacet coupling and live-facet resolution
Health-check enforcement
Immutable binding configuration
Deployment reminders and metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…-log path readDeployLog built a path from a CLI argument without validating it, so a name like `../../.env` would traverse out of deployments/ (Aikido: potential file inclusion via ReadFile). The sibling facetRefundReminder.ts already guards the same shape with isValidContractName; mirror that with isValidNetworkName, matching the alphanumeric/-/_ network keys used in config/networks.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 1183-1225: Replace the silent `else return` in the periphery
registration check with a warning through `ctx.logWarn`, indicating that
registration could not be checked because neither a Tron RPC URL nor an EVM
public client is available; preserve the existing Tron and EVM branches
unchanged.
- Around line 1183-1204: In the Tron periphery-check loop around
callTronContract, replace the catch block’s immediate return with logic that
records the failed lookup and continues processing the remaining wanted
peripheries. Preserve the existing error logging, and ensure later required
couplings are still evaluated despite an individual RPC failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ca6d62f-c2c4-4f0c-842f-ebc69e41a881
📒 Files selected for processing (9)
.agents/rules/601-healthcheck-invariants.mdconfig/global.jsonscript/deploy/deploySingleContract.shscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.tsscript/deploy/resources/facetCompanionReminder.test.tsscript/deploy/resources/facetCompanionReminder.tsscript/deploy/shared/facetPeripheryCouplings.test.tsscript/deploy/shared/facetPeripheryCouplings.ts
…path Bot review follow-ups on facet-required-periphery. A failed registry read is not evidence of absence, but the invariant treated it as fatal or as a violation: - Tron: a single callTronContract throw returned out of the whole invariant, abandoning every companion and coupling not yet checked. One flaky read on a severity=error production invariant silently skipped unrelated couplings. - EVM: Promise.all rejected the whole batch on one failed read. - Neither branch available: returned silently, so reduced coverage was invisible in the sweep report. Now every companion is looked up independently (continue / allSettled), a failed lookup is warned and recorded as unresolved, and a coupling is only reported as a violation when at least one companion actually resolved to "not registered". A coupling whose companions all failed to resolve warns as undetermined; a missing chain client warns too. readDeployLog additionally checks the resolved path stays inside deployments/, so containment no longer rests on the name regex alone, and the CLI now says nothing for a name that is not a plain network key instead of printing a reminder naming an impossible network. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on the registry shape and contents. - Key by facet name instead of an invented coupling label. The label was never matched against anything - it only appeared in log lines - while the actual match has always been on the facet name. Keying by facet makes the lookup a direct key hit and matches how the sibling registries (deployRequirements.json, whitelistPeripheryFunctions) are keyed. Facets needing the same companion are merged back into one requirement at evaluation time, so the reporting is unchanged. - Drop the acrossV3 coupling: AcrossFacet is deprecated and listed in no network's target state. - Drop ReceiverAcrossV3 as an alternative for the V4 facets. It was allowed because Across kept the handleV3AcrossMessage signature, but accepting a deprecated contract would let it mask a missing current one. Verified against every active chain: no chain runs V4 facets with only the V3 receiver, so this tightens the check without creating a single new failure. - Activate the OIF coupling. Policy is to ship ReceiverOIF on every chain where either LiFiIntentEscrow facet is live, so the notRequiredYet marker is gone and the missing receivers are now reported as the gaps they are. Also adds a test asserting every facet sharing a coupled family's prefix is itself coupled, so a future AcrossFacetV5 cannot land unchecked - the allowlist's main failure mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the deploy log The facet-required-periphery gate resolved facet identity through deployments/<network>.json (ctx.deployedContracts) — the same deploy log this check deliberately bypasses on the periphery side (it reads getPeripheryContract on chain because the log can be incomplete). A coupled facet registered on chain but missing from the log resolved to no name, was filtered out, and its coupling was never evaluated: a silent miss on an error-severity gate. The no-unexpected-facets warning already proves on-chain facets do go missing from the log, so this was a real blind spot, not a hypothetical one. Resolve live facets from two independent sources, unioned so coverage only grows: the deploy log (address -> name) and on-chain selectors matched against compiled artifacts. A diamond maps each selector to exactly one facet, so a coupled facet is present iff some on-chain facet registers its full selector set — identity that does not depend on the log. When neither source can identify an on-chain facet absent from the log (e.g. out/ not built), surface a warning so the gate never passes silently. Addresses review feedback from Daniela on PR #2125 (EXSC-684). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctor loader loadFacetSelectorsFromArtifact composes a facet name into a file path. Names come from config/global.json (repo-controlled, not attacker input), but harden the read regardless: validate the name as a Solidity identifier and assert the resolved path stays inside out/. Mirrors readDeployLog / isValidNetworkName in the sibling facetCompanionReminder.ts. Clears the Aikido path-traversal finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…3e1c2 # Conflicts: # config/global.json
…enforce registry/log sync Review follow-ups (a, b, d) on the coupling PR: - receiver-executor-binding and receiver-owner now resolve receiver addresses from the on-chain PeripheryRegistry first, deploy log as fallback - the same union principle as facet identity. Previously both skipped any receiver missing from deployments/<network>.json, which silently exempted ReceiverOIF on mainnet, base and arbitrum from binding and ownership coverage. receiver-owner additionally now covers the bridge-specific receivers, which had no owner check anywhere. - New periphery-registry-log-sync invariant (error, production): every known periphery name (corePeriphery, whitelistPeripheryFunctions, coupling companions, receiver getter list) registered on chain must appear in the deploy log with the same address. An incomplete log is not cosmetic: it silently exempts contracts from every log-resolved check. - Deploy logs backfilled from on-chain truth: ReceiverOIF (0x761B0e8f6e80BBd23F3886663Cc071a554be37A3) verified registered with code on mainnet, base AND arbitrum (arbitrum was previously assumed missing) and added to all three logs. Verified live: all new checks green on mainnet. - Drift test ties facetPeripheryCouplings to RECEIVER_EXECUTOR_GETTERS: a new Receiver coupling without a binding check entry now fails CI instead of shipping presence-checked but binding-unchecked. - HEALTH_CHECK_EXCLUSIONS moved from a TS literal to config/healthCheckExclusions.json so per-network carve-outs are ops-editable config; existing integrity tests validate entries against real invariant names and networks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ig (EXSC-684) Review follow-up (c): contracts like ReceiverAcrossV4 bind their counterparty (SPOKEPOOL) immutably at construction. When the integration migrates and the config file moves on, presence and executor-binding checks stay green while destination calls fail against a dead counterparty - nothing compared the live binding to config. deployRequirements.json already maps each constructor arg to a config file + per-network key; entries annotated with a "getter" (the public getter exposing the bound value) are now checkable. The new immutable-bindings-match-config invariant (error, production) resolves the contract registry-first, reads the getter on chain, and compares against the config-resolved expected address. Coverage grows by annotating entries; the extra JSON key is ignored by the bash consumer (checkDeployRequirements reads named keys only). Annotated: ReceiverAcrossV4.SPOKEPOOL, ReceiverStargateV2.endpointV2 / tokenMessaging, ReceiverChainflip.chainflipVault - each getter validated against the compiled artifact in tests. Verified live on mainnet: all annotated bindings match config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/healthCheckInvariants.ts (1)
1609-1643: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMixed confirmed-absent + unresolved companions are misreported as a hard error, not a warning.
undetermined.length === requirement.requiresAnyOf.lengthonly downgrades to a warning when every companion lookup failed. IfrequiresAnyOfhas ≥2 entries and one resolves to confirmed-false while a sibling lookup merely fails (RPC blip), the requirement falls through toctx.logErroreven though the failed sibling's real status is unknown — a false positive on a production, severity="error" invariant. The one-time re-verify inexecuteInvariantreduces but doesn't eliminate this (needs the same lookup to fail twice).🔧 Proposed fix: downgrade to warning whenever any companion lookup is unresolved
- const undetermined = requirement.requiresAnyOf.filter((periphery) => - unresolved.has(periphery) - ) - if (undetermined.length === requirement.requiresAnyOf.length) { - ctx.logWarn( - `${requirement.triggeredBy.join( - ', ' - )}: could not determine whether a companion is registered (all lookups failed: ${undetermined.join( - ', ' - )})` - ) - continue - } + const undetermined = requirement.requiresAnyOf.filter((periphery) => + unresolved.has(periphery) + ) + if (undetermined.length > 0) { + ctx.logWarn( + `${requirement.triggeredBy.join( + ', ' + )}: could not fully determine companion registration (lookup failed for: ${undetermined.join( + ', ' + )})` + ) + continue + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.ts` around lines 1609 - 1643, Update the companion-status handling in the required-requirements loop so any unresolved companion lookup causes a warning instead of a hard error, including when other companions are confirmed absent. Change the condition around undetermined to check for at least one unresolved entry, preserve the existing warning message and continue behavior, and leave the registered-success path unchanged.
🧹 Nitpick comments (1)
script/deploy/healthCheckInvariants.test.ts (1)
707-953: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated mock-address constants and context-builder pattern across four describe blocks.
periphery-registry-log-sync,receiver-executor-binding registry-first resolution,receiver-owner covers bridge-specific receivers, andimmutable-bindings-match-configeach redefineRECEIVER/OTHER/DIAMOND/ZEROand a near-identicalmakeXCtxthat stubspublicClient.readContract. Extracting shared constants and a generic mock-context builder (parameterized by thereadContractbehavior) would reduce this duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.test.ts` around lines 707 - 953, The four invariant test suites duplicate mock address constants and context builders. Extract shared RECEIVER, OTHER, DIAMOND, and ZERO constants plus a generic mock context builder parameterized by readContract behavior, then update makeSyncCtx, makeBindingCtx, makeOwnerCtx, and makeBindingsCtx to reuse them while preserving each suite’s specific context fields and responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deployments/arbitrum.json`:
- Around line 70-71: Rename the ReceiverOIF configuration key to the agreed
snake_case spelling consistently in deployments/arbitrum.json:70-71,
deployments/base.json:62-63, and deployments/mainnet.json:88-89, and update
every reader atomically; alternatively, document an explicit exception for
contract-name keys.
---
Outside diff comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 1609-1643: Update the companion-status handling in the
required-requirements loop so any unresolved companion lookup causes a warning
instead of a hard error, including when other companions are confirmed absent.
Change the condition around undetermined to check for at least one unresolved
entry, preserve the existing warning message and continue behavior, and leave
the registered-success path unchanged.
---
Nitpick comments:
In `@script/deploy/healthCheckInvariants.test.ts`:
- Around line 707-953: The four invariant test suites duplicate mock address
constants and context builders. Extract shared RECEIVER, OTHER, DIAMOND, and
ZERO constants plus a generic mock context builder parameterized by readContract
behavior, then update makeSyncCtx, makeBindingCtx, makeOwnerCtx, and
makeBindingsCtx to reuse them while preserving each suite’s specific context
fields and responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86c3c6a9-2b4b-4488-b8cc-8e2aea956b28
📒 Files selected for processing (12)
config/global.jsonconfig/healthCheckExclusions.jsondeployments/arbitrum.jsondeployments/base.jsondeployments/mainnet.jsonscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.tsscript/deploy/resources/deployRequirements.jsonscript/deploy/shared/facetPeripheryCouplings.test.tsscript/deploy/shared/facetPeripheryCouplings.tsscript/deploy/shared/immutableBindings.test.tsscript/deploy/shared/immutableBindings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- config/global.json
- script/deploy/shared/facetPeripheryCouplings.test.ts
- script/deploy/shared/facetPeripheryCouplings.ts
…ments (EXSC-684) Audit: every deploy script consuming config/deploy-log addresses was compared against deployRequirements.json. Five contracts deployed with address args but no pre-deploy validation gate: ReceiverOIF, DeBridgeDlnFacet, EcoFacet, LidoWrapper, MayanFacet. All five get entries mirroring their deploy scripts' actual sources, each address arg annotated with its public getter so the immutable-bindings-match-config invariant covers them too. Verified live on mainnet: DLN_SOURCE, PORTAL, MAYAN and OUTPUT_SETTLER all match their config values on chain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a dependency (EXSC-684) deployRequirements.json contractAddresses records the forward edge (Receiver needs Executor); nothing surfaced the reverse: redeploying the Executor invalidates every deployed Receiver's immutable binding, and redeploying the ERC20Proxy cascades through the Executor to all of them. contractDependencyReminder.ts walks the reverse graph transitively and, on deploy, lists every dependent present in the network's deploy log with its path (e.g. "ReceiverAcrossV4 (via Executor)"). Non-fatal nudge in deploySingleContract.sh, same tier as facetCompanionReminder; the binding health-check invariants remain the enforcing gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cks to Tron (EXSC-684) periphery-registry-log-sync and immutable-bindings-match-config were scoped evm-only. Both gain a Tron branch (callTronContract + base58 comparison via parseTronAddressOutput/ensureTronAddress, zero guarded against both Tron encodings; candidates use getTronCorePeriphery). receiver-executor-binding stays evm-only with the reason documented inline: none of the coupled receivers exist on Tron - grow the branch with the first one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…argeted exemptions (EXSC-684)
The blanket skipHealthcheck flag (arc, robinhood, tempo) is removed entirely -
flag, INetwork field, and the early return in healthCheck.ts. Every chain now
runs every applicable invariant; genuine per-network specialties are carved out
per-invariant or per-contract with a mandatory reason:
- config/healthCheckExclusions.json becomes { invariantExclusions,
corePeripheryExemptions }. The new per-contract tier exempts a single core
periphery contract on a single network where a whole-invariant skip would
hide unrelated coverage. TokenWrapper is exempted on arc and tempo (no
native/wrap path - reasons from the networks' devNotes); the skip prints its
reason whenever it applies.
Inventory results from running the previously-skipped chains:
- robinhood: the new periphery-registry-log-sync invariant immediately found
OutputValidator and ReceiverOIF registered on chain but missing from the
deploy log, plus OutputValidator absent from whitelist.json. All three fixed
(addresses verified on chain; ReceiverOIF binds the correct Executor).
robinhood now passes its first-ever full health check (exit 0).
- tempo: 3 genuine gaps remain red BY DESIGN - ReceiverAcrossV4 deployed but
never registered (needs the timelock proposal, see PR body) and SquidFacet
in target state but not deployed. Exactly the visibility this PR exists for.
- arc: RPC unreachable from this session; CI (Mongo-fetched RPCs) may still
reach it - the sweep will show its true state instead of a silent skip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
…is being deprecated Per review: V1 is superseded by LiFiIntentEscrowFacetV2 and will be deprecated, so its coupling entry goes. Zero production coverage lost: every production chain running V1 also runs V2 (verified across all deploy logs), and the only V1-without-V2 chains are four testnets, out of scope for the production-scoped invariant. V1 joins AcrossFacet as a documented exemption in the family drift test so its absence from the registry stays deliberate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
melianessa
left a comment
melianessa
left a comment
There was a problem hiding this comment.
Reviewed the full diff against a local main checkout. Design is sound and the read-failure-is-not-absence handling is the right call for an error-severity gate. Four things I'd want changed before merge.
1. The on-chain selector identity source never fires for the Across V4 family — the exact family this PR exists for
identifyCoupledFacetsOnChain requires a facet's entire compiled selector set to be registered on one on-chain facet:
onChainSelectorSets.some((set) => wanted.every((selector) => set.has(selector)))But facets are registered with explicit selector exclusions:
script/deploy/facets/UpdateAcrossFacetV4.s.sol:10-14excludesSPOKEPOOL()andWRAPPED_NATIVE()UpdateAcrossFacetPackedV4.s.sol:20-30excludes 9 selectorsUpdateAcrossV4SwapFacet.s.sol:11-19excludes 7
Meanwhile out/AcrossFacetV4.sol/AcrossFacetV4.json has 6 methodIdentifiers, two of which are exactly SPOKEPOOL() / WRAPPED_NATIVE(). So the subset test can never succeed for those three facets, and resolveLiveFacets silently degrades to deploy-log-only identity for them — reintroducing the blind spot round 2 was added to close, for the Across family specifically.
It is silent because blindSpotWarning only fires when every candidate is unresolved (i.e. out/ absent entirely). With out/ built, unresolved is empty and nothing is logged.
Suggested fix: match on a non-empty, unambiguous selector intersection, or subtract the update script's excludes, or require a quorum of facet-unique selectors. Whichever you pick, please add a test using real artifacts plus a realistic post-exclusion on-chain set — every current test injects synthetic selector maps (facetPeripheryCouplings.test.ts, the SELECTORS fixtures), which is why this slipped through.
2. receiver-owner's new loop has no per-receiver error handling
const owner = await getOwnableContract(address, ctx.publicClient).read.owner()An uncaught throw here is converted into a hard error by executeInvariant (healthCheckInvariants.ts:1805-1808) and aborts the loop, so the remaining receivers go unchecked. Every sibling addition in this PR carefully warns-and-continues on read failure; the re-verify pass covers transient flake but not a permanent read failure. Suggest wrapping the read in try/catch → logWarn, matching immutable-bindings-match-config.
3. immutable-bindings-match-config resolves facets through resolvePeripheryAddress
Four of the eight annotated entries are facets (DeBridgeDlnFacet, EcoFacet, MayanFacet; LidoWrapper is genuinely periphery). For a facet, getPeripheryContract("MayanFacet") is always zero, so resolution falls through to ctx.deployedContracts — which holds the latest deployed facet, not necessarily the one registered in the diamond. Between a facet deploy and its diamondUpdate the invariant reads a contract that is not live. It also spends a pointless RPC call per facet per network on the daily sweep.
The invariant already has the machinery for this: consider readsOnChainFacets: true and resolving facet-typed entries from ctx.onChainFacets.
4. Redundant registry reads (small individually, daily × 40 networks)
periphery-registry-log-sync reads getPeripheryContract for all 15 candidates, then receiver-executor-binding, receiver-owner and immutable-bindings-match-config each independently re-read it for overlapping names via resolvePeripheryAddress — 3–4× duplicate reads per receiver. A memoized cache on ctx (same pattern as onChainFacets) would cut it and reduce rate-limit exposure, which matters because a rate-limited read in facet-required-periphery degrades the gate from error to warning.
Verified as correct while reviewing, for the record: all five new deployRequirements.json entries (getters exist on the contracts, every config key path resolves, and every network where each contract is currently deployed has a config value — so the new hard pre-deploy gates break no existing deploy path); across.json / stargateV2.json / chainflip.json cover 100% of the networks where the corresponding receiver is logged (23 / 40 / 2), so no "cannot verify" warning noise; the robinhood OutputValidator whitelist entry matches deployments/robinhood.json and sits in the right slot; and no dangling skipHealthcheck references remain in code or config.
QA Review — EXSC-684 — PR #2125Review type: Post-approval re-review (Run #28)
Post-Approval Commit Scope (301f5db — the new HEAD)The post-approval commit chain (
The merge-from-main changes are treated as already-reviewed upstream. No regression relevant to EXSC-684 was introduced. Melianessa's 7 Findings — StatusFinding 1 — robinhood.diamond.json ReceiverOIF inconsistency [BLOCKING — NOT FIXED]Verified on PR HEAD (SHA 301f5db):
The +1/-1 on Impact is real: Verdict: Blocking. Must fix before merge. Finding 2 — periphery-registry-log-sync scope gap [Advisory — new ticket acceptable]Confirmed: the candidate set at Melianessa's point that feeding However, the invariant still catches the primary class of bugs it was designed to prevent (registered-but-unlogged periphery contracts), and the gap is a coverage improvement rather than a blocking defect. Acceptable as a follow-up ticket. Verdict: Advisory. New ticket recommended. Finding 3 — receiver-owner fleet scope [BLOCKING — NOT FIXED]Verified: Developer's comment states "Live diamond measurements: ChainflipFacet, MayanFacet, DeBridgeDlnFacet show selector drift" — these are in the facet-required-periphery path, but the receiver-owner scope issue is separate. There is no fleet enumeration provided anywhere in the PR for receiver ownership state, and no indication that the full sweep has been run and all receivers confirmed to be owned by Melianessa's framing is precise: "The whole case for merging something that turns the sweep red is that every red was enumerated in advance." Without that enumeration, merging an error-severity gate that covers ~160 network-receiver combinations that have never been swept is a meaningful operational risk. Acceptable resolution options (either would unblock):
Verdict: Blocking. Requires fleet enumeration or temporary severity downgrade before merge. Finding 4 — periphery-registry-log-sync warning-first [Advisory]Valid operational concern: an error-severity gate on hand-maintained log completeness verified against only four of ~forty production networks risks false-positives that erode alert value. Melianessa recommends shipping as warning, clearing the fleet, then promoting. This is a judgment call on risk tolerance. The invariant's intent matches the ticket AC, and downgrading is a one-word change that can be made in a follow-up. Acceptable to defer. Verdict: Advisory. Defer to follow-up. Finding 5 — docs/TronFork.md stale escape hatch [BLOCKING — NOT FIXED]Verified: Current state in Verdict: Blocking. Both TronFork.md references must be updated to Finding 6 — dead comparison in isNonZeroTronAddress [BLOCKING — NOT FIXED]Verified at PR HEAD: export function isNonZeroTronAddress(value: string): boolean {
return (
value.startsWith('T') &&
value.length === 34 &&
value !== TRON_ZERO_ADDRESS_BASE58 &&
value !== TRON_ZERO_ADDRESS // <-- unreachable
)
}
Code defect: a hex-encoded zero address passed to Verdict: Blocking. The dead check is a code defect with a misleading comment. Must fix. Finding 7 — devNotes wording mismatch [Advisory — partially addressed]
Both are documentation-only. Melianessa's framing is correct — since the per-contract vs per-invariant split is deliberate, the one place people read about it should be accurate. Verdict: Advisory (Low). Recommended fix, not blocking in isolation. Developer's 2 Deferred ItemsItem 1 — Selector identity version-locked to HEAD artifacts (architectural deferral)
The current behavior is safe: version-drift facets log an info note and continue to be covered by the deploy log source. The gate does not pass silently (deploy-log coverage persists; Assessment: Acceptable deferral. New ticket recommended. Item 2 — getExpectedPairs unguarded getAddress (pre-existing) Reviewed at Developer is correct that this pre-dates this PR. The try/catch wrapper means the failure is loud but could still degrade the whitelist check to an empty comparison. Low practical risk since Assessment: Acceptable deferral. Noted as pre-existing. New ticket for Summary
4 blocking items remain (F1, F3, F5, F6). Verdict: Needs Work. Required before re-approval:
|
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ QA approved. Facet-periphery coupling health-check infrastructure is correctly implemented: facetPeripheryCouplings registry, facet-required-periphery invariant (severity error, production scope), facetCompanionReminder deploy-time hook, skipHealthcheck flag removal, and comprehensive test coverage. One pending action before merge: SC core dev formal APPROVED review required (melianessa has only commented, not approved). (lifi-qa-agent Run #26)
…artifact set Facets are cut into the diamond with getExcludes() exclusions (immutable getters, ownership functions), so a facet's full artifact selector set never appears on chain for such facets - the selector identity source could not fire for the entire Across V4 family it was built for. Match against the registered set (artifact minus the update script's excludes) instead, parsed from the real Update<Facet>.s.sol with a declared-size validity check so an unparseable exclude shape degrades to "unresolved" rather than a set that never matches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…from the diamond, cache registry reads - receiver-owner / receiver-executor-binding: wrap the per-receiver read in try/catch -> logWarn + continue, so one flaky RPC read no longer aborts the remaining receivers (matches the warn-and-continue convention of the sibling checks in this PR) - immutable-bindings-match-config: facet-typed entries (DeBridgeDlnFacet, EcoFacet, MayanFacet) now resolve from the diamond's selector map (readsOnChainFacets) instead of a pointless getPeripheryContract read that falls back to the deploy log's latest-deployed address - which between a facet deploy and its diamondUpdate is not the live facet - cache PeripheryRegistry reads per run on ctx: four invariants probe overlapping name sets, previously 3-4x duplicate RPC reads per name per network; failed reads are evicted so re-verify still hits the RPC fresh Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…try robustness - immutable-bindings-match-config: skip with a warning when the on-chain facet list is unavailable - the deploy-log fallback could otherwise verify a stale (deployed-but-not-cut) facet as a false pass; success lines now name the resolution path (live diamond facet vs registry/deploy-log address) - blindSpotWarning fires per unresolved candidate, not only when ALL candidates are unresolved - unresolvability became a per-facet condition with excludes parsing, and one unparseable update script must not hide behind the others - resolveLiveFacets reports version drift: a log-identified facet whose current artifact selectors match nothing on chain is logged (selector identity inactive for it until the deployed build catches up) - loadFacetRegisteredSelectors consults the zksync update script variant and degrades to unresolved when its excludes diverge from the canonical ones - periphery-registered: Promise.allSettled + per-name warn; a failing read is skipped, never misreported as "not registered" - resolvePeripheryAddress guards getAddress on deploy-log entries so a malformed address cannot abort a receiver loop - executeInvariant re-verify clears the registry read cache so a successful-but-stale read is not replayed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
script/deploy/shared/facetPeripheryCouplings.ts (1)
216-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrace matching is literal-unaware.
The depth scan counts
{/}anywhere after the marker, including inside string literals (e.g.revert("{")). No current update script hits this, but a future one would silently yieldbodyEndat the wrong place and turn intonull("identity unknown"), which downgrades selector identity for that facet. A cheap guard is to skip characters inside"/'runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/shared/facetPeripheryCouplings.ts` around lines 216 - 259, Update the brace-depth scan in the getExcludes parsing logic to ignore `{` and `}` characters inside single- or double-quoted string literals, including escaped characters, while preserving normal brace matching outside strings. Keep the existing null fallback for unmatched braces and the returned selector extraction behavior unchanged.script/deploy/shared/facetPeripheryCouplings.test.ts (1)
425-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact-equality snapshot will break on any new AcrossFacetV4 function.
This asserts the full registered set equals a hand-maintained list, so adding any external function to
AcrossFacetV4fails a parser/loader test for an unrelated reason. The assertions at Lines 452-461 (excludes absent, length < artifact) already capture the behavior under test without the maintenance tripwire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/shared/facetPeripheryCouplings.test.ts` around lines 425 - 432, Remove the exact-equality assertion comparing registered selectors with ACROSS_V4_REGISTERED from the AcrossFacetV4 test, while retaining the existing assertions that verify excluded selectors are absent and the registered count is below the artifact count. Keep the test focused on post-exclusion behavior without requiring updates when new AcrossFacetV4 functions are added.script/deploy/healthCheckInvariants.ts (1)
2476-2483: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache clear during the re-verify pass races with concurrently running invariants.
runHealthCheckInvariantsexecutes each phase withPromise.all, so when one error-severity invariant clearsbaseCtx.peripheryRegistryCache, other invariants may have in-flight reads whose entries were just dropped, and a later failure handler (peripheryRegistryCache.delete(name)at Line 998) can remove an entry repopulated after the clear. Both outcomes are only extra RPC reads, so no correctness impact — worth a note in the comment so the interaction is not mistaken for a stale-read guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.ts` around lines 2476 - 2483, The cache-clear comment near the error-severity re-verification in runHealthCheckInvariants should explicitly note that phases run concurrently and in-flight reads or later peripheryRegistryCache.delete calls may race with the clear, causing extra RPC reads without affecting correctness. Update only the comment to document this interaction while preserving the existing stale-cache invalidation explanation.config/networks.json (1)
1032-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale
skipHealthcheckreferences.
config/networks.jsonno longer carries the flag forrobinhood/tempoandscript/deploy/healthCheck.tsdocuments no network-level bypass, butdocs/TronFork.mdstill describessomnia.skipHealthcheck = trueand the earlier fix as a temporary config change. Update the docs to say the flag is retired and link to the new per-invariant exclusion model instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/networks.json` around lines 1032 - 1036, The stale documentation in TronFork.md still instructs users to set somnia.skipHealthcheck and describes the old temporary workaround. Update the relevant section to state that skipHealthcheck is retired and reference the current per-invariant exclusion model, removing the obsolete network-level configuration guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/networks.json`:
- Line 173: Update the arc network devNotes entry to state that TokenWrapper is
intentionally not deployed and exempted through corePeripheryExemptions, rather
than claiming TokenWrapper-related invariants are excluded per-invariant;
preserve the existing native USDC and dummy wrappedNativeAddress details.
In `@script/deploy/healthCheckInvariants.test.ts`:
- Around line 1053-1058: In the test setup containing the onChainFacets entry
for FACET, capture the result of loadFacetRegisteredSelectors('MayanFacet') and
assert it is non-null before constructing the facet configuration. Pass the
validated selectors to identifyCoupledFacetsOnChain so exclude-parse failures
report the loader assertion instead of failing during selectors.map.
---
Nitpick comments:
In `@config/networks.json`:
- Around line 1032-1036: The stale documentation in TronFork.md still instructs
users to set somnia.skipHealthcheck and describes the old temporary workaround.
Update the relevant section to state that skipHealthcheck is retired and
reference the current per-invariant exclusion model, removing the obsolete
network-level configuration guidance.
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 2476-2483: The cache-clear comment near the error-severity
re-verification in runHealthCheckInvariants should explicitly note that phases
run concurrently and in-flight reads or later peripheryRegistryCache.delete
calls may race with the clear, causing extra RPC reads without affecting
correctness. Update only the comment to document this interaction while
preserving the existing stale-cache invalidation explanation.
In `@script/deploy/shared/facetPeripheryCouplings.test.ts`:
- Around line 425-432: Remove the exact-equality assertion comparing registered
selectors with ACROSS_V4_REGISTERED from the AcrossFacetV4 test, while retaining
the existing assertions that verify excluded selectors are absent and the
registered count is below the artifact count. Keep the test focused on
post-exclusion behavior without requiring updates when new AcrossFacetV4
functions are added.
In `@script/deploy/shared/facetPeripheryCouplings.ts`:
- Around line 216-259: Update the brace-depth scan in the getExcludes parsing
logic to ignore `{` and `}` characters inside single- or double-quoted string
literals, including escaped characters, while preserving normal brace matching
outside strings. Keep the existing null fallback for unmatched braces and the
returned selector extraction behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47985cc1-bbfc-4629-9c16-cc65d65a6007
📒 Files selected for processing (15)
config/global.jsonconfig/healthCheckExclusions.jsonconfig/networks.jsonconfig/whitelist.jsondeployments/robinhood.jsonscript/common/types.tsscript/deploy/deploySingleContract.shscript/deploy/healthCheck.tsscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.tsscript/deploy/resources/contractDependencyReminder.test.tsscript/deploy/resources/contractDependencyReminder.tsscript/deploy/resources/deployRequirements.jsonscript/deploy/shared/facetPeripheryCouplings.test.tsscript/deploy/shared/facetPeripheryCouplings.ts
💤 Files with no reviewable changes (2)
- script/common/types.ts
- config/global.json
There was a problem hiding this comment.
✅ QA approved (Run #27, post-approval re-review). 3 new commits improve facet-selector identity, per-receiver RPC resilience, and registry robustness. Advisory: devNotes wording + test null-coalesce. (lifi-qa-agent[bot])
…path - re-verify gets a private read cache instead of clearing the shared one, so a retrying invariant cannot evict entries sibling invariants are mid-flight on; cache eviction is identity-guarded so a stale rejection cannot tear down a newer healthy entry - periphery-registered judges deploy-log presence BEFORE skipping unresolved names, so a flaky read no longer downgrades a real "not deployed" error to a warning - guard the two remaining getAddress call sites on deploy-log entries (periphery-registered, periphery-registry-log-sync) via a shared helper - one malformed entry no longer aborts a whole check loop - loadFacetRegisteredSelectors takes an injectable repo root; the zksync divergence/unparseable/zksync-only/no-script paths are now covered - skip the immutable-binding warning when there are no annotated checks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…3e1c2 # Conflicts: # deployments/robinhood.json
…ctable root The injectable repo root added for test isolation turned a constant path base into a caller-supplied one, which Aikido flags as a path-traversal sink (2 new HIGH). Production code resolves from process.cwd() again; the zksync exclude tests enter a synthetic checkout via process.chdir instead, restoring cwd and removing the temp dir in a finally block. Also pin the other half of the re-verify cache contract: a stale cached success must not survive into the retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review-gate residual notes (not fixed in this PR)Follow-up on the last review round. All four findings are addressed in 1. Selector identity is version-locked to the HEAD artifacts (architectural, needs a decision)
Rather than leave that invisible, 2.
Verification note: the registered-selector sets were checked against the live mainnet and arbitrum diamonds (exact set match), not only against unit tests; the parser was run over all 63 |
There was a problem hiding this comment.
1. This PR adds a new production log inconsistency on robinhood. deployments/robinhood.json gains ReceiverOIF: 0xdD54bEa53F94554d632d0D844D88a4fd51b2C576, but robinhood.diamond.json → LiFiDiamond.Periphery.ReceiverOIF is still "" on this branch. OutputValidator in that same file is populated, so the empty string reads as "not registered here", not as a placeholder convention.
That's not cosmetic: script/helperFunctions.sh:810-821 reads .diamond.json → Periphery to resolve a periphery contract's version during deploys and updates, so an empty value means the tooling can't resolve robinhood's ReceiverOIF. I checked the rest of the fleet, and the only other networks where a .diamond.json Periphery entry is "" while the flat log has an address are dead testnets (goerli, mumbai, lineatest, bsc-testnet). So this would be the one production case, on the chain the PR is remediating. One-line fix.
2. periphery-registry-log-sync only checks one of the two logs. The description calls it "every known periphery name", but the candidate set at healthCheckInvariants.ts:1392 is the union of corePeriphery, whitelistPeripheryFunctions, the coupling companions and RECEIVER_EXECUTOR_GETTERS, which comes to about 15 names. Receiver, FeeCollector, LiFiDEXAggregator, ReceiverAcrossV3 and Composer aren't probed. And it only ever compares against the flat log.
.diamond.json already enumerates the full registered set per network. Feeding its Periphery keys in as extra candidates and its values in as a second comparison target closes both gaps cheaply, and would have caught #1 on its own. Left as is, that second log is exactly the sort of unchecked identity source this PR exists to kill, which feels worth either fixing or naming as a follow-up in the description.
3. Has anyone audited receiver-owner across the fleet? It has scope {}, so all chains and all environments, and it now asserts owner() == refundWallet for all four RECEIVER_EXECUTOR_GETTERS entries (healthCheckInvariants.ts:2128) rather than just the generic Receiver. That's roughly four receivers times forty production networks newly under an error gate, and the description only reports mainnet verified. Any receiver deployed before the refundWallet-owner convention, or owned by a Safe, goes red.
The description is careful to enumerate the expected new reds for ReceiverOIF presence but doesn't mention ownership at all. The whole case for merging something that turns the sweep red is that every red was enumerated in advance, so I'd want this one run fleet-wide and folded into that section.
4. Consider landing periphery-registry-log-sync as a warning first. Same worry from a different angle: it's an error-severity gate on hand-maintained deploy-log completeness, verified on four of about forty production networks. The sweep posts to Slack on failure. If it goes red on a dozen chains for log-hygiene reasons, people stop reading the alerts, which is the mechanism that let EXSC-682 sit there in the first place. Ship it as a warning, clear the fleet, promote it in a one-liner. facet-required-periphery is the check that actually gates functionality and should stay an error.
5. docs/TronFork.md goes stale, and the fork loses its escape hatch. Line 94 lists somnia.skipHealthcheck = true as one of the fork's tracked config diffs, and line 181 prescribes it as the fix for a documented sync-PR failure class. Once this merges and syncs down, that field is a silent no-op, since there's no schema validation on networks.json to complain about an unknown key. The doc needs to point at invariantExclusions instead, and the fork needs the equivalent entry.
6. Dead comparison in isNonZeroTronAddress. TRON_ZERO_ADDRESS from the devkit is '410000…0000', a 42-char hex string (dist/constants.d.ts:20), so the value !== TRON_ZERO_ADDRESS check at line 413 can't fire behind startsWith('T') && length === 34. The import on line 18 exists only to feed it. Either drop both, or drop the shape checks if the intent was to accept both encodings. Tests pass either way, which is presumably how it survived.
7. The arc / tempo devNotes name the wrong mechanism. They now say the TokenWrapper invariants are "excluded per-invariant in config/healthCheckExclusions.json", but what's actually used is corePeripheryExemptions, which is per-contract; invariantExclusions is empty. Since the two-tier split is the point, it'd be good not to blur it in the one place people will read.
Smaller stuff, none of it blocking:
deploySingleContract.shnow spawns two morebunx tsxprocesses per contract, roughly a second or two each, which is a minute-plus on a full diamond rollout. Worth folding both reminders andfacetRefundReminderinto one invocation. Also,2>/dev/null || truemeans a typo in either script silently disables the nudge forever; a smoke test asserting each CLI exits 0 and prints something for a known input would be cheap.isValidNetworkNameandreadDeployLogare exported fromfacetCompanionReminder.tsand imported bycontractDependencyReminder.ts, which drags in a module with a top-levelif (isDirectRun()) runCli(). The guard is correct, but these are generic deploy-log helpers and belong inscript/deploy/shared/.resolveLiveFacetslowercases addresses, and base58 is case-sensitive. It's applied to both sides so Tron matching still works; it's only a trap if that map is ever reused for display or compared against a non-lowercased source. A comment would cover it.identifyCoupledFacetsOnChainmatches on subset, so if facet A's registered set were a strict subset of on-chain facet B's, A resolves to B's address. The one-selector-one-facet rule makes that close to impossible and you have a test for the sibling case, but inimmutable-bindings-match-configthe consequence is reading a getter on the wrong contract, so requiring that the candidate isn't already claimed would tighten it for almost nothing.facet-required-peripheryonly asserts non-zero, so a registration pointing at a wrong or codeless address passes.receiver-ownerwould then fail its read andlogWarnrather thanlogError, so a bogus registration is warning-only. The tiers compose reasonably; just noting where the ceiling is.resolveConfigValuecan't handle jq-quoted paths. All eight current annotations use plain dot paths, but annotating something like.networks."my-chain".xyieldsexpectedAddress: nulland a warning, i.e. coverage shrinks quietly, which is the failure mode this PR is about. A test asserting every annotated path resolves on at least one network would guard it; right now there's only the across-specific precondition.- The
delete-pending-proposals.tschange is pure Prettier reformatting and unrelated.
There was a problem hiding this comment.
Post-approval re-review (Run #28): 4 blocking items require fixes before merge.
-
robinhood.diamond.json ReceiverOIF still empty —
deployments/robinhood.diamond.jsonhas"ReceiverOIF": ""whiledeployments/robinhood.jsonhas the address.helperFunctions.shreads.diamond.jsonfor version resolution; one-line fix. -
receiver-owner fleet scope unvalidated —
scope: {}puts ~160 network-receiver combinations under the error gate with no prior sweep. Either provide fleet enumeration (all green) or downgrade toseverity: 'warning'for this release. -
TronFork.md prescribes a no-op field — Lines 94 and 181 still prescribe
skipHealthcheck(now absent from the schema). Both references must point toinvariantExclusions. -
isNonZeroTronAddress dead comparison —
value !== TRON_ZERO_ADDRESS(hex form) can never be false afterstartsWith('T') && length === 34pass. Drop the dead check and import, or drop the shape guards if both encodings were intended.
Full analysis: see QA review comment on this PR.
# Conflicts: # config/networks.json # script/common/types.ts
- robinhood.diamond.json: fill the empty ReceiverOIF entry (was breaking helperFunctions.sh version resolution) - periphery-registry-log-sync: widen candidates to both deploy logs' keys and compare the diamond log as a second target; ship warning-first (fleet dry-run 2026-07-31: 114 inconsistencies across 38 production networks would have gone red on an error gate) - receiver-owner: keep error severity - fleet dry-run verified owner() == refundWallet on all 65 active EVM production networks; survive a flaky generic-Receiver read like the per-receiver loop already does - isNonZeroTronAddress: drop the dead hex-form comparison and unused import - docs/TronFork.md: point the fork's healthcheck escape hatch at invariantExclusions (skipHealthcheck no longer exists) - networks.json arc devNotes: name corePeripheryExemptions, not per-invariant exclusion - identifyCoupledFacetsOnChain: exact matches claim facets before subset matches, so a strict-subset candidate cannot resolve to another facet - move isValidNetworkName/readDeployLog to shared/deployLog.ts (generic deploy log helpers, out of the CLI-entry module) - CLI smoke tests for both reminder scripts (they run behind '|| true' in deploySingleContract.sh - a crash would silently disable them) - guard test: every getter-annotated deployRequirements path must resolve on at least one network - revert unrelated prettier churn in delete-pending-proposals.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e use Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… pharos, polygon The widened periphery-registry-log-sync fleet dry-run found ReceiverOIF registered in these five diamonds' PeripheryRegistry while absent from both deploy logs (the same EXSC-682 gap this PR fixes on mainnet/base/arbitrum/ robinhood). Addresses read from each network's on-chain registry and re-verified via cast (registry entry + non-empty code). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@melianessa Thanks for the round — all seven items are addressed in 1. robinhood diamond log — Fixed: 2. 3. 4. Warning-first — You were right, and the data is unambiguous: the widened check finds 114 log/registry inconsistencies across 38 production networks. Top classes: Bonus: the dry-run found ReceiverOIF live-but-unlogged on katana, megaeth, optimism, pharos, polygon — same EXSC-682 class this PR fixes on mainnet/base/arbitrum/robinhood. Synced both logs for all five in 5. TronFork.md — Both spots updated to prescribe an 6. Dead Tron comparison — Dropped the hex-form compare and the now-unused import; fixed the 7. arc devNotes — Now says "exempted per-contract via corePeripheryExemptions". (Only arc carried the wording; tempo's devNotes was replaced wholesale by main's gasEstimateMultiplier rewrite in the merge.) Smaller bullets:
Leaving all threads open for you to resolve. |
| const diamondLogPath = path.join( | ||
| process.cwd(), | ||
| 'deployments', | ||
| `${networkLower}.diamond.json` | ||
| ) | ||
| if (!existsSync(diamondLogPath)) return null | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(diamondLogPath, 'utf8')) as { |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
| const diamondLogPath = path.join( | |
| process.cwd(), | |
| 'deployments', | |
| `${networkLower}.diamond.json` | |
| ) | |
| if (!existsSync(diamondLogPath)) return null | |
| try { | |
| const parsed = JSON.parse(readFileSync(diamondLogPath, 'utf8')) as { | |
| const base = path.resolve(process.cwd(), 'deployments') | |
| const target = path.resolve(base, `${networkLower}.diamond.json`) | |
| const relative = path.relative(base, target) | |
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | |
| return null | |
| } | |
| if (!existsSync(target)) return null | |
| try { | |
| const parsed = JSON.parse(readFileSync(target, 'utf8')) as { |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Fixed in ee2cebf: loadDiamondLogPeriphery now applies the same containment as shared/deployLog.ts readDeployLog — network-name regex gate plus a resolved-path check that the target stays inside deployments/.
|
@lifi-qa-agent All four blocking items from Run #28 are addressed (details in the reply to Daniela's review above):
Additionally, |
Review-gate residual notes (not fixed in this PR)The fleet dry-run of the widened
Raw per-network data is reproducible by running the invariant standalone; happy to park a ticket if wanted. |
…ments/ Same name-regex + resolved-path containment as shared/deployLog.ts, per the Aikido finding on the new read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
script/deploy/shared/facetPeripheryCouplings.test.ts (1)
280-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the nested ternary in
subsetLoad.The coding guidelines forbid nested ternary operators. A lookup record also gives a place for an explicit return type.
♻️ Proposed refactor
- const subsetLoad = (name: string) => - name === 'SuperFacet' - ? ['0xaaaa0001', '0xaaaa0002', '0xaaaa0003'] - : name === 'SubsetFacet' - ? ['0xaaaa0001', '0xaaaa0002'] - : null + const subsetSelectors: Record<string, string[]> = { + SuperFacet: ['0xaaaa0001', '0xaaaa0002', '0xaaaa0003'], + SubsetFacet: ['0xaaaa0001', '0xaaaa0002'], + } + const subsetLoad = (name: string): string[] | null => + subsetSelectors[name] ?? nullAs per coding guidelines: "Avoid nested ternary operators" and "Use explicit return types for functions in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/shared/facetPeripheryCouplings.test.ts` around lines 280 - 285, Replace the nested conditional expression in subsetLoad with a lookup record keyed by facet name and an explicit function return type. Preserve the existing arrays for SuperFacet and SubsetFacet, and return null for names not present in the lookup.Source: Coding guidelines
script/deploy/healthCheckInvariants.ts (1)
1447-1458: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRestrict the candidate set to periphery names.
Object.keys(ctx.deployedContracts)covers the whole flat deploy log, which also lists facets and other non-periphery contracts. Each of those names becomes agetPeripheryContractread that always returnsZERO_ADDRESS. On EVM the reads are cached and batched, so the cost is bounded but still large. On Tron the loop at line 1496 awaits each read in sequence, so the added names multiply the wall-clock time of the invariant.Consider filtering deploy-log keys to names that are periphery candidates (for example, keys also present in the diamond log periphery section, the core periphery lists, or the coupling/getter lists), or keep the flat-log keys only when they are absent from the facet artifacts.
Run the following script to size the effect on a production log:
#!/bin/bash # Count flat deploy-log entries vs diamond-log periphery entries per network. fd -e json . deployments --max-depth 1 | head -40 | while read -r f; do base=$(basename "$f" .json) case "$base" in *.diamond) continue;; esac total=$(jq 'keys | length' "$f" 2>/dev/null) periphery=$(jq '.LiFiDiamond.Periphery // {} | keys | length' "deployments/${base}.diamond.json" 2>/dev/null) echo "$base flat=$total diamondPeriphery=$periphery" done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/healthCheckInvariants.ts` around lines 1447 - 1458, Restrict the candidates assembled in the candidate-set construction to periphery names instead of all keys from ctx.deployedContracts. Filter flat deploy-log entries against known periphery sources such as diamondLogPeriphery, getCorePeriphery/getTronCorePeriphery, coupling requirements, and receiver getter names, while preserving candidates supplied by the existing non-deploy-log sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 1447-1458: Restrict the candidates assembled in the candidate-set
construction to periphery names instead of all keys from ctx.deployedContracts.
Filter flat deploy-log entries against known periphery sources such as
diamondLogPeriphery, getCorePeriphery/getTronCorePeriphery, coupling
requirements, and receiver getter names, while preserving candidates supplied by
the existing non-deploy-log sources.
In `@script/deploy/shared/facetPeripheryCouplings.test.ts`:
- Around line 280-285: Replace the nested conditional expression in subsetLoad
with a lookup record keyed by facet name and an explicit function return type.
Preserve the existing arrays for SuperFacet and SubsetFacet, and return null for
names not present in the lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41ce67c0-33cd-4063-be47-ce56fc85e667
📒 Files selected for processing (29)
config/networks.jsondeployments/arbitrum.jsondeployments/base.jsondeployments/katana.diamond.jsondeployments/katana.jsondeployments/mainnet.jsondeployments/megaeth.diamond.jsondeployments/megaeth.jsondeployments/optimism.diamond.jsondeployments/optimism.jsondeployments/pharos.diamond.jsondeployments/pharos.jsondeployments/polygon.diamond.jsondeployments/polygon.jsondeployments/robinhood.diamond.jsondocs/TronFork.mdscript/common/types.tsscript/deploy/deploySingleContract.shscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.tsscript/deploy/resources/contractDependencyReminder.test.tsscript/deploy/resources/contractDependencyReminder.tsscript/deploy/resources/facetCompanionReminder.test.tsscript/deploy/resources/facetCompanionReminder.tsscript/deploy/safe/delete-pending-proposals.tsscript/deploy/shared/deployLog.tsscript/deploy/shared/facetPeripheryCouplings.test.tsscript/deploy/shared/facetPeripheryCouplings.tsscript/deploy/shared/immutableBindings.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- deployments/base.json
- deployments/mainnet.json
- config/networks.json
- script/common/types.ts
- script/deploy/shared/immutableBindings.test.ts
- script/deploy/deploySingleContract.sh
- script/deploy/resources/contractDependencyReminder.ts
- deployments/arbitrum.json
- script/deploy/shared/facetPeripheryCouplings.ts
🔍 QA Review — EXSC-684 — Health-Check Invariants + Deployment Registry Fixes
SummaryThis is a re-review following Resolved Findings (previously blocking)
Acceptance Criteria Verification
FindingsNone. All prior concerns resolved.
|
There was a problem hiding this comment.
✅ QA AI Approved (Run #30 — 2026-07-31). All 4 previously blocking findings from Run #28 CHANGES_REQUESTED are confirmed resolved: F1 robinhood.diamond.json ReceiverOIF populated, F3 receiver-owner fleet dry-run 65 networks 0 mismatches, F5 TronFork.md updated to invariantExclusions, F6 isNonZeroTronAddress dead hex branch removed.
* chore(deploy): add ReceiverOIF v1.0.0 on 7 chains (EXSC-684) Deploys ReceiverOIF v1.0.0 to arc, bsc, megaeth, optimism, pharos, polygon and robinhood, so the facet-required-periphery invariant from #2125 has a receiver to find wherever an intent-escrow facet is live. Also adds the two pieces of framework config the rollout needed: a ReceiverOIF entry in deployRequirements.json (constructor-arg validation) and ReceiverOIF 1.0.0 in _targetState.json for the target chains plus mainnet/base/arbitrum, which had it deployed but never declared. jovay is excluded: neither OIF settler has code there, so the contract cannot be deployed. katana is pending a deployer top-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(deploy): add ReceiverOIF v1.0.0 on katana (EXSC-684) katana's deploy failed forge's pre-flight balance check: the pinned 3 gwei gasPrice combined with the repo-wide GAS_ESTIMATE_MULTIPLIER=500 made forge reserve 5,023,370 gas x 3 gwei (0.01507 ETH) against a 0.01006 ETH balance, for a deploy whose real cost is ~918k gas (~0.0000009 ETH at the 0.001 gwei live base fee). Sets katana's gasEstimateMultiplier to 200 rather than touching the gasPrice pin, which is a deliberate documented workaround for the RPC over-reporting eth_gasPrice. 200 keeps a 2x buffer on the gas limit and is fundable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(networks): correct katana gasEstimateMultiplier rationale (EXSC-684) The note claimed 500 was the repo-wide multiplier; .env.example ships 130, so 500 was the local operator value. Restates the reserve as scaling with whatever GAS_ESTIMATE_MULTIPLIER the operator's .env carries, which is the actual reason the per-network pin makes the reserve deterministic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(networks): drop temporary katana gasEstimateMultiplier override (EXSC-684) The override existed only to get katana's ReceiverOIF deploy past forge's pre-flight gas reserve; the deploy and its Safe proposal are complete, so the override is no longer load-bearing. config/networks.json returns to main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(deploy): sync ReceiverOIF diamond logs after cut execution on 8 chains (EXSC-684) diamondUpdatePeriphery timelock ops executed on-chain for arc, bsc, katana, megaeth, optimism, pharos, polygon, robinhood. Registry entries regenerated and scoped to ReceiverOIF only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Goran Vladika <goran.vladika@gmail.com>
…3e1c2 # Conflicts: # deployments/arbitrum.json # deployments/base.json # deployments/katana.diamond.json # deployments/katana.json # deployments/mainnet.json # deployments/optimism.diamond.json # deployments/optimism.json # deployments/pharos.diamond.json # deployments/pharos.json # deployments/polygon.diamond.json # deployments/polygon.json # deployments/robinhood.diamond.json # deployments/robinhood.json # script/deploy/resources/deployRequirements.json
4 participants
Which Linear task belongs to this PR?
https://linear.app/lifi-linear/issue/EXSC-684/couple-facets-to-their-companion-periphery-contracts-so-a-receiver
Motivating incident: EXSC-682 (missing
ReceiverAcrossV4on Robinhood, #2124).Why did I implement it this way?
A bridge facet only covers the source side; destination calls need its companion Receiver on
the same chain. Nothing tied the two together, so a facet could be rolled out to a new chain
while its Receiver was silently forgotten — which is what disabled Across destination calls
on Robinhood, and Daniel reports it has happened several times.
Why nothing caught it. Receivers had no "must exist" coverage at any tier:
non-core-facets-deployedfilters target-state entries onk.includes('Facet'), so aReceiver listed there is never checked for deployment.
periphery-registeredonly checkscorePeriphery ∪ whitelistPeripheryFunctions; noReceiver is in either list.
receiver-executor-bindingandreceiver-ownerboth skip a Receiver whose address isabsent, so a missing one is silently exempt from the only checks that mention it.
Why the requirement is derived from the facet, not declared per chain. Robinhood's
_targetState.jsonnever listedReceiverAcrossV4either — a check that only comparedtarget state against chain state would have stayed green. So the coupling lives once, next to
coreFacets/corePeripheryinconfig/global.json, and the requirement is derived fromwhichever facets are actually live on a chain. Adding a chain cannot silently opt out.
Why
config/global.jsonand notdeployRequirements.json.deployRequirements.jsonisconsumed by
checkDeployRequirements()(script/helperFunctions.sh) and both its sectionsmean one thing: this is a constructor argument, resolve it or refuse to deploy. A companion
Receiver is not a constructor arg, and the ordering is inverted — the facet is legitimately
deployed first — so a hard pre-deploy block would be wrong by construction. It is also
bash-only, so the TypeScript health check could not consume it without a second reader.
global.jsonalready holds the comparable contract-topology maps and is already loaded byhealthCheck.ts.Why the registry is keyed by facet name. The match has always been on the facet name, so
that is the key:
facetPeripheryCouplings["AcrossFacetV4"]is a direct lookup, and it mirrorshow the sibling registries (
deployRequirements.json,whitelistPeripheryFunctions) are keyed.Variants of one family each get their own entry pointing at the same companion, and the
evaluator merges them back into a single requirement so reporting stays per-integration rather
than per-facet.
Why
requiresAnyOf(a list) even though every entry currently has one element. It existsfor genuine interchangeability. Across kept the
handleV3AcrossMessagesignature in V4, soReceiverAcrossV3could service a V4 destination call — but V3 is deprecated, and acceptingit would let a deprecated contract mask a missing current one, so V4 now requires
ReceiverAcrossV4specifically. Checked against every active chain first: no chain runs V4facets with only the V3 receiver, so tightening this created zero new failures.
Why two enforcement tiers.
facet-required-peripheryhealth-check invariant (severityerror, production scope) isthe gate. It triggers on facets registered on chain and asserts the on-chain
PeripheryRegistryreturns a non-zero address for one of the companions. It reads theregistry rather than
ctx.deployedContractsbecause the deploy log can be incomplete —ReceiverOIFis live on mainnet and base but has no entry in eitherdeployments/*.json.It runs on the existing daily sweep (
healthCheckAllNetworks.yml) and on new-network PRs(
healthCheckForNewNetworkDeployment.yml), so no new workflow is needed.facetCompanionReminder.tsis a non-fatal deploy-time nudge, following the existingfacetRefundReminder.tspattern indeploySingleContract.sh. Non-fatal becausefacet-before-Receiver is the normal order.
Triggering on on-chain facets rather than target state also means chains mid-rollout (plume
has the Across V4 facets in target state but nothing deployed) are not flagged here — that
gap is a target-state concern, not this invariant's.
Per-network carve-outs require a reason and print it when they fire.
notRequiredOnmaps anetwork key to why the destination side genuinely does not apply there; the skip is logged with
that reason so it is never invisible. It is currently empty — no chain has a justified exemption.
Live gaps found by this check — in scope for this ticket
Every active production network was audited on chain with this invariant's logic (reading
PeripheryRegistry.getPeripheryContract, not the deploy logs).ReceiverOIFis missing on 7 production chains. Policy is to ship it wherever eitherLiFiIntentEscrowfacet is live. It is registered only onmainnet,baseandarbitrum;the escrow facets are live with no receiver on
jovay,katana,megaeth,optimism,pharos,polygonandrobinhood, plus four testnets (arbitrumsepolia,arctestnet,basesepolia,optimismsepolia).arbitrum,arcandbsccould not be read from thissession and are likely in the same state.
arbitrumwasre-read and is fine —
ReceiverOIFis registered and has code there; its deploy-log entry wasmissing and is now backfilled.
arcandbscstill could not be read; the newperiphery-registry-log-syncinvariant will surface their state on the daily sweep.Slack on failure. That is the intended behaviour — they are real gaps of exactly the class this
PR exists to surface — but it is a deliberate, visible consequence, not a surprise. Deploying
the missing receivers is the remediation.
tempo—ReceiverAcrossV4deployed but never registered.AcrossFacetV4andAcrossV4SwapFacetare registered on chain, andReceiverAcrossV4is deployed and correctly wired (0xac6ab3D8026Bfd31eDeA055deFedF61956439d0f,has code,
EXECUTOR()→0x4556099dde35755d00fEc81100481C582A5EE63c, tempo's Executor) — butgetPeripheryContract("ReceiverAcrossV4")returns the zero address. Deployed, never registered,so Across destination calls are disabled on tempo.
ReceiverStargateV2on the same diamond isregistered, so nothing structural stopped it; the step was simply skipped. It is also already in
_targetState.json, so this is purely a missing on-chain registration, not a config gap.Remediating it needs
diamondUpdatePeripheryon the tempo diamond, which is owned by theLiFiTimelockController (
0xa7A28FB774a742e82dc237a08d515745f96A46b1) — so Safe proposal →timelock → execute, which needs the
lifi-connecttunnel and hardware-wallet signing. That cannotbe done from an agent session (
docs/Setup-agents.md), so the resultingdeployments/tempo.diamond.jsonPeriphery entry is not in this PR yet; it lands here as afollow-up commit once the registration executes. The log is deliberately left untouched rather than
pre-filled, so it never claims a registration that has not happened.
Known limitationRESOLVED in round 3:skipHealthcheckis gone — every chain runs every checkskipHealthcheck: trueused to blanket-skiparc,robinhoodandtempo— including robinhoodand tempo, precisely the two chains where this bug class actually manifested. Round 3 removes
the flag entirely (the
INetworkfield, the early return inhealthCheck.ts, and all threenetwork entries). Genuine specialties are now carved out narrowly with mandatory, printed reasons
in
config/healthCheckExclusions.json, which has two tiers:invariantExclusions— skip one whole invariant on one network (currently empty).corePeripheryExemptions— exempt one core periphery contract on one network, where awhole-invariant skip would hide unrelated coverage. Currently:
TokenWrapperonarcandtempo(no native/wrap path on either — reasons lifted from theirdevNotes).What running the previously-skipped chains surfaced (all verified live):
periphery-registry-log-syncinvariant immediately caughtOutputValidatorandReceiverOIFregistered on chain but missing from the deploy log, plus
OutputValidatorabsent fromwhitelist.json. All fixed in this PR (addresses verified on chain; robinhood'sReceiverOIFat
0xdD54…C576binds the correct Executor — so the "missing on robinhood" entry in the gaplist above is outdated: it was deployed and registered after the incident, just never logged).
ReceiverAcrossV4deployed-but-unregistered(the timelock-gated remediation described above) and
SquidFacetin target state but notdeployed (deploy it or de-scope it from
_targetState.json— team decision).it. Either way the sweep now shows arc's true state instead of a silent green skip.
Review round 2 — deploy-log blind spots closed, immutables gated (Daniela's finding + follow-ups)
Daniela's review found the facet side of
facet-required-peripheryresolving identity throughthe same incomplete deploy log the PR rejects for receivers. Confirmed and fixed, plus the
generalizations agreed with Daniel:
from the log is identified by matching its full compiled selector set against the diamond's
facets()output (resolveLiveFacetsinshared/facetPeripheryCouplings.ts). When neithersource can identify an on-chain facet, a warning fires instead of a silent pass.
receiver-executor-binding/receiver-ownerresolve receivers registry-first (deploylog as fallback) — previously both silently skipped any receiver missing from the log, which
exempted
ReceiverOIFon mainnet/base/arbitrum from binding and ownership coverage entirely.receiver-ownernow also covers the bridge-specific receivers (they had no owner check).periphery-registry-log-syncinvariant (error): every known periphery name registeredon chain must appear in the deploy log with the same address — an incomplete log silently
shrinks the coverage of every log-resolved check, so it is now a first-class failure.
deployments/{mainnet,base,arbitrum}.jsonbackfilled with the on-chain-verifiedReceiverOIF(
0x761B0e8f6e80BBd23F3886663Cc071a554be37A3); verified live green on mainnet.immutable-bindings-match-configinvariant (error): contracts binding externalcounterparties immutably (
ReceiverAcrossV4.SPOKEPOOL,ReceiverStargateV2.tokenMessaging/endpointV2,ReceiverChainflip.chainflipVault) are compared against the config files, drivenby
getterannotations on the existingdeployRequirements.jsonentries (extra key is ignoredby the bash consumer). Catches the "integration migrated, config moved on, immutable still
points at the dead counterparty" class that presence + executor-binding checks cannot see.
Verified live: all annotated bindings match config on mainnet.
HEALTH_CHECK_EXCLUSIONSmoved toconfig/healthCheckExclusions.jsonso per-networkcarve-outs are ops-editable config, not TS edits; integrity tests validate every entry against
real invariant names and networks.
Receiver*companion infacetPeripheryCouplingsmust appearin
RECEIVER_EXECUTOR_GETTERS(deprecatedReceiverAcrossV3exempt) — a new coupling can nolonger ship presence-checked but binding-unchecked.
Known remaining gaps (deliberately not in this PR):
ReceiverOIFhas nodeployRequirements.jsonentry (so its
OUTPUT_SETTLERbinding is not yet annotatable — needs the OIF config key addedfirst), the log-sync and binding invariants are EVM-only (Tron receivers unchecked), and the
reverse-dependency deploy-time cascade advisor ("redeploying Executor → these N contracts bind it
immutably") is a follow-up ticket.
Review round 3 — Daniel's follow-ups: full deployRequirements coverage, Tron, cascade advisor, no more blanket skips
fbf485f67): every deploy script consuming config/deploy-logaddresses was compared against
deployRequirements.json. Five contracts had no pre-deploygate:
ReceiverOIF,DeBridgeDlnFacet,EcoFacet,LidoWrapper,MayanFacet— all fiveadded, each address arg annotated with its getter so
immutable-bindings-match-configcoversthem. Live-verified on mainnet:
DLN_SOURCE,PORTAL,MAYAN,OUTPUT_SETTLERall matchconfig (8/8 annotated bindings green).
912f1b45e):contractDependencyReminder.tswalksdeployRequirements.json→contractAddressesin reverse, transitively — deploying theExecutor now warns "ReceiverAcrossV4, ReceiverChainflip, ReceiverOIF, ReceiverStargateV2 bind
it at construction"; deploying ERC20Proxy shows the full chain (
ReceiverAcrossV4 (via Executor)). Non-fatal nudge indeploySingleContract.sh; the binding invariants stay the gate.131f70df7):periphery-registry-log-syncandimmutable-bindings-match-configgain Tron branches (base58 comparison, both zero-encodings guarded).
receiver-executor-bindingstays evm-only with the reason documented inline: no coupledreceiver exists on Tron yet.
82ba279ee): see the resolved section above.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)