fix(healthCheck): eliminate false failures + refresh target state (68/71 networks red -> green) - #2120
Conversation
The scheduled Diamond Health Check reported 68/71 networks failing. Most of that was the check disagreeing with config, not real on-chain drift. - whitelist-integrity: resolve periphery addresses from the `address` field in config/whitelist.json instead of by name from the deployments file. Contracts we whitelist but do not deploy (Composer) were absent from deployments, so every one of their on-chain pairs was reported stale; name-keyed lookup also cannot represent the several distinct Composer addresses per network. Warn, rather than silently skip, when an entry resolves to nothing, and warn when a config address disagrees with one we did deploy. Fixes 22 networks. - core facets: add CORE_FACET_EXEMPTIONS so a facet can be core going forward. LiFiIntentEscrowFacet stays in coreFacets, so newly onboarded networks are enforced by default, while the networks that predate that decision are exempt until the facet is backfilled. Table is validated in tests and the exemption is printed per network. Fixes 58 networks. - remove the feecollector-owner invariant: FeeCollector is deprecated and its owner is no longer maintained against config.feeCollectorOwner. - drop ReceiverAcrossV3 from the receiver/executor binding check: deprecated in favour of ReceiverAcrossV4, which stays checked along with the other three. - invariant banner no longer prints its severity as a `[error]`-style prefix. It runs once per invariant per network, so the token made every passing check match a grep for '[error]' and made a healthy run read as mass failure. Verified against production RPCs: katana (previously stale-pair failure) and flow (previously missing-facet failure) both pass; xdc no longer runs the FeeCollector owner check. Not addressed here (real drift, tracked separately): ERC20Proxy owner on 6 networks, and 8 networks with genuinely missing facets. 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:
WalkthroughHealth checks now support network-specific core-facet exemptions, authoritative whitelist periphery addresses, updated facet derivation, deprecated-check removal, improved Tron handling, refreshed deployment state, and new zkSync facet update scripts. ChangesHealth-check and deployment updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/deploy/healthCheckInvariants.ts (2)
191-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
readonlyfor the exemption fields.
ICoreFacetExemptiondescribes a static, never-mutated configuration table. Marking its fieldsreadonlywould make that intent explicit and prevent accidental mutation.♻️ Proposed diff
export interface ICoreFacetExemption { /** Facet name as listed in `config/global.json` → `coreFacets`. */ - facet: string + readonly facet: string /** Why these networks are exempt, including the ticket that documents the decision. */ - reason: string + readonly reason: string /** Network keys (as in config/networks.json) that predate the facet becoming core. */ - networks: string[] + readonly networks: readonly string[] }🤖 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 191 - 198, Update the ICoreFacetExemption interface so facet, reason, and networks are readonly, reflecting that exemption configuration is static and preventing accidental mutation.Source: Coding guidelines
500-544: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIsolate per-entry failures so one bad config address doesn't blank out all expected pairs.
getAddress(contractAddr as Address)throws on a malformed address; since the periphery loop shares the outer try/catch with the DEXS loop, one badconfig/whitelist.jsonentry (now a human-edited source, unlike the deployments file) would discard every already-computed DEX pair and return[], silently disabling whitelist checks for the whole network for that run instead of just excluding/warning on the one bad entry.♻️ Proposed approach
Wrap just the per-
peripheryContractbody in its own try/catch, callinglogWarn/logErrorfor that entry andcontinue, so a single malformed entry doesn't blank out unrelated DEX/periphery pairs already collected.🤖 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 500 - 544, Isolate processing of each peripheryContract in the periphery loop so malformed addresses cannot abort the outer health-check flow. Wrap the per-entry address resolution and selector iteration, including getAddress, in a try/catch; log a warning or error identifying the affected entry, then continue to the next peripheryContract while preserving already collected DEX and periphery expected pairs.
🤖 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 504-540: Update the mismatch warning in the periphery whitelist
loop around `networkPeripheryContracts` and
`deployedContracts[peripheryContract.name]` so it is skipped when multiple
entries share the same name on the network. Keep the config address as the
source of truth and retain the warning for uniquely named entries where the
deployed address can be compared unambiguously.
---
Nitpick comments:
In `@script/deploy/healthCheckInvariants.ts`:
- Around line 191-198: Update the ICoreFacetExemption interface so facet,
reason, and networks are readonly, reflecting that exemption configuration is
static and preventing accidental mutation.
- Around line 500-544: Isolate processing of each peripheryContract in the
periphery loop so malformed addresses cannot abort the outer health-check flow.
Wrap the per-entry address resolution and selector iteration, including
getAddress, in a try/catch; log a warning or error identifying the affected
entry, then continue to the next peripheryContract while preserving already
collected DEX and periphery expected pairs.
🪄 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: 22861d7a-95e9-475f-98fe-f03d4213c996
📒 Files selected for processing (3)
script/deploy/healthCheck.tsscript/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.ts
…d names The staleness warning compared each periphery entry's config address against a single deployedContracts[name] lookup. When several entries share one name on a network — the Composer case this PR exists to support — at most one could match, so the rest would warn spuriously. Skip the comparison when the name is not unique on that network; deployedContracts cannot disambiguate it. Latent today (Composer is the only duplicated name and is in no deployments file, so the branch was unreachable), but it would fire the moment any shared name got deployed. Exports getExpectedPairs and covers the resolution rules with unit tests: config address wins when the contract is undeployed, all same-name entries are kept, no staleness warning on shared names, warning on a unique-name mismatch, and a warning rather than a silent skip when nothing resolves. Found by CodeRabbit on #2120. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
script/deploy/healthCheckInvariants.test.ts (2)
287-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse descriptive helper names and explicit return types.
Rename
SEL,A,B,cfg, andrunto describe their roles, and declare return types for the helper functions. As per coding guidelines, TypeScript functions need explicit return types and variables/functions must use descriptive names.🤖 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 287 - 317, In the test helpers, rename SEL, A, B, cfg, and run to descriptive role-based names, and add explicit return types to the cfg and run helper functions. Keep their existing behavior and signatures otherwise unchanged.Source: Coding guidelines
358-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover deployed-address fallback when config is empty.
This only tests the fully unresolved path. Add a case with
address: ''and a matchingdeployedentry, asserting that its selector pair is emitted without a reduced-coverage warning; that protects theconfigAddr || deployedAddrcontract.🤖 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 358 - 363, Add a test alongside the existing unresolved case in the health-check invariant suite using an empty config address and a matching deployed entry. Assert that the resolved selector pair is emitted and no reduced-coverage warning is produced, covering the configAddr || deployedAddr fallback behavior.
🤖 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.test.ts`:
- Around line 287-317: In the test helpers, rename SEL, A, B, cfg, and run to
descriptive role-based names, and add explicit return types to the cfg and run
helper functions. Keep their existing behavior and signatures otherwise
unchanged.
- Around line 358-363: Add a test alongside the existing unresolved case in the
health-check invariant suite using an empty config address and a matching
deployed entry. Assert that the resolved selector pair is emitted and no
reduced-coverage warning is produced, covering the configAddr || deployedAddr
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f4ef7625-5371-404b-ac92-6d9be7a25e5e
📒 Files selected for processing (2)
script/deploy/healthCheckInvariants.test.tsscript/deploy/healthCheckInvariants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/healthCheckInvariants.ts
nonCoreFacets was derived by filtering target state against coreFacetsToCheck — the core list AFTER per-network exclusions. So a core facet excluded for a network (GasZip unsupported, or grandfathered via CORE_FACET_EXEMPTIONS) was absent from that list, fell through the filter as "non-core", and was re-checked via target state anyway. The exclusion printed, then the facet failed regardless. Verified before the fix by adding LiFiIntentEscrowFacet to flow's target state: the exemption logged and non-core-facets-deployed still reported it missing. After the fix the same input passes. This is why updating the exemption table alone cannot suppress the alerts when a facet is listed in both coreFacets and target state. Extracts the filter into deriveNonCoreFacets so the invariant is testable, and documents that callers must pass the FULL core list. Six unit tests, including one that pins the bug shape when a pre-excluded list is handed in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PROVISIONAL — pending confirmation that V2 fully replaces V1. Revert this commit alone to undo; the two preceding fixes are independent of it. Swaps coreFacets from LiFiIntentEscrowFacet to LiFiIntentEscrowFacetV2 and retargets CORE_FACET_EXEMPTIONS to V2, recomputed from deployments: V2 is live on 10 production networks, so 63 are exempt until backfilled. Needed because the exemption mechanism only covers CORE facets. Leaving V2 non-core while target state lists it on 75 networks would fail on the 63 where it is not deployed, with nothing available to exempt them. Verified against production RPCs with the refreshed target state applied: flow (exempt) passes, katana (V2 deployed) checks and passes V2. Also verified with the current unmodified target state so this commit stands alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d FeeCollector from corePeriphery Folds the target-state refresh into this PR. The two changes are coupled and must land together: target state now lists LiFiIntentEscrowFacetV2 on 75 networks, which only passes because this PR makes V2 the core facet and exempts the 63 where it is not yet deployed. Landing the refresh alone would fail those 63. Target state regenerated from the production sheet (1995 -> 2056 entries): - deprecations removed: FeeCollector (71 networks), LiFiDEXAggregator (70), ReceiverAcrossV3 (19), DexManagerFacet (3), GenericSwapFacet (tron) - not-supported-there removals: DeBridgeDlnFacet/boba, SquidFacet/sei, AcrossFacetV4 + AcrossFacetPackedV4 + ArbitrumBridgeFacet + ReceiverAcrossV4/plume - LiFiIntentEscrowFacet -> LiFiIntentEscrowFacetV2 - tron row added (was absent from the sheet, so a full refresh previously wiped tron's 20 entries to 0) - 363 version upgrades, no downgrades FeeCollector is deprecated, so it is removed from corePeriphery — otherwise core-periphery-deployed keeps requiring it on every network regardless of target state, which is the same false-failure class this PR removes. Deliberately NOT removing FeeCollector or LiFiDEXAggregator from whitelistPeripheryFunctions or config/whitelist.json: both are still whitelisted on-chain (75 and 74 networks). Removing them from config first would report every one of those pairs as "stale, not in config" — the exact failure this PR fixes. They must be unwhitelisted on-chain first, then dropped from config. (LiFiDEXAggregator was never in corePeriphery, so nothing to remove there.) Verified against production RPCs: flow (was missing-facet), katana (was stale pairs), xdc and botanix (were FeeCollector owner) all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0xDEnYO
changed the title
…m whitelistPeripheryFunctions Both are deprecated. Removing them from whitelistPeripheryFunctions stops updateWhitelistPeriphery / diamondUpdatePeriphery treating them as whitelist-eligible and stops checkDeploymentAddressConsistency demanding whitelist.json coverage for them. Safe with respect to whitelist-integrity: getExpectedPairs builds the expected pair set from config/whitelist.json PERIPHERY, not from whitelistPeripheryFunctions, so this cannot turn on-chain pairs into "stale, not in config". Verified — katana (20 pairs) and botanix (11 pairs) both still report the pair array synced. Also safe with respect to periphery-registered: that invariant only considers contracts present in the network's target state, and the target-state refresh in this PR already removed both (FeeCollector on 71 networks, LiFiDEXAggregator on 70). Still deliberately untouched: config/whitelist.json, which lists FeeCollector on 75 networks and LiFiDEXAggregator on 74 and matches what is whitelisted on-chain. Removing them there before unwhitelisting on-chain would report ~149 pairs as stale. Order has to be diamondSyncWhitelist first, then the config removal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 QA Review — EXSC-680 / PR #2120 (Post-Approval Re-Review — Run #24)Ticket: EXSC-680 — Diamond health check: eliminate false failures (68/71 networks red) + refresh target state Post-Approval Commits Reviewed
No Post-Approval Code Review
|
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Pass (Run #20) — TypeScript health check tooling fix: getExpectedPairs address resolution, deriveNonCoreFacets pre-exclusion list, CORE_FACET_EXEMPTIONS, deprecated check removal, and target state refresh all correct and well-tested. Merge gate: await backend confirmation of LiFiIntentEscrowFacetV2 switch-over before merging.
…tis CalldataVerificationFacet
Both are records drift, not on-chain drift. No on-chain change in this commit.
avalanche DiamondCutFacet: the diamond has 0xf7993A8d…58ABf registered, while
deployments/avalanche.json recorded 0xaD501185…957013, which is not among
facetAddresses(). Verified 0xf7993A8d is the genuine facet before repointing:
- serves exactly one selector, 0x1f931c1c = diamondCut((address,uint8,bytes4[])[],address,bytes)
- runtime bytecode is byte-identical to the recorded contract once the trailing
solc metadata is excluded; both solc 0.8.17. Only the metadata hash differs,
explained by optimizer runs (10000 vs 1000000)
- Routescan reports it verified as ContractName "DiamondCutFacet"
Created 2022-10-18 (block 21218935), i.e. EARLIER than the recorded 2023-07-20
address: the diamond runs the original facet and the 2023 redeploy was never
registered. Both entries are kept in the deployment log, chronologically.
This went undetected because the avalanche.diamond.json facet entry had an empty
Name, and checkDeploymentAddressConsistency only indexes diamond facets that have
one — so the mismatch was invisible to it. Name/Version are now filled in, which
also clears the no-unexpected-facets warning for that address.
metis CalldataVerificationFacet: deployed and registered at 0x78659196…312E1, and
already correct in metis.diamond.json and the deployment log (v1.1.1, verified),
but missing from the flat deployments/metis.json the health check reads. Added.
Verified: consistency check passes; avalanche and metis both pass the health check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…and PioneerFacet Sheet re-parsed after two upstream edits. Only four entries change: - blast/CBridgeFacetPacked (not supported there) - PioneerFacet on arbitrum, mainnet, soneium (deprecated, confirmed by Alexander; its own deprecation PR is in flight) No additions, no version changes. blast and mainnet now pass the health check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e blob
callTronContract prepends TronWeb's diagnostic lines ("⚙ Initializing TronWeb…",
"⚙ Calling <fn> on <addr>", …) to the actual return value, so the address is the
LAST meaningful line. parseTronAddressOutput trimmed the whole blob, which then
started with the first diagnostic line and failed every "is this a T… address"
check — so a correctly registered contract was reported as absent.
On tron this produced four false failures in periphery-registered (ERC20Proxy,
Executor, FeeForwarder, OutputValidator, each "got: null") while the registry
actually held the right addresses. Probed directly to confirm: getPeripheryContract
returns TDCo8wrq…dtcn9 / TUJnqSDo…kAbht / TBiwoFZp…dAF8, all matching deployments.
periphery-registered also had its own ad-hoc trim instead of the shared helper —
despite the Executor-binding branch documenting that it "mirror[s] the parsing used
by the periphery-registration check". Both now go through parseTronAddressOutput.
Six unit tests, including the exact diagnostic-prefixed output shape.
Verified against tron mainnet: all four periphery contracts now report registered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deployed at 0x2E09CeC715CB767F15c46aF8c33ff780eaAbFa74 via the standard path (deploySingleContract -> DeployCalldataVerificationFacet.zksync.s.sol, pinned foundry-zksync v0.0.32, zksolc + solc 0.8.29). Cost 0.000111 ETH. Verified after deploy: - runtime bytecode at the address is byte-identical to zkout/CalldataVerificationFacet.sol/CalldataVerificationFacet.json - already verified on the zksync explorer - deployment record written to MongoDB (contract-deployments) zksync was the only production network missing this core facet. It is NOT yet registered in the diamond — that needs a governance diamondCut, proposed separately. Note: the deploy wrote the record to MongoDB only; deployments/_deployments_log_file.json was not updated by the run (update-deployment-logs.ts syncs JSON -> MongoDB, not the reverse), so the tracked master log does not yet contain this entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tron was the last network failing purely on rate limits: refund-wallet-access makes one call per approved selector and TronGrid throttled anonymous traffic before the check could finish, which reads as drift rather than reduced coverage. The viem path already sends TRON-PRO-API-KEY via the devkit's applyTronGridViemTransportExtras, but the health check's tron reads go through callTronContract -> a `bun troncast` subprocess, which talks to TronWeb directly and never saw the key. Wired it into initTronWeb using the devkit's own env var (TRONGRID_API_KEY) and header constant, gated on isTronGridRpcUrl. Also passes the secret to the nightly workflow, which previously supplied only MONGODB_URI — so CI would have kept 429-ing regardless of local config. Measured on tron mainnet: 429 count 43 -> 0. Tron's remaining failure is unrelated (whitelist config load), tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
diamondUpdateFacet resolves zksync scripts as script/deploy/zksync/<name>.zksync.s.sol and aborts when the file is absent, so a facet with only an EVM update script cannot be registered on zksync at all. That currently blocks registering CalldataVerificationFacet v2.0.0, which is deployed on zksync but not yet in the diamond. Adds update scripts for the three facets that are deployed on zksync but had none: - CalldataVerificationFacet (core; blocks the pending registration) - CBridgeFacetPacked - SymbiosisFacet Each is the standard one-line UpdateScriptBase wrapper, identical in structure to the 13 existing zksync update scripts (verified by diff against the EVM counterpart). Not included, deliberately: - CBridgeFacet and LiFiIntentEscrowFacetV2 are in zksync's target state but not deployed there, so update scripts alone would not help. CBridgeFacet is worth following up on: config/cbridge.json HAS a zksync entry and CBridgeFacetPacked is already deployed there, so the base facet looks like an oversight rather than an incompatibility — but it needs a deploy script plus a deployment, not just this. - AccessManagerFacet, DiamondCutFacet, DiamondLoupeFacet, OwnershipFacet and PeripheryRegistryFacet are covered by UpdateCoreFacets.zksync.s.sol. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
script/troncast/utils/tronweb.ts (1)
53-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the API-key gating branches.
Please add or confirm tests for a trimmed key with a TronGrid URL, a key with a non-TronGrid URL, and a blank key. These cases prevent regressions that silently restore anonymous health-check traffic.
🤖 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/troncast/utils/tronweb.ts` around lines 53 - 72, Add tests covering the TronWeb initialization flow around the API-key header setup: verify a trimmed TRONGRID_API_KEY is sent for TronGrid URLs, no header is sent for non-TronGrid URLs, and blank or whitespace-only keys are omitted. Reuse the existing TronWeb construction and RPC URL test utilities, and assert the resulting headers for each branch.script/deploy/zksync/UpdateCBridgeFacetPacked.zksync.s.sol (2)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a lowercase contract name.
DeployScriptviolates the repository rule requiring lowercase Solidity contract names. Rename this entrypoint consistently and update any Forge references that selectDeployScript.🤖 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/zksync/UpdateCBridgeFacetPacked.zksync.s.sol` at line 7, Rename the DeployScript contract to a lowercase-compliant name and update all Forge configuration or invocation references that select DeployScript so the deployment entrypoint remains unchanged.Source: Coding guidelines
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the contract-naming rule consistently.
The three entrypoints declare
contract DeployScript, but the Solidity guideline requires lowercase contract names. Rename each entrypoint contract inscript/deploy/zksync/UpdateCBridgeFacetPacked.zksync.s.sol,UpdateCalldataVerificationFacet.zksync.s.sol, andUpdateSymbiosisFacet.zksync.s.sol, and update any Forge invocation/artifact lookup that targetsDeployScript.🤖 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/zksync/UpdateCBridgeFacetPacked.zksync.s.sol` at line 1, Rename the entrypoint contracts from DeployScript to lowercase names in UpdateCBridgeFacetPacked.zksync.s.sol, UpdateCalldataVerificationFacet.zksync.s.sol, and UpdateSymbiosisFacet.zksync.s.sol, then update every corresponding Forge invocation and artifact lookup to use the new contract names.Source: Coding guidelines
script/deploy/zksync/UpdateCalldataVerificationFacet.zksync.s.sol (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a lowercase contract name.
DeployScriptviolates the repository rule requiring lowercase Solidity contract names. Rename this entrypoint consistently and update any Forge references that selectDeployScript.🤖 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/zksync/UpdateCalldataVerificationFacet.zksync.s.sol` at line 7, Rename the Solidity contract DeployScript to a lowercase contract name, and update all Forge configuration or invocation references that select DeployScript to use the new name consistently.Source: Coding guidelines
script/deploy/zksync/UpdateSymbiosisFacet.zksync.s.sol (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a lowercase contract name.
DeployScriptviolates the repository rule requiring lowercase Solidity contract names. Rename this entrypoint consistently and update any Forge references that selectDeployScript.🤖 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/zksync/UpdateSymbiosisFacet.zksync.s.sol` at line 7, Rename the DeployScript contract to a lowercase name in the UpdateSymbiosisFacet deployment entrypoint, and update all Forge configuration or invocation references that select DeployScript to use the new name consistently.Source: Coding guidelines
🤖 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/zksync/UpdateCalldataVerificationFacet.zksync.s.sol`:
- Line 7: Rename the Solidity contract DeployScript to a lowercase contract
name, and update all Forge configuration or invocation references that select
DeployScript to use the new name consistently.
In `@script/deploy/zksync/UpdateCBridgeFacetPacked.zksync.s.sol`:
- Line 7: Rename the DeployScript contract to a lowercase-compliant name and
update all Forge configuration or invocation references that select DeployScript
so the deployment entrypoint remains unchanged.
- Line 1: Rename the entrypoint contracts from DeployScript to lowercase names
in UpdateCBridgeFacetPacked.zksync.s.sol,
UpdateCalldataVerificationFacet.zksync.s.sol, and
UpdateSymbiosisFacet.zksync.s.sol, then update every corresponding Forge
invocation and artifact lookup to use the new contract names.
In `@script/deploy/zksync/UpdateSymbiosisFacet.zksync.s.sol`:
- Line 7: Rename the DeployScript contract to a lowercase name in the
UpdateSymbiosisFacet deployment entrypoint, and update all Forge configuration
or invocation references that select DeployScript to use the new name
consistently.
In `@script/troncast/utils/tronweb.ts`:
- Around line 53-72: Add tests covering the TronWeb initialization flow around
the API-key header setup: verify a trimmed TRONGRID_API_KEY is sent for TronGrid
URLs, no header is sent for non-TronGrid URLs, and blank or whitespace-only keys
are omitted. Reuse the existing TronWeb construction and RPC URL test utilities,
and assert the resulting headers for each branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a27754c-220c-4404-9788-3580a0f7ab58
📒 Files selected for processing (5)
.github/workflows/healthCheckAllNetworks.ymlscript/deploy/zksync/UpdateCBridgeFacetPacked.zksync.s.solscript/deploy/zksync/UpdateCalldataVerificationFacet.zksync.s.solscript/deploy/zksync/UpdateSymbiosisFacet.zksync.s.solscript/troncast/utils/tronweb.ts
There was a problem hiding this comment.
QA Pass (Run #21, post-approval re-review) — 6 post-approval commits are clean tooling additions: tron parseTronAddressOutput fix is well-tested, TRONGRID_API_KEY wired correctly to CI and tronweb, zksync update scripts follow established repo convention, _targetState.json cleanup consistent with PioneerFacet deprecation. Merge gate carry-forward: await backend confirmation of LiFiIntentEscrowFacetV2 switch-over before merging.
…gnostics
The whitelist-integrity invariant parsed callTronContract's raw output for
getAllContractSelectorPairs() with .trim() + startsWith('['). That output is
prefixed by the troncast command echo ($ bun run ...) and TronWeb's diagnostic
lines -- one of which, "Formatted params: []", itself contains a '['. So the
trim left it starting with the echo line, startsWith('[') was false, and it
threw "Expected array format", which the outer catch swallowed into the generic
"Whitelist configuration not available" -- the last remaining tron failure.
Add a tested shared helper parseTroncastArrayOutput that strips the echo and
diagnostic lines before locating the bracketed payload (mirroring the fix
already applied to parseTronAddressOutput), and surface the real error/stack in
the catch so future throws are diagnosable rather than masked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t source-into-zsh The stale/missing-pair remediation printed `source script/tasks/diamondSyncWhitelist.sh && diamondSyncWhitelist <net> <env>`. Sourcing runs the #!/bin/bash script's body in the caller's interactive shell, and its `read -ra` (a bash-only `read` option) fails under zsh -- the macOS default -- with "bad option: -a", leaving the network list empty so the sync runs against `./deployments/.json`. Point at ./script/tasks/syncWhitelistToNetworks.sh instead, which executes under its own bash shebang. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Pass (Run #22, post-approval re-review) — 2 post-approval commits: tron parseTroncastArrayOutput fix (well-tested) and whitelist bash wrapper remediation hint. No Solidity changes.
tempo is active/mainnet with LiFiIntentEscrowFacetV2 in its target state but the facet is not yet deployed there, so core-facets-deployed would go red — the same not-yet-backfilled situation as the other exempted networks. Add it to CORE_FACET_EXEMPTIONS so the check stays green with a printed exemption until the facet is backfilled, rather than reintroducing a red the PR is meant to eliminate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Pass (Run #24, post-approval re-review) — 2 new commits: routine merge from main + single-line tempo exemption added to CORE_FACET_EXEMPTIONS (caught by melianessa, verified correct: tempo is active/mainnet with V2 in target state but not yet deployed — identical pattern to the 63 other exempted networks). Merge gate carry-forward: await backend confirmation of LiFiIntentEscrowFacetV2 switch-over.
3 participants
Which Linear task belongs to this PR?
EXSC-680
Note
BE confirmation (2026-07-28): Backend has switched over to
LiFiIntentEscrowFacetV2. The V2-as-core + target-state couple in this PR is unblocked for merge.Why did I implement it this way?
The nightly
Diamond Health Check (all networks)reported 68 of 71 networks failing (run 30192494683). Bucketing every per-network reason showed it was 6 root causes, and most of the volume was the check disagreeing with config rather than real on-chain drift. This PR fixes the parts that are ours to fix in code; the genuine drift is called out below and left alone.LiFiIntentEscrowFacetincoreFacetsbut deployed on 13 chainsFeeCollectorowner mismatchReceiverAcrossV3.executor()mismatchERC20Proxyowner mismatch54 of the 68 go green from this PR alone.
1.
whitelist-integritywas resolving periphery addresses by the wrong keygetExpectedPairslooked periphery contracts up asdeployedContracts[peripheryContract.name], ignoring theaddressfield thatconfig/whitelist.jsoncarries (and thatIWhitelistConfigdeclares as required). Two consequences:Composeris whitelisted but never deployed by this repo, so it is absent from everydeployments/*.json.contractAddrcame backundefined, the entry was silently skipped, and all of its on-chain pairs were then reported as stale.Composeraddresses — e.g.PERIPHERY.arbitrumhas three, which is exactly the "3 stale pairs" in the log.Config is the right source of truth for a check that asks "does the diamond's whitelist match config". I also made the two failure modes visible instead of silent: a warning when an entry resolves to no address at all, and a warning when a config address disagrees with a contract we did deploy (that means the whitelist entry is stale and
diamondSyncWhitelistwould whitelist the wrong address).2. Making a facet core "going forward" —
CORE_FACET_EXEMPTIONSLiFiIntentEscrowFacetwas added tocoreFacetsin #1859, which makes the check demand it on all networks, but it was only deployed to the chains in #1997. Rather than drop it fromcoreFacets, this adds a narrow, per-network exemption table:coreFacets, so any newly onboarded network is absent from the table and therefore enforced by default — the safe direction, and what "core going forward" should mean;Deliberately narrower than the existing
HEALTH_CHECK_EXCLUSIONS, which disables a whole invariant on a network and would have cost coverage for every other core facet. This drops one facet from the expected set and leavescore-facets-deployed/facets-registeredotherwise fully enforced.The
networkslist is a shrinking to-do, not a steady state: delete a network the moment the facet lands there, and delete the entry when the list empties. Four tests guard it — the facet must really be incoreFacets, every network must exist inconfig/networks.json, the reason must be non-empty, no duplicates — so a stale exemption fails CI instead of quietly hiding a gap. Exemptions apply to the health check only;deployCoreFacets.shstill readscoreFacets, so deploying it everywhere stays a one-command job.3. Deprecated checks
feecollector-ownerremoved (FeeCollector is deprecated; its owner is no longer maintained againstconfig.feeCollectorOwner, which the deploy scripts still read).ReceiverAcrossV3dropped fromRECEIVER_EXECUTOR_GETTERS— superseded by V4, which stays checked along with Chainflip, OIF and StargateV2. The now-unusedfeeCollectorOwnercontext plumbing is removed rather than left dead.4. The log was overstating severity
executeInvariantprinted its banner as`[${invariant.severity}] ${name} — ${description}`. That banner runs once per invariant per network, so ~1.3k passing checks rendered as[error] diamond-deployed — LiFiDiamond is deployed, each immediately followed by[success] LiFiDiamond deployed. Grep-indistinguishable from real failures, and most of why the run reads as catastrophic. Nowname — description (severity: error).Verification
Run against production RPCs from this branch:
✔ Pair Array (getAllContractSelectorPairs) is synced. (20 pairs),✔ Deployment checks passed✔ Deployment checks passed, exit 0bun test script/→ 570 pass / 0 fail.tsc --noEmit, eslint and prettier clean.Deliberately not in scope
ERC20Proxyowner on bob, etherlink, jovay, morph, nibiru, swellchain — real drift, needs the two-step ownership transfer via governance. feat(scripts): ERC20Proxy ownership remediation toolkit (EXSC-660) #2088 looks like it covers exactly this.Could not enumerate ERC20Proxy authorized callers (RPC log range limit?)— one unchunkedeth_getLogssilently skipping that sub-check on 45 networks. Real coverage loss, pre-existing, worth its own PR.Checklist before requesting a review
🤖 Generated with Claude Code