Migrate deployment to use cannon#257
Migrate deployment to use cannon#257kaze-cow wants to merge 18 commits intonew-chain-deploymentsfrom
Conversation
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
anxolin
left a comment
There was a problem hiding this comment.
Test plan was great, and worked. Only thing, I saw a incoherence with the generated artifact path in the README.
I checked the contract matches, the tests pass, and the main configs. Looks pretty good.
Description of PR was good, but some things that could be better are
- Title is strange. Should be written like a command you give to the code to change. For example in this case "Migrate deployment to use cannon" or sometihng similar
- The 2 notes are a bit strange, if CI fails, maybe is good we set it up later so we have an independent PR just for that that is more soped and easier to review and test. This PR doess a lot of things at once. Anyways, im not concerns, so don't split it.
- The description feels more like "a context" you are giving, which is nice, but I would love to read as the first part of a description a highlight of what I expect to see later in the code (so I understand the scope). Something like, "This PR includes the migration to cannon, and the following are the main changes:" or sth similar
README.md
Outdated
| NETWORK='rinkeby' | ||
| GAS_PRICE_WEI='1000000000' | ||
| yarn deploy --network $NETWORK --gasprice $GAS_PRICE_WEI | ||
| yarn hardhat cannon:build --network cannon --wipe |
There was a problem hiding this comment.
what's the plan for networks.json file? do we keep it? probably nice cause is likely theres many places pointing to it and is convenient, but is also duplicated info, plus can fall out of sync with the cannon/deploy.json
Also, what's the plan with multiple environments? We only have one file cannon/deploy.json
There was a problem hiding this comment.
what's the plan for networks.json file? do we keep it?
I think we should keep it as is for backwards-compatibility reasons, but direct users to refer to the deployments repo for most up-to-date information
Also, what's the plan with multiple environments? We only have one file cannon/deploy.json
thats right. The file that is stored in this repo is the "local network" deployment (chain id 13370), and its designed to serve as an effective "blueprint" for what actually gets deployed on the real networks later. For the multiple networks that will be deployed in the future, those manifests all get stored in the deployments repository.
There was a problem hiding this comment.
I think we should keep it as is for backwards-compatibility reasons, but direct users to refer to the deployments repo for most up-to-date information
But if we want to keep it, I guess we will need to do a script or something for generating it automatically?
Otherwise, it will fall out of sync soon.
Maybe we don't need it if we can provide an alternative way to get the addresses. Like pointing to our docs or the deployment github repo or something. I think we should discuss it a bit more what's best here. I'm fine keeping it out of the PR, but I think we will need to follow up on this
There was a problem hiding this comment.
But if we want to keep it, I guess we will need to do a script or something for generating it automatically?
Otherwise, it will fall out of sync soon.
Yea I think its Ok if its OOD as long as we tell users they have to go elsewhere for the most up to date info
There was a problem hiding this comment.
@kaze-cow would it make sense to always create the same NPM scripts (same name) in all projects?
For example, this project usesyarn build to build the contracts.
Given this command to build cannon packages don't change (they have the same netork and params).
What do you think we add some convenient scripts:
yarn build:cannon: would run the 2 commands bellowyarn cannon:package: would runyarn hardhat cannon:build --network cannon --wipeyarn cannon:registry: Rename for yourrecord-cannonscript
Just a suggestion, maybe there's better naming conventions. The point is to have a simple command that is familiar across projects (always the same)
Co-authored-by: Anxo Rodriguez <anxo@cow.fi>
I like the naming approach you suggested. currently we have one script |
Description
In order to improve multi-network multi-repository deployments of CoW protocol, we are switching to using Cannon. This retains the existing deployment system. Unfortunately, many of the smart contracts have been updated (causing changes in the deployed contract address of the settlement contract), so techincally these also have to be referted in order for the correct package version to be generated.
Considering the smart contract changes we ultimately want to have in the next settlement contract, it seems smart to move them into a new
devbranch which we use as the branch where PRs get merged until we are ready to deploy settlement v1.5It also includes all the appropriate documentation for continued maintenance and usage of cannon.
Test Plan
Similar to other cannon releases:
yarn install)yarn hardhat cannon:build --network cannon --wipe)yarn record-cannonresults in no substantial changes to the recorded package manifest (some changes are to be expected, such astimestampandchainDumpswill change due to the time changing)Related PRs
This PR is the product of two previous PRs as I experimented with getting a good implementation of Cannon in this repo.
#255 -- previous PR which also removes
hardhat-deployand has much, much more changes. This PR is much more minimal.#256 -- this one was supposed to be the "minimized" version, but its based off the
mainbranch which doesn't produce the proper artifacts. This PR is based onnew-chain-deployments