[WIP] feat: new lockbox by agusduha · Pull Request #192 · defi-wonderland/optimism

agusduha · 2025-01-09T23:28:44Z

No description provided.

tynes · 2025-01-09T23:35:08Z

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

@@ -28,9 +28,6 @@ contract SharedLockbox is ISemver {
    /// @notice The address of the SuperchainConfig contract.
    ISuperchainConfig public immutable SUPERCHAIN_CONFIG;


We should not leak the fact that this is immutable into the abi, could we add a superchainConfig()(address) getter and make internal immutable SUPERCHAIN_CONFIG ?

This contract address should never change, the SharedLockbox should always have the same SuperchaingConfig contract address, or were you thinking of something else?

We can make it immutable but no leak the camel case to the abi, then we leak implementation details into the abi

tynes · 2025-01-10T02:25:05Z

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

+/// @notice
+contract UpgradeHandler {
+    function addDependency(address _supechainConfig, uint256 _chainId, address _systemConfig) external {
+        if (msg.sender != Constants.DEPOSITOR_ACCOUNT) revert Unauthorized(); // TODO: check if this is safe enough


This is safe enough because only the derivation pipeline can mock this identity

tynes · 2025-01-10T02:25:31Z

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

+/// @title UpgradeHandler
+/// @notice
+contract UpgradeHandler {
+    function addDependency(address _supechainConfig, uint256 _chainId, address _systemConfig) external {


nit: typo _superchainConfig

tynes · 2025-01-10T02:25:57Z

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

+
+        IL2ToL1MessagePasser(payable(Predeploys.L2_TO_L1_MESSAGE_PASSER)).initiateWithdrawal(
+            _supechainConfig,
+            400_000, // TODO: find an arbitrary value that is enough


We need to remove the gas limit on withdrawals, its the source of so much pain

tynes · 2025-01-10T02:26:49Z

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

+
+    event ETHMigrated(uint256 amount);
+
+    function migrateLiquidity() external {


Do you think its better to move away from the intermediate upgrade contract?

This approach does simplify multisig operations which is nice

Yes! Also you can be sure that the ETH liquidity migration is done at the same time as the portal is authorized to withdraw from the lockbox. The other way it has to be manually done and checked before processing the addDependency withdrawal tx .

Ok i am on board with this

tynes · 2025-01-10T03:25:24Z

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


-        // Check the Identifier.
-        _checkIdentifier(_id);
+        if (!IDependencySet(Predeploys.L1_BLOCK_ATTRIBUTES).isInDependencySet(_id.chainId)) revert InvalidChainId();


If we have this, then we will need the first block to set the dependency set, which makes testing a bit more complicated. The real benefit that I see is on outgoing messages from the L2 to L2 xdm

We need to make sure that we do not have unprovable logs. Its most simple to assume that you cannot consume logs before the interop start. We would need to move that logic offchain in the block builder, proof and fork choice rule if its not onchain. I think we can get away with keeping everything that was in _checkIdentifier offchain, it will save a bit of gas over time

Ok perfect, we will remove the check from here but leave it in the L2toL2CDM

tynes · 2025-01-10T03:28:03Z

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

@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.25;


Curious if you have an opinion on the dependency set living in this contract vs the L1Block contract. The tradeoff is with it being in the L1Block contract, it expands the ABI, which causes more gas cost for every single system transaction doing 4byte selector matching. Its likely a minimal amount of gas but it adds up every block. Historically we just always added things to L1Block but this consideration exists

I like this approach, as we talked, we can have a DependencyManager predeploy handling this

tynes · 2025-01-10T03:44:09Z

These changes generally look good to me

tynes · 2025-01-13T17:35:53Z

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

+    // TODO: check if this function makes sense
+    /// @notice Initializes the first portal of the cluster.
+    /// @param _initialPortal The address of the initial portal.
+    function initializePortal(address _initialPortal) external {


We agreed that for the first version, we would make the auth be "withdrawal OR multisig" and add a new role to superchain config that represents the multisig. This solves the bootstrapping problem and lets us handle the first upgrade to interop in a simple way. In the future, we can integrate more tightly into OPCM where when deploying a cluster, the first portal is auth'd by op-deployer/opcm. cc @maurelian for visibility

tynes · 2025-01-13T20:42:41Z

packages/contracts-bedrock/snapshots/storageLayout/SuperchainConfig.json

    "type": "bool"
  },
+  {
+    "bytes": "20",


We may want to move this storage slot away from being managed by solc, its putting it on the same slot as _initialized which is annoying to deal with

We are going to change everything to use unstructured storage

tynes · 2025-01-13T20:45:08Z

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

    string public constant version = "1.1.1-beta.5";

+    /// @notice The Shared Lockbox contract
+    ISharedLockbox public sharedLockbox;


I would recommend that we either use a spacer to move this away from being in the same slot as initialized or we use a custom setter/getter + a hash to define the slot

slight preference for custom storage

Agree! We are moving to unstructured storage to avoid this problems and future upgrades

tynes · 2025-01-13T20:47:41Z

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


        if (_dependencySet.length() == type(uint8).max) revert DependencySetTooLarge();
-        if (_chainId == block.chainid) revert InvalidChainID();
+        if (_chainId == block.chainid) revert InvalidChainID(); // TODO: is this check really necessary?


This check is good for preventing footguns altho i don't think its that useful in practice. If we mess up the config for the dep set then we are going to have a bad time regardless of this check. I would be down to remove this to reduce test maintenance

tynes · 2025-01-13T20:52:15Z

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

-
    /// @notice Stores the Identifier in transient storage.
    /// @param _id Identifier to store.
    function _storeIdentifier(Identifier calldata _id) internal {


We chatted about this out of band, but we can remove the whole _storeIdentifier function and the getters for the context variables given the only entrypoint now is the validateMessage function

tynes · 2025-01-13T20:53:39Z

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

+        // Initiate a withdrawal tx to update the dependency set on L1.
+        IL2ToL1MessagePasser(payable(Predeploys.L2_TO_L1_MESSAGE_PASSER)).initiateWithdrawal(
+            _superchainConfig,
+            400_000, // TODO: find an arbitrary value that is enough


In the future, I want to make a new "type" of withdrawal message that doesn't have a gas limit for the call and there is a require on the success of the call. This would simplify so much. For now we have to deal with this whole gaslimit thing and be careful of the footgun

tynes · 2025-01-13T20:54:06Z

packages/contracts-bedrock/test/L1/LiquidityMigrator.t.sol

        // Set the migrator as an authorized portal so it can lock the ETH while migrating
        vm.prank(address(superchainConfig));
-        sharedLockbox.authorizePortal(address(liquidityMigrator));
+        // sharedLockbox.authorizePortal(address(liquidityMigrator));


nit: dead code

tynes · 2025-01-13T21:01:15Z

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

+import { IL2ToL1MessagePasser } from "interfaces/L2/IL2ToL1MessagePasser.sol";
+import { ISuperchainConfig } from "interfaces/L1/ISuperchainConfig.sol";
+
+/// @custom:proxied true


TODO: backport spec ethereum-optimism/specs#495

tynes · 2025-01-13T21:02:13Z

This is good to merge into the feature branch once the comments have been ack'd

tynes · 2025-01-14T16:13:52Z

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

@@ -1,37 +0,0 @@
-// SPDX-License-Identifier: MIT
-pragma solidity 0.8.15;


I think that we still need this contract for the multisig path

Now the new CLUSTER_MANAGER role can directly call addDependency following the same flow as withdrawals

tynes · 2025-01-14T16:15:03Z

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

-    ///         One-time use logic upgraded over OptimismPortalProxy address and then deprecated by another approval.
-    function migrateETH() external {
-        uint256 balance = address(this).balance;
-        SHARED_LOCKBOX.lockETH{ value: balance }();


This contract could be generic if the abi on the shared lockbox was just fallback rather than a function

tynes · 2025-01-14T16:15:51Z

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

@@ -1,16 +1,23 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.15;
+pragma solidity 0.8.25;


In favor of making new contracts 0.8.25

tynes · 2025-01-14T16:18:41Z

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

        if (_destination == block.chainid) revert MessageDestinationSameChain();
        if (_target == Predeploys.CROSS_L2_INBOX) revert MessageTargetCrossL2Inbox();
        if (_target == Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER) revert MessageTargetL2ToL2CrossDomainMessenger();
+        if (!IDependencySet(Predeploys.L1_BLOCK_ATTRIBUTES).isInDependencySet(_destination)) revert InvalidChainId();


I think we need to update the address being called here

Fixing, also changing L2ToL2CDM tests to be integrated with the setup so this can be catch

The Setup can't be integrated rn because of how the tests are being handled for transient storage testing. Maybe we can do it once the CommonTest and Setup are migrated to 0.8.25.

* feat: refactor interop feature contracts * fix: add noops comment

* feat: add shared lockbox (#126) * 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 * feat: integrate portal to lockbox (#139) * feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert * feat: add liquidity migrator contract with its unit test and interface (#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 system config with superchain config (#140) * 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) * chore: add zero dependencies check (#142) * fix: pre pr * feat: Add pause check (#145) * 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> * fix: pre pr and interfaces imports * feat: add upgrader role to superchain config (#163) * feat: use superchain config lockbox in portal (#164) * feat: use superchain config lockbox in portal * test: add new sharedlockbox test * fix: pre pr * feat: liquidity migrator deployment (#166) * feat: liquidity migrator deployment * test: fix comment * test: fix internal variables names * feat: dependency set refactor (#170) * feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr * fix: pre pr * fix: semgrep * fix: merge conflict * [WIP] feat: new lockbox (#192) * chore: partial implementation comments * feat: new lockbox * feat: introduce dependency manager predeploy * feat: remove timestamp check from CrossL2Inbox * feat: introduce cluster manager role and remove immutables * fix: remove unnecessary code, fix tests and setup * feat: use unstructured storage and OZ v5 * fix: L2ToL2CDM dependency set check * fix: dependency manager gas limit * feat: refactor interop feature contracts (#200) * feat: refactor interop feature contracts * fix: add noops comment * feat: adds OptimismPortal migrated flag * test: add missing tests * fix: portal interop storage naming --------- Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com> * fix: pre pr, setup and tests * fix: remove system config interop and add interop portal target check (#205) * fix: remove system config interop and add interop portal check * fix: interop portal target check order * fix: remove wrong comment * fix: refactor portal noops function (#206) * test: add dependency manager and portal interop tests (#209) * feat: initialize shared lockbox in interop portal (#211) * feat: initialize shared lockbox in interop portal * fix: refactor shared lockbox storage getter * fix: lockbox pr fixes (#214) * fix: pre pr * fix: semver lock * fix: semver lock * feat: descope dependency manager (#242) * feat: descope dependency manager * test: fix tests * test: fix tests * chore: improve eth liquidity test (#248) * fix: internal review fixes (#243) * fix: I-0 * fix: I-1 * fix: I-2 * fix: I-3 * fix: I-6 * fix: I-7 * fix: I-9 * fix: pre pr * fix: pre pr * fix: portal withdrawal checks (#255) * fix: portal withdrawal checks * fix: include current withdrawal check * fix: remove unused interop contracts (#256) * test: fix flake tests (#257) * fix: adjust op-deployer interop scripts (#262) * fix: pre pr * fix op-deployer tests * remove dependency code * fix lint * use Bob account --------- Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Co-authored-by: AgusDuha <81362284+agusduha@users.noreply.github.com> Co-authored-by: agusduha <agusnduha@gmail.com> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks> Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com>

skeletor-spaceman and others added 2 commits January 9, 2025 10:20

chore: partial implementation comments

628e4ff

feat: new lockbox

0a7b24b

agusduha requested a review from skeletor-spaceman January 9, 2025 23:28

agusduha self-assigned this Jan 9, 2025

tynes reviewed Jan 9, 2025

View reviewed changes

tynes reviewed Jan 10, 2025

View reviewed changes

agusduha and others added 2 commits January 10, 2025 15:33

feat: introduce dependency manager predeploy

9f00ac4

feat: remove timestamp check from CrossL2Inbox

d147251

tynes reviewed Jan 13, 2025

View reviewed changes

feat: introduce cluster manager role and remove immutables

8cf1687

tynes reviewed Jan 13, 2025

View reviewed changes

agusduha added 2 commits January 13, 2025 16:37

fix: remove unnecessary code, fix tests and setup

6f03189

feat: use unstructured storage and OZ v5

9ebe502

tynes reviewed Jan 14, 2025

View reviewed changes

agusduha added 2 commits January 14, 2025 10:55

fix: L2ToL2CDM dependency set check

f2c8520

fix: dependency manager gas limit

0cc6d72

sigma mentioned this pull request Jan 15, 2025

feat(kurtosis-devnet): add non-trivial smoke test ethereum-optimism/optimism#13778

Closed

agusduha and others added 4 commits January 15, 2025 16:47

feat: refactor interop feature contracts (#200)

0714122

* feat: refactor interop feature contracts * fix: add noops comment

feat: adds OptimismPortal migrated flag

faa912f

test: add missing tests

71c1eb6

fix: portal interop storage naming

6537688

skeletor-spaceman approved these changes Jan 16, 2025

View reviewed changes

agusduha merged commit 18bd71e into sc-feat/add-shared-lockbox Jan 16, 2025

agusduha deleted the feat/new-lockbox branch January 16, 2025 16:38

		@@ -28,9 +28,6 @@ contract SharedLockbox is ISemver {
		/// @notice The address of the SuperchainConfig contract.
		ISuperchainConfig public immutable SUPERCHAIN_CONFIG;


		event ETHMigrated(uint256 amount);

		function migrateLiquidity() external {

		@@ -0,0 +1,30 @@
		// SPDX-License-Identifier: MIT
		pragma solidity 0.8.25;

		@@ -1,37 +0,0 @@
		// SPDX-License-Identifier: MIT
		pragma solidity 0.8.15;

Conversation

agusduha commented Jan 9, 2025

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!

agusduha Jan 10, 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!

tynes Jan 10, 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!

tynes commented Jan 10, 2025

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!

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!

tynes commented Jan 13, 2025

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!

agusduha Jan 14, 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!

Reviewers

agusduha Jan 10, 2025 •

edited

Loading

tynes Jan 10, 2025 •

edited

Loading

agusduha Jan 14, 2025 •

edited

Loading