diff --git a/substrate/frame/assets/src/lib.rs b/substrate/frame/assets/src/lib.rs index 87e74c0c6c..3beb247170 100644 --- a/substrate/frame/assets/src/lib.rs +++ b/substrate/frame/assets/src/lib.rs @@ -84,7 +84,8 @@ //! ### Simple Code Snippet //! //! ```rust,ignore -//! use frame_support::{decl_module, dispatch}; +//! use pallet_assets as assets; +//! use frame_support::{decl_module, dispatch, ensure}; //! use frame_system::{self as system, ensure_signed}; //! //! pub trait Trait: assets::Trait { } @@ -92,14 +93,15 @@ //! decl_module! { //! pub struct Module for enum Call where origin: T::Origin { //! pub fn issue_token_airdrop(origin) -> dispatch::Result { +//! let sender = ensure_signed(origin).map_err(|e| e.as_str())?; +//! //! const ACCOUNT_ALICE: u64 = 1; //! const ACCOUNT_BOB: u64 = 2; -//! const COUNT_AIRDROP_RECIPIENTS = 2; +//! const COUNT_AIRDROP_RECIPIENTS: u64 = 2; //! const TOKENS_FIXED_SUPPLY: u64 = 100; //! //! ensure!(!COUNT_AIRDROP_RECIPIENTS.is_zero(), "Divide by zero error."); //! -//! let sender = ensure_signed(origin)?; //! let asset_id = Self::next_asset_id(); //! //! >::mutate(|asset_id| *asset_id += 1); @@ -130,8 +132,8 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use frame_support::{Parameter, decl_module, decl_event, decl_storage, ensure}; -use sp_runtime::traits::{Member, SimpleArithmetic, Zero, StaticLookup}; +use frame_support::{Parameter, decl_module, decl_event, decl_storage, decl_error, ensure}; +use sp_runtime::traits::{Member, SimpleArithmetic, Zero, StaticLookup, ModuleDispatchError}; use frame_system::{self as system, ensure_signed}; use sp_runtime::traits::One; @@ -149,12 +151,14 @@ pub trait Trait: frame_system::Trait { decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; /// Issue a new class of fungible assets. There are, and will only ever be, `total` /// such assets and they'll all belong to the `origin` initially. It will have an /// identifier `AssetId` instance: this will be specified in the `Issued` event. fn issue(origin, #[compact] total: T::Balance) { - let origin = ensure_signed(origin)?; + let origin = ensure_signed(origin).map_err(|e| e.as_str())?; let id = Self::next_asset_id(); >::mutate(|id| *id += One::one()); @@ -171,12 +175,12 @@ decl_module! { target: ::Source, #[compact] amount: T::Balance ) { - let origin = ensure_signed(origin)?; + let origin = ensure_signed(origin).map_err(|e| e.as_str())?; let origin_account = (id, origin.clone()); let origin_balance = >::get(&origin_account); let target = T::Lookup::lookup(target)?; - ensure!(!amount.is_zero(), "transfer amount should be non-zero"); - ensure!(origin_balance >= amount, "origin account balance must be greater than or equal to the transfer amount"); + ensure!(!amount.is_zero(), Error::AmountZero); + ensure!(origin_balance >= amount, Error::BalanceLow); Self::deposit_event(RawEvent::Transferred(id, origin, target.clone(), amount)); >::insert(origin_account, origin_balance - amount); @@ -185,9 +189,9 @@ decl_module! { /// Destroy any assets of `id` owned by `origin`. fn destroy(origin, #[compact] id: T::AssetId) { - let origin = ensure_signed(origin)?; + let origin = ensure_signed(origin).map_err(|e| e.as_str())?; let balance = >::take((id, &origin)); - ensure!(!balance.is_zero(), "origin balance should be non-zero"); + ensure!(!balance.is_zero(), Error::BalanceZero); >::mutate(id, |total_supply| *total_supply -= balance); Self::deposit_event(RawEvent::Destroyed(id, origin, balance)); @@ -195,7 +199,7 @@ decl_module! { } } -decl_event!( +decl_event! { pub enum Event where ::AccountId, ::Balance, @@ -208,7 +212,19 @@ decl_event!( /// Some assets were destroyed. Destroyed(AssetId, AccountId, Balance), } -); +} + +decl_error! { + pub enum Error { + /// Transfer amount should be non-zero + AmountZero, + /// Account balance must be greater than or equal to the transfer amount + BalanceLow, + /// Balance should be non-zero + BalanceZero, + } + +} decl_storage! { trait Store for Module as Assets { @@ -337,7 +353,7 @@ mod tests { assert_eq!(Assets::balance(0, 2), 50); assert_ok!(Assets::destroy(Origin::signed(1), 0)); assert_eq!(Assets::balance(0, 1), 0); - assert_noop!(Assets::transfer(Origin::signed(1), 0, 1, 50), "origin account balance must be greater than or equal to the transfer amount"); + assert_noop!(Assets::transfer(Origin::signed(1), 0, 1, 50), Error::BalanceLow); }); } @@ -346,7 +362,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Assets::issue(Origin::signed(1), 100)); assert_eq!(Assets::balance(0, 1), 100); - assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 0), "transfer amount should be non-zero"); + assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 0), Error::AmountZero); }); } @@ -355,7 +371,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Assets::issue(Origin::signed(1), 100)); assert_eq!(Assets::balance(0, 1), 100); - assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 101), "origin account balance must be greater than or equal to the transfer amount"); + assert_noop!(Assets::transfer(Origin::signed(1), 0, 2, 101), Error::BalanceLow); }); } @@ -373,7 +389,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Assets::issue(Origin::signed(1), 100)); assert_eq!(Assets::balance(0, 2), 0); - assert_noop!(Assets::destroy(Origin::signed(2), 0), "origin balance should be non-zero"); + assert_noop!(Assets::destroy(Origin::signed(2), 0), Error::BalanceZero); }); } } diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index 1fd718bc01..11134c87de 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -26,12 +26,12 @@ use sp_std::{prelude::*, result}; use sp_core::u32_trait::Value as U32; use sp_runtime::RuntimeDebug; -use sp_runtime::traits::{Hash, EnsureOrigin}; +use sp_runtime::traits::{Hash, EnsureOrigin, ModuleDispatchError}; use frame_support::weights::SimpleDispatchInfo; use frame_support::{ dispatch::{Dispatchable, Parameter}, codec::{Encode, Decode}, traits::{ChangeMembers, InitializeMembers}, decl_module, decl_event, - decl_storage, ensure, + decl_storage, decl_error, ensure, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -102,7 +102,7 @@ decl_storage! { } } -decl_event!( +decl_event! { pub enum Event where ::Hash, ::AccountId, @@ -122,13 +122,32 @@ decl_event!( /// A single member did some action; `bool` is true if returned without error. MemberExecuted(Hash, bool), } -); +} + +decl_error! { + pub enum Error { + /// Account is not a member + NotMember, + /// Duplicate proposals not allowed + DuplicateProposal, + /// Proposal must exist + ProposalMissing, + /// Mismatched index + WrongIndex, + /// Duplicate vote ignored + DuplicateVote, + /// Members are already initialized! + AlreadyInitialized, + } +} // Note: this module is not benchmarked. The weights are obtained based on the similarity of the // executed logic with other democracy function. Note that councillor operations are assigned to the // operational class. decl_module! { pub struct Module, I: Instance=DefaultInstance> for enum Call where origin: ::Origin { + type Error = Error; + fn deposit_event() = default; /// Set the collective's membership manually to `new_members`. Be nice to the chain and @@ -137,7 +156,7 @@ decl_module! { /// Requires root origin. #[weight = SimpleDispatchInfo::FixedOperational(100_000)] fn set_members(origin, new_members: Vec) { - ensure_root(origin)?; + ensure_root(origin).map_err(|e| e.as_str())?; let mut new_members = new_members; new_members.sort(); >::mutate(|m| { @@ -151,8 +170,8 @@ decl_module! { /// Origin must be a member of the collective. #[weight = SimpleDispatchInfo::FixedOperational(100_000)] fn execute(origin, proposal: Box<>::Proposal>) { - let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), "proposer not a member"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(Self::is_member(&who), Error::NotMember); let proposal_hash = T::Hashing::hash_of(&proposal); let ok = proposal.dispatch(RawOrigin::Member(who).into()).is_ok(); @@ -165,12 +184,12 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedOperational(5_000_000)] fn propose(origin, #[compact] threshold: MemberCount, proposal: Box<>::Proposal>) { - let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), "proposer not a member"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(Self::is_member(&who), Error::NotMember); let proposal_hash = T::Hashing::hash_of(&proposal); - ensure!(!>::exists(proposal_hash), "duplicate proposals not allowed"); + ensure!(!>::exists(proposal_hash), Error::DuplicateProposal); if threshold < 2 { let seats = Self::members().len() as MemberCount; @@ -194,11 +213,11 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedOperational(200_000)] fn vote(origin, proposal: T::Hash, #[compact] index: ProposalIndex, approve: bool) { - let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), "voter not a member"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(Self::is_member(&who), Error::NotMember); - let mut voting = Self::voting(&proposal).ok_or("proposal must exist")?; - ensure!(voting.index == index, "mismatched index"); + let mut voting = Self::voting(&proposal).ok_or(Error::ProposalMissing)?; + ensure!(voting.index == index, Error::WrongIndex); let position_yes = voting.ayes.iter().position(|a| a == &who); let position_no = voting.nays.iter().position(|a| a == &who); @@ -207,7 +226,7 @@ decl_module! { if position_yes.is_none() { voting.ayes.push(who.clone()); } else { - return Err("duplicate vote ignored") + return Err(Error::DuplicateVote) } if let Some(pos) = position_no { voting.nays.swap_remove(pos); @@ -216,7 +235,7 @@ decl_module! { if position_no.is_none() { voting.nays.push(who.clone()); } else { - return Err("duplicate vote ignored") + return Err(Error::DuplicateVote) } if let Some(pos) = position_yes { voting.ayes.swap_remove(pos); @@ -565,7 +584,7 @@ mod tests { let proposal = make_proposal(42); assert_noop!( Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone())), - "proposer not a member" + Error::NotMember ); }); } @@ -579,7 +598,7 @@ mod tests { assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); assert_noop!( Collective::vote(Origin::signed(42), hash.clone(), 0, true), - "voter not a member", + Error::NotMember, ); }); } @@ -593,7 +612,7 @@ mod tests { assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); assert_noop!( Collective::vote(Origin::signed(2), hash.clone(), 1, true), - "mismatched index", + Error::WrongIndex, ); }); } @@ -611,7 +630,7 @@ mod tests { ); assert_noop!( Collective::vote(Origin::signed(1), hash.clone(), 0, true), - "duplicate vote ignored", + Error::DuplicateVote, ); assert_ok!(Collective::vote(Origin::signed(1), hash.clone(), 0, false)); assert_eq!( @@ -620,7 +639,7 @@ mod tests { ); assert_noop!( Collective::vote(Origin::signed(1), hash.clone(), 0, false), - "duplicate vote ignored", + Error::DuplicateVote, ); assert_eq!(System::events(), vec![ diff --git a/substrate/frame/democracy/src/lib.rs b/substrate/frame/democracy/src/lib.rs index ea51c4e1d9..eb9cf819c5 100644 --- a/substrate/frame/democracy/src/lib.rs +++ b/substrate/frame/democracy/src/lib.rs @@ -22,12 +22,14 @@ use sp_std::prelude::*; use sp_std::{result, convert::TryFrom}; use sp_runtime::{ RuntimeDebug, - traits::{Zero, Bounded, CheckedMul, CheckedDiv, EnsureOrigin, Hash, Dispatchable, Saturating}, + traits::{ + Zero, Bounded, CheckedMul, CheckedDiv, EnsureOrigin, Hash, + Dispatchable, Saturating, ModuleDispatchError, + }, }; -use codec::{Ref, Encode, Decode, Input, Output, Error}; +use codec::{Ref, Encode, Decode, Input, Output}; use frame_support::{ - decl_module, decl_storage, decl_event, ensure, - dispatch, + decl_module, decl_storage, decl_event, decl_error, ensure, Parameter, weights::SimpleDispatchInfo, traits::{ @@ -163,12 +165,12 @@ impl Encode for Vote { impl codec::EncodeLike for Vote {} impl Decode for Vote { - fn decode(input: &mut I) -> core::result::Result { + fn decode(input: &mut I) -> core::result::Result { let b = input.read_byte()?; Ok(Vote { aye: (b & 0b1000_0000) == 0b1000_0000, conviction: Conviction::try_from(b & 0b0111_1111) - .map_err(|_| Error::from("Invalid conviction"))?, + .map_err(|_| codec::Error::from("Invalid conviction"))?, }) } } @@ -320,7 +322,7 @@ decl_storage! { } } -decl_event!( +decl_event! { pub enum Event where Balance = BalanceOf, ::AccountId, @@ -360,10 +362,60 @@ decl_event!( /// A registered preimage was removed and the deposit collected by the reaper (last item). PreimageReaped(Hash, AccountId, Balance, AccountId), } -); +} + +decl_error! { + pub enum Error { + /// Value too low + ValueLow, + /// Proposal does not exist + ProposalMissing, + /// Not a proxy + NotProxy, + /// Unknown index + BadIndex, + /// Cannot cancel the same proposal twice + AlreadyCanceled, + /// Proposal already made + DuplicateProposal, + /// Proposal still blacklisted + ProposalBlacklisted, + /// Next external proposal not simple majority + NotSimpleMajority, + /// Invalid hash + InvalidHash, + /// No external proposal + NoProposal, + /// Identity may not veto a proposal twice + AlreadyVetoed, + /// Already a proxy + AlreadyProxy, + /// Wrong proxy + WrongProxy, + /// Not delegated + NotDelegated, + /// Preimage already noted + DuplicatePreimage, + /// Not imminent + NotImminent, + /// Too early + Early, + /// Imminent + Imminent, + /// Preimage not found + PreimageMissing, + /// Vote given for invalid referendum + ReferendumInvalid, + /// Invalid preimage + PreimageInvalid, + /// No proposals waiting + NoneWaiting, + } +} decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; /// The minimum period of locking and the period between a proposal being approved and enacted. /// /// It should generally be a little more than the unstake period to ensure that @@ -402,10 +454,9 @@ decl_module! { proposal_hash: T::Hash, #[compact] value: BalanceOf ) { - let who = ensure_signed(origin)?; - ensure!(value >= T::MinimumDeposit::get(), "value too low"); - T::Currency::reserve(&who, value) - .map_err(|_| "proposer's balance too low")?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(value >= T::MinimumDeposit::get(), Error::ValueLow); + T::Currency::reserve(&who, value)?; let index = Self::public_prop_count(); PublicPropCount::put(index + 1); @@ -425,11 +476,10 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(5_000_000)] fn second(origin, #[compact] proposal: PropIndex) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; let mut deposit = Self::deposit_of(proposal) - .ok_or("can only second an existing proposal")?; - T::Currency::reserve(&who, deposit.0) - .map_err(|_| "seconder's balance too low")?; + .ok_or(Error::ProposalMissing)?; + T::Currency::reserve(&who, deposit.0)?; deposit.1.push(who); >::insert(proposal, deposit); } @@ -445,8 +495,8 @@ decl_module! { fn vote(origin, #[compact] ref_index: ReferendumIndex, vote: Vote - ) -> dispatch::Result { - let who = ensure_signed(origin)?; + ) -> Result<(), Error> { + let who = ensure_signed(origin).map_err(|e| e.as_str())?; Self::do_vote(who, ref_index, vote) } @@ -461,8 +511,10 @@ decl_module! { fn proxy_vote(origin, #[compact] ref_index: ReferendumIndex, vote: Vote - ) -> dispatch::Result { - let who = Self::proxy(ensure_signed(origin)?).ok_or("not a proxy")?; + ) -> Result<(), Error> { + let who = Self::proxy( + ensure_signed(origin).map_err(|e| e.as_str())? + ).ok_or(Error::NotProxy)?; Self::do_vote(who, ref_index, vote) } @@ -470,11 +522,11 @@ decl_module! { /// referendum. #[weight = SimpleDispatchInfo::FixedOperational(500_000)] fn emergency_cancel(origin, ref_index: ReferendumIndex) { - T::CancellationOrigin::ensure_origin(origin)?; + T::CancellationOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; - let info = Self::referendum_info(ref_index).ok_or("unknown index")?; + let info = Self::referendum_info(ref_index).ok_or(Error::BadIndex)?; let h = info.proposal_hash; - ensure!(!>::exists(h), "cannot cancel the same proposal twice"); + ensure!(!>::exists(h), Error::AlreadyCanceled); >::insert(h, true); Self::clear_referendum(ref_index); @@ -484,10 +536,10 @@ decl_module! { /// referendum. #[weight = SimpleDispatchInfo::FixedNormal(5_000_000)] fn external_propose(origin, proposal_hash: T::Hash) { - T::ExternalOrigin::ensure_origin(origin)?; - ensure!(!>::exists(), "proposal already made"); + T::ExternalOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; + ensure!(!>::exists(), Error::DuplicateProposal); if let Some((until, _)) = >::get(proposal_hash) { - ensure!(>::block_number() >= until, "proposal still blacklisted"); + ensure!(>::block_number() >= until, Error::ProposalBlacklisted); } >::put((proposal_hash, VoteThreshold::SuperMajorityApprove)); } @@ -499,7 +551,7 @@ decl_module! { /// pre-scheduled `external_propose` call. #[weight = SimpleDispatchInfo::FixedNormal(5_000_000)] fn external_propose_majority(origin, proposal_hash: T::Hash) { - T::ExternalMajorityOrigin::ensure_origin(origin)?; + T::ExternalMajorityOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; >::put((proposal_hash, VoteThreshold::SimpleMajority)); } @@ -510,7 +562,7 @@ decl_module! { /// pre-scheduled `external_propose` call. #[weight = SimpleDispatchInfo::FixedNormal(5_000_000)] fn external_propose_default(origin, proposal_hash: T::Hash) { - T::ExternalDefaultOrigin::ensure_origin(origin)?; + T::ExternalDefaultOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; >::put((proposal_hash, VoteThreshold::SuperMajorityAgainst)); } @@ -529,13 +581,13 @@ decl_module! { voting_period: T::BlockNumber, delay: T::BlockNumber ) { - T::FastTrackOrigin::ensure_origin(origin)?; - let (e_proposal_hash, threshold) = >::get().ok_or("no proposal made")?; + T::FastTrackOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; + let (e_proposal_hash, threshold) = >::get().ok_or(Error::ProposalMissing)?; ensure!( threshold != VoteThreshold::SuperMajorityApprove, - "next external proposal not simple majority" + Error::NotSimpleMajority ); - ensure!(proposal_hash == e_proposal_hash, "invalid hash"); + ensure!(proposal_hash == e_proposal_hash, Error::InvalidHash); >::kill(); let now = >::block_number(); @@ -547,19 +599,19 @@ decl_module! { /// Veto and blacklist the external proposal hash. #[weight = SimpleDispatchInfo::FixedNormal(200_000)] fn veto_external(origin, proposal_hash: T::Hash) { - let who = T::VetoOrigin::ensure_origin(origin)?; + let who = T::VetoOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?; if let Some((e_proposal_hash, _)) = >::get() { - ensure!(proposal_hash == e_proposal_hash, "unknown proposal"); + ensure!(proposal_hash == e_proposal_hash, Error::ProposalMissing); } else { - Err("no external proposal")?; + Err(Error::NoProposal)?; } let mut existing_vetoers = >::get(&proposal_hash) .map(|pair| pair.1) .unwrap_or_else(Vec::new); let insert_position = existing_vetoers.binary_search(&who) - .err().ok_or("identity may not veto a proposal twice")?; + .err().ok_or(Error::AlreadyVetoed)?; existing_vetoers.insert(insert_position, who.clone()); let until = >::block_number() + T::CooloffPeriod::get(); @@ -572,24 +624,24 @@ decl_module! { /// Remove a referendum. #[weight = SimpleDispatchInfo::FixedOperational(10_000)] fn cancel_referendum(origin, #[compact] ref_index: ReferendumIndex) { - ensure_root(origin)?; + ensure_root(origin).map_err(|e| e.as_str())?; Self::clear_referendum(ref_index); } /// Cancel a proposal queued for enactment. #[weight = SimpleDispatchInfo::FixedOperational(10_000)] fn cancel_queued(origin, which: ReferendumIndex) { - ensure_root(origin)?; + ensure_root(origin).map_err(|e| e.as_str())?; let mut items = >::get(); let original_len = items.len(); items.retain(|i| i.2 != which); - ensure!(items.len() < original_len, "proposal not found"); + ensure!(items.len() < original_len, Error::ProposalMissing); >::put(items); } fn on_initialize(n: T::BlockNumber) { if let Err(e) = Self::begin_block(n) { - sp_runtime::print(e); + sp_runtime::print(e.as_str()); } } @@ -600,8 +652,8 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn set_proxy(origin, proxy: T::AccountId) { - let who = ensure_signed(origin)?; - ensure!(!>::exists(&proxy), "already a proxy"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(!>::exists(&proxy), Error::AlreadyProxy); >::insert(proxy, who) } @@ -612,7 +664,7 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn resign_proxy(origin) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; >::remove(who); } @@ -623,8 +675,8 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn remove_proxy(origin, proxy: T::AccountId) { - let who = ensure_signed(origin)?; - ensure!(&Self::proxy(&proxy).ok_or("not a proxy")? == &who, "wrong proxy"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(&Self::proxy(&proxy).ok_or(Error::NotProxy)? == &who, Error::WrongProxy); >::remove(proxy); } @@ -635,7 +687,7 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(500_000)] pub fn delegate(origin, to: T::AccountId, conviction: Conviction) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; >::insert(&who, (&to, conviction)); // Currency is locked indefinitely as long as it's delegated. T::Currency::extend_lock( @@ -655,8 +707,8 @@ decl_module! { /// # #[weight = SimpleDispatchInfo::FixedNormal(500_000)] fn undelegate(origin) { - let who = ensure_signed(origin)?; - ensure!(>::exists(&who), "not delegated"); + let who = ensure_signed(origin).map_err(|e| e.as_str())?; + ensure!(>::exists(&who), Error::NotDelegated); let (_, conviction) = >::take(&who); // Indefinite lock is reduced to the maximum voting lock that could be possible. let now = >::block_number(); @@ -674,7 +726,7 @@ decl_module! { /// Veto and blacklist the proposal hash. Must be from Root origin. #[weight = SimpleDispatchInfo::FixedNormal(10_000)] fn clear_public_proposals(origin) { - ensure_root(origin)?; + ensure_root(origin).map_err(|e| e.as_str())?; >::kill(); } @@ -683,9 +735,9 @@ decl_module! { /// in the dispatch queue but does require a deposit, returned once enacted. #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn note_preimage(origin, encoded_proposal: Vec) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); - ensure!(!>::exists(&proposal_hash), "preimage already noted"); + ensure!(!>::exists(&proposal_hash), Error::DuplicatePreimage); let deposit = >::from(encoded_proposal.len() as u32) .saturating_mul(T::PreimageByteDeposit::get()); @@ -701,11 +753,11 @@ decl_module! { /// in the dispatch queue. No deposit is needed. #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn note_imminent_preimage(origin, encoded_proposal: Vec) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); - ensure!(!>::exists(&proposal_hash), "preimage already noted"); + ensure!(!>::exists(&proposal_hash), Error::DuplicatePreimage); let queue = >::get(); - ensure!(queue.iter().any(|item| &item.1 == &proposal_hash), "not imminent"); + ensure!(queue.iter().any(|item| &item.1 == &proposal_hash), Error::NotImminent); let now = >::block_number(); let free = >::zero(); @@ -721,16 +773,16 @@ decl_module! { /// work an additional `EnactmentPeriod` later. #[weight = SimpleDispatchInfo::FixedNormal(10_000)] fn reap_preimage(origin, proposal_hash: T::Hash) { - let who = ensure_signed(origin)?; + let who = ensure_signed(origin).map_err(|e| e.as_str())?; - let (_, old, deposit, then) = >::get(&proposal_hash).ok_or("not found")?; + let (_, old, deposit, then) = >::get(&proposal_hash).ok_or(Error::PreimageMissing)?; let now = >::block_number(); let (voting, enactment) = (T::VotingPeriod::get(), T::EnactmentPeriod::get()); let additional = if who == old { Zero::zero() } else { enactment }; - ensure!(now >= then + voting + additional, "too early"); + ensure!(now >= then + voting + additional, Error::Early); let queue = >::get(); - ensure!(!queue.iter().any(|item| &item.1 == &proposal_hash), "imminent"); + ensure!(!queue.iter().any(|item| &item.1 == &proposal_hash), Error::Imminent); let _ = T::Currency::repatriate_reserved(&old, &who, deposit); >::remove(&proposal_hash); @@ -879,8 +931,8 @@ impl Module { // private. /// Actually enact a vote, if legit. - fn do_vote(who: T::AccountId, ref_index: ReferendumIndex, vote: Vote) -> dispatch::Result { - ensure!(Self::is_active_referendum(ref_index), "vote given for invalid referendum."); + fn do_vote(who: T::AccountId, ref_index: ReferendumIndex, vote: Vote) -> Result<(), Error> { + ensure!(Self::is_active_referendum(ref_index), Error::ReferendumInvalid); if !>::exists((ref_index, &who)) { >::append_or_insert(ref_index, &[&who][..]); } @@ -921,7 +973,7 @@ impl Module { } /// Enact a proposal from a referendum. - fn enact_proposal(proposal_hash: T::Hash, index: ReferendumIndex) -> dispatch::Result { + fn enact_proposal(proposal_hash: T::Hash, index: ReferendumIndex) -> Result<(), Error> { if let Some((encoded_proposal, who, amount, _)) = >::take(&proposal_hash) { if let Ok(proposal) = T::Proposal::decode(&mut &encoded_proposal[..]) { let _ = T::Currency::unreserve(&who, amount); @@ -934,25 +986,25 @@ impl Module { } else { T::Slash::on_unbalanced(T::Currency::slash_reserved(&who, amount).0); Self::deposit_event(RawEvent::PreimageInvalid(proposal_hash, index)); - Err("invalid preimage") + Err(Error::PreimageInvalid) } } else { Self::deposit_event(RawEvent::PreimageMissing(proposal_hash, index)); - Err("missing preimage") + Err(Error::PreimageMissing) } } /// Table the next waiting proposal for a vote. - fn launch_next(now: T::BlockNumber) -> dispatch::Result { + fn launch_next(now: T::BlockNumber) -> Result<(), Error> { if LastTabledWasExternal::take() { Self::launch_public(now).or_else(|_| Self::launch_external(now)) } else { Self::launch_external(now).or_else(|_| Self::launch_public(now)) - }.map_err(|_| "No proposals waiting") + }.map_err(|_| Error::NoneWaiting) } /// Table the waiting external proposal for a vote, if there is one. - fn launch_external(now: T::BlockNumber) -> dispatch::Result { + fn launch_external(now: T::BlockNumber) -> Result<(), Error> { if let Some((proposal, threshold)) = >::take() { LastTabledWasExternal::put(true); Self::deposit_event(RawEvent::ExternalTabled); @@ -964,12 +1016,12 @@ impl Module { ); Ok(()) } else { - Err("No external proposal waiting") + Err(Error::NoneWaiting) } } /// Table the waiting public proposal with the highest backing for a vote. - fn launch_public(now: T::BlockNumber) -> dispatch::Result { + fn launch_public(now: T::BlockNumber) -> Result<(), Error> { let mut public_props = Self::public_props(); if let Some((winner_index, _)) = public_props.iter() .enumerate() @@ -994,7 +1046,7 @@ impl Module { } Ok(()) } else { - Err("No public proposals waiting") + Err(Error::NoneWaiting) } } @@ -1003,7 +1055,7 @@ impl Module { now: T::BlockNumber, index: ReferendumIndex, info: ReferendumInfo - ) -> dispatch::Result { + ) -> Result<(), Error> { let (approve, against, capital) = Self::tally(index); let total_issuance = T::Currency::total_issuance(); let approved = info.threshold.approved(approve, against, capital, total_issuance); @@ -1051,7 +1103,7 @@ impl Module { } /// Current era is ending; we should finish up any proposals. - fn begin_block(now: T::BlockNumber) -> dispatch::Result { + fn begin_block(now: T::BlockNumber) -> Result<(), Error> { // pick out another public referendum if it's time. if (now % T::LaunchPeriod::get()).is_zero() { // Errors come from the queue being empty. we don't really care about that, and even if @@ -1237,13 +1289,13 @@ mod tests { let p = set_balance_proposal(value); let h = BlakeTwo256::hash(&p[..]); match Democracy::note_preimage(Origin::signed(6), p) { - Ok(_) | Err("preimage already noted") => (), + Ok(_) | Err(Error::DuplicatePreimage) => (), Err(x) => panic!(x), } h } - fn propose_set_balance(who: u64, value: u64, delay: u64) -> dispatch::Result { + fn propose_set_balance(who: u64, value: u64, delay: u64) -> Result<(), Error> { Democracy::propose( Origin::signed(who), set_balance_proposal_hash(value), @@ -1251,7 +1303,7 @@ mod tests { ) } - fn propose_set_balance_and_note(who: u64, value: u64, delay: u64) -> dispatch::Result { + fn propose_set_balance_and_note(who: u64, value: u64, delay: u64) -> Result<(), Error> { Democracy::propose( Origin::signed(who), set_balance_proposal_hash_and_note(value), @@ -1297,7 +1349,7 @@ mod tests { PREIMAGE_BYTE_DEPOSIT.with(|v| *v.borrow_mut() = 100); assert_noop!( Democracy::note_preimage(Origin::signed(6), vec![0; 500]), - "not enough free funds" + Error::Other("not enough free funds") ); // fee of 1 is reasonable. PREIMAGE_BYTE_DEPOSIT.with(|v| *v.borrow_mut() = 1); @@ -1332,7 +1384,7 @@ mod tests { next_block(); assert_noop!( Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2)), - "too early" + Error::Early ); next_block(); assert_ok!(Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2))); @@ -1348,7 +1400,7 @@ mod tests { System::set_block_number(1); assert_noop!( Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2)), - "not found" + Error::PreimageMissing ); PREIMAGE_BYTE_DEPOSIT.with(|v| *v.borrow_mut() = 1); @@ -1360,7 +1412,7 @@ mod tests { next_block(); assert_noop!( Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2)), - "too early" + Error::Early ); next_block(); @@ -1387,7 +1439,7 @@ mod tests { assert_noop!( Democracy::note_imminent_preimage(Origin::signed(7), set_balance_proposal(2)), - "not imminent" + Error::NotImminent ); next_block(); @@ -1411,7 +1463,7 @@ mod tests { next_block(); next_block(); // now imminent. - assert_noop!(Democracy::reap_preimage(Origin::signed(6), h), "imminent"); + assert_noop!(Democracy::reap_preimage(Origin::signed(6), h), Error::Imminent); }); } @@ -1540,7 +1592,7 @@ mod tests { ); assert!(Democracy::referendum_info(r).is_some()); - assert_noop!(Democracy::emergency_cancel(Origin::signed(3), r), "Invalid origin"); + assert_noop!(Democracy::emergency_cancel(Origin::signed(3), r), "Invalid origin".into()); assert_ok!(Democracy::emergency_cancel(Origin::signed(4), r)); assert!(Democracy::referendum_info(r).is_none()); @@ -1553,7 +1605,7 @@ mod tests { 2 ); assert!(Democracy::referendum_info(r).is_some()); - assert_noop!(Democracy::emergency_cancel(Origin::signed(4), r), "cannot cancel the same proposal twice"); + assert_noop!(Democracy::emergency_cancel(Origin::signed(4), r), Error::AlreadyCanceled); }); } @@ -1575,14 +1627,14 @@ mod tests { assert_noop!(Democracy::external_propose( Origin::signed(2), set_balance_proposal_hash(2), - ), "proposal still blacklisted"); + ), Error::ProposalBlacklisted); fast_forward_to(1); // fails as we're still in cooloff period. assert_noop!(Democracy::external_propose( Origin::signed(2), set_balance_proposal_hash(2), - ), "proposal still blacklisted"); + ), Error::ProposalBlacklisted); fast_forward_to(2); // works; as we're out of the cooloff period. @@ -1595,7 +1647,7 @@ mod tests { // 3 can't veto the same thing twice. assert_noop!( Democracy::veto_external(Origin::signed(3), h.clone()), - "identity may not veto a proposal twice" + Error::AlreadyVetoed ); // 4 vetoes. @@ -1608,7 +1660,7 @@ mod tests { assert_noop!(Democracy::external_propose( Origin::signed(2), set_balance_proposal_hash(2), - ), "proposal still blacklisted"); + ), Error::ProposalBlacklisted); // different proposal works fine. assert_ok!(Democracy::external_propose( Origin::signed(2), @@ -1624,7 +1676,7 @@ mod tests { assert_noop!(Democracy::external_propose( Origin::signed(1), set_balance_proposal_hash(2), - ), "Invalid origin"); + ), "Invalid origin".into()); assert_ok!(Democracy::external_propose( Origin::signed(2), set_balance_proposal_hash_and_note(2), @@ -1632,7 +1684,7 @@ mod tests { assert_noop!(Democracy::external_propose( Origin::signed(2), set_balance_proposal_hash(1), - ), "proposal already made"); + ), Error::DuplicateProposal); fast_forward_to(2); assert_eq!( Democracy::referendum_info(0), @@ -1653,7 +1705,7 @@ mod tests { assert_noop!(Democracy::external_propose_majority( Origin::signed(1), set_balance_proposal_hash(2) - ), "Invalid origin"); + ), "Invalid origin".into()); assert_ok!(Democracy::external_propose_majority( Origin::signed(3), set_balance_proposal_hash_and_note(2) @@ -1678,7 +1730,7 @@ mod tests { assert_noop!(Democracy::external_propose_default( Origin::signed(3), set_balance_proposal_hash(2) - ), "Invalid origin"); + ), "Invalid origin".into()); assert_ok!(Democracy::external_propose_default( Origin::signed(1), set_balance_proposal_hash_and_note(2) @@ -1701,12 +1753,12 @@ mod tests { new_test_ext().execute_with(|| { System::set_block_number(0); let h = set_balance_proposal_hash_and_note(2); - assert_noop!(Democracy::fast_track(Origin::signed(5), h, 3, 2), "no proposal made"); + assert_noop!(Democracy::fast_track(Origin::signed(5), h, 3, 2), Error::ProposalMissing); assert_ok!(Democracy::external_propose_majority( Origin::signed(3), set_balance_proposal_hash_and_note(2) )); - assert_noop!(Democracy::fast_track(Origin::signed(1), h, 3, 2), "Invalid origin"); + assert_noop!(Democracy::fast_track(Origin::signed(1), h, 3, 2), "Invalid origin".into()); assert_ok!(Democracy::fast_track(Origin::signed(5), h, 0, 0)); assert_eq!( Democracy::referendum_info(0), @@ -1731,7 +1783,7 @@ mod tests { )); assert_noop!( Democracy::fast_track(Origin::signed(5), h, 3, 2), - "next external proposal not simple majority" + Error::NotSimpleMajority ); }); } @@ -1813,7 +1865,7 @@ mod tests { (6, set_balance_proposal_hash_and_note(2), 0) ]); - assert_noop!(Democracy::cancel_queued(Origin::ROOT, 1), "proposal not found"); + assert_noop!(Democracy::cancel_queued(Origin::ROOT, 1), Error::ProposalMissing); assert_ok!(Democracy::cancel_queued(Origin::ROOT, 0)); assert_eq!(Democracy::dispatch_queue(), vec![]); }); @@ -1827,7 +1879,7 @@ mod tests { assert_eq!(Democracy::proxy(10), Some(1)); // Can't set when already set. - assert_noop!(Democracy::set_proxy(Origin::signed(2), 10), "already a proxy"); + assert_noop!(Democracy::set_proxy(Origin::signed(2), 10), Error::AlreadyProxy); // But this works because 11 isn't proxying. assert_ok!(Democracy::set_proxy(Origin::signed(2), 11)); @@ -1835,7 +1887,7 @@ mod tests { assert_eq!(Democracy::proxy(11), Some(2)); // 2 cannot fire 1's proxy: - assert_noop!(Democracy::remove_proxy(Origin::signed(2), 10), "wrong proxy"); + assert_noop!(Democracy::remove_proxy(Origin::signed(2), 10), Error::WrongProxy); // 1 fires his proxy: assert_ok!(Democracy::remove_proxy(Origin::signed(1), 10)); @@ -2042,7 +2094,7 @@ mod tests { fn proposal_with_deposit_below_minimum_should_not_work() { new_test_ext().execute_with(|| { System::set_block_number(1); - assert_noop!(propose_set_balance(1, 2, 0), "value too low"); + assert_noop!(propose_set_balance(1, 2, 0), Error::ValueLow); }); } @@ -2050,7 +2102,7 @@ mod tests { fn poor_proposer_should_not_work() { new_test_ext().execute_with(|| { System::set_block_number(1); - assert_noop!(propose_set_balance(1, 2, 11), "proposer\'s balance too low"); + assert_noop!(propose_set_balance(1, 2, 11), Error::Other("not enough free funds")); }); } @@ -2059,7 +2111,7 @@ mod tests { new_test_ext().execute_with(|| { System::set_block_number(1); assert_ok!(propose_set_balance_and_note(2, 2, 11)); - assert_noop!(Democracy::second(Origin::signed(1), 0), "seconder\'s balance too low"); + assert_noop!(Democracy::second(Origin::signed(1), 0), Error::Other("not enough free funds")); }); } diff --git a/substrate/frame/evm/src/lib.rs b/substrate/frame/evm/src/lib.rs index 6215dfaab2..cf973cdd5a 100644 --- a/substrate/frame/evm/src/lib.rs +++ b/substrate/frame/evm/src/lib.rs @@ -24,13 +24,13 @@ mod backend; pub use crate::backend::{Account, Log, Vicinity, Backend}; use sp_std::{vec::Vec, marker::PhantomData}; -use frame_support::{dispatch, decl_module, decl_storage, decl_event}; +use frame_support::{decl_module, decl_storage, decl_event, decl_error}; use frame_support::weights::{Weight, WeighData, ClassifyDispatch, DispatchClass, PaysFee}; use frame_support::traits::{Currency, WithdrawReason, ExistenceRequirement}; use frame_system::{self as system, ensure_signed}; use sp_runtime::ModuleId; use frame_support::weights::SimpleDispatchInfo; -use sp_runtime::traits::{UniqueSaturatedInto, AccountIdConversion, SaturatedConversion}; +use sp_runtime::traits::{UniqueSaturatedInto, AccountIdConversion, SaturatedConversion, ModuleDispatchError}; use sp_core::{U256, H256, H160}; use evm::{ExitReason, ExitSucceed, ExitError}; use evm::executor::StackExecutor; @@ -137,21 +137,42 @@ decl_storage! { } } -decl_event!( +decl_event! { /// EVM events pub enum Event { /// Ethereum events from contracts. Log(Log), } -); +} + +decl_error! { + pub enum Error { + /// Not enough balance to perform action + BalanceLow, + /// Calculating total fee overflowed + FeeOverflow, + /// Calculating total payment overflowed + PaymentOverflow, + /// Withdraw fee failed + WithdrawFailed, + /// Call failed + ExitReasonFailed, + /// Call reverted + ExitReasonRevert, + /// Call returned VM fatal error + ExitReasonFatal, + } +} decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; #[weight = SimpleDispatchInfo::FixedNormal(10_000)] - fn deposit_balance(origin, value: BalanceOf) -> dispatch::Result { - let sender = ensure_signed(origin)?; + fn deposit_balance(origin, value: BalanceOf) { + let sender = ensure_signed(origin).map_err(|e| e.as_str())?; let imbalance = T::Currency::withdraw( &sender, @@ -166,19 +187,17 @@ decl_module! { Accounts::mutate(&address, |account| { account.balance += bvalue; }); - - Ok(()) } #[weight = SimpleDispatchInfo::FixedNormal(10_000)] - fn withdraw_balance(origin, value: BalanceOf) -> dispatch::Result { - let sender = ensure_signed(origin)?; + fn withdraw_balance(origin, value: BalanceOf) { + let sender = ensure_signed(origin).map_err(|e| e.as_str())?; let address = T::ConvertAccountId::convert_account_id(&sender); let bvalue = U256::from(UniqueSaturatedInto::::unique_saturated_into(value)); let mut account = Accounts::get(&address); account.balance = account.balance.checked_sub(bvalue) - .ok_or("Not enough balance to withdraw")?; + .ok_or(Error::BalanceLow)?; let imbalance = T::Currency::withdraw( &Self::account_id(), @@ -190,15 +209,11 @@ decl_module! { Accounts::insert(&address, account); T::Currency::resolve_creating(&sender, imbalance); - - Ok(()) } #[weight = WeightForCallCreate::::default()] - fn call(origin, target: H160, input: Vec, value: U256, gas_limit: u32) - -> dispatch::Result - { - let sender = ensure_signed(origin)?; + fn call(origin, target: H160, input: Vec, value: U256, gas_limit: u32) { + let sender = ensure_signed(origin).map_err(|e| e.as_str())?; let source = T::ConvertAccountId::convert_account_id(&sender); let gas_price = T::FeeCalculator::gas_price(); @@ -216,13 +231,13 @@ decl_module! { ); let total_fee = gas_price.checked_mul(U256::from(gas_limit)) - .ok_or("Calculating total fee overflowed")?; + .ok_or(Error::FeeOverflow)?; if Accounts::get(&source).balance < - value.checked_add(total_fee).ok_or("Calculating total payment overflowed")? + value.checked_add(total_fee).ok_or(Error::PaymentOverflow)? { - return Err("Not enough balance to pay transaction fee") + return Err(Error::BalanceLow) } - executor.withdraw(source, total_fee).map_err(|_| "Withdraw fee failed")?; + executor.withdraw(source, total_fee).map_err(|_| Error::WithdrawFailed)?; let reason = executor.transact_call( source, @@ -234,9 +249,9 @@ decl_module! { let ret = match reason { ExitReason::Succeed(_) => Ok(()), - ExitReason::Error(_) => Err("Execute message call failed"), - ExitReason::Revert(_) => Err("Execute message call reverted"), - ExitReason::Fatal(_) => Err("Execute message call returned VM fatal error"), + ExitReason::Error(_) => Err(Error::ExitReasonFailed), + ExitReason::Revert(_) => Err(Error::ExitReasonRevert), + ExitReason::Fatal(_) => Err(Error::ExitReasonFatal), }; let actual_fee = executor.fee(gas_price); executor.deposit(source, total_fee.saturating_sub(actual_fee)); @@ -244,12 +259,12 @@ decl_module! { let (values, logs) = executor.deconstruct(); backend.apply(values, logs, true); - ret + return ret; } #[weight = WeightForCallCreate::::default()] - fn create(origin, init: Vec, value: U256, gas_limit: u32) -> dispatch::Result { - let sender = ensure_signed(origin)?; + fn create(origin, init: Vec, value: U256, gas_limit: u32) { + let sender = ensure_signed(origin).map_err(|e| e.as_str())?; let source = T::ConvertAccountId::convert_account_id(&sender); let gas_price = T::FeeCalculator::gas_price(); @@ -267,13 +282,13 @@ decl_module! { ); let total_fee = gas_price.checked_mul(U256::from(gas_limit)) - .ok_or("Calculating total fee overflowed")?; + .ok_or(Error::FeeOverflow)?; if Accounts::get(&source).balance < - value.checked_add(total_fee).ok_or("Calculating total payment overflowed")? + value.checked_add(total_fee).ok_or(Error::PaymentOverflow)? { - return Err("Not enough balance to pay transaction fee") + return Err(Error::BalanceLow) } - executor.withdraw(source, total_fee).map_err(|_| "Withdraw fee failed")?; + executor.withdraw(source, total_fee).map_err(|_| Error::WithdrawFailed)?; let reason = executor.transact_create( source, @@ -284,9 +299,9 @@ decl_module! { let ret = match reason { ExitReason::Succeed(_) => Ok(()), - ExitReason::Error(_) => Err("Execute contract creation failed"), - ExitReason::Revert(_) => Err("Execute contract creation reverted"), - ExitReason::Fatal(_) => Err("Execute contract creation returned VM fatal error"), + ExitReason::Error(_) => Err(Error::ExitReasonFailed), + ExitReason::Revert(_) => Err(Error::ExitReasonRevert), + ExitReason::Fatal(_) => Err(Error::ExitReasonFatal), }; let actual_fee = executor.fee(gas_price); executor.deposit(source, total_fee.saturating_sub(actual_fee)); @@ -294,7 +309,7 @@ decl_module! { let (values, logs) = executor.deconstruct(); backend.apply(values, logs, true); - ret + return ret; } } } diff --git a/substrate/frame/finality-tracker/src/lib.rs b/substrate/frame/finality-tracker/src/lib.rs index 4837a9cf78..7830a2b8f5 100644 --- a/substrate/frame/finality-tracker/src/lib.rs +++ b/substrate/frame/finality-tracker/src/lib.rs @@ -19,9 +19,9 @@ #![cfg_attr(not(feature = "std"), no_std)] use sp_inherents::{InherentIdentifier, ProvideInherent, InherentData, MakeFatalError}; -use sp_runtime::traits::{One, Zero, SaturatedConversion}; +use sp_runtime::traits::{One, Zero, SaturatedConversion, ModuleDispatchError}; use sp_std::{prelude::*, result, cmp, vec}; -use frame_support::{decl_module, decl_storage}; +use frame_support::{decl_module, decl_storage, decl_error, ensure}; use frame_support::traits::Get; use frame_system::{ensure_none, Trait as SystemTrait}; use sp_finality_tracker::{INHERENT_IDENTIFIER, FinalizedInherentData}; @@ -56,8 +56,18 @@ decl_storage! { } } +decl_error! { + pub enum Error { + /// Final hint must be updated only once in the block + AlreadyUpdated, + /// Finalized height above block number + BadHint, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; /// The number of recent samples to keep from this chain. Default is 101. const WindowSize: T::BlockNumber = T::WindowSize::get(); @@ -67,11 +77,11 @@ decl_module! { /// Hint that the author of this block thinks the best finalized /// block is the given number. fn final_hint(origin, #[compact] hint: T::BlockNumber) { - ensure_none(origin)?; - assert!(!::Update::exists(), "Final hint must be updated only once in the block"); - assert!( + ensure_none(origin).map_err(|e| e.as_str())?; + ensure!(!::Update::exists(), Error::AlreadyUpdated); + ensure!( frame_system::Module::::block_number() >= hint, - "Finalized height above block number", + Error::BadHint, ); ::Update::put(hint); } diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index 8c017acc91..eae43affde 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -31,10 +31,12 @@ pub use sp_finality_grandpa as fg_primitives; use sp_std::prelude::*; -use codec::{self as codec, Encode, Decode, Error}; -use frame_support::{decl_event, decl_storage, decl_module, dispatch, storage}; +use codec::{self as codec, Encode, Decode}; +use frame_support::{decl_event, decl_storage, decl_module, decl_error, storage}; use sp_runtime::{ - generic::{DigestItem, OpaqueDigestItemId}, traits::Zero, Perbill, + generic::{DigestItem, OpaqueDigestItemId}, + traits::{Zero, ModuleDispatchError}, + Perbill, }; use sp_staking::{ SessionIndex, @@ -82,7 +84,7 @@ pub struct StoredPendingChange { } impl Decode for StoredPendingChange { - fn decode(value: &mut I) -> core::result::Result { + fn decode(value: &mut I) -> core::result::Result { let old = OldStoredPendingChange::decode(value)?; let forced = >::decode(value).unwrap_or(None); @@ -123,7 +125,7 @@ pub enum StoredState { }, } -decl_event!( +decl_event! { pub enum Event { /// New authority set has been applied. NewAuthorities(AuthorityList), @@ -132,7 +134,22 @@ decl_event!( /// Current authority set has been resumed. Resumed, } -); +} + +decl_error! { + pub enum Error { + /// Attempt to signal GRANDPA pause when the authority set isn't live + /// (either paused or already pending pause). + PauseFailed, + /// Attempt to signal GRANDPA resume when the authority set isn't paused + /// (either live or already pending resume). + ResumeFailed, + /// Attempt to signal GRANDPA change with one already pending. + ChangePending, + /// Cannot signal forced change so soon after last. + TooSoon, + } +} decl_storage! { trait Store for Module as GrandpaFinality { @@ -170,11 +187,13 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; /// Report some misbehavior. fn report_misbehavior(origin, _report: Vec) { - ensure_signed(origin)?; + ensure_signed(origin).map_err(|e| e.as_str())?; // FIXME: https://github.com/paritytech/substrate/issues/1112 } @@ -264,7 +283,7 @@ impl Module { /// Schedule GRANDPA to pause starting in the given number of blocks. /// Cannot be done when already paused. - pub fn schedule_pause(in_blocks: T::BlockNumber) -> dispatch::Result { + pub fn schedule_pause(in_blocks: T::BlockNumber) -> Result<(), Error> { if let StoredState::Live = >::get() { let scheduled_at = >::block_number(); >::put(StoredState::PendingPause { @@ -274,13 +293,12 @@ impl Module { Ok(()) } else { - Err("Attempt to signal GRANDPA pause when the authority set isn't live \ - (either paused or already pending pause).") + Err(Error::PauseFailed) } } /// Schedule a resume of GRANDPA after pausing. - pub fn schedule_resume(in_blocks: T::BlockNumber) -> dispatch::Result { + pub fn schedule_resume(in_blocks: T::BlockNumber) -> Result<(), Error> { if let StoredState::Paused = >::get() { let scheduled_at = >::block_number(); >::put(StoredState::PendingResume { @@ -290,8 +308,7 @@ impl Module { Ok(()) } else { - Err("Attempt to signal GRANDPA resume when the authority set isn't paused \ - (either live or already pending resume).") + Err(Error::ResumeFailed) } } @@ -313,13 +330,13 @@ impl Module { next_authorities: AuthorityList, in_blocks: T::BlockNumber, forced: Option, - ) -> dispatch::Result { + ) -> Result<(), Error> { if !>::exists() { let scheduled_at = >::block_number(); if let Some(_) = forced { if Self::next_forced().map_or(false, |next| next > scheduled_at) { - return Err("Cannot signal forced change so soon after last."); + return Err(Error::TooSoon); } // only allow the next forced change when twice the window has passed since @@ -336,7 +353,7 @@ impl Module { Ok(()) } else { - Err("Attempt to signal GRANDPA change with one already pending.") + Err(Error::ChangePending) } }