From e932c3ecd23077816d3f1ce304be812ea12ec882 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Wed, 7 Apr 2021 23:40:28 +0200 Subject: [PATCH] Add more asserts and debug_asserts (#8541) * Add more asserts and debug_asserts fixing #8106 * Remove assignments * convert debug_assert to runtime assert --- substrate/frame/balances/src/benchmarking.rs | 3 +- substrate/frame/balances/src/tests.rs | 16 +++--- substrate/frame/balances/src/tests_local.rs | 6 ++- .../frame/balances/src/tests_reentrancy.rs | 2 +- substrate/frame/bounties/src/tests.rs | 5 +- substrate/frame/contracts/src/wasm/mod.rs | 54 +++++++++---------- .../src/benchmarking.rs | 2 +- .../election-provider-multi-phase/src/lib.rs | 4 +- substrate/frame/executive/src/lib.rs | 10 ++-- substrate/frame/recovery/src/lib.rs | 3 +- substrate/frame/society/src/lib.rs | 3 +- substrate/frame/staking/src/tests.rs | 3 +- substrate/frame/tips/src/benchmarking.rs | 2 +- substrate/frame/treasury/src/lib.rs | 11 ++-- 14 files changed, 66 insertions(+), 58 deletions(-) diff --git a/substrate/frame/balances/src/benchmarking.rs b/substrate/frame/balances/src/benchmarking.rs index 62959c4f1d..6e86d18d7c 100644 --- a/substrate/frame/balances/src/benchmarking.rs +++ b/substrate/frame/balances/src/benchmarking.rs @@ -44,7 +44,8 @@ benchmarks_instance_pallet! { let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&caller, balance); - // Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account, and reap this user. + // Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account, + // and reap this user. let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); let transfer_amount = existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into(); diff --git a/substrate/frame/balances/src/tests.rs b/substrate/frame/balances/src/tests.rs index 3eb70e401e..de12c39ede 100644 --- a/substrate/frame/balances/src/tests.rs +++ b/substrate/frame/balances/src/tests.rs @@ -169,13 +169,13 @@ macro_rules! decl_tests { &info_from_weight(1), 1, ).is_err()); - assert!( as SignedExtension>::pre_dispatch( + assert_ok!( as SignedExtension>::pre_dispatch( ChargeTransactionPayment::from(0), &1, CALL, &info_from_weight(1), 1, - ).is_ok()); + )); Balances::set_lock(ID_1, &1, 10, WithdrawReasons::TRANSACTION_PAYMENT); assert_ok!(>::transfer(&1, &2, 1, AllowDeath)); @@ -394,7 +394,7 @@ macro_rules! decl_tests { fn refunding_balance_should_work() { <$ext_builder>::default().build().execute_with(|| { let _ = Balances::deposit_creating(&1, 42); - assert!(Balances::mutate_account(&1, |a| a.reserved = 69).is_ok()); + assert_ok!(Balances::mutate_account(&1, |a| a.reserved = 69)); Balances::unreserve(&1, 69); assert_eq!(Balances::free_balance(1), 111); assert_eq!(Balances::reserved_balance(1), 0); @@ -669,7 +669,9 @@ macro_rules! decl_tests { assert_eq!(Balances::reserved_balance(1), 50); // Reserve some free balance - let _ = Balances::slash(&1, 1); + let res = Balances::slash(&1, 1); + assert_eq!(res, (NegativeImbalance::new(1), 0)); + // The account should be dead. assert_eq!(Balances::free_balance(1), 0); assert_eq!(Balances::reserved_balance(1), 0); @@ -727,7 +729,8 @@ macro_rules! decl_tests { ] ); - let _ = Balances::slash(&1, 1); + let res = Balances::slash(&1, 1); + assert_eq!(res, (NegativeImbalance::new(1), 0)); assert_eq!( events(), @@ -756,7 +759,8 @@ macro_rules! decl_tests { ] ); - let _ = Balances::slash(&1, 100); + let res = Balances::slash(&1, 100); + assert_eq!(res, (NegativeImbalance::new(100), 0)); assert_eq!( events(), diff --git a/substrate/frame/balances/src/tests_local.rs b/substrate/frame/balances/src/tests_local.rs index f6f0bf8389..ac5adfd8d1 100644 --- a/substrate/frame/balances/src/tests_local.rs +++ b/substrate/frame/balances/src/tests_local.rs @@ -175,12 +175,14 @@ fn emit_events_with_no_existential_deposit_suicide_with_dust() { ] ); - let _ = Balances::slash(&1, 98); + let res = Balances::slash(&1, 98); + assert_eq!(res, (NegativeImbalance::new(98), 0)); // no events assert_eq!(events(), []); - let _ = Balances::slash(&1, 1); + let res = Balances::slash(&1, 1); + assert_eq!(res, (NegativeImbalance::new(1), 0)); assert_eq!( events(), diff --git a/substrate/frame/balances/src/tests_reentrancy.rs b/substrate/frame/balances/src/tests_reentrancy.rs index 4016cdb463..3d6a90929a 100644 --- a/substrate/frame/balances/src/tests_reentrancy.rs +++ b/substrate/frame/balances/src/tests_reentrancy.rs @@ -105,7 +105,7 @@ impl pallet_transaction_payment::Config for Test { pub struct OnDustRemoval; impl OnUnbalanced> for OnDustRemoval { fn on_nonzero_unbalanced(amount: NegativeImbalance) { - let _ = Balances::resolve_into_existing(&1, amount); + assert_ok!(Balances::resolve_into_existing(&1, amount)); } } parameter_types! { diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs index d676c940f5..c3cfe531ec 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -312,7 +312,7 @@ fn pot_underflow_should_not_diminish() { >::on_initialize(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed - let _ = Balances::deposit_into_existing(&Treasury::account_id(), 100).unwrap(); + assert_ok!(Balances::deposit_into_existing(&Treasury::account_id(), 100)); >::on_initialize(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed @@ -689,7 +689,8 @@ fn claim_handles_high_fee() { >::on_initialize(5); // make fee > balance - let _ = Balances::slash(&Bounties::bounty_account_id(0), 10); + let res = Balances::slash(&Bounties::bounty_account_id(0), 10); + assert_eq!(res.0.peek(), 10); assert_ok!(Bounties::claim_bounty(Origin::signed(1), 0)); diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index f7fde5ba17..3f92320b94 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -254,7 +254,7 @@ mod tests { use sp_core::H256; use hex_literal::hex; use sp_runtime::DispatchError; - use frame_support::{dispatch::DispatchResult, weights::Weight}; + use frame_support::{assert_ok, dispatch::DispatchResult, weights::Weight}; use assert_matches::assert_matches; use pallet_contracts_primitives::{ExecReturnValue, ReturnFlags, ExecError, ErrorOrigin}; use pretty_assertions::assert_eq; @@ -597,12 +597,12 @@ mod tests { #[test] fn contract_transfer() { let mut mock_ext = MockExt::default(); - let _ = execute( + assert_ok!(execute( CODE_TRANSFER, vec![], &mut mock_ext, &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); assert_eq!( &mock_ext.transfers, @@ -663,12 +663,12 @@ mod tests { #[test] fn contract_call() { let mut mock_ext = MockExt::default(); - let _ = execute( + assert_ok!(execute( CODE_CALL, vec![], &mut mock_ext, &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); assert_eq!( &mock_ext.transfers, @@ -739,12 +739,12 @@ mod tests { #[test] fn contract_instantiate() { let mut mock_ext = MockExt::default(); - let _ = execute( + assert_ok!(execute( CODE_INSTANTIATE, vec![], &mut mock_ext, &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); assert_matches!( &mock_ext.instantiates[..], @@ -851,12 +851,12 @@ mod tests { #[test] fn contract_call_limited_gas() { let mut mock_ext = MockExt::default(); - let _ = execute( + assert_ok!(execute( &CODE_TRANSFER_LIMITED_GAS, vec![], &mut mock_ext, &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); assert_eq!( &mock_ext.transfers, @@ -994,12 +994,12 @@ mod tests { #[test] fn caller() { - let _ = execute( + assert_ok!(execute( CODE_CALLER, vec![], MockExt::default(), &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); } /// calls `seal_address` and compares the result with the constant 69. @@ -1047,12 +1047,12 @@ mod tests { #[test] fn address() { - let _ = execute( + assert_ok!(execute( CODE_ADDRESS, vec![], MockExt::default(), &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); + )); } const CODE_BALANCE: &str = r#" @@ -1099,12 +1099,12 @@ mod tests { #[test] fn balance() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_BALANCE, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_GAS_PRICE: &str = r#" @@ -1151,12 +1151,12 @@ mod tests { #[test] fn gas_price() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_GAS_PRICE, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_GAS_LEFT: &str = r#" @@ -1258,12 +1258,12 @@ mod tests { #[test] fn value_transferred() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_VALUE_TRANSFERRED, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_RETURN_FROM_START_FN: &str = r#" @@ -1346,12 +1346,12 @@ mod tests { #[test] fn now() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_TIMESTAMP_NOW, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_MINIMUM_BALANCE: &str = r#" @@ -1397,12 +1397,12 @@ mod tests { #[test] fn minimum_balance() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_MINIMUM_BALANCE, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_TOMBSTONE_DEPOSIT: &str = r#" @@ -1448,12 +1448,12 @@ mod tests { #[test] fn tombstone_deposit() { let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_TOMBSTONE_DEPOSIT, vec![], MockExt::default(), &mut gas_meter, - ).unwrap(); + )); } const CODE_RANDOM: &str = r#" @@ -1637,12 +1637,12 @@ mod tests { fn deposit_event() { let mut mock_ext = MockExt::default(); let mut gas_meter = GasMeter::new(GAS_LIMIT); - let _ = execute( + assert_ok!(execute( CODE_DEPOSIT_EVENT, vec![], &mut mock_ext, &mut gas_meter - ).unwrap(); + )); assert_eq!(mock_ext.events, vec![ (vec![H256::repeat_byte(0x33)], diff --git a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs index 40c7e801ae..90e90d427d 100644 --- a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs @@ -208,7 +208,7 @@ frame_benchmarking::benchmarks! { // assume a queued solution is stored, regardless of where it comes from. >::put(ready_solution); }: { - let _ = as ElectionProvider>::elect(); + assert_ok!( as ElectionProvider>::elect()); } verify { assert!(>::queued_solution().is_none()); assert!(>::get().is_none()); diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 5545b39611..17978566a8 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1411,7 +1411,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_signed()); - let _ = MultiPhase::elect().unwrap(); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -1434,7 +1434,7 @@ mod tests { assert!(MultiPhase::current_phase().is_off()); // this module is now only capable of doing on-chain backup. - let _ = MultiPhase::elect().unwrap(); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); }); diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index fc4f5be5dc..97bdfbfdd5 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -553,25 +553,25 @@ mod tests { #[weight = 100] fn some_function(origin) { // NOTE: does not make any different. - let _ = frame_system::ensure_signed(origin); + frame_system::ensure_signed(origin)?; } #[weight = (200, DispatchClass::Operational)] fn some_root_operation(origin) { - let _ = frame_system::ensure_root(origin); + frame_system::ensure_root(origin)?; } #[weight = 0] fn some_unsigned_message(origin) { - let _ = frame_system::ensure_none(origin); + frame_system::ensure_none(origin)?; } #[weight = 0] fn allowed_unsigned(origin) { - let _ = frame_system::ensure_root(origin)?; + frame_system::ensure_root(origin)?; } #[weight = 0] fn unallowed_unsigned(origin) { - let _ = frame_system::ensure_root(origin)?; + frame_system::ensure_root(origin)?; } // module hooks. diff --git a/substrate/frame/recovery/src/lib.rs b/substrate/frame/recovery/src/lib.rs index cb991e6494..ceb2f5a688 100644 --- a/substrate/frame/recovery/src/lib.rs +++ b/substrate/frame/recovery/src/lib.rs @@ -621,7 +621,8 @@ decl_module! { let active_recovery = >::take(&who, &rescuer).ok_or(Error::::NotStarted)?; // Move the reserved funds from the rescuer to the rescued account. // Acts like a slashing mechanism for those who try to maliciously recover accounts. - let _ = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit, BalanceStatus::Free); + let res = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit, BalanceStatus::Free); + debug_assert!(res.is_ok()); Self::deposit_event(RawEvent::RecoveryClosed(who, rescuer)); } diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs index 64caf32800..171815e56e 100644 --- a/substrate/frame/society/src/lib.rs +++ b/substrate/frame/society/src/lib.rs @@ -997,7 +997,8 @@ decl_module! { match kind { BidKind::Deposit(deposit) => { // Slash deposit and move it to the society account - let _ = T::Currency::repatriate_reserved(&who, &Self::account_id(), deposit, BalanceStatus::Free); + let res = T::Currency::repatriate_reserved(&who, &Self::account_id(), deposit, BalanceStatus::Free); + debug_assert!(res.is_ok()); } BidKind::Vouch(voucher, _) => { // Ban the voucher from vouching again diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 7a3ec19f8a..05eb6fdc5e 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -437,7 +437,8 @@ fn no_candidate_emergency_condition() { ::MinimumValidatorCount::put(10); // try to chill - let _ = Staking::chill(Origin::signed(10)); + let res = Staking::chill(Origin::signed(10)); + assert_ok!(res); // trigger era mock::start_active_era(1); diff --git a/substrate/frame/tips/src/benchmarking.rs b/substrate/frame/tips/src/benchmarking.rs index e6a0284d82..6c304fabb5 100644 --- a/substrate/frame/tips/src/benchmarking.rs +++ b/substrate/frame/tips/src/benchmarking.rs @@ -23,7 +23,7 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account, whitelisted_caller, impl_benchmark_test_suite}; -use sp_runtime::{traits::{Saturating}}; +use sp_runtime::traits::Saturating; use crate::Module as TipsMod; diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index cef50706b5..d69462c92d 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -68,16 +68,16 @@ use serde::{Serialize, Deserialize}; use sp_std::prelude::*; use frame_support::{decl_module, decl_storage, decl_event, ensure, print, decl_error}; use frame_support::traits::{ - Currency, Get, Imbalance, OnUnbalanced, ExistenceRequirement::{KeepAlive}, + Currency, Get, Imbalance, OnUnbalanced, ExistenceRequirement::KeepAlive, ReservableCurrency, WithdrawReasons }; use sp_runtime::{Permill, ModuleId, RuntimeDebug, traits::{ Zero, StaticLookup, AccountIdConversion, Saturating }}; use frame_support::weights::{Weight, DispatchClass}; -use frame_support::traits::{EnsureOrigin}; +use frame_support::traits::EnsureOrigin; use codec::{Encode, Decode}; -use frame_system::{ensure_signed}; +use frame_system::ensure_signed; pub use weights::WeightInfo; pub type BalanceOf = @@ -187,10 +187,7 @@ decl_storage! { let account_id = >::account_id(); let min = T::Currency::minimum_balance(); if T::Currency::free_balance(&account_id) < min { - let _ = T::Currency::make_free_balance_be( - &account_id, - min, - ); + let _ = T::Currency::make_free_balance_be(&account_id, min); } }); }