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

Renamed Vault functions and events with Ousd in them #1629

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

naddison36
Copy link
Collaborator

@naddison36 naddison36 commented Jun 15, 2023

This is an optional addition to the PR #1627 that refactors the vaults

It does more more of the items in #1533
Specifically, it covers removing Ousd from function names and events.

  • setOusdMetaStrategy -> setMetaStrategy
  • setNetOusdMintForStrategyThreshold -> setNetMintForStrategyThreshold
  • OusdMetaStrategyUpdated -> MetaStrategyUpdated
  • NetOusdMintForStrategyThresholdChanged -> NetMintForStrategyThresholdChanged

The two functions and corresponding events are only used by the governor so breaking the ABI is not such a big deal

@naddison36 naddison36 added the contracts Works related to contracts label Jun 15, 2023
@naddison36 naddison36 self-assigned this Jun 15, 2023
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 03:50 Inactive
@naddison36 naddison36 mentioned this pull request Jun 15, 2023
@@ -165,9 +165,9 @@ interface IVault {

function netOusdMintForStrategyThreshold() external view returns (uint256);

function setOusdMetaStrategy(address _ousdMetaStrategy) external;
function setMetaStrategy(address _strategy) external;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't plan to use this for OETH though right? This is exclusive to OUSDMetStrategy contract. the curve AMO uses mintForStrategy I believe

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It currently is on the OETHVaultAdmin hence the reason for are trying to clean it up.

There may be a better way of doing this. eg

  1. Move these OUSD specific functions to an OUSD Vault implementation contract
  2. Change the OUSD strategies to use the more generic functions.

Both sound like a bigger change for another day.

@naddison36 naddison36 force-pushed the nicka/vault-refactoring-ousd-functions branch from a61b560 to 3f6ed08 Compare June 15, 2023 03:56
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 03:56 Inactive
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 04:22 Inactive
@naddison36 naddison36 force-pushed the nicka/vault-refactoring-ousd-functions branch from 7f5997f to 7f22eb8 Compare June 15, 2023 04:29
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 04:29 Inactive
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 04:36 Inactive
@naddison36 naddison36 force-pushed the nicka/vault-refactoring-ousd-functions branch from 358dcf8 to d8a65ea Compare June 15, 2023 05:39
@rafaelugolini rafaelugolini temporarily deployed to preview-oeth-nicka-vaul-outqa8 June 15, 2023 05:40 Inactive
@DanielVF
Copy link
Member

I'd rather do these changes, later after the current OETH vault deploy

Base automatically changed from nicka/vault-refactoring to master June 16, 2023 02:04
@sparrowDom
Copy link
Member

@naddison36 do we want to pair this PR with deployment of some other Vault functionality? So we don't kick off the Vault deploys just for the naming changes?

@sparrowDom
Copy link
Member

Maybe it'b be cool to have a list of PRs that are waiting to join some other more impactful functionality to be deployed

@naddison36
Copy link
Collaborator Author

For the OETH Vault, I'd say it's best to pair with something else.
For the OUSD Vault, we need to decide if we include with 052_upgrade_ousd_vault_harvester or do after. I'm not set on either way but I'd lean toward doing after so both OETH and OUSD are on the same codebase.

@sparrowDom
Copy link
Member

Yes agreed, rolling them out for both protocols simultaneously would be nice - in respect of not complicating our environments and mental load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Works related to contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants