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 <shawntabrizi@gmail.com>
This commit is contained in:
Falco Hirschenberger
2021-03-24 18:35:05 +01:00
committed by GitHub
parent f93d7b874e
commit 9a72134188
13 changed files with 66 additions and 34 deletions
+2 -1
View File
@@ -763,7 +763,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
);
}
// 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<T: Config<I>, I: 'static> Pallet<T, I> {
}
}
});
debug_assert!(res.is_ok());
let existed = Locks::<T, I>::contains_key(who);
if locks.is_empty() {
+3 -3
View File
@@ -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!(
+15 -7
View File
@@ -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::<T>::BountyCanceled(bounty_id));
@@ -736,7 +743,8 @@ impl<T: Config> pallet_treasury::SpendFunds<T> for Module<T> {
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));
+4 -2
View File
@@ -1010,7 +1010,8 @@ decl_module! {
ensure!(now >= since + voting + additional, Error::<T>::TooEarly);
ensure!(expiry.map_or(true, |e| now > e), Error::<T>::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());
<Preimages<T>>::remove(&proposal_hash);
Self::deposit_event(RawEvent::PreimageReaped(proposal_hash, provider, deposit, who));
}
@@ -1541,7 +1542,8 @@ impl<T: Config> Module<T> {
let preimage = <Preimages<T>>::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();
+10 -5
View File
@@ -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! {
<SuperOf<T>>::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::<T>::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();
<IdentityOf<T>>::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));
});
}
+2 -1
View File
@@ -324,7 +324,8 @@ decl_module! {
let winning_number = Self::choose_winner(ticket_count);
let winner = Tickets::<T>::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));
+2 -1
View File
@@ -436,7 +436,8 @@ decl_module! {
ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
ensure!(m.depositor == who, Error::<T>::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());
<Multisigs<T>>::remove(&id, &call_hash);
Self::clear_call(&call_hash);
+2 -1
View File
@@ -179,7 +179,8 @@ decl_module! {
let deposit = <NameOf<T>>::take(&sender).ok_or(Error::<T>::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));
}
+10 -5
View File
@@ -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, _) => {
<Vouching<T, I>>::remove(&voucher);
@@ -1241,7 +1242,8 @@ impl<T: Config<I>, I: Instance> Module<T, I> {
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, _) => {
<Vouching<T, I>>::remove(&voucher);
@@ -1408,7 +1410,8 @@ impl<T: Config<I>, I: Instance> Module<T, I> {
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<T: Config<I>, I: Instance> Module<T, I> {
// 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<T: Config<I>, I: Instance> Module<T, I> {
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) => {
@@ -80,7 +80,8 @@ mod tests {
let translate_fn = |old: Option<u32>| -> 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));
+8 -4
View File
@@ -291,7 +291,8 @@ decl_module! {
Reasons::<T>::remove(&tip.reason);
Tips::<T>::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<T: Config> Module<T> {
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<T: Config> Module<T> {
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));
}
+2 -1
View File
@@ -377,7 +377,8 @@ impl<T: Config<I>, I: Instance> Module<T, I> {
<Proposals<T, I>>::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));
+4 -2
View File
@@ -394,7 +394,8 @@ impl<T: Config> VestingSchedule<T::AccountId> for Pallet<T> where
};
Vesting::<T>::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<T: Config> VestingSchedule<T::AccountId> for Pallet<T> where
fn remove_vesting_schedule(who: &T::AccountId) {
Vesting::<T>::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());
}
}