diff --git a/prdoc/pr_2758.prdoc b/prdoc/pr_2758.prdoc new file mode 100644 index 0000000000..d8cb0557e9 --- /dev/null +++ b/prdoc/pr_2758.prdoc @@ -0,0 +1,10 @@ +title: Fix vote weights of ranked members in the Society pallet + +doc: + - audience: Runtime User + description: | + Fixes a bug in the tally accrual of approvals/rejections when + ranked members vote for Candidates and Defender in the Society pallet. + +crates: + - name: pallet-society diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs index ca8d96e193..99dd580a40 100644 --- a/substrate/frame/society/src/lib.rs +++ b/substrate/frame/society/src/lib.rs @@ -1433,8 +1433,8 @@ impl, I: 'static> Pallet { let weight_root = rank + 1; let weight = weight_root * weight_root; match approve { - true => tally.approvals.saturating_accrue(1), - false => tally.rejections.saturating_accrue(1), + true => tally.approvals.saturating_accrue(weight), + false => tally.rejections.saturating_accrue(weight), } Vote { approve, weight } } diff --git a/substrate/frame/society/src/tests.rs b/substrate/frame/society/src/tests.rs index ea2afef3b3..2163575a86 100644 --- a/substrate/frame/society/src/tests.rs +++ b/substrate/frame/society/src/tests.rs @@ -720,7 +720,7 @@ fn unvouch_works() { // But their pick doesn't resign (yet). conclude_intake(false, None); // Voting still happening and voucher cannot unvouch. - assert_eq!(candidacies(), vec![(20, candidacy(1, 100, Vouch(10, 0), 0, 1))]); + assert_eq!(candidacies(), vec![(20, candidacy(1, 100, Vouch(10, 0), 0, 4))]); assert_eq!(Members::::get(10).unwrap().vouching, Some(VouchingStatus::Vouching)); // Candidate gives in and resigns. @@ -836,72 +836,72 @@ fn founder_and_head_cannot_be_removed() { fn challenges_work() { EnvBuilder::new().execute(|| { // Add some members - place_members([20, 30, 40]); + place_members([20, 30, 40, 50]); // Votes are empty - assert_eq!(DefenderVotes::::get(0, 10), None); assert_eq!(DefenderVotes::::get(0, 20), None); assert_eq!(DefenderVotes::::get(0, 30), None); assert_eq!(DefenderVotes::::get(0, 40), None); + assert_eq!(DefenderVotes::::get(0, 50), None); // Check starting point - assert_eq!(members(), vec![10, 20, 30, 40]); + assert_eq!(members(), vec![10, 20, 30, 40, 50]); assert_eq!(Defending::::get(), None); - // 30 will be challenged during the challenge rotation + // 40 will be challenged during the challenge rotation next_challenge(); - assert_eq!(Defending::::get().unwrap().0, 30); + assert_eq!(Defending::::get().unwrap().0, 40); // They can always free vote for themselves - assert_ok!(Society::defender_vote(Origin::signed(30), true)); + assert_ok!(Society::defender_vote(Origin::signed(40), true)); // If no one else votes, nothing happens next_challenge(); - assert_eq!(members(), vec![10, 20, 30, 40]); + assert_eq!(members(), vec![10, 20, 30, 40, 50]); // Reset votes for last challenge assert_ok!(Society::cleanup_challenge(Origin::signed(0), 0, 10)); - // New challenge period - assert_eq!(Defending::::get().unwrap().0, 30); + // New challenge period, 20 is challenged + assert_eq!(Defending::::get().unwrap().0, 20); // Non-member cannot vote assert_noop!(Society::defender_vote(Origin::signed(1), true), Error::::NotMember); // 3 people say accept, 1 reject - assert_ok!(Society::defender_vote(Origin::signed(10), true)); assert_ok!(Society::defender_vote(Origin::signed(20), true)); assert_ok!(Society::defender_vote(Origin::signed(30), true)); - assert_ok!(Society::defender_vote(Origin::signed(40), false)); + assert_ok!(Society::defender_vote(Origin::signed(40), true)); + assert_ok!(Society::defender_vote(Origin::signed(50), false)); next_challenge(); - // 30 survives - assert_eq!(members(), vec![10, 20, 30, 40]); + // 20 survives + assert_eq!(members(), vec![10, 20, 30, 40, 50]); // Reset votes for last challenge assert_ok!(Society::cleanup_challenge(Origin::signed(0), 1, 10)); // Votes are reset - assert_eq!(DefenderVotes::::get(0, 10), None); assert_eq!(DefenderVotes::::get(0, 20), None); assert_eq!(DefenderVotes::::get(0, 30), None); assert_eq!(DefenderVotes::::get(0, 40), None); + assert_eq!(DefenderVotes::::get(0, 50), None); - // One more time - assert_eq!(Defending::::get().unwrap().0, 30); + // One more time, 40 is challenged + assert_eq!(Defending::::get().unwrap().0, 40); // 2 people say accept, 2 reject - assert_ok!(Society::defender_vote(Origin::signed(10), true)); assert_ok!(Society::defender_vote(Origin::signed(20), true)); - assert_ok!(Society::defender_vote(Origin::signed(30), false)); + assert_ok!(Society::defender_vote(Origin::signed(30), true)); assert_ok!(Society::defender_vote(Origin::signed(40), false)); + assert_ok!(Society::defender_vote(Origin::signed(50), false)); next_challenge(); - // 30 is suspended - assert_eq!(members(), vec![10, 20, 40]); + // 40 is suspended + assert_eq!(members(), vec![10, 20, 30, 50]); assert_eq!( - SuspendedMembers::::get(30), - Some(MemberRecord { rank: 0, strikes: 0, vouching: None, index: 2 }) + SuspendedMembers::::get(40), + Some(MemberRecord { rank: 0, strikes: 0, vouching: None, index: 3 }) ); // Reset votes for last challenge assert_ok!(Society::cleanup_challenge(Origin::signed(0), 2, 10)); - // New defender is chosen - assert_eq!(Defending::::get().unwrap().0, 20); + // New defender is chosen, 30 is challenged + assert_eq!(Defending::::get().unwrap().0, 30); // Votes are reset - assert_eq!(DefenderVotes::::get(0, 10), None); assert_eq!(DefenderVotes::::get(0, 20), None); assert_eq!(DefenderVotes::::get(0, 30), None); assert_eq!(DefenderVotes::::get(0, 40), None); + assert_eq!(DefenderVotes::::get(0, 50), None); }); } @@ -1044,6 +1044,9 @@ fn vouching_handles_removed_member_with_candidate() { fn votes_are_working() { EnvBuilder::new().execute(|| { place_members([20]); + // Member 10 is rank 1 and Member 20 is rank 0 + assert_eq!(Members::::get(10).unwrap().rank, 1); + assert_eq!(Members::::get(20).unwrap().rank, 0); // Users make bids of various amounts assert_ok!(Society::bid(RuntimeOrigin::signed(50), 500)); assert_ok!(Society::bid(RuntimeOrigin::signed(40), 400)); @@ -1061,6 +1064,32 @@ fn votes_are_working() { assert_eq!(Votes::::get(30, 20), Some(Vote { approve: true, weight: 1 })); assert_eq!(Votes::::get(40, 10), Some(Vote { approve: true, weight: 4 })); assert_eq!(Votes::::get(50, 10), None); + + // Votes have correct weights on the tally + assert_eq!( + Candidates::::get(30).unwrap().tally, + Tally { approvals: 5, rejections: 0 } + ); + assert_eq!( + Candidates::::get(40).unwrap().tally, + Tally { approvals: 4, rejections: 0 } + ); + // Member 10 changes his vote for Candidate 30 + assert_ok!(Society::vote(Origin::signed(10), 30, false)); + // Assert the tally calculation is correct + assert_eq!( + Candidates::::get(30).unwrap().tally, + Tally { approvals: 1, rejections: 4 } + ); + // Member 10 changes his vote again + assert_ok!(Society::vote(Origin::signed(10), 30, true)); + // Assert the tally is still correct + assert_eq!( + Candidates::::get(30).unwrap().tally, + Tally { approvals: 5, rejections: 0 } + ); + + // Finish intake conclude_intake(false, None); // Cleanup the candidacy assert_ok!(Society::cleanup_candidacy(Origin::signed(0), 30, 10)); @@ -1240,7 +1269,7 @@ fn waive_repay_works() { Payouts::::get(20), PayoutRecord { paid: 0, payouts: vec![].try_into().unwrap() } ); - assert_eq!(Members::::get(10).unwrap().rank, 1); + assert_eq!(Members::::get(20).unwrap().rank, 1); assert_eq!(Balances::free_balance(20), 50); }); }