Introduce safe types for handling imbalances (#2048)

* Be a little safer with total issuance.

* PairT instead of _Pair

* Remove rev causing upset

* Remove fees stuff.

* Fix build (including tests)

* Update runtime, bump version

* Fix

* Handle gas refunds properly.

* Rename identifier

ala #2025

* Address grumbles

* New not-quite-linear-typing API

* Slimmer API

* More linear-type test fixes

* Fix tests

* Tidy

* Fix some grumbles

* Keep unchecked functions private

* Remove another less-than-safe currency function and ensure that
contracts module can never create cash.

* Address a few grumbles and fix tests

substrate/srml/contract/src/account_db.rs

@@ -21,7 +21,9 @@ use {balances, system};
 use rstd::cell::RefCell;
 use rstd::collections::btree_map::{BTreeMap, Entry};
 use rstd::prelude::*;
-use srml_support::{StorageMap, StorageDoubleMap, traits::UpdateBalanceOutcome};
+use runtime_primitives::traits::Zero;
+use srml_support::{StorageMap, StorageDoubleMap, traits::{UpdateBalanceOutcome,
+	SignedImbalance, Currency, Imbalance}};
 
 pub struct ChangeEntry<T: Trait> {
 	balance: Option<T::Balance>,
@@ -63,11 +65,12 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 		balances::Module::<T>::free_balance(account)
 	}
 	fn commit(&mut self, s: ChangeSet<T>) {
+		let mut total_imbalance = SignedImbalance::zero();
 		for (address, changed) in s.into_iter() {
 			if let Some(balance) = changed.balance {
-				if let UpdateBalanceOutcome::AccountKilled =
-					balances::Module::<T>::set_free_balance_creating(&address, balance)
-				{
+				let (imbalance, outcome) = balances::Module::<T>::ensure_free_balance_is(&address, balance);
+				total_imbalance = total_imbalance.merge(imbalance);
+				if let UpdateBalanceOutcome::AccountKilled = outcome {
 					// Account killed. This will ultimately lead to calling `OnFreeBalanceZero` callback
 					// which will make removal of CodeHashOf and StorageOf for this account.
 					// In order to avoid writing over the deleted properties we `continue` here.
@@ -89,6 +92,16 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 				}
 			}
 		}
+
+		match total_imbalance {
+			// If we've detected a positive imbalance as a result of our contract-level machinations
+			// then it's indicative of a buggy contracts system.
+			// Panicking is far from ideal as it opens up a DoS attack on block validators, however
+			// it's a less bad option than allowing arbitrary value to be created.
+			SignedImbalance::Positive(ref p) if !p.peek().is_zero() =>
+				panic!("contract subsystem resulting in positive imbalance!"),
+			_ => {}
+		}
 	}
 }
 

substrate/srml/contract/src/exec.rs

@@ -20,7 +20,7 @@ use crate::gas::{GasMeter, Token, approx_gas_for_balance};
 
 use rstd::prelude::*;
 use runtime_primitives::traits::{CheckedAdd, CheckedSub, Zero};
-use srml_support::traits::WithdrawReason;
+use srml_support::traits::{WithdrawReason, Currency};
 use timestamp;
 
 pub type BalanceOf<T> = <T as balances::Trait>::Balance;
@@ -520,7 +520,7 @@ fn transfer<'a, T: Trait, V: Vm<T>, L: Loader<T>>(
 	if would_create && value < ctx.config.existential_deposit {
 		return Err("value too low to create account");
 	}
-	<balances::Module<T>>::ensure_account_can_withdraw(transactor, value, WithdrawReason::Transfer, new_from_balance)?;
+	<balances::Module<T>>::ensure_can_withdraw(transactor, value, WithdrawReason::Transfer, new_from_balance)?;
 
 	let new_to_balance = match to_balance.checked_add(&value) {
 		Some(b) => b,

substrate/srml/contract/src/gas.rs

@@ -18,7 +18,7 @@ use crate::{GasSpent, Module, Trait};
 use balances;
 use runtime_primitives::BLOCK_FULL;
 use runtime_primitives::traits::{As, CheckedMul, CheckedSub, Zero};
-use srml_support::StorageValue;
+use srml_support::{StorageValue, traits::{OnUnbalanced, ExistenceRequirement, WithdrawReason, Currency, Imbalance}};
 
 #[cfg(test)]
 use std::{any::Any, fmt::Debug};
@@ -202,7 +202,7 @@ impl<T: Trait> GasMeter<T> {
 pub fn buy_gas<T: Trait>(
 	transactor: &T::AccountId,
 	gas_limit: T::Gas,
-) -> Result<GasMeter<T>, &'static str> {
+) -> Result<(GasMeter<T>, balances::NegativeImbalance<T>), &'static str> {
 	// Check if the specified amount of gas is available in the current block.
 	// This cannot underflow since `gas_spent` is never greater than `block_gas_limit`.
 	let gas_available = <Module<T>>::block_gas_limit() - <Module<T>>::gas_spent();
@@ -213,40 +213,47 @@ pub fn buy_gas<T: Trait>(
 
 	// Buy the specified amount of gas.
 	let gas_price = <Module<T>>::gas_price();
-	let b = <balances::Module<T>>::free_balance(transactor);
 	let cost = <T::Gas as As<T::Balance>>::as_(gas_limit.clone())
 		.checked_mul(&gas_price)
 		.ok_or("overflow multiplying gas limit by price")?;
 
-	let new_balance = b.checked_sub(&cost);
-	if new_balance < Some(<balances::Module<T>>::existential_deposit()) {
-		return Err("not enough funds for transaction fee");
-	}
+	let imbalance = <balances::Module<T>>::withdraw(
+		transactor,
+		cost,
+		WithdrawReason::Fee,
+		ExistenceRequirement::KeepAlive
+	)?;
 
-	<balances::Module<T>>::set_free_balance(transactor, b - cost);
-	<balances::Module<T>>::decrease_total_stake_by(cost);
-	Ok(GasMeter {
+	Ok((GasMeter {
 		limit: gas_limit,
 		gas_left: gas_limit,
 		gas_price,
+
 		#[cfg(test)]
 		tokens: Vec::new(),
-	})
+	}, imbalance))
 }
 
 /// Refund the unused gas.
-pub fn refund_unused_gas<T: Trait>(transactor: &T::AccountId, gas_meter: GasMeter<T>) {
+pub fn refund_unused_gas<T: Trait>(
+	transactor: &T::AccountId,
+	gas_meter: GasMeter<T>,
+	imbalance: balances::NegativeImbalance<T>,
+) {
+	let gas_spent = gas_meter.spent();
+	let gas_left = gas_meter.gas_left();
+
 	// Increase total spent gas.
 	// This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which
 	// also has T::Gas type.
-	let gas_spent = <Module<T>>::gas_spent() + gas_meter.spent();
-	<GasSpent<T>>::put(gas_spent);
+	<GasSpent<T>>::mutate(|block_gas_spent| *block_gas_spent += gas_spent);
 
 	// Refund gas left by the price it was bought.
-	let b = <balances::Module<T>>::free_balance(transactor);
-	let refund = <T::Gas as As<T::Balance>>::as_(gas_meter.gas_left) * gas_meter.gas_price;
-	<balances::Module<T>>::set_free_balance(transactor, b + refund);
-	<balances::Module<T>>::increase_total_stake_by(refund);
+	let refund = <T::Gas as As<T::Balance>>::as_(gas_left) * gas_meter.gas_price;
+	let refund_imbalance = <balances::Module<T>>::deposit_creating(transactor, refund);
+	if let Ok(imbalance) = imbalance.offset(refund_imbalance) {
+		T::GasPayment::on_unbalanced(imbalance);
+	}
 }
 
 /// A little handy utility for converting a value in balance units into approximitate value in gas units

substrate/srml/contract/src/lib.rs

@@ -74,11 +74,10 @@ use parity_codec::{Codec, Encode, Decode};
 use runtime_primitives::traits::{Hash, As, SimpleArithmetic,Bounded, StaticLookup};
 use srml_support::dispatch::{Result, Dispatchable};
 use srml_support::{Parameter, StorageMap, StorageValue, StorageDoubleMap, decl_module, decl_event, decl_storage};
-use srml_support::traits::OnFreeBalanceZero;
+use srml_support::traits::{OnFreeBalanceZero, OnUnbalanced};
 use system::{ensure_signed, RawOrigin};
 use runtime_io::{blake2_256, twox_128};
 use timestamp;
-use fees;
 
 pub type CodeHash<T> = <T as system::Trait>::Hash;
 
@@ -92,7 +91,7 @@ pub trait ComputeDispatchFee<Call, Balance> {
 	fn compute_dispatch_fee(call: &Call) -> Balance;
 }
 
-pub trait Trait: fees::Trait + balances::Trait + timestamp::Trait {
+pub trait Trait: balances::Trait + timestamp::Trait {
 	/// The outer call dispatch type.
 	type Call: Parameter + Dispatchable<Origin=<Self as system::Trait>::Origin>;
 
@@ -110,6 +109,9 @@ pub trait Trait: fees::Trait + balances::Trait + timestamp::Trait {
 	/// It is recommended (though not required) for this function to return a fee that would be taken
 	/// by executive module for regular dispatch.
 	type ComputeDispatchFee: ComputeDispatchFee<Self::Call, <Self as balances::Trait>::Balance>;
+
+	/// Handler for the unbalanced reduction when making a gas payment.
+	type GasPayment: OnUnbalanced<balances::NegativeImbalance<Self>>;
 }
 
 /// Simple contract address determintator.
@@ -136,14 +138,14 @@ where
 }
 
 /// The default dispatch fee computor computes the fee in the same way that
-/// implementation of `ChargeBytesFee` for fees module does.
+/// implementation of `MakePayment` for balances module does.
 pub struct DefaultDispatchFeeComputor<T: Trait>(PhantomData<T>);
 impl<T: Trait> ComputeDispatchFee<T::Call, T::Balance> for DefaultDispatchFeeComputor<T> {
 	fn compute_dispatch_fee(call: &T::Call) -> T::Balance {
 		let encoded_len = call.using_encoded(|encoded| encoded.len());
-		let base_fee = <fees::Module<T>>::transaction_base_fee();
-		let byte_fee = <fees::Module<T>>::transaction_byte_fee();
-		<T::Balance as As<u64>>::sa(base_fee.as_() + byte_fee.as_() * encoded_len as u64)
+		let base_fee = <balances::Module<T>>::transaction_base_fee();
+		let byte_fee = <balances::Module<T>>::transaction_byte_fee();
+		base_fee + byte_fee * <T::Balance as As<u64>>::sa(encoded_len as u64)
 	}
 }
 
@@ -175,14 +177,14 @@ decl_module! {
 			let origin = ensure_signed(origin)?;
 			let schedule = <Module<T>>::current_schedule();
 
-			let mut gas_meter = gas::buy_gas::<T>(&origin, gas_limit)?;
+			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
 
 			let result = wasm::save_code::<T>(code, &mut gas_meter, &schedule);
 			if let Ok(code_hash) = result {
 				Self::deposit_event(RawEvent::CodeStored(code_hash));
 			}
 
-			gas::refund_unused_gas::<T>(&origin, gas_meter);
+			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
 
 			result.map(|_| ())
 		}
@@ -202,7 +204,7 @@ decl_module! {
 			//
 			// NOTE: it is very important to avoid any state changes before
 			// paying for the gas.
-			let mut gas_meter = gas::buy_gas::<T>(&origin, gas_limit)?;
+			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
 
 			let cfg = Config::preload();
 			let vm = crate::wasm::WasmVm::new(&cfg.schedule);
@@ -212,7 +214,7 @@ decl_module! {
 			let result = ctx.call(dest, value, &mut gas_meter, &data, exec::EmptyOutputBuf::new());
 
 			if let Ok(_) = result {
-				// Commit all changes that made it thus far into the persistant storage.
+				// Commit all changes that made it thus far into the persistent storage.
 				account_db::DirectAccountDb.commit(ctx.overlay.into_change_set());
 
 				// Then deposit all events produced.
@@ -223,7 +225,7 @@ decl_module! {
 			//
 			// NOTE: this should go after the commit to the storage, since the storage changes
 			// can alter the balance of the caller.
-			gas::refund_unused_gas::<T>(&origin, gas_meter);
+			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
 
 			// Dispatch every recorded call with an appropriate origin.
 			ctx.calls.into_iter().for_each(|(who, call)| {
@@ -234,7 +236,7 @@ decl_module! {
 			result.map(|_| ())
 		}
 
-		/// Create a new contract, optionally transfering some balance to the created account.
+		/// Create a new contract, optionally transferring some balance to the created account.
 		///
 		/// Creation is executed as follows:
 		///
@@ -256,7 +258,7 @@ decl_module! {
 			//
 			// NOTE: it is very important to avoid any state changes before
 			// paying for the gas.
-			let mut gas_meter = gas::buy_gas::<T>(&origin, gas_limit)?;
+			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
 
 			let cfg = Config::preload();
 			let vm = crate::wasm::WasmVm::new(&cfg.schedule);
@@ -276,7 +278,7 @@ decl_module! {
 			//
 			// NOTE: this should go after the commit to the storage, since the storage changes
 			// can alter the balance of the caller.
-			gas::refund_unused_gas::<T>(&origin, gas_meter);
+			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
 
 			// Dispatch every recorded call with an appropriate origin.
 			ctx.calls.into_iter().for_each(|(who, call)| {

substrate/srml/contract/src/tests.rs

@@ -24,10 +24,10 @@ use runtime_primitives::testing::{Digest, DigestItem, H256, Header, UintAuthorit
 use runtime_primitives::traits::{BlakeTwo256, IdentityLookup};
 use runtime_primitives::BuildStorage;
 use runtime_io;
-use srml_support::{StorageMap, StorageDoubleMap, assert_ok, impl_outer_event, impl_outer_dispatch, impl_outer_origin};
-use substrate_primitives::{Blake2Hasher};
+use srml_support::{StorageMap, StorageDoubleMap, assert_ok, impl_outer_event, impl_outer_dispatch,
+	impl_outer_origin, traits::Currency};
+use substrate_primitives::Blake2Hasher;
 use system::{self, Phase, EventRecord};
-use fees;
 use {wabt, balances, consensus};
 use hex_literal::*;
 use assert_matches::assert_matches;
@@ -45,7 +45,7 @@ mod contract {
 }
 impl_outer_event! {
 	pub enum MetaEvent for Test {
-		balances<T>, contract<T>, fees<T>,
+		balances<T>, contract<T>,
 	}
 }
 impl_outer_origin! {
@@ -78,6 +78,9 @@ impl balances::Trait for Test {
 	type OnFreeBalanceZero = Contract;
 	type OnNewAccount = ();
 	type Event = MetaEvent;
+	type TransactionPayment = ();
+	type DustRemoval = ();
+	type TransferPayment = ();
 }
 impl timestamp::Trait for Test {
 	type Moment = u64;
@@ -88,16 +91,13 @@ impl consensus::Trait for Test {
 	type SessionKey = UintAuthorityId;
 	type InherentOfflineReport = ();
 }
-impl fees::Trait for Test {
-	type Event = MetaEvent;
-	type TransferAsset = Balances;
-}
 impl Trait for Test {
 	type Call = Call;
 	type Gas = u64;
 	type DetermineContractAddress = DummyContractAddressFor;
 	type Event = MetaEvent;
 	type ComputeDispatchFee = DummyComputeDispatchFee;
+	type GasPayment = ();
 }
 
 type Balances = balances::Module<Test>;
@@ -168,6 +168,8 @@ impl ExtBuilder {
 			.0;
 		t.extend(
 			balances::GenesisConfig::<Test> {
+				transaction_base_fee: 0,
+				transaction_byte_fee: 0,
 				balances: vec![],
 				existential_deposit: self.existential_deposit,
 				transfer_fee: self.transfer_fee,
@@ -199,8 +201,7 @@ impl ExtBuilder {
 #[test]
 fn refunds_unused_gas() {
 	with_externalities(&mut ExtBuilder::default().build(), || {
-		Balances::set_free_balance(&0, 100_000_000);
-		Balances::increase_total_stake_by(100_000_000);
+		Balances::deposit_creating(&0, 100_000_000);
 
 		assert_ok!(Contract::call(
 			Origin::signed(0),
@@ -221,13 +222,11 @@ fn account_removal_removes_storage() {
 		|| {
 			// Setup two accounts with free balance above than exsistential threshold.
 			{
-				Balances::set_free_balance(&1, 110);
-				Balances::increase_total_stake_by(110);
+				Balances::deposit_creating(&1, 110);
 				<StorageOf<Test>>::insert(&1, &b"foo".to_vec(), b"1".to_vec());
 				<StorageOf<Test>>::insert(&1, &b"bar".to_vec(), b"2".to_vec());
 
-				Balances::set_free_balance(&2, 110);
-				Balances::increase_total_stake_by(110);
+				Balances::deposit_creating(&2, 110);
 				<StorageOf<Test>>::insert(&2, &b"hello".to_vec(), b"3".to_vec());
 				<StorageOf<Test>>::insert(&2, &b"world".to_vec(), b"4".to_vec());
 			}
@@ -288,8 +287,7 @@ fn instantiate_and_call() {
 	with_externalities(
 		&mut ExtBuilder::default().existential_deposit(100).build(),
 		|| {
-			Balances::set_free_balance(&ALICE, 1_000_000);
-			Balances::increase_total_stake_by(1_000_000);
+			Balances::deposit_creating(&ALICE, 1_000_000);
 
 			assert_ok!(Contract::put_code(
 				Origin::signed(ALICE),
@@ -306,6 +304,10 @@ fn instantiate_and_call() {
 			));
 
 			assert_eq!(System::events(), vec![
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(0),
+					event: MetaEvent::balances(balances::RawEvent::NewAccount(1, 1_000_000)),
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(0),
 					event: MetaEvent::contract(RawEvent::CodeStored(HASH_RETURN_FROM_START_FN.into())),
@@ -359,8 +361,7 @@ fn dispatch_call() {
 	with_externalities(
 		&mut ExtBuilder::default().existential_deposit(50).build(),
 		|| {
-			Balances::set_free_balance(&ALICE, 1_000_000);
-			Balances::increase_total_stake_by(1_000_000);
+			Balances::deposit_creating(&ALICE, 1_000_000);
 
 			assert_ok!(Contract::put_code(
 				Origin::signed(ALICE),
@@ -371,6 +372,10 @@ fn dispatch_call() {
 			// Let's keep this assert even though it's redundant. If you ever need to update the
 			// wasm source this test will fail and will show you the actual hash.
 			assert_eq!(System::events(), vec![
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(0),
+					event: MetaEvent::balances(balances::RawEvent::NewAccount(1, 1_000_000)),
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(0),
 					event: MetaEvent::contract(RawEvent::CodeStored(HASH_DISPATCH_CALL.into())),
@@ -394,6 +399,10 @@ fn dispatch_call() {
 			));
 
 			assert_eq!(System::events(), vec![
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(0),
+					event: MetaEvent::balances(balances::RawEvent::NewAccount(1, 1_000_000)),
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(0),
 					event: MetaEvent::contract(RawEvent::CodeStored(HASH_DISPATCH_CALL.into())),