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.

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

Add set_partial_params dispatchable function #3843

Merged

Conversation

chungquantin
Copy link
Contributor

@chungquantin chungquantin commented Mar 26, 2024

ISSUE

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
  • Each argument of the ParamsType is an Option field. Introduce through
pub type PartialParamsOf<T, I> =
		ParamsType<Option<<T as Config<I>>::Balance>, Option<BlockNumberFor<T>>, RANK_COUNT>;

Outcome

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

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

substrate/frame/core-fellowship/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/core-fellowship/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/core-fellowship/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 18, 2024 01:38
@chungquantin
Copy link
Contributor Author

@ggwpez I have resolved your feedback. CI passes as well.

@chungquantin chungquantin requested a review from xlc April 18, 2024 11:16
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6373987

@chungquantin
Copy link
Contributor Author

chungquantin commented Jun 4, 2024

@xlc Apologize for tagging you, I am trying to wrap up this PR but there is an issue with CI that I don't know how to reproduce.

Below is the result of the benchmark tests I ran:
Screenshot 2024-06-04 at 15 30 46

@ggwpez ggwpez added the T2-pallets This PR/Issue is related to a particular pallet. label Jun 15, 2024
substrate/frame/core-fellowship/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr added this pull request to the merge queue Jun 18, 2024
Merged via the queue into paritytech:master with commit cc38713 Jun 18, 2024
153 of 157 checks passed
@chungquantin chungquantin deleted the chungquantin/fellowship_core_set_params branch June 18, 2024 15:49
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
# 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 <donalm@seadanda.dev>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants