Transition contracts to use buidler by smartcontracts · Pull Request #170 · ethereum-optimism/optimism

smartcontracts · 2020-06-30T21:51:57Z

Description

Builds upon #166.

This PR transitions our contracts to use buidler. I didn't want to clutter this PR too much, so haven't cleaned up the tests. Tests are passing locally but I'll make sure its working well in CI. No significant changes to the way that contracts are compiled or deployed, we just have a cleaner API now.

Going to retroactively make a ticket for this since it's become its own task.

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

…po into YAS-467/rollup-contracts/refactor

smartcontracts · 2020-06-30T21:57:04Z

Whoops, forgot to add the lint script back in.

smartcontracts · 2020-06-30T22:40:25Z

Likely difficult to review until #166 is merged

smartcontracts · 2020-07-01T20:45:08Z

Ok, this now includes the changes already merged to master via #166

willmeister

LGTM 👍

willmeister · 2020-07-01T21:22:08Z

packages/contracts/buidler-env.d.ts

+/// <reference types="@nomiclabs/buidler-ethers/src/type-extensions" />
+/// <reference types="@nomiclabs/buidler-waffle/src/type-extensions" />


Just want to make sure these are supposed to be commented out

Yeah it's just the weird way that typescript has you explicitly identify typing files (https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html).

karlfloersch

I glanced over everything and overall it looks good! Had a comment here and there but for the most part everything looks in order! I won't say that I really digested every line because most of it just looked like refactoring, so assuming all the old tests are still running (which it seemed to be the case) it LGTM!!!!

packages/contracts/buidler-env.d.ts

karlfloersch · 2020-07-01T21:08:25Z

packages/contracts/config/.env.example

-DEPLOY_NETWORK='local'
-
-# Only if deploying locally
-DEPLOY_LOCAL_URL='http://127.0.0.1:8545'


Did this config move to another place? Or is it just not required anymore?

Ah I'm going to rewrite the deployment scripts since we've had some API changes in the contracts. So I'll include this as part of a new PR.

packages/contracts/deploy/rollup-chain.ts

karlfloersch · 2020-07-01T21:18:16Z

packages/contracts/plugins/hijack-compiler.ts

@@ -0,0 +1,34 @@
+import * as path from 'path'


This is dope!!!!!! I'm excited to see if we can use buidler for our fullnode tests!

Should this live in this package? Would we ever use this plugin in the contracts package? My guess would be that if we do any transpilation in the contract unit tests, that we would only be transpiling directly & not hook into the low level of buidler. Does that vibe with your intuitions?

Talked about it in a different forum -- Kelvin suggested / I think the move is to keep the plugin here for now, and then in a new PR move it into it's own package similar to how we have the truffle plugins in a different package -- https://github.com/ethereum-optimism/optimism-monorepo/tree/9b94f0d317b719d2f6b7b48a2eae0bc9c69c5307/packages/ovm-truffle-provider-wrapper

Hey 👋 I'm one of the authors of Buidler, and just happened to found this browsing github. I'm really curious about this override. Do you do this to run solc nightlies, or do you use your own version/fork of solc? Do you have any idea about what would make this easier to achieve? Thanks!

Hey @alcuadrado! We currently make use of a custom solidity compiler (translating certain L1-related op codes into operations that can be executed on L2). Since buidler only allows version strings by default, we had to make this slight modification to allow paths to custom solc wrappers. I think it'd be great to allow for path/custom config variables within the solc config.

If this isn't something you can/want to support natively, an easier change could be to split the steps where the compiler is initialized and the files are compiled into two internal tasks. That'd allow us to simply hook into the compilation step like a normal plugin (instead of overriding the task entirely).

Thanks for the feedback, @kfichter! We are currently going through an overhaul of the compilation pipeline, so we'll take your ideas into account. If you want to follow it, or propose any other thing, here's the issue track ihttps://github.com/NomicFoundation/hardhat/issues/553

karlfloersch · 2020-07-01T21:28:32Z

packages/contracts/test/contracts/ovm/execution-manager/ExecutionManager.l1-l2-opcodes.spec.ts

-        ZERO_ADDRESS,
-        true,
-      ])
+      const data = executionManager.interface.encodeFunctionData(


Am I correct that there's a "better" way to do this in Ethers 5? I vaguely remember that being the case

Ah yeah I think this was it -- ethers-io/ethers.js#535 (comment)

karlfloersch · 2020-07-01T21:35:08Z

...ages/contracts/test/contracts/ovm/execution-manager/ExecutionManager.purity-checking.spec.ts

@@ -1,10 +1,10 @@
 import '../../../setup'


Just noticed (unrelated to the PR but relevant) -- the filename uses the out of date "purity checking" term and should be changed to "safety checking"

Ah yeah I'll change that in the next PR.

karlfloersch · 2020-07-01T21:36:23Z

packages/contracts/test/contracts/utils/ContractAddressGenerator.spec.ts

      const nonce = 1
      const expectedAddress = utils.getContractAddress({
-        from: wallet1.address,
+        from: await wallet1.getAddress(),


woah interesting that the get address is async! is that a buidler thing?

It's a little weird. Buidler uses Signer accounts by default. I ended up having to sort of reimplement the getWallets() behavior anyway, so I may switch everything back to use Wallet instances later on.

* feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr

* feat: add shared lockbox (#126) * feat: create shared lockbox contract with its interface and unit tests * chore: polish tests and interfaces * chore: run pre-pr * chore: improve natspec * chore: run pre-pr * chore: update compiler version * feat: integrate portal to lockbox (#139) * feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert * feat: add liquidity migrator contract with its unit test and interface (#128) * feat: create shared lockbox contract with its interface and unit tests * chore: polish tests and interfaces * chore: run pre-pr * chore: improve natspec * chore: run pre-pr * feat: add liqudity migrator contract with its unit test and interface * chore: remove underscore on stack var * chore: add todo * chore: run pre-pr * chore: add contract title natspec and proxied * refactor: integrate testing suite with common test * chore: pre-pr * chore: add spec test * feat: integrate system config with superchain config (#140) * feat: integrate portal to lockbox * fix: pr fixes * test: refactor assert * feat: integrate system config with superchain config * fix: remove OPCM interop * test: add dependency counter test * feat: manage dependency set on superchain config (#138) * chore: add zero dependencies check (#142) * fix: pre pr * feat: Add pause check (#145) * feat: Add pause check Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks> * test: add tests natspecs --------- Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks> * fix: pre pr and interfaces imports * feat: add upgrader role to superchain config (#163) * feat: use superchain config lockbox in portal (#164) * feat: use superchain config lockbox in portal * test: add new sharedlockbox test * fix: pre pr * feat: liquidity migrator deployment (#166) * feat: liquidity migrator deployment * test: fix comment * test: fix internal variables names * feat: dependency set refactor (#170) * feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr * fix: pre pr * fix: semgrep * fix: merge conflict * [WIP] feat: new lockbox (#192) * chore: partial implementation comments * feat: new lockbox * feat: introduce dependency manager predeploy * feat: remove timestamp check from CrossL2Inbox * feat: introduce cluster manager role and remove immutables * fix: remove unnecessary code, fix tests and setup * feat: use unstructured storage and OZ v5 * fix: L2ToL2CDM dependency set check * fix: dependency manager gas limit * feat: refactor interop feature contracts (#200) * feat: refactor interop feature contracts * fix: add noops comment * feat: adds OptimismPortal migrated flag * test: add missing tests * fix: portal interop storage naming --------- Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com> * fix: pre pr, setup and tests * fix: remove system config interop and add interop portal target check (#205) * fix: remove system config interop and add interop portal check * fix: interop portal target check order * fix: remove wrong comment * fix: refactor portal noops function (#206) * test: add dependency manager and portal interop tests (#209) * feat: initialize shared lockbox in interop portal (#211) * feat: initialize shared lockbox in interop portal * fix: refactor shared lockbox storage getter * fix: lockbox pr fixes (#214) * fix: pre pr * fix: semver lock * fix: semver lock * feat: descope dependency manager (#242) * feat: descope dependency manager * test: fix tests * test: fix tests * chore: improve eth liquidity test (#248) * fix: internal review fixes (#243) * fix: I-0 * fix: I-1 * fix: I-2 * fix: I-3 * fix: I-6 * fix: I-7 * fix: I-9 * fix: pre pr * fix: pre pr * fix: portal withdrawal checks (#255) * fix: portal withdrawal checks * fix: include current withdrawal check * fix: remove unused interop contracts (#256) * test: fix flake tests (#257) * fix: adjust op-deployer interop scripts (#262) * fix: pre pr * fix op-deployer tests * remove dependency code * fix lint * use Bob account --------- Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Co-authored-by: AgusDuha <81362284+agusduha@users.noreply.github.com> Co-authored-by: agusduha <agusnduha@gmail.com> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Joxess <joxes@defi.sucks> Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com>

* add semgrep * minor * count time * lint check * print time even fail * dedup test * env checks * avoid duplicated build of contracts

instead of copying the entire buffer, de-reference twice to make a new mutable reference of the input u8 array

…thereum-optimism#170

## Overview Adjusts the `RollupConfig` type to backwards-activate forks. Right now, if partially constructed with only a later fork timestamp set, prior forks aren't activated. This causes inconsistency in testing in `kona` in a way that's easy to miss.

Implements end to end tests for proof collection. These tests create a blockchain, run the backfill job, then run the live collector. The live collector executes on top of the backfilled state and it already checks that the state root matches, so the tests just ensure that it runs without errors. Fixes #170 --------- Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>

smartcontracts added 17 commits June 24, 2020 14:16

Moved all OVM files and deleted the package

207cbe8

Started cleaning contracts

358ea4c

Quick contract cleanup

922536a

Added cleaned contracts

073448a

Fixed merge conflicts

64b7000

Fixed package.json

ba1d7bc

Removed /ovm requirements

16b9374

Fixed circular dependencies

30e6b2e

Fixed various linting errors

3e1801b

Fixed yet another ganache-core issue

fc409d8

Fixed bug causing tests to fail

41bc77c

Fixed bug causing tests to fail

3baed52

Merged master

64c7f01

Merge branch 'master' of github.com:ethereum-optimism/optimism-monore…

78ce1bc

…po into YAS-467/rollup-contracts/refactor

Added older RLPEncode file again

4593033

Renamed rollup-contracts => contracts

5840831

Transitioned contracts to use buidler

45e94f4

Linted and added better export method

470fbe1

smartcontracts added 2 commits June 30, 2020 19:50

Fixed a typo

a2b9b85

Merged master

9b94f0d

willmeister approved these changes Jul 1, 2020

View reviewed changes

karlfloersch approved these changes Jul 1, 2020

View reviewed changes

smartcontracts merged commit 6713282 into master Jul 1, 2020

gakonst deleted the YAS-467/rollup-contracts/audit branch March 18, 2021 15:09

snario pushed a commit that referenced this pull request Apr 14, 2021

version: bump to 0.1.0 (#170)

59111a5

OptimismBot pushed a commit that referenced this pull request Jan 16, 2025

feat: dependency set refactor (#170)

4183809

* feat: dependency set refactor * fix: deploy script variable name * fix: pr * fix: pr

theochap pushed a commit that referenced this pull request Dec 10, 2025

fix(derive): fix span batch utils read_tx_data() (#170)

bd7ac13

instead of copying the entire buffer, de-reference twice to make a new mutable reference of the input u8 array

Zena-park added a commit to tokamak-network/optimism that referenced this pull request Dec 30, 2025

[ACM-01C] Inefficient Application of Modifiers ethereum-optimism#169 e…

d1f2157

…thereum-optimism#170

		/// <reference types="@nomiclabs/buidler-ethers/src/type-extensions" />
		/// <reference types="@nomiclabs/buidler-waffle/src/type-extensions" /> No newline at end of file

Conversation

smartcontracts commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributing Agreement

Uh oh!

smartcontracts commented Jun 30, 2020

Uh oh!

smartcontracts commented Jun 30, 2020

Uh oh!

smartcontracts commented Jul 1, 2020

Uh oh!

willmeister left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karlfloersch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smartcontracts commented Jun 30, 2020 •

edited

Loading