fix(test): assert ownership of the IN-USE production beacons per chain - #266
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProduction beacon definitions and chain dispatch are added for Base and Ethereum. Beacon invariants now verify deployment and Safe ownership, while deployment tests centralize ownership checks and add coverage for routing and failure cases. ChangesProduction beacon invariants
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeploymentTest
participant LibBeaconInvariants
participant ProdBeaconSet
participant ChainSafe
DeploymentTest->>LibBeaconInvariants: assertProdBeaconsOwnedByChainSafe(chainId)
LibBeaconInvariants->>ProdBeaconSet: resolve three beacons
LibBeaconInvariants->>ProdBeaconSet: check code.length and owner()
LibBeaconInvariants->>ChainSafe: resolve expected Safe
ChainSafe-->>LibBeaconInvariants: return Safe address
LibBeaconInvariants-->>DeploymentTest: complete or revert with typed error
Poem
🚥 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 |
83bec06 to
898c31a
Compare
898c31a to
70d49c5
Compare
`testProdDeployEthereumV4` failed because `checkProd_0_1_1OnChain` asserted every 0.1.1-address beacon is owned by `BEACON_INITIAL_OWNER` (rainlang.eth) — stale since the Ethereum beacon-ownership migration moved them to the Safe. The deeper problem: the check conflated deploy-artifact integrity with live ownership, and pinned the owner of beacons production doesn't even run on. Restructure around what actually matters operationally: - `LibBeaconInvariants.prodBeaconsForChainId(chainId)` — single source of truth for the beacons production tokens RUN ON per chain: Base = the V1-generation addresses (`LibProdDeployV1`), Ethereum = the 0.1.1 set (`LibProdBeaconsEthereum`). Beacon addresses are per-chain artifacts that never change; only their implementations upgrade. - `assertProdBeaconsOwnedByChainSafe(chainId)` — every in-use beacon is owned by THAT chain's token-owner Safe (whoever owns an in-use beacon can repoint every production vault proxy on the chain). Where beacons POINT is the cross-chain parity pin's concern, not duplicated here. - `checkProd_0_1_1OnChain` drops its owner parameter and owner asserts entirely: it now checks deploy-artifact integrity only (code, codehash, constructor wiring). Base's unadopted 0.1.1-address beacons no longer have their owner pinned — dead state that says nothing about production. - Both network tests call the per-chain in-use ownership assert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPs1hCTxusmaSeFKvoc4Kr
18acfcf to
4d875cc
Compare
70d49c5 to
1934241
Compare
`prodBeaconsForChainId` and `assertProdBeaconsOwnedByChainSafe` had no unit coverage: the per-chain dispatch, the unsupported-chain revert, the not-deployed guard and the owner comparison were exercised only incidentally by the network tests, which cannot distinguish "the right beacons are Safe-owned" from "some Safe-owned beacons are". `LibProdBeaconsBase` claimed to name the beacons Base production RUNS ON and nothing enforced it — the only code that ever read a production proxy's beacon slot, `LibTokenInvariantsAddressesTest.checkTokenSet`, has no callers and still asserts the pre-migration owner. So the new test reads the beacon out of every production token on a live fork and pins the lib against it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed test coverage for the new library surface (3a54ed0) — the review found it had none, and the network tests can't substitute because they cannot distinguish "the right beacons are Safe-owned" from "some Safe-owned beacons are".
All 10 pass. Mutation-validated — each mutant killed by the intended test:
Separately verified against live chains: all three Base V1 beacons are owned by Not addressed here, for you to judge: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/LibProdBeaconsBase.sol`:
- Line 5: Update LibProdBeaconsBase to stop importing or referencing
LibProdDeployV1. Add the required retained Base-V1 beacon aliases to the current
versioned deployment library, such as LibProdDeployV4, then replace all
production references in LibProdBeaconsBase with those aliases while preserving
the existing addresses and codehashes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfa5bbb2-cfe1-4fec-a9dc-cd9cab15a4f7
📒 Files selected for processing (6)
src/lib/LibBeaconInvariants.solsrc/lib/LibProdBeaconsBase.soltest/src/concrete/deploy/StoxProdV4.t.soltest/src/lib/LibBeaconInvariants.t.soltest/src/lib/LibBeaconInvariantsHarness.soltest/src/lib/LibProdBeaconsBase.t.sol
|
@coderabbitai review |
✅ Action performedReview finished.
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviewed f06de9b: approve The fresh test run (29951072083) confirms this does what it claims: Verified independently against live chains rather than taking the description on trust: all three Base V1-generation beacons are owned by The restructure is right. Conflating deploy-artifact integrity with live ownership is what made Test coverage added in 3a54ed0 (mutation-validated, 4/4 mutants killed by the intended test). Worth flagging what writing it turned up: Two notes left for you, neither blocking: |

testProdDeployEthereumV4failed becausecheckProd_0_1_1OnChainasserted every 0.1.1-address beacon is owned byBEACON_INITIAL_OWNER(rainlang.eth) — stale since the Ethereum beacon-ownership migration moved them to the Safe. The deeper problem: the check conflated deploy-artifact integrity with live ownership, and pinned the owner of beacons production doesn't even run on (Base's unadopted 0.1.1-address set).Restructured around what matters operationally:
LibBeaconInvariants.prodBeaconsForChainId(chainId)— single source of truth for the beacons production tokens RUN ON per chain: Base = the V1-generation addresses (LibProdDeployV1), Ethereum = the 0.1.1 set (LibProdBeaconsEthereum). Beacon addresses are per-chain artifacts that never change; only their implementations upgrade.assertProdBeaconsOwnedByChainSafe(chainId)— every in-use beacon is owned by THAT chain's token-owner Safe (whoever owns an in-use beacon can repoint every production vault proxy on the chain). Where beacons POINT is the cross-chain parity pin's concern, not duplicated here.checkProd_0_1_1OnChaindrops its owner parameter and owner asserts entirely: deploy-artifact integrity only (code, codehash, constructor wiring). Base's unadopted 0.1.1-address beacons no longer have their owner pinned.Verified against live Base + Ethereum: both network tests pass.
Sits at the bottom of the Ethereum-multichain stack so the stale owner assert doesn't red-line the PRs above it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VPs1hCTxusmaSeFKvoc4Kr
Summary by CodeRabbit
New Features
Tests