Add kick_member to Society pallet#11154
Conversation
|
/cmd prdoc --audience runtime_dev --bump minor |
…time_dev --bump minor'
|
/cmd label T1-FRAME |
|
/cmd fmt |
substrate/frame/society/src/lib.rs
Outdated
| ); | ||
| let who = T::Lookup::lookup(who)?; | ||
|
|
||
| Self::slash_payout(&who, BalanceOf::<T, I>::max_value()); |
There was a problem hiding this comment.
is it intentional that we don't call unreserve_payout() unlike what we do in judge_suspended_member?
There was a problem hiding this comment.
Just wondering if the correct pattern is the one in judge_suspended_member or the one in strike_member where we also don't call unreserve_payout...
There was a problem hiding this comment.
re-reading the code... I believe not calling unreserve_payout is a bug both here and in strike_member.
In the end the scope of the PR is to give super powers to the founder to force-suspend a member so as I see it, kick_member = suspend_member(who) + judge_suspended_member(who, false)
There was a problem hiding this comment.
is it intentional that we don't call
unreserve_payout()unlike what we do in judge_suspended_member?
Yeah good point. I read the code also again and it looks like slash_payout is actually broken.
There was a problem hiding this comment.
In this case, shouldn't unreserve_payout be coupled with slash_payout every time i.e. unreserve_payout should be called inside slash_payout, at the end?
There was a problem hiding this comment.
I also think so honestly - but it's up to you if you prefer a very punctual fix for the kick_member case only in this PR and have any other fix / refactor / improvement for later or not. I am fine with both approaches.
I would be fine with a very minimal change like
let slashed = Self::slash_payout(&who, BalanceOf::<T, I>::max_value());
Self::unreserve_payout(slashed);
Self::suspend_member(&who).map(drop)or with fixing slash payout as you suggest - or going for @bkchr' suggestion to go with remove_member - all works for me.
If I have to choose one, I think the cleaner is remove_member + unreserve_payout (+ KickedMember event)
There was a problem hiding this comment.
a variant with remove_member here: cb6a9ff PTAL and let me know if it makes sense
| /// - `who`: The member to be suspended. | ||
| #[pallet::call_index(21)] | ||
| #[pallet::weight(T::WeightInfo::kick_member())] | ||
| pub fn kick_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult { |
There was a problem hiding this comment.
should we add an ad-hoc event to the list https://github.com/paritytech/polkadot-sdk/blob/kick-member-society-extrinsic/substrate/frame/society/src/lib.rs#L626-L626 or we are fine with just MemberSuspended?
There was a problem hiding this comment.
Yeah, we probably don't need to call suspend_member at all.
There was a problem hiding this comment.
@bkchr wdym? just call remove_member instead?
There was a problem hiding this comment.
My rationale here was to treat kicking the same way as a regular suspension, adding the kicked member to the SuspendedMembers list, but in this case, I guess it makes sense to simply remove straight away
There was a problem hiding this comment.
I can replace suspend_member with remove_member and add a MemberKicked event here, if it makes sense.
However, I think there are a few places where the events in this pallet can be improved. There's no event for claim_membership iirc, which is arguably the most important event. I plan to propose a set of improvements after we deal with this.
There was a problem hiding this comment.
I can take care of backporting to 2512, merge there, ask rel team to publish a new crate and finally bump the crate version in the runtime PR for 2.1.0, don't worry. No need to open upgrade proposal, it will be part of 2.1.0 (cc @ggwpez )
There was a problem hiding this comment.
also westend doesn't have society pallet so my /cmd bench errored out 😓 it would make sense to generate benchmarks for kusama only there. Here only rococo seems to support it but no point in regenerate weights there so we are good to merge I believe.
There was a problem hiding this comment.
I can take care of backporting to 2512, merge there, ask rel team to publish a new crate and finally bump the crate version in the runtime PR for 2.1.0, don't worry. No need to open upgrade proposal, it will be part of 2.1.0 (cc @ggwpez )
Okay! Do you think 2.1.0 can be enacted in the next 7 days or so? The bad actor's payout will mature in block 32,716,826. We also need 1 or 2 days of buffer ideally, so the Founder can call the extrinsic
There was a problem hiding this comment.
cc @ggwpez - the plan would be if all planets are aligned (mostly CI, especially runtime CI) to make a referendum for Kusama by end of this week. Realistically I don't think we will have the PR for bumping crates and to rerun pallet-society crates ready before end of tomorrow (we need to wait for this PR to be merged, backported one to be merged, crate to be published, PR runtime to run benchmarks and be green and reviewed). I believe it's doable if we can make a very short referendum on Kusama.
There was a problem hiding this comment.
polkadot-fellows/runtimes#1080 has just been merged by @ggwpez and contains the fix for kick_member - once runtime is built there will be a referendum @laurogripa
|
I have some concerns about merging in the current state but it's also the 1st time I am looking at this code. |
Thanks for the thorough review, @sigurpol! Agreed, it's not ideal, but it's also temporary. I plan to open an RFC right after this, so we can discuss a better solution (possibly with collective governance and a minimum quorum to avoid further collusions) |
Head branch was pushed to by a user without write access
|
@bkchr @sigurpol guys, thanks for the reviews and discussions! I fixed the test and updated the benchmark with max payouts. It's night here, so I'll be back in 9 hours or so, but feel free to commit changes if you guys want. |
|
/cmd bench --pallet pallet_society --runtime collectives-westend |
|
Command "bench --pallet pallet_society --runtime collectives-westend" has started 🚀 See logs here |
|
Command "bench --pallet pallet_society --runtime collectives-westend" has failed ❌! See logs here |
|
/cmd bench --pallet pallet_society --runtime kitchensink-runtime |
|
Command "bench --pallet pallet_society --runtime kitchensink-runtime" has started 🚀 See logs here |
|
Command "bench --pallet pallet_society --runtime kitchensink-runtime" has failed ❌! See logs here |
695397f
Adds a kick_member extrinsic to pallet-society, callable by the Founder, that removes a member,slashes its payout and returns funds to the Society pot. ### Context Recently, a collusion was discovered where a member managed to claim membership without providing PoI. This change introduces a way for the Founder to kick and slash the bad actors. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Paolo La Camera <paolo@parity.io> (cherry picked from commit 695397f)
|
Successfully created backport PR for |
Adds a kick_member extrinsic to pallet-society, callable by the Founder, that removes a member,slashes its payout and returns funds to the Society pot. ### Context Recently, a collusion was discovered where a member managed to claim membership without providing PoI. This change introduces a way for the Founder to kick and slash the bad actors. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Paolo La Camera <paolo@parity.io> (cherry picked from commit 695397f)
|
Successfully created backport PR for |
Backport #11154 into `stable2512` from laurogripa. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Lauro Gripa <laurogripa@gmail.com> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Paolo La Camera <paolo@parity.io>
Backport #11154 into `stable2603` from laurogripa. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Lauro Gripa <laurogripa@gmail.com> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Paolo La Camera <paolo@parity.io>
|
@laurogripa this is the PR in runtime that contains the fix: polkadot-fellows/runtimes#1101 |
|
Kicking happend here https://assethub-kusama.subscan.io/event/14290025-4 |
Backport paritytech#11154 into `stable2512` from laurogripa. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Lauro Gripa <laurogripa@gmail.com> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Paolo La Camera <paolo@parity.io>
Adds a kick_member extrinsic to pallet-society, callable by the Founder, that removes a member,slashes its payout and returns funds to the Society pot.
Context
Recently, a collusion was discovered where a member managed to claim membership without providing PoI. This change introduces a way for the Founder to kick and slash the bad actors.