Skip to content
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 deploy #245

Merged
merged 15 commits into from
Oct 15, 2021
Merged

CR oracle deploy #245

merged 15 commits into from
Oct 15, 2021

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
Copy link
Contributor

@kryptoklob kryptoklob 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

@kryptoklob kryptoklob 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.

@kryptoklob
Copy link
Contributor

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

@kryptoklob kryptoklob merged commit ea220c4 into develop Oct 15, 2021
@kryptoklob kryptoklob 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