Fix vote weights of ranked members in the Society pallet #2758
Conversation
|
User @laurogripa, please sign the CLA here. |
|
@laurogripa can you please write a prdoc? |
|
bot fmt |
|
@bkchr https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4792269 was started for your command Comment |
|
@bkchr Command |
Done. Let me know if it makes sense. |
…2758) This PR fixes a bug in the tally accrual of approvals/rejections for Candidates and Defender. The issue happened because: - The `match maybe_old` is reducing `weight` from the tally: ``` Some(Vote { approve: true, weight }) => tally.approvals.saturating_reduce(weight), Some(Vote { approve: false, weight }) => tally.rejections.saturating_reduce(weight), ``` - But `match approval` is accruing only `1` to the tally: ``` true => tally.approvals.saturating_accrue(1), false => tally.rejections.saturating_accrue(1), ``` This way, if a member is rank 1 his vote is going to have weight 1 when accruing but weight 4 when reducing from the tally. For example, let's say: - There's a Candidate with 0 approvals and 12 rejections; - A ranked Member votes against the Candidate; - The tally changes to 0 approvals and 13 rejections (should be 16); - The Member changes his vote to an approval; - Now tally changes to 1 approvals and 9 rejections, removing the accrued approvals from other Members; - If the Member keeps changing his vote, it wipes the tally clean. So this PR changes `match approval` to accrue `weight` instead of just `1` and changes the tests: - Fixes `challenges_work`. This test started failing after the fix. The reason is that the test assumes that all Members have equal weights to their votes, but Member 10 is ranked, so his vote should have weight 4 against the Defender. So instead of using Member 10, I added Member 50 of rank 0 to keep the same logic; - Improves `votes_are_working`. Added some assertions to check if the tally is correct even after a ranked Member changes his vote a couple times; - Fixes `waive_repay_works`. Unrelated to the bug, but this test was yielding a false positive. The test is ranking up Member 20, but asserting the rank of Member 10, which is already ranked up. --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: command-bot <>
This PR fixes a bug in the tally accrual of approvals/rejections for Candidates and Defender. The issue happened because:
match maybe_oldis reducingweightfrom the tally:match approvalis accruing only1to the tally:This way, if a member is rank 1 his vote is going to have weight 1 when accruing but weight 4 when reducing from the tally. For example, let's say:
So this PR changes
match approvalto accrueweightinstead of just1and changes the tests:challenges_work. This test started failing after the fix. The reason is that the test assumes that all Members have equal weights to their votes, but Member 10 is ranked, so his vote should have weight 4 against the Defender. So instead of using Member 10, I added Member 50 of rank 0 to keep the same logic;votes_are_working. Added some assertions to check if the tally is correct even after a ranked Member changes his vote a couple times;waive_repay_works. Unrelated to the bug, but this test was yielding a false positive. The test is ranking up Member 20, but asserting the rank of Member 10, which is already ranked up.