feat: add shared lockbox spec #465

agusduha · 2024-11-22T21:31:47Z

Description

Introducing the SharedLockbox spec and its corresponding upgrade process. This enhancement improves the Superchain’s interoperable ETH withdrawal user experience and prevents withdrawal failures caused by insufficient ETH in the OptimismPortal. Additionally, the dependency manager role is being added to SuperchainConfig to manage the network’s dependency set.

Additional context

Design doc: ethereum-optimism/design-docs#84

* feat: add shared lockbox spec * feat: update shared lockbox spec * fix: specs * feat: add mermaid diagram * fix: prs comments * fix: nits * fix: remove dependency manager * feat: refactor lockbox specs * fix: nit * fix: misspellings * fix: verb tense * fix: prs comments * fix: small changes

tynes · 2024-11-27T21:26:03Z

specs/interop/shared-lockbox.md

+## Overview
+
+With interoperable ETH, withdrawals will fail if the referenced `OptimismPortal` lacks sufficient ETH.
+This is due to having the possibility to move ETH liquidity accross the different chains and it could happen


typo: across

maurelian · 2024-12-11T17:21:59Z

specs/interop/shared-lockbox.md

+Deposits and locks ETH into the lockbox's liquidity pool.
+
+- The function MUST accept ETH.
+- Only authorized `OptimismPortal` addresses SHOULD be allowed to interact.


I think it's worth adding "MUST not revert" when called by an allowed address.

maurelian · 2024-12-11T17:23:15Z

specs/interop/shared-lockbox.md

+and `unlockETH` for withdrawing ETH from the lockbox.
+These functions are called by the `OptimismPortal` contracts to manage the shared ETH liquidity
+when making deposits or finalizing withdrawals.
+These `OptimismPortal`s will be allowlisted by the `SuperchanConfig` using the `authorizePortal` function


Suggested change

These `OptimismPortal`s will be allowlisted by the `SuperchanConfig` using the `authorizePortal` function

These `OptimismPortal`s will be allowlisted by the `SuperchainConfig` using the `authorizePortal` function

maurelian · 2024-12-11T17:24:25Z

specs/interop/shared-lockbox.md

+
+- The function MUST accept ETH.
+- Only authorized `OptimismPortal` addresses SHOULD be allowed to interact.
+- The function MUST emit the `ETHLocked` event with the `portal` that called it and the `amount`.


This is a bit of a product decision, but it might be helfpul to also emit an l2 chain id.

We don't have the L2 chainid legible on L1 right now to be able to do this but I do agree this is useful

maurelian · 2024-12-11T17:25:00Z

specs/interop/shared-lockbox.md

+
+Withdraws a specified amount of ETH from the lockbox's liquidity pool.
+
+- Only authorized `OptimismPortal` addresses SHOULD be allowed to interact.


Nit: why isn't this a MUST?

maurelian · 2024-12-11T17:31:37Z

specs/interop/shared-lockbox-upgrade.md

+## Overview
+
+Based on the assumption that a chain joining the dependency set is an irreversible process,
+the on-chain chains list is simplified by assuming that joining the Shared Lockbox is


I lack some context here, what/where is the on-chain chains list?
It seems like it's becoming equivalent to the dependency set, was it previously different?

maurelian · 2024-12-11T17:36:55Z

specs/interop/shared-lockbox.md

+To do so, it unifies ETH L1 liquidity in a single contract (`SharedLockbox`), enabling seamless withdrawals of ETH
+from any OP chain in the Superchain, regardless of where the ETH was initially deposited.
+
+## Design


Is this contract proxied/upgradeable?

I see in the impl that it is. Should be added to the spec, as otherwise the options for moving assets out of the lockbox again are much trickier.

maurelian · 2024-12-11T17:37:45Z

specs/interop/shared-lockbox-upgrade.md

