-
Notifications
You must be signed in to change notification settings - Fork 69
feat: Added foundry deploy script #1044
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
base: master
Are you sure you want to change the base?
Conversation
64e7247
to
7ebdaf8
Compare
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 think this is a great start.
script/DeployConstants.sol
Outdated
|
||
/** | ||
* @title DeployConstants | ||
* @notice Contains constants used in deployment scripts, converted from consts.ts |
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.
Could we read this in via one or more json configuration files? Two advantages:
- Config/code split so it's clear (in the future) which changes are configuration and which changes are logical.
- More flexibility: it will allow us to pull in this info from somewhere else easily. For instance, the json could be generated/updated from an API OR we could import one or more of the config jsons from a common package (like our constants package or something).
Thoughts?
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.
Mainly because dealing with json inside a foundry script is not trivial, I also have deployed-addresses.json
that is generated regarding your second point
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.
Don't they have utilities for that that makes it fairly trivial if the json is well structured? I may be missing some additional complexity (I haven't tried to use these utils before)
https://getfoundry.sh/reference/cheatcodes/parse-json/?highlight=json
https://getfoundry.sh/reference/cheatcodes/parse-json-keys
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.
updated to using perse-json, much cleaner now :)
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.
Thoughts on expanding this, s.t. the Constants.sol values are read in from json as well?
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.
Constants are currently in a ts file, but I can create a json for constants that Constants.sol uses
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
72e2502
to
84ccd43
Compare
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.
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
1a9787a
to
6b82900
Compare
Signed-off-by: Faisal Usmani <[email protected]>
Signed-off-by: Faisal Usmani <[email protected]>
300516f
to
6aec3c4
Compare
First attempt at deploy script migration to foundry. Just did one deploy script and looking for feedback.
List of changes:
DeployHubPool.s.sol
andscript/DeployEthereumSpokePool.s.sol
insidescript
folderDeployConstants.sol
insidescript
folder that works similar todeploy/consts.ts
by providing third party smart contract addressesDeploymentUtils.sol
insidescript
that works similar toutils/utils.hre.ts
forge script script/DeployHubPool.s.sol:DeployHubPool --rpc-url $RPC_URL --broadcast --verify -vvvv
which deploys and verifies the contract and adds the deploy info in thebroadcast
folderscript/extract_foundry_addresses.sh
that extracts all the deployed addresses from thebroadcast
folder and puts it in a json, an md file and a sol file (similar todeployments.json
)Looking for feedback on the proposed flow and how I can make this better and address some of the short comings of the current deployment flow
Fixes https://linear.app/uma/issue/UMA-2906/contracts-migrate-smart-contract-deploy-to-foundry