improve fellowshipCore.setParams and related code

[xlc]

Issues identified so far:

• Any set parameter / update config call with multiple arguments should have each argument to be an Option field. Please put this to some best practice document. This allows new update config call does not need to duplicate the fields that does not need to update. It also makes concurrent votes of update call possible, otherwise there will be race condition. It also helps with review such proposal otherwise reviewers need to check the other fields should remain the same.
• Right now the fellowshipCore pallet and the salary pallet are tightly coupled in an unexpected way. On each cycle, the salary pallet will ask the fellowshipCore pallet about how much amount to pay and such amount is configured in the fellowshipCore pallet. When setting the parameters, we need to know the cycle length, which is configured in the salary pallet. And then if we adjust the cycle length, we also have to adjust the fellowshipCore params. This is just not a good thing.
  • Two possible solutions:
    • Move the payout cycle related config to the fellowshipCore pallet so they are next to the payout amount code and then we can more easier to figure out they are tightly coupled code
    • New config in the fellowshipCore pallet to specify the meaning of the parameters. i.e. payout amount per duration. And then the salary can use the duration from fellowshipCore and its own payout cycle to calculate the payout amount per cycle.
• It assumes block number is 12s, which is not async backing friendly. This may or may not be tracked elsewhere.

Good thing identified:

• The budget feature is a helpful guard to cap the max damage to bad config.

Tasks

Preview Give feedback

1. New extrinsic to allow update each individual parameter
2. Refactor payout cycle realted code so it is harder to make mistake
3. Make it async backing friendly
4. Document best practice about set parameter calls

[chungquantin]

@xlc I created the new PR to handle the first bullet point: #3843. My approach is creating a new dispatchable function instead of overwriting the current set_params method. About weight for the new set_partial_params method, as the method parameter is smaller in size, is it significant to introduce a new weight for this new method?

[PolkadotDom]

I'll pick up the 'make it async backing friendly' 👍

And here's a task list:

• Add block number provider to westend Add block provider to core-fellowship #6978
• Migrate Westend collectives to new config Add block provider to core-fellowship #6978
• Migrate Polkadot collectives to new config
• Add block number provider to salary pallet Add BlockNumberProvider to salary pallet config #7000
• Migrate westend collectives to new config Add BlockNumberProvider to salary pallet config #7000
• Migrate Polkadot collectives to new config

[PolkadotDom]

@xlc @kianenigma My suggestion for the salary, core-fellowship pallet coupling would be to change the GetSalary trait to something like GetSalaryPerBlock. Then set rank salaries based on how much they should be paid per block. In this way the amount accrued in salary over time is independent of the cycle length. With the cycle length just determining how long they need to wait between payments. Will move forward upon approval.

Wasn't there for the initial structuring of the salary pallet, but I wonder if in general it would be better to design it more as a consistent stream of income that one can claim at their discretion, rather than a paged salary. More important issues for now but wanted to jot it down somewhere.

[xlc]

I will prefer to make it time based instead of block based. i.e. GetSalaryPerPeriod and PeriodDurationSeconds

but I wonder if in general it would be better to design it more as a consistent stream of income that one can claim at their discretion, rather than a paged salary. More important issues for now but wanted to jot it down somewhere.

I can see some advantages but one of the benefits of the current implementation is budgeting. We can easily limit the max payout per cycle and that applies to every registered payouts in this cycle (i.e. not first come first serve).