Joystream / joystream

Joystream Monorepo
http://www.joystream.org
GNU General Public License v3.0
1.42k stars 115 forks source link

Reorganizing parameters: mutable, configurable and unified #4225

Open bedeho opened 2 years ago

bedeho commented 2 years ago

Background

We have a large number of literal constants across

littered across the code base which impact business logic. To be clear, by this I mean a literal value in the Rust code which itself can only be changed or removed with a runtime upgrade or a fresh network. I am here not talking purely about numerical constants, although they constitute the majority, I really mean any literal value. There are a few problems with them being organised this way.

Proposal

A very attractive part of making parameter values configurable is that for some it may be important to harmonize or synchronize the way they are set with other values in GenesisConfig for some pallet, and making all of them originate from the chain spec building process makes that much easier to enforce properly.

┆Issue is synchronized with this Asana task by Unito

bedeho commented 2 years ago

Before we start this, it may be wise to actually map out across all pallet values which ones are safe to mutate, and which ones are not, in particular for built in pallets. This may take substantial amount of time.

bedeho commented 2 years ago

I am wondering whether this is actually too risky and complex to do now before release, there may be too many edge cases, and its not a must now, it mostly is a must for #3626, and otherwise is just a nice cleanup making it easy to launch new chains, which won't be important until next runtime upgrade at least. Here is one problem that popped up.

It may be that some pallets have "constants" defined through pallet:constants, and even if the business logic of the pallet allows it, its [unclear if its wise or safe to actually mutate such values in the absence of runtime upgrades](https://substrate.stackexchange.com/questions/4556/are-palletconstants-really-constant, e.g. a proposal. If it is not then this would be a second family of values which could not be mutated. So now we have these two groups of pallet values (mutation not safe + mutation not possible due to pallet:constants), and they may both need to have values which are coordinated by other exogenous values, such as implied market cap or something else. Now, when you update the exogenous values, you will be automatically updating lots of pallet values which work fine, but you will have this other category of values which will be stuck with stale inputs so to speak. So now you have to remember that whenever certain parameter upgrade proposals are made, you have to do runtime upgrades very quickly after to make sure all values are in harmony across all pallets. This is bound to go wrong.

If this analysis applies even to a few cases, then it may make this whole approach infeasible, or at least, we have to be very careful and systematic about what exact values can be made mutable.

bedeho commented 2 years ago

Update on mutability of pallet::constants

While in principle they can change, and this change will even be reflected in the metadata across RPC calls, despite no runtime upgrade, folks at Parity are adamant that we don't do this, so we should not. The implication here is that we pretty much have to abandon mutable parameters for all pallets we dont control, because otherwise we would have to fork them all and change the parameter to no longer be a pallet::constant. Perhaps that could make sense in the future, but it would be a risky undertaking, so it would have to be done very carefully, and also a very detailed examination of what parameters are truly safe to mutate anyway, which is very time intensive to determine in many cases. It also implies we should in general avoid using pallet::constant unless we know the value has to actually be constant in a pallet.

bedeho commented 1 year ago

Removing from mainnet scope, too many unknowns here.