refactor: harden sequencer registry bootstrap and admin handoff#285
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The two-step admin transfer is correctly implemented, the storage layout shift is consistent everywhere (Solidity source, slot constants, Foundry fixtures, Rust unit tests, and docs), and the field renaming/hardcoding rationale is clearly documented. Test coverage at the Foundry level is thorough. One gap worth noting: Everything else looks good. |
|
Previous feedback addressed — the second commit adds LGTM. |
vincent-k2026
left a comment
There was a problem hiding this comment.
LGTM. Two related hardenings landed cleanly together:
- Two-step admin transfer (
transferAdmin→acceptAdmin), matching OZOwnable2Stepsemantics with the slot placement (slot 3 next to_admin, no__gap) reasoned through in the PR body. _initialSystemAddresspinned toMEGA_SYSTEM_ADDRESSat genesis, removing a configurability that would silently diverge non-EVM components (payload executor, txpool, replay) from the on-chain state.
Cross-checked layout consistency end-to-end:
- Solidity field declaration order in
SequencerRegistry.sol:14-51matches the doc table, which matches the Rust slot constants insystem-contracts/src/lib.rs, which matches the slot-number assertions in both forge and Rust tests. - Dynamic-array element keys in the new deployed bytecode include
0x0175b7a6…(=keccak256(uint256(11))for_systemAddressHistory) and the new_sequencerHistory[0]hash — the slot reorder propagates through to the bytecode literals, not just the source. acceptAdminaccess control is sound:pending == 0after a cancel cannot be exploited becausemsg.senderis always non-zero in EVM, andtest_acceptAdmin_revertsWhenNoPendingpins this.- The two
test_setUp_*invariant tests (one per forge file) are nice fixture-drift guards — anyone reordering the layout in the future without keeping fixtures in sync gets a loud failure rather than silent reinterpretation.
This PR pairs with #282 (gas budget for applyPendingChanges); both should land together or close to each other to avoid devnet rebase churn.
Two non-blocking nits:
-
transferAdmin(address(0))semantics inverted vs. the previous version (used to revertZeroAddress, now silently cancels pending). This is by design and tested, but the inversion is easy to miss for operators / frontend integrators. Could the NatSpec ontransferAdminget a one-line "passes zero == cancel pending; does NOT renounce admin" callout? Pure docs. -
The "future versions may only append new slots" rule is doc-only. A follow-up forge invariant test (or a
forge inspect storage-layoutsnapshot in CI) that asserts each named slot's index would catch a future accidental reorder before bytecode lands.
Signed-off-by: RealiCZ <wylbzc4928@gmail.com>
|
The new commit is a merge-of-main sync only — no code changes. All prior feedback has been addressed. LGTM. |
|
LGTM. All prior feedback addressed: integration test added, docs updated (code hash and |
Summary
SequencerRegistry.transferAdminis now a two-step handoff.transferAdminonly sets_pendingAdmin; the new admin must callacceptAdminto take effect. Prevents permanent admin lockout from a mistyped, phished, or clipboard-substituted address.SequencerRegistryConfig: dropinitial_system_address(the contract now hardcodesMEGA_SYSTEM_ADDRESSat genesis); rename remaining fields torex5_initial_sequencer/rex5_initial_admin._pendingAdminplaced at slot 3, immediately after_admin.Note
Why slot 3
Pairing
_pendingAdminwith_adminmatches OpenZeppelinOwnable2Step, where_pendingOwnersits next to_owner.Why no
__gapOpenZeppelin's
uint256[N] __gapexists to let a parent contract grow its storage without colliding with a child contract in an upgradeable proxy / multi-layer inheritance setup.SequencerRegistryis deployed via raw state patch, has no contract inheritance with state, and is upgraded by spec-gated bytecode replacement that only ever appends new slots — so__gapsolves an insertion problem we don't have. The real invariant is now stated in the spec doc: future versions may only append; never reorder or insert.Test plan
cargo fmt --all --check— clean.cargo clippy --workspace --lib --examples --tests --benches --all-features --locked— clean .prettier --check 'docs/**/*.md'— clean.cargo test --workspace— clean