Skip to content

refactor(inbox): remove activation mechanism#21475

Open
dantaik wants to merge 13 commits intomainfrom
dantaik/inbox-rm-activation
Open

refactor(inbox): remove activation mechanism#21475
dantaik wants to merge 13 commits intomainfrom
dantaik/inbox-rm-activation

Conversation

@dantaik
Copy link
Copy Markdown
Member

@dantaik dantaik commented Mar 18, 2026

Remove the explicit inbox activation flow that previously required a separate owner call before the Inbox could accept proposals. The activation is now assumed to have occurred in a prior contract version, with state properly initialized during contract upgrade.

This change simplifies the deployment flow and reduces gas consumption (~17 gas per proposal) by eliminating the activation check. The activationTimestamp state variable is retained but deprecated to maintain storage layout compatibility for proxy upgrades.

Remove the explicit inbox activation flow that required a separate owner call before the Inbox could accept proposals. The activation is now assumed to have occurred in a prior contract version, with state initialized during upgrade.

- Remove activate() function and ActivationRequired guard check from Inbox.sol
- Remove LibInboxSetup.activate() function and related error handling
- Update deployment script to no longer call inbox.activate()
- Update tests to simulate pre-activated state using vm.store cheatcodes
- Keep activationTimestamp state variable (deprecated) for storage layout compatibility
- Gas savings: ~17 gas per propose() call from removing the activation check
dantaik and others added 2 commits March 18, 2026 10:20
Rename to _activationTimestamp with internal visibility since it is
deprecated and no longer part of the public API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
File no longer contains activation tests after removing the activation
mechanism. Rename to reflect its current contents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@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: ba1548b07e

ℹ️ 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".

dantaik and others added 2 commits March 18, 2026 02:27
Instead of requiring a separate activation step, init() now sets up
the core state (nextProposalId, lastProposalBlockId, genesis hash,
etc.) directly. This removes the need for vm.store hacks in tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dantaik dantaik added this to the Unzen Hard Fork (v6) milestone Mar 18, 2026
@dantaik dantaik self-assigned this Mar 18, 2026
dantaik and others added 3 commits March 18, 2026 02:37
Move validateConfig, its constant, and errors from the external library
into the Inbox contract as a private function, removing the DELEGATECALL
overhead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dantaik dantaik marked this pull request as draft March 21, 2026 08:38
@dantaik dantaik marked this pull request as ready for review March 21, 2026 08:54
@taikoxyz taikoxyz deleted a comment from claude bot Mar 21, 2026
@claude
Copy link
Copy Markdown

claude bot commented Mar 21, 2026

Claude finished @dantaik's task in 1m 57s —— View job


PR Review: refactor(inbox): remove activation mechanism

  • Gather context and read changed files
  • Review Inbox.sol changes
  • Review deployment script changes
  • Review test changes
  • Provide final feedback

Summary

Clean refactor that completes the activation removal. The initialization logic previously split across init() + activate() is now fully consolidated into init(). The overall approach is sound. A few things worth flagging:


Issues

1. Storage layout documentation mismatch (DevnetInbox_Layout.sol, MainnetInbox_Layout.sol)

The layout files (auto-generated) were updated to show _activationTimestamp (private naming), but the actual contract declares:

uint48 public activationTimestamp;  // Inbox.sol:127

The layout files are inconsistent with the live code. If someone reads these layout docs during an upgrade safety review, they'll see a variable name that doesn't exist. The layout script should be re-run against the current code (or the layout files were generated from an intermediate version where the variable was briefly renamed).

2. activator field in DeploymentConfig has stale semantics (DeployShastaContracts.s.sol)

The activator field is still validated non-zero and used as the initial inbox owner, but its original purpose (owning the inbox so it could call activate()) is gone. The field name now implies a concept that no longer exists. Not a bug, but confusing for operators setting up deployments. Consider renaming to initialOwner or adding a comment clarifying its current role.

3. _validateConfig inlining trades deployment size for simplicity

LibInboxSetup used public library functions specifically to reduce Inbox.sol deployment bytecode via external linking (this was explicitly documented in the old library comment). Now that _validateConfig is a private pure function in Inbox.sol, the constructor validation code is part of the deployment bytecode. Not a bug, but the gas report should show slightly larger deployment cost. Worth confirming the trade-off is acceptable.


Minor

  • The init() comment in Inbox.sol says _genesisBlockHash is "The block hash used as the genesis finalized block hash", but lastFinalizedBlockHash being set here isn't the L2 genesis block hash—it's the last Pacaya block hash used to anchor the chain. The comment is slightly misleading.

  • _coreState.lastProposalBlockId = 1 initialization means the first proposal can happen at block 2. This is correct, but a short inline comment explaining why (to avoid blockhash(0) == 0) would help future readers—the old LibInboxSetup.activate() had this explanation.


What's Good

  • Removing the ActivationRequired check from propose() is the right call—17 gas per proposal adds up at scale.
  • The Codex reviewer's concern about nextProposalId being 0 on fresh deployments is already handled: init() sets it to 1 (line 192) and seeds proposal 0's hash (line 198). dantaik's rebuttal is correct.
  • activationTimestamp being retained as public (per davidtaikocha's request) is correct for client compatibility.
  • Test cleanup is clean—InboxActivation.t.sol is properly replaced by InboxConfig.t.sol covering the inlined validation logic.
  • Deployment scripts correctly require L2_GENESIS_HASH env var and validate it non-zero.

@dantaik dantaik enabled auto-merge March 21, 2026 08:55
@dantaik
Copy link
Copy Markdown
Member Author

dantaik commented Mar 21, 2026

@davidtaikocha please take another look

@dantaik dantaik requested a review from davidtaikocha March 21, 2026 08:55
Copy link
Copy Markdown

@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: 635d7ad4a8

ℹ️ 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 +191 to +195
activationTimestamp = _activationTimestamp;
_coreState.nextProposalId = 1;
_coreState.lastProposalBlockId = 1;
_coreState.lastFinalizedTimestamp = uint48(block.timestamp);
_coreState.lastFinalizedBlockHash = _genesisBlockHash;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep a way to reseed the fork parent hash after deploy

Checked the mainnet/Hoodi deployment flow in packages/protocol/script/layer1/core/DeployShastaContracts.s.sol:125-128: we still predeploy Shasta and only switch SignalService at shastaForkTimestamp. This refactor now stores _coreState.lastFinalizedBlockHash once during init() and removes the only post-deployment path that used to update it. If the Pacaya head reorgs after deployment but before the fork, the first Shasta proof will hit ParentBlockHashMismatch in Inbox.prove() (packages/protocol/contracts/layer1/core/impl/Inbox.sol:283-286) and the fork is stuck until the proxy is redeployed.

Useful? React with 👍 / 👎.

)
),
data: abi.encodeCall(Inbox.init, config.activator)
data: abi.encodeCall(Inbox.init, (config.activator, config.l2GenesisHash, uint48(block.timestamp)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Initialize activationTimestamp from the configured fork time

In the mainnet/Hoodi path, this passes block.timestamp into Inbox.init() even though the same script already has the real fork time in config.shastaForkTimestamp. taiko-client treats activationTimestamp as the L1 block where Shasta started (packages/taiko-client/pkg/rpc/methods.go:1139-1155) and uses it to reset L1Current for the first Shasta proposal (packages/taiko-client/driver/state/l1_current.go:76-79). If these contracts are deployed ahead of the scheduled fork, proposal 1 will be anchored to the deployment block instead of the actual fork block, which skews client bootstrap and reorg recovery around genesis.

Useful? React with 👍 / 👎.

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.

2 participants