Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

AP-680 Limit what vault config update can change #374

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

stevieraykatz
Copy link
Contributor

Explanation of the solution

We allowed the setVaultConfig method to overwrite a number of fields that should be treated as immutable. There are now two structs:

  • VaultConfig which is stored upon construction
  • UpdateVaultConfig which is passed as an argument and only contains limited mutable fields:
  struct VaultConfigUpdate {
    address strategy;
    address registrar;
  }

Instructions on making this work

  • run yarn or yarn install to install npm dependencies

@linear
Copy link

linear bot commented Sep 11, 2023

AP-680 Change "IVault.setVaultConfig" to only accept a subset of `VaultConfig` fields to update

As per ser Steve's comment:

Fair points re: these tokens and the Vault type being editable by config updates. Perhaps we should have a concept of config and a concept of constructorArgs and they should be separate. See longer winded answer about why these fields exist here in addition to in Strategy here: AngelProtocolFinance/ap-subgraph#21 (comment)

In short, the only fields that should be updateable are: registrar address, apTokenName and apTokenSymbol (andrey katzman please confirm). Field admin should probably be removed and the APVault_V1 contract made to inherit ~OwnableUpgradeable~ Ownable (see Andrey's comment below).

@stevieraykatz stevieraykatz self-assigned this Sep 11, 2023

contract APVault_V1 is IVault, ERC4626AP {
contract APVault_V1 is IVault, ERC4626AP, Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call!

@SovereignAndrey SovereignAndrey merged commit 6101ef2 into master Sep 12, 2023
1 check passed
@SovereignAndrey SovereignAndrey deleted the AP-680-vault-config branch September 12, 2023 00:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants