[VEN-2516] Deploy weETH Oracle #175
Conversation
|
| EtherFiLiquidityPool = mockEtherFiLiquidityPool.address; | ||
| await mockEtherFiLiquidityPool.transferOwnership(proxyOwnerAddress); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would also deploy the mocks tokens for weETH and eETH if the address is not available in the ADDRESSES mapping (for example on hardhat). /cc @coreyar
There was a problem hiding this comment.
I highly recommend not using address mappings at all and instead fetching deployments
There was a problem hiding this comment.
We can structure deployment scripts into 3 types of scripts for each deployment, (mocks, deploy, setup)
for example there could be
deploy/6-mocks-weETH-oracle.ts ( mocks wouldn't run in the right order but a name like dependencies would )
deploy/6-deploy-weETH-oracle.ts
deploy/6-setup-weETH-oracle.ts
Where setup receives an id so that it is only run once and mocks isn't deployed on live networks ( it would deploy the token contracts Jesus mentions) deploy only deploys contracts and setup doesn't deploy and only calls methods on contracts
There was a problem hiding this comment.
Right now this deployment script is following the pattern of the other deployment scripts in this repo.
I agree with your comments but I think we should make these changes in all the deployment scripts and not just this one to keep consistency. I would suggest we create a separate ticket and PR for this changes and this it itself requires a lot of code fixes.
There was a problem hiding this comment.
VEN-2547. We can leave this as it is in this PR, and review it in the new task.
|
|
||
| export default func; | ||
| func.tags = ["weETH"]; | ||
| func.skip = async (hre: HardhatRuntimeEnvironment) => hre.network.name !== "ethereum" && hre.network.name !== "sepolia"; |
There was a problem hiding this comment.
Let's enable the deployment to hardhat too
There was a problem hiding this comment.
script 4 is also skipped on hardhat
There was a problem hiding this comment.
Even this I think we should review for all the scripts and fix them if we want to enable for hardhat
| function amountForShare(uint256 _share) external view override returns (uint256) { | ||
| return (_share * amountPerShare) / 1e18; | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Our linting should catch and fix files that don't end with a newline
| proxy: { | ||
| owner: proxyOwnerAddress, | ||
| proxyContract: "OptimizedTransparentProxy", | ||
| }, |
There was a problem hiding this comment.
should we add skipIfAlreadyDeployed flag here? Just for consistency I guess wit will be okay. However it will not have any impact on the deployment (or any potential impl updates as we will have a separate script for impl update if needed)
There was a problem hiding this comment.
I have added skipIfAlreadyDeployed to this deployment. I think without skipIfAlreadyDeployed=true the contracts are getting re-deployed. So I think we should have a separate issue to include this flag in all the deployment scripts.
| @@ -81,4 +80,4 @@ describe("WeETHOracle unit tests", () => { | |||
| expect(price).to.equal(parseUnits("3199.9033516136821482", 18)); | |||
There was a problem hiding this comment.
can we either have a comment how did we calculated this price or perform the exact calculation as a parameter in the equal expression
There was a problem hiding this comment.
I have added a comment.
[VEN-2540] Quantstamp Audit Fixes
Description
Resolves #
Checklist