From 1a004bbc1d98aa11705af6205d43d688b467415a Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 14 Aug 2023 18:00:36 +0200 Subject: [PATCH 01/10] transfer to beneficiary after transfer_on_hold --- frame/contracts/src/exec.rs | 21 ++----- frame/contracts/src/storage/meter.rs | 88 +++++++++++++++++++--------- frame/contracts/src/tests.rs | 18 +++--- 3 files changed, 73 insertions(+), 54 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index c9318faac5f2e..c7c441c25c99e 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -22,7 +22,7 @@ use crate::{ storage::{self, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DebugBufferVec, Determinism, Error, Event, Nonce, Origin, Pallet as Contracts, Schedule, - System, WasmBlob, LOG_TARGET, + WasmBlob, LOG_TARGET, }; use frame_support::{ crypto::ecdsa::ECDSAExt, @@ -33,7 +33,7 @@ use frame_support::{ storage::{with_transaction, TransactionOutcome}, traits::{ fungible::{Inspect, Mutate}, - tokens::{Fortitude::Polite, Preservation}, + tokens::Preservation, Contains, OriginTrait, Randomness, Time, }, weights::Weight, @@ -1283,21 +1283,8 @@ where } let frame = self.top_frame_mut(); let info = frame.terminate(); - frame.nested_storage.terminate(&info); - - // During the termination process we first transfer all the free balance of the contract to - // the beneficiary. And then we release the held balance under the `StorageDepositReserve` - // reason to be transferred back. - // We need a provider so that the `reducible_balance` calculation includes the `ed` and the - // `transfer` succeeds while keeping the account alive. The provider is removed once the - // storage deposit process is finished. - System::::inc_providers(&frame.account_id); - Self::transfer( - Preservation::Expendable, - &frame.account_id, - beneficiary, - T::Currency::reducible_balance(&frame.account_id, Preservation::Expendable, Polite), - )?; + frame.nested_storage.terminate(&info, beneficiary); + info.queue_trie_for_deletion(); ContractInfoOf::::remove(&frame.account_id); E::decrement_refcount(info.code_hash); diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index fae535eec0a5b..f6982951e464c 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -18,8 +18,8 @@ //! This module contains functions to meter the storage deposit. use crate::{ - storage::ContractInfo, BalanceOf, CodeInfo, Config, Error, Event, HoldReason, Inspect, Origin, - Pallet, StorageDeposit as Deposit, System, LOG_TARGET, + storage::ContractInfo, AccountIdOf, BalanceOf, CodeInfo, Config, Error, Event, HoldReason, + Inspect, Origin, Pallet, StorageDeposit as Deposit, System, LOG_TARGET, }; use frame_support::{ @@ -88,7 +88,7 @@ pub trait Ext { origin: &T::AccountId, contract: &T::AccountId, amount: &DepositOf, - terminated: bool, + status: &ContractStatus, ) -> Result<(), DispatchError>; } @@ -224,6 +224,18 @@ impl Diff { } } +/// The beneficiary of a contract termination. +type BeneficiaryOf = AccountIdOf; + +/// The status of a contract. +/// +/// In case of termination the beneficiary is indicated. +#[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] +pub enum ContractStatus { + Alive, + Terminated(BeneficiaryOf), +} + /// Records information to charge or refund a plain account. /// /// All the charges are deferred to the end of a whole call stack. Reason is that by doing @@ -237,7 +249,7 @@ impl Diff { struct Charge { contract: T::AccountId, amount: DepositOf, - terminated: bool, + status: ContractStatus, } /// Records the storage changes of a storage meter. @@ -250,7 +262,7 @@ enum Contribution { Checked(DepositOf), /// The contract was terminated. In this process the [`Diff`] was converted into a [`Deposit`] /// in order to calculate the refund. - Terminated(DepositOf), + Terminated(DepositOf, BeneficiaryOf), } impl Contribution { @@ -258,7 +270,7 @@ impl Contribution { fn update_contract(&self, info: Option<&mut ContractInfo>) -> DepositOf { match self { Self::Alive(diff) => diff.update_contract::(info), - Self::Terminated(deposit) | Self::Checked(deposit) => deposit.clone(), + Self::Terminated(deposit, _) | Self::Checked(deposit) => deposit.clone(), } } } @@ -325,7 +337,7 @@ where self.charges.push(Charge { contract: contract.clone(), amount: own_deposit, - terminated: absorbed.is_terminated(), + status: absorbed.is_terminated(), }); } } @@ -341,8 +353,12 @@ where } /// True if the contract is terminated. - fn is_terminated(&self) -> bool { - matches!(self.own_contribution, Contribution::Terminated(_)) + fn is_terminated(&self) -> ContractStatus { + match &self.own_contribution { + Contribution::Terminated(_, beneficiary) => + ContractStatus::Terminated(beneficiary.clone()), + _ => ContractStatus::Alive, + } } } @@ -386,10 +402,10 @@ where Origin::Signed(o) => o, }; for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) { - E::charge(origin, &charge.contract, &charge.amount, charge.terminated)?; + E::charge(origin, &charge.contract, &charge.amount, &charge.status)?; } for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) { - E::charge(origin, &charge.contract, &charge.amount, charge.terminated)?; + E::charge(origin, &charge.contract, &charge.amount, &charge.status)?; } Ok(self.total_deposit) } @@ -417,7 +433,7 @@ where /// deposit charge separately from the storage charge. pub fn charge_deposit(&mut self, contract: T::AccountId, amount: DepositOf) { self.total_deposit = self.total_deposit.saturating_add(&amount); - self.charges.push(Charge { contract, amount, terminated: false }); + self.charges.push(Charge { contract, amount, status: ContractStatus::Alive }); } /// Charge from `origin` a storage deposit plus the ed for contract instantiation. @@ -447,10 +463,17 @@ where // We need to make sure that the contract's account exists. T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?; + System::::inc_consumers(&contract)?; + // Normally, deposit charges are deferred to be able to coalesce them with refunds. // However, we need to charge immediately so that the account is created before // charges possibly below the ed are collected and fail. - E::charge(origin, contract, &deposit.saturating_sub(&Deposit::Charge(ed)), false)?; + E::charge( + origin, + contract, + &deposit.saturating_sub(&Deposit::Charge(ed)), + &ContractStatus::Alive, + )?; Ok(deposit) } @@ -458,10 +481,12 @@ where /// Call to tell the meter that the currently executing contract was executed. /// /// This will manipulate the meter so that all storage deposit accumulated in - /// `contract_info` will be refunded to the `origin` of the meter. - pub fn terminate(&mut self, info: &ContractInfo) { + /// `contract_info` will be refunded to the `origin` of the meter. And the free + /// (`reducible_balance`) will be sent to the `beneficiary`. + pub fn terminate(&mut self, info: &ContractInfo, beneficiary: &T::AccountId) { debug_assert!(self.is_alive()); - self.own_contribution = Contribution::Terminated(Deposit::Refund(info.total_deposit())); + self.own_contribution = + Contribution::Terminated(Deposit::Refund(info.total_deposit()), beneficiary.clone()); } /// [`Self::charge`] does not enforce the storage limit since we want to do this check as late @@ -530,7 +555,7 @@ impl Ext for ReservingExt { origin: &T::AccountId, contract: &T::AccountId, amount: &DepositOf, - terminated: bool, + status: &ContractStatus, ) -> Result<(), DispatchError> { match amount { Deposit::Charge(amount) | Deposit::Refund(amount) if amount.is_zero() => return Ok(()), @@ -588,8 +613,15 @@ impl Ext for ReservingExt { } }, } - if terminated { - System::::dec_providers(&contract)?; + if let ContractStatus::::Terminated(beneficiary) = status { + System::::dec_consumers(&contract); + // Whatever is left in the contract is sent to the termination beneficiary. + T::Currency::transfer( + &contract, + beneficiary, + T::Currency::reducible_balance(&contract, Preservation::Expendable, Polite), + Preservation::Expendable, + )?; } Ok(()) } @@ -629,7 +661,7 @@ mod tests { origin: AccountIdOf, contract: AccountIdOf, amount: DepositOf, - terminated: bool, + status: ContractStatus, } #[derive(Default, Debug, PartialEq, Eq, Clone)] @@ -663,14 +695,14 @@ mod tests { origin: &AccountIdOf, contract: &AccountIdOf, amount: &DepositOf, - terminated: bool, + status: &ContractStatus, ) -> Result<(), DispatchError> { TestExtTestValue::mutate(|ext| { ext.charges.push(Charge { origin: origin.clone(), contract: contract.clone(), amount: amount.clone(), - terminated, + status: status.clone(), }) }); Ok(()) @@ -757,19 +789,19 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(10), - terminated: false, + status: ContractStatus::Alive, }, Charge { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(20), - terminated: false, + status: ContractStatus::Alive, }, Charge { origin: ALICE, contract: BOB, amount: Deposit::Charge(2), - terminated: false, + status: ContractStatus::Alive, }, ], }, @@ -848,13 +880,13 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(119), - terminated: true, + status: ContractStatus::Terminated(CHARLIE), }, Charge { origin: ALICE, contract: BOB, amount: Deposit::Charge(12), - terminated: false, + status: ContractStatus::Alive, }, ], }, @@ -890,7 +922,7 @@ mod tests { let mut nested1 = nested0.nested(BalanceOf::::zero()); nested1.charge(&Diff { items_removed: 5, ..Default::default() }); nested1.charge(&Diff { bytes_added: 20, ..Default::default() }); - nested1.terminate(&nested1_info); + nested1.terminate(&nested1_info, &CHARLIE); nested0.enforce_limit(Some(&mut nested1_info)).unwrap(); nested0.absorb(nested1, &CHARLIE, None); diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 0ca4736969d0e..78d4980f0226f 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -1598,15 +1598,6 @@ fn self_destruct_works() { pretty_assertions::assert_eq!( System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Balances(pallet_balances::Event::Transfer { - from: addr.clone(), - to: DJANGO, - amount: 100_000 + min_balance, - }), - topics: vec![], - }, EventRecord { phase: Phase::Initialization, event: RuntimeEvent::Contracts(crate::Event::Terminated { @@ -1641,6 +1632,15 @@ fn self_destruct_works() { }), topics: vec![], }, + EventRecord { + phase: Phase::Initialization, + event: RuntimeEvent::Balances(pallet_balances::Event::Transfer { + from: addr.clone(), + to: DJANGO, + amount: 100_000 + min_balance, + }), + topics: vec![], + }, ], ); }); From 7552de965dfa9210eee91bf63186dd6aa738a30a Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 09:17:33 +0200 Subject: [PATCH 02/10] wip --- frame/contracts/src/storage/meter.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 0c0d58ca7a907..eb4f92957e196 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -462,7 +462,8 @@ where // We need to make sure that the contract's account exists. T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?; - System::::inc_consumers(&contract)?; + + System::::inc_consumers(contract)?; // Normally, deposit charges are deferred to be able to coalesce them with refunds. // However, we need to charge immediately so that the account is created before From f1d1002463c72e28387cea40bbd621fadd0b5f79 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 10:03:23 +0200 Subject: [PATCH 03/10] add consumer comment --- frame/contracts/src/storage/meter.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index eb4f92957e196..876665aec5699 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -463,6 +463,9 @@ where // We need to make sure that the contract's account exists. T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?; + // A consumer is added at account creation and remove it on termination, otherwise the + // runtime could remove the account. As long as a contract exists its account must exist. + // With the consumer a correct runtime cannot remove the account. System::::inc_consumers(contract)?; // Normally, deposit charges are deferred to be able to coalesce them with refunds. From e051ce7a8aa152b06dfc2fa732b287e33b71df11 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 10:29:11 +0200 Subject: [PATCH 04/10] review updates --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/storage/meter.rs | 31 +++++++++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index c7c441c25c99e..c30bce11e785d 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -1283,7 +1283,7 @@ where } let frame = self.top_frame_mut(); let info = frame.terminate(); - frame.nested_storage.terminate(&info, beneficiary); + frame.nested_storage.terminate(&info, beneficiary.clone()); info.queue_trie_for_deletion(); ContractInfoOf::::remove(&frame.account_id); diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 876665aec5699..1ad6d7dfbf85e 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -39,7 +39,7 @@ use sp_runtime::{ traits::{Saturating, Zero}, FixedPointNumber, FixedU128, }; -use sp_std::{marker::PhantomData, vec, vec::Vec}; +use sp_std::{marker::PhantomData, ops::Deref, vec, vec::Vec}; /// Deposit that uses the native fungible's balance type. pub type DepositOf = Deposit>; @@ -225,7 +225,16 @@ impl Diff { } /// The beneficiary of a contract termination. -type BeneficiaryOf = AccountIdOf; +#[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] +pub struct Beneficiary(AccountIdOf); + +impl Deref for Beneficiary { + type Target = AccountIdOf; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} /// The status of a contract. /// @@ -233,7 +242,7 @@ type BeneficiaryOf = AccountIdOf; #[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] pub enum ContractStatus { Alive, - Terminated(BeneficiaryOf), + Terminated(Beneficiary), } /// Records information to charge or refund a plain account. @@ -262,7 +271,7 @@ enum Contribution { Checked(DepositOf), /// The contract was terminated. In this process the [`Diff`] was converted into a [`Deposit`] /// in order to calculate the refund. - Terminated(DepositOf, BeneficiaryOf), + Terminated(DepositOf, Beneficiary), } impl Contribution { @@ -486,10 +495,12 @@ where /// This will manipulate the meter so that all storage deposit accumulated in /// `contract_info` will be refunded to the `origin` of the meter. And the free /// (`reducible_balance`) will be sent to the `beneficiary`. - pub fn terminate(&mut self, info: &ContractInfo, beneficiary: &T::AccountId) { + pub fn terminate(&mut self, info: &ContractInfo, beneficiary: T::AccountId) { debug_assert!(self.is_alive()); - self.own_contribution = - Contribution::Terminated(Deposit::Refund(info.total_deposit()), beneficiary.clone()); + self.own_contribution = Contribution::Terminated( + Deposit::Refund(info.total_deposit()), + Beneficiary(beneficiary), + ); } /// [`Self::charge`] does not enforce the storage limit since we want to do this check as late @@ -621,7 +632,7 @@ impl Ext for ReservingExt { // Whatever is left in the contract is sent to the termination beneficiary. T::Currency::transfer( &contract, - beneficiary, + &*beneficiary, T::Currency::reducible_balance(&contract, Preservation::Expendable, Polite), Preservation::Expendable, )?; @@ -883,7 +894,7 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(119), - status: ContractStatus::Terminated(CHARLIE), + status: ContractStatus::Terminated(Beneficiary(CHARLIE)), }, Charge { origin: ALICE, @@ -925,7 +936,7 @@ mod tests { let mut nested1 = nested0.nested(BalanceOf::::zero()); nested1.charge(&Diff { items_removed: 5, ..Default::default() }); nested1.charge(&Diff { bytes_added: 20, ..Default::default() }); - nested1.terminate(&nested1_info, &CHARLIE); + nested1.terminate(&nested1_info, CHARLIE); nested0.enforce_limit(Some(&mut nested1_info)).unwrap(); nested0.absorb(nested1, &CHARLIE, None); From 7b74b28f4c7eb31347b55be9133c68e1579d35c8 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 10:30:44 +0200 Subject: [PATCH 05/10] fix typo --- frame/contracts/src/storage/meter.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 1ad6d7dfbf85e..10243a7ff2102 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -472,9 +472,9 @@ where // We need to make sure that the contract's account exists. T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?; - // A consumer is added at account creation and remove it on termination, otherwise the + // A consumer is added at account creation and removed it on termination, otherwise the // runtime could remove the account. As long as a contract exists its account must exist. - // With the consumer a correct runtime cannot remove the account. + // With the consumer, a correct runtime cannot remove the account. System::::inc_consumers(contract)?; // Normally, deposit charges are deferred to be able to coalesce them with refunds. From bfe12dcaccaa0d90ca53255b1ecf18e4193e3a20 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 10:46:18 +0200 Subject: [PATCH 06/10] make clippy happy --- frame/contracts/src/storage/meter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 10243a7ff2102..bfdfc351872eb 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -632,7 +632,7 @@ impl Ext for ReservingExt { // Whatever is left in the contract is sent to the termination beneficiary. T::Currency::transfer( &contract, - &*beneficiary, + &**beneficiary, T::Currency::reducible_balance(&contract, Preservation::Expendable, Polite), Preservation::Expendable, )?; From 1a9f6c2d920af0fd8019b194529725505ec4dc77 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 15 Aug 2023 18:15:01 +0200 Subject: [PATCH 07/10] refactor `Terminated` --- frame/contracts/src/storage/meter.rs | 42 +++++++++++----------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index bfdfc351872eb..a4f741571d667 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -39,7 +39,7 @@ use sp_runtime::{ traits::{Saturating, Zero}, FixedPointNumber, FixedU128, }; -use sp_std::{marker::PhantomData, ops::Deref, vec, vec::Vec}; +use sp_std::{marker::PhantomData, vec, vec::Vec}; /// Deposit that uses the native fungible's balance type. pub type DepositOf = Deposit>; @@ -224,25 +224,13 @@ impl Diff { } } -/// The beneficiary of a contract termination. -#[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] -pub struct Beneficiary(AccountIdOf); - -impl Deref for Beneficiary { - type Target = AccountIdOf; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - /// The status of a contract. /// /// In case of termination the beneficiary is indicated. #[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] pub enum ContractStatus { Alive, - Terminated(Beneficiary), + Terminated { beneficiary: AccountIdOf }, } /// Records information to charge or refund a plain account. @@ -270,8 +258,9 @@ enum Contribution { /// its execution. In this process the [`Diff`] was converted into a [`Deposit`]. Checked(DepositOf), /// The contract was terminated. In this process the [`Diff`] was converted into a [`Deposit`] - /// in order to calculate the refund. - Terminated(DepositOf, Beneficiary), + /// in order to calculate the refund. Upon termination the `reducible_balance` in the + /// contract's account is transferred to the [`beneficiary`]. + Terminated { deposit: DepositOf, beneficiary: AccountIdOf }, } impl Contribution { @@ -279,7 +268,8 @@ impl Contribution { fn update_contract(&self, info: Option<&mut ContractInfo>) -> DepositOf { match self { Self::Alive(diff) => diff.update_contract::(info), - Self::Terminated(deposit, _) | Self::Checked(deposit) => deposit.clone(), + Self::Terminated { deposit, beneficiary: _ } | Self::Checked(deposit) => + deposit.clone(), } } } @@ -364,8 +354,8 @@ where /// True if the contract is terminated. fn is_terminated(&self) -> ContractStatus { match &self.own_contribution { - Contribution::Terminated(_, beneficiary) => - ContractStatus::Terminated(beneficiary.clone()), + Contribution::Terminated { deposit: _, beneficiary } => + ContractStatus::Terminated { beneficiary: beneficiary.clone() }, _ => ContractStatus::Alive, } } @@ -497,10 +487,10 @@ where /// (`reducible_balance`) will be sent to the `beneficiary`. pub fn terminate(&mut self, info: &ContractInfo, beneficiary: T::AccountId) { debug_assert!(self.is_alive()); - self.own_contribution = Contribution::Terminated( - Deposit::Refund(info.total_deposit()), - Beneficiary(beneficiary), - ); + self.own_contribution = Contribution::Terminated { + deposit: Deposit::Refund(info.total_deposit()), + beneficiary, + }; } /// [`Self::charge`] does not enforce the storage limit since we want to do this check as late @@ -627,12 +617,12 @@ impl Ext for ReservingExt { } }, } - if let ContractStatus::::Terminated(beneficiary) = status { + if let ContractStatus::::Terminated { beneficiary } = status { System::::dec_consumers(&contract); // Whatever is left in the contract is sent to the termination beneficiary. T::Currency::transfer( &contract, - &**beneficiary, + &beneficiary, T::Currency::reducible_balance(&contract, Preservation::Expendable, Polite), Preservation::Expendable, )?; @@ -894,7 +884,7 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(119), - status: ContractStatus::Terminated(Beneficiary(CHARLIE)), + status: ContractStatus::Terminated { beneficiary: CHARLIE }, }, Charge { origin: ALICE, From e00cd3fcaf3bc378e236c5692dbe733c408f94e7 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 16 Aug 2023 16:38:28 +0200 Subject: [PATCH 08/10] rename ContractStatus to ContractState --- frame/contracts/src/storage/meter.rs | 61 +++++++++++++--------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index a4f741571d667..7356a9af3e9de 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -88,7 +88,7 @@ pub trait Ext { origin: &T::AccountId, contract: &T::AccountId, amount: &DepositOf, - status: &ContractStatus, + state: &ContractState, ) -> Result<(), DispatchError>; } @@ -224,11 +224,11 @@ impl Diff { } } -/// The status of a contract. +/// The state of a contract. /// /// In case of termination the beneficiary is indicated. #[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)] -pub enum ContractStatus { +pub enum ContractState { Alive, Terminated { beneficiary: AccountIdOf }, } @@ -246,7 +246,7 @@ pub enum ContractStatus { struct Charge { contract: T::AccountId, amount: DepositOf, - status: ContractStatus, + state: ContractState, } /// Records the storage changes of a storage meter. @@ -294,7 +294,7 @@ where /// usage for this sub call separately. This is necessary because we want to exchange balance /// with the current contract we are interacting with. pub fn nested(&self, limit: BalanceOf) -> RawMeter { - debug_assert!(self.is_alive()); + debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); // If a special limit is specified higher than it is available, // we want to enforce the lesser limit to the nested meter, to fail in the sub-call. let limit = self.available().min(limit); @@ -336,7 +336,7 @@ where self.charges.push(Charge { contract: contract.clone(), amount: own_deposit, - status: absorbed.is_terminated(), + state: absorbed.contract_state(), }); } } @@ -346,17 +346,12 @@ where self.total_deposit.available(&self.limit) } - /// True if the contract is alive. - fn is_alive(&self) -> bool { - matches!(self.own_contribution, Contribution::Alive(_)) - } - - /// True if the contract is terminated. - fn is_terminated(&self) -> ContractStatus { + /// Returns the status of the currently executed contract. + fn contract_state(&self) -> ContractState { match &self.own_contribution { Contribution::Terminated { deposit: _, beneficiary } => - ContractStatus::Terminated { beneficiary: beneficiary.clone() }, - _ => ContractStatus::Alive, + ContractState::Terminated { beneficiary: beneficiary.clone() }, + _ => ContractState::Alive, } } } @@ -401,10 +396,10 @@ where Origin::Signed(o) => o, }; for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) { - E::charge(origin, &charge.contract, &charge.amount, &charge.status)?; + E::charge(origin, &charge.contract, &charge.amount, &charge.state)?; } for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) { - E::charge(origin, &charge.contract, &charge.amount, &charge.status)?; + E::charge(origin, &charge.contract, &charge.amount, &charge.state)?; } Ok(self.total_deposit) } @@ -432,7 +427,7 @@ where /// deposit charge separately from the storage charge. pub fn charge_deposit(&mut self, contract: T::AccountId, amount: DepositOf) { self.total_deposit = self.total_deposit.saturating_add(&amount); - self.charges.push(Charge { contract, amount, status: ContractStatus::Alive }); + self.charges.push(Charge { contract, amount, state: ContractState::Alive }); } /// Charge from `origin` a storage deposit plus the ed for contract instantiation. @@ -445,7 +440,7 @@ where contract_info: &mut ContractInfo, code_info: &CodeInfo, ) -> Result, DispatchError> { - debug_assert!(self.is_alive()); + debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); let ed = Pallet::::min_balance(); let deposit = contract_info.update_base_deposit(&code_info); @@ -474,19 +469,19 @@ where origin, contract, &deposit.saturating_sub(&Deposit::Charge(ed)), - &ContractStatus::Alive, + &ContractState::Alive, )?; Ok(deposit) } - /// Call to tell the meter that the currently executing contract was executed. + /// Call to tell the meter that the currently executing contract was terminated. /// /// This will manipulate the meter so that all storage deposit accumulated in /// `contract_info` will be refunded to the `origin` of the meter. And the free /// (`reducible_balance`) will be sent to the `beneficiary`. pub fn terminate(&mut self, info: &ContractInfo, beneficiary: T::AccountId) { - debug_assert!(self.is_alive()); + debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); self.own_contribution = Contribution::Terminated { deposit: Deposit::Refund(info.total_deposit()), beneficiary, @@ -509,7 +504,7 @@ where let deposit = self.own_contribution.update_contract(info); let total_deposit = self.total_deposit.saturating_add(&deposit); // We don't want to override a `Terminated` with a `Checked`. - if self.is_alive() { + if matches!(self.own_contribution, Contribution::Alive(_)) { self.own_contribution = Contribution::Checked(deposit); } if let Deposit::Charge(amount) = total_deposit { @@ -559,7 +554,7 @@ impl Ext for ReservingExt { origin: &T::AccountId, contract: &T::AccountId, amount: &DepositOf, - status: &ContractStatus, + state: &ContractState, ) -> Result<(), DispatchError> { match amount { Deposit::Charge(amount) | Deposit::Refund(amount) if amount.is_zero() => return Ok(()), @@ -617,7 +612,7 @@ impl Ext for ReservingExt { } }, } - if let ContractStatus::::Terminated { beneficiary } = status { + if let ContractState::::Terminated { beneficiary } = state { System::::dec_consumers(&contract); // Whatever is left in the contract is sent to the termination beneficiary. T::Currency::transfer( @@ -665,7 +660,7 @@ mod tests { origin: AccountIdOf, contract: AccountIdOf, amount: DepositOf, - status: ContractStatus, + state: ContractState, } #[derive(Default, Debug, PartialEq, Eq, Clone)] @@ -699,14 +694,14 @@ mod tests { origin: &AccountIdOf, contract: &AccountIdOf, amount: &DepositOf, - status: &ContractStatus, + state: &ContractState, ) -> Result<(), DispatchError> { TestExtTestValue::mutate(|ext| { ext.charges.push(Charge { origin: origin.clone(), contract: contract.clone(), amount: amount.clone(), - status: status.clone(), + state: state.clone(), }) }); Ok(()) @@ -793,19 +788,19 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(10), - status: ContractStatus::Alive, + state: ContractState::Alive, }, Charge { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(20), - status: ContractStatus::Alive, + state: ContractState::Alive, }, Charge { origin: ALICE, contract: BOB, amount: Deposit::Charge(2), - status: ContractStatus::Alive, + state: ContractState::Alive, }, ], }, @@ -884,13 +879,13 @@ mod tests { origin: ALICE, contract: CHARLIE, amount: Deposit::Refund(119), - status: ContractStatus::Terminated { beneficiary: CHARLIE }, + state: ContractState::Terminated { beneficiary: CHARLIE }, }, Charge { origin: ALICE, contract: BOB, amount: Deposit::Charge(12), - status: ContractStatus::Alive, + state: ContractState::Alive, }, ], }, From 75cac676069bcca6ef13f3cdebecd65e78d542ac Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 16 Aug 2023 16:41:49 +0200 Subject: [PATCH 09/10] rename status to state --- frame/contracts/src/storage/meter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 7356a9af3e9de..1d98b72c6997d 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -346,7 +346,7 @@ where self.total_deposit.available(&self.limit) } - /// Returns the status of the currently executed contract. + /// Returns the state of the currently executed contract. fn contract_state(&self) -> ContractState { match &self.own_contribution { Contribution::Terminated { deposit: _, beneficiary } => From 7418969a6828797f118ed359959a864b4fff08b5 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 16 Aug 2023 17:28:23 +0200 Subject: [PATCH 10/10] replace Contribution::Alive to ContractState::Alive --- frame/contracts/src/storage/meter.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 1d98b72c6997d..18654b080a724 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -294,7 +294,7 @@ where /// usage for this sub call separately. This is necessary because we want to exchange balance /// with the current contract we are interacting with. pub fn nested(&self, limit: BalanceOf) -> RawMeter { - debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); + debug_assert!(matches!(self.contract_state(), ContractState::Alive)); // If a special limit is specified higher than it is available, // we want to enforce the lesser limit to the nested meter, to fail in the sub-call. let limit = self.available().min(limit); @@ -440,7 +440,7 @@ where contract_info: &mut ContractInfo, code_info: &CodeInfo, ) -> Result, DispatchError> { - debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); + debug_assert!(matches!(self.contract_state(), ContractState::Alive)); let ed = Pallet::::min_balance(); let deposit = contract_info.update_base_deposit(&code_info); @@ -481,7 +481,7 @@ where /// `contract_info` will be refunded to the `origin` of the meter. And the free /// (`reducible_balance`) will be sent to the `beneficiary`. pub fn terminate(&mut self, info: &ContractInfo, beneficiary: T::AccountId) { - debug_assert!(matches!(self.own_contribution, Contribution::Alive(_))); + debug_assert!(matches!(self.contract_state(), ContractState::Alive)); self.own_contribution = Contribution::Terminated { deposit: Deposit::Refund(info.total_deposit()), beneficiary, @@ -504,7 +504,7 @@ where let deposit = self.own_contribution.update_contract(info); let total_deposit = self.total_deposit.saturating_add(&deposit); // We don't want to override a `Terminated` with a `Checked`. - if matches!(self.own_contribution, Contribution::Alive(_)) { + if matches!(self.contract_state(), ContractState::Alive) { self.own_contribution = Contribution::Checked(deposit); } if let Deposit::Charge(amount) = total_deposit {