refactor: Transaction-Payment module (#3816)

* Initial draft that compiles

* Extract payment stuff from balances

* Extract multiplier update stuff from system

* Some fixes.

* Update len-fee as well

* some review comments.

* Remove todo

* bump

substrate/srml/balances/src/lib.rs

@@ -86,17 +86,6 @@
 //!
 //! - `vesting_balance` - Get the amount that is currently being vested and cannot be transferred out of this account.
 //!
-//! ### Signed Extensions
-//!
-//! The balances module defines the following extensions:
-//!
-//!   - [`TakeFees`]: Consumes fees proportional to the length and weight of the transaction.
-//!     Additionally, it can contain a single encoded payload as a `tip`. The inclusion priority
-//!     is increased proportional to the tip.
-//!
-//! Lookup the runtime aggregator file (e.g. `node/runtime`) to see the full list of signed
-//! extensions included in a chain.
-//!
 //! ## Usage
 //!
 //! The following examples show how to use the Balances module in your custom module.
@@ -171,15 +160,11 @@ use support::{
 	dispatch::Result,
 };
 use sr_primitives::{
-	transaction_validity::{
-		TransactionPriority, ValidTransaction, InvalidTransaction, TransactionValidityError,
-		TransactionValidity,
-	},
 	traits::{
 		Zero, SimpleArithmetic, StaticLookup, Member, CheckedAdd, CheckedSub, MaybeSerializeDebug,
-		Saturating, Bounded, SignedExtension, SaturatedConversion, Convert,
+		Saturating, Bounded,
 	},
-	weights::{DispatchInfo, SimpleDispatchInfo, Weight},
+	weights::SimpleDispatchInfo,
 };
 use system::{IsDeadAccount, OnNewAccount, ensure_signed, ensure_root};
 
@@ -210,15 +195,6 @@ pub trait Subtrait<I: Instance = DefaultInstance>: system::Trait {
 
 	/// The fee required to create an account.
 	type CreationFee: Get<Self::Balance>;
-
-	/// The fee to be paid for making a transaction; the base.
-	type TransactionBaseFee: Get<Self::Balance>;
-
-	/// The fee to be paid for making a transaction; the per-byte portion.
-	type TransactionByteFee: Get<Self::Balance>;
-
-	/// Convert a weight value into a deductible fee based on the currency type.
-	type WeightToFee: Convert<Weight, Self::Balance>;
 }
 
 pub trait Trait<I: Instance = DefaultInstance>: system::Trait {
@@ -235,9 +211,6 @@ pub trait Trait<I: Instance = DefaultInstance>: system::Trait {
 	/// Handler for when a new account is created.
 	type OnNewAccount: OnNewAccount<Self::AccountId>;
 
-	/// Handler for the unbalanced reduction when taking transaction fees.
-	type TransactionPayment: OnUnbalanced<NegativeImbalance<Self, I>>;
-
 	/// Handler for the unbalanced reduction when taking fees associated with balance
 	/// transfer (which may also include account creation).
 	type TransferPayment: OnUnbalanced<NegativeImbalance<Self, I>>;
@@ -256,15 +229,6 @@ pub trait Trait<I: Instance = DefaultInstance>: system::Trait {
 
 	/// The fee required to create an account.
 	type CreationFee: Get<Self::Balance>;
-
-	/// The fee to be paid for making a transaction; the base.
-	type TransactionBaseFee: Get<Self::Balance>;
-
-	/// The fee to be paid for making a transaction; the per-byte portion.
-	type TransactionByteFee: Get<Self::Balance>;
-
-	/// Convert a weight value into a deductible fee based on the currency type.
-	type WeightToFee: Convert<Weight, Self::Balance>;
 }
 
 impl<T: Trait<I>, I: Instance> Subtrait<I> for T {
@@ -274,9 +238,6 @@ impl<T: Trait<I>, I: Instance> Subtrait<I> for T {
 	type ExistentialDeposit = T::ExistentialDeposit;
 	type TransferFee = T::TransferFee;
 	type CreationFee = T::CreationFee;
-	type TransactionBaseFee = T::TransactionBaseFee;
-	type TransactionByteFee = T::TransactionByteFee;
-	type WeightToFee = T::WeightToFee;
 }
 
 decl_event!(
@@ -414,12 +375,6 @@ decl_module! {
 		/// The fee required to create an account.
 		const CreationFee: T::Balance = T::CreationFee::get();
 
-		/// The fee to be paid for making a transaction; the base.
-		const TransactionBaseFee: T::Balance = T::TransactionBaseFee::get();
-
-		/// The fee to be paid for making a transaction; the per-byte portion.
-		const TransactionByteFee: T::Balance = T::TransactionByteFee::get();
-
 		fn deposit_event() = default;
 
 		/// Transfer some liquid free balance to another account.
@@ -776,7 +731,7 @@ mod imbalances {
 // This works as long as `increase_total_issuance_by` doesn't use the Imbalance
 // types (basically for charging fees).
 // This should eventually be refactored so that the three type items that do
-// depend on the Imbalance type (TransactionPayment, TransferPayment, DustRemoval)
+// depend on the Imbalance type (TransferPayment, DustRemoval)
 // are placed in their own SRML module.
 struct ElevatedTrait<T: Subtrait<I>, I: Instance>(T, I);
 impl<T: Subtrait<I>, I: Instance> Clone for ElevatedTrait<T, I> {
@@ -796,7 +751,6 @@ impl<T: Subtrait<I>, I: Instance> system::Trait for ElevatedTrait<T, I> {
 	type AccountId = T::AccountId;
 	type Lookup = T::Lookup;
 	type Header = T::Header;
-	type WeightMultiplierUpdate = T::WeightMultiplierUpdate;
 	type Event = ();
 	type BlockHashCount = T::BlockHashCount;
 	type MaximumBlockWeight = T::MaximumBlockWeight;
@@ -809,15 +763,11 @@ impl<T: Subtrait<I>, I: Instance> Trait<I> for ElevatedTrait<T, I> {
 	type OnFreeBalanceZero = T::OnFreeBalanceZero;
 	type OnNewAccount = T::OnNewAccount;
 	type Event = ();
-	type TransactionPayment = ();
 	type TransferPayment = ();
 	type DustRemoval = ();
 	type ExistentialDeposit = T::ExistentialDeposit;
 	type TransferFee = T::TransferFee;
 	type CreationFee = T::CreationFee;
-	type TransactionBaseFee = T::TransactionBaseFee;
-	type TransactionByteFee = T::TransactionByteFee;
-	type WeightToFee = T::WeightToFee;
 }
 
 impl<T: Trait<I>, I: Instance> Currency<T::AccountId> for Module<T, I>
@@ -1194,91 +1144,6 @@ where
 	}
 }
 
-/// Require the transactor pay for themselves and maybe include a tip to gain additional priority
-/// in the queue.
-#[derive(Encode, Decode, Clone, Eq, PartialEq)]
-pub struct TakeFees<T: Trait<I>, I: Instance = DefaultInstance>(#[codec(compact)] T::Balance);
-
-impl<T: Trait<I>, I: Instance> TakeFees<T, I> {
-	/// utility constructor. Used only in client/factory code.
-	pub fn from(fee: T::Balance) -> Self {
-		Self(fee)
-	}
-
-	/// Compute the final fee value for a particular transaction.
-	///
-	/// The final fee is composed of:
-	///   - _length-fee_: This is the amount paid merely to pay for size of the transaction.
-	///   - _weight-fee_: This amount is computed based on the weight of the transaction. Unlike
-	///      size-fee, this is not input dependent and reflects the _complexity_ of the execution
-	///      and the time it consumes.
-	///   - (optional) _tip_: if included in the transaction, it will be added on top. Only signed
-	///      transactions can have a tip.
-	fn compute_fee(len: usize, info: DispatchInfo, tip: T::Balance) -> T::Balance {
-		let len_fee = if info.pay_length_fee() {
-			let len = T::Balance::from(len as u32);
-			let base = T::TransactionBaseFee::get();
-			let per_byte = T::TransactionByteFee::get();
-			base.saturating_add(per_byte.saturating_mul(len))
-		} else {
-			Zero::zero()
-		};
-
-		let weight_fee = {
-			// cap the weight to the maximum defined in runtime, otherwise it will be the `Bounded`
-			// maximum of its data type, which is not desired.
-			let capped_weight = info.weight.min(<T as system::Trait>::MaximumBlockWeight::get());
-			let weight_update = <system::Module<T>>::next_weight_multiplier();
-			let adjusted_weight = weight_update.apply_to(capped_weight);
-			T::WeightToFee::convert(adjusted_weight)
-		};
-
-		len_fee.saturating_add(weight_fee).saturating_add(tip)
-	}
-}
-
-#[cfg(feature = "std")]
-impl<T: Trait<I>, I: Instance> rstd::fmt::Debug for TakeFees<T, I> {
-	fn fmt(&self, f: &mut rstd::fmt::Formatter) -> rstd::fmt::Result {
-		self.0.fmt(f)
-	}
-}
-
-impl<T: Trait<I>, I: Instance + Clone + Eq> SignedExtension for TakeFees<T, I> {
-	type AccountId = T::AccountId;
-	type Call = T::Call;
-	type AdditionalSigned = ();
-	type Pre = ();
-	fn additional_signed(&self) -> rstd::result::Result<(), TransactionValidityError> { Ok(()) }
-
-	fn validate(
-		&self,
-		who: &Self::AccountId,
-		_call: &Self::Call,
-		info: DispatchInfo,
-		len: usize,
-	) -> TransactionValidity {
-		// pay any fees.
-		let fee = Self::compute_fee(len, info, self.0);
-		let imbalance = match <Module<T, I>>::withdraw(
-			who,
-			fee,
-			WithdrawReason::TransactionPayment,
-			ExistenceRequirement::KeepAlive,
-		) {
-			Ok(imbalance) => imbalance,
-			Err(_) => return InvalidTransaction::Payment.into(),
-		};
-		T::TransactionPayment::on_unbalanced(imbalance);
-
-		let mut r = ValidTransaction::default();
-		// NOTE: we probably want to maximize the _fee (of any type) per weight unit_ here, which
-		// will be a bit more than setting the priority to tip. For now, this is enough.
-		r.priority = fee.saturated_into::<TransactionPriority>();
-		Ok(r)
-	}
-}
-
 impl<T: Trait<I>, I: Instance> IsDeadAccount<T::AccountId> for Module<T, I>
 where
 	T::Balance: MaybeSerializeDebug

substrate/srml/balances/src/mock.rs

@@ -18,7 +18,7 @@
 
 #![cfg(test)]
 
-use sr_primitives::{Perbill, traits::{Convert, IdentityLookup}, testing::Header,
+use sr_primitives::{Perbill, traits::{ConvertInto, IdentityLookup}, testing::Header,
 	weights::{DispatchInfo, Weight}};
 use primitives::H256;
 use runtime_io;
@@ -35,10 +35,6 @@ thread_local! {
 	static EXISTENTIAL_DEPOSIT: RefCell<u64> = RefCell::new(0);
 	static TRANSFER_FEE: RefCell<u64> = RefCell::new(0);
 	static CREATION_FEE: RefCell<u64> = RefCell::new(0);
-	static TRANSACTION_BASE_FEE: RefCell<u64> = RefCell::new(0);
-	static TRANSACTION_BYTE_FEE: RefCell<u64> = RefCell::new(1);
-	static TRANSACTION_WEIGHT_FEE: RefCell<u64> = RefCell::new(1);
-	static WEIGHT_TO_FEE: RefCell<u64> = RefCell::new(1);
 }
 
 pub struct ExistentialDeposit;
@@ -56,23 +52,6 @@ impl Get<u64> for CreationFee {
 	fn get() -> u64 { CREATION_FEE.with(|v| *v.borrow()) }
 }
 
-pub struct TransactionBaseFee;
-impl Get<u64> for TransactionBaseFee {
-	fn get() -> u64 { TRANSACTION_BASE_FEE.with(|v| *v.borrow()) }
-}
-
-pub struct TransactionByteFee;
-impl Get<u64> for TransactionByteFee {
-	fn get() -> u64 { TRANSACTION_BYTE_FEE.with(|v| *v.borrow()) }
-}
-
-pub struct WeightToFee(u64);
-impl Convert<Weight, u64> for WeightToFee {
-	fn convert(t: Weight) -> u64 {
-		WEIGHT_TO_FEE.with(|v| *v.borrow() * (t as u64))
-	}
-}
-
 // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct Runtime;
@@ -92,7 +71,6 @@ impl system::Trait for Runtime {
 	type AccountId = u64;
 	type Lookup = IdentityLookup<Self::AccountId>;
 	type Header = Header;
-	type WeightMultiplierUpdate = ();
 	type Event = ();
 	type BlockHashCount = BlockHashCount;
 	type MaximumBlockWeight = MaximumBlockWeight;
@@ -100,26 +78,31 @@ impl system::Trait for Runtime {
 	type AvailableBlockRatio = AvailableBlockRatio;
 	type Version = ();
 }
+parameter_types! {
+	pub const TransactionBaseFee: u64 = 0;
+	pub const TransactionByteFee: u64 = 1;
+}
+impl transaction_payment::Trait for Runtime {
+	type Currency = Module<Runtime>;
+	type OnTransactionPayment = ();
+	type TransactionBaseFee = TransactionBaseFee;
+	type TransactionByteFee = TransactionByteFee;
+	type WeightToFee = ConvertInto;
+	type FeeMultiplierUpdate = ();
+}
 impl Trait for Runtime {
 	type Balance = u64;
 	type OnFreeBalanceZero = ();
 	type OnNewAccount = ();
 	type Event = ();
-	type TransactionPayment = ();
 	type DustRemoval = ();
 	type TransferPayment = ();
 	type ExistentialDeposit = ExistentialDeposit;
 	type TransferFee = TransferFee;
 	type CreationFee = CreationFee;
-	type TransactionBaseFee = TransactionBaseFee;
-	type TransactionByteFee = TransactionByteFee;
-	type WeightToFee = WeightToFee;
 }
 
 pub struct ExtBuilder {
-	transaction_base_fee: u64,
-	transaction_byte_fee: u64,
-	weight_to_fee: u64,
 	existential_deposit: u64,
 	transfer_fee: u64,
 	creation_fee: u64,
@@ -129,9 +112,6 @@ pub struct ExtBuilder {
 impl Default for ExtBuilder {
 	fn default() -> Self {
 		Self {
-			transaction_base_fee: 0,
-			transaction_byte_fee: 0,
-			weight_to_fee: 0,
 			existential_deposit: 0,
 			transfer_fee: 0,
 			creation_fee: 0,
@@ -141,12 +121,6 @@ impl Default for ExtBuilder {
 	}
 }
 impl ExtBuilder {
-	pub fn transaction_fees(mut self, base_fee: u64, byte_fee: u64, weight_fee: u64) -> Self {
-		self.transaction_base_fee = base_fee;
-		self.transaction_byte_fee = byte_fee;
-		self.weight_to_fee = weight_fee;
-		self
-	}
 	pub fn existential_deposit(mut self, existential_deposit: u64) -> Self {
 		self.existential_deposit = existential_deposit;
 		self
@@ -175,9 +149,6 @@ impl ExtBuilder {
 		EXISTENTIAL_DEPOSIT.with(|v| *v.borrow_mut() = self.existential_deposit);
 		TRANSFER_FEE.with(|v| *v.borrow_mut() = self.transfer_fee);
 		CREATION_FEE.with(|v| *v.borrow_mut() = self.creation_fee);
-		TRANSACTION_BASE_FEE.with(|v| *v.borrow_mut() = self.transaction_base_fee);
-		TRANSACTION_BYTE_FEE.with(|v| *v.borrow_mut() = self.transaction_byte_fee);
-		WEIGHT_TO_FEE.with(|v| *v.borrow_mut() = self.weight_to_fee);
 	}
 	pub fn build(self) -> runtime_io::TestExternalities {
 		self.set_associated_consts();
@@ -211,7 +182,6 @@ impl ExtBuilder {
 pub type System = system::Module<Runtime>;
 pub type Balances = Module<Runtime>;
 
-
 pub const CALL: &<Runtime as system::Trait>::Call = &();
 
 /// create a transaction info struct from weight. Handy to avoid building the whole struct.

substrate/srml/balances/src/tests.rs

@@ -20,12 +20,13 @@
 
 use super::*;
 use mock::{Balances, ExtBuilder, Runtime, System, info_from_weight, CALL};
+use sr_primitives::traits::SignedExtension;
 use support::{
 	assert_noop, assert_ok, assert_err,
 	traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons,
 	Currency, ReservableCurrency}
 };
-use sr_primitives::weights::DispatchClass;
+use transaction_payment::ChargeTransactionPayment;
 use system::RawOrigin;
 
 const ID_1: LockIdentifier = *b"1       ";
@@ -115,7 +116,6 @@ fn lock_reasons_should_work() {
 	ExtBuilder::default()
 		.existential_deposit(1)
 		.monied(true)
-		.transaction_fees(0, 1, 0)
 		.build()
 		.execute_with(|| {
 			Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::Transfer.into());
@@ -125,8 +125,8 @@ fn lock_reasons_should_work() {
 			);
 			assert_ok!(<Balances as ReservableCurrency<_>>::reserve(&1, 1));
 			// NOTE: this causes a fee payment.
-			assert!(<TakeFees<Runtime> as SignedExtension>::pre_dispatch(
-				TakeFees::from(1),
+			assert!(<ChargeTransactionPayment<Runtime> as SignedExtension>::pre_dispatch(
+				ChargeTransactionPayment::from(1),
 				&1,
 				CALL,
 				info_from_weight(1),
@@ -139,8 +139,8 @@ fn lock_reasons_should_work() {
 				<Balances as ReservableCurrency<_>>::reserve(&1, 1),
 				"account liquidity restrictions prevent withdrawal"
 			);
-			assert!(<TakeFees<Runtime> as SignedExtension>::pre_dispatch(
-				TakeFees::from(1),
+			assert!(<ChargeTransactionPayment<Runtime> as SignedExtension>::pre_dispatch(
+				ChargeTransactionPayment::from(1),
 				&1,
 				CALL,
 				info_from_weight(1),
@@ -150,8 +150,8 @@ fn lock_reasons_should_work() {
 			Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::TransactionPayment.into());
 			assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1));
 			assert_ok!(<Balances as ReservableCurrency<_>>::reserve(&1, 1));
-			assert!(<TakeFees<Runtime> as SignedExtension>::pre_dispatch(
-				TakeFees::from(1),
+			assert!(<ChargeTransactionPayment<Runtime> as SignedExtension>::pre_dispatch(
+				ChargeTransactionPayment::from(1),
 				&1,
 				CALL,
 				info_from_weight(1),
@@ -736,89 +736,6 @@ fn liquid_funds_should_transfer_with_delayed_vesting() {
 		});
 }
 
-#[test]
-fn signed_extension_take_fees_work() {
-	ExtBuilder::default()
-		.existential_deposit(10)
-		.transaction_fees(10, 1, 5)
-		.monied(true)
-		.build()
-		.execute_with(|| {
-			let len = 10;
-			assert!(
-				TakeFees::<Runtime>::from(0)
-					.pre_dispatch(&1, CALL, info_from_weight(5), len)
-					.is_ok()
-			);
-			assert_eq!(Balances::free_balance(&1), 100 - 20 - 25);
-			assert!(
-				TakeFees::<Runtime>::from(5 /* tipped */)
-					.pre_dispatch(&1, CALL, info_from_weight(3), len)
-					.is_ok()
-			);
-			assert_eq!(Balances::free_balance(&1), 100 - 20 - 25 - 20 - 5 - 15);
-		});
-}
-
-#[test]
-fn signed_extension_take_fees_is_bounded() {
-	ExtBuilder::default()
-		.existential_deposit(1000)
-		.transaction_fees(0, 0, 1)
-		.monied(true)
-		.build()
-		.execute_with(|| {
-			use sr_primitives::weights::Weight;
-
-			// maximum weight possible
-			assert!(
-				TakeFees::<Runtime>::from(0)
-					.pre_dispatch(&1, CALL, info_from_weight(Weight::max_value()), 10)
-					.is_ok()
-			);
-			// fee will be proportional to what is the actual maximum weight in the runtime.
-			assert_eq!(
-				Balances::free_balance(&1),
-				(10000 - <Runtime as system::Trait>::MaximumBlockWeight::get()) as u64
-			);
-		});
-}
-
-#[test]
-fn signed_extension_allows_free_transactions() {
-	ExtBuilder::default()
-		.transaction_fees(100, 1, 1)
-		.monied(false)
-		.build()
-		.execute_with(|| {
-			// 1 ain't have a penny.
-			assert_eq!(Balances::free_balance(&1), 0);
-
-			// like a FreeOperational
-			let operational_transaction = DispatchInfo {
-				weight: 0,
-				class: DispatchClass::Operational
-			};
-			let len = 100;
-			assert!(
-				TakeFees::<Runtime>::from(0)
-					.validate(&1, CALL, operational_transaction , len)
-					.is_ok()
-			);
-
-			// like a FreeNormal
-			let free_transaction = DispatchInfo {
-				weight: 0,
-				class: DispatchClass::Normal
-			};
-			assert!(
-				TakeFees::<Runtime>::from(0)
-					.validate(&1, CALL, free_transaction , len)
-					.is_err()
-			);
-		});
-}
-
 #[test]
 fn burn_must_work() {
 	ExtBuilder::default().monied(true).build().execute_with(|| {