Deprecate x/params

[aaronc]

When we released Stargate one thing we knew we hadn't migrated to Protobuf was x/params state and we knew it was complex (#6983). Since then, what to do about x/params has sort of sat there as a can of worms we know we need to get to some day but with no clear solution.

In the meantime, a number of issues have come up related to x/params. There is broad support for #9245, but a sticking point is how to update existing state when the param changes. So the idea of hooks to listen for param changes has come up. At Regen, we attempted to add a param that required comparison to the existing param value for validation (the param can only increase not decrease), but there is no API in x/params to do this. In Osmosis, an emergency upgrade was required because a proposal passed which set the minimum gov deposit to the non-exist denom osmo (should have been uosmo). Again, there is no such hook for validation logic in x/params.

The solution to this might actually be quite simple. Rather than patching the dynamic, ad-hoc JSON data model of x/params with a set of validation and event hooks to handle every conceivable use case of a generic ad-hoc governance-controlled JSON param store, maybe we just shouldn’t use x/params.

Alternatively, all of the above params could have been coded as unique gov proposal types with their own proposal handlers and state stored in the module which needs them. Then, how to handle stateful validation and state changes based on param changes would be a non-issue. Also, we wouldn’t need to worry about the fact that the underlying data model is basically untyped JSON - we would just use proto messages for each operation. This might be a bit more state logic in each module, but it’s not as if setting up all the KeyTable stuff is super straightforward either and all modules expose query methods for their params anyway.

With #9438, this becomes possibly simpler because we just need to set up Msg handlers that only allow root to execute them. In #9438, we do need to add a MsgSetParam to handle existing uses of x/params. But it may be worth considering just dumping x/params usage within SDK modules and handling each case separately and correctly on its own.

[alexanderbez]

I'm generally in favor of the proposal to deprecate x/params (sorry @mossid). The like the idea of turning parameter changes into concrete messages with their respective handlers allowing them to be stateful and essentially covering all major use-cases.

The only main drawback I can think of is that as of right now, users get parameter management along with parameter changes via governance proposals essentially for "free". With this proposal, module and app devs will now have to management message types and their handlers explicitly. But I think this is OK if parameter changes can be handled in groups or sets, e.g. a module could have 12 or more parameters, it would be silly to have a unique message and handler for all of them, so we should strive to make the UX as easy as possible.

I'd be more than happy to take ownership of this effort.

[aaronc]

The only main drawback I can think of is that as of right now, users get parameter management along with parameter changes via governance proposals essentially for "free". With this proposal, module and app devs will now have to management message types and their handlers explicitly. But I think this is OK if parameter changes can be handled in groups or sets, e.g. a module could have 12 or more parameters, it would be silly to have a unique message and handler for all of them, so we should strive to make the UX as easy as possible.

Possibly some helper functions could be useful here. Maybe even just https://pkg.go.dev/google.golang.org/protobuf@v1.27.1/proto#Merge (assuming it ignores unset fields in src which I think is the case)

[robert-zaremba]

Parameter set makes sense. Selective update can implemented using nullable types - all struct fields can be null, and we only update non null values.

[Terra854]

Any progress on this? Cause past activity is a month ago and it seems that this needs to be done before #9245 can be implemented.

[alexanderbez]

I imagine It'll be deprecated for a release or two and removed eventually maybe.

[Terra854]

So are we deprecating it with this ADR?

[alexanderbez]

Yes.

[Terra854]

In that case, we should add in the title that we are deprecatinng x/params so that we can give developers some time to prepare for the eventual removal

[alexanderbez]

I don't think adding it to the title makes sense. It's in the ADR body. The title should be concise ;-)

[jackzampolin]

Just reading over this it seems like a rather large breaking change that will require painful upgrade for SDK chains with relatively small benefit for those chains. All this does is just force a different pattern for params. Why don't we just maintain the existing params stuff and have this new path as an additive change?

[alexanderbez]

Just reading over this it seems like a rather large breaking change that will require painful upgrade for SDK chains with relatively small benefit for those chains.

No this is not the case. x/params will still exist in deprecated form. Devs can opt-in to using the new parameter method, which IMO, is not terribly difficult. Most users will just need to define a method handler, a method to save/get and that's it.