-
Notifications
You must be signed in to change notification settings - Fork 44
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: localhost and hardhat deploy can be run with zero config #539
Conversation
network, | ||
'MANAGEMENT_DAO_MULTISIG_APPROVERS', | ||
// random address | ||
'0x6B2b5d4F0a40134189330e2d46a9CDD01C01AECD' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the implications of using a random address here?
Instead of a random address, we could think about using the known hardhat default address (derived from hardhat's default mnemonic 'test test test test test test test test test test test junk'
.
I think it is 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
.
This could result in tests passing (that mostly use signers[0]), although this could be equally undesirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already use the private key for the HH address you listed as default. So possibly we can use the next derived address from that same mnemonic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds good. Maybe for the default, we can have two or three approvers (and a 1/2 or 2/3 threshold) - all from the default mnemonic.
I am wondering how this will work in combination with local fork tests.
In the future, we might want to do similar fork testing like I did it for the osx-plugin-template-hardhat
.
I guess we can adapt these functions once we decide to work on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding multiple approvers is a good improvement that should be added to the backlog for fork tests. I would avoid changing that inside this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, can you add a task and link it here?
Proper punctuation in doc comments Co-authored-by: Michael Heuer <[email protected]>
...ges/contracts/deploy/new/40_finalize-management-dao/30_install-multisig-on-management-dao.ts
Outdated
Show resolved
Hide resolved
typo Co-authored-by: Michael Heuer <[email protected]>
@@ -18,7 +22,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { | |||
const {ethers, network} = hre; | |||
const [deployer] = await ethers.getSigners(); | |||
|
|||
if (network.name !== 'localhost' && network.name !== 'hardhat') { | |||
if (!['localhost', 'hardhat'].includes(network.name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above https://github.com/aragon/osx/pull/539/files#r1489567374
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double-check that there is no scenario, in which a wrong list of approvers can be set accidentally (see my comment about || [deployer]
.
In production, we should throw errors if env variables are not set, which we do in the script tagged 'Env'
check. However, you can select/exclude certain tags which could result in the 00-env-check.ts
not being run.
In this case, the script must throw an error too and not silently run with a wrong approver list.
Let's make sure that this is the case
Yeah so any time one of the
In this way, the Env check is a redundancy but also helps to signpost all the required variables before any transactions run. If the required env variable is not set and a deploy script is run, we will still throw when the variable is accessed with a helpful error message. |
Co-authored-by: Michael Heuer <[email protected]>
Description
Currently, running the deploy script requires several steps and env var config. Default values are suggested in the README so this PR uses the default values as fallbacks when the network is
localhost
orhardhat
.Implementing this properly required:
process.env
with the fallback functions.package.json
Task ID: OS-1040
Type of change
Checklist:
CHANGELOG.md
file in the root folder.DEPLOYMENT_CHECKLIST
file in the root folder.UPDATE_CHECKLIST
file in the root folder.