feat: introduce SharedLockbox by agusduha · Pull Request #13144 · ethereum-optimism/optimism

agusduha · 2024-11-29T18:24:29Z

Description

Introducing the SharedLockbox contract to improve the Superchain’s interoperable ETH withdrawal user experience, preventing withdrawal failures caused by insufficient ETH in the OptimismPortal. Additionally, the SuperchainConfig is being updated to manage the network’s dependency set.

Tests

Add SharedLockbox tests
Adjust corresponding integration tests from OptimismPortal, SuperchainConfig and SystemConfig

Additional context

As we talked, this should be applied on the Isthmus upgrade.

Remove OPContractsManagerInterop and SystemConfigInterop because its only purpose was to set the denpendencyManager address and is not needed anymore
Refactor DeploySuperchain script to enforce contract deployment order to precalculate addresses. This is needed because the SharedLockbox and the SuperchainConfig initializers have a cross dependency.
Additional check can be added to the addDependency function to enforce the chain ID and SystemConfig address compatibility as mentioned in the spec, but there is no current source of truth for that

Metadata

Closes #13143

* feat: create shared lockbox contract with its interface and unit tests * chore: polish tests and interfaces * chore: run pre-pr * chore: improve natspec * chore: run pre-pr * chore: update compiler version

chore: sync develop

* feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert

#128) * feat: create shared lockbox contract with its interface and unit tests * chore: polish tests and interfaces * chore: run pre-pr * chore: improve natspec * chore: run pre-pr * feat: add liqudity migrator contract with its unit test and interface * chore: remove underscore on stack var * chore: add todo * chore: run pre-pr * chore: add contract title natspec and proxied * refactor: integrate testing suite with common test * chore: pre-pr * chore: add spec test

* feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert * feat: integrate system config with superchain config * fix: remove OPCM interop * test: add dependency counter test

fix: merge conflict lockbox

* feat: Add pause check Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks> * test: add tests natspecs --------- Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks>

…ox-2 fix: merge conflicts in lockbox

* feat: use superchain config lockbox in portal * test: add new sharedlockbox test

…ox-3 fix: merge conflict in develop sync

* feat: liquidity migrator deployment * test: fix comment * test: fix internal variables names

* feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr

…ox-4 fix: merge conflict lockbox

ControlCplusControlV

looks good overall, just some general stylistic comments

ControlCplusControlV · 2025-01-30T21:37:03Z

packages/contracts-bedrock/src/L1/SharedLockbox.sol

+        emit ETHLocked(msg.sender, msg.value);
+    }
+
+    /// @notice Unlocks ETH from the lockbox.


Would add natspec alerts that it cannot be called when paused as well

ControlCplusControlV · 2025-01-30T21:44:30Z

packages/contracts-bedrock/src/L1/SuperchainConfig.sol

