Skip to content

feat(protocol): add immutable ejector manager role to preconf whitelist#21382

Open
ggonzalez94 wants to merge 5 commits intotaiko-alethia-protocol-v3.0.0from
new-ejectors
Open

feat(protocol): add immutable ejector manager role to preconf whitelist#21382
ggonzalez94 wants to merge 5 commits intotaiko-alethia-protocol-v3.0.0from
new-ejectors

Conversation

@ggonzalez94
Copy link
Member

@ggonzalez94 ggonzalez94 commented Feb 26, 2026

Summary

  • add immutable _ejectorManager to PreconfWhitelist and wire it via constructor
  • allow setEjecter to be called by either owner or ejector manager
  • update all PreconfWhitelist constructor callsites in protocol scripts/tests
  • add tests for manager authorization and unauthorized caller revert

Validation

  • forge test --match-path test/layer1/preconf/whitelist/PreconfWhitelist.t.sol
  • forge test --match-path test/layer1/core/inbox/InboxWhitelistProver.t.sol

Note

Medium Risk
Changes access control on PreconfWhitelist.setEjecter and introduces a new constructor parameter that must be wired correctly during deployment/upgrade; misconfiguration could block or broaden ejecter administration.

Overview
Adds an immutable _ejectorManager role to PreconfWhitelist (constructor-enforced non-zero) and expands setEjecter authorization from owner-only to owner or ejector manager via a new onlyOwnerOrEjectorManager modifier, with new revert errors.

Updates deployment/upgrade scripts and tests to pass the new constructor arg (including EJECTOR_MANAGER env var defaulting to CONTRACT_OWNER), and adds coverage for manager access and unauthorized caller reverts. Also updates Derivation.md to document chain-specific MAX_ANCHOR_OFFSET/TIMESTAMP_MAX_OFFSET values and refreshes gas report snapshots.

Written by Cursor Bugbot for commit 531729d. This will update automatically on new commits. Configure here.

@claude
Copy link

claude bot commented Feb 26, 2026

Claude finished @ggonzalez94's task in 1m 54s —— View job


PR Review: feat(protocol): add immutable ejector manager role to preconf whitelist

  • Read changed files
  • Review implementation
  • Post feedback

Overall, the approach is sound — using an immutable for the ejector manager is the right choice (no storage slot consumed, baked into bytecode per-deployment). The PR wires things correctly in the proxy pattern. A few issues to address:


Issues

1. DeployShastaContracts.s.sol — No dedicated ejectorManager config field

DeploymentConfig in this script has no ejectorManager field; line 91 hardcodes config.contractOwner:

// DeployShastaContracts.s.sol:91
address preconfWhitelist = address(new PreconfWhitelist(config.contractOwner));

If the intent is to allow a different address to manage ejecters in the mainnet deployment path, this needs a dedicated field (and corresponding env var) like DeployProtocolOnL1.s.sol does. If using contractOwner is intentional here, a comment explaining why would help avoid confusion. Fix this →

2. DeployShastaContracts.s.sol — Deployed implementation is never used

The implementation deployed at line 91 is only logged; it's never used to upgrade the existing proxy (config.preconfWhitelist). The inbox at line 110 consumes config.preconfWhitelist (the already-deployed proxy), not the new implementation. This means after running this script, the proxy still runs the old bytecode with no ejector manager. If the upgrade is expected to happen out-of-band manually, document that explicitly. Otherwise, add an explicit upgradeTo() call — like DeployProtocolOnL1.s.sol does at lines 183–185.

3. Missing test: ejector manager can revoke an ejecter

test_setEjecter_allowedForEjectorManager only tests setting _isEjecter = true. The ejector manager calling setEjecter(addr, false) to revoke an ejecter is not covered. Fix this →

4. No zero-address guard on _ejecter in setEjecter

