diff --git a/Cargo.lock b/Cargo.lock index 08829f4ae2bf2..a1b2961e693ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3235,6 +3235,7 @@ dependencies = [ "sr-io 0.1.0", "sr-primitives 0.1.0", "sr-std 0.1.0", + "srml-balances 0.1.0", "srml-support 0.1.0", "srml-system 0.1.0", "substrate-primitives 0.1.0", diff --git a/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm b/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm index 82ede8590330a..70fc361c0eacb 100644 Binary files a/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm and b/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm differ diff --git a/node/executor/src/lib.rs b/node/executor/src/lib.rs index 84b2de336ed58..01e37999aa55d 100644 --- a/node/executor/src/lib.rs +++ b/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)) - } ]); }); } diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 7ca5c697f6fcb..c8a1e7ede8273 100644 --- a/node/runtime/src/lib.rs +++ b/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, }; diff --git a/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm b/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm index f6ba616469aea..ea7e55a2ec35e 100644 Binary files a/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm and b/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm differ diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index b6cc498f79dac..f8440816b4b21 100644 --- a/srml/executive/src/lib.rs +++ b/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, Context: Default, - Payment: ChargeBytesFee, + Payment: MakeTransactionPayment, AllModules: OnInitialise + OnFinalise, > ExecuteBlock for Executive where Block::Extrinsic: Checkable + Codec, @@ -85,7 +85,7 @@ impl< System: system::Trait, Block: traits::Block, Context: Default, - Payment: ChargeBytesFee, + Payment: MakeTransactionPayment, AllModules: OnInitialise + OnFinalise, > Executive where Block::Extrinsic: Checkable + 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) } diff --git a/srml/fees/Cargo.toml b/srml/fees/Cargo.toml index 2bb4474ce6e8d..ec7f12c360a18 100644 --- a/srml/fees/Cargo.toml +++ b/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 = [ diff --git a/srml/fees/src/lib.rs b/srml/fees/src/lib.rs index 09e32db9b0d4b..20405ccc2ec5d 100644 --- a/srml/fees/src/lib.rs +++ b/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 for enum Call where origin: T::Origin { fn deposit_event() = default; - - fn on_finalise() { - let extrinsic_count = >::extrinsic_count(); - (0..extrinsic_count).for_each(|index| { - // Deposit `Charged` event if some amount of fee charged. - let fee = >::take(index); - if !fee.is_zero() { - Self::deposit_event(RawEvent::Charged(index, fee)); - } - }); - } } } -decl_event!( - pub enum Event where Amount = AssetOf { - /// Fee charged (extrinsic_index, fee_amount) - Charged(u32, Amount), - } -); - decl_storage! { trait Store for Module as Fees { /// The fee to be paid for making a transaction; the base. pub TransactionBaseFee get(transaction_base_fee) config(): AssetOf; /// The fee to be paid for making a transaction; the per-byte portion. pub TransactionByteFee get(transaction_byte_fee) config(): AssetOf; - - /// 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; } } -impl ChargeBytesFee for Module { - fn charge_base_bytes_fee(transactor: &T::AccountId, encoded_len: usize) -> Result { +decl_event!( + pub enum Event where ::AccountId, Amount = AssetOf { + /// Transaction payment charged (transactor, fee_amount) + TransactionPayment(AccountId, Amount), + } +); + +impl MakeTransactionPayment for Module { + fn make_transaction_payment(transactor: &T::AccountId, encoded_len: usize) -> Result { let bytes_fee = Self::transaction_byte_fee().checked_mul( & as As>::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 ChargeFee for Module { - type Amount = AssetOf; - - fn charge_fee(transactor: &T::AccountId, amount: AssetOf) -> Result { - let extrinsic_index = >::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)?; - - >::insert(extrinsic_index, new_fee); - Ok(()) - } - - fn refund_fee(transactor: &T::AccountId, amount: AssetOf) -> Result { - let extrinsic_index = >::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)); - >::insert(extrinsic_index, new_fee); Ok(()) } } diff --git a/srml/fees/src/mock.rs b/srml/fees/src/mock.rs index 7f6c715f4b8bb..90fcf76ec1cbe 100644 --- a/srml/fees/src/mock.rs +++ b/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, + balances, fees, } } -pub struct TransferAssetMock; - -impl TransferAsset 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; pub type Fees = Module; +pub type Balances = balances::Module; pub struct ExtBuilder { transaction_base_fee: u64, @@ -106,6 +98,13 @@ impl ExtBuilder { } pub fn build(self) -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::::default().build_storage().unwrap().0; + t.extend(balances::GenesisConfig::{ + balances: vec![(0, 1000)], + existential_deposit: 0, + transfer_fee: 0, + creation_fee: 0, + vesting: vec![], + }.build_storage().unwrap().0); t.extend(GenesisConfig:: { transaction_base_fee: self.transaction_base_fee, transaction_byte_fee: self.transaction_byte_fee, diff --git a/srml/fees/src/tests.rs b/srml/fees/src/tests.rs index a1c96570629e8..29a36ce19df7f 100644 --- a/srml/fees/src/tests.rs +++ b/srml/fees/src/tests.rs @@ -20,12 +20,22 @@ use super::*; use runtime_io::with_externalities; -use runtime_primitives::traits::{OnFinalise}; use system::{EventRecord, Phase}; -use mock::{Fees, System, ExtBuilder}; +use mock::{Fees, System, ExtBuilder, Balances}; use srml_support::{assert_ok, assert_err}; +fn get_events() -> Vec> { + System::events() + .into_iter() + .filter(|e| match e.event { + mock::TestEvent::fees(_) => true, + mock::TestEvent::balances(_) => true, + _ => false, + }) + .collect() +} + #[test] fn charge_base_bytes_fee_should_work() { with_externalities( @@ -35,16 +45,25 @@ fn charge_base_bytes_fee_should_work() { .build(), || { System::set_extrinsic_index(0); - assert_ok!(Fees::charge_base_bytes_fee(&0, 7)); - assert_eq!(Fees::current_transaction_fee(0), 3 + 5 * 7); + let fee = 3 + 5 * 7; + assert_ok!(Fees::make_transaction_payment(&0, 7)); + assert_eq!(Balances::free_balance(&0), 1000 - fee); System::set_extrinsic_index(1); - assert_ok!(Fees::charge_base_bytes_fee(&0, 11)); - assert_eq!(Fees::current_transaction_fee(1), 3 + 5 * 11); - - System::set_extrinsic_index(3); - assert_ok!(Fees::charge_base_bytes_fee(&0, 13)); - assert_eq!(Fees::current_transaction_fee(3), 3 + 5 * 13); + let fee2 = 3 + 5 * 11; + assert_ok!(Fees::make_transaction_payment(&0, 11)); + assert_eq!(Balances::free_balance(&0), 1000 - fee - fee2); + + assert_eq!(get_events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: RawEvent::TransactionPayment(0, fee).into(), + }, + EventRecord { + phase: Phase::ApplyExtrinsic(1), + event: RawEvent::TransactionPayment(0, fee2).into(), + }, + ]); } ); } @@ -58,11 +77,12 @@ fn charge_base_bytes_fee_should_not_work_if_bytes_fee_overflow() { .transaction_byte_fee(u64::max_value()) .build(), || { - System::set_extrinsic_index(0); assert_err!( - Fees::charge_base_bytes_fee(&0, 2), + Fees::make_transaction_payment(&0, 2), "bytes fee overflow" ); + + assert_eq!(get_events(), Vec::new()); } ); } @@ -76,99 +96,12 @@ fn charge_base_bytes_fee_should_not_work_if_overall_fee_overflow() { .transaction_byte_fee(1) .build(), || { - System::set_extrinsic_index(0); assert_err!( - Fees::charge_base_bytes_fee(&0, 1), + Fees::make_transaction_payment(&0, 1), "bytes fee overflow" ); + + assert_eq!(get_events(), Vec::new()); } ); } - -#[test] -fn charge_fee_should_work() { - with_externalities(&mut ExtBuilder::default().build(), || { - System::set_extrinsic_index(0); - assert_ok!(Fees::charge_fee(&0, 2)); - assert_ok!(Fees::charge_fee(&0, 3)); - assert_eq!(Fees::current_transaction_fee(0), 2 + 3); - - System::set_extrinsic_index(2); - assert_ok!(Fees::charge_fee(&0, 5)); - assert_ok!(Fees::charge_fee(&0, 7)); - assert_eq!(Fees::current_transaction_fee(2), 5 + 7); - }); -} - -#[test] -fn charge_fee_when_overflow_should_not_work() { - with_externalities(&mut ExtBuilder::default().build(), || { - System::set_extrinsic_index(0); - assert_ok!(Fees::charge_fee(&0, u64::max_value())); - assert_err!(Fees::charge_fee(&0, 1), "fee got overflow after charge"); - }); -} - -#[test] -fn refund_fee_should_work() { - with_externalities(&mut ExtBuilder::default().build(), || { - System::set_extrinsic_index(0); - assert_ok!(Fees::charge_fee(&0, 5)); - assert_ok!(Fees::refund_fee(&0, 3)); - assert_eq!(Fees::current_transaction_fee(0), 5 - 3); - }); -} - -#[test] -fn refund_fee_when_underflow_should_not_work() { - with_externalities(&mut ExtBuilder::default().build(), || { - System::set_extrinsic_index(0); - assert_err!(Fees::refund_fee(&0, 1), "fee got underflow after refund"); - }); -} - -#[test] -fn on_finalise_should_work() { - with_externalities(&mut ExtBuilder::default().build(), || { - // charge fees in extrinsic index 3 - System::set_extrinsic_index(3); - assert_ok!(Fees::charge_fee(&0, 1)); - System::note_applied_extrinsic(&Ok(()), 1); - // charge fees in extrinsic index 5 - System::set_extrinsic_index(5); - assert_ok!(Fees::charge_fee(&0, 1)); - System::note_applied_extrinsic(&Ok(()), 1); - System::note_finished_extrinsics(); - - // `current_transaction_fee`, `extrinsic_count` should be as expected. - assert_eq!(Fees::current_transaction_fee(3), 1); - assert_eq!(Fees::current_transaction_fee(5), 1); - assert_eq!(System::extrinsic_count(), 5 + 1); - - >::on_finalise(1); - - // When finalised, `CurrentTransactionFee` records should be cleared. - assert_eq!(Fees::current_transaction_fee(3), 0); - assert_eq!(Fees::current_transaction_fee(5), 0); - - // When finalised, if any fee charged in a extrinsic, a `Charged` event should be deposited - // for it. - let fee_charged_events: Vec> = System::events() - .into_iter() - .filter(|e| match e.event { - mock::TestEvent::fees(RawEvent::Charged(_, _)) => return true, - _ => return false, - }) - .collect(); - assert_eq!(fee_charged_events, vec![ - EventRecord { - phase: Phase::Finalization, - event: RawEvent::Charged(3, 1).into(), - }, - EventRecord { - phase: Phase::Finalization, - event: RawEvent::Charged(5, 1).into(), - }, - ]); - }); -} diff --git a/srml/support/src/traits.rs b/srml/support/src/traits.rs index fbce46eac0aef..f72101da4b7d8 100644 --- a/srml/support/src/traits.rs +++ b/srml/support/src/traits.rs @@ -207,23 +207,11 @@ pub trait LockableCurrency: Currency { ); } -/// Charge bytes fee trait -pub trait ChargeBytesFee { +/// Make transaction payment trait +pub trait MakeTransactionPayment { /// Charge fees from `transactor` for an extrinsic (transaction) of encoded length /// `encoded_len` bytes. Return Ok if the payment was successful. - fn charge_base_bytes_fee(transactor: &AccountId, encoded_len: usize) -> Result<(), &'static str>; -} - -/// Charge fee trait -pub trait ChargeFee: ChargeBytesFee { - /// The type of fee amount. - type Amount; - - /// Charge `amount` of fees from `transactor`. Return Ok iff the payment was successful. - fn charge_fee(transactor: &AccountId, amount: Self::Amount) -> Result<(), &'static str>; - - /// Refund `amount` of previous charged fees from `transactor`. Return Ok if the refund was successful. - fn refund_fee(transactor: &AccountId, amount: Self::Amount) -> Result<(), &'static str>; + fn make_transaction_payment(transactor: &AccountId, encoded_len: usize) -> Result<(), &'static str>; } bitmask! { @@ -258,15 +246,8 @@ pub trait TransferAsset { fn deposit(who: &AccountId, amount: Self::Amount) -> Result<(), &'static str>; } -impl ChargeBytesFee for () { - fn charge_base_bytes_fee(_: &T, _: usize) -> Result<(), &'static str> { Ok(()) } -} - -impl ChargeFee for () { - type Amount = (); - - fn charge_fee(_: &T, _: Self::Amount) -> Result<(), &'static str> { Ok(()) } - fn refund_fee(_: &T, _: Self::Amount) -> Result<(), &'static str> { Ok(()) } +impl MakeTransactionPayment for () { + fn make_transaction_payment(_: &T, _: usize) -> Result<(), &'static str> { Ok(()) } } impl TransferAsset for () {