-
Notifications
You must be signed in to change notification settings - Fork 8
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
v2.3.0 scripts: migrations, dry-run, detect changes #686
Conversation
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 have some considerations
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.
Looks good to me :)
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.
LGTM
docs/local-development.md
Outdated
#### Using the migrations | ||
If you are testing an upgrade of official release, you can simply run | ||
``` | ||
npx hardhat migrate version --network local --env "" |
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.
- network
local
does not exist. Should behardhat
orlocalhost
?? - use "<...>" notation to mean variable arguments (?)
npx hardhat migrate version --network local --env "" | |
npx hardhat migrate <version> --network localhost --env "" |
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.
good catch! localhost should be the correct value
docs/local-development.md
Outdated
### Upgrade clients | ||
To test the upgrade functionality, you first need to setup an upgrader account as described in section [Manage roles](local-development.md#optional-manage-roles) | ||
|
||
To perform the upgrade you then | ||
- Update some of the existing clients | ||
- Run `npm run upgrade-clients:local`. This will deploy new clients and set implementation address on beacon. It also updates the existing addresses file `addresses/<chain-id>-<environment>.json` (for example `addresses/31337-localhost.json` if you are using a default local hardhat node) and outputs the upgrade log to the console. | ||
- Run `npm run upgrade-clients:local --new-version version`. This will deploy new clients and set implementation address on beacon. It also updates the existing addresses file `addresses/<chain-id>-<environment>.json` (for example `addresses/31337-localhost.json` if you are using a default local hardhat node) and outputs the upgrade log to the console. |
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.
- missing
--
separator between the npm run command and the arguments (otherwise the "--new-version" string is dropped from the command) - use "<...>" notation to mean variable arguments (?)
- Run `npm run upgrade-clients:local --new-version version`. This will deploy new clients and set implementation address on beacon. It also updates the existing addresses file `addresses/<chain-id>-<environment>.json` (for example `addresses/31337-localhost.json` if you are using a default local hardhat node) and outputs the upgrade log to the console. | |
- Run `npm run upgrade-clients:local -- --new-version <version>`. This will deploy new clients and set implementation address on beacon. It also updates the existing addresses file `addresses/<chain-id>-<environment>.json` (for example `addresses/31337-localhost.json` if you are using a default local hardhat node) and outputs the upgrade log to the console. |
docs/tasks.md
Outdated
|
||
To use them, execute the following command | ||
``` | ||
npx hardhat migrate version --network network --env environment [--dry-run] |
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.
use "<...>" notation to mean variable arguments (?)
npx hardhat migrate version --network network --env environment [--dry-run] | |
npx hardhat migrate <version> --network <network> --env <environment> [--dry-run] |
docs/tasks.md
Outdated
### Upgrade clients | ||
Upgrade existing clients (currently only BosonVoucher). Script deploys new implementation and updates address on beacon. | ||
We provide different npm scripts for different use cases. A script for Hardhat network does not exist. Since contracts are discarded after the deployment, they cannot be upgraded. | ||
For upgrade to succeed you need an account with UPGRADER role. Refer to [Manage roles](#manage-roles) to see how to grant it. | ||
If you are not sure which contracts were changed since last deployment/upgrade, refer to [Detect changed contract](#detect-changed-contract) to see how to get the list of changed contracts. | ||
|
||
- **local network**. This upgrades the clients on a independent instance of local network (e.g. `npx hardhat node`). Upgrade process is described [here](local-development.md#upgrade-clients). | ||
```npm run upgrade-clients:local``` | ||
```npm run upgrade-clients:local --new-version version``` |
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.
```npm run upgrade-clients:local --new-version version``` | |
```npm run upgrade-clients:local --new-version <version>``` |
docs/tasks.md
Outdated
- **internal test node**. This upgrades the clients on a custom test network. You need to modifiy `.env` with appropriate values for this to work. | ||
```npm run upgrade-clients:test``` | ||
```npm run upgrade-clients:test --new-version version``` |
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.
```npm run upgrade-clients:test --new-version version``` | |
```npm run upgrade-clients:test --new-version <version>``` |
docs/tasks.md
Outdated
```npm run upgrade-clients:polygon:mumbai-test``` | ||
```npm run upgrade-clients:polygon:mumbai-staging``` | ||
```npm run upgrade-clients:polygon:mumbai-test --new-version version``` | ||
```npm run upgrade-clients:polygon:mumbai-staging --new-version version``` |
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.
```npm run upgrade-clients:polygon:mumbai-staging --new-version version``` | |
```npm run upgrade-clients:polygon:mumbai-staging --new-version <version>``` |
docs/tasks.md
Outdated
- **Polygon Mainnet**. This upgrades the clients on Polygon Mainnet. | ||
```npm run upgrade-clients:polygon:mainnet``` | ||
```npm run upgrade-clients:polygon:mainnet --new-version version``` |
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.
```npm run upgrade-clients:polygon:mainnet --new-version version``` | |
```npm run upgrade-clients:polygon:mainnet --new-version <version>``` |
docs/tasks.md
Outdated
- **Ethereum Mainnet**. This upgrades the clients on Ethereum Mainnet. | ||
```npm run upgrade-clients:ethereum:mainnet``` | ||
```npm run upgrade-clients:ethereum:mainnet --new-version version``` |
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.
```npm run upgrade-clients:ethereum:mainnet --new-version version``` | |
```npm run upgrade-clients:ethereum:mainnet --new-version <version>``` |
hardhat.config.js
Outdated
({ setupDryRun, getBalance } = await lazyImport(`./scripts/migrations/dry-run.js`)); | ||
({ env, upgraderBalance: balanceBefore } = await setupDryRun(env)); | ||
} else { | ||
process.exit(); |
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.
If the dry-run option is not activated, the command does nothing (???)
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.
removed
const { chainId } = await ethers.provider.getNetwork(); | ||
const contractsFile = readContracts(chainId, network, env); | ||
if (contractsFile?.protocolVersion != "2.2.1") { | ||
throw new Error("Current contract version must be 2.2.1"); |
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.
This check should be done at the very beginning of the migrate function (before any change has been done on the repo), so that if the criteria is not met, the repo is not modified
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 move to after the local changes checks otherwise, the version could be wrong if we only change the version locally.
@levalleux-ludo applied all! |
c89c054
to
9c01246
Compare
Changes in this PR:
detect-changed-contracts.js
to work when protocol versions from different tags use different compiler versionsmigrate_2.3.0.js
. NB: This is not the final version of the migration script. It will be adapted as part of v2.3.0 migration script and upgrade test #680 when other PRs are merged into the main. The parts that are common withmigrate_2.2.1
will be refactored into util functions.