From 2403cf320c2c35265ec94ba0403dff75cbbbde3f Mon Sep 17 00:00:00 2001 From: Stanislav Tkach Date: Tue, 24 Dec 2019 12:11:57 +0200 Subject: [PATCH] Migrate election-phragmen, election contracts and authorship to decl_error (#4479) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Migrate election-phragmen * Migrate elections * Migrate contracts module * Update authorship module * Apply suggestions from code review Co-authored-by: Bastian Köcher --- substrate/frame/authorship/src/lib.rs | 52 +++++-- substrate/frame/contracts/src/lib.rs | 37 +++-- substrate/frame/elections-phragmen/src/lib.rs | 96 +++++++++---- substrate/frame/elections/src/lib.rs | 135 +++++++++++++----- substrate/frame/elections/src/tests.rs | 50 +++---- substrate/frame/generic-asset/src/lib.rs | 2 +- 6 files changed, 255 insertions(+), 117 deletions(-) diff --git a/substrate/frame/authorship/src/lib.rs b/substrate/frame/authorship/src/lib.rs index 5c2f964220..d8fd742026 100644 --- a/substrate/frame/authorship/src/lib.rs +++ b/substrate/frame/authorship/src/lib.rs @@ -22,7 +22,7 @@ use sp_std::{result, prelude::*}; use sp_std::collections::btree_set::BTreeSet; -use frame_support::{decl_module, decl_storage, dispatch, ensure}; +use frame_support::{decl_module, decl_storage, decl_error, dispatch, ensure}; use frame_support::traits::{FindAuthor, VerifySeal, Get}; use codec::{Encode, Decode}; use frame_system::ensure_none; @@ -161,8 +161,30 @@ decl_storage! { } } +decl_error! { + /// Error for the authorship module. + pub enum Error for Module { + /// The uncle parent not in the chain. + InvalidUncleParent, + /// Uncles already set in the block. + UnclesAlreadySet, + /// Too many uncles. + TooManyUncles, + /// The uncle is genesis. + GenesisUncle, + /// The uncle is too high in chain. + TooHighUncle, + /// The uncle is already included. + UncleAlreadyIncluded, + /// The uncle isn't recent enough to be included. + OldUncle, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn on_initialize(now: T::BlockNumber) { let uncle_generations = T::UncleGenerations::get(); // prune uncles that are older than the allowed number of generations. @@ -186,10 +208,10 @@ decl_module! { #[weight = SimpleDispatchInfo::FixedOperational(10_000)] fn set_uncles(origin, new_uncles: Vec) -> dispatch::DispatchResult { ensure_none(origin)?; - ensure!(new_uncles.len() <= MAX_UNCLES, "Too many uncles"); + ensure!(new_uncles.len() <= MAX_UNCLES, Error::::TooManyUncles); if ::DidSetUncles::get() { - Err("Uncles already set in block.")? + Err(Error::::UnclesAlreadySet)? } ::DidSetUncles::put(true); @@ -251,7 +273,7 @@ impl Module { uncle: &T::Header, existing_uncles: I, accumulator: &mut >::Accumulator, - ) -> Result, &'static str> + ) -> Result, dispatch::DispatchError> { let now = >::block_number(); @@ -269,34 +291,34 @@ impl Module { let hash = uncle.hash(); if uncle.number() < &One::one() { - return Err("uncle is genesis"); + return Err(Error::::GenesisUncle.into()); } if uncle.number() > &maximum_height { - return Err("uncle is too high in chain"); + return Err(Error::::TooHighUncle.into()); } { let parent_number = uncle.number().clone() - One::one(); let parent_hash = >::block_hash(&parent_number); if &parent_hash != uncle.parent_hash() { - return Err("uncle parent not in chain"); + return Err(Error::::InvalidUncleParent.into()); } } if uncle.number() < &minimum_height { - return Err("uncle not recent enough to be included"); + return Err(Error::::OldUncle.into()); } let duplicate = existing_uncles.into_iter().find(|h| **h == hash).is_some(); let in_chain = >::block_hash(uncle.number()) == hash; if duplicate || in_chain { - return Err("uncle already included") + return Err(Error::::UncleAlreadyIncluded.into()) } // check uncle validity. - T::FilterUncle::filter_uncle(&uncle, accumulator) + T::FilterUncle::filter_uncle(&uncle, accumulator).map_err(|e| Into::into(e)) } fn prune_old_uncles(minimum_height: T::BlockNumber) { @@ -360,7 +382,7 @@ impl ProvideInherent for Module { fn check_inherent(call: &Self::Call, _data: &InherentData) -> result::Result<(), Self::Error> { match call { Call::set_uncles(ref uncles) if uncles.len() > MAX_UNCLES => { - Err(InherentError::Uncles("Too many uncles".into())) + Err(InherentError::Uncles(Error::::TooManyUncles.as_str().into())) }, _ => { Ok(()) @@ -566,7 +588,7 @@ mod tests { ); assert_eq!( Authorship::verify_and_import_uncles(vec![uncle_a.clone(), uncle_a.clone()]), - Err("uncle already included".into()), + Err(Error::::UncleAlreadyIncluded.into()), ); } @@ -581,7 +603,7 @@ mod tests { assert_eq!( Authorship::verify_and_import_uncles(vec![uncle_a.clone()]), - Err("uncle already included".into()), + Err(Error::::UncleAlreadyIncluded.into()), ); } @@ -591,7 +613,7 @@ mod tests { assert_eq!( Authorship::verify_and_import_uncles(vec![uncle_clone]), - Err("uncle already included".into()), + Err(Error::::UncleAlreadyIncluded.into()), ); } @@ -615,7 +637,7 @@ mod tests { assert_eq!( Authorship::verify_and_import_uncles(vec![gen_2]), - Err("uncle not recent enough to be included".into()), + Err(Error::::OldUncle.into()), ); } diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index d1bea0ca98..d17f09cda8 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -121,7 +121,7 @@ use sp_runtime::{ }; use frame_support::dispatch::{DispatchResult, Dispatchable}; use frame_support::{ - Parameter, decl_module, decl_event, decl_storage, storage::child, + Parameter, decl_module, decl_event, decl_storage, decl_error, storage::child, parameter_types, IsSubType, weights::DispatchInfo, }; @@ -467,9 +467,29 @@ impl ComputeDispatchFee<::Call, BalanceOf> for DefaultD } } +decl_error! { + /// Error for the contracts module. + pub enum Error for Module { + /// A new schedule must have a greater version than the current one. + InvalidScheduleVersion, + /// An origin must be signed or inherent and auxiliary sender only provided on inherent. + InvalidSurchargeClaim, + /// Cannot restore from nonexisting or tombstone contract. + InvalidSourceContract, + /// Cannot restore to nonexisting or alive contract. + InvalidDestinationContract, + /// Tombstones don't match. + InvalidTombstone, + /// An origin TrieId written in the current block. + InvalidContractOrigin + } +} + decl_module! { /// Contracts module. pub struct Module for enum Call where origin: ::Origin { + type Error = Error; + /// Number of block delay an extrinsic claim surcharge has. /// /// When claim surcharge is called by an extrinsic the rent is checked @@ -542,7 +562,7 @@ decl_module! { pub fn update_schedule(origin, schedule: Schedule) -> DispatchResult { ensure_root(origin)?; if >::current_schedule().version >= schedule.version { - Err("new schedule must have a greater version than current")? + Err(Error::::InvalidScheduleVersion)? } Self::deposit_event(RawEvent::ScheduleUpdated(schedule.version)); @@ -636,10 +656,7 @@ decl_module! { (Ok(frame_system::RawOrigin::None), Some(aux_sender)) => { (false, aux_sender) }, - _ => Err( - "Invalid surcharge claim: origin must be signed or \ - inherent and auxiliary sender only provided on inherent" - )?, + _ => Err(Error::::InvalidSurchargeClaim)?, }; // Add some advantage for block producers (who send unsigned extrinsics) by @@ -786,17 +803,17 @@ impl Module { ) -> DispatchResult { let mut origin_contract = >::get(&origin) .and_then(|c| c.get_alive()) - .ok_or("Cannot restore from inexisting or tombstone contract")?; + .ok_or(Error::::InvalidSourceContract)?; let current_block = >::block_number(); if origin_contract.last_write == Some(current_block) { - Err("Origin TrieId written in the current block")? + Err(Error::::InvalidContractOrigin)? } let dest_tombstone = >::get(&dest) .and_then(|c| c.get_tombstone()) - .ok_or("Cannot restore to inexisting or alive contract")?; + .ok_or(Error::::InvalidDestinationContract)?; let last_write = if !delta.is_empty() { Some(current_block) @@ -841,7 +858,7 @@ impl Module { ); } - return Err("Tombstones don't match".into()); + return Err(Error::::InvalidTombstone.into()); } origin_contract.storage_size -= key_values_taken.iter() diff --git a/substrate/frame/elections-phragmen/src/lib.rs b/substrate/frame/elections-phragmen/src/lib.rs index 8e93775649..8c12f314f8 100644 --- a/substrate/frame/elections-phragmen/src/lib.rs +++ b/substrate/frame/elections-phragmen/src/lib.rs @@ -83,9 +83,9 @@ #![cfg_attr(not(feature = "std"), no_std)] use sp_std::prelude::*; -use sp_runtime::{print, DispatchResult, traits::{Zero, StaticLookup, Bounded, Convert}}; +use sp_runtime::{print, DispatchResult, DispatchError, traits::{Zero, StaticLookup, Bounded, Convert}}; use frame_support::{ - decl_storage, decl_event, ensure, decl_module, weights::SimpleDispatchInfo, + decl_storage, decl_event, ensure, decl_module, decl_error, weights::SimpleDispatchInfo, traits::{ Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons, ChangeMembers, OnUnbalanced, WithdrawReason @@ -167,8 +167,44 @@ decl_storage! { } } +decl_error! { + /// Error for the elections-phragmen module. + pub enum Error for Module { + /// Cannot vote when no candidates or members exist. + UnableToVote, + /// Must vote for at least one candidate. + NoVotes, + /// Cannot vote more than candidates. + TooManyVotes, + /// Cannot vote more than maximum allowed. + MaximumVotesExceeded, + /// Cannot vote with stake less than minimum balance. + LowBalance, + /// Voter can not pay voting bond. + UnableToPayBond, + /// Must be a voter. + MustBeVoter, + /// Cannot report self. + ReportSelf, + /// Duplicated candidate submission. + DuplicatedCandidate, + /// Member cannot re-submit candidacy. + MemberSubmit, + /// Runner cannot re-submit candidacy. + RunnerSubmit, + /// Candidate does not have enough funds. + InsufficientCandidateFunds, + /// Origin is not a candidate, member or a runner up. + InvalidOrigin, + /// Not a member. + NotMember, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; const CandidacyBond: BalanceOf = T::CandidacyBond::get(); @@ -201,20 +237,20 @@ decl_module! { // addition is valid: candidates and members never overlap. let allowed_votes = candidates_count + members_count; - ensure!(!allowed_votes.is_zero(), "cannot vote when no candidates or members exist"); - ensure!(votes.len() <= allowed_votes, "cannot vote more than candidates"); - ensure!(votes.len() <= MAXIMUM_VOTE, "cannot vote more than maximum allowed"); - ensure!(!votes.is_empty(), "must vote for at least one candidate."); + ensure!(!allowed_votes.is_zero(), Error::::UnableToVote); + ensure!(votes.len() <= allowed_votes, Error::::TooManyVotes); + ensure!(votes.len() <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); + ensure!(!votes.is_empty(), Error::::NoVotes); ensure!( value > T::Currency::minimum_balance(), - "cannot vote with stake less than minimum balance" + Error::::LowBalance, ); if !Self::is_voter(&who) { // first time voter. Reserve bond. T::Currency::reserve(&who, T::VotingBond::get()) - .map_err(|_| "voter can not pay voting bond")?; + .map_err(|_| Error::::UnableToPayBond)?; } // Amount to be locked up. let locked_balance = value.min(T::Currency::total_balance(&who)); @@ -242,7 +278,7 @@ decl_module! { fn remove_voter(origin) { let who = ensure_signed(origin)?; - ensure!(Self::is_voter(&who), "must be a voter"); + ensure!(Self::is_voter(&who), Error::::MustBeVoter); Self::do_remove_voter(&who, true); } @@ -265,8 +301,8 @@ decl_module! { let reporter = ensure_signed(origin)?; let target = T::Lookup::lookup(target)?; - ensure!(reporter != target, "cannot report self"); - ensure!(Self::is_voter(&reporter), "reporter must be a voter"); + ensure!(reporter != target, Error::::ReportSelf); + ensure!(Self::is_voter(&reporter), Error::::MustBeVoter); // Checking if someone is a candidate and a member here is O(LogN), making the whole // function O(MLonN) with N candidates in total and M of them being voted by `target`. @@ -308,15 +344,15 @@ decl_module! { let who = ensure_signed(origin)?; let is_candidate = Self::is_candidate(&who); - ensure!(is_candidate.is_err(), "duplicate candidate submission"); + ensure!(is_candidate.is_err(), Error::::DuplicatedCandidate); // assured to be an error, error always contains the index. let index = is_candidate.unwrap_err(); - ensure!(!Self::is_member(&who), "member cannot re-submit candidacy"); - ensure!(!Self::is_runner(&who), "runner cannot re-submit candidacy"); + ensure!(!Self::is_member(&who), Error::::MemberSubmit); + ensure!(!Self::is_runner(&who), Error::::RunnerSubmit); T::Currency::reserve(&who, T::CandidacyBond::get()) - .map_err(|_| "candidate does not have enough funds")?; + .map_err(|_| Error::::InsufficientCandidateFunds)?; >::mutate(|c| c.insert(index, who)); } @@ -373,7 +409,7 @@ decl_module! { return Ok(()); } - Err("origin is not a candidate, member or a runner up.")? + Err(Error::::InvalidOrigin)? } /// Remove a particular member from the set. This is effective immediately and the bond of @@ -402,7 +438,7 @@ decl_module! { if !had_replacement { Self::do_phragmen(); } - }).map_err(Into::into) + }) } /// What to do at the end of each block. Checks if an election needs to happen or not. @@ -444,7 +480,7 @@ impl Module { /// accordingly. Furthermore, the membership change is reported. /// /// O(phragmen) in the worse case. - fn remove_and_replace_member(who: &T::AccountId) -> Result { + fn remove_and_replace_member(who: &T::AccountId) -> Result { let mut members_with_stake = Self::members(); if let Ok(index) = members_with_stake.binary_search_by(|(ref m, ref _s)| m.cmp(who)) { members_with_stake.remove(index); @@ -469,7 +505,7 @@ impl Module { } result } else { - Err("not a member") + Err(Error::::NotMember)? } } @@ -1072,7 +1108,7 @@ mod tests { assert_eq!(Elections::candidates(), vec![1]); assert_noop!( Elections::submit_candidacy(Origin::signed(1)), - "duplicate candidate submission" + Error::::DuplicatedCandidate, ); }); } @@ -1093,7 +1129,7 @@ mod tests { assert_noop!( Elections::submit_candidacy(Origin::signed(5)), - "member cannot re-submit candidacy" + Error::::MemberSubmit, ); }); } @@ -1116,7 +1152,7 @@ mod tests { assert_noop!( Elections::submit_candidacy(Origin::signed(3)), - "runner cannot re-submit candidacy", + Error::::RunnerSubmit, ); }); } @@ -1127,7 +1163,7 @@ mod tests { assert_eq!(Elections::candidates(), Vec::::new()); assert_noop!( Elections::submit_candidacy(Origin::signed(7)), - "candidate does not have enough funds", + Error::::InsufficientCandidateFunds, ); }); } @@ -1186,7 +1222,7 @@ mod tests { ExtBuilder::default().build().execute_with(|| { assert_noop!( Elections::vote(Origin::signed(2), vec![], 20), - "cannot vote when no candidates or members exist" + Error::::UnableToVote, ); }); } @@ -1217,7 +1253,7 @@ mod tests { assert_noop!( Elections::vote(Origin::signed(2), vec![10, 20, 30], 20), - "cannot vote more than candidates", + Error::::TooManyVotes, ); }); } @@ -1230,7 +1266,7 @@ mod tests { assert_noop!( Elections::vote(Origin::signed(2), vec![4], 1), - "cannot vote with stake less than minimum balance", + Error::::LowBalance, ); }) } @@ -1276,7 +1312,7 @@ mod tests { #[test] fn non_voter_remove_should_not_work() { ExtBuilder::default().build().execute_with(|| { - assert_noop!(Elections::remove_voter(Origin::signed(3)), "must be a voter"); + assert_noop!(Elections::remove_voter(Origin::signed(3)), Error::::MustBeVoter); }); } @@ -1289,7 +1325,7 @@ mod tests { assert_ok!(Elections::remove_voter(Origin::signed(2))); assert_eq!(all_voters(), vec![]); - assert_noop!(Elections::remove_voter(Origin::signed(2)), "must be a voter"); + assert_noop!(Elections::remove_voter(Origin::signed(2)), Error::::MustBeVoter); }); } @@ -1318,7 +1354,7 @@ mod tests { ExtBuilder::default().build().execute_with(|| { assert_noop!( Elections::report_defunct_voter(Origin::signed(1), 2), - "reporter must be a voter", + Error::::MustBeVoter, ); }); } @@ -2032,7 +2068,7 @@ mod tests { ExtBuilder::default().build().execute_with(|| { assert_noop!( Elections::renounce_candidacy(Origin::signed(5)), - "origin is not a candidate, member or a runner up.", + Error::::InvalidOrigin, ); }) } diff --git a/substrate/frame/elections/src/lib.rs b/substrate/frame/elections/src/lib.rs index 1435c05951..9b3dc61540 100644 --- a/substrate/frame/elections/src/lib.rs +++ b/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 { + /// 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 for enum Call where origin: T::Origin { + type Error = Error; + /// How much should be locked up in order to submit one's candidacy. A reasonable /// default value is 9. const CandidacyBond: BalanceOf = T::CandidacyBond::get(); @@ -363,7 +427,7 @@ decl_module! { hint: SetIndex, #[compact] value: BalanceOf ) -> DispatchResult { - let who = Self::proxy(ensure_signed(origin)?).ok_or("not a proxy")?; + let who = Self::proxy(ensure_signed(origin)?).ok_or(Error::::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::::CannotReapPresenting); + ensure!(Self::voter_info(&reporter).is_some(), Error::::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::::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::::InvalidVoteIndex); ensure!( assumed_vote_index > last_active + T::InactiveGracePeriod::get(), - "cannot reap during grace period" + Error::::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::::InvalidReporterIndex)?; + let assumed_who = Self::voter_at(who_index).ok_or(Error::::InvalidTargetIndex)?; - ensure!(assumed_reporter == reporter, "bad reporter index"); - ensure!(assumed_who == who, "bad target index"); + ensure!(assumed_reporter == reporter, Error::::InvalidReporterIndex); + ensure!(assumed_who == who, Error::::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!(>::exists(&who), "cannot retract non-voter"); + ensure!(!Self::presentation_active(), Error::::CannotRetractPresenting); + ensure!(>::exists(&who), Error::::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::::InvalidRetractionIndex)?; + ensure!(voter == who, Error::::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::::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::::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::::InsufficientCandidateFunds)?; >::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::::ZeroDeposit, ); let candidate = T::Lookup::lookup(candidate)?; - ensure!(index == Self::vote_index(), "index not current"); + ensure!(index == Self::vote_index(), Error::::InvalidVoteIndex); let (_, _, expiring) = Self::next_finalize() - .ok_or("cannot present outside of presentation period")?; + .ok_or(Error::::NotPresentationPeriod)?; let bad_presentation_punishment = T::PresentSlashPerVoter::get() * BalanceOf::::from(Self::voter_count() as u32); ensure!( T::Currency::can_slash(&who, bad_presentation_punishment), - "presenter must have sufficient slashable funds" + Error::::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::::LeaderboardMustExist)?; + ensure!(total > leaderboard[0].0, Error::::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::::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::::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::::DuplicatedPresentation } else { Error::::IncorrectTotal })? } } @@ -752,11 +815,11 @@ impl Module { ) -> DispatchResult { let candidates_len = ::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::::ApprovalPresentation); + ensure!(index == Self::vote_index(), Error::::InvalidVoteIndex); ensure!( !candidates_len.is_zero(), - "amount of candidates to receive approval votes should be non-zero" + Error::::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 Module { // bond would cover. ensure!( candidates_len >= votes.len(), - "amount of candidate votes cannot exceed amount of candidates" + Error::::TooManyVotes, ); - ensure!(value >= T::MinimumVotingLock::get(), "locked value must be more than limit"); + ensure!(value >= T::MinimumVotingLock::get(), Error::::InsufficientLockedValue); // Amount to be locked up. let mut locked_balance = value.min(T::Currency::total_balance(&who)); @@ -775,8 +838,8 @@ impl Module { 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::::InvalidVoterIndex)?; + ensure!(voter == who, Error::::InvalidVoterIndex); // write new accumulated offset. let last_win = info.last_win; @@ -787,7 +850,7 @@ impl Module { // 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::::InsufficientVoterFunds, ); let (set_index, vec_index) = Self::split_index(hint, VOTER_SET_SIZE); diff --git a/substrate/frame/elections/src/tests.rs b/substrate/frame/elections/src/tests.rs index d7d7e8718b..40acb72f9e 100644 --- a/substrate/frame/elections/src/tests.rs +++ b/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::::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::::InvalidVoterIndex, ); // wrong index assert_noop!( Elections::set_approvals(Origin::signed(4), vec![true], 0, 0, 40), - "wrong voter index" + Error::::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::::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::::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::::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::::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::::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::::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::::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::::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::::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::::InvalidCandidateSlot ); }); } @@ -815,7 +815,7 @@ fn candidacy_bad_candidate_slot_submission_should_not_work() { assert_eq!(Elections::candidates(), Vec::::new()); assert_noop!( Elections::submit_candidacy(Origin::signed(1), 1), - "invalid candidate slot" + Error::::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::::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::::DuplicatedCandidate, ); }); } @@ -855,7 +855,7 @@ fn candidacy_poor_candidate_submission_should_not_work() { assert_eq!(Elections::candidates(), Vec::::new()); assert_noop!( Elections::submit_candidacy(Origin::signed(7), 0), - "candidate has not enough funds" + Error::::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::::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::::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::::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::::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::::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::::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::::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::::IncorrectTotal); assert_eq!(Balances::total_balance(&4), 38); }); diff --git a/substrate/frame/generic-asset/src/lib.rs b/substrate/frame/generic-asset/src/lib.rs index d332d63c4e..26f5161d87 100644 --- a/substrate/frame/generic-asset/src/lib.rs +++ b/substrate/frame/generic-asset/src/lib.rs @@ -321,7 +321,7 @@ impl Into> for PermissionLatest { /// No new assets id available. NoIdAvailable,