Skip to content

Conversation

@krebernisak
Copy link
Contributor

@krebernisak krebernisak commented Apr 6, 2021

  • Add OVM compilation target through Hardhat Optimism plugin
  • Add @chainlink/optimism-utils a set of useful functions to interact with Optimism network
  • Add L1 & L2 contracts required for the Optimism token bridge (solc v0.7)
  • Add LinkTokenChild contract, an access-controlled mintable & burnable LinkToken, for use on sidechains and L2 networks
  • Add Optimism integration test
    • L1 -> L2 -> L1 deposit-wihdraw integration test
    • L2 withdrawals safety check integration test (blocking contract withdrawals but allowing EOA contracts to withdraw)
  • Moved CI from Travis to GitHub Actions
    • Add CI integration tests running L1 & L2 networks (optimism-integration repo)
  • Add CHANGELOG information
  • Expose methods like deployGateways to deploy the L1<->L2 bridge that consumers might find useful.
  • Add @openzeppelin/contracts-upgradeable, the upgradeable variant of OpenZeppelin Contracts, meant for use in upgradeable contracts.

TODO:

  • Decide should we deploy the bridge behind a proxy?
    • Optimism team suggests that possibly the bridge interface could slightly change in the coming weeks, so a good idea would be to use a proxy to be able to update with the latest ABI changes.
    • Optimism is deploying the ETH bridge behind a proxy.
    • Synthetix has migrate functionality for their L1 & L2 gateways, while they also plan to add proxy deployment for the bridge.
  • Split OVM_L2DepositedLinkToken.sol into a proxied OVM_L2ERC20Gateway.sol and a non-proxied LinkTokenChild.sol token contract
  • Make L1/L2 Gateways an ERC677Receiver so transferAndCall can be used to avoid approving tokens before deposit/withdraw
  • Add proxy deployment for the bridge contracts (L1 & L2 gateways)
  • Remove OpUnsafe helper contract when/if upstream deposit/depositTo & withdraw/withdrawTo are made virtual (PR #31)
  • Document the reasoning behind "unsafe" bridge operations
  • Document upgradebility and bridge extension scenarios
  • Get feedback
  • Polish & finalize contracts
  • Audit & approve

@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch 5 times, most recently from 5f686cf to 5e69a49 Compare April 7, 2021 09:49
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch 2 times, most recently from f338602 to 371770a Compare April 7, 2021 10:06
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from 371770a to a6cadd5 Compare April 7, 2021 10:27
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch 4 times, most recently from 32dba9a to 83054d9 Compare April 7, 2021 13:23
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from 83054d9 to 48a0863 Compare April 7, 2021 13:25
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from b865a95 to 188a322 Compare April 12, 2021 19:34
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from f94550b to b063cb4 Compare April 16, 2021 13:18
onlyInitialized()
{
// Unless explicitly unsafe op, stop deposits to contracts (avoid accidentally lost tokens)
require(_isUnsafe() || !Address.isContract(_to), "Unsafe deposit to contract");
Copy link

@aalu1418 aalu1418 Apr 16, 2021

Choose a reason for hiding this comment

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

I'm a bit confused by how this line functions. Since this contract exists on L1, but the _to address is pointing at an L2 address. Can Address.isContract(_to) check if it's a contract on L2 from L1?
(this might have to do with my lack of understanding of how Optimism fully works...)

Also shows up in the L2 contract. so also wondering the flip of the statement

Choose a reason for hiding this comment

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

Wait...I think I figured it out. Correct me if I am wrong:

  • Since OVM is a roll up solution where it is completely dependent on L1, the addresses line up directly with what is seen on L1
  • So if the address on L1 is a smart contract, funds sent to that address on L2 would only be accessible if the smart contract also had integrated with OVM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The questions are great! :)

This specific line is a little bit confusing, mostly because we could not extend deposit/depositTo & withdraw/withdrawTo (they are not virtual). So I added an unsafe marker to the third function depositToUnsafe/withdrawToUnsafe to be able to set up this check.

We want to block the bridge transfers to (local) contracts, unless explicit.

This is because, when sending to a contract it's easy to get tokens stuck, as you need to have a contract be available at the same address on L2. The "only way" to have the same contract address on L1 & L2 is to use CREATE with the same nonce. The CREATE2 opcode will not work, as the OVM compiled bytecode will be different than EVM (for contracts of any relevant complexity), so they will get different addresses on L1 & L2 networks.

This actually happened to a Synthetix user with an Argent SC wallet, that used the bridge from his smart wallet and locked his funds. The wallet sent to itself on L2, and there is no way (except luck) to recreate the same address on L2.

Copy link

@aalu1418 aalu1418 Apr 20, 2021

Choose a reason for hiding this comment

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

That's a great explanation! And yes blocking token transfers to contracts makes sense.

If I understand your explanation correctly, then on L1 there could be a smart contract deployed at a certain address, but not on L2 (and vice versa).

So I think I still have my original question: how can a contract deployed on L1 as the gateway detect that an address on L2 is a smart contract or not? it seems like !Address.isContract(_to) would only check on L1... (or on the layer it is deployed on)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we only check for local contracts, L1 gateway checks for L1 contracts, and similarly on L2.

If an L1 gateway receives a transfer request to L2 address, it checks if that address is a contract on L1. If it is, the gateway makes sure the user is explicit in its intention. If not, the gateway rejects to avoid accidental mistakes, as the contract address space, at least for the CREATE2 opcode, is vastly different for the two networks.

Choose a reason for hiding this comment

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

Cool, that makes sense! thanks :)

Copy link

@aalu1418 aalu1418 left a comment

Choose a reason for hiding this comment

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

Overall, the logic is looking pretty good! Left a few comments, but most of it is me trying to understand how Optimism works with L1 (Eth) at the smart contract level. Feel free to correct me if you think I am misunderstanding :)

I have only taken a look at the bridge contracts so far. I will also take a look at the other components (mocks, scripts, etc) at another point. (lots to go through!)

@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from b063cb4 to 556d5cb Compare April 18, 2021 10:17
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from 556d5cb to aef60f6 Compare April 18, 2021 10:39
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from af21a76 to add2c60 Compare April 18, 2021 11:16
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch 3 times, most recently from 90c6a28 to 486f675 Compare April 18, 2021 17:09
@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from 486f675 to 0a984b2 Compare April 18, 2021 18:33
* As the OVM_ProxyEOA.sol contract source could potentially change in the future (i.e., due to a fork),
* here we actually track a set of possible EOA proxy contracts.
*/
abstract contract OVM_EOACodeHashSet is ConfirmedOwner {

Choose a reason for hiding this comment

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

This feels like something we should pull into our repo and give you a nice library for :)

@krebernisak krebernisak force-pushed the feature/ovm-integration-bridge-hardhat branch from 93152a0 to d874349 Compare April 26, 2021 09:23
@krebernisak
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants