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

AssestReserve's governed APIs are mis-configured #7072

Closed
Chris-Hibbert opened this issue Feb 24, 2023 · 1 comment
Closed

AssestReserve's governed APIs are mis-configured #7072

Chris-Hibbert opened this issue Feb 24, 2023 · 1 comment
Assignees
Labels
bug Something isn't working Inter-protocol Overarching Inter Protocol vaults_triage DO NOT USE Vaults VaultFactor (née Treasury)
Milestone

Comments

@Chris-Hibbert
Copy link
Contributor

Describe the bug

While refactoring AssetReserve to remove AMM dependencies (#7055), I became suspicious of the way the reserve configures its governed APIs. After the removal the only governed API will be burnFeesToReduceShortfall.

I think there's a problem in contractHelper.js. makeFarGovernorFacet takes governedApis as a parameter, but makeGovernorFacet does not, which means it can't add getGovernedApis() or getGovernedApiNames() to the creatorFacet. In addition, AssetReserve doesn't include governedApis in its call to makeVirtualGovernorFacet. fluxAggregatorContract.js shows an example of how to do it.

To Reproduce

test-reserve verifies that burnFeesToReduceShortfall can be called, but it's using the puppetContractGovernor, which cheats. I verified it using test-gov-collateral, which configures a real AssetReserve. I inserted a debugger statement in contractGovernor.js to verify that in initApiGovernance getGovernedNames returns ["burnFeesToReduceShortfall"], but governedApis` is empty.

Expected behavior

governance should be able to invoke burnFeesToReduceShortfall

@Chris-Hibbert Chris-Hibbert added bug Something isn't working Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury) labels Feb 24, 2023
@Chris-Hibbert Chris-Hibbert self-assigned this Feb 24, 2023
@ivanlei ivanlei added this to the Vaults EVP milestone Feb 27, 2023
@ivanlei ivanlei added the vaults_triage DO NOT USE label Mar 2, 2023
@Chris-Hibbert
Copy link
Contributor Author

I don't know whether I was confused before, or whether this was repaired in a way I haven't found by looking through the file history, but there's nothing wrong here.
When I do the same thing that's described under To Reproduce, I can see that burnFeesToReduceShortfall is present both in governedApis and getGovernedNames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Inter-protocol Overarching Inter Protocol vaults_triage DO NOT USE Vaults VaultFactor (née Treasury)
Projects
None yet
Development

No branches or pull requests

2 participants