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: Add hardhat deployment infrastructure #2950

Merged
merged 50 commits into from
May 10, 2021
Merged

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented May 6, 2021

Motivation

Add example deployment, etherscan-verification, and post-deployment setup sprints, all using the native hardhat tools as much as possible!

Summary

Follow up features:

  • Figure out best way to export deployed addresses, bytecode, and ABI similar to core/networks/X.json.
  • Take full advantage of hardhat-deploy tags to make deploying different systems (test, prod, sink-oracle, etc.) possible in one line CLI commands.
  • Make verification work on Matic, might require some custom logic because Matic has an Etherscan substitute. We can hopefully use sourcify to verify programatically on Matic.
  • Merge core/hardhat.config.js with common/HardhatConfig.js which generally should serve the same role. It would be great if we could share HardhatConfig.js similar to how we use TruffleConfig.js across packages.
  • Use deployments in tests.

Details

NOTE FOR REVIEWERS:

  • The entrypoint for this PR is in README.md and package.json to see the new CLI scripts added.

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

Issue(s)

Fixes #XXXX

nicholaspai and others added 29 commits April 26, 2021 11:00
Signed-off-by: Nick Pai <[email protected]>
Signed-off-by: Nick Pai <[email protected]>
Signed-off-by: Nick Pai <[email protected]>
Signed-off-by: Nick Pai <[email protected]>
Signed-off-by: Nick Pai <[email protected]>
Signed-off-by: Nick Pai <[email protected]>
@nicholaspai nicholaspai changed the title WIP: Add hardhat deployment infrastructure feat: Add hardhat deployment infrastructure May 7, 2021
@nicholaspai nicholaspai requested a review from mrice32 May 7, 2021 21:16
Signed-off-by: Nick Pai <[email protected]>
@@ -0,0 +1,20 @@
const { hexlify, toUtf8Bytes } = require("ethers/utils");

function stringToBytes32(text) {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, is this copied from somewhere? I feel like I recognize the code

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it from this ethers issue

Copy link
Member

Choose a reason for hiding this comment

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

Ah yep! I think @Tulun used this some snippet in the voter dapp. May be useful (later) to pull this out into a separate lib.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah thats why I started this Ethers.js file to start collecting common ethers utils

@@ -0,0 +1,14 @@
const func = async function(hre) {
Copy link
Member

Choose a reason for hiding this comment

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

i'm guessing we'll add to these when we add contracts to the setup (like financial contracts, collateral whitelists, etc)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup! For each contract we add extra functionality to run if the contract gets newly deployed.

@@ -0,0 +1,33 @@
const { stringToBytes32, interfaceName, ZERO_ADDRESS } = require("@uma/common");
Copy link
Member

Choose a reason for hiding this comment

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

Is the mock oracle only expected to be deployed on a test L1? Do you think we should have a prod tag and a test tag or something so we can deploy the entire test setup or the entire prod setup? Also, are you allowed to provide multiple tags on deployment? If so, how do they mix (union or intersection of the tagged scripts?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes we should totally use prod and test tags. I'm thinking we give each contract two tags at least, its name (for use as a dependency in other deployment scripts) and a prod tag. This way we can leave out the prod tag from the MockOracle so we can just run yarn hardhat deploy --tags prod to deploy all prod contracts

Comment on lines 101 to 109
The following commands are implemented as [hardhat tasks](https://hardhat.org/guides/create-task.html) that make it easy to interact with deployed contracts via the CLI:

`yarn hardhat register-deployer --network <NETWORK-NAME>`

Registers the `deployer` account (as defined in the `namedAccounts` param in `hardhat.config.js`) with the deployed Registry for the network.

`yarn hardhat setup-finder --registry --bridge --generichandler --network <NETWORK-NAME>`

Sets specified contracts in the deployed Finder. More contracts available to be set can be found in the `core/scripts/hardhat/tasks/finder.js` script.
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea that most/all scripts that we currently have in scripts/ should be implemented as tasks in hardhat?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's what my crazy thought is long term. They could be ported pretty easily as scripts, which would make the CLI invocation a lot less verbose. The main benefit it seems of using tasks is you get access to the hardhat runtime environment which has a lot of nifty objects: https://hardhat.org/advanced/hardhat-runtime-environment.html

Copy link
Member

@mrice32 mrice32 May 7, 2021

Choose a reason for hiding this comment

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

I think you could also get the HRE in hardhat scripts too right? https://hardhat.org/guides/scripts.html

Note: nothing to change now, but worth a discussion as we start to port things over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep you can use HRE in js scripts

packages/core/networks/hardhat/deployed.json Outdated Show resolved Hide resolved
#!/usr/bin/env bash

# The type of oracle system to set up, for example "beacon-l1" or "beacon-l2". Default is "dvm".
ORACLE_TYPE=$1
Copy link
Member

Choose a reason for hiding this comment

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

Could we just make higher-level tags (in addition to the existing ones), like beacon-l1, beacon-l2, dvm to avoid having to write this script? Or is that a misuse of tags?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I just created production, sink-oracle and source-oracle tags for now. Will remove this script

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Very cool. What does the production tag run? Both oracle deployments or just the source?

Copy link
Member Author

Choose a reason for hiding this comment

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

production runs everything, but I'm thinking now we should change the name to "dvm" and not deploy either sink or source. "dvm" would be Finder, Registry, IdWhitelist, etc.

packages/core/scripts/hardhat/tasks/registry.js Outdated Show resolved Hide resolved
@@ -0,0 +1,33 @@
const { task } = require("hardhat/config");
Copy link
Member

Choose a reason for hiding this comment

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

I generally find the task syntax to be more pleasant than the truffle syntax. Do you prefer this to a normal node script? What do you think the tradeoffs are between this and a normal node script for this sort of script?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree! you also get CLI input args and params super easily along, and tasks can call each other easily as well: https://hardhat.org/guides/create-task.html

I think we should switch all of our scripts over to hardhat tasks because we can remove all of the logic that sets up parts of web3.

Copy link
Member

Choose a reason for hiding this comment

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

I think the big downside is coupling ourselves (again) to a specific framework. I can see arguments either way. But definitely agreed. Might be nice to avoid web3/provider setup, although, we could always write a utility function to do all that work for us (or just use getWeb3 :)).

fi

# The network to verify Etherscan contracts for
yarn hardhat etherscan-verify --license AGPL-3.0 --force-license --network $NETWORK_NAME
Copy link
Member

Choose a reason for hiding this comment

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

Very cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

We need this actually because our license tag SPDX-License-Identifier: AGPL-3.0-only is "not supported" by Etherscan?

Copy link
Member

@mrice32 mrice32 May 7, 2021

Choose a reason for hiding this comment

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

Weird, I thought it was listed on etherscan when doing manual verification....

}
};

// Tasks: These tasks are conveniently available via the hardhat CLI: `yarn hardhat <TASK>`
// Documentation on creating tasks here: https://hardhat.org/guides/create-task.html
require("./scripts/hardhat/tasks/finder");
Copy link
Member

@mrice32 mrice32 May 7, 2021

Choose a reason for hiding this comment

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

(Nothing to change now)

One advantage to hardhat (or node) scripts rather than tasks is that we don't need to import them into the config. That matters for two reasons:

  • Every time you add a "script" you have to add it to this long list of imports. If you forget, it doesn't work.
  • Every time the config is loaded, node will need to parse (and execute the global-level stuff, like adding the task to hardhat's global list) these tasks, even when not used, which just means added performance overhead to all hardhat commands.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... I see your point. We could move to using scripts in the next PR but i wanted to demonstrate the power of tasks.

Copy link
Member

@mrice32 mrice32 May 10, 2021

Choose a reason for hiding this comment

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

That's fair. It's just something I think we should consider. I think tasks are meant for high-level custom commands (similar to hardhat compile or deploy, but not built in) that are more general. I don't think it was meant as a place to put you contract-specific scripts, which is where hardhat run comes in.

@mrice32 mrice32 self-requested a review May 7, 2021 22:43
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM! Nothing blocking, but I left a few responses and comments that we can talk about more as we port more of core to hardhat!

Copy link
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

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

This is pretty awesome! I really like the tasks. I look forward to us being able to move everything over to using this cleaner, more useable syantx!

good work!

One additional question: did you get running node scripts working correctly with hardhat? i.e the equivalent of yarn truffle run using the yarn hardhat run.


`yarn void:deploy`

This will deploy your contracts on the in-memory hardhat network and exit, leaving no trace. Quickest way to ensure that deployments work as intended without consequences.
Copy link
Member

Choose a reason for hiding this comment

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

do you want to put the description above the command? makes it a bit easier to read.


`yarn hardhat deploy --network <NETWORK-NAME> --tags <TAGS>`

Deploy all contracts to specified network. Requires a `CUSTOM_NODE_URL` HTTP(s) endpoint and a `MNEMONIC` to be set in environment. Available contract tags can be found in `/deploy` scripts, and available networks are found in the `networks` object within `hardhat.config.js`. Tags can be powerful, for example running `yarn hardhat deploy --tags Bridge` will only deploy the Bridge contract its dependencies (such as the Finder).
Copy link
Member

Choose a reason for hiding this comment

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

siiick

stringToBytes32(interfaceName.IdentifierWhitelist),
deployResult.address
);
log(
Copy link
Member

Choose a reason for hiding this comment

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

some scripts have logs and others dont. should we make that more consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

So my rule has been to only log if there is additional work being done besides the deployment itself. For example, this script adds the IDWhitelist to the Finder.

const { stringToBytes32, interfaceName } = require("@uma/common");

task("setup-finder", "Points Finder to DVM system contracts")
.addFlag("registry", "Use if you want to set Registry")
Copy link
Member

Choose a reason for hiding this comment

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

this is cool. will be very powerful for us to define all logic and scripts using these lags.

Copy link
Member Author

@nicholaspai nicholaspai May 10, 2021

Choose a reason for hiding this comment

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

Agreed, but see Matt's comment about coupling ourselves to another framework. I tend to agree with him, but maybe we should (1) make sure all current scripts are compatible with yarn hardhat run and (2) rather than replace all scripts with tasks, add some commonly used tasks that we might want to run via CLI!

fi

# The network to verify Etherscan contracts for
yarn hardhat etherscan-verify --license AGPL-3.0 --force-license --network $NETWORK_NAME
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to tell this function what contracts you are verifying? does it just try every contract? how does this work exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

it just tries to verify every single contract stored in the local deployments folder. This is a description of the etherscan-verify task that the hardhat-deploy plugin adds to hardhat:

This plugin adds the etherscan-verify task to Hardhat.

This task will submit the contract source and other info of all deployed contracts to allow etherscan to verify and record the sources.

Copy link
Member

Choose a reason for hiding this comment

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

nice. this is pretty nifty

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