refactor: new messengers by smartcontracts · Pull Request #421 · ethereum-optimism/optimistic-specs

smartcontracts · 2022-05-06T21:50:28Z

Implements the new messenger flow. This ensures that the messengers and bridges work in a backwards compatible way. The previous iteration of the messengers didn't take into account deposits that were in flight during the upgrade. This pull request has some major architectural changes so that the bridges now go through the messengers again. This allows for replayability in the bridges as well as gives the bridges replay protection.

A main feature of this PR is that the messenger and bridges now extend a base messenger or bridge contract to make the functionality basically the same on both L1 and L2. The previous iteration of the contracts made a distinction between the L1 and L2 sides of things. The previous APIs are persisted, but now there is a universal API that can be used on the bridges/messengers no matter which domain you are in.

Hardhat deploy scripts are added to the ops setup.

The bindings for the l2os and opnode are regenerated.

Deprecates:

Unblocks:

opnode: Withdrawal E2E test in go #423

Closes:

Withdrawer receive function #398

codecov-commenter · 2022-05-06T21:52:30Z

Codecov Report

Merging #421 (5e37e47) into main (3a1f6ff) will increase coverage by 1.42%.
The diff coverage is 7.14%.

@@            Coverage Diff             @@
##             main     #421      +/-   ##
==========================================
+ Coverage   51.68%   53.10%   +1.42%     
==========================================
  Files          57       62       +5     
  Lines        5764     6498     +734     
==========================================
+ Hits         2979     3451     +472     
- Misses       2415     2608     +193     
- Partials      370      439      +69

