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 4 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
12 changes: 10 additions & 2 deletions frame/collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
MAX_BYTES,
true, // Include aye vote on behalf of proposer.
)?;
let hash = T::Hashing::hash_of(&proposal);
// Vote on the proposal to increase state relevant for `set_members`.
Expand Down Expand Up @@ -159,7 +160,7 @@ benchmarks_instance! {
let proposal: T::Proposal = SystemCall::<T>::remark(vec![1; b as usize]).into();
let threshold = 1;

}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()), bytes_in_storage)
}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()), bytes_in_storage, true)
verify {
let proposal_hash = T::Hashing::hash_of(&proposal);
// Note that execution fails due to mis-matched origin
Expand Down Expand Up @@ -196,14 +197,15 @@ benchmarks_instance! {
threshold,
Box::new(proposal),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
}

assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);

let proposal: T::Proposal = SystemCall::<T>::remark(vec![p as u8; b as usize]).into();

}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()), bytes_in_storage)
}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()), bytes_in_storage, true)
verify {
// New proposal is recorded
assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
Expand Down Expand Up @@ -244,6 +246,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
Expand Down Expand Up @@ -320,6 +323,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
Expand Down Expand Up @@ -398,6 +402,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
Expand Down Expand Up @@ -484,6 +489,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
Expand Down Expand Up @@ -556,6 +562,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
Expand Down Expand Up @@ -619,6 +626,7 @@ benchmarks_instance! {
threshold,
Box::new(proposal.clone()),
bytes_in_storage,
true, // Include aye vote on behalf of proposer.
)?;
last_hash = T::Hashing::hash_of(&proposal);
}
emostov marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
103 changes: 76 additions & 27 deletions frame/collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,9 @@ decl_module! {
/// `threshold` determines whether `proposal` is executed directly (`threshold < 2`)
/// or put up for voting.
///
/// `include_aye_vote` determines whether or not the caller will record an aye vote for
/// the proposal.
///
/// # <weight>
/// ## Weight
/// - `O(B + M + P1)` or `O(B + M + P2)` where:
Expand Down Expand Up @@ -443,7 +446,8 @@ decl_module! {
fn propose(origin,
#[compact] threshold: MemberCount,
proposal: Box<<T as Config<I>>::Proposal>,
#[compact] length_bound: u32
#[compact] length_bound: u32,
include_aye_vote: bool,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
let members = Self::members();
Expand Down Expand Up @@ -476,8 +480,13 @@ 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();
let ayes = if include_aye_vote { vec![who.clone()] } else { vec![] };
Votes { index, threshold, ayes, nays: vec![], end }
};

<Voting<T, I>>::insert(proposal_hash, votes);

Self::deposit_event(RawEvent::Proposed(who, index, proposal_hash, threshold));
Expand Down Expand Up @@ -1093,7 +1102,7 @@ mod tests {
let proposal_weight = proposal.get_dispatch_info().weight;
let hash = BlakeTwo256::hash_of(&proposal);

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, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(3);
Expand Down Expand Up @@ -1124,7 +1133,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
// 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::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true));
// With 1's prime vote, this should pass
System::set_block_number(4);
assert_noop!(
Expand All @@ -1143,7 +1152,7 @@ mod tests {
let proposal_weight = proposal.get_dispatch_info().weight;
let hash = BlakeTwo256::hash_of(&proposal);

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, true));
// No votes, this proposal wont pass
System::set_block_number(4);
assert_ok!(
Expand All @@ -1161,7 +1170,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
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::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(4);
Expand All @@ -1186,7 +1195,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
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::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));

System::set_block_number(4);
Expand All @@ -1212,7 +1221,7 @@ mod tests {
let hash = BlakeTwo256::hash_of(&proposal);
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::propose(Origin::signed(1), 5, Box::new(proposal.clone()), proposal_len, 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 @@ -1238,7 +1247,7 @@ mod tests {
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
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::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1253,7 +1262,7 @@ mod tests {
let proposal = make_proposal(69);
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::propose(Origin::signed(2), 2, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1274,7 +1283,7 @@ mod tests {
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
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::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1289,7 +1298,7 @@ mod tests {
let proposal = make_proposal(69);
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::propose(Origin::signed(2), 2, Box::new(proposal.clone()), proposal_len, true));
assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false));
assert_eq!(
Collective::voting(&hash),
Expand All @@ -1310,7 +1319,7 @@ mod tests {
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash = proposal.blake2_256().into();
let end = 4;
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, true));
assert_eq!(*Collective::proposals(), vec![hash]);
assert_eq!(Collective::proposal_of(&hash), Some(proposal));
assert_eq!(
Expand All @@ -1333,18 +1342,57 @@ mod tests {
});
}

#[test]
fn propose_with_no_self_vote_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 = proposal.blake2_256().into();
let end = 4;
assert_ok!(Collective::propose(
Origin::signed(1),
3,
Box::new(proposal.clone()),
proposal_len,
false, // Do not include an aye vote on behalf of the caller.
));
assert_eq!(*Collective::proposals(), vec![hash]);
assert_eq!(Collective::proposal_of(&hash), Some(proposal));
assert_eq!(
Collective::voting(&hash),
// No aye (or nay) votes leaked into storage while proposing.
Some(Votes { index: 0, threshold: 3, ayes: vec![], nays: vec![], end })
);

assert_eq!(
System::events(),
vec![EventRecord {
phase: Phase::Initialization,
event: Event::Collective(RawEvent::Proposed(
1,
0,
hex!["68eea8f20b542ec656c6ac2d10435ae3bd1729efc34d1354ab85af840aad2d35"]
.into(),
3,
)),
topics: vec![],
}]
);
});
}

#[test]
fn limit_active_proposals() {
new_test_ext().execute_with(|| {
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, true));
}
let proposal = make_proposal(MaxProposals::get() as u64 + 1);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
assert_noop!(
Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len),
Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, true),
Error::<Test, Instance1>::TooManyProposals
);
})
Expand All @@ -1355,7 +1403,7 @@ mod tests {
new_test_ext().execute_with(|| {
let proposal = Call::Collective(crate::Call::set_members(vec![1, 2, 3], None, MaxMembers::get()));
let length = proposal.encode().len() as u32;
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), length));
assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), length, true));

let hash = BlakeTwo256::hash_of(&proposal);
let weight = proposal.get_dispatch_info().weight;
Expand Down Expand Up @@ -1385,7 +1433,7 @@ mod tests {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
assert_noop!(
Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone()), proposal_len),
Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone()), proposal_len, true),
Error::<Test, Instance1>::NotMember
);
});
Expand All @@ -1397,7 +1445,7 @@ mod tests {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash: H256 = proposal.blake2_256().into();
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, true));
assert_noop!(
Collective::vote(Origin::signed(42), hash.clone(), 0, true),
Error::<Test, Instance1>::NotMember,
Expand All @@ -1412,7 +1460,7 @@ mod tests {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash: H256 = proposal.blake2_256().into();
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, true));
assert_noop!(
Collective::vote(Origin::signed(2), hash.clone(), 1, true),
Error::<Test, Instance1>::WrongIndex,
Expand All @@ -1427,7 +1475,7 @@ mod tests {
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));
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len, true));
assert_eq!(
Collective::voting(&hash),
Some(Votes { index: 0, threshold: 2, ayes: vec![1], nays: vec![], end })
Expand Down Expand Up @@ -1485,6 +1533,7 @@ mod tests {
2,
Box::new(proposal.clone()),
proposal_len,
true
)
);
assert_eq!(
Expand Down Expand Up @@ -1569,11 +1618,11 @@ mod tests {
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_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, 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));
assert_eq!(*Collective::proposals(), vec![]);
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len, true));
assert_eq!(*Collective::proposals(), vec![hash]);
});
}
Expand All @@ -1585,7 +1634,7 @@ mod tests {
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_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), proposal_len, 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 Down Expand Up @@ -1637,7 +1686,7 @@ mod tests {
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), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len, 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 Down Expand Up @@ -1698,7 +1747,7 @@ mod tests {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len, true));
// First we make the proposal succeed
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
// It will not close with bad weight/len information
Expand All @@ -1724,7 +1773,7 @@ mod tests {
let proposal = make_proposal(42);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
let hash: H256 = proposal.blake2_256().into();
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len));
assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), proposal_len, true));
// Proposal would normally succeed
assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true));
// But Root can disapprove and remove it anyway
Expand Down