-
Notifications
You must be signed in to change notification settings - Fork 99
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
CR Oracle + Timelock Ops #925
Conversation
EDIT: deleted old calldata |
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.
Looks good overall, few tidy up comments
proposals/dao/timelock_admin.ts
Outdated
// Run any validations required on the fip using mocha or console logging | ||
// IE check balances, check state of contracts, etc. | ||
const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => { | ||
console.log('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.
Nit: Remove this console.log
proposals/dao/timelock_admin.ts
Outdated
// This should exclusively include new contract deployments | ||
const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: NamedAddresses, logging: boolean) => { | ||
const factory = (await hre.ethers.getContractFactory('ERC20PCVDepositWrapper')) as ERC20PCVDepositWrapper__factory; | ||
const rariTimelockFeiOldLens = await factory.deploy(addresses.rariInfraFeiTimelock, addresses.fei, true); |
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.
Will make sure to add lenses for contracts with significant Fei that are becoming part of the Fei ecosystem in the future!
import { TemplatedProposalDescription } from '@custom-types/types'; | ||
|
||
const timelock_admin: TemplatedProposalDescription = { | ||
title: 'Timelock Updates', |
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.
Would probably add a TIP 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.
Yup!
|
||
const proposals: TemplatedProposalsConfigMap = { | ||
tip_111: { | ||
// tip_111: { |
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.
Think this is leftover commenting and needs deleting? tokemak_withdraw
and tip_112
can both be removed.
pod_exec_v2
needs to stay here, it hasn't been proposed/executed by the TC yet
|
||
// Increase PCV balance to exceed deviation threshold | ||
const pcvStats = await collateralizationOracleWrapper.pcvStats(); | ||
await namedStaticPCVDepositWrapper.addDeposit({ |
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 test relies on named static PCV deposit which will be deprecated in this proposal. Erwan's #923 also deprecates the keeper
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.
Looks fine to me, will give a final once over tomorrow then approve
values: '0', | ||
method: 'grantRole(bytes32,address)', | ||
arguments: (addresses) => [ | ||
'0x471cfe1a44bf1b786db7d7104d51e6728ed7b90a35394ad7cc424adf8ed16816', // SWAP_ADMIN_ROLE |
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.
Nit, you can import ethers
now and replace this with ethers.utils.id('SWAP_ADMIN_ROLE)
@@ -47,6 +52,9 @@ const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, | |||
expect(await contracts.rariTimelockFeiOldLens.balanceReportedIn()).to.be.equal(addresses.fei); | |||
expect(await contracts.tribalCouncilTimelockFeiLens.balanceReportedIn()).to.be.equal(addresses.fei); | |||
expect(await contracts.namedStaticPCVDepositWrapper.numDeposits()).to.be.equal('0'); | |||
expect(await contracts.ethToDaiLBPSwapper.swapEndTime()).to.be.gt( | |||
ethers.BigNumber.from((await time.latest()).toString()) | |||
); |
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.
Depends how confident are, but I would probably make sure you can do a swap via the LBP pool here
false, | ||
false | ||
], | ||
description: 'Withdraw 10k ETH to LBP Swapper' |
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.
Weights are 95 / 5 . You're sending $1m DAI and ~$10.5m ETH. So overfunding by $500k, should be fine
Calldata:
Execute Calldata:
|
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.
Looks okay to me. I would add a validation that you can do a swap and that the implied price is reasonable first if there's time.
Have verified the propose and execute calldata, looks good
PR Type: Feature
PR Description:
Executes a few cleanup actions related to collateralization and operations:
PR Checklist - Feature (Proposal)