Introduce cancel_proposal to rid us of those pesky proposals (#7111)

* Introduce `cancel_proposal` Also fix proposal weight. * Support proposal cancellation from runtime. * Fixes * Fixes * Fixes * Fixes * Fixes * Fixes * Fixes * Fix benchmarks * fix benchmark * whitelisted caller weights * fix build * Fixes * Fixes * Fixes * Fixes * Update frame/democracy/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Fixes * Fixes * Fixes * Fixes * Fixes * doc updates * new weights Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
2026-05-31 20:21:03 +00:00 · 2020-09-24 23:33:02 +02:00
parent 1b350a050c
commit 26465eadaa
10 changed files with 423 additions and 235 deletions
@@ -19,7 +19,7 @@

 use super::*;

-use frame_benchmarking::{benchmarks, account};
+use frame_benchmarking::{benchmarks, account, whitelist_account};
 use frame_support::{
 	IterableStorageMap,
 	traits::{Currency, Get, EnsureOrigin, OnInitialize, UnfilteredDispatchable, schedule::DispatchTime},
@@ -30,7 +30,7 @@ use sp_runtime::traits::{Bounded, One};
 use crate::Module as Democracy;

 const SEED: u32 = 0;
-const MAX_REFERENDUMS: u32 = 100;
+const MAX_REFERENDUMS: u32 = 99;
 const MAX_SECONDERS: u32 = 100;
 const MAX_BYTES: u32 = 16_384;

@@ -63,7 +63,7 @@ fn add_proposal<T: Trait>(n: u32) -> Result<T::Hash, &'static str> {
 }

 fn add_referendum<T: Trait>(n: u32) -> Result<ReferendumIndex, &'static str> {
-	let proposal_hash = add_proposal::<T>(n)?;
+	let proposal_hash: T::Hash = T::Hashing::hash_of(&n);
 	let vote_threshold = VoteThreshold::SimpleMajority;

 	Democracy::<T>::inject_referendum(
@@ -100,12 +100,19 @@ benchmarks! {
 	_ { }

 	propose {
+		let p = T::MaxProposals::get();
+
+		for i in 0 .. (p - 1) {
+			add_proposal::<T>(i)?;
+		}
+
 		let caller = funded_account::<T>("caller", 0);
 		let proposal_hash: T::Hash = T::Hashing::hash_of(&0);
 		let value = T::MinimumDeposit::get();
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller), proposal_hash, value.into())
 	verify {
-		assert_eq!(Democracy::<T>::public_props().len(), 1, "Proposals not created.");
+		assert_eq!(Democracy::<T>::public_props().len(), p as usize, "Proposals not created.");
 	}

 	second {
@@ -122,6 +129,7 @@ benchmarks! {

 		let deposits = Democracy::<T>::deposit_of(0).ok_or("Proposal not created")?;
 		assert_eq!(deposits.0.len(), (s + 1) as usize, "Seconds not recorded");
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller), 0, u32::max_value())
 	verify {
 		let deposits = Democracy::<T>::deposit_of(0).ok_or("Proposal not created")?;
@@ -146,7 +154,7 @@ benchmarks! {
 		assert_eq!(votes.len(), r as usize, "Votes were not recorded.");

 		let referendum_index = add_referendum::<T>(r)?;
-
+		whitelist_account!(caller);
 	}: vote(RawOrigin::Signed(caller.clone()), referendum_index, account_vote)
 	verify {
 		let votes = match VotingOf::<T>::get(&caller) {
@@ -179,6 +187,7 @@ benchmarks! {
 		let referendum_index = Democracy::<T>::referendum_count() - 1;

 		// This tests when a user changes a vote
+		whitelist_account!(caller);
 	}: vote(RawOrigin::Signed(caller.clone()), referendum_index, new_vote)
 	verify {
 		let votes = match VotingOf::<T>::get(&caller) {
@@ -206,6 +215,31 @@ benchmarks! {
 		assert!(Democracy::<T>::referendum_status(referendum_index).is_err());
 	}

+	blacklist {
+		let p in 1 .. T::MaxProposals::get();
+
+		// Place our proposal at the end to make sure it's worst case.
+		for i in 0 .. p - 1 {
+			add_proposal::<T>(i)?;
+		}
+		// We should really add a lot of seconds here, but we're not doing it elsewhere.
+
+		// Place our proposal in the external queue, too.
+		let hash = T::Hashing::hash_of(&0);
+		assert!(Democracy::<T>::external_propose(T::ExternalOrigin::successful_origin(), hash.clone()).is_ok());
+
+		// Add a referendum of our proposal.
+		let referendum_index = add_referendum::<T>(0)?;
+		assert!(Democracy::<T>::referendum_status(referendum_index).is_ok());
+
+		let call = Call::<T>::blacklist(hash, Some(referendum_index));
+		let origin = T::BlacklistOrigin::successful_origin();
+	}: { call.dispatch_bypass_filter(origin)? }
+	verify {
+		// Referendum has been canceled
+		assert!(Democracy::<T>::referendum_status(referendum_index).is_err());
+	}
+
 	// Worst case scenario, we external propose a previously blacklisted proposal
 	external_propose {
 		let v in 1 .. MAX_VETOERS as u32;
@@ -287,6 +321,15 @@ benchmarks! {
 		assert_eq!(new_vetoers.len(), (v + 1) as usize, "vetoers not added");
 	}

+	cancel_proposal {
+		let p in 1 .. T::MaxProposals::get();
+
+		// Place our proposal at the end to make sure it's worst case.
+		for i in 0 .. p {
+			add_proposal::<T>(i)?;
+		}
+	}: _(RawOrigin::Root, 0)
+
 	cancel_referendum {
 		let referendum_index = add_referendum::<T>(0)?;
 	}: _(RawOrigin::Root, referendum_index)
@@ -301,7 +344,8 @@ benchmarks! {
 		let referendum_index = add_referendum::<T>(r)?;
 	}: _(RawOrigin::Root, referendum_index)

-	// Note that we have a separate benchmark for `launch_next`
+	// This measures the path of `launch_next` external. Not currently used as we simply
+	// assume the weight is `MaxBlockWeight` when executing.
 	#[extra]
 	on_initialize_external {
 		let r in 0 .. MAX_REFERENDUMS;
@@ -341,6 +385,8 @@ benchmarks! {
 		}
 	}

+	// This measures the path of `launch_next` public. Not currently used as we simply
+	// assume the weight is `MaxBlockWeight` when executing.
 	#[extra]
 	on_initialize_public {
 		let r in 1 .. MAX_REFERENDUMS;
@@ -352,6 +398,7 @@ benchmarks! {
 		assert_eq!(Democracy::<T>::referendum_count(), r, "referenda not created");

 		// Launch public
+		assert!(add_proposal::<T>(r).is_ok(), "proposal not created");
 		LastTabledWasExternal::put(true);

 		let block_number = T::LaunchPeriod::get();
@@ -359,7 +406,7 @@ benchmarks! {
 	}: { Democracy::<T>::on_initialize(block_number) }
 	verify {
 		// One extra because of next public
-		assert_eq!(Democracy::<T>::referendum_count(), r + 1, "referenda not created");
+		assert_eq!(Democracy::<T>::referendum_count(), r + 1, "proposal not accepted");

 		// All should be finished
 		for i in 0 .. r {
@@ -437,6 +484,7 @@ benchmarks! {
 			_ => return Err("Votes are not direct"),
 		};
 		assert_eq!(votes.len(), r as usize, "Votes were not recorded.");
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller.clone()), new_delegate.clone(), Conviction::Locked1x, delegated_balance)
 	verify {
 		let (target, balance) = match VotingOf::<T>::get(&caller) {
@@ -488,6 +536,7 @@ benchmarks! {
 			_ => return Err("Votes are not direct"),
 		};
 		assert_eq!(votes.len(), r as usize, "Votes were not recorded.");
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller.clone()))
 	verify {
 		// Voting should now be direct
@@ -508,6 +557,7 @@ benchmarks! {

 		let caller = funded_account::<T>("caller", 0);
 		let encoded_proposal = vec![1; b as usize];
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller), encoded_proposal.clone())
 	verify {
 		let proposal_hash = T::Hashing::hash(&encoded_proposal[..]);
@@ -529,6 +579,7 @@ benchmarks! {

 		let caller = funded_account::<T>("caller", 0);
 		let encoded_proposal = vec![1; b as usize];
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller), encoded_proposal.clone())
 	verify {
 		let proposal_hash = T::Hashing::hash(&encoded_proposal[..]);
@@ -555,6 +606,7 @@ benchmarks! {
 		assert!(Preimages::<T>::contains_key(proposal_hash));

 		let caller = funded_account::<T>("caller", 0);
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller), proposal_hash.clone(), u32::max_value())
 	verify {
 		let proposal_hash = T::Hashing::hash(&encoded_proposal[..]);
@@ -577,6 +629,7 @@ benchmarks! {
 		}

 		let caller = funded_account::<T>("caller", 0);
+		whitelist_account!(caller);
 	}: unlock(RawOrigin::Signed(caller), locker.clone())
 	verify {
 		// Note that we may want to add a `get_lock` api to actually verify
@@ -614,6 +667,7 @@ benchmarks! {
 		Democracy::<T>::remove_vote(RawOrigin::Signed(locker.clone()).into(), referendum_index)?;

 		let caller = funded_account::<T>("caller", 0);
+		whitelist_account!(caller);
 	}: unlock(RawOrigin::Signed(caller), locker.clone())
 	verify {
 		let votes = match VotingOf::<T>::get(&locker) {
@@ -645,7 +699,7 @@ benchmarks! {
 		assert_eq!(votes.len(), r as usize, "Votes not created");

 		let referendum_index = r - 1;
-
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller.clone()), referendum_index)
 	verify {
 		let votes = match VotingOf::<T>::get(&caller) {
@@ -674,7 +728,7 @@ benchmarks! {
 		assert_eq!(votes.len(), r as usize, "Votes not created");

 		let referendum_index = r - 1;
-
+		whitelist_account!(caller);
 	}: _(RawOrigin::Signed(caller.clone()), caller.clone(), referendum_index)
 	verify {
 		let votes = match VotingOf::<T>::get(&caller) {
@@ -765,6 +819,8 @@ mod tests {
 			assert_ok!(test_benchmark_remove_other_vote::<Test>());
 			assert_ok!(test_benchmark_enact_proposal_execute::<Test>());
 			assert_ok!(test_benchmark_enact_proposal_slash::<Test>());
+			assert_ok!(test_benchmark_blacklist::<Test>());
+			assert_ok!(test_benchmark_cancel_proposal::<Test>());
 		});
 	}
 }