-
Notifications
You must be signed in to change notification settings - Fork 75
feat: add contract finder to export artifacts and addresses for contracts #129
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
Conversation
…acts Signed-off-by: chrismaree <christopher.maree@gmail.com>
src/ContractFinder.ts
Outdated
|
|
||
| export function getContractArtifact(contractName: string, networkId: number) { | ||
| try { | ||
| return deploymentExport[networkId.toString()][0].contracts[contractName]; |
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.
what is [0] accessing?
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 element is within a nested array. we need to extract the 0th element.
tsconfig.json
Outdated
| "target": "es2018", | ||
| "module": "commonjs", | ||
| "strict": true, | ||
| "strict": false, |
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 seems dangerous, not sure we should change to false. What's the reasoning?
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.
Yeah, +1 on @nicholaspai's question
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.
The structure of the export.json file makes this fail. to make it pass I needed to make this change.
nicholaspai
left a comment
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.
The diff on this PR looks huge but the logic changes are very minimal. I've left a few comments but have one question: what is deployments/export.json?
this is the exported file containing the addresses deployed on all networks and associated ABI. It is produced by running: hardhat export --export-all ./deployments/export.json |
src/ContractFinder.ts
Outdated
| @@ -0,0 +1,9 @@ | |||
| import * as deploymentExport from "../deployments/export.json"; | |||
|
|
|||
| export function getContractArtifact(contractName: string, networkId: number) { | |||
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.
OOC, why do we have this function here rather than having the users access the JSON directly? Just convenience I assume?
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.
it's just much easier to have a method to call & export rather than needing to know how to deal with the structure internally of this file.
tsconfig.json
Outdated
| "target": "es2018", | ||
| "module": "commonjs", | ||
| "strict": true, | ||
| "strict": false, |
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.
Yeah, +1 on @nicholaspai's question
…ol/across-smart-contracts-v2 into chrismaree/hardhat-export
| "test:gas-analytics": "GAS_TEST_ENABLED=true hardhat test ./test/gas-analytics/*", | ||
| "test:all": "GAS_TEST_ENABLED=true REPORT_GAS=true yarn hardhat test", | ||
| "prepublish": "yarn build" | ||
| "prepublish": "yarn build && hardhat export --export-all ./cache/massExport.json && ts-node ./scripts/processHardhatExport.ts && prettier --write ./deployments/deployments.json" |
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.
more involved now.
scripts/processHardhatExport.ts
Outdated
| @@ -0,0 +1,24 @@ | |||
| var fs = require("fs"); | |||
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.
new script.
| _amount?: string, | ||
| _realizedLpFeePct?: string, | ||
| _relayerFeePct?: string | ||
| _amount?: BigNumber, | ||
| _realizedLpFeePct?: BigNumber, | ||
| _relayerFeePct?: BigNumber |
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 is the reason all the other .toStrings() were removed: we now use the correct types.
We need the ability to import contract artifacts and addresses from this repo in other repos. this PR adds an export process for this.