-
Notifications
You must be signed in to change notification settings - Fork 49
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
build: multi-chain deployment script #748
Conversation
79aa953
to
5b2eb75
Compare
build: remove etherscan api keys in foundry.toml feat: add deploy core 3 scripts chore: improve .env.example file
chore: git ignore deployments dir
chore: remove forge fmt comment
chore: improve multi-chain deployment script refactor: rename env var MAX_SEGMENT_COUNT
8827002
to
0b6d4ee
Compare
It looks like
Are you aware of this, @smol-ninja, @andreivladbrg? Might be worth digging this up. |
Yes. I have used it before. In order to use it, we will have to explicitly specify it in the script files. Since we will still need an option to select chains to deploy on (could be because of higher gas fee on one chain at the time of deployment), the script will be required to take list of chains as input. This works without using IMO it does not add any additional benefit other than handing over the multi deployment logic to the script files. function run() {
vm.createSelectFork(POLYGON_RPC_URL);
vm.startBroadcast();
vm.stopBroadcast();
vm.createSelectFork(ETHEREUM_RPC_URL);
vm.startBroadcast();
vm.stopBroadcast();
} |
As realized in #748, keeping the Etherscan API keys in version control is not worth the maintainance cost.
As realized in #748, keeping the Etherscan API keys in version control is not worth the maintenance cost.
As realized in #748, keeping the Etherscan API keys in version control is not worth the maintenance cost.
* build: add script to generate deployment command * feat: new command-line options for deploy-multi-chain command * chore: continue for warnings and throw for errors * perf: polish deployment script --------- Co-authored-by: Paul Razvan Berg <[email protected]>
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 except for the admin addresses that can be hard-coded directly in the script. I've pushed a commit myself.
Good to merge, but as I've said here, we should do some more testing when we get to deploy V2.2.
Agree. Thanks for explaining! |
Feel free to accept the PR and merge it @smol-ninja @andreivladbrg. |
@andreivladbrg - you have the honor to squash and merge. |
great job @andreivladbrg, I like the script. Let me know what you think about my changes.