Skip to content

CR oracle deploy#245

Merged
xklob merged 15 commits intodevelopfrom
feat/CR-Oracle-Deploy
Oct 15, 2021
Merged

CR oracle deploy#245
xklob merged 15 commits intodevelopfrom
feat/CR-Oracle-Deploy

Conversation

@Joeysantoro
Copy link
Contributor

@Joeysantoro Joeysantoro commented Oct 13, 2021

TODO:

  • add wrappers for Fuse Pools 31, 28
  • make sure admin role is held by Guardian or OA to start
  • Come up with a plan for "one offs" (Plan is for OA to adjust static wrapper)
  • Add ERC20 Wrapper for FEI in OA ERC20 PCV deposit wrapper #247

Joey Santoro added 2 commits October 13, 2021 14:50
@Joeysantoro Joeysantoro changed the base branch from master to develop October 13, 2021 22:12
xklob
xklob previously requested changes Oct 14, 2021
Copy link
Contributor

@xklob xklob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks great and I don't see any bugs. However, a couple of requests before we go live:

  1. @Joeysantoro you're more familiar with which pcv deposits are protocol-owned (which would make the second deploy parameter True). You should definitely check over all of the ones here to make sure this parameter is set correctly for each one.

  2. In the "Deploy Actions" comment, there are 28 actions. Can you number each deployment action in the script corresponding to this comment? This will ensure we aren't missing anything.

  3. You should do one more check over to make sure there's no PCV deposits we forgot to add.

@Joeysantoro
Copy link
Contributor Author

  1. @Joeysantoro you're more familiar with which pcv deposits are protocol-owned (which would make the second deploy parameter True). You should definitely check over all of the ones here to make sure this parameter is set correctly for each one.

I had them organized into two groups so it should be false in the first and true in the second

2. In the "Deploy Actions" comment, there are 28 actions. Can you number each deployment action in the script corresponding to this comment? This will ensure we aren't missing anything.

Done

3. You should do one more check over to make sure there's no PCV deposits we forgot to add.

Can't think of any more after we add ERC20 one. I also incremented the manual PC FEI for Idle and BarnBridge. I made the guardian the owner until we ratify on-chain so we can add/remove

Copy link
Contributor

@xklob xklob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great. We should definitely add a bunch of unit & integration tests before using it "in prod" however.

@xklob
Copy link
Contributor

xklob commented Oct 15, 2021

linting & prettier for you, and also pushed the small fix for the typescript-casting for the contracts.

@xklob xklob merged commit ea220c4 into develop Oct 15, 2021
@xklob xklob deleted the feat/CR-Oracle-Deploy branch October 15, 2021 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments