Migrate election-phragmen, election contracts and authorship to decl_error (#4479)

* Migrate election-phragmen

* Migrate elections

* Migrate contracts module

* Update authorship module

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/frame/elections/src/lib.rs

@@ -29,7 +29,7 @@ use sp_runtime::{
 	traits::{Zero, One, StaticLookup, Bounded, Saturating},
 };
 use frame_support::{
-	decl_storage, decl_event, ensure, decl_module,
+	decl_storage, decl_event, ensure, decl_module, decl_error,
 	weights::SimpleDispatchInfo,
 	traits::{
 		Currency, ExistenceRequirement, Get, LockableCurrency, LockIdentifier,
@@ -267,8 +267,72 @@ decl_storage! {
 	}
 }
 
+decl_error! {
+	/// Error for the elections module.
+	pub enum Error for Module<T: Trait> {
+		/// Reporter must be a voter.
+		NotVoter,
+		/// Target for inactivity cleanup must be active.
+		InactiveTarget,
+		/// Cannot reap during presentation period.
+		CannotReapPresenting,
+		/// Cannot reap during grace period.
+		ReapGrace,
+		/// Not a proxy.
+		NotProxy,
+		/// Invalid reporter index.
+		InvalidReporterIndex,
+		/// Invalid target index.
+		InvalidTargetIndex,
+		/// Invalid vote index.
+		InvalidVoteIndex,
+		/// Cannot retract when presenting.
+		CannotRetractPresenting,
+		/// Cannot retract non-voter.
+		RetractNonVoter,
+		/// Invalid retraction index.
+		InvalidRetractionIndex,
+		/// Duplicate candidate submission.
+		DuplicatedCandidate,
+		/// Invalid candidate slot.
+		InvalidCandidateSlot,
+		/// Candidate has not enough funds.
+		InsufficientCandidateFunds,
+		/// Presenter must have sufficient slashable funds.
+		InsufficientPresenterFunds,
+		/// Stake deposited to present winner and be added to leaderboard should be non-zero.
+		ZeroDeposit,
+		/// Candidate not worthy of leaderboard.
+		UnworthyCandidate,
+		/// Leaderboard must exist while present phase active.
+		LeaderboardMustExist,
+		/// Cannot present outside of presentation period.
+		NotPresentationPeriod,
+		/// Presented candidate must be current.
+		InvalidCandidate,
+		/// Duplicated presentation.
+		DuplicatedPresentation,
+		/// Incorrect total.
+		IncorrectTotal,
+		/// Invalid voter index.
+		InvalidVoterIndex,
+		/// New voter must have sufficient funds to pay the bond.
+		InsufficientVoterFunds,
+		/// Locked value must be more than limit.
+		InsufficientLockedValue,
+		/// Amount of candidate votes cannot exceed amount of candidates.
+		TooManyVotes,
+		/// Amount of candidates to receive approval votes should be non-zero.
+		ZeroCandidates,
+		/// No approval changes during presentation period.
+		ApprovalPresentation,
+	}
+}
+
 decl_module! {
 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
+		type Error = Error<T>;
+
 		/// How much should be locked up in order to submit one's candidacy. A reasonable
 		/// default value is 9.
 		const CandidacyBond: BalanceOf<T> = T::CandidacyBond::get();
@@ -363,7 +427,7 @@ decl_module! {
 			hint: SetIndex,
 			#[compact] value: BalanceOf<T>
 		) -> DispatchResult {
-			let who = Self::proxy(ensure_signed(origin)?).ok_or("not a proxy")?;
+			let who = Self::proxy(ensure_signed(origin)?).ok_or(Error::<T>::NotProxy)?;
 			Self::do_set_approvals(who, votes, index, hint, value)
 		}
 
@@ -390,26 +454,25 @@ decl_module! {
 			let reporter = ensure_signed(origin)?;
 			let who = T::Lookup::lookup(who)?;
 
-			ensure!(!Self::presentation_active(), "cannot reap during presentation period");
-			ensure!(Self::voter_info(&reporter).is_some(), "reporter must be a voter");
+			ensure!(!Self::presentation_active(), Error::<T>::CannotReapPresenting);
+			ensure!(Self::voter_info(&reporter).is_some(), Error::<T>::NotVoter);
 
-			let info = Self::voter_info(&who)
-				.ok_or("target for inactivity cleanup must be active")?;
+			let info = Self::voter_info(&who).ok_or(Error::<T>::InactiveTarget)?;
 			let last_active = info.last_active;
 
-			ensure!(assumed_vote_index == Self::vote_index(), "vote index not current");
+			ensure!(assumed_vote_index == Self::vote_index(), Error::<T>::InvalidVoteIndex);
 			ensure!(
 				assumed_vote_index > last_active + T::InactiveGracePeriod::get(),
-				"cannot reap during grace period"
+				Error::<T>::ReapGrace,
 			);
 
 			let reporter_index = reporter_index as usize;
 			let who_index = who_index as usize;
-			let assumed_reporter = Self::voter_at(reporter_index).ok_or("invalid reporter index")?;
-			let assumed_who = Self::voter_at(who_index).ok_or("invalid target index")?;
+			let assumed_reporter = Self::voter_at(reporter_index).ok_or(Error::<T>::InvalidReporterIndex)?;
+			let assumed_who = Self::voter_at(who_index).ok_or(Error::<T>::InvalidTargetIndex)?;
 
-			ensure!(assumed_reporter == reporter, "bad reporter index");
-			ensure!(assumed_who == who, "bad target index");
+			ensure!(assumed_reporter == reporter, Error::<T>::InvalidReporterIndex);
+			ensure!(assumed_who == who, Error::<T>::InvalidTargetIndex);
 
 			// will definitely kill one of reporter or who now.
 
@@ -458,11 +521,11 @@ decl_module! {
 		fn retract_voter(origin, #[compact] index: u32) {
 			let who = ensure_signed(origin)?;
 
-			ensure!(!Self::presentation_active(), "cannot retract when presenting");
-			ensure!(<VoterInfoOf<T>>::exists(&who), "cannot retract non-voter");
+			ensure!(!Self::presentation_active(), Error::<T>::CannotRetractPresenting);
+			ensure!(<VoterInfoOf<T>>::exists(&who), Error::<T>::RetractNonVoter);
 			let index = index as usize;
-			let voter = Self::voter_at(index).ok_or("retraction index invalid")?;
-			ensure!(voter == who, "retraction index mismatch");
+			let voter = Self::voter_at(index).ok_or(Error::<T>::InvalidRetractionIndex)?;
+			ensure!(voter == who, Error::<T>::InvalidRetractionIndex);
 
 			Self::remove_voter(&who, index);
 			T::Currency::unreserve(&who, T::VotingBond::get());
@@ -486,18 +549,18 @@ decl_module! {
 		fn submit_candidacy(origin, #[compact] slot: u32) {
 			let who = ensure_signed(origin)?;
 
-			ensure!(!Self::is_a_candidate(&who), "duplicate candidate submission");
+			ensure!(!Self::is_a_candidate(&who), Error::<T>::DuplicatedCandidate);
 			let slot = slot as usize;
 			let count = Self::candidate_count() as usize;
 			let candidates = Self::candidates();
 			ensure!(
 				(slot == count && count == candidates.len()) ||
 					(slot < candidates.len() && candidates[slot] == T::AccountId::default()),
-				"invalid candidate slot"
+				Error::<T>::InvalidCandidateSlot,
 			);
 			// NOTE: This must be last as it has side-effects.
 			T::Currency::reserve(&who, T::CandidacyBond::get())
-				.map_err(|_| "candidate has not enough funds")?;
+				.map_err(|_| Error::<T>::InsufficientCandidateFunds)?;
 
 			<RegisterInfoOf<T>>::insert(&who, (Self::vote_index(), slot as u32));
 			let mut candidates = candidates;
@@ -529,35 +592,35 @@ decl_module! {
 			let who = ensure_signed(origin)?;
 			ensure!(
 				!total.is_zero(),
-				"stake deposited to present winner and be added to leaderboard should be non-zero",
+				Error::<T>::ZeroDeposit,
 			);
 
 			let candidate = T::Lookup::lookup(candidate)?;
-			ensure!(index == Self::vote_index(), "index not current");
+			ensure!(index == Self::vote_index(), Error::<T>::InvalidVoteIndex);
 			let (_, _, expiring) = Self::next_finalize()
-				.ok_or("cannot present outside of presentation period")?;
+				.ok_or(Error::<T>::NotPresentationPeriod)?;
 			let bad_presentation_punishment =
 				T::PresentSlashPerVoter::get()
 				* BalanceOf::<T>::from(Self::voter_count() as u32);
 			ensure!(
 				T::Currency::can_slash(&who, bad_presentation_punishment),
-				"presenter must have sufficient slashable funds"
+				Error::<T>::InsufficientPresenterFunds,
 			);
 
 			let mut leaderboard = Self::leaderboard()
-				.ok_or("leaderboard must exist while present phase active")?;
-			ensure!(total > leaderboard[0].0, "candidate not worthy of leaderboard");
+				.ok_or(Error::<T>::LeaderboardMustExist)?;
+			ensure!(total > leaderboard[0].0, Error::<T>::UnworthyCandidate);
 
 			if let Some(p) = Self::members().iter().position(|&(ref c, _)| c == &candidate) {
 				ensure!(
 					p < expiring.len(),
-					"candidate must not form a duplicated member if elected"
+					Error::<T>::DuplicatedCandidate,
 				);
 			}
 
 			let voters = Self::all_voters();
 			let (registered_since, candidate_index): (VoteIndex, u32) =
-				Self::candidate_reg_info(&candidate).ok_or("presented candidate must be current")?;
+				Self::candidate_reg_info(&candidate).ok_or(Error::<T>::InvalidCandidate)?;
 			let actual_total = voters.iter()
 				.filter_map(|maybe_voter| maybe_voter.as_ref())
 				.filter_map(|voter| match Self::voter_info(voter) {
@@ -586,7 +649,7 @@ decl_module! {
 				// better safe than sorry.
 				let imbalance = T::Currency::slash(&who, bad_presentation_punishment).0;
 				T::BadPresentation::on_unbalanced(imbalance);
-				Err(if dupe { "duplicate presentation" } else { "incorrect total" })?
+				Err(if dupe { Error::<T>::DuplicatedPresentation } else { Error::<T>::IncorrectTotal })?
 			}
 		}
 
@@ -752,11 +815,11 @@ impl<T: Trait> Module<T> {
 	) -> DispatchResult {
 		let candidates_len = <Self as Store>::Candidates::decode_len().unwrap_or(0_usize);
 
-		ensure!(!Self::presentation_active(), "no approval changes during presentation period");
-		ensure!(index == Self::vote_index(), "incorrect vote index");
+		ensure!(!Self::presentation_active(), Error::<T>::ApprovalPresentation);
+		ensure!(index == Self::vote_index(), Error::<T>::InvalidVoteIndex);
 		ensure!(
 			!candidates_len.is_zero(),
-			"amount of candidates to receive approval votes should be non-zero"
+			Error::<T>::ZeroCandidates,
 		);
 		// Prevent a vote from voters that provide a list of votes that exceeds the candidates
 		// length since otherwise an attacker may be able to submit a very long list of `votes` that
@@ -764,9 +827,9 @@ impl<T: Trait> Module<T> {
 		// bond would cover.
 		ensure!(
 			candidates_len >= votes.len(),
-			"amount of candidate votes cannot exceed amount of candidates"
+			Error::<T>::TooManyVotes,
 		);
-		ensure!(value >= T::MinimumVotingLock::get(), "locked value must be more than limit");
+		ensure!(value >= T::MinimumVotingLock::get(), Error::<T>::InsufficientLockedValue);
 
 		// Amount to be locked up.
 		let mut locked_balance = value.min(T::Currency::total_balance(&who));
@@ -775,8 +838,8 @@ impl<T: Trait> Module<T> {
 
 		if let Some(info) = Self::voter_info(&who) {
 			// already a voter. Index must be valid. No fee. update pot. O(1)
-			let voter = Self::voter_at(hint).ok_or("invalid voter index")?;
-			ensure!(voter == who, "wrong voter index");
+			let voter = Self::voter_at(hint).ok_or(Error::<T>::InvalidVoterIndex)?;
+			ensure!(voter == who, Error::<T>::InvalidVoterIndex);
 
 			// write new accumulated offset.
 			let last_win = info.last_win;
@@ -787,7 +850,7 @@ impl<T: Trait> Module<T> {
 			// not yet a voter. Index _could be valid_. Fee might apply. Bond will be reserved O(1).
 			ensure!(
 				T::Currency::free_balance(&who) > T::VotingBond::get(),
-				"new voter must have sufficient funds to pay the bond"
+				Error::<T>::InsufficientVoterFunds,
 			);
 
 			let (set_index, vec_index) = Self::split_index(hint, VOTER_SET_SIZE);

substrate/frame/elections/src/tests.rs

@@ -256,7 +256,7 @@ fn chunking_voter_index_does_not_take_holes_into_account() {
 		// proof: can submit a new approval with the old index.
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(65), vec![], 0, 64 - 2, 10),
-			"wrong voter index"
+			Error::<Test>::InvalidVoterIndex,
 		);
 		assert_ok!(Elections::set_approvals(Origin::signed(65), vec![], 0, 64, 10));
 	})
@@ -338,12 +338,12 @@ fn voting_subsequent_set_approvals_checks_voter_index() {
 		// invalid index
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(4), vec![true], 0, 5, 40),
-			"invalid voter index"
+			Error::<Test>::InvalidVoterIndex,
 		);
 		// wrong index
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(4), vec![true], 0, 0, 40),
-			"wrong voter index"
+			Error::<Test>::InvalidVoterIndex,
 		);
 		// correct
 		assert_ok!(Elections::set_approvals(Origin::signed(4), vec![true], 0, 1, 40));
@@ -357,7 +357,7 @@ fn voting_cannot_lock_less_than_limit() {
 
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(3), vec![], 0, 0, 4),
-			"locked value must be more than limit",
+			Error::<Test>::InsufficientLockedValue,
 		);
 		assert_ok!(Elections::set_approvals(Origin::signed(3), vec![], 0, 0, 5));
 	});
@@ -414,7 +414,7 @@ fn voting_without_any_candidate_count_should_not_work() {
 
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(4), vec![], 0, 0, 40),
-			"amount of candidates to receive approval votes should be non-zero"
+			Error::<Test>::ZeroCandidates,
 		);
 	});
 }
@@ -429,7 +429,7 @@ fn voting_setting_an_approval_vote_count_more_than_candidate_count_should_not_wo
 
 		assert_noop!(
 			Elections::set_approvals(Origin::signed(4),vec![true, true], 0, 0, 40),
-			"amount of candidate votes cannot exceed amount of candidates"
+			Error::<Test>::TooManyVotes,
 		);
 	});
 }
@@ -507,7 +507,7 @@ fn voting_invalid_retraction_index_should_not_work() {
 		assert_ok!(Elections::set_approvals(Origin::signed(1), vec![true], 0, 0, 10));
 		assert_ok!(Elections::set_approvals(Origin::signed(2), vec![true], 0, 0, 20));
 		assert_eq!(voter_ids(), vec![1, 2]);
-		assert_noop!(Elections::retract_voter(Origin::signed(1), 1), "retraction index mismatch");
+		assert_noop!(Elections::retract_voter(Origin::signed(1), 1), Error::<Test>::InvalidRetractionIndex);
 	});
 }
 
@@ -518,7 +518,7 @@ fn voting_overflow_retraction_index_should_not_work() {
 		assert_ok!(Elections::submit_candidacy(Origin::signed(3), 0));
 
 		assert_ok!(Elections::set_approvals(Origin::signed(1), vec![true], 0, 0, 10));
-		assert_noop!(Elections::retract_voter(Origin::signed(1), 1), "retraction index invalid");
+		assert_noop!(Elections::retract_voter(Origin::signed(1), 1), Error::<Test>::InvalidRetractionIndex);
 	});
 }
 
@@ -529,7 +529,7 @@ fn voting_non_voter_retraction_should_not_work() {
 		assert_ok!(Elections::submit_candidacy(Origin::signed(3), 0));
 
 		assert_ok!(Elections::set_approvals(Origin::signed(1), vec![true], 0, 0, 10));
-		assert_noop!(Elections::retract_voter(Origin::signed(2), 0), "cannot retract non-voter");
+		assert_noop!(Elections::retract_voter(Origin::signed(2), 0), Error::<Test>::RetractNonVoter);
 	});
 }
 
@@ -627,7 +627,7 @@ fn retracting_inactive_voter_with_bad_reporter_index_should_not_work() {
 			42,
 			2, (voter_ids().iter().position(|&i| i == 2).unwrap() as u32).into(),
 			2
-		), "invalid reporter index");
+		), Error::<Test>::InvalidReporterIndex);
 	});
 }
 
@@ -656,7 +656,7 @@ fn retracting_inactive_voter_with_bad_target_index_should_not_work() {
 			(voter_ids().iter().position(|&i| i == 2).unwrap() as u32).into(),
 			2, 42,
 			2
-		), "invalid target index");
+		), Error::<Test>::InvalidTargetIndex);
 	});
 }
 
@@ -733,7 +733,7 @@ fn retracting_inactive_voter_by_nonvoter_should_not_work() {
 			0,
 			2, (voter_ids().iter().position(|&i| i == 2).unwrap() as u32).into(),
 			2
-		), "reporter must be a voter");
+		), Error::<Test>::NotVoter);
 	});
 }
 
@@ -803,7 +803,7 @@ fn candidacy_submission_not_using_free_slot_should_not_work() {
 		System::set_block_number(1);
 		assert_noop!(
 			Elections::submit_candidacy(Origin::signed(4), 3),
-			"invalid candidate slot"
+			Error::<Test>::InvalidCandidateSlot
 		);
 	});
 }
@@ -815,7 +815,7 @@ fn candidacy_bad_candidate_slot_submission_should_not_work() {
 		assert_eq!(Elections::candidates(), Vec::<u64>::new());
 		assert_noop!(
 			Elections::submit_candidacy(Origin::signed(1), 1),
-			"invalid candidate slot"
+			Error::<Test>::InvalidCandidateSlot
 		);
 	});
 }
@@ -829,7 +829,7 @@ fn candidacy_non_free_candidate_slot_submission_should_not_work() {
 		assert_eq!(Elections::candidates(), vec![1]);
 		assert_noop!(
 			Elections::submit_candidacy(Origin::signed(2), 0),
-			"invalid candidate slot"
+			Error::<Test>::InvalidCandidateSlot
 		);
 	});
 }
@@ -843,7 +843,7 @@ fn candidacy_dupe_candidate_submission_should_not_work() {
 		assert_eq!(Elections::candidates(), vec![1]);
 		assert_noop!(
 			Elections::submit_candidacy(Origin::signed(1), 1),
-			"duplicate candidate submission"
+			Error::<Test>::DuplicatedCandidate,
 		);
 	});
 }
@@ -855,7 +855,7 @@ fn candidacy_poor_candidate_submission_should_not_work() {
 		assert_eq!(Elections::candidates(), Vec::<u64>::new());
 		assert_noop!(
 			Elections::submit_candidacy(Origin::signed(7), 0),
-			"candidate has not enough funds"
+			Error::<Test>::InsufficientCandidateFunds,
 		);
 	});
 }
@@ -1014,7 +1014,7 @@ fn election_presentations_with_zero_staked_deposit_should_not_work() {
 		System::set_block_number(6);
 		assert_noop!(
 			Elections::present_winner(Origin::signed(4), 2, 0, 0),
-			"stake deposited to present winner and be added to leaderboard should be non-zero"
+			Error::<Test>::ZeroDeposit,
 		);
 	});
 }
@@ -1036,7 +1036,7 @@ fn election_double_presentations_should_be_punished() {
 		assert_ok!(Elections::present_winner(Origin::signed(4), 5, 50, 0));
 		assert_eq!(
 			Elections::present_winner(Origin::signed(4), 5, 50, 0),
-			Err("duplicate presentation".into()),
+			Err(Error::<Test>::DuplicatedPresentation.into()),
 		);
 		assert_ok!(Elections::end_block(System::block_number()));
 
@@ -1067,7 +1067,7 @@ fn election_presenting_for_double_election_should_not_work() {
 		System::set_block_number(10);
 		assert_noop!(
 			Elections::present_winner(Origin::signed(4), 2, 20, 1),
-			"candidate must not form a duplicated member if elected"
+			Error::<Test>::DuplicatedCandidate,
 		);
 	});
 }
@@ -1101,7 +1101,7 @@ fn election_presenting_loser_should_not_work() {
 			(60, 1)
 		]));
 
-		assert_noop!(Elections::present_winner(Origin::signed(4), 2, 20, 0), "candidate not worthy of leaderboard");
+		assert_noop!(Elections::present_winner(Origin::signed(4), 2, 20, 0), Error::<Test>::UnworthyCandidate);
 	});
 }
 
@@ -1144,7 +1144,7 @@ fn election_present_outside_of_presentation_period_should_not_work() {
 		assert!(!Elections::presentation_active());
 		assert_noop!(
 			Elections::present_winner(Origin::signed(5), 5, 1, 0),
-			"cannot present outside of presentation period"
+			Error::<Test>::NotPresentationPeriod,
 		);
 	});
 }
@@ -1160,7 +1160,7 @@ fn election_present_with_invalid_vote_index_should_not_work() {
 		assert_ok!(Elections::end_block(System::block_number()));
 
 		System::set_block_number(6);
-		assert_noop!(Elections::present_winner(Origin::signed(4), 2, 20, 1), "index not current");
+		assert_noop!(Elections::present_winner(Origin::signed(4), 2, 20, 1), Error::<Test>::InvalidVoteIndex);
 	});
 }
 
@@ -1190,7 +1190,7 @@ fn election_present_when_presenter_is_poor_should_not_work() {
 				if p > 5 {
 					assert_noop!(Elections::present_winner(
 						Origin::signed(1), 1, 10, 0),
-						"presenter must have sufficient slashable funds"
+						Error::<Test>::InsufficientPresenterFunds,
 					);
 				} else {
 					assert_ok!(Elections::present_winner(Origin::signed(1), 1, 10, 0));
@@ -1215,7 +1215,7 @@ fn election_invalid_present_tally_should_slash() {
 		assert_ok!(Elections::end_block(System::block_number()));
 
 		System::set_block_number(6);
-		assert_err!(Elections::present_winner(Origin::signed(4), 2, 80, 0), "incorrect total");
+		assert_err!(Elections::present_winner(Origin::signed(4), 2, 80, 0), Error::<Test>::IncorrectTotal);
 
 		assert_eq!(Balances::total_balance(&4), 38);
 	});