Alliance pallet: retirement notice call by muharem · Pull Request #11970 · paritytech/substrate

muharem · 2022-08-03T15:21:00Z

Fixes #11936

Current implementation of retirement has a bug, which makes it impossible to retire for a member if there was a not enacted proposal to kick the member.

In the PR we introducing a new retirement_notice call, required to be executed by a member before retiring.
The notice will start a 'retirement period' that has to pass in order to retire.
The required period prevents a misbehaving member to retire before it will be kicked and slashed.

The PR does not cover next use cases:

if alliance does not execute the voted proposal to kick a misbehaving member, the member can retire without being slashed;
a member can not revoke the retirement process if retirement notice was given. It must retire and join alliance again if desires;

Cumulus companion: paritytech/cumulus#1573

joepetrowski · 2022-08-03T16:17:26Z

bin/node/runtime/src/lib.rs

 	pub const MaxFellows: u32 = AllianceMaxMembers::get() - MaxFounders::get();
 	pub const MaxAllies: u32 = 100;
 	pub const AllyDeposit: Balance = 10 * DOLLARS;
+	pub const RetirementPeriod: BlockNumber = ALLIANCE_MOTION_DURATION + (1 * DAYS);


This is fine in the node template but when we introduce a Root call to force dissolve the Alliance, we should make this longer than the root voting period.

Adding a bit to this, I think it's OK for the retirement period to be really long (e.g. >3 months). We expect Alliance members to be quite committed to the network and they shouldn't retire just because they want to unreserve their DOT the way that someone might unbond their DOT.

frame/alliance/src/lib.rs

joepetrowski · 2022-08-04T05:50:00Z

frame/alliance/src/lib.rs

+	#[pallet::getter(fn retiring_members)]
+	pub type RetiringMembers<T: Config<I>, I: 'static = ()> =
+		StorageMap<_, Blake2_128Concat, T::AccountId, T::BlockNumber, OptionQuery>;


Mostly just curious, why not store this in the MemberRole variant itself (i.e. Retiring(T::BlockNumber))? I'm not sure if we have FRAME guidance on which is more idiomatic or when one method is better than another for a given use case (@kianenigma or @shawntabrizi ?).

A storage item makes it easier for UIs to query all retiring members, but they could also periodically just check all the members and build that info from there.

Right now there is one storage map to store all members, where the key is a member role and value is map of AccountIds (Members).
And this additional map to store the BlockNumber for retirement period.

muharem · 2022-08-04T09:26:38Z

/bench pallet pallet_alliance

ggwpez · 2022-08-04T09:30:20Z

/cmd queue -c bench-bot $ pallet dev pallet_alliance

@muharem the syntax is here paritytech/pipeline-scripts#63

command-bot · 2022-08-04T09:30:25Z

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1719871 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 4-d46fb067-71de-480e-a40e-58ff9f432176 to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot · 2022-08-04T09:49:26Z

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1719871 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1719871/artifacts/download.

muharem · 2022-08-04T12:57:22Z

/cmd queue -c bench-bot $ pallet dev pallet_alliance

command-bot · 2022-08-04T12:57:27Z

@muharem https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721126 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 6-ee0c4611-5302-4ae0-a583-b15a6cc45272 to cancel this command or /cmd cancel to cancel all commands in this pull request.

… HEAD

ggwpez · 2022-08-04T13:02:48Z

/cmd queue -c bench-bot $ pallet dev pallet_alliance

The benchbot does not push anything if you change the branch in between.

command-bot · 2022-08-04T13:02:52Z

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721128 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 7-6de5d2ca-1b3f-438c-adc2-bc15050571b3 to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot · 2022-08-04T13:18:23Z

@muharem Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721126 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721126/artifacts/download.

command-bot · 2022-08-04T13:39:11Z

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721128 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1721128/artifacts/download.

frame/alliance/src/lib.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

frame/alliance/src/lib.rs

frame/alliance/src/mock.rs

frame/alliance/src/lib.rs

ruseinov · 2022-08-24T09:54:04Z

frame/alliance/src/lib.rs

+			Self::add_member(&who, MemberRole::Retiring)?;
+			<RetiringMembers<T, I>>::insert(
+				&who,
+				frame_system::Pallet::<T>::block_number() + T::RetirementPeriod::get(),


saturating_add might be a good idea here just to be safe as @bkontur mentioned above.

ruseinov · 2022-08-24T09:59:41Z

Mostly looks good, but needs some minor fixes for clarity it seems.

bkontur

lgtm,
I would just vote for renaming those two constants:
MOTION_DURATION -> MOTION_DURATION_IN_BLOCKS
ALLIANCE_MOTION_DURATION -> ALLIANCE_MOTION_DURATION_IN_BLOCKS

muharem · 2022-08-26T11:15:44Z

/cmd queue -c bench-bot $ pallet dev pallet_alliance

command-bot · 2022-08-26T11:15:48Z

@muharem https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1779299 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 35-a8a5785a-c81c-40e5-b220-2c0876aaf06e to cancel this command or /cmd cancel to cancel all commands in this pull request.

… HEAD

command-bot · 2022-08-26T11:58:09Z

@muharem Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1779299 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1779299/artifacts/download.

ruseinov

lgtm, one minor nit related to a comment

ruseinov · 2022-08-26T11:59:45Z

frame/alliance/src/lib.rs

 		type WeightInfo: WeightInfo;

-		/// The period required to pass from retirement notice for a member to retire.
+		/// The number of blocks a member must wait between giving a retirement notice and retiring.


nit: wait for*

* add negative tests * remove tests powerless asserts checking against announcment origin * assert bad origin from announcement origin checks Co-authored-by: muharem <ismailov.m.h@gmail.com>

muharem · 2022-08-29T16:02:15Z

bot merge

* Alliance pallet: retirement notice * add alliance pallet to benchmark list for dev chain * fix type * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * link weight generated by bench for retirement_notice method * migration to clear UpForKicking storage prefix * rename migration from v1 to v0_to_v1 * Apply suggestions from code review Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * rename `retirement-notice to give-retirement-notice * Apply suggestions from code review Co-authored-by: Squirrel <gilescope@gmail.com> * review fixes: update doc, saturating add, BlockNumber type alias * add suffix to duratin consts *_IN_BLOCKS * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * add negative tests (paritytech#11995) * add negative tests * remove tests powerless asserts checking against announcment origin * assert bad origin from announcement origin checks Co-authored-by: muharem <ismailov.m.h@gmail.com> Co-authored-by: command-bot <> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Squirrel <gilescope@gmail.com>

Alliance pallet: retirement notice

bb091a2

github-actions bot added the A0-please_review Pull request needs code review. label Aug 3, 2022

muharem added B3-apinoteworthy C3-medium PR touches the given topic and has a medium impact on builders. labels Aug 3, 2022

joepetrowski reviewed Aug 3, 2022

View reviewed changes

joepetrowski reviewed Aug 4, 2022

View reviewed changes

muharem added 2 commits August 4, 2022 12:17

add alliance pallet to benchmark list for dev chain

decd293

fix type

7b09b07

Merge branch 'master' of https://github.com/paritytech/substrate into…

c2b5c71

… HEAD

paritytech deleted a comment from command-bot bot Aug 4, 2022

".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

524988b

".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

ea78b19

link weight generated by bench for retirement_notice method

37ff3c2

muharem requested review from kianenigma and shawntabrizi August 5, 2022 09:40

muharem removed the A0-please_review Pull request needs code review. label Aug 9, 2022

muharem added E5-breaksapi D2-breaksapi labels Aug 16, 2022

muharem requested a review from ruseinov August 16, 2022 12:52

gilescope reviewed Aug 17, 2022

View reviewed changes

frame/alliance/src/lib.rs Outdated Show resolved Hide resolved

gilescope reviewed Aug 17, 2022

View reviewed changes

frame/alliance/src/lib.rs Show resolved Hide resolved

gilescope approved these changes Aug 17, 2022

View reviewed changes

Apply suggestions from code review

114e94b

Co-authored-by: Squirrel <gilescope@gmail.com>

bkontur reviewed Aug 24, 2022

View reviewed changes

frame/alliance/src/lib.rs Outdated Show resolved Hide resolved

bkontur reviewed Aug 24, 2022

View reviewed changes

frame/alliance/src/mock.rs Outdated Show resolved Hide resolved

bkontur reviewed Aug 24, 2022

View reviewed changes

frame/alliance/src/lib.rs Show resolved Hide resolved

ruseinov reviewed Aug 24, 2022

View reviewed changes

review fixes: update doc, saturating add, BlockNumber type alias

4786244

muharem requested review from bkontur and ruseinov August 24, 2022 16:43

bkontur approved these changes Aug 26, 2022

View reviewed changes

add suffix to duratin consts *_IN_BLOCKS

090a179

muharem mentioned this pull request Aug 26, 2022

Collectives runtime: alliance pallet upgrade introducing new retirement notice feature paritytech/cumulus#1573

Merged

command-bot added 2 commits August 26, 2022 11:37

Merge branch 'master' of https://github.com/paritytech/substrate into…

f031402

… HEAD

".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

69f0802

ruseinov approved these changes Aug 26, 2022

View reviewed changes

add negative tests (#11995)

23bdc2e

* add negative tests * remove tests powerless asserts checking against announcment origin * assert bad origin from announcement origin checks Co-authored-by: muharem <ismailov.m.h@gmail.com>

paritytech-processbot bot merged commit 5cdb054 into master Aug 29, 2022

paritytech-processbot bot deleted the pallet-alliance-retirement-notice branch August 29, 2022 16:02

librelois mentioned this pull request Sep 15, 2022

Update substrate/polkadot/cumulus from v0.9.26 to v0.9.29 moonbeam-foundation/moonbeam#1814

Closed

2 tasks

Conversation

muharem commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joepetrowski Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

joepetrowski Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joepetrowski Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muharem Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

muharem commented Aug 4, 2022

Uh oh!

ggwpez commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 4, 2022

Uh oh!

muharem commented Aug 4, 2022

Uh oh!

command-bot bot commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggwpez commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 4, 2022

Uh oh!

command-bot bot commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ruseinov Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

muharem Aug 24, 2022

Choose a reason for hiding this comment

Uh oh!

ruseinov commented Aug 24, 2022

Uh oh!

bkontur left a comment

Choose a reason for hiding this comment

Uh oh!

muharem commented Aug 26, 2022

Uh oh!

command-bot bot commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 26, 2022

Uh oh!

ruseinov left a comment

Choose a reason for hiding this comment

Uh oh!

ruseinov Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

muharem commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

muharem commented Aug 3, 2022 •

edited

Loading

joepetrowski Aug 4, 2022 •

edited

Loading

ggwpez commented Aug 4, 2022 •

edited

Loading

command-bot bot commented Aug 4, 2022 •

edited

Loading

command-bot bot commented Aug 4, 2022 •

edited

Loading

ggwpez commented Aug 4, 2022 •

edited

Loading

command-bot bot commented Aug 4, 2022 •

edited

Loading

command-bot bot commented Aug 26, 2022 •

edited

Loading