// PreconfWhitelist.sol:115
function setEjecter(address _ejecter, bool _isEjecter) external onlyOwnerOrEjectorManager {
    ejecters[_ejecter] = _isEjecter;

setEjecter(address(0), true) succeeds silently. address(0) can never call the contract in practice, so the risk is low, but it's a footgun. Consider require(_ejecter != address(0)).

5. setEjecter missing @dev note on access control in NatSpec

Per CLAUDE.md, internal/implementation-specific behavior should have @dev tags. The current comment says what it does but not who can call it (onlyOwnerOrEjectorManager). Add a @dev Only callable by owner or ejector manager. line.


Nits

  • DeployProtocolOnL1.s.sol:111config.preconfWhitelist is assigned twice (lines 107 and 111). Line 111 is a duplicate and can be removed.
  • The _ejectorManager is internal visibility. Since it's not exposed by any getter, external callers have no way to verify what the ejector manager is. Consider making it public or adding a public getter for transparency/auditability.

What's Good

  • Immutable correctly avoids a storage slot — storage layout and __gap are unchanged, upgrade safety is maintained.
  • Constructor guard require(_ejectorManagerAddress != address(0), InvalidEjectorManager()) is correct.
  • The two-modifier pattern (onlyOwnerOrEjecter vs onlyOwnerOrEjectorManager) clearly separates ejection (operators) from ejector management (access control).
  • InboxTestBase.sol correctly passes address(this) as the ejector manager.
  • Test coverage for the happy path and unauthorized revert are solid.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e16e708a7f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


function _loadConfig() private view returns (DeploymentConfig memory config) {
config.contractOwner = vm.envAddress("CONTRACT_OWNER");
config.ejectorManager = vm.envOr("EJECTOR_MANAGER", config.contractOwner);

Choose a reason for hiding this comment

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

P2 Badge Preserve existing ejector manager on upgrades

When PRECONF_WHITELIST points to an existing proxy, this script upgrades it using new PreconfWhitelist(config.ejectorManager), but _loadConfig defaults ejectorManager to CONTRACT_OWNER whenever EJECTOR_MANAGER is unset. That means any upgrade run that forgets EJECTOR_MANAGER will silently overwrite a previously configured dedicated manager and revoke its ability to call setEjecter, which can break operational ejection workflows; consider requiring EJECTOR_MANAGER for upgrade flows or reading/persisting the current manager explicitly.

Useful? React with 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1cd379786a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 17 to 21
TimestampMaxOffset = 12 * 1800
// AnchorMinOffset The minimum anchor block number offset from the proposal origin block number, refer to LibManifest.MIN_ANCHOR_OFFSET.
AnchorMinOffset = 2
// AnchorMaxOffset The maximum anchor block number offset from the proposal origin block number, refer to LibManifest.MAX_ANCHOR_OFFSET.
AnchorMaxOffset = 128
AnchorMaxOffset = 1800

Choose a reason for hiding this comment

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

P2 Badge Synchronize manifest offsets across client implementations

Update the companion Rust Shasta constants in packages/taiko-client-rs/crates/protocol/src/shasta/constants.rs when changing these Go offsets, otherwise mixed Go/Rust deployments will validate different payload windows (1800/21600 here vs 128/1536 in Rust) and can disagree on proposal validity during derivation. This commit introduces that cross-client mismatch by changing only the Go binding values.

Useful? React with 👍 / 👎.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable autofix in the Cursor dashboard.

| Constant | Value | Description |
| -------------------------------- | ----------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **DERIVATION_SOURCE_MAX_BLOCKS** | `192` | The per-derivation-source block limit. If we assume block times of seconds,192 blocks will cover an Ethereum epoch. |
| **MAX_ANCHOR_OFFSET** | `1800` | The maximum anchor block number offset from the proposal origin block number (6 hours of L1 blocks). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any specific reason we need such a large anchor offset? It introduces significant overhead for the prover/Raiko, since we have to walk back block-by-block to the anchor. In the worst case, that’s ~1800 steps, which means tens of network I/O operations and ~1800 additional block-hash calculations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants