-
Notifications
You must be signed in to change notification settings - Fork 70
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
OETH Base deploy script #2061
OETH Base deploy script #2061
Conversation
deployWithConfirmation, | ||
withConfirmation, | ||
} = require("../../utils/deploy"); | ||
const { getTxOpts } = require("../../utils/tx"); |
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.
You can rename this file to 002_woeth_on_base.js
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.
Done
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.
Aside from a minor file rename, the CI seems to not work. Would be cool that before deployment the all the unit & fork tests in the CI succeed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## PraneshASP/aerodrome-amo #2061 +/- ##
===========================================================
Coverage ? 62.55%
===========================================================
Files ? 64
Lines ? 3213
Branches ? 823
===========================================================
Hits ? 2010
Misses ? 1199
Partials ? 4 ☔ View full report in Codecov by Sentry. |
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.
Few comments inline. Random question: Storage layout file isn't updated?
contracts/fork-test.sh
Outdated
@@ -80,6 +80,10 @@ main() | |||
# Run all files with `.holesky.fork-test.js` suffix when no file name param is given | |||
# pass all other params along | |||
params+="test/**/*.holesky.fork-test.js" | |||
elif [[ $FORK_NETWORK_NAME == "holesky" ]]; then |
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 this be base
?
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.
good catch, changed.
contracts/fork-test.sh
Outdated
@@ -101,7 +105,7 @@ main() | |||
FORK=true IS_TEST=true npx --no-install hardhat coverage --testfiles "${params[@]}" | |||
else | |||
echo "Running fork tests..." | |||
FORK=true IS_TEST=true npx --no-install hardhat test ${params[@]} | |||
FORK=true IS_TEST=true npx --no-install hardhat test ${params[@]} --show-stack-traces |
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.
why this change?
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.
removed
contracts/hardhat.config.js
Outdated
@@ -195,6 +205,8 @@ module.exports = { | |||
process.env.FORK === "true" | |||
? isHoleskyFork | |||
? HOLESKY_DEPLOYER | |||
: isBaseFork | |||
? BASE_DEPLOYER |
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 don't agree with this change personally. If we change the governor on fork, we are deploying/testing the contracts in a state that's gonna be completely different from mainnet.
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.
Agreed, this has been removed.
contracts/test/_global-hooks.js
Outdated
const isArbTestFile = s.file.endsWith(".arb.fork-test.js"); | ||
const isBaseTestFile = s.file.endsWith(".base.fork-test.js"); | ||
|
||
if (isMainnetForkTest) { |
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 I changed the order and fixed it in some PR. isMainnetForkTestFile
should always be at the end and should always be fully spread like isMainnetForkTestFile && !isHoleskeyTestFile && !isArbTestFile
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.
Thanks for the fix
contracts/utils/hardhat-helpers.js
Outdated
@@ -5,14 +5,19 @@ const isFork = process.env.FORK === "true"; | |||
const isArbitrumFork = process.env.FORK_NETWORK_NAME === "arbitrumOne"; | |||
const isHoleskyFork = process.env.FORK_NETWORK_NAME === "holesky"; | |||
const isHolesky = process.env.NETWORK_NAME === "holesky"; | |||
const isBase = process.env.NETWORK_NAME === "base"; |
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.
checking chainId would be much better here
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.
Yup agreed, much safer
…nProtocol/origin-dollar into PraneshASP/oethbase-deploy
* wip * refactor: update deploy scripts to use guardian
If you made a contract change, make sure to complete the checklist below before merging it in master.
Refer to our documentation for more details about contract security best practices.
Contract change checklist: