feat(script): 20260729-deploy-governance-timelock broadcast - #285
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds deterministic governance timelock deployment for Base, Ethereum, and HyperEVM. It validates deployment state, writes a manifest, enforces address invariants, adds fork tests, and updates broadcast verification. ChangesGovernance timelock deployment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ManualBroadcast
participant DeployGovernanceTimelock
participant Network
participant Sourcify
participant Etherscan
Operator->>ManualBroadcast: select governance timelock script
ManualBroadcast->>DeployGovernanceTimelock: run with legacy transactions
DeployGovernanceTimelock->>Network: deploy or verify timelock
DeployGovernanceTimelock-->>ManualBroadcast: write deployment manifest
ManualBroadcast->>Sourcify: verify each manifest deployment
opt Etherscan key exists
ManualBroadcast->>Etherscan: submit verification request
end
Possibly related PRs
Suggested reviewers: 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 |
ec9653a to
72742aa
Compare
e1c1ccd to
61806fa
Compare
72742aa to
bdfaa30
Compare
61806fa to
009ce0a
Compare
bdfaa30 to
f2df295
Compare
009ce0a to
d785f3b
Compare
f2df295 to
3366565
Compare
d785f3b to
b23db21
Compare
Deploys the ST0x governance timelock (unmodified pre-audited OZ TimelockController) on the active chain via the Zoltu deterministic factory, dispatched from manual-broadcast.yaml as the CI deploy key. Constructor-configured, no post-deploy ceremony: minDelay = 48h, the chain's token-owner Safe is sole proposer (hence canceller) + executor, admin = address(0) so the timelock self-administers from birth — the deploy key is never granted any role (asserted in post-state, not just assumed). Deploying grants the timelock no power; it becomes governance admin only when the Safe executes the separate migration bundle. Pre-flight: live Safe policy assert, pin-not-hydrated guard, Zoltu factory codehash pin, derived-address-empty guard (deploy landed but pin PR outstanding surfaces by name). Post-state: full LibTimelockInvariants.assertTimelockState + deployer-holds-nothing sweep. Live-fork tests drive the full run() on Base + Ethereum heads: the deploy lands at the derived per-chain address with the pinned configuration, re-dispatch refuses, unsupported chains refuse, and the holds-nothing check is proven non-vacuous against the Safe itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
…rness The Rain one-contract-per-file convention check fails the static job: the deploy-script test declared both the harness and the test contract. Move the harness into its own file, matching the existing DeployV4AuthoriserCloneHarness pattern.
Drives the deploy against a HyperEVM head fork alongside Base and Ethereum, and records the deliberate consequence of HyperEVM sharing Ethereum's token-owner Safe: the Zoltu-derived timelock address is identical on the two chains, so the equality is asserted rather than discovered at pin time.
…gured HYPEREVM_RPC_URL is not provisioned in CI yet (rainix is adding the RPC_URL_HYPEREVM_FORK slot, RAI-1511) and the static job bans vm.skip, so this matches the PENDING-log-and-return pattern the multichain stack's own HyperEVM suites already use. The assertion stays in the tree and starts running the moment the secret lands.
manual-broadcast.yaml ran forge script with --broadcast but no --verify and no explorer key, so nothing deployed through it has ever been verified — unlike the rainix suite deploys. Adds --verify and forwards the per-chain CI_DEPLOY_*_ETHERSCAN_API_KEY secrets that foundry.toml's [etherscan] block resolves by network alias. HyperEVM had no [etherscan] entry at all. It has no dedicated *scan deployment; Etherscan's multichain v2 API serves it (chain 999, explorer hyperevmscan.io), so the api url pins the chainid explicitly — the same reason the ethereum entry pins chain = 1. Multi-chain scripts broadcast to HyperEVM within a single run whatever network was dispatched, so they always take the legacy-tx path that HyperEVM's fee-history-less RPC requires.
forge script --verify cannot serve a multi-chain broadcast: it resolves one explorer from --rpc-url, then fails to locate the contracts that landed on the other chains — the Ethereum timelock was submitted to Base's explorer and the run failed with 'Unable to locate ContractCode'. run() now writes the per-chain addresses to out/, and a dedicated step verifies each against its own explorer. Reading a manifest rather than the broadcast artifact also keeps verification working on a re-dispatch, where every chain is skipped and no transactions are produced. --guess-constructor-args reads the creation tx from the explorer, so the per-chain Safe baked into the constructor is not duplicated in the workflow.
Etherscan's v2 multichain API rejects a free-tier key for anything beyond
mainnet ('Free API access is not supported for this chain'), and the
Ethereum and HyperEVM key secrets were never set at all. Sourcify covers all
three chains — including HyperEVM — with no API key, and matches on bytecode
so neither constructor args nor an RPC endpoint are needed.
Trade-off recorded: Sourcify is a separate index, so this does NOT surface as
verified on Basescan. Explorers that ingest Sourcify (Blockscout) show it;
Etherscan does not.
Etherscan does not ingest Sourcify, so Sourcify alone never surfaces on Basescan — which was the original gap. Run both: Sourcify unconditionally (free, and the only verifier covering HyperEVM), and Etherscan per chain only where a key is configured, so a missing or free-tier key skips that leg instead of failing the deploy. Setting a paid v2 key later turns the Etherscan leg on with no further change.
…ION_KEY
Every working deploy in this org verifies through rainix, which resolves
each chain's key as EXPLORER_VERIFICATION_KEY first and only then falls back
to the per-chain CI_DEPLOY_*_ETHERSCAN_API_KEY. That single unified key is an
Etherscan v2 multichain key, which is why verification has never been a
problem on the rainix path.
This workflow read the per-chain secrets directly instead — a stale free-tier
key on Base ('Free API access is not supported for this chain'), and nothing
at all for Ethereum and HyperEVM. Adopting the same resolution order makes it
use the key that already works.
0f09c0a to
78aeb1d
Compare
92e25f4 to
263e26a
Compare
# Conflicts: # .github/workflows/manual-broadcast.yaml
![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 @.github/workflows/manual-broadcast.yaml:
- Line 36: Update the concurrency configuration for the
20260729-deploy-governance-timelock workflow so its group is derived solely from
the workflow/script identity and does not include inputs.network. Ensure every
dispatch shares the same concurrency group and is serialized across all target
chains.
In `@test/script/20260729-deploy-governance-timelock.t.sol`:
- Around line 40-46: Add a test alongside testNetworksCoversEveryProductionChain
that sets block.chainid to an unsupported value, calls
DeployGovernanceTimelockHarness.callDeployOnActiveChain(), and asserts the
expected pre-flight revert. Use the existing harness and revert expectation
conventions so the deployment allowlist’s rejection behavior is covered.
🪄 Autofix
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 Plus
Run ID: ea6e41dc-0e37-467e-908c-a9bf2d1ee5f2
📒 Files selected for processing (5)
.github/workflows/manual-broadcast.yamlfoundry.tomlscript/20260729-deploy-governance-timelock.s.soltest/script/20260729-deploy-governance-timelock.t.soltest/script/DeployGovernanceTimelockHarness.sol
Two surfaces treated address(0) as a case to route around instead of the single most likely drift value to catch: - testPinsMatchDerivedAddressesOnceHydrated guarded each pin equality with 'if not zero' — hydration-lifecycle machinery that is now spent (all three deploys executed) and inverted into a hole: zeroing a pin made the test silently skip. Renamed testPinsMatchDerivedAddresses; the three equalities are now unconditional. A future chain's placeholder phase gets its own guarded branch when its arm is added; these three never go back. - LibMigrationInvariant's address overload accepted actual == post, so on any chain in the unhydrated window (post pin == 0) an owner renounced to address(0) read as 'already migrated' and passed until the deadline. The overload now rejects a zero actual unconditionally, on either side of the deadline: for every address-valued surface it asserts (owners, role holders) zero is the default/renounced/unset reading, so it is its own checked case and an unhydrated post means 'post-state unreachable', never 'post-state equals whatever reads as unset'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in refusal The manual-broadcast concurrency group keyed on script+network, which is right for single-chain scripts (per-chain nonces cannot collide across networks) but wrong for the timelock deploy: it broadcasts on all three chains regardless of the selected network, so two dispatches with different network inputs ran concurrently with the same deploy key on the same chains. Multi-chain scripts now share one network-independent group, with the property documented at the ternary. The allowlist's rejection arm had no discriminating test: nothing drove _deployOnActiveChain on a chain outside the governed set. Covered on the local EVM (31337) asserting the typed UnsupportedChainForTokenOwnerSafe pre-flight revert — the deploy cannot reach broadcast machinery on a chain with no pinned Safe policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The deploy script guard-skipped the pin cross-check on a zero pin and fell through to the idempotent-skip branch with a 'PIN OUTSTANDING' log — the zero case routed around instead of checked. That bootstrap phase no longer exists: with the creation bytecode frozen, a chain's pin is a pure function of its Safe pin and is written with the chain arm before any deploy, so zero can only mean a reverted or never-hydrated arm and now refuses with typed TimelockNotPinned. The pin-vs-derivation cross-check became unconditional and the dead PIN OUTSTANDING branch is gone. Pin resolution is virtual (pinnedTimelock) mirroring the migration script's pattern, so UnpinnedDeployGovernanceTimelockHarness can simulate the zeroed arm: testDeployRefusesZeroPin proves the refusal on a Base fork — on the pre-fix code that exact scenario returned success against the live deployment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
timelockForChainId still documented zero as 'until deployed and the pin hydrated' — the retired doctrine. Pins derive from the frozen creation bytecode before any deploy; zero is a reverted or never-hydrated arm that every consumer refuses. Co-Authored-By: Claude Fable 5 <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 (3)
.github/workflows/manual-broadcast.yaml (1)
150-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when manifest parsing fails.
Without
pipefail, a missing or invalid manifest can makejqfail while the emptywhileloop exits successfully. The workflow then reports successful verification without verifying any deployment.Enable
set -euo pipefailbefore the pipeline. Validate that${MANIFEST}is a nonempty JSON array before iterating.Proposed fix
run: | + set -euo pipefail MANIFEST=out/20260729-governance-timelock-deployments.json CONTRACT=dependencies/@openzeppelin-contracts-5.6.1/governance/TimelockController.sol:TimelockController + jq -e 'type == "array" and length > 0' "${MANIFEST}" >/dev/null jq -c '.[]' "${MANIFEST}" | while read -r entry; do🤖 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 @.github/workflows/manual-broadcast.yaml around lines 150 - 153, Update the verification run block in the manual broadcast workflow to enable set -euo pipefail before the jq pipeline, then validate that MANIFEST exists, is nonempty, and parses as a JSON array before iterating. Keep the existing jq-based deployment verification flow unchanged after this validation.script/20260729-deploy-governance-timelock.s.sol (2)
225-227: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the pin hand-off instruction.
The pre-flight at lines 183-189 requires a nonzero pin that already equals the derived address. A post-execution pin PR cannot enable this deployment.
Remove the instruction to hydrate
STOX_GOVERNANCE_TIMELOCK*after deployment. State that the pin must be merged before broadcast.🤖 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/20260729-deploy-governance-timelock.s.sol` around lines 225 - 227, Update the operator comment near the timelock address log to remove the instruction to hydrate the LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK* pin after deployment, and instead state that the matching nonzero pin must be merged before broadcast.
194-197: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the deployer on the idempotent path.
Start the broadcast context before this branch and capture
address deployer = msg.sender. Call_assertPostState(expected, safe, deployer)before returning. Stop the broadcast context before the early return. Otherwise, a re-dispatch can accept a timelock whose deployer holds a privileged role.🤖 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/20260729-deploy-governance-timelock.s.sol` around lines 194 - 197, Update the idempotent deployment branch around expected and safe to start the broadcast context and capture msg.sender as deployer before checking expected.code.length. Invoke _assertPostState(expected, safe, deployer) before returning the existing deployment, and stop the broadcast context before the early return while preserving the current invariant validation and logging.
🤖 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 `@test/src/lib/LibMigrationInvariant.t.sol`:
- Around line 204-210: Add a complementary test next to the existing `post ==
address(0)` case in `LibMigrationInvariant` that sets `pre` to `address(0)`
while `actual` is also zero and executes before `DEADLINE`; assert
`harness.callAssertMigrationAddress` reverts with the expected
`MigrationStateDrift` selector and arguments, preserving the existing deadline
setup and test structure.
---
Outside diff comments:
In @.github/workflows/manual-broadcast.yaml:
- Around line 150-153: Update the verification run block in the manual broadcast
workflow to enable set -euo pipefail before the jq pipeline, then validate that
MANIFEST exists, is nonempty, and parses as a JSON array before iterating. Keep
the existing jq-based deployment verification flow unchanged after this
validation.
In `@script/20260729-deploy-governance-timelock.s.sol`:
- Around line 225-227: Update the operator comment near the timelock address log
to remove the instruction to hydrate the
LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK* pin after deployment, and
instead state that the matching nonzero pin must be merged before broadcast.
- Around line 194-197: Update the idempotent deployment branch around expected
and safe to start the broadcast context and capture msg.sender as deployer
before checking expected.code.length. Invoke _assertPostState(expected, safe,
deployer) before returning the existing deployment, and stop the broadcast
context before the early return while preserving the current invariant
validation and logging.
🪄 Autofix
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 Plus
Run ID: 671f26c4-ca35-477b-8fef-1d4070bed0ec
📒 Files selected for processing (8)
.github/workflows/manual-broadcast.yamlscript/20260729-deploy-governance-timelock.s.solsrc/lib/LibMigrationInvariant.solsrc/lib/LibTimelockInvariants.soltest/script/20260729-deploy-governance-timelock.t.soltest/script/UnpinnedDeployGovernanceTimelockHarness.soltest/src/lib/LibMigrationInvariant.t.soltest/src/lib/LibTimelockInvariants.t.sol
Completes the configuration walk in testAddressZeroActualAlwaysRejected: hydrated both sides, zero post, and now zero pre — a zero pre side must not bless a zero actual any more than a zero post does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed 50c4400: ship — multi-chain timelock deploy broadcast whose three executions produced the live, frozen-generation-verified timelocks; manifest-driven explorer verification; zero-pin refusal with fork-tested fail-closed pre-flight; serialized multi-chain dispatch; every CI check green and no unresolved review threads. |
2 participants
Deploys the ST0x governance timelock (unmodified pre-audited OZ
TimelockController) on the active chain via the Zoltu deterministic
factory, dispatched from manual-broadcast.yaml as the CI deploy key.
Constructor-configured, no post-deploy ceremony: minDelay = 48h, the
chain's token-owner Safe is sole proposer (hence canceller) + executor,
admin = address(0) so the timelock self-administers from birth — the
deploy key is never granted any role (asserted in post-state, not just
assumed). Deploying grants the timelock no power; it becomes governance
admin only when the Safe executes the separate migration bundle.
Pre-flight: live Safe policy assert, pin-not-hydrated guard, Zoltu factory
codehash pin, derived-address-empty guard (deploy landed but pin PR
outstanding surfaces by name). Post-state: full
LibTimelockInvariants.assertTimelockState + deployer-holds-nothing sweep.
Live-fork tests drive the full run() on Base + Ethereum heads: the deploy
lands at the derived per-chain address with the pinned configuration,
re-dispatch refuses, unsupported chains refuse, and the holds-nothing
check is proven non-vacuous against the Safe itself.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
Summary by CodeRabbit
New Features
Bug Fixes
Tests