feat(invariants): governance-timelock migration-window forcing functions - #287
Conversation
📝 WalkthroughWalkthroughThe change adds a receipt-vault ownership migration invariant and harness access. It also adds fork-based governance timelock migration tests for Base, Ethereum, and optional HyperEVM, plus chain-map and unexpected-owner checks. ChangesGovernance timelock migration invariants
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GovernanceTimelockMigrationTest
participant LibSafeInvariantsHarness
participant ReceiptVault
GovernanceTimelockMigrationTest->>LibSafeInvariantsHarness: resolve pinned Safe for chain
LibSafeInvariantsHarness-->>GovernanceTimelockMigrationTest: return Safe address
GovernanceTimelockMigrationTest->>ReceiptVault: validate owner and _ADMIN holder
ReceiptVault-->>GovernanceTimelockMigrationTest: return migration state
Possibly related PRs
Suggested labels: 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 |
c7d353c to
bd359aa
Compare
fa417a8 to
a4d3b1a
Compare
195bd48 to
e3d5ff7
Compare
6743df4 to
fa5842f
Compare
54124fc to
38ca6c8
Compare
024b9c7 to
6834dcc
Compare
38ca6c8 to
84cabb3
Compare
6834dcc to
beb665d
Compare
84cabb3 to
260c991
Compare
beb665d to
0edf040
Compare
260c991 to
009d091
Compare
c767a32 to
34e2037
Compare
009d091 to
b173b41
Compare
GovernanceTimelockMigration.t.sol: live-fork (Base + Ethereum head) forcing function for the whole timelock rollout. Vault ownership and each authoriser _ADMIN role must be Safe (pending) or timelock (landed) until 2026-10-01T00:00:00Z; only the timelock after. Split holding (both) and orphaned roles (neither) map to sentinels that trip MigrationStateDrift immediately. While a chain's timelock pin is a placeholder the post-state is unreachable, so the suite logs a loud PENDING and the deadline forces the full rollout — deploy broadcast, pin hydration, Safe execution — not just the final step. LibTokenInvariants.assertUniformOwnershipMigration: chain-parametric ownership analogue of assertUniformAuthoriserMigration, so the invariant merges alongside the migration script instead of waiting for on-chain execution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
The governance-timelock rollout and the multichain (HyperEVM) rollout are independent stacks, so whichever merges second must reconcile the chain tables with the timelock's chain map. Without a guard, a multichain stack landing first would put production tokens on a chain that assertChainMigrationWindow never walks — no vault ownership assertion, no beacon assertion, no deadline — and nothing would go red. The gap is invisible precisely because the forcing function is enumerated per chain. A chain counts as governed once it has a pinned token-owner Safe; from then on timelockForChainId must resolve it rather than reverting UnsupportedChainForGovernanceTimelock. HyperEVM's chain id is listed as an inert candidate today, so the guard is armed before that stack lands and fires the moment it does. Triggering on the Safe pin rather than a populated token table fires at chain bootstrap rather than first token deploy; eager is the right bias here, and the fix is a placeholder pin slot exactly like Base and Ethereum carry today. Needs no fork — both resolvers are pure.
…guard Adds HyperEVM's migration-window leg alongside Base and Ethereum, and points the coverage guard at the real LibSafeInvariants.HYPEREVM_CHAIN_ID now that the multichain stack sits underneath this one — replacing the locally-declared candidate id that was there to pre-arm the guard. The guard is no longer inert for HyperEVM: the chain has a pinned Safe and now a timelock arm, and removing that arm fails the guard by name. The window leg soft-skips while HYPEREVM_RPC_URL is unprovisioned in CI (RAI-1511), so the fork-free guard is what actually holds HyperEVM's coverage today.
The drift test asserted address(0) as the accepted post value, which was only correct while the timelock pin was a placeholder. With the pins hydrated the expected revert carries the real timelock, so read it from the constant rather than restating a zero that is now wrong.
b173b41 to
f11215e
Compare
…iately Same ruling as the deploy script: zero is a checked case, never a skipped one. The window suite soft-logged PENDING and accepted the pre-state when a chain's pin read zero — machinery for an unhydrated phase that no longer exists (pins are pure functions of the frozen creation bytecode and each chain's Safe pin, written with the chain arm before any deploy). A zero pin now fails the suite outright as a reverted or never-hydrated arm, deadline notwithstanding, and the timelock != address(0) short-circuit in the exclusive-holder walk is gone with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A chain arm never resolves to an unhydrated address(0) pin any more — zero is a checked failure everywhere. The guard's doc now states what it actually asserts: the arm exists; the pin's value is asserted non-zero and derivation-equal elsewhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@test/src/concrete/deploy/GovernanceTimelockMigration.t.sol`:
- Around line 244-263: Update testOwnershipMigrationRejectsThirdOwner to warp vm
time to a timestamp below GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE before invoking
callAssertUniformOwnershipMigration, while preserving its MigrationStateDrift
assertion. Add a companion test covering the deadline boundary by warping to
GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE and asserting MigrationDeadlinePassed.
- Around line 70-76: Update adminRoles() to populate its seven entries using the
explicit _ADMIN role identities rather than grants[i].role positional access.
Preserve the expected seven-role length, but ensure reordering or changes in
expectedGrants() cannot silently alter the admin role mapping.
🪄 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: 7f5ad301-e520-44b0-8786-f610f0c7b6c2
📒 Files selected for processing (4)
src/lib/LibTokenInvariants.soltest/src/concrete/deploy/GovernanceTimelockMigration.t.soltest/src/lib/LibSafeInvariantsHarness.soltest/src/lib/LibTokenInvariantsHarness.sol
| function adminRoles() internal pure returns (bytes32[] memory roles) { | ||
| RoleGrant[] memory grants = LibAuthoriserInvariants.expectedGrants(address(0), address(1)); | ||
| roles = new bytes32[](7); | ||
| for (uint256 i = 0; i < 7; i++) { | ||
| roles[i] = grants[i].role; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect expectedGrants ordering and the _ADMIN role constants.
fd -t f 'LibAuthoriserInvariants.sol' -x ast-grep outline {} --items all
fd -t f 'LibAuthoriserInvariants.sol' -x rg -n -C3 '_ADMIN|RoleGrant\(|new RoleGrant\[\]' {}Repository: S01-Issuer/st0x.deploy
Length of output: 8502
🏁 Script executed:
#!/bin/bash
set -e
target=$(fd -t f 'GovernanceTimelockMigration.t.sol' | head -n1)
lib=$(fd -t f 'LibAuthoriserInvariants.sol' | head -n1)
printf '%s\n' "--- target: $target ---"
sed -n '45,95p' "$target"
printf '%s\n' "--- relevant library: $lib ---"
sed -n '115,175p' "$lib"
python3 - "$target" "$lib" <<'PY'
import re, sys
target, lib = sys.argv[1:3]
t = open(target).read()
l = open(lib).read()
m = re.search(r'function\s+adminRoles\s*\(\)\s+internal\s+pure.*?\{(.*?)\n\s*\}', t, re.S)
assert m, "adminRoles() not found"
body = m.group(1)
length = re.search(r'roles\s*=\s*new\s+bytes32\[\]\((\d+)\)', body)
loop = re.search(r'for\s*\([^;]+;\s*i\s*<\s*(\d+)', body)
print("adminRoles_length:", length.group(1) if length else None)
print("adminRoles_loop_bound:", loop.group(1) if loop else None)
gm = re.search(r'grants\s*=\s*new\s+RoleGrant\[\]\((\d+)\)', l)
assert gm, "expectedGrants allocation not found"
count = int(gm.group(1))
entries = re.findall(r'grants\[(\d+)\]\s*=\s*RoleGrant\(keccak256\("([^"]+)"\)', l)
print("expectedGrants_length:", count)
print("expectedGrants_role_entries:", entries)
print("first_seven_are_admin_suffixes:", all(name.endswith("_ADMIN") for _, name in entries[:7]))
print("admin_entries_count:", sum(name.endswith("_ADMIN") for _, name in entries))
print("admin_indices:", [int(i) for i, name in entries if name.endswith("_ADMIN")])
PYRepository: S01-Issuer/st0x.deploy
Length of output: 7072
Bind adminRoles() to role identities
expectedGrants() currently places the seven _ADMIN roles at indices 0–6. Build adminRoles() from explicit role identities instead of positional indices. A length assertion only detects truncation; it does not detect reordering.
🤖 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 `@test/src/concrete/deploy/GovernanceTimelockMigration.t.sol` around lines 70 -
76, Update adminRoles() to populate its seven entries using the explicit _ADMIN
role identities rather than grants[i].role positional access. Preserve the
expected seven-role length, but ensure reordering or changes in expectedGrants()
cannot silently alter the admin role mapping.
| function testOwnershipMigrationRejectsThirdOwner() external { | ||
| vm.createSelectFork(LibRainDeploy.BASE); | ||
| address safe = LibSafeInvariants.STOX_TOKEN_OWNER_SAFE; | ||
| address stranger = address(0xBAD); | ||
| TokenInstance[] memory tokens = LibTokenInvariants.productionTokensBase(); | ||
| vm.mockCall(tokens[0].receiptVault, abi.encodeWithSignature("owner()"), abi.encode(stranger)); | ||
|
|
||
| LibTokenInvariantsHarness harness = new LibTokenInvariantsHarness(); | ||
| vm.expectRevert( | ||
| abi.encodeWithSelector( | ||
| MigrationStateDrift.selector, | ||
| "receiptVault.owner()", | ||
| bytes32(uint256(uint160(safe))), | ||
| bytes32(uint256(uint160(LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK))), | ||
| bytes32(uint256(uint160(stranger))) | ||
| ) | ||
| ); | ||
| harness.callAssertUniformOwnershipMigration( | ||
| tokens, safe, LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK, GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Warp below the deadline so this test keeps asserting drift.
This test forks Base head, so block.timestamp is real. LibMigrationInvariant.assertMigration reverts with MigrationDeadlinePassed, not MigrationStateDrift, once block.timestamp >= GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE. From 2026-10-01 this test fails on the wrong revert data, and the claim in the doc comment ("deadline notwithstanding") stops holding.
The other tests in this suite are time-sensitive by design. This one is not; it asserts a deadline-independent property. Pin the timestamp below the deadline.
🐛 Proposed fix to make the drift assertion deadline-independent
function testOwnershipMigrationRejectsThirdOwner() external {
vm.createSelectFork(LibRainDeploy.BASE);
+ // Pre-deadline is the branch under test: the post-deadline branch
+ // reverts `MigrationDeadlinePassed` instead.
+ vm.warp(GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE - 1);
address safe = LibSafeInvariants.STOX_TOKEN_OWNER_SAFE;Add a companion test that warps to GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE and expects MigrationDeadlinePassed. Do you want me to generate it?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function testOwnershipMigrationRejectsThirdOwner() external { | |
| vm.createSelectFork(LibRainDeploy.BASE); | |
| address safe = LibSafeInvariants.STOX_TOKEN_OWNER_SAFE; | |
| address stranger = address(0xBAD); | |
| TokenInstance[] memory tokens = LibTokenInvariants.productionTokensBase(); | |
| vm.mockCall(tokens[0].receiptVault, abi.encodeWithSignature("owner()"), abi.encode(stranger)); | |
| LibTokenInvariantsHarness harness = new LibTokenInvariantsHarness(); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| MigrationStateDrift.selector, | |
| "receiptVault.owner()", | |
| bytes32(uint256(uint160(safe))), | |
| bytes32(uint256(uint160(LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK))), | |
| bytes32(uint256(uint160(stranger))) | |
| ) | |
| ); | |
| harness.callAssertUniformOwnershipMigration( | |
| tokens, safe, LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK, GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE | |
| ); | |
| function testOwnershipMigrationRejectsThirdOwner() external { | |
| vm.createSelectFork(LibRainDeploy.BASE); | |
| // Pre-deadline is the branch under test: the post-deadline branch | |
| // reverts `MigrationDeadlinePassed` instead. | |
| vm.warp(GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE - 1); | |
| address safe = LibSafeInvariants.STOX_TOKEN_OWNER_SAFE; | |
| address stranger = address(0xBAD); | |
| TokenInstance[] memory tokens = LibTokenInvariants.productionTokensBase(); | |
| vm.mockCall(tokens[0].receiptVault, abi.encodeWithSignature("owner()"), abi.encode(stranger)); | |
| LibTokenInvariantsHarness harness = new LibTokenInvariantsHarness(); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| MigrationStateDrift.selector, | |
| "receiptVault.owner()", | |
| bytes32(uint256(uint160(safe))), | |
| bytes32(uint256(uint160(LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK))), | |
| bytes32(uint256(uint160(stranger))) | |
| ) | |
| ); | |
| harness.callAssertUniformOwnershipMigration( | |
| tokens, safe, LibTimelockInvariants.STOX_GOVERNANCE_TIMELOCK, GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE | |
| ); |
🤖 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 `@test/src/concrete/deploy/GovernanceTimelockMigration.t.sol` around lines 244
- 263, Update testOwnershipMigrationRejectsThirdOwner to warp vm time to a
timestamp below GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE before invoking
callAssertUniformOwnershipMigration, while preserving its MigrationStateDrift
assertion. Add a companion test covering the deadline boundary by warping to
GOVERNANCE_TIMELOCK_MIGRATION_DEADLINE and asserting MigrationDeadlinePassed.
|
Reviewed b7e76f5: ship — the migration-window forcing function: per-chain live-fork assertions that every production vault owner and every authoriser _ADMIN role sits on exactly one accepted side (Safe pending / timelock landed), sentinel-flagged dual-holder and orphan states tripping drift immediately, a hard failure on any zero timelock pin, the chain-map coverage guard closing the parallel-stack onboarding gap, and the 2026-10-01T00:00:00Z operator SLA with fail-safe >= boundary red-lining cron on an unfinished rollout. All checks green, no unresolved review threads. |

GovernanceTimelockMigration.t.sol: live-fork (Base + Ethereum head)
forcing function for the whole timelock rollout. Vault ownership and each
authoriser _ADMIN role must be Safe (pending) or timelock (landed) until
2026-10-01T00:00:00Z; only the timelock after. Split holding (both) and
orphaned roles (neither) map to sentinels that trip MigrationStateDrift
immediately. While a chain's timelock pin is a placeholder the post-state
is unreachable, so the suite logs a loud PENDING and the deadline forces
the full rollout — deploy broadcast, pin hydration, Safe execution — not
just the final step.
LibTokenInvariants.assertUniformOwnershipMigration: chain-parametric
ownership analogue of assertUniformAuthoriserMigration, so the invariant
merges alongside the migration script instead of waiting for on-chain
execution.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
Summary by CodeRabbit
Bug Fixes
Tests