-
Notifications
You must be signed in to change notification settings - Fork 10
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: simulation support in deploy-cli #406
Conversation
@@ -33,7 +33,6 @@ const argv = minimist(process.argv.slice(2), { | |||
|
|||
const ZAddress = z.string().regex(/^0x[0-9a-fA-F]{40}$/); | |||
const ZDeploymentContext = z.object({ | |||
deployedContracts: z.array(z.tuple([z.string(), ZAddress])), |
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 was not used anywhere, was it @bpierre?
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.
Only in the subgraph branch, but I will adapt to the new format 👍
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.
Ah, thanks, I didn't know!
If it needs any contracts missing from the current output, we should be able to add them to protocolContracts
or collateralContracts
easily.
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 it’s only for the BoldToken
address (the subgraph deployment script can update it from the latest deployment), which we have already :)
Now that it's multi-collateral, it takes longer to deploy.
await fs.writeJson("deployment-context-latest.json", { | ||
options: safeOptions, | ||
deployedContracts, | ||
collateralContracts, | ||
protocolContracts, | ||
}); |
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.
Should we remove this entirely? Or just keep the options there?
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 guess we could inline the JSON coming from the Solidity script as-is, for simplicity. Also, it would be more complete. It might make sense to keep the options. Also, we might want to add additional metadata at some point, like deployment block (might come in handy for the subgraph too), timestamp, etc.
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.
🔥
Add the new flag
--dry-run
todeploy-cli
, which can be used to simulate the deployment without broadcasting transactions. To make this work, we generate a deployment manifest (a JSON file of deployed addresses) from the Solidity script (DeployLiquity2
) itself, which lets us avoid having to make RPC calls to get the deployed addresses (which wouldn't work when simulating).Also add the
--slow
flag which tells Foundry to broadcast one transaction at a time, which is needed when deploying through providers like Alchemy.Further, add a
--gas-price
flag which can be used to setmaxFeePerGas
in broadcast transactions.