From 9a721341880c563276ac12fd1f4003b7c9cb9b14 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Wed, 24 Mar 2021 18:35:05 +0100 Subject: [PATCH] Check `unreserve` and `transfer` returnvalues in debug code (#8398) * Check `unreserve` and `transfer` returnvalues in debug code fixes #8106 * few more Co-authored-by: Shawn Tabrizi --- substrate/frame/balances/src/lib.rs | 3 ++- substrate/frame/balances/src/tests.rs | 6 ++--- substrate/frame/bounties/src/lib.rs | 22 +++++++++++++------ substrate/frame/democracy/src/lib.rs | 6 +++-- substrate/frame/identity/src/lib.rs | 15 ++++++++----- substrate/frame/lottery/src/lib.rs | 3 ++- substrate/frame/multisig/src/lib.rs | 3 ++- substrate/frame/nicks/src/lib.rs | 3 ++- substrate/frame/society/src/lib.rs | 15 ++++++++----- .../support/src/storage/generator/mod.rs | 3 ++- substrate/frame/tips/src/lib.rs | 12 ++++++---- substrate/frame/treasury/src/lib.rs | 3 ++- substrate/frame/vesting/src/lib.rs | 6 +++-- 13 files changed, 66 insertions(+), 34 deletions(-) diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 8908f4c097..fc4dab7cec 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -763,7 +763,7 @@ impl, I: 'static> Pallet { ); } // No way this can fail since we do not alter the existential balances. - let _ = Self::mutate_account(who, |b| { + let res = Self::mutate_account(who, |b| { b.misc_frozen = Zero::zero(); b.fee_frozen = Zero::zero(); for l in locks.iter() { @@ -775,6 +775,7 @@ impl, I: 'static> Pallet { } } }); + debug_assert!(res.is_ok()); let existed = Locks::::contains_key(who); if locks.is_empty() { diff --git a/substrate/frame/balances/src/tests.rs b/substrate/frame/balances/src/tests.rs index da6c99d46c..3eb70e401e 100644 --- a/substrate/frame/balances/src/tests.rs +++ b/substrate/frame/balances/src/tests.rs @@ -684,7 +684,7 @@ macro_rules! decl_tests { let _ = Balances::deposit_creating(&1, 100); System::set_block_number(2); - let _ = Balances::reserve(&1, 10); + assert_ok!(Balances::reserve(&1, 10)); assert_eq!( last_event(), @@ -692,7 +692,7 @@ macro_rules! decl_tests { ); System::set_block_number(3); - let _ = Balances::unreserve(&1, 5); + assert!(Balances::unreserve(&1, 5).is_zero()); assert_eq!( last_event(), @@ -700,7 +700,7 @@ macro_rules! decl_tests { ); System::set_block_number(4); - let _ = Balances::unreserve(&1, 6); + assert_eq!(Balances::unreserve(&1, 6), 1); // should only unreserve 5 assert_eq!( diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index 7d6cd6fc14..dafa7cd61d 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -435,7 +435,8 @@ decl_module! { } else { // Else this is the curator, willingly giving up their role. // Give back their deposit. - let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); + let err_amount = T::Currency::unreserve(&curator, bounty.curator_deposit); + debug_assert!(err_amount.is_zero()); // Continue to change bounty status below... } }, @@ -548,9 +549,13 @@ decl_module! { let balance = T::Currency::free_balance(&bounty_account); let fee = bounty.fee.min(balance); // just to be safe let payout = balance.saturating_sub(fee); - let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); - let _ = T::Currency::transfer(&bounty_account, &curator, fee, AllowDeath); // should not fail - let _ = T::Currency::transfer(&bounty_account, &beneficiary, payout, AllowDeath); // should not fail + let err_amount = T::Currency::unreserve(&curator, bounty.curator_deposit); + debug_assert!(err_amount.is_zero()); + let res = T::Currency::transfer(&bounty_account, &curator, fee, AllowDeath); // should not fail + debug_assert!(res.is_ok()); + let res = T::Currency::transfer(&bounty_account, &beneficiary, payout, AllowDeath); // should not fail + debug_assert!(res.is_ok()); + *maybe_bounty = None; BountyDescriptions::remove(bounty_id); @@ -604,7 +609,8 @@ decl_module! { }, BountyStatus::Active { curator, .. } => { // Cancelled by council, refund deposit of the working curator. - let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); + let err_amount = T::Currency::unreserve(&curator, bounty.curator_deposit); + debug_assert!(err_amount.is_zero()); // Then execute removal of the bounty below. }, BountyStatus::PendingPayout { .. } => { @@ -621,7 +627,8 @@ decl_module! { BountyDescriptions::remove(bounty_id); let balance = T::Currency::free_balance(&bounty_account); - let _ = T::Currency::transfer(&bounty_account, &Self::account_id(), balance, AllowDeath); // should not fail + let res = T::Currency::transfer(&bounty_account, &Self::account_id(), balance, AllowDeath); // should not fail + debug_assert!(res.is_ok()); *maybe_bounty = None; Self::deposit_event(Event::::BountyCanceled(bounty_id)); @@ -736,7 +743,8 @@ impl pallet_treasury::SpendFunds for Module { bounty.status = BountyStatus::Funded; // return their deposit. - let _ = T::Currency::unreserve(&bounty.proposer, bounty.bond); + let err_amount = T::Currency::unreserve(&bounty.proposer, bounty.bond); + debug_assert!(err_amount.is_zero()); // fund the bounty account imbalance.subsume(T::Currency::deposit_creating(&Self::bounty_account_id(index), bounty.value)); diff --git a/substrate/frame/democracy/src/lib.rs b/substrate/frame/democracy/src/lib.rs index 37a2fd5ce7..b3b37b0b34 100644 --- a/substrate/frame/democracy/src/lib.rs +++ b/substrate/frame/democracy/src/lib.rs @@ -1010,7 +1010,8 @@ decl_module! { ensure!(now >= since + voting + additional, Error::::TooEarly); ensure!(expiry.map_or(true, |e| now > e), Error::::Imminent); - let _ = T::Currency::repatriate_reserved(&provider, &who, deposit, BalanceStatus::Free); + let res = T::Currency::repatriate_reserved(&provider, &who, deposit, BalanceStatus::Free); + debug_assert!(res.is_ok()); >::remove(&proposal_hash); Self::deposit_event(RawEvent::PreimageReaped(proposal_hash, provider, deposit, who)); } @@ -1541,7 +1542,8 @@ impl Module { let preimage = >::take(&proposal_hash); if let Some(PreimageStatus::Available { data, provider, deposit, .. }) = preimage { if let Ok(proposal) = T::Proposal::decode(&mut &data[..]) { - let _ = T::Currency::unreserve(&provider, deposit); + let err_amount = T::Currency::unreserve(&provider, deposit); + debug_assert!(err_amount.is_zero()); Self::deposit_event(RawEvent::PreimageUsed(proposal_hash, provider, deposit)); let ok = proposal.dispatch(frame_system::RawOrigin::Root.into()).is_ok(); diff --git a/substrate/frame/identity/src/lib.rs b/substrate/frame/identity/src/lib.rs index 6d6e3170d5..880d202795 100644 --- a/substrate/frame/identity/src/lib.rs +++ b/substrate/frame/identity/src/lib.rs @@ -598,7 +598,8 @@ decl_module! { T::Currency::reserve(&sender, id.deposit - old_deposit)?; } if old_deposit > id.deposit { - let _ = T::Currency::unreserve(&sender, old_deposit - id.deposit); + let err_amount = T::Currency::unreserve(&sender, old_deposit - id.deposit); + debug_assert!(err_amount.is_zero()); } let judgements = id.judgements.len(); @@ -655,7 +656,8 @@ decl_module! { if old_deposit < new_deposit { T::Currency::reserve(&sender, new_deposit - old_deposit)?; } else if old_deposit > new_deposit { - let _ = T::Currency::unreserve(&sender, old_deposit - new_deposit); + let err_amount = T::Currency::unreserve(&sender, old_deposit - new_deposit); + debug_assert!(err_amount.is_zero()); } // do nothing if they're equal. @@ -713,7 +715,8 @@ decl_module! { >::remove(sub); } - let _ = T::Currency::unreserve(&sender, deposit.clone()); + let err_amount = T::Currency::unreserve(&sender, deposit.clone()); + debug_assert!(err_amount.is_zero()); Self::deposit_event(RawEvent::IdentityCleared(sender, deposit)); @@ -819,7 +822,8 @@ decl_module! { Err(Error::::JudgementGiven)? }; - let _ = T::Currency::unreserve(&sender, fee); + let err_amount = T::Currency::unreserve(&sender, fee); + debug_assert!(err_amount.is_zero()); let judgements = id.judgements.len(); let extra_fields = id.info.additional.len(); >::insert(&sender, id); @@ -1095,7 +1099,8 @@ decl_module! { sub_ids.retain(|x| x != &sub); let deposit = T::SubAccountDeposit::get().min(*subs_deposit); *subs_deposit -= deposit; - let _ = T::Currency::unreserve(&sender, deposit); + let err_amount = T::Currency::unreserve(&sender, deposit); + debug_assert!(err_amount.is_zero()); Self::deposit_event(RawEvent::SubIdentityRemoved(sub, sender, deposit)); }); } diff --git a/substrate/frame/lottery/src/lib.rs b/substrate/frame/lottery/src/lib.rs index 84b924c173..94b7dd4598 100644 --- a/substrate/frame/lottery/src/lib.rs +++ b/substrate/frame/lottery/src/lib.rs @@ -324,7 +324,8 @@ decl_module! { let winning_number = Self::choose_winner(ticket_count); let winner = Tickets::::get(winning_number).unwrap_or(lottery_account); // Not much we can do if this fails... - let _ = T::Currency::transfer(&Self::account_id(), &winner, lottery_balance, KeepAlive); + let res = T::Currency::transfer(&Self::account_id(), &winner, lottery_balance, KeepAlive); + debug_assert!(res.is_ok()); Self::deposit_event(RawEvent::Winner(winner, lottery_balance)); diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 1d3a83ccb6..8c8e1c0dbc 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -436,7 +436,8 @@ decl_module! { ensure!(m.when == timepoint, Error::::WrongTimepoint); ensure!(m.depositor == who, Error::::NotOwner); - let _ = T::Currency::unreserve(&m.depositor, m.deposit); + let err_amount = T::Currency::unreserve(&m.depositor, m.deposit); + debug_assert!(err_amount.is_zero()); >::remove(&id, &call_hash); Self::clear_call(&call_hash); diff --git a/substrate/frame/nicks/src/lib.rs b/substrate/frame/nicks/src/lib.rs index 02eb488c1b..67e62a09da 100644 --- a/substrate/frame/nicks/src/lib.rs +++ b/substrate/frame/nicks/src/lib.rs @@ -179,7 +179,8 @@ decl_module! { let deposit = >::take(&sender).ok_or(Error::::Unnamed)?.1; - let _ = T::Currency::unreserve(&sender, deposit.clone()); + let err_amount = T::Currency::unreserve(&sender, deposit.clone()); + debug_assert!(err_amount.is_zero()); Self::deposit_event(RawEvent::NameCleared(sender, deposit)); } diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs index 8e5ce6c867..64caf32800 100644 --- a/substrate/frame/society/src/lib.rs +++ b/substrate/frame/society/src/lib.rs @@ -590,7 +590,8 @@ decl_module! { // no reason that either should fail. match b.remove(pos).kind { BidKind::Deposit(deposit) => { - let _ = T::Currency::unreserve(&who, deposit); + let err_amount = T::Currency::unreserve(&who, deposit); + debug_assert!(err_amount.is_zero()); } BidKind::Vouch(voucher, _) => { >::remove(&voucher); @@ -1241,7 +1242,8 @@ impl, I: Instance> Module { let Bid { who: popped, kind, .. } = bids.pop().expect("b.len() > 1000; qed"); match kind { BidKind::Deposit(deposit) => { - let _ = T::Currency::unreserve(&popped, deposit); + let err_amount = T::Currency::unreserve(&popped, deposit); + debug_assert!(err_amount.is_zero()); } BidKind::Vouch(voucher, _) => { >::remove(&voucher); @@ -1408,7 +1410,8 @@ impl, I: Instance> Module { Self::bump_payout(winner, maturity, total_slash); } else { // Move the slashed amount back from payouts account to local treasury. - let _ = T::Currency::transfer(&Self::payouts(), &Self::account_id(), total_slash, AllowDeath); + let res = T::Currency::transfer(&Self::payouts(), &Self::account_id(), total_slash, AllowDeath); + debug_assert!(res.is_ok()); } } @@ -1419,7 +1422,8 @@ impl, I: Instance> Module { // this should never fail since we ensure we can afford the payouts in a previous // block, but there's not much we can do to recover if it fails anyway. - let _ = T::Currency::transfer(&Self::account_id(), &Self::payouts(), total_payouts, AllowDeath); + let res = T::Currency::transfer(&Self::account_id(), &Self::payouts(), total_payouts, AllowDeath); + debug_assert!(res.is_ok()); } // if at least one candidate was accepted... @@ -1520,7 +1524,8 @@ impl, I: Instance> Module { BidKind::Deposit(deposit) => { // In the case that a normal deposit bid is accepted we unreserve // the deposit. - let _ = T::Currency::unreserve(candidate, deposit); + let err_amount = T::Currency::unreserve(candidate, deposit); + debug_assert!(err_amount.is_zero()); value } BidKind::Vouch(voucher, tip) => { diff --git a/substrate/frame/support/src/storage/generator/mod.rs b/substrate/frame/support/src/storage/generator/mod.rs index fc2a21ff72..86eafe86f4 100644 --- a/substrate/frame/support/src/storage/generator/mod.rs +++ b/substrate/frame/support/src/storage/generator/mod.rs @@ -80,7 +80,8 @@ mod tests { let translate_fn = |old: Option| -> Option<(u64, u64)> { old.map(|o| (o.into(), (o*2).into())) }; - let _ = Value::translate(translate_fn); + let res = Value::translate(translate_fn); + debug_assert!(res.is_ok()); // new storage should be `(1111, 1111 * 2)` assert_eq!(Value::get(), (1111, 2222)); diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 88cb65963a..6d85df33f1 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -291,7 +291,8 @@ decl_module! { Reasons::::remove(&tip.reason); Tips::::remove(&hash); if !tip.deposit.is_zero() { - let _ = T::Currency::unreserve(&who, tip.deposit); + let err_amount = T::Currency::unreserve(&who, tip.deposit); + debug_assert!(err_amount.is_zero()); } Self::deposit_event(RawEvent::TipRetracted(hash)); } @@ -505,7 +506,8 @@ impl Module { let mut payout = tips[tips.len() / 2].1.min(max_payout); if !tip.deposit.is_zero() { - let _ = T::Currency::unreserve(&tip.finder, tip.deposit); + let err_amount = T::Currency::unreserve(&tip.finder, tip.deposit); + debug_assert!(err_amount.is_zero()); } if tip.finders_fee && tip.finder != tip.who { @@ -514,11 +516,13 @@ impl Module { payout -= finders_fee; // this should go through given we checked it's at most the free balance, but still // we only make a best-effort. - let _ = T::Currency::transfer(&treasury, &tip.finder, finders_fee, KeepAlive); + let res = T::Currency::transfer(&treasury, &tip.finder, finders_fee, KeepAlive); + debug_assert!(res.is_ok()); } // same as above: best-effort only. - let _ = T::Currency::transfer(&treasury, &tip.who, payout, KeepAlive); + let res = T::Currency::transfer(&treasury, &tip.who, payout, KeepAlive); + debug_assert!(res.is_ok()); Self::deposit_event(RawEvent::TipClosed(hash, tip.who, payout)); } diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index 46098c14fb..cef50706b5 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -377,7 +377,8 @@ impl, I: Instance> Module { >::remove(index); // return their deposit. - let _ = T::Currency::unreserve(&p.proposer, p.bond); + let err_amount = T::Currency::unreserve(&p.proposer, p.bond); + debug_assert!(err_amount.is_zero()); // provide the allocation. imbalance.subsume(T::Currency::deposit_creating(&p.beneficiary, p.value)); diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 483e734fe4..c02e9dc78c 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -394,7 +394,8 @@ impl VestingSchedule for Pallet where }; Vesting::::insert(who, vesting_schedule); // it can't fail, but even if somehow it did, we don't really care. - let _ = Self::update_lock(who.clone()); + let res = Self::update_lock(who.clone()); + debug_assert!(res.is_ok()); Ok(()) } @@ -402,7 +403,8 @@ impl VestingSchedule for Pallet where fn remove_vesting_schedule(who: &T::AccountId) { Vesting::::remove(who); // it can't fail, but even if somehow it did, we don't really care. - let _ = Self::update_lock(who.clone()); + let res = Self::update_lock(who.clone()); + debug_assert!(res.is_ok()); } }