Skip to content

refactor: harden sequencer registry bootstrap and admin handoff#285

Merged
RealiCZ merged 5 commits into
mainfrom
cz/fix/sequencer-registry-hardening
May 7, 2026
Merged

refactor: harden sequencer registry bootstrap and admin handoff#285
RealiCZ merged 5 commits into
mainfrom
cz/fix/sequencer-registry-hardening

Conversation

@RealiCZ
Copy link
Copy Markdown
Collaborator

@RealiCZ RealiCZ commented May 1, 2026

Summary

  • Two-step admin transfer: SequencerRegistry.transferAdmin is now a two-step handoff. transferAdmin only sets _pendingAdmin; the new admin must call acceptAdmin to take effect. Prevents permanent admin lockout from a mistyped, phished, or clipboard-substituted address.
  • SequencerRegistryConfig: drop initial_system_address (the contract now hardcodes MEGA_SYSTEM_ADDRESS at genesis); rename remaining fields to rex5_initial_sequencer / rex5_initial_admin.
  • Storage layout: _pendingAdmin placed at slot 3, immediately after _admin.

Note

Why slot 3

Pairing _pendingAdmin with _admin matches OpenZeppelin Ownable2Step, where _pendingOwner sits next to _owner.

Why no __gap

OpenZeppelin's uint256[N] __gap exists to let a parent contract grow its storage without colliding with a child contract in an upgradeable proxy / multi-layer inheritance setup. SequencerRegistry is 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 __gap solves 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

@RealiCZ RealiCZ added spec:unstable Changes to the unstable spec (currently REX5) comp:core Changes to the `mega-evm` core crate rust Pull requests that update rust code comp:doc Changes in the documentation labels May 1, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.5%. Comparing base (79828cb) to head (b1e997d).

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

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: crates/mega-evm/tests/block_executor/sequencer_registry.rs has no Rust-level integration test for the new admin handoff flow. The existing block executor tests cover bootstrap and system-address rotation, and per CLAUDE.md, logic changes should be paired with tests. A single test that calls transferAdmin + acceptAdmin through the block executor (similar to how test_second_block_resolves_updated_system_address exercises scheduleNextSystemAddressChange + applyPendingChanges) would close this gap.

Everything else looks good.

@RealiCZ RealiCZ added the api:breaking Crate interface change — downstream users must update label May 1, 2026
Comment thread crates/mega-evm/tests/block_executor/sequencer_registry.rs Dismissed
Comment thread crates/mega-evm/tests/block_executor/sequencer_registry.rs Dismissed
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

Previous feedback addressed — the second commit adds test_admin_handoff_via_block_executor which exercises transferAdmin + acceptAdmin through the block executor and checks both the ADMIN and PENDING_ADMIN storage slots.

LGTM.

Copy link
Copy Markdown

@vincent-k2026 vincent-k2026 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Two related hardenings landed cleanly together:

  • Two-step admin transfer (transferAdminacceptAdmin), matching OZ Ownable2Step semantics with the slot placement (slot 3 next to _admin, no __gap) reasoned through in the PR body.
  • _initialSystemAddress pinned to MEGA_SYSTEM_ADDRESS at 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-51 matches the doc table, which matches the Rust slot constants in system-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.
  • acceptAdmin access control is sound: pending == 0 after a cancel cannot be exploited because msg.sender is always non-zero in EVM, and test_acceptAdmin_revertsWhenNoPending pins 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:

  1. transferAdmin(address(0)) semantics inverted vs. the previous version (used to revert ZeroAddress, now silently cancels pending). This is by design and tested, but the inversion is easy to miss for operators / frontend integrators. Could the NatSpec on transferAdmin get a one-line "passes zero == cancel pending; does NOT renounce admin" callout? Pure docs.

  2. The "future versions may only append new slots" rule is doc-only. A follow-up forge invariant test (or a forge inspect storage-layout snapshot 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>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

The new commit is a merge-of-main sync only — no code changes. All prior feedback has been addressed. LGTM.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

LGTM. All prior feedback addressed: integration test added, docs updated (code hash and NotPendingAdmin error), and the merge-of-main sync introduced no new issues.

@RealiCZ RealiCZ merged commit 09ceb5d into main May 7, 2026
20 checks passed
@RealiCZ RealiCZ deleted the cz/fix/sequencer-registry-hardening branch May 7, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api:breaking Crate interface change — downstream users must update comp:core Changes to the `mega-evm` core crate comp:doc Changes in the documentation rust Pull requests that update rust code spec:unstable Changes to the unstable spec (currently REX5)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants