Skip to content

feat: ban deposits interop#11712

Merged
protolambda merged 13 commits intodevelopfrom
feat/ban-deposits-interop
Sep 11, 2024
Merged

feat: ban deposits interop#11712
protolambda merged 13 commits intodevelopfrom
feat/ban-deposits-interop

Conversation

@skeletor-spaceman
Copy link
Copy Markdown
Collaborator

Description

To get ready for Interop, we need to make sure to disable ExecutingMessages to be referenced by Deposit transactions.

This PR touches both the client and the relevant pre-deploy contracts.

more context: ticket & spec

Tests

Both solidity and go additions and modifications have been properly tested.

Additional context

you can check #11362 for more context on the conversations leading to this final PR

Metadata

Comment thread op-node/rollup/derive/deposit_source.go
Comment thread packages/contracts-bedrock/src/L2/CrossL2Inbox.sol Outdated
Comment thread packages/contracts-bedrock/src/L2/L1BlockIsthmus.sol
@protolambda protolambda force-pushed the feat/ban-deposits-interop branch from 617ec63 to 67e362b Compare September 6, 2024 23:28
@protolambda protolambda requested review from a team September 6, 2024 23:28
@protolambda protolambda force-pushed the feat/ban-deposits-interop branch 2 times, most recently from 67d2030 to bd0ed78 Compare September 9, 2024 23:43
@protolambda
Copy link
Copy Markdown
Contributor

Rebased on develop, applied a bunch of fixes/improvements to the tests, fixed the deposit-source of the comple-deposits tx not mixing in the sequence-number properly, and updated metadata etc.

Now just need the spec to properly document the new deposit-source, and then ready to merge.

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Sep 11, 2024

Looks like the contracts failures come from warnings from solc

Compiler run failed:
Warning (9302): Return value of low-level calls not used.
   --> test/BenchmarkTest.t.sol:239:9:
    |
239 |         address(l1Block).call(abi.encodePacked(setValuesEcotoneCalldata));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Perhaps we should use the SafeCall library to not copy back the calldata to keep gas estimation realistic and fix this error

@protolambda
Copy link
Copy Markdown
Contributor

Rebasing this PR on develop, and updating the interfaces metadata

@protolambda protolambda force-pushed the feat/ban-deposits-interop branch from bd0ed78 to 447e7b3 Compare September 11, 2024 00:11
@protolambda
Copy link
Copy Markdown
Contributor

Rebased. But just interfaces-check-no-build complains about the solidity version that is used in CrossL2Inbox (affecting the IL1Block interface). What solidity version should it be on? cc @tynes.

Comment thread packages/contracts-bedrock/src/L2/interfaces/IL1BlockIsthmus.sol
@protolambda protolambda added this pull request to the merge queue Sep 11, 2024
Merged via the queue into develop with commit 627f7af Sep 11, 2024
@protolambda protolambda deleted the feat/ban-deposits-interop branch September 11, 2024 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interop: Ban Deposits that Trigger Executing Messages for Devnet

3 participants