feat: dependency set refactor by agusduha · Pull Request #170 · defi-wonderland/optimism

agusduha · 2024-12-26T18:31:03Z

The rationale behind this PR is found in the following PR

The SuperchainConfig is now the only source for storing the dependencySet

The changes have impact in L1BlockInterop, SystemConfigInterop, CrossL2Inbox and L2toL2CDM

hexshire · 2024-12-27T03:04:23Z

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


    /// @notice Semantic version.
    /// @custom:semver 1.1.1-beta.5
    string public constant version = "1.1.1-beta.5";


Do you have to upgrade this?

No need to because it is updated in the main PR that has not been merged to develop yet

hexshire · 2024-12-27T14:27:09Z

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

-    SET_GAS_PAYING_TOKEN,
-    ADD_DEPENDENCY,
-    REMOVE_DEPENDENCY
+    SET_GAS_PAYING_TOKEN


Why using a enum with just one member?

hexshire · 2024-12-27T14:29:42Z

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

@@ -109,10 +72,6 @@ contract L1BlockInterop is L1Block {

        if (_type == ConfigType.SET_GAS_PAYING_TOKEN) {


Same here, are we expecting to add new members to ConfigType?

Is a good question we could ask to them

It is how it works, OptimismPortal.setConfig() can send multiple types, so this has to be extensible.

We will be using this feature in another PR so it should stay like this

Mm... I think that the enum is right cause it will be easier to expand if needed... but this is no needed cause if you have to add another member you have to modify the function too.

Without the if any call to setConfig means calling _setGasPayingToken and it shouldn't be like that

0xDiscotech

Amazing job ser, left only few small comments on the code.
Besides them, 1 more question and 1 suggestion:

On the PoC, Mark wrote a function to remove dependencies on L1. Do we need to implement it?
If maintaining the dependencySetSize() function, missing its tests (the other view functions have tests as well)

packages/contracts-bedrock/scripts/deploy/ManageDependencies.s.sol

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

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

gotzenx · 2024-12-30T18:00:39Z

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


        // Ensure there are no repeated values on the input array
        for (uint256 i; i < _chainIdsArray.length; i++) {
+            vm.assume(_chainIdsArray[i] != block.chainid);


Suggested change

vm.assume(_chainIdsArray[i] != block.chainid);

if (_chainIdsArray[i] != block.chainid) chainIds.add(_chainIdsArray[i]);

I think that adding an assume here is a bit too overkill and will cause to discard and re-run this test an unnecessary amount of times. We could instead just decide to skip the chainId if its equal to chainId. I dont think we are affecting the fuzz testing here since we are steal keeping with the "RNG"
What do you guys think?

* 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>

feat: dependency set refactor

c0b840a

agusduha requested review from 0xDiscotech, gotzenx and hexshire December 26, 2024 18:31

agusduha self-assigned this Dec 26, 2024

fix: deploy script variable name

85b8c21

hexshire reviewed Dec 27, 2024

View reviewed changes

0xDiscotech reviewed Dec 27, 2024

View reviewed changes

fix: pr

560d06c

0xDiscotech reviewed Dec 30, 2024

View reviewed changes

packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol Show resolved Hide resolved

0xDiscotech previously approved these changes Dec 30, 2024

View reviewed changes

gotzenx reviewed Dec 30, 2024

View reviewed changes

fix: pr

651e5db

agusduha dismissed 0xDiscotech’s stale review via 651e5db December 30, 2024 18:27

gotzenx approved these changes Dec 30, 2024

View reviewed changes

agusduha merged commit 4183809 into sc-feat/add-shared-lockbox Dec 30, 2024

agusduha deleted the feat/dependency-set-refactor branch December 30, 2024 18:29

		@@ -109,10 +72,6 @@ contract L1BlockInterop is L1Block {

		if (_type == ConfigType.SET_GAS_PAYING_TOKEN) {

	vm.assume(_chainIdsArray[i] != block.chainid);
	if (_chainIdsArray[i] != block.chainid) chainIds.add(_chainIdsArray[i]);

Comments

Conversation

agusduha commented Dec 26, 2024

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!

0xDiscotech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gotzenx Dec 30, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gotzenx Dec 30, 2024 •

edited

Loading