gov: simplify governance-controlled parameter changes #4319
Conversation
hdevalence
changed the title
hdevalence
added
the
consensus-breaking
This commit refactors the governance component to dramatically simplify the way that governance-controlled parameter changes are performed. Previously, the governance component duplicated all of the parameter data of all of the other components, which had to be kept in sync in a complicated and error-prone way. Now, parameter changes are specified and performed as key-value edits: - A parameter change proposal specifies a list of `(component, key, value)` pairs - The governance component can apply these without knowledge of other components' structures by operating on a `serde_json::Value` - The application can validate the changed parameters by deserializing a complete `AppParameters` from ProtoJSON. The logic around applying these changes is also dramatically simplified. The top-level app is responsible for all parameter changes. Components do not need to track their own individual "dirty bit" for parameter changes. The governance component can report to any other component when a parameter change was scheduled to occur. In followup work, we should change the `pcli` interface to work with JSON proposal data and delete the `ProposalToml` entirely.
The existing logic didn't balance delegator vote transactions properly. This commit is the first step to fixing it. The purpose of allowing the `Planner` to have an async API was so that it could fetch data directly from the view service, rather than having to implement our own async logic at the application level (by saving stuff we want to query later). However, the new planner logic doesn't seem to be correct, because adding the `DelegatorVotePlan` gives an error that there were no actions to balance.
|
It's not actually possible to make a delegator vote to test it, however, so it's not yet good to merge. The planner needs to be fixed to make this possible, this was a recent regression. I am working on that now. |
The part of the web planner refactoring we wanted to bring over to the core repo was the creation of a separate `ActionList` modeling a list of actions that may or may not be balanced. This commit pulls the planner into that shape, and extends the `ActionList` design found in the web code in a few ways: - The `ActionList` tracks a target fee internally, rather than leaving that to its caller. This allows us to handle cases like a swap claim, where we want to put the prepaid fee released by a swap claim towards the transaction fee, and not create dust outputs. - The `ActionList` has the groundwork for supporting multi-asset fee payments, by not assuming that the fee is always in the staking token when accounting for fees. Later we can add an asset ID to the `GasPrices` and it should be a relatively drop-in extension. - The `ActionList` can be used to create a `TransactionPlan` directly. In the future, we should consider restructuring the `view` crate to have most of its current contents behind a `svc` flag, allowing us to depend on the `view` crate from the wasm code and have a common `ActionList` impl between them, just different code that manipulates it. Various API cleanups were necessary along the way.
|
I restructured the The part of the web planner refactoring we wanted to bring over to the core
Various API cleanups were necessary along the way. cc @TalDerei -- might be interesting to you, sorry we had confusion about the goals there. |
|
I tested this code on a local devnet and was able to freely adjust parameters (and had all actions have correctly priced fees, too!). |
|
Delegator/validator voting is supported by the planner thanks to @TalDerei quick follow-up |
|
Ah I hadn’t looked at the planner rerefactor, so I actually don’t know |
When I tried using it to exercise parameter changes there were balance errors, but I was able to build further on the cleaned-up code and get through it reasonably easily, so it was a great groundwork. |
|
Can I propagate the planner changes to the web? Currently reviewing the changeset in this PR |
|
No, I’d rather make a plan of how to share code and get the plan right first, before we do more work. |
Describe your changes
This commit refactors the governance component to dramatically simplify the way
that governance-controlled parameter changes are performed. Previously, the
governance component duplicated all of the parameter data of all of the other
components, which had to be kept in sync in a complicated and error-prone way.
Now, parameter changes are specified and performed as key-value edits:
(component, key, value)pairsserde_json::ValueAppParametersfrom ProtoJSON.The logic around applying these changes is also dramatically simplified. The
top-level app is responsible for all parameter changes. Components do not need
to track their own individual "dirty bit" for parameter changes. The governance
component can report to any other component when a parameter change was
scheduled to occur.
Another crucial improvement is that this code allows different parameters to be changed by different proposals, because changes are made to individual parameters, not to entire subsections.
In followup work, we should change the
pcliinterface to work with JSONproposal data and delete the
ProposalTomlentirely.Issue ticket number and link
None
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: