Fees module refactor

node/executor/src/lib.rs

@@ -499,6 +499,10 @@ mod tests {
 					phase: Phase::ApplyExtrinsic(0),
 					event: Event::system(system::Event::ExtrinsicSuccess)
 				},
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(1),
+					event: Event::fees(fees::RawEvent::TransactionPayment(alice().into(), 1))
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(1),
 					event: Event::balances(balances::RawEvent::Transfer(
@@ -524,10 +528,6 @@ mod tests {
 					phase: Phase::Finalization,
 					event: Event::treasury(treasury::RawEvent::Rollover(0))
 				},
-				EventRecord {
-					phase: Phase::Finalization,
-					event: Event::fees(fees::RawEvent::Charged(1, 1))
-				}
 			]);
 		});
 
@@ -550,6 +550,10 @@ mod tests {
 					phase: Phase::ApplyExtrinsic(0),
 					event: Event::system(system::Event::ExtrinsicSuccess)
 				},
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(1),
+					event: Event::fees(fees::RawEvent::TransactionPayment(bob().into(), 1))
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(1),
 					event: Event::balances(
@@ -565,6 +569,10 @@ mod tests {
 					phase: Phase::ApplyExtrinsic(1),
 					event: Event::system(system::Event::ExtrinsicSuccess)
 				},
+				EventRecord {
+					phase: Phase::ApplyExtrinsic(2),
+					event: Event::fees(fees::RawEvent::TransactionPayment(alice().into(), 1))
+				},
 				EventRecord {
 					phase: Phase::ApplyExtrinsic(2),
 					event: Event::balances(
@@ -604,14 +612,6 @@ mod tests {
 					phase: Phase::Finalization,
 					event: Event::treasury(treasury::RawEvent::Rollover(0))
 				},
-				EventRecord {
-					phase: Phase::Finalization,
-					event: Event::fees(fees::RawEvent::Charged(1, 1))
-				},
-				EventRecord {
-					phase: Phase::Finalization,
-					event: Event::fees(fees::RawEvent::Charged(2, 1))
-				}
 			]);
 		});
 	}

node/runtime/src/lib.rs

@@ -58,7 +58,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 35,
+	spec_version: 36,
 	impl_version: 38,
 	apis: RUNTIME_API_VERSIONS,
 };

srml/executive/src/lib.rs

@@ -25,7 +25,7 @@ use primitives::traits::{
 	self, Header, Zero, One, Checkable, Applyable, CheckEqual, OnFinalise,
 	OnInitialise, Hash, As, Digest, NumberFor, Block as BlockT
 };
-use srml_support::{Dispatchable, traits::ChargeBytesFee};
+use srml_support::{Dispatchable, traits::MakeTransactionPayment};
 use parity_codec::{Codec, Encode};
 use system::extrinsics_root;
 use primitives::{ApplyOutcome, ApplyError};
@@ -64,7 +64,7 @@ impl<
 	System: system::Trait,
 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
 	Context: Default,
-	Payment: ChargeBytesFee<System::AccountId>,
+	Payment: MakeTransactionPayment<System::AccountId>,
 	AllModules: OnInitialise<System::BlockNumber> + OnFinalise<System::BlockNumber>,
 > ExecuteBlock<Block> for Executive<System, Block, Context, Payment, AllModules> where
 	Block::Extrinsic: Checkable<Context> + Codec,
@@ -85,7 +85,7 @@ impl<
 	System: system::Trait,
 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
 	Context: Default,
-	Payment: ChargeBytesFee<System::AccountId>,
+	Payment: MakeTransactionPayment<System::AccountId>,
 	AllModules: OnInitialise<System::BlockNumber> + OnFinalise<System::BlockNumber>,
 > Executive<System, Block, Context, Payment, AllModules> where
 	Block::Extrinsic: Checkable<Context> + Codec,
@@ -214,7 +214,7 @@ impl<
 			) }
 
 			// pay any fees.
-			Payment::charge_base_bytes_fee(sender, encoded_len).map_err(|_| internal::ApplyError::CantPay)?;
+			Payment::make_transaction_payment(sender, encoded_len).map_err(|_| internal::ApplyError::CantPay)?;
 
 			// AUDIT: Under no circumstances may this function panic from here onwards.
 
@@ -286,7 +286,7 @@ impl<
 
 		if let (Some(sender), Some(index)) = (xt.sender(), xt.index()) {
 			// pay any fees.
-			if Payment::charge_base_bytes_fee(sender, encoded_len).is_err() {
+			if Payment::make_transaction_payment(sender, encoded_len).is_err() {
 				return TransactionValidity::Invalid(ApplyError::CantPay as i8)
 			}
 

srml/fees/Cargo.toml

@@ -16,6 +16,9 @@ runtime_primitives = { package = "sr-primitives", path = "../../core/sr-primitiv
 srml-support = { package = "srml-support", path = "../support", default-features = false }
 system = { package = "srml-system", path = "../system", default-features = false }
 
+[dev-dependencies]
+balances = { package = "srml-balances", path = "../balances" }
+
 [features]
 default = ["std"]
 std = [

srml/fees/src/lib.rs

@@ -20,11 +20,11 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use srml_support::{
-	dispatch::Result, StorageMap, decl_event, decl_storage, decl_module,
-	traits::{ArithmeticType, ChargeBytesFee, ChargeFee, TransferAsset, WithdrawReason}
+	dispatch::Result, decl_event, decl_storage, decl_module,
+	traits::{ArithmeticType, MakeTransactionPayment, TransferAsset, WithdrawReason}
 };
 use runtime_primitives::traits::{
-	As, CheckedAdd, CheckedSub, CheckedMul, Zero
+	As, CheckedAdd, CheckedMul
 };
 use system;
 
@@ -44,74 +44,35 @@ pub trait Trait: system::Trait {
 decl_module! {
 	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
 		fn deposit_event<T>() = default;
-
-		fn on_finalise() {
-			let extrinsic_count = <system::Module<T>>::extrinsic_count();
-			(0..extrinsic_count).for_each(|index| {
-				// Deposit `Charged` event if some amount of fee charged.
-				let fee = <CurrentTransactionFee<T>>::take(index);
-				if !fee.is_zero() {
-					Self::deposit_event(RawEvent::Charged(index, fee));
-				}
-			});
-		}
 	}
 }
 
-decl_event!(
-	pub enum Event<T> where Amount = AssetOf<T> {
-		/// Fee charged (extrinsic_index, fee_amount)
-		Charged(u32, Amount),
-	}
-);
-
 decl_storage! {
 	trait Store for Module<T: Trait> as Fees {
 		/// The fee to be paid for making a transaction; the base.
 		pub TransactionBaseFee get(transaction_base_fee) config(): AssetOf<T>;
 		/// The fee to be paid for making a transaction; the per-byte portion.
 		pub TransactionByteFee get(transaction_byte_fee) config(): AssetOf<T>;
-
-		/// The `extrinsic_index => accumulated_fees` map, containing records to
-		/// track the overall charged fees for each transaction.
-		///
-		/// All records should be removed at finalise stage.
-		CurrentTransactionFee get(current_transaction_fee): map u32 => AssetOf<T>;
 	}
 }
 
-impl<T: Trait> ChargeBytesFee<T::AccountId> for Module<T> {
-	fn charge_base_bytes_fee(transactor: &T::AccountId, encoded_len: usize) -> Result {
+decl_event!(
+	pub enum Event<T> where <T as system::Trait>::AccountId, Amount = AssetOf<T> {
+		/// Transaction payment charged (transactor, fee_amount)
+		TransactionPayment(AccountId, Amount),
+	}
+);
+
+impl<T: Trait> MakeTransactionPayment<T::AccountId> for Module<T> {
+	fn make_transaction_payment(transactor: &T::AccountId, encoded_len: usize) -> Result {
 		let bytes_fee = Self::transaction_byte_fee().checked_mul(
 			&<AssetOf<T> as As<u64>>::sa(encoded_len as u64)
 		).ok_or_else(|| "bytes fee overflow")?;
 		let overall = Self::transaction_base_fee().checked_add(&bytes_fee).ok_or_else(|| "bytes fee overflow")?;
-		Self::charge_fee(transactor, overall)
-	}
-}
-
-impl<T: Trait> ChargeFee<T::AccountId> for Module<T> {
-	type Amount = AssetOf<T>;
-
-	fn charge_fee(transactor: &T::AccountId, amount: AssetOf<T>) -> Result {
-		let extrinsic_index = <system::Module<T>>::extrinsic_index().ok_or_else(|| "no extrinsic index found")?;
-		let current_fee = Self::current_transaction_fee(extrinsic_index);
-		let new_fee = current_fee.checked_add(&amount).ok_or_else(|| "fee got overflow after charge")?;
-
-		T::TransferAsset::withdraw(transactor, amount, WithdrawReason::TransactionPayment)?;
-
-		<CurrentTransactionFee<T>>::insert(extrinsic_index, new_fee);
-		Ok(())
-	}
-
-	fn refund_fee(transactor: &T::AccountId, amount: AssetOf<T>) -> Result {
-		let extrinsic_index = <system::Module<T>>::extrinsic_index().ok_or_else(|| "no extrinsic index found")?;
-		let current_fee = Self::current_transaction_fee(extrinsic_index);
-		let new_fee = current_fee.checked_sub(&amount).ok_or_else(|| "fee got underflow after refund")?;
 
-		T::TransferAsset::deposit(transactor, amount)?;
+		T::TransferAsset::withdraw(transactor, overall, WithdrawReason::TransactionPayment)?;
+		Self::deposit_event(RawEvent::TransactionPayment(transactor.clone(), overall));
 
-		<CurrentTransactionFee<T>>::insert(extrinsic_index, new_fee);
 		Ok(())
 	}
 }

srml/fees/src/mock.rs

@@ -27,7 +27,6 @@ use primitives::{H256, Blake2Hasher};
 use runtime_io;
 use srml_support::{
 	impl_outer_origin, impl_outer_event,
-	traits::{ArithmeticType, TransferAsset, WithdrawReason}
 };
 use crate::{GenesisConfig, Module, Trait, system};
 
@@ -41,24 +40,10 @@ mod fees {
 
 impl_outer_event!{
 	pub enum TestEvent for Test {
-		fees<T>,
+		balances<T>, fees<T>,
 	}
 }
 
-pub struct TransferAssetMock;
-
-impl<AccountId> TransferAsset<AccountId> for TransferAssetMock {
-	type Amount = u64;
-
-	fn transfer(_: &AccountId, _: &AccountId, _: Self::Amount) -> Result<(), &'static str> { Ok(()) }
-	fn withdraw(_: &AccountId, _: Self::Amount, _: WithdrawReason) -> Result<(), &'static str> { Ok(()) }
-	fn deposit(_: &AccountId, _: Self::Amount) -> Result<(), &'static str> { Ok(()) }
-}
-
-impl ArithmeticType for TransferAssetMock {
-	type Type = u64;
-}
-
 // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct Test;
@@ -75,13 +60,20 @@ impl system::Trait for Test {
 	type Event = TestEvent;
 	type Log = DigestItem;
 }
+impl balances::Trait for Test {
+	type Balance = u64;
+	type OnFreeBalanceZero = ();
+	type OnNewAccount = ();
+	type Event = TestEvent;
+}
 impl Trait for Test {
 	type Event = TestEvent;
-	type TransferAsset = TransferAssetMock;
+	type TransferAsset = Balances;
 }
 
 pub type System = system::Module<Test>;
 pub type Fees = Module<Test>;
+pub type Balances = balances::Module<Test>;
 
 pub struct ExtBuilder {
 	transaction_base_fee: u64,
@@ -106,6 +98,13 @@ impl ExtBuilder {
 	}
 	pub fn build(self) -> runtime_io::TestExternalities<Blake2Hasher> {
 		let mut t = system::GenesisConfig::<Test>::default().build_storage().unwrap().0;
+		t.extend(balances::GenesisConfig::<Test>{
+			balances: vec![(0, 1000)],
+			existential_deposit: 0,
+			transfer_fee: 0,
+			creation_fee: 0,
+			vesting: vec![],
+		}.build_storage().unwrap().0);
 		t.extend(GenesisConfig::<Test> {
 			transaction_base_fee: self.transaction_base_fee,
 			transaction_byte_fee: self.transaction_byte_fee,