+The upgrade process consists of three main points:
+
+- Add the chain to the op-governed dependency set
+- Move ETH liquidity from `OptimismPortal` to `SharedLockbox`
+- Upgrade the code of `OptimismPortal` to include the `SharedLockbox` integration
+
+This process also requires that:
+
+- `SharedLockbox` is deployed
+- `SuperchainConfig` is upgraded to manage the dependency set
+- `SystemConfig` is upgraded to the interop contract version


Comment: This process should be managed by an OPCM (there is a different OCPM for each l1 contracts release).
Thoughts: Chains in the initial interop set will upgrade together to this shared lockbox. But other chains will upgrade seperately to their own shared lockbox.
Future versions of the OPCM will therefore need to enable some distinct upgrade path which enables moving ETH into the interop set's lockbox.

maurelian · 2024-12-11T18:09:54Z

specs/interop/shared-lockbox.md

+MUST be triggered when `authorizePortal` is called
+
+```solidity
+event PortalAuthorized(address indexed portal);


to be clear, there is no intention to enable deletion from the list right?

This is correct, it is a confirmed decision across the organization https://docs.google.com/document/d/1UixbH3G088fD7ecHSOA8ZinzE4UdcvxFl_1GLyM_VcI

smartcontracts · 2024-12-11T18:30:45Z

specs/interop/optimism-portal-interop.md

+**`SHARED_LOCKBOX`**
+
+- An immutable address pointing to the `SharedLockbox` contract.
+- This address MUST be immutable because all `OptimismPortals` will point to the same `SharedLockbox`,


Immutable but not an immutable variable (just a note for implementation)

The value should never change but if we make it an immutable in practice, it means that chains that upgrade but don't join the cluster instantly will need to deploy their own portal contract, which just adds cost. Another way to do it would be to be the address of the lockbox on the superchain config and fetch it from there, it adds an extra call which isn't ideal but it does reduce the amount of config footguns

smartcontracts · 2024-12-11T18:33:19Z

specs/interop/shared-lockbox.md

+
+<!-- END doctoc generated TOC please keep comment here to allow auto update -->
+
+## Overview


General feedback, would like to see more of an invariant-driven approach to this specification. We need to understand the top-level properties that the contract must satisfy, then we can define the implementation spec that satisfies these properties.

tynes · 2024-12-11T19:39:21Z

specs/interop/shared-lockbox.md

+    require(_authorizedPortals[msg.sender], "Unauthorized");
+
+    // Using `donateETH` to not trigger a deposit
+    IOptimismPortal(msg.sender).donateETH{ value: _value }();


What do you think about using the selfdestruct trick to transfer ether rather than using donateETH? we have a SafeSend library that triggers a balance transfer without any execution in the contract

We have a few questions about this:

What is the motivation behind this change?

What about gas costs? Wouldn’t deploying a contract solely for making a transfer increase costs?

Is donateETH being used anywhere? If not, could we rename it to make its functionality clearer?

tynes · 2024-12-11T19:40:48Z

specs/protocol/superchain-configuration.md

+
+The contract will add the following storage layout and function:
+
+**`SHARED_LOCKBOX`**


Small nit: #### rather than ** so there are hyperliinks directly to the section

tynes · 2024-12-11T19:41:36Z

specs/protocol/superchain-configuration.md

+
+**`dependencySet`**
+
+- An `EnumerableSet` that stores the current list of chain IDs in the dependency set.


There isn't context on what an EnumerableSet is. Its an implementation detail, we should describe the properties of this data structure that we depend on

ControlCplusControlV

Left some minor comments, but overall looks great!

ControlCplusControlV · 2024-12-11T21:06:49Z

specs/interop/optimism-portal-interop.md

+
+## Overview
+
+The `OptimismPortal` contract is upgraded to integrate the `SharedLockbox` and start using the shared ETH liquidity.


"using the shared ETH liquidity" is a bit ambiguous imo, as each OptimismPortal is practically storing its Ether in the shared lock box, so we should note that funds are being moved with this change

ControlCplusControlV · 2024-12-11T21:08:32Z

specs/interop/optimism-portal-interop.md

+
+The integration with the `SharedLockbox` involves adding extra steps when executing deposit transactions
+or finalizing withdrawal transactions.
+These steps include locking and unlocking ETH without altering other aspects of the current `OptimismPortal` implementation.


I would also encourage more direct language here instead of "adding extra steps" and "include," are these definitively all the extra steps being added?

ControlCplusControlV · 2024-12-11T21:09:15Z

specs/interop/optimism-portal-interop.md

+
+Calls `lockETH` on the `SharedLockbox` with the `msg.value`.
+
+- The function MUST call `lockETH` on the `SharedLockbox` if:


I would specify the value it must call with here as well

ControlCplusControlV · 2024-12-11T21:10:04Z

specs/interop/shared-lockbox-upgrade.md

+
+The upgrade process consists of three main points:
+
+- Add the chain to the op-governed dependency set


How exactly is this defined? Which contract is involved here?

ControlCplusControlV · 2024-12-11T21:11:02Z

specs/interop/shared-lockbox-upgrade.md

+Its `addChain` function will be used to add the chain to the dependency set and call the `SystemConfig` of each chain
+to keep them in sync.
+It will also allowlist the corresponding `OptimismPortal`, enabling it to lock and unlock ETH from the `SharedLockbox`.
+Once this process is complete, the system will be ready to process deposits and withdrawals.


I would replace "this process" for "addChain is called"

ControlCplusControlV · 2024-12-11T21:11:48Z

specs/interop/shared-lockbox-upgrade.md

+
+### Migrate ETH liquidity from `OptimismPortal` to `SharedLockbox`
+
+The ETH will be transferred from the `OptimismPortal` to the `SharedLockbox` using an intermediate contract.


I think we should name this intermediate contract specifically, and just mention defined below

ControlCplusControlV · 2024-12-11T21:13:59Z

specs/interop/shared-lockbox-upgrade.md

+To set this up, the upgrade function will be called via `ProxyAdmin` to implement the new code,
+which includes the necessary `SharedLockbox` integration.
+The `SharedLockbox` address will be set during the `initialize` function. After this step,
+the `OptimismPortal` will not be able to process deposits and withdrawals until the chain is registered


We should probably specify a downtime consideration here

ControlCplusControlV · 2024-12-11T21:14:34Z

specs/interop/shared-lockbox.md

+}
+
+function authorizePortal(address _portal) external {
+    require(msg.sender == superchainConfig, "Unauthorized");


fwiw I would like to use custom errors going forward in new code

In the new implementation code, we’re using custom errors throughout. However, in the specs, we’ve been using require statements to keep the reading simpler.

smartcontracts · 2024-12-12T20:50:26Z

Dropping this comment here as per request from @tynes. I think the most valuable think to add to this PR would be to clearly define the invariants that need to be maintained when we introduce this component.

Some examples here would be:

Only authorized Portal contracts should be able to use the lockbox
Portal contracts can only be authorized by
Only safe Portal contracts can be authorized (now we need to define "safe")

I think point (3) is the most important and it would be valuable to explore all of the ways in which this condition could be violated. What is "safe"? We need to be exhaustive.

tynes · 2024-12-17T03:36:16Z

specs/interop/optimism-portal-interop.md

+
+<!-- END doctoc generated TOC please keep comment here to allow auto update -->
+
+## Overview


We should be clear that its possible to upgrade to this hardfork and not be part of a cluster. The requirement would be that a chain has its own lockbox

tynes · 2024-12-17T03:36:46Z

specs/interop/optimism-portal-interop.md

+
+The `OptimismPortal` contract will add the following storage layout and modified functions:
+
+**`SHARED_LOCKBOX`**


nit: prefer #### over ** ... ** so that hyperlinks can be generated to specific sections

* feat: add shared lockbox invariants * fix: remove immutable variable * feat: add upgrader role * feat: add authorized invariants to the portal * fix: pr * fix: add upgrader role and remove new portal invariants

agusduha requested review from protolambda, tynes, ajsutton, sebastianst and mslipper as code owners November 22, 2024 21:31

fix: lint

46aaed0

tynes reviewed Nov 27, 2024

View reviewed changes

gotzenx mentioned this pull request Nov 29, 2024

Implement ETH Lockbox ethereum-optimism/optimism#13143

Open

agusduha mentioned this pull request Nov 29, 2024

feat: introduce SharedLockbox ethereum-optimism/optimism#13144

Open

maurelian reviewed Dec 11, 2024

View reviewed changes

smartcontracts reviewed Dec 11, 2024

View reviewed changes

tynes reviewed Dec 11, 2024

View reviewed changes

ControlCplusControlV reviewed Dec 11, 2024

View reviewed changes

0xDiscotech mentioned this pull request Dec 13, 2024

feat: add shared lockbox invariants defi-wonderland/specs#15

Merged

tynes mentioned this pull request Dec 13, 2024

docs(dependency-set): specify that dependency is bidirectional #480

Closed

tynes reviewed Dec 17, 2024

View reviewed changes

feat: add shared lockbox invariants (#15)

b25ed80

* feat: add shared lockbox invariants * fix: remove immutable variable * feat: add upgrader role * feat: add authorized invariants to the portal * fix: pr * fix: add upgrader role and remove new portal invariants

Joxess mentioned this pull request Dec 20, 2024

feat: Add FMA Shared Lockbox ethereum-optimism/design-docs#180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add shared lockbox spec #465

feat: add shared lockbox spec #465

agusduha commented Nov 22, 2024

tynes Nov 27, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

tynes Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

maurelian Dec 11, 2024

agusduha Dec 13, 2024

smartcontracts Dec 11, 2024 •

edited

Loading

tynes Dec 11, 2024

smartcontracts Dec 11, 2024

tynes Dec 11, 2024

agusduha Dec 13, 2024

tynes Dec 11, 2024 •

edited

Loading

tynes Dec 11, 2024

ControlCplusControlV left a comment

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

ControlCplusControlV Dec 11, 2024

agusduha Dec 13, 2024

smartcontracts commented Dec 12, 2024

tynes Dec 17, 2024

tynes Dec 17, 2024

	These `OptimismPortal`s will be allowlisted by the `SuperchanConfig` using the `authorizePortal` function
	These `OptimismPortal`s will be allowlisted by the `SuperchainConfig` using the `authorizePortal` function


		Withdraws a specified amount of ETH from the lockbox's liquidity pool.

		- Only authorized `OptimismPortal` addresses SHOULD be allowed to interact.


		<!-- END doctoc generated TOC please keep comment here to allow auto update -->

		## Overview


		The contract will add the following storage layout and function:

		`SHARED_LOCKBOX`


		`dependencySet`

		- An `EnumerableSet` that stores the current list of chain IDs in the dependency set.


		## Overview

		The `OptimismPortal` contract is upgraded to integrate the `SharedLockbox` and start using the shared ETH liquidity.


		Calls `lockETH` on the `SharedLockbox` with the `msg.value`.

		- The function MUST call `lockETH` on the `SharedLockbox` if:


		The upgrade process consists of three main points:

		- Add the chain to the op-governed dependency set


		### Migrate ETH liquidity from `OptimismPortal` to `SharedLockbox`

		The ETH will be transferred from the `OptimismPortal` to the `SharedLockbox` using an intermediate contract.


		The `OptimismPortal` contract will add the following storage layout and modified functions:

		`SHARED_LOCKBOX`

feat: add shared lockbox spec #465

Are you sure you want to change the base?

feat: add shared lockbox spec #465

Conversation

agusduha commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartcontracts Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ControlCplusControlV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartcontracts commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartcontracts Dec 11, 2024 •

edited

Loading

tynes Dec 11, 2024 •

edited

Loading