From 9d9368de6ce93098ac70cb16c011baeb10e6c490 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 23 Jan 2020 21:18:31 +0100 Subject: [PATCH 1/2] use decl_error in common runtime modules --- runtime/common/src/attestations.rs | 15 ++- runtime/common/src/claims.rs | 36 +++++-- runtime/common/src/crowdfund.rs | 160 ++++++++++++++++++++--------- runtime/common/src/parachains.rs | 131 ++++++++++++++--------- runtime/common/src/registrar.rs | 35 +++++-- runtime/common/src/slots.rs | 59 ++++++++--- 6 files changed, 300 insertions(+), 136 deletions(-) diff --git a/runtime/common/src/attestations.rs b/runtime/common/src/attestations.rs index fb2277649e13..aea9b89a3d24 100644 --- a/runtime/common/src/attestations.rs +++ b/runtime/common/src/attestations.rs @@ -21,7 +21,9 @@ use rstd::prelude::*; use codec::{Encode, Decode}; -use frame_support::{decl_storage, decl_module, ensure, dispatch::DispatchResult, traits::Get}; +use frame_support::{ + decl_storage, decl_module, decl_error, ensure, dispatch::DispatchResult, traits::Get +}; use primitives::{Hash, parachain::{AttestedCandidate, CandidateReceipt, Id as ParaId}}; use sp_runtime::RuntimeDebug; @@ -106,13 +108,22 @@ decl_storage! { } } +decl_error!{ + pub enum Error for Module{ + /// More attestations can be added only once in a block. + TooManyAttestations, + } +} + decl_module! { /// Parachain-attestations module. pub struct Module for enum Call where origin: ::Origin { + type Error = Error; + /// Provide candidate receipts for parachains, in ascending order by id. fn more_attestations(origin, _more: MoreAttestations) -> DispatchResult { ensure_none(origin)?; - ensure!(!::exists(), "More attestations can be added only once in a block."); + ensure!(!::exists(), Error::::TooManyAttestations); ::put(true); Ok(()) diff --git a/runtime/common/src/claims.rs b/runtime/common/src/claims.rs index a2bef829fa9d..fb8dedf19a50 100644 --- a/runtime/common/src/claims.rs +++ b/runtime/common/src/claims.rs @@ -18,7 +18,7 @@ use rstd::prelude::*; use sp_io::{hashing::keccak_256, crypto::secp256k1_ecdsa_recover}; -use frame_support::{decl_event, decl_storage, decl_module}; +use frame_support::{decl_event, decl_storage, decl_module, decl_error}; use frame_support::weights::SimpleDispatchInfo; use frame_support::traits::{Currency, Get, VestingCurrency}; use system::{ensure_root, ensure_none}; @@ -102,6 +102,15 @@ decl_event!( } ); +decl_error!{ + pub enum Error for Module{ + /// Invalid Ethereum signature. + InvalidEthereumSignature, + /// Ethereum address has no claim. + SignerHasNoClaim, + } +} + decl_storage! { // A macro for the Storage trait, and its implementation, for this module. // This allows for type-safe usage of the Substrate storage database, so you can @@ -126,6 +135,8 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + /// The Prefix that is used in signed Ethereum messages for this network const Prefix: &[u8] = T::Prefix::get(); @@ -139,10 +150,10 @@ decl_module! { let data = dest.using_encoded(to_ascii_hex); let signer = Self::eth_recover(ðereum_signature, &data) - .ok_or("Invalid Ethereum signature")?; + .ok_or(Error::::InvalidEthereumSignature)?; let balance_due = >::get(&signer) - .ok_or("Ethereum address has no claim")?; + .ok_or(Error::::SignerHasNoClaim)?; // Check if this claim should have a vesting schedule. if let Some(vs) = >::get(&signer) { @@ -415,7 +426,7 @@ mod tests { assert_eq!(Balances::free_balance(&42), 0); assert_noop!( Claims::claim(Origin::NONE, 69, sig(&bob(), &69u64.encode())), - "Ethereum address has no claim" + Error::::SignerHasNoClaim ); assert_ok!(Claims::mint_claim(Origin::ROOT, eth(&bob()), 200, None)); assert_ok!(Claims::claim(Origin::NONE, 69, sig(&bob(), &69u64.encode()))); @@ -434,7 +445,7 @@ mod tests { assert_eq!(Balances::free_balance(&42), 0); assert_noop!( Claims::claim(Origin::NONE, 69, sig(&bob(), &69u64.encode())), - "Ethereum address has no claim" + Error::::SignerHasNoClaim ); assert_ok!(Claims::mint_claim(Origin::ROOT, eth(&bob()), 200, Some((50, 10, 1)))); assert_ok!(Claims::claim(Origin::NONE, 69, sig(&bob(), &69u64.encode()))); @@ -459,7 +470,10 @@ mod tests { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(&42), 0); assert_ok!(Claims::claim(Origin::NONE, 42, sig(&alice(), &42u64.encode()))); - assert_noop!(Claims::claim(Origin::NONE, 42, sig(&alice(), &42u64.encode())), "Ethereum address has no claim"); + assert_noop!( + Claims::claim(Origin::NONE, 42, sig(&alice(), &42u64.encode())), + Error::::SignerHasNoClaim + ); }); } @@ -467,7 +481,10 @@ mod tests { fn non_sender_sig_doesnt_work() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(&42), 0); - assert_noop!(Claims::claim(Origin::NONE, 42, sig(&alice(), &69u64.encode())), "Ethereum address has no claim"); + assert_noop!( + Claims::claim(Origin::NONE, 42, sig(&alice(), &69u64.encode())), + Error::::SignerHasNoClaim + ); }); } @@ -475,7 +492,10 @@ mod tests { fn non_claimant_doesnt_work() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(&42), 0); - assert_noop!(Claims::claim(Origin::NONE, 42, sig(&bob(), &69u64.encode())), "Ethereum address has no claim"); + assert_noop!( + Claims::claim(Origin::NONE, 42, sig(&bob(), &69u64.encode())), + Error::::SignerHasNoClaim + ); }); } diff --git a/runtime/common/src/crowdfund.rs b/runtime/common/src/crowdfund.rs index 8898f45c64a5..f47cfad1ba6f 100644 --- a/runtime/common/src/crowdfund.rs +++ b/runtime/common/src/crowdfund.rs @@ -67,7 +67,7 @@ //! funds ultimately end up in module's fund sub-account. use frame_support::{ - decl_module, decl_storage, decl_event, storage::child, ensure, traits::{ + decl_module, decl_storage, decl_event, decl_error, storage::child, ensure, traits::{ Currency, Get, OnUnbalanced, WithdrawReason, ExistenceRequirement::AllowDeath } }; @@ -187,8 +187,56 @@ decl_event! { } } +decl_error!{ + pub enum Error for Module{ + /// Last slot must be greater than first slot. + LastSlotBeforeFirstSlot, + /// The last slot cannot be more then 3 slots after the first slot. + LastSlotTooFarInFuture, + /// The campaign ends before the current block number. The end must be in the future. + CannotEndInPast, + /// There was an overflow. + Overflow, + /// The contribution was below the minimum, `MinContribution`. + ContributionTooSmall, + /// Invalid fund index. + InvalidFundIndex, + /// Contributions exceed maximum amount. + CapExceeded, + /// The contribution period has already ended. + ContributionPeriodOver, + /// The origin of this call is invalid. + InvalidOrigin, + /// Deployment data for a fund can only be set once. The deployment data for this fund + /// already exists. + ExistingDeployData, + /// Deployment data has not been set for this fund. + UnsetDeployData, + /// This fund has already been onboarded. + AlreadyOnboard, + /// This crowdfund does not correspond to a parachain. + NotParachain, + /// This parachain still has its deposit. Implies that it has already been offboarded. + ParaHasDeposit, + /// Funds have not yet been returned. + FundsNotReturned, + /// Fund has not yet retired. + FundNotRetired, + /// The crowdfund has not yet ended. + FundNotEnded, + /// There are no contributions stored in this crowdfund. + NoContributions, + /// This crowdfund has an active parachain and cannot be dissolved. + HasActiveParachain, + /// The retirement period has not ended. + InRetirementPeriod, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; /// Create a new crowdfunding campaign for a parachain slot deposit for the current auction. @@ -201,16 +249,16 @@ decl_module! { ) { let owner = ensure_signed(origin)?; - ensure!(first_slot < last_slot, "last slot must be greater than first slot"); - ensure!(last_slot <= first_slot + 3.into(), "last slot cannot be more then 3 more than first slot"); - ensure!(end > >::block_number(), "end must be in the future"); + ensure!(first_slot < last_slot, Error::::LastSlotBeforeFirstSlot); + ensure!(last_slot <= first_slot + 3.into(), Error::::LastSlotTooFarInFuture); + ensure!(end > >::block_number(), Error::::CannotEndInPast); let deposit = T::SubmissionDeposit::get(); let transfer = WithdrawReason::Transfer.into(); let imb = T::Currency::withdraw(&owner, deposit, transfer, AllowDeath)?; let index = FundCount::get(); - let next_index = index.checked_add(1).ok_or("overflow when adding fund")?; + let next_index = index.checked_add(1).ok_or(Error::::Overflow)?; FundCount::put(next_index); // No fees are paid here if we need to create this account; that's why we don't just @@ -239,14 +287,14 @@ decl_module! { fn contribute(origin, #[compact] index: FundIndex, #[compact] value: BalanceOf) { let who = ensure_signed(origin)?; - ensure!(value >= T::MinContribution::get(), "contribution too small"); - let mut fund = Self::funds(index).ok_or("invalid fund index")?; - fund.raised = fund.raised.checked_add(&value).ok_or("overflow when adding new funds")?; - ensure!(fund.raised <= fund.cap, "contributions exceed cap"); + ensure!(value >= T::MinContribution::get(), Error::::ContributionTooSmall); + let mut fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + fund.raised = fund.raised.checked_add(&value).ok_or(Error::::Overflow)?; + ensure!(fund.raised <= fund.cap, Error::::CapExceeded); // Make sure crowdfund has not ended let now = >::block_number(); - ensure!(fund.end > now, "contribution period ended"); + ensure!(fund.end > now, Error::::ContributionPeriodOver); T::Currency::transfer(&who, &Self::fund_account_id(index), value, AllowDeath)?; @@ -301,9 +349,9 @@ decl_module! { ) { let who = ensure_signed(origin)?; - let mut fund = Self::funds(index).ok_or("invalid fund index")?; - ensure!(fund.owner == who, "origin must be fund owner"); - ensure!(fund.deploy_data.is_none(), "deploy data already set"); + let mut fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + ensure!(fund.owner == who, Error::::InvalidOrigin); // must be fund owner + ensure!(fund.deploy_data.is_none(), Error::::ExistingDeployData); fund.deploy_data = Some((code_hash, initial_head_data)); @@ -324,9 +372,9 @@ decl_module! { ) { let _ = ensure_signed(origin)?; - let mut fund = Self::funds(index).ok_or("invalid fund index")?; - let (code_hash, initial_head_data) = fund.clone().deploy_data.ok_or("deploy data not fixed")?; - ensure!(fund.parachain.is_none(), "fund already onboarded"); + let mut fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + let (code_hash, initial_head_data) = fund.clone().deploy_data.ok_or(Error::::UnsetDeployData)?; + ensure!(fund.parachain.is_none(), Error::::AlreadyOnboard); fund.parachain = Some(para_id); let fund_origin = system::RawOrigin::Signed(Self::fund_account_id(index)).into(); @@ -341,13 +389,13 @@ decl_module! { fn begin_retirement(origin, #[compact] index: FundIndex) { let _ = ensure_signed(origin)?; - let mut fund = Self::funds(index).ok_or("invalid fund index")?; - let parachain_id = fund.parachain.take().ok_or("fund has no parachain")?; + let mut fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + let parachain_id = fund.parachain.take().ok_or(Error::::NotParachain)?; // No deposit information implies the parachain was off-boarded - ensure!(>::deposits(parachain_id).len() == 0, "parachain still has deposit"); + ensure!(>::deposits(parachain_id).len() == 0, Error::::ParaHasDeposit); let account = Self::fund_account_id(index); // Funds should be returned at the end of off-boarding - ensure!(T::Currency::free_balance(&account) >= fund.raised, "funds not yet returned"); + ensure!(T::Currency::free_balance(&account) >= fund.raised, Error::::FundsNotReturned); // This fund just ended. Withdrawal period begins. let now = >::block_number(); @@ -362,15 +410,15 @@ decl_module! { fn withdraw(origin, #[compact] index: FundIndex) { let who = ensure_signed(origin)?; - let mut fund = Self::funds(index).ok_or("invalid fund index")?; - ensure!(fund.parachain.is_none(), "fund has not retired"); + let mut fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + ensure!(fund.parachain.is_none(), Error::::FundNotRetired); let now = >::block_number(); // `fund.end` can represent the end of a failed crowdsale or the beginning of retirement - ensure!(now >= fund.end, "fund has not ended"); + ensure!(now >= fund.end, Error::::FundNotEnded); let balance = Self::contribution_get(index, &who); - ensure!(balance > Zero::zero(), "no contributions stored"); + ensure!(balance > Zero::zero(), Error::::NoContributions); // Avoid using transfer to ensure we don't pay any fees. let fund_account = &Self::fund_account_id(index); @@ -392,12 +440,12 @@ decl_module! { fn dissolve(origin, #[compact] index: FundIndex) { let _ = ensure_signed(origin)?; - let fund = Self::funds(index).ok_or("invalid fund index")?; - ensure!(fund.parachain.is_none(), "cannot dissolve fund with active parachain"); + let fund = Self::funds(index).ok_or(Error::::InvalidFundIndex)?; + ensure!(fund.parachain.is_none(), Error::::HasActiveParachain); let now = >::block_number(); ensure!( now >= fund.end.saturating_add(T::RetirementPeriod::get()), - "retirement period not over" + Error::::InRetirementPeriod ); let account = Self::fund_account_id(index); @@ -673,6 +721,7 @@ mod tests { type Crowdfund = Module; type RandomnessCollectiveFlip = randomness_collective_flip::Module; use balances::Error as BalancesError; + use slots::Error as SlotsError; // This function basically just builds a genesis storage key/value store according to // our desired mockup. @@ -749,11 +798,20 @@ mod tests { fn create_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot create a crowdfund with bad slots - assert_noop!(Crowdfund::create(Origin::signed(1), 1000, 4, 1, 9), "last slot must be greater than first slot"); - assert_noop!(Crowdfund::create(Origin::signed(1), 1000, 1, 5, 9), "last slot cannot be more then 3 more than first slot"); + assert_noop!( + Crowdfund::create(Origin::signed(1), 1000, 4, 1, 9), + Error::::LastSlotBeforeFirstSlot + ); + assert_noop!( + Crowdfund::create(Origin::signed(1), 1000, 1, 5, 9), + Error::::LastSlotTooFarInFuture + ); // Cannot create a crowdfund without some deposit funds - assert_noop!(Crowdfund::create(Origin::signed(1337), 1000, 1, 3, 9), BalancesError::::InsufficientBalance); + assert_noop!( + Crowdfund::create(Origin::signed(1337), 1000, 1, 3, 9), + BalancesError::::InsufficientBalance + ); }); } @@ -791,22 +849,22 @@ mod tests { fn contribute_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot contribute to non-existing fund - assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 49), "invalid fund index"); + assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 49), Error::::InvalidFundIndex); // Cannot contribute below minimum contribution - assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 9), "contribution too small"); + assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 9), Error::::ContributionTooSmall); // Set up a crowdfund assert_ok!(Crowdfund::create(Origin::signed(1), 1000, 1, 4, 9)); assert_ok!(Crowdfund::contribute(Origin::signed(1), 0, 101)); // Cannot contribute past the limit - assert_noop!(Crowdfund::contribute(Origin::signed(2), 0, 900), "contributions exceed cap"); + assert_noop!(Crowdfund::contribute(Origin::signed(2), 0, 900), Error::::CapExceeded); // Move past end date run_to_block(10); // Cannot contribute to ended fund - assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 49), "contribution period ended"); + assert_noop!(Crowdfund::contribute(Origin::signed(1), 0, 49), Error::::ContributionPeriodOver); }); } @@ -845,7 +903,7 @@ mod tests { 0, ::Hash::default(), vec![0]), - "origin must be fund owner" + Error::::InvalidOrigin ); // Cannot set deploy data to an invalid index @@ -854,7 +912,7 @@ mod tests { 1, ::Hash::default(), vec![0]), - "invalid fund index" + Error::::InvalidFundIndex ); // Cannot set deploy data after it already has been set @@ -870,7 +928,7 @@ mod tests { 0, ::Hash::default(), vec![1]), - "deploy data already set" + Error::::ExistingDeployData ); }); } @@ -924,9 +982,9 @@ mod tests { run_to_block(10); // Cannot onboard invalid fund index - assert_noop!(Crowdfund::onboard(Origin::signed(1), 1, 0.into()), "invalid fund index"); + assert_noop!(Crowdfund::onboard(Origin::signed(1), 1, 0.into()), Error::::InvalidFundIndex); // Cannot onboard crowdfund without deploy data - assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 0.into()), "deploy data not fixed"); + assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 0.into()), Error::::UnsetDeployData); // Add deploy data assert_ok!(Crowdfund::fix_deploy_data( @@ -937,13 +995,13 @@ mod tests { )); // Cannot onboard fund with incorrect parachain id - assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 1.into()), "parachain id not in onboarding"); + assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 1.into()), SlotsError::::ParaNotOnboarding); // Onboard crowdfund assert_ok!(Crowdfund::onboard(Origin::signed(1), 0, 0.into())); // Cannot onboard fund again - assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 0.into()), "fund already onboarded"); + assert_noop!(Crowdfund::onboard(Origin::signed(1), 0, 0.into()), Error::::AlreadyOnboard); }); } @@ -1011,7 +1069,7 @@ mod tests { run_to_block(10); // Cannot retire fund that is not onboarded - assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), "fund has no parachain"); + assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), Error::::NotParachain); // Onboard crowdfund assert_ok!(Crowdfund::onboard(Origin::signed(1), 0, 0.into())); @@ -1020,16 +1078,16 @@ mod tests { assert_eq!(fund.parachain, Some(0.into())); // Cannot retire fund whose deposit has not been returned - assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), "parachain still has deposit"); + assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), Error::::ParaHasDeposit); run_to_block(50); // Cannot retire invalid fund index - assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 1), "invalid fund index"); + assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 1), Error::::InvalidFundIndex); // Cannot retire twice assert_ok!(Crowdfund::begin_retirement(Origin::signed(1), 0)); - assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), "fund has no parachain"); + assert_noop!(Crowdfund::begin_retirement(Origin::signed(1), 0), Error::::NotParachain); }); } @@ -1072,14 +1130,14 @@ mod tests { run_to_block(5); // Cannot withdraw before fund ends - assert_noop!(Crowdfund::withdraw(Origin::signed(1), 0), "fund has not ended"); + assert_noop!(Crowdfund::withdraw(Origin::signed(1), 0), Error::::FundNotEnded); run_to_block(10); // Cannot withdraw if they did not contribute - assert_noop!(Crowdfund::withdraw(Origin::signed(2), 0), "no contributions stored"); + assert_noop!(Crowdfund::withdraw(Origin::signed(2), 0), Error::::NoContributions); // Cannot withdraw from a non-existent fund - assert_noop!(Crowdfund::withdraw(Origin::signed(1), 1), "invalid fund index"); + assert_noop!(Crowdfund::withdraw(Origin::signed(1), 1), Error::::InvalidFundIndex); }); } @@ -1130,9 +1188,9 @@ mod tests { assert_ok!(Crowdfund::contribute(Origin::signed(3), 0, 300)); // Cannot dissolve an invalid fund index - assert_noop!(Crowdfund::dissolve(Origin::signed(1), 1), "invalid fund index"); + assert_noop!(Crowdfund::dissolve(Origin::signed(1), 1), Error::::InvalidFundIndex); // Cannot dissolve a fund in progress - assert_noop!(Crowdfund::dissolve(Origin::signed(1), 0), "retirement period not over"); + assert_noop!(Crowdfund::dissolve(Origin::signed(1), 0), Error::::InRetirementPeriod); run_to_block(10); @@ -1146,7 +1204,7 @@ mod tests { assert_ok!(Crowdfund::onboard(Origin::signed(1), 0, 0.into())); // Cannot dissolve an active fund - assert_noop!(Crowdfund::dissolve(Origin::signed(1), 0), "cannot dissolve fund with active parachain"); + assert_noop!(Crowdfund::dissolve(Origin::signed(1), 0), Error::::HasActiveParachain); }); } diff --git a/runtime/common/src/parachains.rs b/runtime/common/src/parachains.rs index a6b8941548b4..548340fa93d1 100644 --- a/runtime/common/src/parachains.rs +++ b/runtime/common/src/parachains.rs @@ -35,7 +35,7 @@ use primitives::{ }, }; use frame_support::{ - Parameter, dispatch::DispatchResult, decl_storage, decl_module, ensure, + Parameter, dispatch::DispatchResult, decl_storage, decl_module, decl_error, ensure, traits::{Currency, Get, WithdrawReason, ExistenceRequirement, Randomness}, }; @@ -182,10 +182,10 @@ decl_storage! { /// The ordered list of ParaIds that have a `RelayDispatchQueue` entry. NeedsDispatch: Vec; - /// Some if the parachain heads get updated in this block, along with the parachain IDs that - /// did update. Ordered in the same way as `registrar::Active` (i.e. by ParaId). + /// `Some` if the parachain heads get updated in this block, along with the parachain IDs + /// that did update. Ordered in the same way as `registrar::Active` (i.e. by ParaId). /// - /// None if not yet updated. + /// `None` if not yet updated. pub DidUpdate: Option>; } add_extra_genesis { @@ -194,19 +194,60 @@ decl_storage! { } } +decl_error!{ + pub enum Error for Module{ + /// Parachain heads must be updated only once in the block. + TooManyHeadUpdates, + /// Too many parachain candidates. + TooManyParaCandidates, + /// Proposed heads must be ascending order by parachain ID without duplicate. + HeadsOutOfOrder, + /// Candidate is for an unregistered parachain. + UnregisteredPara, + /// Invalid collator. + InvalidCollator, + /// The message queue is full. Messages will be added when there is space. + QueueFull, + /// The message origin is invalid. + InvalidMessageOrigin, + /// Egress routes should be in ascending order by parachain ID without duplicates. + EgressOutOfOrder, + /// A parachain cannot route a message to itself. + SelfAddressed, + /// The trie root cannot be empty. + EmptyTrieRoot, + /// Cannot route to a non-existing parachain. + DestinationDoesNotExist, + /// No validator group for parachain. + NoValidatorGroup, + /// Not enough validity votes for candidate. + NotEnoughValidityVotes, + /// The number of attestations exceeds the number of authorities. + VotesExceedsAuthorities, + /// Attesting validator not on this chain's validation duty. + WrongValidatorAttesting, + /// Invalid signature from attester. + InvalidSignature, + /// Extra untagged validity votes along with candidate. + UntaggedVotes, + } +} + decl_module! { /// Parachains module. pub struct Module for enum Call where origin: ::Origin { + type Error = Error; + /// Provide candidate receipts for parachains, in ascending order by id. #[weight = SimpleDispatchInfo::FixedNormal(1_000_000)] pub fn set_heads(origin, heads: Vec) -> DispatchResult { ensure_none(origin)?; - ensure!(!::exists(), "Parachain heads must be updated only once in the block"); + ensure!(!::exists(), Error::::TooManyHeadUpdates); let active_parachains = Self::active_parachains(); let parachain_count = active_parachains.len(); - ensure!(heads.len() <= parachain_count, "Too many parachain candidates"); + ensure!(heads.len() <= parachain_count, Error::::TooManyParaCandidates); let mut proceeded = Vec::with_capacity(heads.len()); @@ -221,15 +262,15 @@ decl_module! { // proposed heads must be ascending order by parachain ID without duplicate. ensure!( last_id.as_ref().map_or(true, |x| x < &id), - "candidate out of order" + Error::::HeadsOutOfOrder ); // must be unknown since active parachains are always sorted. let (_, maybe_required_collator) = iter.find(|para| para.0 == id) - .ok_or("candidate for unregistered parachain {}")?; + .ok_or(Error::::UnregisteredPara)?; if let Some((required_collator, _)) = maybe_required_collator { - ensure!(required_collator == &head.candidate.collator, "invalid collator"); + ensure!(required_collator == &head.candidate.collator, Error::::InvalidCollator); } Self::check_upward_messages( @@ -371,11 +412,11 @@ impl Module { .fold(size as usize, |a, x| a + x.data.len()) <= watermark_queue_size ), - "Messages added when queue full" + Error::::QueueFull ); if !id.is_system() { for m in upward_messages.iter() { - ensure!(m.origin != ParachainDispatchOrigin::Root, "bad message origin"); + ensure!(m.origin != ParachainDispatchOrigin::Root, Error::::InvalidMessageOrigin); } } } @@ -625,25 +666,25 @@ impl Module { // egress routes should be ascending order by parachain ID without duplicate. ensure!( last_egress_id.as_ref().map_or(true, |x| x < &egress_para_id), - "Egress routes out of order by ID", + Error::::EgressOutOfOrder, ); // a parachain can't route to self ensure!( *egress_para_id != head.candidate.parachain_index, - "Parachain routing to self", + Error::::SelfAddressed, ); // no empty trie roots ensure!( *root != EMPTY_TRIE_ROOT.into(), - "Empty trie root included", + Error::::EmptyTrieRoot, ); // can't route to a parachain which doesn't exist ensure!( iter.find(|x| x == egress_para_id).is_some(), - "Routing to non-existent parachain", + Error::::DestinationDoesNotExist, ); last_egress_id = Some(egress_para_id) @@ -737,16 +778,16 @@ impl Module { for candidate in attested_candidates { let para_id = candidate.parachain_index(); let validator_group = validator_groups.group_for(para_id) - .ok_or("no validator group for parachain")?; + .ok_or(Error::::NoValidatorGroup)?; ensure!( candidate.validity_votes.len() >= majority_of(validator_group.len()), - "Not enough validity attestations", + Error::::NotEnoughValidityVotes, ); ensure!( candidate.validity_votes.len() <= authorities.len(), - "The number of attestations exceeds the number of authorities", + Error::::VotesExceedsAuthorities, ); let fees = candidate.candidate().fees; @@ -764,7 +805,7 @@ impl Module { .enumerate() { let validity_attestation = match candidate.validity_votes.get(vote_index) { - None => Err("Not enough validity votes")?, + None => Err(Error::::NotEnoughValidityVotes)?, Some(v) => { expected_votes_len = vote_index + 1; v @@ -772,7 +813,7 @@ impl Module { }; if validator_group.iter().find(|&(idx, _)| *idx == auth_index).is_none() { - Err("Attesting validator not on this chain's validation duty.")? + Err(Error::::WrongValidatorAttesting)? } let (payload, sig) = match validity_attestation { @@ -798,7 +839,7 @@ impl Module { ensure!( sig.verify(&payload[..], &authorities[auth_index]), - "Candidate validity attestation signature is bad.", + Error::::InvalidSignature, ); } @@ -806,7 +847,7 @@ impl Module { ensure!( candidate.validity_votes.len() == expected_votes_len, - "Extra untagged validity votes along with candidate", + Error::::UntaggedVotes ); } @@ -915,7 +956,7 @@ mod tests { }; use keyring::Sr25519Keyring; use frame_support::{ - impl_outer_origin, impl_outer_dispatch, assert_ok, assert_err, parameter_types, + impl_outer_origin, impl_outer_dispatch, assert_ok, assert_err, assert_noop, parameter_types, }; use crate::parachains; use crate::registrar; @@ -1447,7 +1488,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full" + Error::::QueueFull ); // too many messages. @@ -1458,7 +1499,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full" + Error::::QueueFull ); }); } @@ -1480,7 +1521,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full" + Error::::QueueFull ); }); } @@ -1501,7 +1542,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full" + Error::::QueueFull ); }); } @@ -1522,7 +1563,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full", + Error::::QueueFull ); }); } @@ -1543,7 +1584,7 @@ mod tests { ]; assert_err!( Parachains::check_upward_messages(0.into(), &messages, 2, 3), - "Messages added when queue full", + Error::::QueueFull ); }); } @@ -1968,12 +2009,10 @@ mod tests { make_attestations(&mut candidate); - let result = Parachains::dispatch( - set_heads(vec![candidate.clone()]), - Origin::NONE, + assert_noop!( + Parachains::set_heads(Origin::NONE, vec![candidate.clone()]), + Error::::DestinationDoesNotExist ); - - assert_eq!(Err("Routing to non-existent parachain".into()), result); }); } @@ -1993,12 +2032,10 @@ mod tests { make_attestations(&mut candidate); - let result = Parachains::dispatch( - set_heads(vec![candidate.clone()]), - Origin::NONE, + assert_noop!( + Parachains::set_heads(Origin::NONE, vec![candidate.clone()]), + Error::::SelfAddressed ); - - assert_eq!(Err("Parachain routing to self".into()), result); }); } @@ -2018,12 +2055,10 @@ mod tests { make_attestations(&mut candidate); - let result = Parachains::dispatch( - set_heads(vec![candidate.clone()]), - Origin::NONE, + assert_noop!( + Parachains::set_heads(Origin::NONE, vec![candidate.clone()]), + Error::::EgressOutOfOrder ); - - assert_eq!(Err("Egress routes out of order by ID".into()), result); }); } @@ -2043,12 +2078,10 @@ mod tests { make_attestations(&mut candidate); - let result = Parachains::dispatch( - set_heads(vec![candidate.clone()]), - Origin::NONE, + assert_noop!( + Parachains::set_heads(Origin::NONE, vec![candidate.clone()]), + Error::::EmptyTrieRoot ); - - assert_eq!(Err("Empty trie root included".into()), result); }); } diff --git a/runtime/common/src/registrar.rs b/runtime/common/src/registrar.rs index 288715b93270..8e6fdd95e03f 100644 --- a/runtime/common/src/registrar.rs +++ b/runtime/common/src/registrar.rs @@ -29,7 +29,7 @@ use sp_runtime::{ }; use frame_support::{ - decl_storage, decl_module, decl_event, ensure, + decl_storage, decl_module, decl_event, decl_error, ensure, dispatch::{DispatchResult, IsSubType}, traits::{Get, Currency, ReservableCurrency}, weights::{SimpleDispatchInfo, DispatchInfo}, }; @@ -70,11 +70,11 @@ impl Registrar for Module { code: Vec, initial_head_data: Vec, ) -> DispatchResult { - ensure!(!Paras::exists(id), "Parachain already exists"); + ensure!(!Paras::exists(id), Error::::ParaAlreadyExists); if let Scheduling::Always = info.scheduling { Parachains::mutate(|parachains| match parachains.binary_search(&id) { - Ok(_) => Err("Parachain already exists"), + Ok(_) => Err(Error::::ParaAlreadyExists), Err(idx) => { parachains.insert(idx, id); Ok(()) @@ -88,12 +88,12 @@ impl Registrar for Module { } fn deregister_para(id: ParaId) -> DispatchResult { - let info = Paras::take(id).ok_or("Invalid id")?; + let info = Paras::take(id).ok_or(Error::::InvalidChainId)?; if let Scheduling::Always = info.scheduling { Parachains::mutate(|parachains| parachains.binary_search(&id) .map(|index| parachains.remove(index)) - .map_err(|_| "Invalid id") + .map_err(|_| Error::::InvalidChainId) )?; } >::cleanup_para(id); @@ -214,9 +214,22 @@ pub fn swap_ordered_existence(ids: &mut [T], one: T, ids.sort(); } +decl_error!{ + pub enum Error for Module{ + /// Parachain already exists. + ParaAlreadyExists, + /// Invalid parachain ID. + InvalidChainId, + /// Invalid parathread ID. + InvalidThreadId, + } +} + decl_module! { /// Parachains module. pub struct Module for enum Call where origin: ::Origin { + type Error = Error; + fn deposit_event() = default; /// Register a parachain with given code. @@ -299,8 +312,8 @@ decl_module! { fn deregister_parathread(origin) { let id = parachains::ensure_parachain(::Origin::from(origin))?; - let info = Paras::get(id).ok_or("invalid id")?; - if let Scheduling::Dynamic = info.scheduling {} else { Err("invalid parathread id")? } + let info = Paras::get(id).ok_or(Error::::InvalidChainId)?; + if let Scheduling::Dynamic = info.scheduling {} else { Err(Error::::InvalidThreadId)? } >::deregister_para(id)?; Self::force_unschedule(|i| i == id); @@ -494,7 +507,7 @@ impl rstd::fmt::Debug for LimitParathreadCommits wher /// Custom validity errors used in Polkadot while validating transactions. #[repr(u8)] -pub enum Error { +pub enum ValidityError { /// Parathread ID has already been submitted for this block. Duplicate = 0, /// Parathread ID does not identify a parathread. @@ -527,7 +540,7 @@ impl SignedExtension for LimitParathreadCommits where if let Some(local_call) = call.is_sub_type() { if let Call::select_parathread(id, collator, hash) = local_call { // ensure that the para ID is actually a parathread. - let e = TransactionValidityError::from(InvalidTransaction::Custom(Error::InvalidId as u8)); + let e = TransactionValidityError::from(InvalidTransaction::Custom(ValidityError::InvalidId as u8)); >::ensure_thread_id(*id).ok_or(e)?; // ensure that we haven't already had a full complement of selected parathreads. @@ -544,14 +557,14 @@ impl SignedExtension for LimitParathreadCommits where ); // ensure that this is not selecting a duplicate parathread ID - let e = TransactionValidityError::from(InvalidTransaction::Custom(Error::Duplicate as u8)); + let e = TransactionValidityError::from(InvalidTransaction::Custom(ValidityError::Duplicate as u8)); let pos = selected_threads .binary_search_by(|&(ref other_id, _)| other_id.cmp(id)) .err() .ok_or(e)?; // ensure that this is a live bid (i.e. that the thread's chain head matches) - let e = TransactionValidityError::from(InvalidTransaction::Custom(Error::InvalidId as u8)); + let e = TransactionValidityError::from(InvalidTransaction::Custom(ValidityError::InvalidId as u8)); let head = >::parachain_head(id).ok_or(e)?; let actual = T::Hashing::hash(&head); ensure!(&actual == hash, InvalidTransaction::Stale); diff --git a/runtime/common/src/slots.rs b/runtime/common/src/slots.rs index 566162180377..bd733984398b 100644 --- a/runtime/common/src/slots.rs +++ b/runtime/common/src/slots.rs @@ -25,7 +25,7 @@ use sp_runtime::traits::{ use frame_support::weights::SimpleDispatchInfo; use codec::{Encode, Decode, Codec}; use frame_support::{ - decl_module, decl_storage, decl_event, ensure, dispatch::DispatchResult, + decl_module, decl_storage, decl_event, decl_error, ensure, dispatch::DispatchResult, traits::{Currency, ReservableCurrency, WithdrawReason, ExistenceRequirement, Get, Randomness}, }; use primitives::parachain::{ @@ -224,8 +224,37 @@ decl_event!( } ); +decl_error!{ + pub enum Error for Module{ + /// This auction is already in progress. + AuctionInProgress, + /// The lease period is in the past. + LeasePeriodInPast, + /// The origin for this call must be a parachain. + NotParaOrigin, + /// The parachain ID is not onboarding. + ParaNotOnboarding, + /// The origin for this call must be the origin who registered the parachain. + InvalidOrigin, + /// Parachain is already registered. + AlreadyRegistered, + /// The code must correspond to the hash. + InvalidCode, + /// Deployment data has not been set for this parachain. + UnsetDeployData, + /// The bid must overlap all intersecting ranges. + NonIntersectingRange, + /// Not a current auction. + NotCurrentAuction, + /// Not an auction. + NotAuction, + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { + type Error = Error; + fn deposit_event() = default; fn on_initialize(n: T::BlockNumber) { @@ -274,8 +303,8 @@ decl_module! { #[compact] lease_period_index: LeasePeriodOf ) { ensure_root(origin)?; - ensure!(!Self::is_in_progress(), "auction already in progress"); - ensure!(lease_period_index >= Self::lease_period_index(), "lease period in past"); + ensure!(!Self::is_in_progress(), Error::::AuctionInProgress); + ensure!(lease_period_index >= Self::lease_period_index(), Error::::LeasePeriodInPast); // Bump the counter. let n = ::mutate(|n| { *n += 1; *n }); @@ -340,7 +369,7 @@ decl_module! { ) { let who = ensure_signed(origin)?; let para_id = ::try_from_account(&who) - .ok_or("account is not a parachain")?; + .ok_or(Error::::NotParaOrigin)?; let bidder = Bidder::Existing(para_id); Self::handle_bid(bidder, auction_index, first_slot, last_slot, amount)?; } @@ -355,7 +384,7 @@ decl_module! { let who = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; let para_id = ::try_from_account(&who) - .ok_or("not a parachain origin")?; + .ok_or(Error::::NotParaOrigin)?; >::insert(para_id, dest); } @@ -375,11 +404,11 @@ decl_module! { ) { let who = ensure_signed(origin)?; let (starts, details) = >::get(¶_id) - .ok_or("parachain id not in onboarding")?; + .ok_or(Error::::ParaNotOnboarding)?; if let IncomingParachain::Unset(ref nb) = details { - ensure!(nb.who == who && nb.sub == sub, "parachain not registered by origin"); + ensure!(nb.who == who && nb.sub == sub, Error::::InvalidOrigin); } else { - Err("already registered")? + Err(Error::::AlreadyRegistered)? } let item = (starts, IncomingParachain::Fixed{code_hash, initial_head_data}); >::insert(¶_id, item); @@ -400,9 +429,9 @@ decl_module! { #[weight = SimpleDispatchInfo::FixedNormal(5_000_000)] pub fn elaborate_deploy_data(_origin, #[compact] para_id: ParaId, code: Vec) -> DispatchResult { let (starts, details) = >::get(¶_id) - .ok_or("parachain id not in onboarding")?; + .ok_or(Error::::ParaNotOnboarding)?; if let IncomingParachain::Fixed{code_hash, initial_head_data} = details { - ensure!(::Hashing::hash(&code) == code_hash, "code not doesn't correspond to hash"); + ensure!(::Hashing::hash(&code) == code_hash, Error::::InvalidCode); if starts > Self::lease_period_index() { // Hasn't yet begun. Replace the on-boarding entry with the new information. let item = (starts, IncomingParachain::Deploy{code, initial_head_data}); @@ -417,7 +446,7 @@ decl_module! { Ok(()) } else { - Err("deploy data not yet fixed".into()) + Err(Error::::UnsetDeployData)? } } } @@ -684,9 +713,9 @@ impl Module { amount: BalanceOf ) -> DispatchResult { // Bidding on latest auction. - ensure!(auction_index == ::get(), "not current auction"); + ensure!(auction_index == ::get(), Error::::NotCurrentAuction); // Assume it's actually an auction (this should never fail because of above). - let (first_lease_period, _) = >::get().ok_or("not an auction")?; + let (first_lease_period, _) = >::get().ok_or(Error::::NotAuction)?; // Our range. let range = SlotRange::new_bounded(first_lease_period, first_slot, last_slot)?; @@ -708,7 +737,7 @@ impl Module { .expect("array has SLOT_RANGE_COUNT items; index never reaches that value; qed") ) )), - "bidder winning non-intersecting range", + Error::::NonIntersectingRange, ); // Ok; we are the new winner of this range - reserve the additional amount and record. @@ -1205,7 +1234,7 @@ mod tests { assert_ok!(Slots::bid(Origin::signed(1), 0, 1, 2, 2, 1)); assert_noop!( Slots::bid(Origin::signed(1), 0, 1, 3, 3, 1), - "bidder winning non-intersecting range" + Error::::NonIntersectingRange ); }); } From f2ecd6a01e6d3cf7bfd45853cc42b23b33e22eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 23 Jan 2020 21:44:43 +0100 Subject: [PATCH 2/2] Apply suggestions from code review --- runtime/common/src/attestations.rs | 4 ++-- runtime/common/src/claims.rs | 4 ++-- runtime/common/src/crowdfund.rs | 4 ++-- runtime/common/src/parachains.rs | 4 ++-- runtime/common/src/registrar.rs | 4 ++-- runtime/common/src/slots.rs | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/runtime/common/src/attestations.rs b/runtime/common/src/attestations.rs index aea9b89a3d24..067493bf66c6 100644 --- a/runtime/common/src/attestations.rs +++ b/runtime/common/src/attestations.rs @@ -108,8 +108,8 @@ decl_storage! { } } -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// More attestations can be added only once in a block. TooManyAttestations, } diff --git a/runtime/common/src/claims.rs b/runtime/common/src/claims.rs index fb8dedf19a50..45f31fe934d6 100644 --- a/runtime/common/src/claims.rs +++ b/runtime/common/src/claims.rs @@ -102,8 +102,8 @@ decl_event!( } ); -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// Invalid Ethereum signature. InvalidEthereumSignature, /// Ethereum address has no claim. diff --git a/runtime/common/src/crowdfund.rs b/runtime/common/src/crowdfund.rs index f47cfad1ba6f..5cfe652bb5a2 100644 --- a/runtime/common/src/crowdfund.rs +++ b/runtime/common/src/crowdfund.rs @@ -187,8 +187,8 @@ decl_event! { } } -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// Last slot must be greater than first slot. LastSlotBeforeFirstSlot, /// The last slot cannot be more then 3 slots after the first slot. diff --git a/runtime/common/src/parachains.rs b/runtime/common/src/parachains.rs index 548340fa93d1..3cf066a0cd35 100644 --- a/runtime/common/src/parachains.rs +++ b/runtime/common/src/parachains.rs @@ -194,8 +194,8 @@ decl_storage! { } } -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// Parachain heads must be updated only once in the block. TooManyHeadUpdates, /// Too many parachain candidates. diff --git a/runtime/common/src/registrar.rs b/runtime/common/src/registrar.rs index 8e6fdd95e03f..62ea529d09a4 100644 --- a/runtime/common/src/registrar.rs +++ b/runtime/common/src/registrar.rs @@ -214,8 +214,8 @@ pub fn swap_ordered_existence(ids: &mut [T], one: T, ids.sort(); } -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// Parachain already exists. ParaAlreadyExists, /// Invalid parachain ID. diff --git a/runtime/common/src/slots.rs b/runtime/common/src/slots.rs index bd733984398b..6f6f56ad8666 100644 --- a/runtime/common/src/slots.rs +++ b/runtime/common/src/slots.rs @@ -224,8 +224,8 @@ decl_event!( } ); -decl_error!{ - pub enum Error for Module{ +decl_error! { + pub enum Error for Module { /// This auction is already in progress. AuctionInProgress, /// The lease period is in the past.