Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-collective: Do not vote aye with propose #9323

Merged
24 commits merged into from
Jul 13, 2021
Merged

pallet-collective: Do not vote aye with propose #9323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f3d7798
pallet-collective Add option to not vote `aye` with `propose`
emostov Jul 10, 2021
202a3e6
Test: propose_with_no_self_vote_works
emostov Jul 10, 2021
9eeb071
Param doc grammar
emostov Jul 10, 2021
ca24990
Update benchmarks
emostov Jul 10, 2021
1cefd16
Revert changes
emostov Jul 10, 2021
c8f4f97
Do note vote when proposing
emostov Jul 11, 2021
4bf3c93
Update benchmarks
emostov Jul 11, 2021
bd144f2
Reduce diff on benchmarks
emostov Jul 11, 2021
53365df
Reduce diff on tests
emostov Jul 11, 2021
96b37f5
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jul 13, 2021
d10a845
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
53bacb8
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
3d86ca9
manual bench
Jul 13, 2021
d4f298f
manual bench 2
Jul 13, 2021
0f752a2
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
9124792
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
52c2157
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
fb17dcb
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
53afff9
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
7410417
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Jul 13, 2021
bac19fa
motion_with_no_votes_closes_with_disapproval
emostov Jul 13, 2021
a0cca65
Merge branch 'zeke-retract-vote' of https://github.com/paritytech/sub…
emostov Jul 13, 2021
88b9133
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
898963b
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions frame/collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,7 @@ benchmarks_instance! {

let index = p - 1;
// Have almost everyone vote aye on last proposal, while keeping it from passing.
// Proposer already voted aye so we start at 1.
for j in 1 .. m - 3 {
for j in 0 .. m - 3 {
let voter = &members[j as usize];
let approve = true;
Collective::<T, _>::vote(
Expand Down Expand Up @@ -326,8 +325,7 @@ benchmarks_instance! {

let index = p - 1;
// Have most everyone vote aye on last proposal, while keeping it from passing.
// Proposer already voted aye so we start at 1.
for j in 1 .. m - 2 {
for j in 0 .. m - 2 {
let voter = &members[j as usize];
let approve = true;
Collective::<T, _>::vote(
Expand Down Expand Up @@ -560,6 +558,14 @@ benchmarks_instance! {
last_hash = T::Hashing::hash_of(&proposal);
}

// The prime member votes aye, so abstentions default to aye.
Collective::<T, _>::vote(
SystemOrigin::Signed(caller.clone()).into(),
last_hash.clone(),
p - 1,
true // Vote aye.
)?;

// Have almost everyone vote nay on last proposal, while keeping it from failing.
// A few abstainers will be the aye votes needed to pass the vote.
for j in 2 .. m - 1 {
Expand Down
114 changes: 107 additions & 7 deletions frame/collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,10 @@ decl_module! {
let index = Self::proposal_count();
<ProposalCount<I>>::mutate(|i| *i += 1);
<ProposalOf<T, I>>::insert(proposal_hash, *proposal);
let end = system::Pallet::<T>::block_number() + T::MotionDuration::get();
let votes = Votes { index, threshold, ayes: vec![who.clone()], nays: vec![], end };
let votes = {
let end = system::Pallet::<T>::block_number() + T::MotionDuration::get();
Votes { index, threshold, ayes: vec![], nays: vec![], end }
};
<Voting<T, I>>::insert(proposal_hash, votes);

Self::deposit_event(RawEvent::Proposed(who, index, proposal_hash, threshold));
Expand Down Expand Up @@ -1094,6 +1096,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);

assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(3);
Expand All @@ -1108,6 +1111,7 @@ mod tests {
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
assert_eq!(System::events(), vec![
record(Event::Collective(RawEvent::Proposed(1, 0, hash.clone(), 3))),
record(Event::Collective(RawEvent::Voted(1, hash.clone(), true, 1, 0))),
record(Event::Collective(RawEvent::Voted(2, hash.clone(), true, 2, 0))),
record(Event::Collective(RawEvent::Closed(hash.clone(), 2, 1))),
record(Event::Collective(RawEvent::Disapproved(hash.clone())))
Expand All @@ -1125,6 +1129,7 @@ mod tests {
// Set 1 as prime voter
Prime::<Test, Instance1>::set(Some(1));
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
// With 1's prime vote, this should pass
System::set_block_number(4);
assert_noop!(
Expand Down Expand Up @@ -1162,6 +1167,7 @@ mod tests {
assert_ok!(Collective::set_members(Origin::root(), vec![1, 2, 3], Some(3), MaxMembers::get()));

assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(4);
Expand All @@ -1170,6 +1176,7 @@ mod tests {
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
assert_eq!(System::events(), vec![
record(Event::Collective(RawEvent::Proposed(1, 0, hash.clone(), 3))),
record(Event::Collective(RawEvent::Voted(1, hash.clone(), true, 1, 0))),
record(Event::Collective(RawEvent::Voted(2, hash.clone(), true, 2, 0))),
record(Event::Collective(RawEvent::Closed(hash.clone(), 2, 1))),
record(Event::Collective(RawEvent::Disapproved(hash.clone())))
Expand All @@ -1187,6 +1194,7 @@ mod tests {
assert_ok!(Collective::set_members(Origin::root(), vec![1, 2, 3], Some(1), MaxMembers::get()));

assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(4);
Expand All @@ -1195,6 +1203,7 @@ mod tests {
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
assert_eq!(System::events(), vec![
record(Event::Collective(RawEvent::Proposed(1, 0, hash.clone(), 3))),
record(Event::Collective(RawEvent::Voted(1, hash.clone(), true, 1, 0))),
record(Event::Collective(RawEvent::Voted(2, hash.clone(), true, 2, 0))),
record(Event::Collective(RawEvent::Closed(hash.clone(), 3, 0))),
record(Event::Collective(RawEvent::Approved(hash.clone()))),
Expand All @@ -1213,6 +1222,7 @@ mod tests {
assert_ok!(CollectiveMajority::set_members(Origin::root(), vec![1, 2, 3, 4, 5], Some(5), MaxMembers::get()));

assert_ok!(CollectiveMajority::propose(Origin::signed(1), 5, Box::new(proposal.clone()), proposal_len));
assert_ok!(CollectiveMajority::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(CollectiveMajority::vote(Origin::signed(2), hash.clone(), 0, true));
assert_ok!(CollectiveMajority::vote(Origin::signed(3), hash.clone(), 0, true));

Expand All @@ -1222,6 +1232,7 @@ mod tests {
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
assert_eq!(System::events(), vec![
record(Event::CollectiveMajority(RawEvent::Proposed(1, 0, hash.clone(), 5))),
record(Event::CollectiveMajority(RawEvent::Voted(1, hash.clone(), true, 1, 0))),
record(Event::CollectiveMajority(RawEvent::Voted(2, hash.clone(), true, 2, 0))),
record(Event::CollectiveMajority(RawEvent::Voted(3, hash.clone(), true, 3, 0))),
record(Event::CollectiveMajority(RawEvent::Closed(hash.clone(), 5, 0))),
Expand All @@ -1239,6 +1250,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
let end = 4;
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1254,6 +1266,7 @@ mod tests {
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash = BlakeTwo256::hash_of(&proposal);
assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 1, true));
assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1275,6 +1288,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
let end = 4;
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1290,6 +1304,7 @@ mod tests {
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash = BlakeTwo256::hash_of(&proposal);
assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 1, true));
assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1315,7 +1330,7 @@ mod tests {
assert_eq!(Collective::proposal_of(&hash), Some(proposal));
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 3, ayes: vec![1], nays: vec![], end })
Some(Votes { index: 0, threshold: 3, ayes: vec![], nays: vec![], end })
);

assert_eq!(System::events(), vec![
Expand All @@ -1336,10 +1351,15 @@ mod tests {
#[test]
fn limit_active_proposals() {
new_test_ext().execute_with(|| {
for i in 0..MaxProposals::get() {
for i in 0 .. MaxProposals::get() {
let proposal = make_proposal(i as u64);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::propose(
Origin::signed(1),
3,
Box::new(proposal.clone()),
proposal_len
));
}
let proposal = make_proposal(MaxProposals::get() as u64 + 1);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
Expand Down Expand Up @@ -1421,26 +1441,36 @@ mod tests {
}

#[test]
fn motions_revoting_works() {
fn motions_vote_after_works() {
new_test_ext().execute_with(|| {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash: H256 = proposal.blake2_256().into();
let end = 4;
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
// Initially there a no votes when the motion is proposed.
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![], end })
);
// Cast first aye vote.
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 2, ayes: vec![1], nays: vec![], end })
);
// Try to cast a duplicate aye vote.
assert_noop!(
Collective::vote(Origin::signed(1), hash.clone(), 0, true),
Error::<Test, Instance1>::DuplicateVote,
);
// Cast a nay vote.
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, false));
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![1], end })
);
// Try to cast a duplicate nay vote.
assert_noop!(
Collective::vote(Origin::signed(1), hash.clone(), 0, false),
Error::<Test, Instance1>::DuplicateVote,
Expand All @@ -1457,6 +1487,18 @@ mod tests {
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
1,
hex!["68eea8f20b542ec656c6ac2d10435ae3bd1729efc34d1354ab85af840aad2d35"]
.into(),
true,
1,
0,
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
Expand Down Expand Up @@ -1489,7 +1531,7 @@ mod tests {
);
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 2, ayes: vec![1], nays: vec![], end })
Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![], end })
);

// For the motion, acc 2's first vote, expecting Ok with Pays::No.
Expand Down Expand Up @@ -1586,6 +1628,7 @@ mod tests {
let proposal_weight = proposal.get_dispatch_info().weight;
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, false));
assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, proposal_weight, proposal_len));

Expand All @@ -1601,6 +1644,17 @@ mod tests {
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
1,
hex!["68eea8f20b542ec656c6ac2d10435ae3bd1729efc34d1354ab85af840aad2d35"].into(),
true,
1,
0,
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
Expand Down Expand Up @@ -1638,6 +1692,7 @@ mod tests {
let proposal_weight = proposal.get_dispatch_info().weight;
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, proposal_weight, proposal_len));

Expand All @@ -1652,6 +1707,17 @@ mod tests {
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
1,
hex!["68eea8f20b542ec656c6ac2d10435ae3bd1729efc34d1354ab85af840aad2d35"].into(),
true,
1,
0,
)),
topics: vec![],
},
EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Voted(
Expand Down Expand Up @@ -1689,6 +1755,37 @@ mod tests {
});
}

#[test]
fn motion_with_no_votes_closes_with_disapproval() {
new_test_ext().execute_with(|| {
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let proposal_weight = proposal.get_dispatch_info().weight;
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len));
assert_eq!(System::events()[0], record(Event::Collective(RawEvent::Proposed(1, 0, hash.clone(), 3))));

// Closing the motion too early is not possible because it has neither
// an approving or disapproving simple majority due to the lack of votes.
assert_noop!(
Collective::close(Origin::signed(2), hash.clone(), 0, proposal_weight, proposal_len),
Error::<Test, Instance1>::TooEarly
);

// Once the motion duration passes,
let closing_block = System::block_number() + MotionDuration::get();
System::set_block_number(closing_block);
// we can successfully close the motion.
assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, proposal_weight, proposal_len));

// Events show that the close ended in a disapproval.
assert_eq!(System::events()[1], record(Event::Collective(RawEvent::Closed(hash.clone(), 0, 3))));
assert_eq!(System::events()[2], record(Event::Collective(RawEvent::Disapproved(hash.clone()))));
})

}

#[test]
fn close_disapprove_does_not_care_about_weight_or_len() {
// This test confirms that if you close a proposal that would be disapproved,
Expand All @@ -1700,6 +1797,7 @@ mod tests {
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
// First we make the proposal succeed
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
// It will not close with bad weight/len information
assert_noop!(
Expand All @@ -1726,12 +1824,14 @@ mod tests {
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
// Proposal would normally succeed
assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
// But Root can disapprove and remove it anyway
assert_ok!(Collective::disapprove_proposal(Origin::root(), hash.clone()));
let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] };
assert_eq!(System::events(), vec![
record(Event::Collective(RawEvent::Proposed(1, 0, hash.clone(), 2))),
record(Event::Collective(RawEvent::Voted(1, hash.clone(), true, 1, 0))),
record(Event::Collective(RawEvent::Voted(2, hash.clone(), true, 2, 0))),
record(Event::Collective(RawEvent::Disapproved(hash.clone()))),
]);
Expand Down
Loading