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

Ability to override the ProvisionPool governedParam PerAccountInitialAmount #10562

Open
Jorge-Lopes opened this issue Nov 25, 2024 · 5 comments · May be fixed by #10638
Open

Ability to override the ProvisionPool governedParam PerAccountInitialAmount #10562

Jorge-Lopes opened this issue Nov 25, 2024 · 5 comments · May be fixed by #10638
Assignees
Labels
enhancement New feature or request

Comments

@Jorge-Lopes
Copy link
Collaborator

Jorge-Lopes commented Nov 25, 2024

What is the Problem Being Solved?

The ProvisionPool contract seems to not provide the ability to override the governed parameter PerAccountInitialAmount during a contract upgrade, which means that it will reset to the default value and discard any value previously changed by the economicCommittee.

Description of the Design

  • Option 1: Update the ProvisionPool source code
    • Allow overriding of governedParams during upgrades by passing PerAccountInitialAmount via privateArgs.
    • Utilize the makeParamManagerFromTerms method from the governance package to set or retain the desired parameter value.
  • Option 2: Parameter Change Proposal Preceding Upgrade
    • After initiating any core-eval for upgrading the ProvisionPool vat, execute a Parameter Change Proposal to restore PerAccountInitialAmount to the desired value.

Security Considerations

I am unclear on the impact of resetting PerAccountInitialAmount on the Agoric chain and Inter-Protocol. Further analysis is needed to assess potential risks or disruptions this might cause

Scaling Considerations

No consideration

Test Plan

Leverage the a3p-integration acceptance test for governance that is included in PR #10555 , which has a test case that verifies that the governance node of ProvisionPool retains its parameter values after a null upgrade.

Upgrade Considerations

No consideration

@Jorge-Lopes Jorge-Lopes added the enhancement New feature or request label Nov 25, 2024
@Jorge-Lopes
Copy link
Collaborator Author

cc: @Chris-Hibbert

@Chris-Hibbert
Copy link
Contributor

In VaultFactory, we pass parameters called directorParamOverrides and managerParams via privateArgs. The first is then passed to makeParamManagerFromTerms to override the values originally passed in terms. The second is passed through to provideVaultParamManagers, where it's processed as managerParamOverrides. Use either of those as the model here.

Further analysis is needed to assess potential risks or disruptions this might cause

I think the EC has not changed the value of this parameter in the running contract. That means, this time around, that resetting the value to its default wouldn't have any effect. We still want to set up the upgradable code so that if the EC does change it, the new value stays set through any subsequent upgrade.

@Jorge-Lopes
Copy link
Collaborator Author

Jorge-Lopes commented Nov 28, 2024

To ensure that , after the contract upgrade, the provisionPool public facet exposes the same methods as it did in the previous incarnation, I included all method declared in the publicMixinAPI methodGuard.

However, I am reconsidering whether it makes sense to include all these methods—particularly those declared in typedAccessors. These methods might not be required by the contracts that consume the provisionPool public facet, potentially adding unnecessary complexity or "noise" to the interface.

Question:
Should all methods, including those from typedAccessors, remain exposed in the provisionPool public facet?

@Jorge-Lopes
Copy link
Collaborator Author

As part of the core-eval proposal required to upgrade provisionPool, we need to fetch the current value of the governed parameter, specifically PerAccountInitialAmount. This value must be included in the privateArgs to ensure it is overridden and does not reset to the default value during the upgrade.

From what I understand, we have two methods from the provisionPool public facet that can be used for retrieving the PerAccountInitialAmount value:

  • getSubscription
  • getGovernedParams

Question:
Is there a preferred method for fetching this value? If so, what is the rationale?

@Chris-Hibbert
Copy link
Contributor

Question:
Should all methods, including those from typedAccessors, remain exposed in the provisionPool public facet?

Definitely not necessary. You do have to check existing code to ensure that anything that will get called remains, but if all callers will be updated with the upgrade, it's fine to drop methods that won't be used going forward. If there might be UIs or other tools that use the methods, you have to tread very carefully. Usually with two separately-release tools, you have to do a multi-release dance to remove usages from the caller before reming them from the service.

Question:
Is there a preferred method for fetching this value? If so, what is the rationale?

We've used both methods in various places when one or the other was blocked or recalcitrant. I would say that looking them up from governance is more general, and more likely to be the common path that works consistently for most upgrades.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment