From 9e5d60496f03dbe217a669faf3da00b5a607e699 Mon Sep 17 00:00:00 2001 From: AlexSedNZ Date: Mon, 24 Feb 2020 13:39:18 +1300 Subject: [PATCH 1/4] Uncouple emptying gas_meter from updating the gas spent for the block. However keep the update function for GasHandler as it seems to be relevant to this trait. --- frame/contracts/src/gas.rs | 18 ++++++++++++------ frame/contracts/src/gas_tests.rs | 4 ++-- frame/contracts/src/lib.rs | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index a65386d396..4af19fc4e6 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -209,6 +209,18 @@ pub trait GasHandler { buy_gas::(transactor, gas_limit) } + /// This function should be called when the contract has stopped consuming gas and the gas_meter + /// is ready to be read. This will update the gas spent for the block and then empties the + /// unused gas. + fn update(transactor: &T::AccountId, gas_meter: GasMeter) { + // Increase total spent gas. + // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which + // also has Gas type. + GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_meter.spent()); + + Self::empty_unused_gas(transactor, gas_meter); + } + /// This function empties the remaining gas in the gas meter /// Default behaviour will deposit currency into the user's balance to refund un-used gas fn empty_unused_gas( @@ -257,14 +269,8 @@ pub fn refund_unused_gas( transactor: &T::AccountId, gas_meter: GasMeter, ) { - 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 Gas type. - GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_spent); - // Refund gas left by the price it was bought at. let refund = gas_meter.gas_price * gas_left.unique_saturated_into(); let _imbalance = T::Currency::deposit_creating(transactor, refund); diff --git a/frame/contracts/src/gas_tests.rs b/frame/contracts/src/gas_tests.rs index dc6d0fa8bb..998796be6c 100644 --- a/frame/contracts/src/gas_tests.rs +++ b/frame/contracts/src/gas_tests.rs @@ -302,7 +302,7 @@ fn user_is_charged_on_empty_unused_gas() { // Spend half the gas, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_used)); - TestGasHandler::empty_unused_gas(&ALICE, gas_meter); + TestGasHandler::update(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); @@ -327,7 +327,7 @@ fn user_is_charged_if_out_of_gas() { // Spend more gas than the `gas_limit`, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_limit + 1)); - TestGasHandler::empty_unused_gas(&ALICE, gas_meter); + TestGasHandler::update(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a6587d8b36..a689d5e65e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -588,7 +588,7 @@ decl_module! { if let Ok(code_hash) = result { Self::deposit_event(RawEvent::CodeStored(code_hash)); } - T::GasHandler::empty_unused_gas(&origin, gas_meter); + T::GasHandler::update(&origin, gas_meter); result.map(|_| ()).map_err(Into::into) } @@ -756,7 +756,7 @@ impl Module { // // NOTE: This should go after the commit to the storage, since the storage changes // can alter the balance of the caller. - T::GasHandler::empty_unused_gas(&origin, gas_meter); + T::GasHandler::update(&origin, gas_meter); // Execute deferred actions. ctx.deferred.into_iter().for_each(|deferred| { From 7771e75832174ece7cce5fe47bbfe2bd640efe8d Mon Sep 17 00:00:00 2001 From: AlexSedNZ Date: Tue, 25 Feb 2020 12:32:15 +1300 Subject: [PATCH 2/4] Choose a better name for update function. Refine a test that would show "finish" will call the right "empty_unused_gas". --- frame/contracts/src/gas.rs | 2 +- frame/contracts/src/gas_tests.rs | 30 +++++++++++++++++++++++++----- frame/contracts/src/lib.rs | 4 ++-- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index 4af19fc4e6..3e2bf504f3 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -212,7 +212,7 @@ pub trait GasHandler { /// This function should be called when the contract has stopped consuming gas and the gas_meter /// is ready to be read. This will update the gas spent for the block and then empties the /// unused gas. - fn update(transactor: &T::AccountId, gas_meter: GasMeter) { + fn finish(transactor: &T::AccountId, gas_meter: GasMeter) { // Increase total spent gas. // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which // also has Gas type. diff --git a/frame/contracts/src/gas_tests.rs b/frame/contracts/src/gas_tests.rs index 998796be6c..d691850002 100644 --- a/frame/contracts/src/gas_tests.rs +++ b/frame/contracts/src/gas_tests.rs @@ -262,6 +262,24 @@ impl GasHandler for TestGasHandler { } } +pub struct NoChargeGasHandler; +impl GasHandler for NoChargeGasHandler { + fn fill_gas( + _transactor: &::AccountId, + gas_limit: Gas, + ) -> Result, DispatchError> { + // fills the gas meter without charging the user + Ok(GasMeter::with_limit(gas_limit, 1)) + } + + fn empty_unused_gas( + transactor: &::AccountId, + gas_meter: GasMeter, + ) { + // Do not charge the transactor. Give gas for free. + } +} + #[test] // Tests that the user is not charged when filling up gas meters fn customized_fill_gas_does_not_charge_the_user() { @@ -273,9 +291,11 @@ fn customized_fill_gas_does_not_charge_the_user() { // Create test account Balances::deposit_creating(&ALICE, 1000); - // fill gas - let gas_meter_result = TestGasHandler::fill_gas(&ALICE, 500); - assert!(gas_meter_result.is_ok()); + let gas_limit = 500; + let mut gas_meter = NoChargeGasHandler::fill_gas(&ALICE, gas_limit).unwrap(); + // Charge as if the whole gas_limit is used + gas_meter.charge(&(), SimpleToken(gas_limit)); + NoChargeGasHandler::finish(&ALICE, gas_meter); // Check the user is not charged assert_eq!(Balances::free_balance(&ALICE), 1000); @@ -302,7 +322,7 @@ fn user_is_charged_on_empty_unused_gas() { // Spend half the gas, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_used)); - TestGasHandler::update(&ALICE, gas_meter); + TestGasHandler::finish(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); @@ -327,7 +347,7 @@ fn user_is_charged_if_out_of_gas() { // Spend more gas than the `gas_limit`, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_limit + 1)); - TestGasHandler::update(&ALICE, gas_meter); + TestGasHandler::finish(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a689d5e65e..28f3dc6fae 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -588,7 +588,7 @@ decl_module! { if let Ok(code_hash) = result { Self::deposit_event(RawEvent::CodeStored(code_hash)); } - T::GasHandler::update(&origin, gas_meter); + T::GasHandler::finish(&origin, gas_meter); result.map(|_| ()).map_err(Into::into) } @@ -756,7 +756,7 @@ impl Module { // // NOTE: This should go after the commit to the storage, since the storage changes // can alter the balance of the caller. - T::GasHandler::update(&origin, gas_meter); + T::GasHandler::finish(&origin, gas_meter); // Execute deferred actions. ctx.deferred.into_iter().for_each(|deferred| { From bf82cd046c4082c037a2a337603476af31eaacc7 Mon Sep 17 00:00:00 2001 From: AlexSedNZ Date: Tue, 25 Feb 2020 15:02:45 +1300 Subject: [PATCH 3/4] Test gas handler updates the gas spent for the block on finish --- frame/contracts/src/gas_tests.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/frame/contracts/src/gas_tests.rs b/frame/contracts/src/gas_tests.rs index d691850002..fbdac7c973 100644 --- a/frame/contracts/src/gas_tests.rs +++ b/frame/contracts/src/gas_tests.rs @@ -280,6 +280,30 @@ impl GasHandler for NoChargeGasHandler { } } +#[test] +// Tests that the user is not charged when filling up gas meters +fn gas_handler_updates_block_gas_spent_on_finish() { + ExtBuilder::default() + .existential_deposit(50) + .gas_price(1) + .build() + .execute_with(|| { + // Create test account + Balances::deposit_creating(&ALICE, 1000); + + let gas_limit = 500; + let mut gas_meter = NoChargeGasHandler::fill_gas(&ALICE, gas_limit).unwrap(); + // Charge as if the whole gas_limit is used + gas_meter.charge(&(), SimpleToken(gas_limit)); + + let gas_spent_for_block = Contract::gas_spent(); + let gas_spent_for_contract = gas_meter.spent(); + NoChargeGasHandler::finish(&ALICE, gas_meter); + + assert_eq!(Contract::gas_spent(), gas_spent_for_block + gas_spent_for_contract); + }); +} + #[test] // Tests that the user is not charged when filling up gas meters fn customized_fill_gas_does_not_charge_the_user() { From 8853cc6837878b830bfecbc4faab584fc1b77624 Mon Sep 17 00:00:00 2001 From: AlexSedNZ Date: Wed, 26 Feb 2020 11:01:48 +1300 Subject: [PATCH 4/4] In lining the call for mutating the gas-spent for blocks. This is because the team see a new interface such as finish fo GasHandler as exposing. The test for checking if gas-spent for the block increases correctly is removed as it couldn't be relevant with this new design. --- frame/contracts/src/gas.rs | 17 ++--------------- frame/contracts/src/gas_tests.rs | 30 +++--------------------------- frame/contracts/src/lib.rs | 14 ++++++++++++-- 3 files changed, 17 insertions(+), 44 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index 3e2bf504f3..ba0471c233 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -14,14 +14,13 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{GasSpent, Module, Trait, BalanceOf}; +use crate::{Module, Trait, BalanceOf}; use sp_std::convert::TryFrom; use sp_runtime::traits::{ CheckedMul, Zero, SaturatedConversion, AtLeast32Bit, UniqueSaturatedInto, }; use frame_support::{ - traits::{Currency, ExistenceRequirement, OnUnbalanced, WithdrawReason}, StorageValue, - dispatch::DispatchError, + traits::{Currency, ExistenceRequirement, OnUnbalanced, WithdrawReason}, dispatch::DispatchError, }; #[cfg(test)] @@ -209,18 +208,6 @@ pub trait GasHandler { buy_gas::(transactor, gas_limit) } - /// This function should be called when the contract has stopped consuming gas and the gas_meter - /// is ready to be read. This will update the gas spent for the block and then empties the - /// unused gas. - fn finish(transactor: &T::AccountId, gas_meter: GasMeter) { - // Increase total spent gas. - // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which - // also has Gas type. - GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_meter.spent()); - - Self::empty_unused_gas(transactor, gas_meter); - } - /// This function empties the remaining gas in the gas meter /// Default behaviour will deposit currency into the user's balance to refund un-used gas fn empty_unused_gas( diff --git a/frame/contracts/src/gas_tests.rs b/frame/contracts/src/gas_tests.rs index fbdac7c973..32c5e1e8e2 100644 --- a/frame/contracts/src/gas_tests.rs +++ b/frame/contracts/src/gas_tests.rs @@ -280,30 +280,6 @@ impl GasHandler for NoChargeGasHandler { } } -#[test] -// Tests that the user is not charged when filling up gas meters -fn gas_handler_updates_block_gas_spent_on_finish() { - ExtBuilder::default() - .existential_deposit(50) - .gas_price(1) - .build() - .execute_with(|| { - // Create test account - Balances::deposit_creating(&ALICE, 1000); - - let gas_limit = 500; - let mut gas_meter = NoChargeGasHandler::fill_gas(&ALICE, gas_limit).unwrap(); - // Charge as if the whole gas_limit is used - gas_meter.charge(&(), SimpleToken(gas_limit)); - - let gas_spent_for_block = Contract::gas_spent(); - let gas_spent_for_contract = gas_meter.spent(); - NoChargeGasHandler::finish(&ALICE, gas_meter); - - assert_eq!(Contract::gas_spent(), gas_spent_for_block + gas_spent_for_contract); - }); -} - #[test] // Tests that the user is not charged when filling up gas meters fn customized_fill_gas_does_not_charge_the_user() { @@ -319,7 +295,7 @@ fn customized_fill_gas_does_not_charge_the_user() { let mut gas_meter = NoChargeGasHandler::fill_gas(&ALICE, gas_limit).unwrap(); // Charge as if the whole gas_limit is used gas_meter.charge(&(), SimpleToken(gas_limit)); - NoChargeGasHandler::finish(&ALICE, gas_meter); + NoChargeGasHandler::empty_unused_gas(&ALICE, gas_meter); // Check the user is not charged assert_eq!(Balances::free_balance(&ALICE), 1000); @@ -346,7 +322,7 @@ fn user_is_charged_on_empty_unused_gas() { // Spend half the gas, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_used)); - TestGasHandler::finish(&ALICE, gas_meter); + TestGasHandler::empty_unused_gas(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); @@ -371,7 +347,7 @@ fn user_is_charged_if_out_of_gas() { // Spend more gas than the `gas_limit`, then empty the gas meter, the user should be charged here gas_meter.charge(&(), SimpleToken(gas_limit + 1)); - TestGasHandler::finish(&ALICE, gas_meter); + TestGasHandler::empty_unused_gas(&ALICE, gas_meter); assert_eq!(Balances::free_balance(&ALICE), expected_remaining_balance); }); diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 28f3dc6fae..b897fe1b85 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -588,7 +588,12 @@ decl_module! { if let Ok(code_hash) = result { Self::deposit_event(RawEvent::CodeStored(code_hash)); } - T::GasHandler::finish(&origin, gas_meter); + // Increase total spent gas. + // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which + // also has Gas type. + GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_meter.spent()); + + T::GasHandler::empty_unused_gas(&origin, gas_meter); result.map(|_| ()).map_err(Into::into) } @@ -752,11 +757,16 @@ impl Module { DirectAccountDb.commit(ctx.overlay.into_change_set()); } + // Increase total spent gas. + // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which + // also has Gas type. + GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_meter.spent()); + // Handle unused gas of the gas meter. Default behaviour is to refund cost of the unused gas. // // NOTE: This should go after the commit to the storage, since the storage changes // can alter the balance of the caller. - T::GasHandler::finish(&origin, gas_meter); + T::GasHandler::empty_unused_gas(&origin, gas_meter); // Execute deferred actions. ctx.deferred.into_iter().for_each(|deferred| {