Impacted Files	Coverage Δ
opnode/contracts/deposit/deposit_feed_raw.go	`5.21% <0.00%> (ø)`
l2os/bindings/l2oo/l2_output_oracle.go	`8.60% <5.71%> (-0.29%)`	⬇️
l2os/drivers/l2output/driver.go	`67.24% <100.00%> (+0.18%)`	⬆️
opnode/rollup/driver/state.go	`71.23% <0.00%> (-5.36%)`	⬇️
l2os/service.go	`90.78% <0.00%> (-3.95%)`	⬇️
opnode/rollup/driver/step.go	`61.37% <0.00%> (-3.13%)`	⬇️
opnode/node/node.go	`60.00% <0.00%> (-1.71%)`	⬇️
opnode/node/api.go	`58.64% <0.00%> (-0.76%)`	⬇️
opnode/p2p/config.go	`7.83% <0.00%> (-0.18%)`	⬇️
opnode/service.go	`0.00% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a1f6ff...5e37e47. Read the comment docs.

Co-authored-by: smartcontracts <kelvin@optimism.io>

Implement the `L1CrossDomainMessenger` and `L2CrossDomainMessenger` based on the base `CrossDomainMessenger`. This makes the interfaces the same on both sides. Co-authored-by: smartcontracts <kelvin@optimism.io>

Also implement the `L1StandardBridge` and `L2StandardBridge` based off of the base `StandardBridge` Co-authored-by: smartcontracts <kelvin@optimism.io>

Co-authored-by: smartcontracts <kelvin@optimism.io>

norswap

I'm missing some kind of document (or even better, diagram) that tie all the components together. In particular, illustrating the flow of L1->L2 bridging and L2->L1 bridging through the components & functions would be really a great help in wrapping your head around the logic.
We should consolidate the "new API" and the "legacy API" (separately!) and provide a mapping / migration path between both.
It seems to me (unless I've missed something) that the relayMessage call is never used. It's not necessary when doing L1->L2 (because of deposit inclusion), but it's not used either when doing L2->L1 (instead the logic lives entirely within OptimismPortal).
- Suggest removing relayMessage from CrossDomainMessenger.
- Preserve it in L1CrossDomainMessenger (for backward compatibility) but call into OptimismPortal instead.

packages/contracts/contracts/universal/StandardBridge.sol

norswap · 2022-05-11T15:39:18Z

packages/contracts/contracts/universal/StandardBridge.sol

+
+        emit ETHBridgeFinalized(_from, _to, _amount, _data);
+        (bool success, ) = _to.call{ value: _amount }(new bytes(0));
+        require(success, "TransferHelper::safeTransferETH: ETH transfer failed");


That require string needs to be updated.

packages/contracts/contracts/universal/StandardBridge.sol

norswap · 2022-05-11T16:31:19Z

packages/contracts/contracts/L1/L1CrossDomainMessenger.sol

+     */
+    function _isSystemMessageSender() internal view override returns (bool) {
+        return msg.sender == address(portal) && portal.l2Sender() == otherMessenger;
+    }


This is only used in CrossDomainMessenger#relayMessage, which is never called by the OptimismPortal??

Also the comment "Ensure that the L1CrossDomainMessenger can only be called by the OptimismPortal" seems false?

The privilege of the messenger is that it can relay messages that have not not been marked as received (but again, the portal never relays currently).

The OptimismPortal does make calls on L1 for a withdrawal, the flow through the L2 messenger would be encoding a call to the L1 messenger relayMessage

Gotcha, I get it now 👍

specs/messengers.md

packages/contracts/contracts/L1/L1StandardBridge.sol

norswap · 2022-05-11T16:57:11Z

packages/contracts/contracts/L1/L1StandardBridge.sol

+    ) internal {
+        emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data);
+        _initiateBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _minGasLimit, _data);
+    }


Is there any reason why the two functions above are not inlined within the external functions that call them?

packages/contracts/contracts/L2/L2StandardBridge.sol

packages/contracts/contracts/L2/L2ToL1MessagePasser.sol

packages/contracts/contracts/L1/OptimismPortal.sol

trianglesphere

I just flagged everywhere I was a gasLimit that was 256 bits, but didn't do as much in terms of determining if it was valid or not.

I also flagged where I had the concern around msg.value and _value on L2. I don't think this is fully relevant yet, but will be in the future.

Otherwise, this looks good to me.

trianglesphere · 2022-05-11T20:47:11Z

packages/contracts/contracts/L1/L2OutputOracle.sol

+    event l2OutputAppended(
+        bytes32 indexed _l2Output,
+        uint256 indexed _l1Timestamp,
+        uint256 indexed _l2timestamp


Should also use block number here. This is a larger refactor though.

We need consensus before making that large of a change, should put together a doc @trianglesphere and schedule discussion for next office hours @K-Ho

trianglesphere · 2022-05-11T20:47:57Z

packages/contracts/contracts/L1/OptimismPortal.sol

+        address _sender,
+        address _target,
+        uint256 _value,
+        uint256 _gasLimit,


Is this new code? This is the gas limit that should be limited to 64 bites

This is new code, good call

packages/contracts/contracts/L1/OptimismPortal.sol

trianglesphere · 2022-05-11T20:48:45Z

packages/contracts/contracts/L2/L2ToL1MessagePasser.sol

@@ -62,21 +72,20 @@ contract Withdrawer {
    function initiateWithdrawal(
        address _target,
        uint256 _gasLimit,


too large gas limit.

This is safe, downstream there is a require(gasleft() > _gasLimit + buffer)

I lean towards not changing any of the gas limits here as the only one that needs to be a uint64 is the one used for deposits. Unless there is strong preference for it to be a uint64 everywhere just for sanity reasons, which I would understand too

I think that it could be fine. I just flagged each instance without too much thought. It's just that any gas limit that serializes to go / actual execution needs to fit in 64 bits (and probably way fewer) otherwise the deposit gets stuck

Thoughts @smartcontracts?

trianglesphere · 2022-05-11T20:49:12Z

packages/contracts/contracts/libraries/Lib_CrossDomainHashing.sol

+        bool _isCreate,
+        uint256 _mint,
+        uint256 _value,
+        uint256 _gas,


too large? Maybe not actually?

trianglesphere · 2022-05-11T20:49:50Z

packages/contracts/contracts/libraries/Lib_CrossDomainHashing.sol

+        address _sender,
+        address _target,
+        uint256 _value,
+        uint256 _gasLimit,


trianglesphere · 2022-05-11T20:50:01Z

packages/contracts/contracts/libraries/Lib_WithdrawalVerifier.sol

        address _sender,
        address _target,
        uint256 _value,
        uint256 _gasLimit,


256 bit gas

trianglesphere · 2022-05-11T20:50:22Z

packages/contracts/contracts/test/CommonTest.t.sol

+        address sender,
+        bytes message,
+        uint256 messageNonce,
+        uint256 gasLimit


trianglesphere · 2022-05-11T20:50:28Z

packages/contracts/contracts/test/CommonTest.t.sol

+        address indexed sender,
+        address indexed target,
+        uint256 value,
+        uint256 gasLimit,


trianglesphere · 2022-05-11T20:53:44Z

packages/contracts/contracts/universal/CrossDomainMessenger.sol

+
+        if (_isSystemMessageSender()) {
+            // Should never happen.
+            require(msg.value == _value, "Mismatched message value.");


I have concerns about this when we are doing stuff on L2 with this. Specifically the L1 value is set to the L2 mint, and then a user specified (to the deposit tx at least) amount is set to the value field. If these fields do not line up exactly, we have have stuck messages. In particular, we expect one version of the Optimism Portal Deposit function to take eth to pay for L2 gas which would change the amount of value emitted.

fix: withdrawal time

smartcontracts force-pushed the sc/base-messenger branch 2 times, most recently from 7dcef83 to 73eef4e Compare May 9, 2022 21:31

tynes mentioned this pull request May 9, 2022

opnode: Withdrawal E2E test in go #423

Merged

tynes force-pushed the sc/base-messenger branch from 810db4d to 179f2f5 Compare May 11, 2022 03:13

tynes and others added 25 commits May 10, 2022 20:15

contracts: consolidate OptimismPortal

573bc0d

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: delete abstracts

f56aecb

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: base CrossDomainMessenger + L1 and L2

325f8d0

Implement the `L1CrossDomainMessenger` and `L2CrossDomainMessenger` based on the base `CrossDomainMessenger`. This makes the interfaces the same on both sides. Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: StandardBridge

5fbc866

Also implement the `L1StandardBridge` and `L2StandardBridge` based off of the base `StandardBridge` Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: L2OutputOracle

c79a6ae

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: standard bridge tests

06af146

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: cross domain hashing lib + tests

046931b

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: tests for cross domain messengers

0408613

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: L2ToL1MessagePasser

bfbf6f3

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: safe call lib

74673f7

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: optimism mintable erc20

f7f4ea8

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: common test setup

6491415

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: optimism mintable token factory

502f154

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update forge-std

a623981

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: test OptimismPortal

4dd57da

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: L1Block attributes

9d5409c

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update libraries

b5feec7

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update foundry.toml

8a355a0

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: Burner

0e19c25

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: delete dead code

0d42f3d

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update hh deploy scripts

173e961

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update gas snapshot

d16b922

Co-authored-by: smartcontracts <kelvin@optimism.io>

integration-tests: update for new messengers

249064d

Co-authored-by: smartcontracts <kelvin@optimism.io>

ops: devnet up script new genesis

c110309

Co-authored-by: smartcontracts <kelvin@optimism.io>

opnode: new bindings

5f8e468

Co-authored-by: smartcontracts <kelvin@optimism.io>

tynes and others added 2 commits May 10, 2022 20:18

l2os: new bindings

28359ec

Co-authored-by: smartcontracts <kelvin@optimism.io>

contracts: update gas snapshot

741cccc

Co-authored-by: smartcontracts <kelvin@optimism.io>

tynes force-pushed the sc/base-messenger branch from 179f2f5 to 741cccc Compare May 11, 2022 03:18

tynes changed the title ~~Sc/base messenger~~ refactor: new messengers May 11, 2022

tynes marked this pull request as ready for review May 11, 2022 03:25

specs: L2ToL1MessagePasser

aaf62b4

tynes self-requested a review May 11, 2022 04:04

tynes force-pushed the sc/base-messenger branch from 2242fdb to 43f201d Compare May 11, 2022 04:06

tynes added 2 commits May 10, 2022 21:19

specs: messengers

1535113

specs: bridges

03c8c86

tynes force-pushed the sc/base-messenger branch from 43f201d to 03c8c86 Compare May 11, 2022 04:19

specs: fix linting issues

d73feb7

This was referenced May 11, 2022

feat: wip gas metering #410

Closed

specs: add predeploys #379

Closed

norswap reviewed May 11, 2022

View reviewed changes

tynes reviewed May 11, 2022

View reviewed changes

packages/contracts/contracts/L2/L2ToL1MessagePasser.sol Show resolved Hide resolved

tynes reviewed May 11, 2022

View reviewed changes

packages/contracts/contracts/L1/OptimismPortal.sol Show resolved Hide resolved

trianglesphere approved these changes May 11, 2022

View reviewed changes

tynes added 6 commits May 11, 2022 13:54

contracts: better contract for is optimism mintable

230aff6

contracts: fix some comments

e2da9e2

contracts: assert finalization window has passed

97f10cc

bindings: regenerate

1449e24

contracts: address comments

5e37e47

Merge pull request #429 from ethereum-optimism/fix/withdrawal-time

756e2a5

fix: withdrawal time

mslipper merged commit 55db896 into main May 12, 2022

Conversation

smartcontracts commented May 6, 2022 • edited by tynes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

norswap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trianglesphere left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

smartcontracts commented May 6, 2022 •

edited by tynes

Loading

codecov-commenter commented May 6, 2022 •

edited

Loading