Refactor and fix issues in diamond deploy script #79
Conversation
package.json
Outdated
| "test": "hardhat test", | ||
| "compile": "hardhat compile && yarn format", | ||
| "compile": "hardhat compile", | ||
| "clean-compile": "set -x && mv ./tasks/index.ts ./tasks/temp_index.ts && touch ./tasks/index.ts && hardhat clean && hardhat compile && mv ./tasks/temp_index.ts ./tasks/index.ts && set +x", |
There was a problem hiding this comment.
set -x ... set +x will enable echoing currently running commands
tasks/deploy/deployDiamond.ts
Outdated
| taskArgs.registrar, | ||
| taskArgs.corestruct, | ||
| taskArgs.stringlib, | ||
| addresses.multiSig.apTeam.proxy, |
There was a problem hiding this comment.
These addresses should be read from the local address file.
…t upgrading all facets
0xNeshi
force-pushed
the
recompile-and-update-diamond-deploy
branch
from
6967491 to
f716fe6
Compare
| @@ -45,7 +45,7 @@ export default async function createFacetCuts( | |||
| facetCuts.push({ | |||
| facetName: facet.name, | |||
| cut: { | |||
| facetAddress: facet.contract.address, | |||
| facetAddress: ADDRESS_ZERO, | |||
There was a problem hiding this comment.
When removing selectors from diamond, facet address must be address(0)
|
|
||
| export default async function verify( | ||
| facetCuts: FacetCut[], | ||
| hre: HardhatRuntimeEnvironment | ||
| ): Promise<void> { | ||
| logger.out("Verifying newly deployed facets..."); | ||
|
|
||
| for (const {facetName, cut} of facetCuts) { | ||
| const facetsToVerify = facetCuts.filter((cut) => cut.cut.action !== FacetCutAction.Remove); |
There was a problem hiding this comment.
Now removed facets need not be verified
| @@ -86,7 +86,6 @@ function createEmpty(): AddressObj { | |||
| diamondInitFacet: "", | |||
| diamondLoupeFacet: "", | |||
| ownershipFacet: "", | |||
| reentrancyGuardFacet: "", | |||
There was a problem hiding this comment.
This facet is inherited by other facets, not deployed separately
| import {task} from "hardhat/config"; | ||
| import {getAddresses, isLocalNetwork, logger} from "utils"; | ||
|
|
||
| task("Deploy:AccountsDiamond", "It will deploy accounts diamond contracts").setAction( |
There was a problem hiding this comment.
Using a task naming convention of Deploy:[ContractName] is as clear as the previous Deploy:deploy[ContractName] convention, without the extra "deploy" in the name.
@stevieraykatz @SovereignAndrey additionally we could update all task names to remove redundant verbs and lower-case first letter:
Deploy:deploy[ContractName]->deploy:[ContractName]upgrade:upgrade[ContractName]->upgrade:[ContractName]manage:[operationName]can remain the same
| async (_, hre) => { | ||
| try { | ||
| const addresses = await getAddresses(hre); | ||
| const verify_contracts = !isLocalNetwork(hre.network); |
There was a problem hiding this comment.
I'm not sure we would ever want to NOT verify contracts deployed on mainnet/testnet, would make sense to remove the verify task param from all scripts (e.g. deployEndowmentMultiSig)?
There was a problem hiding this comment.
also seems fine to change default to verify = true. it can add some non neglible time to deployments since it runs a compile/compile check. leaving it optional but turned on seems like the right way to go. tasks have an optional arg type that could be used here
| @@ -7,7 +7,8 @@ | |||
| "format": "prettier --write './**/*.{ts,tsx,js}' --write contracts/**/*.sol", | |||
| "test": "hardhat test", | |||
| "compile": "hardhat compile && yarn format", | |||
| "deploy": "hardhat compile && hardhat run ./scripts/deploy.ts", | |||
| "clean-compile": "set -x && mv ./tasks/index.ts ./tasks/temp_index.ts && touch ./tasks/index.ts && hardhat clean && yarn compile && mv ./tasks/temp_index.ts ./tasks/index.ts && set +x", | |||
| "deploy": "yarn compile && hardhat run ./scripts/deploy.ts", | |||
There was a problem hiding this comment.
Just running hardhat compile results in unformatted typechain-types/* folder. Then again, it makes little sense to even format this folder as it's auto-generated (same goes for dist/* folder and any other non-source-code folder).
| @@ -7,7 +7,8 @@ | |||
| "format": "prettier --write './**/*.{ts,tsx,js}' --write contracts/**/*.sol", | |||
| "test": "hardhat test", | |||
| "compile": "hardhat compile && yarn format", | |||
| "deploy": "hardhat compile && hardhat run ./scripts/deploy.ts", | |||
| "clean-compile": "set -x && mv ./tasks/index.ts ./tasks/temp_index.ts && touch ./tasks/index.ts && hardhat clean && yarn compile && mv ./tasks/temp_index.ts ./tasks/index.ts && set +x", | |||
There was a problem hiding this comment.
Added @SovereignAndrey 's script for clean compile.
set -x ... set +x will enable echoing the current command being run, which could be useful in such a long chain of commands.
0xNeshi
changed the title
Ticket(s):
Explanation of the solution
See comments below.
Instructions on making this work
yarnoryarn installto install npm dependenciesyarn testto verify all tests still passnpx hardhat nodenpx hardhat Deploy:AccountsDiamond --network localhost