Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interop: cross L2 inbox #8258

Closed
wants to merge 3 commits into from
Closed

interop: cross L2 inbox #8258

wants to merge 3 commits into from

Conversation

protolambda
Copy link
Contributor

Description

Draft of interop CrossL2Inbox contract, integrated into deploy-config, inserted into L2 genesis as predeploy if interop is active at genesis.

For the prototype, we simply update the inbox with a configurable EOA. In the next iteration we can swap this out with a system-derived message, that the node validates with subjective safety (i.e. check the output-root of the other L2 chain by running a full node of said chain, or through alternative light-client backed mechanism).

Depends on #8256

Tests

  • Smart-contract unit-tests of the L2InboxContract.
  • Op-e2e action-test that runs a chain with the deploy-config, and successfully inserts an output-root into the inbox.

Metadata

Fix https://github.com/ethereum-optimism/interop/issues/21

@protolambda protolambda requested review from a team as code owners November 23, 2023 20:00
@protolambda protolambda requested review from tynes and removed request for a team November 23, 2023 20:00
return SUPERCHAIN_POSTIE;
}

/// @notice The inbox receives mail from the postie.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make the calldata structure more complex, by grouping output-roots of the same chain ID? There's no place to share a local intermediate storage key. It should be a single SSTORE either way. There's a double lookup to two related but different calldata struct fields, but there's nothing to share their either. I'm down to optimize this function with assembly and other tricks later FTW, but we're only in the prototype stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, depending on the design, we might be able to include output-roots without their chain ID, by including the chain-identity in the output-root pre-image itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Was just commenting on the declaring a local storage mapping when doing the nested accesses to save some hashing. I thought it was a cool pattern to share

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the chain identity in the output does seem like an interesting idea but unclear what UX implications that would have. Ideally there is p2p network for serving output root preimages

require(msg.sender == SUPERCHAIN_POSTIE, "CrossL2Inbox: only postie can deliver mail");
for (uint256 i = 0; i < mail.length; i++) {
roots[mail[i].chain][mail[i].output] = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want events here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? It increases cross-L2 bridging cost, but it may make UX better. Currently we don't depend on receipts for this. Will ignore in prototype, it's rather easy to add later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, for indexers having the event is ideal until shadow events are the norm

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #8258 (48c24b1) into develop (65d5813) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8258      +/-   ##
===========================================
+ Coverage    34.69%   34.73%   +0.03%     
===========================================
  Files          167      168       +1     
  Lines         7136     7140       +4     
  Branches      1205     1206       +1     
===========================================
+ Hits          2476     2480       +4     
  Misses        4509     4509              
  Partials       151      151              
Flag Coverage Δ
cannon-go-tests 63.48% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests 20.58% <100.00%> (+0.15%) ⬆️
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 42.18% <ø> (ø)
sdk-tests 42.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ges/contracts-bedrock/src/interop/CrossL2Inbox.sol 100.00% <100.00%> (ø)


/// @notice The collection of output roots, by chain.
/// chain ID => output root => bool.
mapping(bytes32 => mapping(bytes32 => bool)) public roots;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is chainid a bytes32 instead of a uint?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a UX perspective it'll be easier to represent as an integer instead of a hex value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're thinking of versioning and formatting the chain identifiers in a way that's more forward compatible if we ever have to deal with new source/destination types, or conflicting chains.

@tynes
Copy link
Contributor

tynes commented Nov 24, 2023

Trying to reduce footguns for adding new predeploys with #8269
The diff is too sprawling to add a new predeploy so I worry that something will eventually be forgotten. Once #8148 is merged then we can move all of the check-l2 logic into solidity unit tests and delete that file

@protolambda protolambda added the M-do-not-merge Meta: Do not merge label Nov 27, 2023
@protolambda protolambda marked this pull request as draft November 27, 2023 16:09
@protolambda
Copy link
Contributor Author

Marking as draft / do-not-merge, this is blocked by interop design decisions, this might not be the right approach to do the bridging.

@protolambda protolambda force-pushed the interop-deploy-config branch 2 times, most recently from 0c89106 to dee66db Compare November 29, 2023 22:13
Base automatically changed from interop-deploy-config to develop November 29, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-do-not-merge Meta: Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants