feat: integrate portal to lockbox#139
Conversation
0xDiscotech
left a comment
There was a problem hiding this comment.
Tests
Github didn't let me comment on untouched code, but here, we need to replace this https://github.dev/defi-wonderland/optimism/blob/141c2e18dbf565ac0b310c7346a9096ae9059447/packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol#L881-L883
with
vm.deal(address(sharedLockbox), _defaultTx_noData.value);
vm.expectCall(address(sharedLockbox), abi.encodeCall(sharedLockbox.unlockETH, (_defaultTx_noData.value)));The same expectCall over the lockbox unlock() can be added on
test_finalizeWithdrawalTransaction_provenWithdrawalHashEther_succeedstest_finalizeWithdrawalTransaction_secondaryProof_succeeds
And finally, it could be added on the rest of these tests (but I'm less sure):
test_finalizeWithdrawalTransaction_targetFails_failstest_finalizeWithdrawalTransaction_onReplay_revertstest_finalizeWithdrawalTransaction_onReentrancy_reverts (on the first call)
The reason why I think that it should be added even on tests that check a failure, is that they still check the events emitted on them, so IMO we should repeat the behavior with the expected call.
| assertEq(success, true); | ||
| assertEq(address(optimismPortal).balance, 100); | ||
| assertEq(address(optimismPortal).balance, 0); | ||
| assertEq(address(sharedLockbox).balance, 100); |
There was a problem hiding this comment.
Missing assertEq(address(sharedLockbox).balance, 0); at the beggining of the test as well
There was a problem hiding this comment.
The same check can be added at the beggining on _preBridgeETH and _preBridgeETHTo to maintain consistency as well.
Maybe we can even remove the assertEq(address(optimismPortal).balance, 0); check on them since it makes no sense now to check that. But if in doubt, we can keep it bc it doesn't harm
There was a problem hiding this comment.
I will add the check at the beginning, but for now will leave the portal one also
There was a problem hiding this comment.
The assertEq(address(sharedLockbox).balance, 0); check is missing on _preBridgeETHTo and _preBridgeETH
0xParti
left a comment
There was a problem hiding this comment.
This is amazing. Very clean code and tests!
* 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>
OptimismPortalto work with theSharedLockboxby locking and unlocking ETH.SharedLockboxto the current scripts to be fully integrated for testing and deployments.OptimismPortal2to be under the contract limit size.Closes OPT-500
Closes OPT-557