fix(contracts): confirm every local deploy write and the refund manager - #2136
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (3)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDeployment and slashing-policy scripts now submit configuration transactions through ChangesDeployment configuration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains; merge after normal checks. Architecture SummaryArchitecture risk: 🟡 Medium · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Reliability and maintainability
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/interfold-contracts/scripts/configureLocalSlashingPolicies.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/interfold-contracts/scripts/deployInterfold.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
a53e04d to
b9c7cd4
Compare
9619942 to
06b23ab
Compare
b9c7cd4 to
730e728
Compare
06b23ab to
2f1c6a9
Compare
6a02a9c to
d08b0ca
Compare
2f1c6a9 to
1cd5ab0
Compare
1cd5ab0 to
0c5088e
Compare
d08b0ca to
dc74d4b
Compare
dc74d4b to
309f1f4
Compare
0c5088e to
ef9b46d
Compare
deployInterfold.ts sent interfoldTicketToken.setRegistry with a bare await, which resolves when the transaction is dispatched, not when it is mined. Several other writes called .wait() directly, so a failure carried no label. The wiring check also did not read back the references that E3RefundManager receives in its initializer. Every configuration write in deployInterfold.ts and configureLocalSlashingPolicies.ts now goes through send(), which waits for the receipt, rejects a missing receipt or a failed status, and names the write. The wiring table reads back E3RefundManager.interfold() and treasury(). With a deliberately wrong treasury, the local deployment now stops with "e3RefundManager.treasury: expected <deployer>, got 0x...01". agent/invariants/04_BUILD_CONFIG.md and 00_INDEX.md no longer list the two gaps. Refs #2068
The wiring check did not read back four references that the deploy sets: BondingRegistry.ticketToken and slashedFundsTreasury, the CiphernodeRegistry DKG fold-attestation verifier, and FOLD's BONDING_REGISTRY. With the gap note removed, 04_BUILD_CONFIG.md said the check covered every reference. The table now reads all four, and the invariant names them. With the BondingRegistry slashed-funds treasury deployed as address(1) on the test host, the deploy stops with "bondingRegistry.slashedFundsTreasury: expected <deployer>, got 0x...01"; before, it printed "Cross-contract wiring verified."
…ring check The wiring check did not compare Interfold's fee token, the ticket token's underlying token, the final BFV decryption, public-key and ciphertext verifiers, or the Sepolia faucet's FOLD and fee token. A ZK deployment that registered the mock public-key verifier, or a deployment with the wrong fee token, printed "Cross-contract wiring verified." and enabled requests. The check now reads all of them. The expected verifiers are the BFV wrappers with ENABLE_ZK_VERIFICATION and the mocks otherwise; the ciphertext verifier is always the mock. On the test host each fault now stops the deploy with the named reference: - ZK path with the mock PK verifier: "interfold.pkVerifiers(BFV)"; - the ticket token as the fee token: "interfold.feeToken"; - the Sepolia branch with the faucet's FOLD set to the fee token: "faucet.fold". Clean mock, ZK and Sepolia deployments pass. 04_BUILD_CONFIG.md lists every compared reference.
The wiring check still skipped references that the deploy sets: the
pricing protocol treasury, FOLD's claim source, BondedVotes' votes source
and, with ZK verification, the BFV wrappers' circuit verifiers and the
decryption wrapper's registry. It also checked one authorization only
(the reward distributor), not the FOLD transfer whitelist or the initial
E3 program registration.
The table now compares all of them, and an authorization list checks the
reward distributor, the FOLD transfer whitelist of the BondingRegistry
(and of the faucet on Sepolia), and the initial E3 program. Every row is
created where the table is built, so a failed read cannot surface early as
an unhandled rejection. 04_BUILD_CONFIG.md states the rule (every
reference and authorization that the script sets) instead of a list.
On the test host, clean mock, ZK and Sepolia deployments pass, and each
fault stops the deploy with the named entry: a wrong pricing treasury
("interfold.pricing.protocolTreasury"), a wrong claim source
("interfoldToken.CLAIM_SOURCE"), and a missing BondingRegistry
whitelist ("interfoldToken.transferWhitelist(bondingRegistry): not
granted").
The wiring rule covers references set through constructor arguments, but the check does not compare a proxy's ERC-1967 implementation and admin slots with the implementation that the deploy helper created. Each deployAndSave helper deploys the implementation and passes it to the proxy constructor in the same function, so only a helper bug can make them disagree. State this as a gap instead of claiming full coverage.
setFeeAssetConfig admits the fee token (_feeTokenAllowed), but the wiring check compared only feeToken(). A deployment whose fee token lost its admission before verification printed "Cross-contract wiring verified." and enabled requests that then failed with FeeTokenNotAllowed. The authorization list now reads isFeeTokenAllowed(feeToken); with the admission revoked on the test host, the deploy stops with "interfold.isFeeTokenAllowed(feeToken): not granted". The 04 gap note claimed that only a helper bug could make a proxy's implementation slot disagree. That holds for fresh deployments only: a helper can reuse a proxy from the deployment record, and its admin can upgrade it later. The note now says so.
309f1f4 to
f79055b
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
What
Fixes the #2068 items "
setRegistryon the ticket token is not awaited" and "The local deployment's wiring check skips the refund manager's references". PR 4 of the Stack 4 stack; base: PR 3.interfoldTicketToken.setRegistry(...)used a bareawait, which resolves when the transaction is dispatched, not when it is mined. Fifteen other writes indeployInterfold.tsand two inconfigureLocalSlashingPolicies.tscalled.wait()directly, so a failure did not name the write. The wiring check did not compare the E3RefundManager references or several others, so a mis-wired deployment could printCross-contract wiring verified.and enable requests.deployInterfold.tsandconfigureLocalSlashingPolicies.tsgoes throughsend(), which waits for the receipt, rejects a missing receipt or a failed status, and names the write. The wiring check also compares:E3RefundManager.interfold()andtreasury();BondingRegistry.ticketToken()andslashedFundsTreasury(), the CiphernodeRegistry DKG fold-attestation verifier, and FOLD'sBONDING_REGISTRY;feeToken()and its BFV decryption, public-key and ciphertext verifiers (the BFV wrappers withENABLE_ZK_VERIFICATION, the mocks otherwise), and the ticket token'sunderlying();fold()andfeeToken();CLAIM_SOURCE(),BondedVotes.votesSource(), and withENABLE_ZK_VERIFICATIONthe BFV wrappers'circuitVerifier()and the decryption wrapper'sciphernodeRegistry();agent/invariants/04_BUILD_CONFIG.mdstates the rule (every reference and authorization that the script sets is read back before requests are enabled) and one gap: the proxies' ERC-1967 implementation and admin slots are not read. It and00_INDEX.mdno longer list the two old gaps.Verification
No unit test can tell
send()from.wait()on an auto-mining node; the deploy runs in CI's integration jobs. On the test host, against a private anvil:mainends withCross-contract wiring verified.: a wrongE3RefundManageror slashed-funds treasury, the mock public-key verifier on the ZK path, the ticket token as Interfold's fee token, the faucet's FOLD set to the fee token on the Sepolia branch, a wrong pricing treasury, a wrong claim source, a missing BondingRegistry whitelist, and a revoked fee-token admission. The Codex review re-ran its own fault set (also the BFV wrapper links and the votes source) on mock, ZK and Sepolia paths.Checklist
committee:newwith each parameter set) on the stack top;tsc --noEmit -p packages/interfold-contracts; prettier.agent/invariants/04_BUILD_CONFIG.md,agent/invariants/00_INDEX.md.tests/integration/base.sh).Summary by CodeRabbit