+    /// @notice Internal initializer.
+    /// @param _guardian    Address of the guardian, can pause the OptimismPortal.
+    /// @param _paused      Initial paused status.
+    function _initialize(address _guardian, bool _paused) internal {


may be a dumb q but why are these functions seperate?

To be reused in the inherited contract. This is because there is a semgrep rule that force the initialize function to be external and to have the initializer modifier

ControlCplusControlV · 2025-01-30T21:51:00Z

packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol

+
+    /// @notice The address of the cluster manager, which can add a chain to the dependency set.
+    ///         It can only be modified by an upgrade.
+    bytes32 public constant CLUSTER_MANAGER_SLOT = bytes32(uint256(keccak256("superchainConfig.clusterManager")) - 1);


Inconsistent with OPTIMISM_PORTAL_STORAGE_SLOT which just sets the resulting value, we should use one format (value and preimagei n comments, or vice versa) across everything, I would favor pre-image and then let the value be determined at compile time

We are trying to follow the ERC7201 for unstructured storage in general, this was an exception because the SuperchainConfig guardian role is being defined like this

ControlCplusControlV · 2025-01-30T21:51:11Z

packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol

+
+    /// @notice Storage slot that the SuperchainConfigDependencies struct is stored at.
+    /// keccak256(abi.encode(uint256(keccak256("superchainConfig.dependencies")) - 1)) & ~bytes32(uint256(0xff));
+    bytes32 internal constant SUPERCHAIN_CONFIG_DEPENDENCIES_SLOT =


same comment as above with OPTIMISM_PORTAL_STORAGE_SLOT

ControlCplusControlV · 2025-01-30T21:51:50Z

packages/contracts-bedrock/src/L1/SuperchainConfigInterop.sol

+
+    /// @notice Checks if a chain is part or not of the dependency set.
+    /// @param _chainId The chain ID to check for.
+    function isInDependencySet(uint256 _chainId) public view returns (bool) {


is this function documented in the spec?

Yes it should, the specs are gonna be refactored too tho

ControlCplusControlV · 2025-01-30T21:52:28Z

packages/contracts-bedrock/src/L2/DependencyManager.sol

+    event DependencyAdded(uint256 indexed chainId, address indexed systemConfig, address indexed superchainConfig);
+
+    /// @notice The minimum gas limit for the withdrawal tx to update the dependency set on L1.
+    uint256 internal constant ADD_DEPENDENCY_WITHDRAWWAL_GAS_LIMIT = 400_000;


where does this value come from?

We made some gas consumption tests, we put a high minimum gas limit to ensure the withdrawal doesn't fail.

This code is not going to be used though

mslipper · 2025-01-31T18:02:11Z

/ci authorize f805e78

tynes · 2025-01-31T23:54:14Z

We made the following decisions:

remove ability to auth portals via withdrawals, only auth via multisig
dont put the dep set manager in state for now

This will make us code complete for the audit, pending any other fixes the wonderland security team finds

* feat: descope dependency manager * test: fix tests * test: fix tests

* fix: I-0 * fix: I-1 * fix: I-2 * fix: I-3 * fix: I-6 * fix: I-7 * fix: I-9 * fix: pre pr

…ox-10 fix: sync develop

* fix: portal withdrawal checks * fix: include current withdrawal check

tynes · 2025-02-05T15:06:32Z

/ci authorize 2860621

tynes · 2025-02-05T16:08:53Z

Not sure what the problem is with the go tests, we may need to rebase

mslipper · 2025-02-05T16:53:34Z

@tynes these are not flakes, these are real bugs.

mslipper · 2025-02-05T17:12:24Z

/ci authorize d1e7d79

tynes · 2025-02-05T23:26:51Z

/ci authorize 24476ae

tynes · 2025-02-05T23:48:14Z

Looks like there are still some failing go tests

tynes · 2025-02-05T23:49:14Z

--- FAIL: TestInteropDeployment (0.68s)
panic: revision id 1 cannot be reverted [recovered]
	panic: revision id 1 cannot be reverted

what does this mean?

mslipper · 2025-02-06T00:00:53Z

It means that there's a revert preventing OP Deployer from deploying the contracts. I'll look into it.

…ox-11 fix: sync develop

mslipper · 2025-02-06T00:50:58Z

Replaced by #14199.

0xDiscotech and others added 11 commits November 20, 2024 18:24

feat: add shared lockbox (#126)

bf3278f

* feat: create shared lockbox contract with its interface and unit tests * chore: polish tests and interfaces * chore: run pre-pr * chore: improve natspec * chore: run pre-pr * chore: update compiler version

Merge pull request #136 from defi-wonderland/chore/sync-develop

0324436

chore: sync develop

feat: integrate portal to lockbox (#139)

27a4515

* feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert

feat: integrate system config with superchain config (#140)

b240094

* feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert * feat: integrate system config with superchain config * fix: remove OPCM interop * test: add dependency counter test

feat: manage dependency set on superchain config (#138)

3254d14

chore: add zero dependencies check (#142)

2c7d996

Merge branch 'develop' into fix/merge-conflict-lockbox

7111d13

fix: pre pr

4a45ed2

Merge pull request #143 from defi-wonderland/fix/merge-conflict-lockbox

cae605e

fix: merge conflict lockbox

agusduha requested a review from a team as a code owner November 29, 2024 18:24

agusduha requested a review from smartcontracts November 29, 2024 18:24

agusduha and others added 3 commits December 9, 2024 14:04

Merge branch 'develop' into fix/merge-conflict-lockbox-2

8bef762

fix: pre pr and interfaces imports

036b1d5

Merge pull request #147 from defi-wonderland/fix/merge-conflict-lockb…

9163c9c

…ox-2 fix: merge conflicts in lockbox

protolambda added A-interop A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Dec 16, 2024

agusduha and others added 5 commits December 19, 2024 15:23

feat: add upgrader role to superchain config (#163)

6f5c86d

feat: use superchain config lockbox in portal (#164)

85d49dc

* feat: use superchain config lockbox in portal * test: add new sharedlockbox test

Merge branch 'develop' into fix/merge-conflict-lockbox-3

078bb36

fix: pre pr

657c97b

Merge pull request #165 from defi-wonderland/fix/merge-conflict-lockb…

36a8897

…ox-3 fix: merge conflict in develop sync

Joxess mentioned this pull request Dec 20, 2024

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

Closed

agusduha and others added 6 commits December 24, 2024 16:04

feat: liquidity migrator deployment (#166)

3b9eba9

* feat: liquidity migrator deployment * test: fix comment * test: fix internal variables names

feat: dependency set refactor (#170)

4183809

* feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr

Merge branch 'develop' into fix/merge-conflict-lockbox-4

0160192

fix: pre pr

f632210

fix: semgrep

2c371fd

Merge pull request #172 from defi-wonderland/fix/merge-conflict-lockb…

ebb669a

…ox-4 fix: merge conflict lockbox

ControlCplusControlV approved these changes Jan 30, 2025

View reviewed changes

tynes mentioned this pull request Jan 31, 2025

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

Merged

agusduha and others added 7 commits February 3, 2025 14:47

feat: descope dependency manager (#242)

56b927a

* feat: descope dependency manager * test: fix tests * test: fix tests

chore: improve eth liquidity test (#248)

9a28526

fix: internal review fixes (#243)

2dda88d

* fix: I-0 * fix: I-1 * fix: I-2 * fix: I-3 * fix: I-6 * fix: I-7 * fix: I-9 * fix: pre pr

Merge branch 'develop' into fix/merge-conflict-lockbox-10

31da5ed

fix: pre pr

92576f1

Merge pull request #253 from defi-wonderland/fix/merge-conflict-lockb…

7770ff2

…ox-10 fix: sync develop

fix: portal withdrawal checks (#255)

2860621

* fix: portal withdrawal checks * fix: include current withdrawal check

agusduha changed the base branch from develop to feat/interop-stable-devnet February 5, 2025 13:17

fix: remove unused interop contracts (#256)

d1e7d79

test: fix flake tests (#257)

24476ae

fix: adjust op-deployer interop scripts (#262)

2b0abfe

agusduha requested a review from a team as a code owner February 5, 2025 23:53

agusduha and others added 3 commits February 5, 2025 21:32

Merge branch 'develop' into fix/merge-conflict-lockbox-11

37f0dc8

fix: pre pr

9c582f4

Merge pull request #264 from defi-wonderland/fix/merge-conflict-lockb…

d0d7c1e

…ox-11 fix: sync develop

mslipper mentioned this pull request Feb 6, 2025

feat: introduce SharedLockbox #14199

Merged

mslipper closed this Feb 6, 2025

Conversation

agusduha commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ControlCplusControlV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agusduha Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mslipper commented Jan 31, 2025

Uh oh!

tynes commented Jan 31, 2025

Uh oh!

tynes commented Feb 5, 2025

Uh oh!

tynes commented Feb 5, 2025

Uh oh!

mslipper commented Feb 5, 2025

Uh oh!

mslipper commented Feb 5, 2025

Uh oh!

tynes commented Feb 5, 2025

Uh oh!

tynes commented Feb 5, 2025

Uh oh!

tynes commented Feb 5, 2025

Uh oh!

mslipper commented Feb 6, 2025

Uh oh!

mslipper commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

agusduha commented Nov 29, 2024 •

edited

Loading

agusduha Feb 4, 2025 •

edited

Loading