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.

Sign up for GitHub

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

Jump to bottom

improve fellowshipCore.setParams and related code #3617

Open
4 tasks
xlc opened this issue Mar 8, 2024 · 2 comments · May be fixed by #5159
Open
4 tasks

improve fellowshipCore.setParams and related code #3617

xlc opened this issue Mar 8, 2024 · 2 comments · May be fixed by #5159
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. T2-pallets This PR/Issue is related to a particular pallet.

Comments

@xlc
Copy link
Contributor

xlc commented Mar 8, 2024 •
edited
Loading

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
@kianenigma kianenigma added the T2-pallets This PR/Issue is related to a particular pallet. label Mar 8, 2024
@ggwpez ggwpez added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I4-refactor Code needs refactoring. labels Mar 18, 2024
@chungquantin chungquantin mentioned this issue Mar 26, 2024
Merged
2 tasks
@chungquantin
Copy link
Contributor

@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?

github-merge-queue bot pushed a commit that referenced this issue Jun 18, 2024
@chungquantin @seadanda
@ggwpez @bkchr
Add set_partial_params dispatchable function (#3843)
cc38713 
# ISSUE
- Link to issue: #3617
# Description
> 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.
- [ ] Concurrent call & race condition testing
- [x] Each argument of the `ParamsType` is an `Option` field. Introduce
through
```rust
pub type PartialParamsOf<T, I> =
		ParamsType<Option<<T as Config<I>>::Balance>, Option<BlockNumberFor<T>>, RANK_COUNT>;
```
# Outcome
```rust
let params = ParamsType {
	active_salary: [None; 9],
	passive_salary: [None; 9],
	demotion_period: [None, Some(10), None, None, None, None, None, None, None],
	min_promotion_period: [None; 9],
	offboard_timeout: Some(1),
};
CoreFellowship::set_partial_params(signed(2), Box::new(params.clone())),
```
Test coverage
```diff
running 21 tests
test tests::unit::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
test tests::unit::basic_stuff ... ok
test tests::integration::test_genesis_config_builds ... ok
test tests::integration::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
test tests::unit::auto_demote_offboard_works ... ok
test tests::unit::auto_demote_works ... ok
test tests::unit::get_salary_works ... ok
test tests::unit::active_changing_get_salary_works ... ok
test tests::integration::swap_bad_noops ... ok
test tests::unit::promote_postpones_auto_demote ... ok
test tests::unit::infinite_demotion_period_works ... ok
test tests::unit::proof_postpones_auto_demote ... ok
test tests::unit::induct_works ... ok
test tests::unit::set_params_works ... ok
test tests::unit::test_genesis_config_builds ... ok
test tests::unit::offboard_works ... ok
test tests::unit::sync_works ... ok
+ test tests::unit::set_partial_params_works ... ok
test tests::integration::swap_exhaustive_works ... ok
test tests::unit::promote_works ... ok
test tests::integration::swap_simple_works ... ok

test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

   Doc-tests pallet_core_fellowship

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
```

polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
@marcuspang marcuspang linked a pull request Jul 26, 2024 that will close this issue
Draft
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
@chungquantin @seadanda
@ggwpez @bkchr @TarekkMA
Add set_partial_params dispatchable function (paritytech#3843)
595a0cd 
# ISSUE
- Link to issue: paritytech#3617
# Description
> 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.
- [ ] Concurrent call & race condition testing
- [x] Each argument of the `ParamsType` is an `Option` field. Introduce
through
```rust
pub type PartialParamsOf<T, I> =
		ParamsType<Option<<T as Config<I>>::Balance>, Option<BlockNumberFor<T>>, RANK_COUNT>;
```
# Outcome
```rust
let params = ParamsType {
	active_salary: [None; 9],
	passive_salary: [None; 9],
	demotion_period: [None, Some(10), None, None, None, None, None, None, None],
	min_promotion_period: [None; 9],
	offboard_timeout: Some(1),
};
CoreFellowship::set_partial_params(signed(2), Box::new(params.clone())),
```
Test coverage
```diff
running 21 tests
test tests::unit::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
test tests::unit::basic_stuff ... ok
test tests::integration::test_genesis_config_builds ... ok
test tests::integration::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
test tests::unit::auto_demote_offboard_works ... ok
test tests::unit::auto_demote_works ... ok
test tests::unit::get_salary_works ... ok
test tests::unit::active_changing_get_salary_works ... ok
test tests::integration::swap_bad_noops ... ok
test tests::unit::promote_postpones_auto_demote ... ok
test tests::unit::infinite_demotion_period_works ... ok
test tests::unit::proof_postpones_auto_demote ... ok
test tests::unit::induct_works ... ok
test tests::unit::set_params_works ... ok
test tests::unit::test_genesis_config_builds ... ok
test tests::unit::offboard_works ... ok
test tests::unit::sync_works ... ok
+ test tests::unit::set_partial_params_works ... ok
test tests::integration::swap_exhaustive_works ... ok
test tests::unit::promote_works ... ok
test tests::integration::swap_simple_works ... ok

test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

   Doc-tests pallet_core_fellowship

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
```

polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
@PolkadotDom
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: reduce coupling between salary and core-fellowship pallet marcuspang/polkadot-sdk
5 participants
@xlc @kianenigma @ggwpez @chungquantin @PolkadotDom