Skip to content

Conversation

@ilitteri
Copy link
Contributor

No description provided.

@ilitteri ilitteri self-assigned this Dec 18, 2025
@ilitteri ilitteri requested a review from a team as a code owner December 18, 2025 14:08
@ilitteri ilitteri added the L2 Rollup client label Dec 18, 2025

Whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.

Our recommendation is that the owner of the contract be a security council. The [Stages Framework](https://medium.com/l2beat/introducing-stages-a-framework-to-evaluate-rollups-maturity-d290bb22befe) recommends that the security council be in the form of a multisig composed of at least 8 people with a consensus threshold of 50%, and at least 50% of the participants must be external to the organization that operates the rollup.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think what we call "owner" should be the security council. The SC can execute things immediately, and what we call owner just have roles of Proposer/Executor so it doesn't have permissions to bypass the minDelay. Maybe we should change names though, currently we have owner and security_council. Maybe the former shouldn't be called owner so that it's not misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll review this part and clarify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


An interesting question is how we protect users from instantaneous executions of these types of operations. One might think of a scenario where the `Timelock` owner updates the delay to 0 to execute a malicious operation, having removed the exit window for users; or perhaps upgrade the `Timelock` contract by removing the delay logic with the same objective.

We solve this by making the `Timelock` itself the only one capable of invoking the corresponding functions for those operations. In this way, if the contract owner wants to push a malicious upgrade or modification to the “protector” contract, they must comply with the configured delay at the time of proposing the operation. For example, if a malicious or compromised owner wishes to set the delay to 0, they must wait the duration of the current exit window for the execution of their malicious operation, thus giving users sufficient time to exit the network.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the Security Council can emergencyExecute and update the delay time to zero.
In case we don't want this behavior what we can do is to enable emergencyExecute only if the target address is different than the timelock contract itself. That means that the SC won't be able to upgrade the Timelock instantly in case of an emergency though, which may backfire.
For a rollup that is not mature it isn't a critical thing, but as the rollup becomes more mature we should consider a more sophisticated approach

Copy link
Contributor

@tomip01 tomip01 left a comment

Choose a reason for hiding this comment

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

Just some nits


There's no such thing as a unique owner of the `Timelock` necessarily. `Timelock` is a `TimelockController`, which is also an `AccessControl`, so we can define different roles and assign them to different entities. By "owner" of the `Timelock`, we refer to the account that has the role to update the contract (i.e. the one that can modify the delay).

This said, whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This said, whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.
That said, whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.

Comment on lines 44 to 45
Whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is duplicated

Suggested change
Whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.


It's worth noting that the designated security council can execute operations instantly in case of emergencies, so it's crucial that the members are trustworthy and committed to the network's security. The [Stages Framework](https://forum.l2beat.com/t/the-stages-framework/291) recommends that the security council be in the form of a multisig composed of at least 8 people with a consensus threshold of 75%, and the participants in the set need to be decentralized and diverse enough, possibly coming from different companies and jurisdictions.

In the case of our `Timelock`, the owner is not the only one who can act on it. In fact, it is recommended that the security council only act in specific emergencies. The `Timelock` is also AccessControl, which means it has special functionality for managing accesses, in this case, in the form of roles.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
In the case of our `Timelock`, the owner is not the only one who can act on it. In fact, it is recommended that the security council only act in specific emergencies. The `Timelock` is also AccessControl, which means it has special functionality for managing accesses, in this case, in the form of roles.
In the case of our `Timelock`, the owner is not the only one who can act on it. In fact, it is recommended that the security council only act in specific emergencies. The `Timelock` is also `AccessControl`, which means it has special functionality for managing accesses, in this case, in the form of roles.

Base automatically changed from timelock_l2 to main January 5, 2026 20:50
Copilot AI review requested due to automatic review settings January 6, 2026 14:41
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

Lines of code report

Total lines added: 28
Total lines removed: 9
Total lines changed: 37

Detailed view
+--------------------------------------------+-------+------+
| File                                       | Lines | Diff |
+--------------------------------------------+-------+------+
| ethrex/cmd/ethrex/build_l2.rs              | 447   | +5   |
+--------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/command.rs            | 647   | +2   |
+--------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/deployer.rs           | 1481  | -9   |
+--------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/options.rs            | 1072  | +10  |
+--------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/configs.rs      | 107   | +1   |
+--------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_committer.rs | 1384  | +10  |
+--------------------------------------------+-------+------+

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces comprehensive exit window functionality for the ethrex L2 stack by implementing a Timelock contract that enforces delays on critical operations, protecting users from unwanted system changes. The changes also refactor the deployment and upgrade documentation, moving from a simple migrations guide to a more detailed upgrades guide.

  • Adds detailed documentation explaining exit windows, settlement windows, and Timelock architecture
  • Implements a new Timelock contract that acts as the owner of OnChainProposer, managing access control through roles
  • Refactors OnChainProposer to remove direct sequencer authorization in favor of Timelock-based role management
  • Updates the deployment flow to optionally deploy and configure the Timelock for non-based deployments

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
docs/l2/fundamentals/exit_window.md New comprehensive documentation explaining exit windows, how they work, settlement windows, and Timelock ownership model
docs/l2/deployment/upgrades.md Replaces migrations.md with detailed upgrade instructions including database migrations and Timelock deployment guide
docs/l2/deployment/migrations.md Removed in favor of upgrades.md
docs/l2/deployment/README.md Updates link from migrations.md to upgrades.md
docs/SUMMARY.md Updates navigation to include exit window documentation and upgrades instead of migrations
crates/l2/contracts/src/l1/Timelock.sol New contract that wraps TimelockController to provide role-based access control and delayed execution for OnChainProposer operations
crates/l2/contracts/src/l1/interfaces/ITimelock.sol Interface defining the Timelock contract's public API
crates/l2/contracts/src/l1/OnChainProposer.sol Refactored to be owned by Timelock; removes onlySequencer modifier and initializeBridgeAddress function; adds bridge parameter to initialize
crates/l2/contracts/src/l1/based/OnChainProposer.sol Similar refactoring for based deployment; adds bridge parameter to initialize function
crates/l2/contracts/src/l1/interfaces/IOnChainProposer.sol Removes initializeBridgeAddress from interface
crates/l2/contracts/src/l1/based/interfaces/IOnChainProposer.sol Removes initializeBridgeAddress from based interface
crates/l2/tee/contracts/src/TDXVerifier.sol Updates to use Timelock's isSequencer instead of OnChainProposer's authorizedSequencerAddresses mapping
crates/l2/tee/contracts/Makefile Updates solc compilation flags to support new import structure
crates/l2/sequencer/l1_proof_verifier.rs Adds timelock_address field and uses it as target when present, otherwise falls back to OCP
crates/l2/sequencer/l1_proof_sender.rs Adds timelock_address field and uses it as target when present, otherwise falls back to OCP
crates/l2/sequencer/l1_committer.rs Adds timelock_address field; requires it for non-based deployments and uses it as transaction target
crates/l2/sequencer/configs.rs Adds timelock_address field to CommitterConfig
crates/l2/sdk/src/sdk.rs Makes ProxyDeployment Clone-able
cmd/ethrex/l2/options.rs Adds timelock_address CLI option and env var support
cmd/ethrex/l2/deployer.rs Implements Timelock deployment and initialization; updates OnChainProposer initialization to set Timelock as owner
cmd/ethrex/l2/command.rs Propagates timelock_address from deployed contracts to sequencer config
cmd/ethrex/build_l2.rs Adds Timelock contract to compilation list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 44 to 45
Whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

There's a duplicate sentence in this paragraph. The sentence "Whoever owns the Timelock decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired." appears twice consecutively (lines 42-44).

Suggested change
Whoever owns the `Timelock` decides its functioning. In our stack, the owner of the contract is established during its initialization, and then that owner can transfer the ownership to another account if desired.

Copilot uses AI. Check for mistakes.

All our contracts are [`UUPSUpgradeable`](https://docs.openzeppelin.com/contracts/5.x/api/proxy#UUPSUpgradeable) (an upgradeability pattern recommended by OpenZeppelin). In particular, to upgrade this type of contract, the operator must call the `upgradeAndCall` function, which invokes an internal function called `_authorizeUpgrade`. It is recommended to override this function by implementing authorization logic. This is the function we must protect in the case of both contracts, and we do so by “delaying” its execution.

Currently, the function used to upgrade the contracts is protected by an `onlyOwner` modifier, which verifies that the caller corresponds to the owner of the contract (all L1 contracts but the `Timelock` are [`Ownable2StepUpgradeable`](https://docs.openzeppelin.com/stellar-contracts/access/ownable)), configured during its initialization. In other words, only the owner can call it. Keeping this in mind is important for understanding how we implement the functionality.
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The hyperlink text should not include "stellar-contracts" in the URL. The correct OpenZeppelin documentation URL for Ownable should reference "contracts-upgradeable" instead of "stellar-contracts". This appears to be a copy-paste error.

Suggested change
Currently, the function used to upgrade the contracts is protected by an `onlyOwner` modifier, which verifies that the caller corresponds to the owner of the contract (all L1 contracts but the `Timelock` are [`Ownable2StepUpgradeable`](https://docs.openzeppelin.com/stellar-contracts/access/ownable)), configured during its initialization. In other words, only the owner can call it. Keeping this in mind is important for understanding how we implement the functionality.
Currently, the function used to upgrade the contracts is protected by an `onlyOwner` modifier, which verifies that the caller corresponds to the owner of the contract (all L1 contracts but the `Timelock` are [`Ownable2StepUpgradeable`](https://docs.openzeppelin.com/contracts-upgradeable/5.x/api/access#Ownable2StepUpgradeable)), configured during its initialization. In other words, only the owner can call it. Keeping this in mind is important for understanding how we implement the functionality.

Copilot uses AI. Check for mistakes.
@ManuelBilbao ManuelBilbao enabled auto-merge January 6, 2026 18:53
@ManuelBilbao ManuelBilbao added this pull request to the merge queue Jan 6, 2026
Merged via the queue into main with commit 72b1baa Jan 6, 2026
47 checks passed
@ManuelBilbao ManuelBilbao deleted the exit_windows_fundamentals branch January 6, 2026 19:56
@github-project-automation github-project-automation bot moved this to Done in ethrex_l2 Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants