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.

Sign up for GitHub

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

Jump to bottom

Bridging between L1 ETH and L2 ERC20 BridgedETH #73

Merged
merged 15 commits into from
Nov 29, 2023
Merged

Bridging between L1 ETH and L2 ERC20 BridgedETH #73

merged 15 commits into from
Nov 29, 2023

Conversation

karlb
Copy link

@karlb karlb commented Oct 25, 2023 •
edited
Loading

  • Add a basic ERC20 contract on L2 which is used to stored bridged ETH. This can be extended to be a fee currency later.
  • Configure bridging between L1 ETH and L2 bridged ETH
  • Add helper code to prove and finalize L2->L1 withdrawals in tests
  • Test bridging in both directions

Notes:

  • Before Bedrock, Optimism also used an ERC20 token to store ETH on L2. But since a lot changed during Bedrock, I didn't try to revive the old version and modified the new one, instead.
  • It is not totally clear whether the ETH should be held in the L1StandardBridge contract or OptimisimPortal on L1. I went with the former, since the it seems to be the expectation that all ETH send to OptimisimPortal should come out as native currency on L2.
  • Any good suggestions how to make sure I didn't miss a way to trigger the old native<->native bridging?

Closes #63

@socket-security
Copy link

socket-security bot commented Oct 25, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@ethersproject/keccak256 5.7.0 None +3 209 kB ricmoo
@ethersproject/rlp 5.7.0 None +2 180 kB ricmoo
@ethersproject/constants 5.7.0 None +4 411 kB ricmoo
@ethersproject/address 5.7.0 None +7 514 kB ricmoo
@ethersproject/web 5.7.1 network +8 700 kB ricmoo
@ethersproject/abstract-signer 5.7.0 None +50 3.2 MB ricmoo
@ethersproject/transactions 5.7.0 None +19 1.19 MB ricmoo
@ethersproject/properties 5.7.0 None +1 100 kB ricmoo
@ethersproject/bignumber 5.7.0 None +3 392 kB ricmoo
@ethersproject/contracts 5.7.0 None +28 2.64 MB ricmoo
@ethersproject/abstract-provider 5.7.0 None +96 6.05 MB ricmoo
@ethersproject/abi 5.7.0 None +27 2.39 MB ricmoo
@ethersproject/bytes 5.7.0 None +1 150 kB ricmoo
@eth-optimism/sdk 0.0.0-develop-20230928190155 network +85 455 MB optibot
@eth-optimism/contracts 0.6.0 network +53 21 MB optibot
chai 4.3.10 environment +18 1.08 MB keithamus
ethers 5.7.2 network +220 47.7 MB ricmoo

@karlb karlb force-pushed the karlb/bridge-eth branch 2 times, most recently from 9f32f03 to 43d175b Compare November 15, 2023 13:15
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023 •
edited
Loading

Codecov Report

Merging #73 (73822e4) into celo3 (cb05d2b) will increase coverage by 0.00%.
The diff coverage is 91.66%.

Additional details and impacted files 
@@           Coverage Diff           @@
##            celo3      #73   +/-   ##
=======================================
  Coverage   47.93%   47.94%           
=======================================
  Files         178      178           
  Lines        6794     6799    +5     
  Branches     1097     1098    +1     
=======================================
+ Hits         3257     3260    +3     
- Misses       3417     3418    +1     
- Partials      120      121    +1
Flag Coverage Δ
cannon-go-tests 63.48% <ø> (ø)
chain-mon-tests 26.95% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests 45.16% <91.66%> (+0.02%) ⬆️
contracts-ts-tests 100.00% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 41.95% <ø> (ø)
sdk-tests 41.95% <ø> (ø)

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

Files Coverage Δ
...ages/contracts-bedrock/src/L1/L1StandardBridge.sol 100.00% <100.00%> (ø)
...ages/contracts-bedrock/src/L2/L2StandardBridge.sol 84.21% <100.00%> (ø)
...contracts-bedrock/src/universal/StandardBridge.sol 83.72% <87.50%> (-0.49%) ⬇️

... and 1 file with indirect coverage changes

@karlb karlb changed the base branch from celo2 to karlb/celo3 November 21, 2023 16:33
gastonponti
gastonponti reviewed Nov 23, 2023
View reviewed changes
Copy link

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

Looks good. Just comments for Constants usage

packages/contracts-bedrock/src/L1/L1StandardBridge.sol Outdated
@@ -211,7 +211,9 @@ contract L1StandardBridge is StandardBridge, ISemver {
/// @param _minGasLimit Minimum gas limit for the deposit message on L2.
/// @param _extraData Optional data to forward to L2.
function _initiateETHDeposit(address _from, address _to, uint32 _minGasLimit, bytes memory _extraData) internal {
_initiateBridgeETH(_from, _to, msg.value, _minGasLimit, _extraData);
_initiateBridgeETHToERC20(
0x4200000000000000000000000000000000001023, _from, _to, msg.value, _minGasLimit, _extraData
Copy link

Choose a reason for hiding this comment

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

Maybe set this as a constant with a name? But I don't totally know if that could cost or not more gas to be honest

Copy link
Author

Choose a reason for hiding this comment

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

Ahh yes. When I started out I was not sure were to put the named constant, and later I just forgot. Fixed now.

packages/contracts-bedrock/src/L2/L2StandardBridge.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/libraries/Predeploys.sol Show resolved Hide resolved
packages/contracts-bedrock/src/universal/StandardBridge.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/universal/StandardBridge.sol Outdated Show resolved Hide resolved
@karlb karlb changed the base branch from karlb/celo3 to celo3 November 24, 2023 13:14
@karlb
Copy link
Author

karlb commented Nov 24, 2023

There's on test waiting to be updated, but I'll do that in a second PR if this one get's ready to merge quickly.

@karlb karlb marked this pull request as ready for review November 24, 2023 15:36
@karlb karlb force-pushed the karlb/bridge-eth branch 2 times, most recently from cd11911 to 1898800 Compare November 28, 2023 09:05
karlb added 8 commits November 28, 2023 12:04
@karlb
Configure L1->L2 ETH bridging
b23601f
@karlb
Configure L2->L1 ETH bridging
954bbde
@karlb
Change bridgeETH(To) calls to ETHToERC20 bridging
29f1ad6
@karlb
Update bindings
9a87512
@karlb
Test bridging
554c2b3
@karlb
Test runner improvements
a8eacaa 
* Fix terminal colors during test runs
  `tput init` does not properly reset colors in all terminals.
* Allow running tests matching a certain pattern
@karlb
Fix tests after bridging changes
2d70154
@karlb
Fix linting (should be done on upstream)
3cd0386 
I am not sure why these linting errors did not prevent merging on
upstream, as I didn't touch these files. I assume that we can drop this
commit after an upstream rebase.
@karlb
circleci: use xlarge instance for bedrock-checks
73822e4 
I measured the required mem usage and it is 8545MB on CircleCI at the
moment. I'm not sure why upstream needs so much less.

The high memory usage might be caused by foundry-rs/foundry#6411
@karlb karlb merged commit 86a037b into celo3 Nov 29, 2023
47 checks passed
@karlb karlb deleted the karlb/bridge-eth branch November 29, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastonponti gastonponti gastonponti left review comments

@carterqw2 carterqw2 carterqw2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bridge ETH to L2
4 participants
@karlb @codecov-commenter @gastonponti @carterqw2