Transfer to beneficiary after transfer_on_hold

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,14 +1283,8 @@ where
 		}
 		let frame = self.top_frame_mut();
 		let info = frame.terminate();
-		frame.nested_storage.terminate(&info);
-		System::<T>::dec_consumers(&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.clone());
+
 		info.queue_trie_for_deletion();
 		ContractInfoOf::<T>::remove(&frame.account_id);
 		E::decrement_refcount(info.code_hash);

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::{
@@ -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<T> = Deposit<BalanceOf<T>>;
@@ -88,7 +88,7 @@ pub trait Ext<T: Config> {
 		origin: &T::AccountId,
 		contract: &T::AccountId,
 		amount: &DepositOf<T>,
-		terminated: bool,
+		status: &ContractStatus<T>,
 	) -> Result<(), DispatchError>;
 }
 
@@ -224,6 +224,27 @@ impl Diff {
 	}
 }
 
+/// The beneficiary of a contract termination.
+#[derive(RuntimeDebugNoBound, Clone, PartialEq, Eq)]
+pub struct Beneficiary<T: Config>(AccountIdOf<T>);
+
+impl<T: Config> Deref for Beneficiary<T> {
+	type Target = AccountIdOf<T>;
+
+	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<T: Config> {
+	Alive,
+	Terminated(Beneficiary<T>),
+}
+
 /// 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 +258,7 @@ impl Diff {
 struct Charge<T: Config> {
 	contract: T::AccountId,
 	amount: DepositOf<T>,
-	terminated: bool,
+	status: ContractStatus<T>,
 }
 
 /// Records the storage changes of a storage meter.
@@ -250,15 +271,15 @@ enum Contribution<T: Config> {
 	Checked(DepositOf<T>),
 	/// The contract was terminated. In this process the [`Diff`] was converted into a [`Deposit`]
 	/// in order to calculate the refund.
-	Terminated(DepositOf<T>),
+	Terminated(DepositOf<T>, Beneficiary<T>),
 }
 
 impl<T: Config> Contribution<T> {
 	/// See [`Diff::update_contract`].
 	fn update_contract(&self, info: Option<&mut ContractInfo<T>>) -> DepositOf<T> {
 		match self {
 			Self::Alive(diff) => diff.update_contract::<T>(info),
-			Self::Terminated(deposit) | Self::Checked(deposit) => deposit.clone(),
+			Self::Terminated(deposit, _) | Self::Checked(deposit) => deposit.clone(),
 		}
 	}
 }
@@ -325,7 +346,7 @@ where
 			self.charges.push(Charge {
 				contract: contract.clone(),
 				amount: own_deposit,
-				terminated: absorbed.is_terminated(),
+				status: absorbed.is_terminated(),
 			});
 		}
 	}
@@ -341,8 +362,12 @@ where
 	}
 
 	/// True if the contract is terminated.
-	fn is_terminated(&self) -> bool {
-		matches!(self.own_contribution, Contribution::Terminated(_))
+	fn is_terminated(&self) -> ContractStatus<T> {
+		match &self.own_contribution {
+			Contribution::Terminated(_, beneficiary) =>
+				ContractStatus::Terminated(beneficiary.clone()),
+			_ => ContractStatus::Alive,
+		}
 	}
 }
 
@@ -386,10 +411,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 +442,7 @@ where
 	/// deposit charge separately from the storage charge.
 	pub fn charge_deposit(&mut self, contract: T::AccountId, amount: DepositOf<T>) {
 		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,23 +472,35 @@ 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 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.
 		System::<T>::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)
 	}
 
 	/// 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<T>) {
+	/// `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<T>, 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(beneficiary),
+		);
 	}
 
 	/// [`Self::charge`] does not enforce the storage limit since we want to do this check as late
@@ -532,7 +569,7 @@ impl<T: Config> Ext<T> for ReservingExt {
 		origin: &T::AccountId,
 		contract: &T::AccountId,
 		amount: &DepositOf<T>,
-		terminated: bool,
+		status: &ContractStatus<T>,
 	) -> Result<(), DispatchError> {
 		match amount {
 			Deposit::Charge(amount) | Deposit::Refund(amount) if amount.is_zero() => return Ok(()),
@@ -590,8 +627,15 @@ impl<T: Config> Ext<T> for ReservingExt {
 				}
 			},
 		}
-		if terminated {
+		if let ContractStatus::<T>::Terminated(beneficiary) = status {
 			System::<T>::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(())
 	}
@@ -631,7 +675,7 @@ mod tests {
 		origin: AccountIdOf<Test>,
 		contract: AccountIdOf<Test>,
 		amount: DepositOf<Test>,
-		terminated: bool,
+		status: ContractStatus<Test>,
 	}
 
 	#[derive(Default, Debug, PartialEq, Eq, Clone)]
@@ -665,14 +709,14 @@ mod tests {
 			origin: &AccountIdOf<Test>,
 			contract: &AccountIdOf<Test>,
 			amount: &DepositOf<Test>,
-			terminated: bool,
+			status: &ContractStatus<Test>,
 		) -> Result<(), DispatchError> {
 			TestExtTestValue::mutate(|ext| {
 				ext.charges.push(Charge {
 					origin: origin.clone(),
 					contract: contract.clone(),
 					amount: amount.clone(),
-					terminated,
+					status: status.clone(),
 				})
 			});
 			Ok(())
@@ -759,19 +803,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,
 						},
 					],
 				},
@@ -850,13 +894,13 @@ mod tests {
 							origin: ALICE,
 							contract: CHARLIE,
 							amount: Deposit::Refund(119),
-							terminated: true,
+							status: ContractStatus::Terminated(Beneficiary(CHARLIE)),
 						},
 						Charge {
 							origin: ALICE,
 							contract: BOB,
 							amount: Deposit::Charge(12),
-							terminated: false,
+							status: ContractStatus::Alive,
 						},
 					],
 				},
@@ -892,7 +936,7 @@ mod tests {
 			let mut nested1 = nested0.nested(BalanceOf::<Test>::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);
 

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![],
+				},
 			],
 		);
 	});