From 9094e2d2b34cf06fa9829fa47b658634202dcf08 Mon Sep 17 00:00:00 2001 From: Lauro Gripa Date: Tue, 19 Dec 2023 13:05:06 -0300 Subject: [PATCH 1/4] Fix tally accrual for ranked members and improve tests --- substrate/frame/society/src/lib.rs | 4 +- substrate/frame/society/src/tests.rs | 71 +++++++++++++++++----------- 2 files changed, 46 insertions(+), 29 deletions(-) diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs index ca8d96e193c8..99dd580a4035 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 ea2afef3b32b..c6f8ace0410a 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,20 @@ 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 +1257,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); }); } From 26aab2dab89bbfead35edf3324d1a081c3fba897 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 22 Dec 2023 12:20:39 +0000 Subject: [PATCH 2/4] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/society/src/tests.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/substrate/frame/society/src/tests.rs b/substrate/frame/society/src/tests.rs index c6f8ace0410a..2163575a8608 100644 --- a/substrate/frame/society/src/tests.rs +++ b/substrate/frame/society/src/tests.rs @@ -1066,16 +1066,28 @@ fn votes_are_working() { 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 }); + 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 }); + 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}); + assert_eq!( + Candidates::::get(30).unwrap().tally, + Tally { approvals: 5, rejections: 0 } + ); // Finish intake conclude_intake(false, None); From 00c0f003a5c346612b9b4d34923832d3ce01fa76 Mon Sep 17 00:00:00 2001 From: Lauro Gripa Date: Fri, 22 Dec 2023 16:28:23 -0300 Subject: [PATCH 3/4] Add prdoc --- prdoc/pr_2758.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_2758.prdoc diff --git a/prdoc/pr_2758.prdoc b/prdoc/pr_2758.prdoc new file mode 100644 index 000000000000..df280bf220ad --- /dev/null +++ b/prdoc/pr_2758.prdoc @@ -0,0 +1,12 @@ +title: Fix vote weights of ranked members in the Society pallet +author: laurogripa +topic: Society + +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 From 006ae58262bc20305038a7514ecb3f7d533db8a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 4 Jan 2024 10:28:45 +0100 Subject: [PATCH 4/4] Update prdoc/pr_2758.prdoc --- prdoc/pr_2758.prdoc | 2 -- 1 file changed, 2 deletions(-) diff --git a/prdoc/pr_2758.prdoc b/prdoc/pr_2758.prdoc index df280bf220ad..d8cb0557e9b6 100644 --- a/prdoc/pr_2758.prdoc +++ b/prdoc/pr_2758.prdoc @@ -1,6 +1,4 @@ title: Fix vote weights of ranked members in the Society pallet -author: laurogripa -topic: Society doc: - audience: Runtime User