feat(authoriser): additional service signer — canonical pin + provisioning - #280
Conversation
|
Warning Review limit reached
Next review available in: 49 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 (2)
📝 WalkthroughWalkthroughAdds a Safe-based script for provisioning an additional service signer on Base and Ethereum. It expands authoriser grant invariants, updates network-aware script dispatch, increases mirrored grants from six to nine, and adds fork and unit coverage. ChangesService signer provisioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant ProvisionAdditionalServiceSigner
participant Safe
participant Authoriser
Dispatcher->>ProvisionAdditionalServiceSigner: select network and run script
ProvisionAdditionalServiceSigner->>Authoriser: validate clone and missing grants
ProvisionAdditionalServiceSigner->>Safe: simulate grantRole bundle
Safe->>Authoriser: apply missing role grants
ProvisionAdditionalServiceSigner->>Safe: simulate revoke and re-grant proof
Possibly related issues
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 |
9d5c95a to
0949e8a
Compare
hardyjosh
changed the title
0949e8a to
f3c68e9
Compare
hardyjosh
changed the title
b215cce to
4663984
Compare
4663984 to
d168ee1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/20260619-deploy-v4-authoriser-clone.s.sol`:
- Around line 127-135: Update the Step 2 and post-state comments near
MIRROR_START_INDEX and MIRROR_COUNT in
script/20260619-deploy-v4-authoriser-clone.s.sol to describe nine grants instead
of six. Also update the happy-path and replica comments near line 335 in
test/script/20260619-deploy-v4-authoriser-clone.t.sol to say nine grants, with
no code behavior changes.
In `@script/20260723-provision-additional-service-signer.s.sol`:
- Around line 153-157: Update the additional-role validation loop to first
assert that roleAdminOf(additional[i].role) matches
acl.getRoleAdmin(additional[i].role), before checking acl.hasRole for safeAddr.
Preserve the existing Safe admin-grant validation and revert behavior after this
live role-admin mapping check.
In `@src/lib/LibAuthoriserInvariants.sol`:
- Around line 63-74: The current V4 authoriser pin is incorrectly reused for
migration; add a distinct retired V3 predecessor constant in
LibAuthoriserInvariants while retaining STOX_PROD_AUTHORISER for assertAll(). In
src/lib/LibAuthoriserInvariants.sol lines 63-74, define and use the V3 pin for
migration. In src/lib/LibInvariants.sol lines 39-40, update the migration-window
documentation and call path to reference that separate V3 predecessor.
🪄 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 Plus
Run ID: 7bb8c455-3d76-4196-9880-d7831315e852
📒 Files selected for processing (9)
.github/workflows/run-script.yamlscript/20260619-deploy-v4-authoriser-clone.s.solscript/20260723-provision-additional-service-signer.s.solsrc/lib/LibAuthoriserInvariants.solsrc/lib/LibInvariants.soltest/script/20260619-deploy-v4-authoriser-clone.t.soltest/script/20260722-swap-remaining-vault-authorisers.t.soltest/script/20260722-swap-rklb-authoriser.t.soltest/script/20260723-provision-additional-service-signer.t.sol
💤 Files with no reviewable changes (2)
- test/script/20260722-swap-remaining-vault-authorisers.t.sol
- test/script/20260722-swap-rklb-authoriser.t.sol
…ning, post-swap test retirement Rebased onto the #254 grant-map restructure (13-entry parametric form, STOX_PROD_AUTHORISER repointed to the live authoriser upstream). GRANTEE_SERVICE_3D0C joins GRANTEE_SERVICE_1C66 as an ADDITIONAL service signer with the same three action roles: - expectedGrants() 13 -> 16; every consumer (parity authoriser leg, production-state bundle, per-chain prod pins) is red per chain until the provisioning bundle executes there, drift-guarded after. 3D0C joins the DEFAULT_ADMIN absence checks. Map docs state purpose, not contents. - 20260619 clone-deploy MIRROR_COUNT 6 -> 9 (+ suite): the ceremony runs again on future chains and its slice guard forces the constant to track the canonical map; executed chains refuse re-runs via hydrated pins. - 20260723-provision-additional-service-signer (run-script, registered): authors the per-chain grant bundle, work items derived from the map, chain-aware (base/ethereum network selector added to the dispatcher), self-scoping, n+1-proven reversible. Fork suite green on live Base + Ethereum. - Post-execution retirement: the executed swap authorings' one-shot test suites deleted; live-state coverage is carried by the standing invariants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPs1hCTxusmaSeFKvoc4Kr
d168ee1 to
cf8599b
Compare
The fork suite reads live Base and Ethereum. Production moved after the last run (2026-07-29) when the additional service signer was signed on mainnet, so that run's result describes a state that no longer exists.
…ocks The provisioning selection was only reachable through `run()`, which reads `block.chainid` and the pinned production Safe + authoriser, so every scenario about WHICH pairs get authored had to be a fork test with `vm.mockCall` standing in for the role state. A mocked fork fixture is a premise the next legitimate operation deletes: signing the bundle on Base made two of those tests fail, because the script was right and the fixture was stale. Extract the selection into `authorBundle(authoriser, safeAddr)` — same pre-flight, same self-scoping, same reverts, every chain-specific input now an argument — and drive it from a harness against a real EIP-1167 clone of the real authoriser whose role state is built by real `grantRole` calls under the Safe's `_ADMIN` roles. `run()` calls the same function, so the logic under test is the logic that ships. Scenarios covered without a fork or a mock: nothing provisioned authors all three pairs; CERTIFY held authors only DEPOSIT/WITHDRAW; DEPOSIT held authors only WITHDRAW/CERTIFY (the bundle is not a prefix of the map); every pair held reverts `AdditionalSignerAlreadyProvisioned`; a missing non-signer row reverts as drift. The last two were previously mock-only or uncovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A prod test must assert what production IS, not what production needed when the PR was written. The two fork tests this replaces asserted the latter — one drove `run()` to completion on Base and Ethereum, the other mocked CERTIFY as held — and both were falsified the moment the Safe signed the bundle, which is the correct outcome for that shape of test, not a regression. Measured directly on both chains: Base `0x315b16faa6eE413faBCa877d3851 B3818369f0cD` and Ethereum `0x66566cc91dEAf818859bD4b09B7903ac48998157` each return true for DEPOSIT, WITHDRAW and CERTIFY on `GRANTEE_SERVICE_3D0C`, with identical clone codehashes and no pinned grantee holding `DEFAULT_ADMIN_ROLE`. The rollout is complete on every chain carrying a live authoriser. So the prod layer asserts that, unmocked, per chain: the signer holds every canonical pair, and `run()` refuses to author an empty bundle there. `run()` reaches that refusal only after its full pre-flight passes, so each test also pins the chain's Safe, the authoriser pin and codehash, and the rest of the grant map. A revoke on either chain turns this red, which is exactly the drift signal wanted. `testRunRevertsWhenAlreadyProvisioned` survives here with its `vm.mockCall`s deleted — on a genuinely provisioned chain it needed none — and now runs per chain rather than Base only. Flips the script's status line to EXECUTED, verified against the chains the prod tests read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Provisioning tests split into a genuine unit layer and a genuine prod layerThe two red tests were red because the script is correct and their premise had expired: the bundle has been signed and executed, so Measured per-chain, per-role matrixRead directly off both chains (
for Ethereum is in the same state as Base, not a different one. The full 16-row The script's status line is flipped to Where each scenario went
Net: 3 fork tests (2 of them mocked) -> 5 unit tests + 2 prod tests. No scenario lost; two gained. Unit layer — no fork, no mocksThe selection was only reachable through The fixture is a real The bundle assertions check the authored calldata ( Prod layer — real forks, no mocks
What was deleted, and why it is safe
The shared Two things worth a reviewer's eye
Verification
|
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 `@script/20260723-provision-additional-service-signer.s.sol`:
- Around line 46-49: Correct the HyperEVM claim in the script comments: since
activeChainAuthoriser does not support HyperEVM and reverts
UnsupportedChainForProvisioning, remove the assertion that HyperEVM is
dispatchable after bootstrap and state that this script supports only Base and
Ethereum, unless an actual HyperEVM branch is implemented.
In `@test/script/20260723-provision-additional-service-signer.t.sol`:
- Around line 110-116: Add a reusable helper in
test/script/20260723-provision-additional-service-signer.t.sol that verifies all
three canonical RoleGrant entries, including role, grantee, and ordering, then
invoke it for the returned additional array at lines 110-116, 126-131, and
142-146. At the final site, retain the additional result before asserting its
canonical contents; leave the existing transaction-selection assertions intact.
🪄 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: 0341732b-e846-4c23-8e97-4d16ba7c10a9
📒 Files selected for processing (4)
script/20260723-provision-additional-service-signer.s.soltest/script/20260723-provision-additional-service-signer.prod.t.soltest/script/20260723-provision-additional-service-signer.t.soltest/script/ProvisionAdditionalServiceSignerHarness.sol
Two fixes and one documented falsification. HyperEVM: `activeChainAuthoriser()` has only Base and Ethereum branches and reverts `UnsupportedChainForProvisioning` for anything else, so the header claim that HyperEVM becomes dispatchable once its authoriser bootstraps was wrong — it needs a branch here first. Say that instead. Canonical pairs: the unit tests asserted `additional.length` but not its contents, so a wrong role, wrong grantee or wrong order could pass. Add `assertCanonicalPairs` and call it in all three selection scenarios; the last one now retains `additional` rather than discarding it. Order is load-bearing — it decides which pair drives the n+1 reversal walk. Role-admin mapping: NOT reading `getRoleAdmin` live is deliberate, and the reason now sits in `roleAdminOf`'s NatSpec rather than being implied. The hierarchy is written by `_setRoleAdmin` inside `initialize` and nowhere else — OpenZeppelin's `_setRoleAdmin` is `internal` and `AccessControlUpgradeable` exposes no external setter — so an implementation's mapping is fixed at initialisation and immutable after. `activeChainAuthoriser()` already asserts the clone's runtime codehash equals the pinned EIP-1167 runtime embedding the audited 0.1.1 impl, which is what establishes WHICH mapping the clone carries. A live `getRoleAdmin` read would re-derive that, and could never fail on a clone that passed the codehash gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The admin-holder parameterisation was written against a 13-entry map; the multichain stack underneath now carries the three additional service-signer rows from #280, so the merged map is 16. The structure test still asserted 13 and failed the restacked branch. Widens it to 16 and asserts the new rows explicitly: the additional signer's three action roles are OPERATIONAL, so they must track the signer regardless of who holds the _ADMIN slice — the property that keeps the timelock migration from ever moving them.
3 participants
A second service EOA —
GRANTEE_SERVICE_3D0C(0x3d0CD66EFA66c05d86c3d4316B03eAE87ab9E8aE) — joinsGRANTEE_SERVICE_1C66as an additional signer with the same three action roles (DEPOSIT/WITHDRAW/CERTIFY). Both signers active; nothing revoked.Current-state invariants (per review — no time-sliced side lists). The signer's rows go straight into the canonical
expectedGrants()map (11 → 14). Every consumer enforces the provisioning per chain — the cross-chain parity authoriser leg, the multichain production-state bundle, and the per-chain prod pins are RED on a chain until its bundle executes, then drift-guarded.Consequences handled on their own terms:
assertAll()repointed to the current production authoriser — the V4 clone (codehash pin + grant map). The retired V3 authoriser is no longer asserted: asserting it against the current map would demand grants nobody will provision on a retired contract; a frozen V3 list would be a time slice. Its constant survives only as the migration-windowprevalue.MIRROR_COUNT6 → 9 — the map grew and the script's own slice guard (GrantsSliceOutOfRange(5,6,14)) exists precisely to force this update; without it the script is un-dispatchable on any chain. Executed chains are unaffected (hydrated pins refuse re-runs); a future chain's ceremony provisions the full current map.Authoring.
20260723-provision-additional-service-signer(run-script, registered; network selector added): work items derived from the map's 3D0C rows, chain-aware, self-scoping, grants-only, n+1-proven reversible. Fork suite green on live Base + Ethereum.Execute:
run-script→20260723-provision-additional-service-signer/run()/ networkbase, sign; repeatethereum. HyperEVM needs no bundle — its clone-deploy ceremony provisions the full current map.🤖 Generated with Claude Code
https://claude.ai/code/session_01VPs1hCTxusmaSeFKvoc4Kr
Summary by CodeRabbit
New Features
Updates
Tests