Conversation
Adds a reusable L2TaskBase template for atomically updating batcherHash and unsafeBlockSigner on SystemConfig in a single batched Multicall3 tx from FoundationUpgradeSafe. Mirrors the SystemConfigGasParams pattern. Includes an OP Sepolia example task and a pinned-calldata regression test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Template Review Findings
Blocking Issues
None found.
Warnings
-
[SetBatcherAndSigner.sol:52-53] No zero-address guards in
_templateSetup()forbatcherAddressandunsafeBlockSigner. If a zero address is accidentally supplied in config,setBatcherHashwould setbytes32(0)andsetUnsafeBlockSignerwould setaddress(0), silently breaking sequencing. Consider:require(batcherAddress != address(0), "SetBatcherAndSigner: batcherAddress is zero"); require(unsafeBlockSigner != address(0), "SetBatcherAndSigner: unsafeBlockSigner is zero");
-
[SetBatcherAndSigner.sol:20] Missing
/// Supports:NatSpec annotation at the contract level.SystemConfigGasParams(the stated reference template) has/// Supports: anything after Holocene. Please document the supported SystemConfig version range, e.g.:/// Supports: any SystemConfig with setBatcherHash + setUnsafeBlockSigner
Passed Checks
- Interface decoupling —
ISystemConfigdefined inline with exactly the 4 methods required; no import from monorepo/@eth-optimism-bedrock - Struct field ordering —
TaskInputshasbatcherHashthenunsafeBlockSigner(alphabetical).check-struct-order.shpasses with 0 violations -
forge fmt --check— PASS -
_templateSetup()callssuper._templateSetup()first -
_build()uses regular function calls (correct forL2TaskBase, not OPCM) -
_build()writes no global state -
_validate()iterates all chains and asserts bothbatcherHashandunsafeBlockSignerslots post-execution -
_taskStorageWrites()listsSystemConfigProxy+FoundationUpgradeSafe— exact match toSystemConfigGasParamsreference -
_getCodeExceptions()returns batcher + signer addresses as EOA exceptions, correctly justified in comment - Example task exists at
test/tasks/example/sep/035-set-batcher-and-signer/ -
config.tomluses real chain ID 11155420 (OP Sepolia Testnet); state override correctly sets SystemConfig owner to FoundationUpgradeSafe -
.envhasFORK_BLOCK_NUMBER=10624099 - NatSpec
@noticepresent on all methods - Regression test
testRegressionCallDataMatches_SetBatcherAndSigner— 1 passed (with archival RPC) - Simulation — succeeded end-to-end; output confirmed below
Simulation
just simulate succeeded with forge v1.1.0 + archival RPC.
State changes — exactly as expected, nothing unexpected:
| Contract | Slot | Before | After | Description |
|---|---|---|---|---|
SystemConfigProxy (0x034edd...b538) |
0x67 (batcherHash) |
(prior value) | bytes32(0x1234...5678) |
New batcher hash |
SystemConfigProxy (0x034edd...b538) |
0x65a7ed... (unsafeBlockSigner) |
0x57CACBB0... |
0xabCDeF0123... |
New unsafe block signer |
FoundationUpgradeSafe (0xDEe571...) |
0x5 (nonce) |
69 |
70 |
Safe nonce increment |
Tenderly simulation link is in the just simulate output (requires TENDERLY_USERNAME/TENDERLY_PROJECT substitution).
User must verify in Tenderly:
- Both expected
SystemConfigslots changed - Safe nonce bumped from 69 → 70
- No unexpected contracts modified
Audit Trail
| Step | Command / Action | Result |
|---|---|---|
| 1 | ls src/template/ && ls src/tasks/types/ |
PASS — confirmed in superchain-ops |
| 2 | forge --version (v1.1.0 via mise) |
PASS |
| 3 | gh pr view 1393 --json files |
4 files identified |
| 4 | forge build --deny-warnings |
FAIL — pre-existing lib/optimism submodule errors, not from this PR |
| 5 | forge fmt --check |
PASS |
| 6 | ./src/script/check-struct-order.sh |
PASS — 0 violations |
| 7 | forge test --match-test SetBatcherAndSigner (archival RPC) |
PASS — 1/1 |
| 8 | Read SetBatcherAndSigner.sol + SystemConfigGasParams.sol |
2 warnings found |
| 9 | just simulate sep 035-set-batcher-and-signer (archival RPC, forge v1.1.0) |
PASS — clean state diff |
Verification commands:
forge fmt --check
./src/script/check-struct-order.sh
forge test --match-test testRegressionCallDataMatches_SetBatcherAndSignerEscalation Checklist — none apply:
- New template type — No, straightforward
L2TaskBasevariation ofSystemConfigGasParams - New OPCM function — No OPCM involved
- Cross-layer operations — No
- Security-critical operations — No (sequencer key rotation, not pause/upgrade)
- Unusual state changes — No, only
SystemConfigProxystorage + safe nonce
🤖 Reviewed with superchain-ops-template-review skill
There was a problem hiding this comment.
Manually reviewed and simulated, state changes looks correct.
The major comment I have that needs to be addressed: the template currently hardcodes FUS as the signer which assumes FUS will ALWAYS own SystemConfig at execution time.
We're not yet aligned on whether that's the case, ownership of the SystemConfig in future migrations may be transferred to a new safe we haven't set up yet.
Two possible paths here:
1- Dynamically fetch the owner: change the template to read SystemConfig.owner() and have a config field where to explicitly declare the expected owner address, so the template can assert they match.
Pros: more flexible template, works regardless of who ends up owning SystemConfig
Cons: more complexity in the template and additional failure points.
2- Align on the owner and keep it hardcoded: decide now who will own SystemConfig (FUS or a new safe), update addresses.toml accordingly and document it as a precondition for this template.
Pros: simpler
Cons: needs decision on ownership before this template is actually usable.
Either way, the state override from the example task should be removed once we know the actual owner. Right now it overrides the mismatch and makes the simulation look like it works when it wouldn't on chain and example tasks should reflect as closely as possible the execution in prod environments.
Looping in @sbvegan for visibility
|
Recap of the checks worth adding, regardless of which path we take on ownership: 1- Zero address guards on batcherAddress and unsafeBlockSigner (in _templateSetup) 2- No-op check: assert that the new values actually differ from the current on chain value. If someone misconfigures the task with the same addresses already set _validate() passes silently. 3- Ownership pre-check: in _templateSetup assert that SystemConfig.owner() matches the expected signer before simulation proceeds. Right now the state override masks this mismatch and the task looks like it works even if the ownership transfer hasn't happened yet. UPDATE: point 2 (no-ops) should be a broad check that still allows changing only one of the two values |
|
@Wazabie, as discussed, the override lets us collect signatures in advance.
In the current setup, when simulating it will always pass, and when executing — if the owner condition doesn't match — the template execution will revert anyway because onlyOwner is not satisfied. So in the current setup it doesn't add much value. |
Adds two defensive checks in _templateSetup and makes _build skip setters whose target value already matches the current on-chain state: - Reject zero batcherAddress or unsafeBlockSigner in TOML parsing. - Reject a task where both fields already match the on-chain values (OR-semantics, so partial rotations remain valid). - Precompute per-chain updateBatcher/updateSigner flags in setup and call only the setters for fields that actually change, avoiding redundant ConfigUpdate events and SSTOREs on partial rotations. Calldata for the existing example task is unchanged (both fields differ at the pinned block), so the regression test still passes byte-identical. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Added a new commit addressing the review:
|
I don't think we need SKIP_STATE_OVERRIDES now. The overrides let us collect signatures in advance and if the ownership condition isn't met at execution time the tx reverts anyway due to onlyOwner. If you have bandwidth it would be worth updating the template to address my comment 1 here and accept the owner as an input from the config (e.g. a safeAddress field in the TOML) instead of having it hardcoded as "FoundationUpgradeSafe". |
|
Thanks @Wazabie, both make sense. |
Wazabie
left a comment
There was a problem hiding this comment.
Template Review: SetBatcherAndSigner
Audit Trail
| Step | Command/Action | Result |
|---|---|---|
| 1 | mise exec -- forge build --deny-warnings |
PASS (0 warnings, 0 errors) |
| 2 | mise exec -- forge fmt --check |
PASS |
| 3 | ./src/script/check-struct-order.sh |
PASS (TaskInputs not TOML-decoded via parseRaw) |
| 4 | mise exec -- forge test --skip Integration |
210 PASS; 74 FAIL — all stale fork-block RPC errors pre-existing across the repo, unrelated to this template |
| 5 | Read src/template/SetBatcherAndSigner.sol |
2 warnings found |
| 6 | mise exec -- just simulate (sep/035) |
PASS — script ran successfully |
Blocking Issues
None.
Warnings
-
[SetBatcherAndSigner.sol:48–75]
_templateSetup()has novm.label()calls for any key addresses. The batcher address, unsafe block signer, and per-chain SystemConfigProxy addresses should be labeled for Tenderly trace readability. -
[SetBatcherAndSigner.sol:65–67] The per-chain no-op guard reverts the entire task if any single chain in
l2chainsalready has the target values, even if other chains still need the update. Behavior is intentional but undocumented — worth adding a comment explaining the rationale.
Passed Checks
-
ISystemConfigdefined inline — no external interface imports -
batcherAddress/unsafeBlockSignercannot be derived from on-chain state (they are the new values) -
TaskInputsfields alphabetically ordered:batcherHash,unsafeBlockSigner,updateBatcher,updateSigner -
_templateSetup()callssuper._templateSetup()first - Non-zero guards on both config inputs
-
_build()uses regular calls — correct forL2TaskBase -
_build()writes no global state; reads fromcfgset in_templateSetup() -
_validate()iterates all chains and checks both fields — correct: ifupdateBatcher=false,taskInput.batcherHashequals the current on-chain value, so the assertion passes trivially -
_taskStorageWrites()—SystemConfigProxy+FoundationUpgradeSafe(nonce), neither too broad nor too narrow -
_getCodeExceptions()correctly returns EOA batcher and signer addresses, sizedchains.length * 2 - Example task at
test/tasks/example/sep/035-set-batcher-and-signer/— 0 missing files -
config.tomluses real chain ID11155420(OP Sepolia Testnet) -
.envhasFORK_BLOCK_NUMBER -
[stateOverrides]correctly overrides SystemConfig owner toFoundationUpgradeSafeon the Sepolia fork
Simulation — State Changes
Script ran successfully. 2 contracts modified, 0 unexpected.
0x034edD2A225f7f429A63E0f1D2084B9E0A93b538 (SystemConfigProxy) — Chain ID 11155420
| Slot | Field | Before | After |
|---|---|---|---|
0x67 |
batcherHash |
(raw bytes32 — state printer renders as empty for non-UTF-8 bytes32; change confirmed by calldata) | (raw bytes32) |
0x65a7ed5... |
unsafeBlockSigner |
0x57CACBB0d30b01eb2462e5dC940c161aff3230D3 |
0xAbCdEf0123456789AbCdEf0123456789AbCdEf01 |
0xDEe57160aAfCF04c34C887B5962D0a69676d3C8B (FoundationUpgradeSafe)
| Slot | Field | Before | After |
|---|---|---|---|
0x05 |
nonce |
69 |
70 |
Note on batcherHash display: The state printer tries to render
bytes32as a UTF-8 string and shows empty for non-printable values. The calldata confirmssetBatcherHashwas included and the slot appeared in the state diff. Pre-existing cosmetic issue in the state printer, not a template bug.
Escalation Checklist
No escalation required. Standard L2TaskBase config-change template — no OPCM, no cross-layer ops, no complex Safe operations.
|
Tenderly simulation and expected data to sign is consistent with the previous simulation. Will ask for a security review now. cc @Ethnical |
|
@donoso-eth we should update the name of the template to something like |
Thanks @Wazabie, fair point — the name slightly overclaims now that partial rotations are supported. My lean is to tighten the contract-level NatSpec instead of renaming, to avoid the churn of renaming the contract, file, imports in Regression.t.sol, and the example task reference: Happy to rename if you feel strongly, but wanted to propose the lower-churn option first — wdyt? |
|
Super clean!! I just left 1 Nit that we can apply? After this we can merge! |
|
@donoso-eth I think we should rename now before the template to show explicitly this is not AND, I am in favor to do this right now and also updating the name of the contract, I feel this is better for clarity tbh! |
- Rename contract/file SetBatcherAndSigner -> SetBatcherAndOrSigner to reflect that either or both fields can be rotated (partial rotations are valid). - Replace hardcoded "FoundationUpgradeSafe" in _taskStorageWrites with safeAddressString() to remove duplication and make the template friendlier to a future configurable-owner refactor. - Tighten _getCodeExceptions NatSpec per review. Calldata and EIP-712 data-to-sign are unaffected by the rename; the existing pinned regression test passes byte-identical. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
comments addressed, including renaming. |
|
@Ethnical if it looks good we also need one final approval from you! |
Summary
Adds a reusable
SetBatcherAndSignertemplate for atomically updatingbatcherHashandunsafeBlockSigneronSystemConfigin a single batchedMulticall3 tx from
FoundationUpgradeSafe. Mirrors theSystemConfigGasParamspattern — same safe, same target, same storage-write surface.
Files
src/template/SetBatcherAndSigner.sol— template with inlineISystemConfigtest/tasks/example/sep/035-set-batcher-and-signer/— example task for OP Sepoliatest/tasks/Regression.t.sol— pinned-calldata regression testTest plan
forge build --deny-warnings/forge fmt --check/check-struct-order.shcleanforge test --match-test SetBatcherAndSigner— 1/1 passedSystemConfigslots + safe nonce bump🤖 Generated with Claude Code