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.

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

feat: OVM #491

Merged
merged 58 commits into from
Jun 11, 2021
Merged

feat: OVM #491

merged 58 commits into from
Jun 11, 2021

Conversation

gakonst
Copy link
Collaborator

@gakonst gakonst commented May 21, 2021

Summary

The OVM is a sandboxed environment for the EVM which allows execution of smart contract code on L2, with the ability to dispute it on L1 via a fraud proof. Solidity code can be compiled directly to OVM code instead of EVM code by using "optimistic solc", a fork of the well-known solc. The OVM bytecode is ~30% larger than EVM bytecode, which may result in the bytecode being larger than the 24kb size limit for smart contracts.

This PR refactors the codebase to allow it to fit in the 24kb limit, and does a minimal diff to the test suite so that tests can pass when ran against an instance of go-ethereum running the OVM. I suspect the test suite changes around waiting for txs to be confirmed would've been required anyway if we wanted to test against geth or any full node impl.

Explanation of code changes

  • Introduces a new CI job for the OVM unit tests
  • Modifies the hardhat config in the following ways:
    • Overrides the default signer and contracts to always .wait(). This is because tests are run against go-ethereum which does not insta-mine, hence receipts are not immediately available in tests.
    • Overrides the default waffle getWallets() method because it fails if the network name is not hardhat
    • Makes Waffle fixtures a no-op. This is because go-ethereum does not support snapshots
    • Makes the default ethers provider polling interval 100ms. This is because when blocks don't get instamined, Ethers by default waits 4s while polling
    • Add a new network called optimism
    • Increase the mocha timeout, given that there's no fixtures to speed tests up
    • Decreases the optimizer runs to keep bytecode size low
    • Adds a new plugin, hardhat-ovm which allows compiling to the OVM when the target network has the ovm: true flag set
  • Adds a scripts/run-optimism.sh script which will clone the Optimism monorepo, build it, and bring up the network
  • Replaces the minimal proxy which contains banned opcodes with a normal delegateproxy contract
  • Refactors libraries with internal functions to public, so that they get deployed as separate contracts and do not contribute to the bytecode of the "main" contract. Certain functions were also batched together to minimize the number of delegatecalls needed (e.g. updateBoth).

Status

All tests pass both in the EVM and in the OVM! Due to the lack of snapshots and running against a real node, tests take much longer, ~90mins.

The linter fails due to perms, but I formatted everything in the last commit.

h/t to @mds1 and @elenadimitrova for the help :)

gakonst and others added 30 commits May 19, 2021 14:10
This reverts commit 7a4e277.

-> Temporary revert. TODO: Load wallets conditionally on `--network optimism`
Waffle event emitter seems to be having problems with Optimism
gakonst added 4 commits May 20, 2021 17:53
The try/catch pattern does not yield the revert msg when used with geth. We would need to use the `expect(tx).to.be.revertedWith` pattern
@gakonst gakonst force-pushed the get-ovm-contracts-deployed branch from 9a9d213 to e5c8912 Compare May 21, 2021 07:30
@moodysalem moodysalem changed the base branch from main to optimism May 25, 2021 18:03
Copy link
Contributor

@moodysalem moodysalem left a comment

Choose a reason for hiding this comment

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

can we do something about the modifiers -> functions change?

.github/workflows/tests.yml Outdated Show resolved Hide resolved
run: yarn optimism-up

- name: Run unit tests
run: UPDATE_SNAPSHOT=1 yarn test:ovm
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

@gakonst gakonst Jun 9, 2021

Choose a reason for hiding this comment

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

We'd need to generate 2 different snapshots since gas costs differ between EVM and OVM (due to including L1 data publishing costs). I don't think the snapshot plugin supports that right now. I changed this to test the snapshot for the EVM tests however, to ensure that the baseline performance is ~same as the one we have in main.

contracts/UniswapV3Pool.sol Outdated Show resolved Hide resolved
@NoahZinsmeister
Copy link
Contributor

did a first pass, nothing jumped out to me as a big problem, though i echo what moody already pointed out

@gakonst gakonst requested a review from moodysalem June 9, 2021 16:03
@@ -256,3 +257,12 @@ export function createMultiPoolFunctions({
swapForExact1Multi,
}
}

/**
* @notice The OVM emits an additional `Transfer` event due to its usage of WETH instead of native ETH. As a result,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this transfer the payment of gas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

@moodysalem moodysalem merged commit 6fab61b into Uniswap:optimism Jun 11, 2021
@gakonst gakonst deleted the get-ovm-contracts-deployed branch June 12, 2021 08:39
@mds1 mds1 restored the get-ovm-contracts-deployed branch June 15, 2021 12:05
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.

5 participants