-
Notifications
You must be signed in to change notification settings - Fork 111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks dope! Left a few comments/nits but overall imo we're verrrry close to a merge!
contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_L1ERC20Gateway.sol
Outdated
Show resolved
Hide resolved
} | ||
|
||
receive() external payable { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this receive(..)
function supposed to be empty?
contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_L2DepositedERC20.sol
Outdated
Show resolved
Hide resolved
sendCrossDomainMessage( | ||
address(l1ERC20Gateway), | ||
data, | ||
8999999 // TODO: meter and set with some buffer (actually I think its currently unused in this direction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to inherit this value from the EM. And maybe do something like ExecutionManager.GAS_LIMIT - 1
or something
contracts/optimistic-ethereum/iOVM/bridge/tokens/iOVM_L2DepositedERC20.sol
Show resolved
Hide resolved
18, | ||
'ETH', | ||
'0x4200000000000000000000000000000000000007', | ||
'0x0000000000000000000000000000000000000000', // will be overridden by geth when state dump is ingested. Storage key: 0x0000000000000000000000000000000000000000000000000000000000000008 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely make a single source of truth for all of these pre-deploy addresses. If we are already hardcoding the addresses then I don't think we need to change it for this PR, but we should definitely at least add an issue for cleaning this up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double LGTM
* initial * get .calls test working * add assertions to withdrawal tests * add missing finalizeDeposit tests * clean up tests * clean up crosschain library * further clean up tests * clean up L1 Gateway, fix bug * further clean up contracts * quick save * quick save * Add unsupported to fix compiler bug, need to remove this * fix constructor inputs * build fixes * improve test case names * l1 gateway testing WIP * add comment on init * Add withdraw tests * temp only * All tests implemented, 2 still failing, needs polish * All tests implemented, 2 still failing, needs polish * add failing test for finalizeWithdrawAndCall * Add univ2erc20, broken build tho * get contracts building * clean up tests and interface * updates * move to uniswap ERC20 * more updates, fix gateway tests with uniswap * finish deploy config? * Add ETH deposit gateway (OVM_L1ETHGateway) (#225) * OVM_L1ETHGateway almost working * OVM_L1ETHGateway tests all passing * clean up unused stuff * lower gaslimit * lower gas further * typo * break out helper * resolve messenger * update deploy config * renaming * lint * update tests to use AM * restructure fs layout * clean unused envar * remove interface version bound * remove todo * various minor cleanups * add safemath to contract account * update naming conventions * fix test config and test name * cleanup for consistency * fix eth send test * remove .only * clean up, add deposit gas limit * lint * add gas config and tests * fix indent Co-authored-by: ben <ben@bens-MacBook-Pro.local> Co-authored-by: Maurelian <maurelian@protonmail.ch> Co-authored-by: Matt Masurka <m.masurka@gmail.com>
Description
This draft PR builds on #219, implementing the gateway contracts and extending them for use with ETH in particular.
Note: We do have plans to add a
withdrawAndCall
function, but probably gonna merge this in without before doing that.Questions
Metadata
Fixes
Contributing Agreement