Extensible deposit withdraw #254
Conversation
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_L1TokenGateway.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_L2DepositedERC20.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_L2DepositedERC20.sol
Outdated
Show resolved
Hide resolved
| * @param _to Account to give the deposit to on L2 | ||
| * @param _amount Amount of the ERC20 to deposit. | ||
| * @param _from L1 address ETH is being deposited from | ||
| * @param _to L2 address that the ETH is being deposited to |
There was a problem hiding this comment.
Nit... these param descriptions make me wonder if we should change the arguments to something like:
_l1Sender, _l2Recipient. Might be a useful cue to devs to help them follow the logic.
contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_L1ETHGateway.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/iOVM/bridge/tokens/iOVM_L1ETHGateway.sol
Outdated
Show resolved
Hide resolved
| /******************* | ||
| * Contract Events * | ||
| *******************/ | ||
| contract OVM_L2DepositedERC20 is Abs_L2DepositedERC20, UniswapV2ERC20 { |
There was a problem hiding this comment.
Maybe we should rename UniswapV2ERC20 to ERC20? We can recognize them in the comments on the file header, but it feels a bit odd to see the project explicitly called out here.
There was a problem hiding this comment.
Maybe PermitERC20/PermissibleERC20?
contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_L2DepositedERC20.sol
Outdated
Show resolved
Hide resolved
ben-chain
changed the title
…m-optimism/contracts into feat/extensible-deposit-withdraw
smartcontracts
left a comment
smartcontracts
left a comment
There was a problem hiding this comment.
LGTM, could use some cleanup (extra whitespace in some places) but logic looks good to me.
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_L1TokenGateway.sol
Outdated
Show resolved
Hide resolved
|
|
||
| // Construct calldata for l2DepositedToken.finalizeDeposit(_to, _amount) | ||
| bytes memory data = abi.encodeWithSelector( | ||
| iOVM_L2DepositedToken.finalizeDeposit.selector, |
There was a problem hiding this comment.
Whoa! When did Solidity add this?
There was a problem hiding this comment.
I think it's been there for a bit, but it still lets you pass in the wrong argument types sooo meh lol
| ) | ||
| external | ||
| override | ||
| onlyFromCrossDomainAccount(l2DepositedToken) |
There was a problem hiding this comment.
Maybe worth asking around about the naming here.
There was a problem hiding this comment.
Gonna merge since this is a bit time-sensitive, and this is not a change from the previous PR, but let's def revisit this.
Description
Reworks the contracts to allow for constructing new bridges via inheritable hooks. This will allow folks to easily use their desired ERC20 implementations.
Contributing Agreement