contracts: Remove OnKilledAccount implementation (#5397)

* contracts: Remove OnKilledAccount implementation

Contracts now longer rely on this callback to tell them when they
are removed. Instead, they can only self destruct  using `ext_terminate`.

* Fix account removal test

* Fix account storage removal

substrate/frame/contracts/src/account_db.rs

@@ -146,16 +146,8 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 		let mut total_imbalance = SignedImbalance::zero();
 		for (address, changed) in s.into_iter() {
 			if let Some(balance) = changed.balance() {
-				let existed  = !T::Currency::total_balance(&address).is_zero();
 				let imbalance = T::Currency::make_free_balance_be(&address, balance);
-				let exists  = !T::Currency::total_balance(&address).is_zero();
 				total_imbalance = total_imbalance.merge(imbalance);
-				if existed && !exists {
-					// Account killed. This will ultimately lead to calling `OnKilledAccount` callback
-					// which will make removal of CodeHashOf and AccountStorage for this account.
-					// In order to avoid writing over the deleted properties we `continue` here.
-					continue;
-				}
 			}
 
 			if changed.code_hash().is_some()

substrate/frame/contracts/src/lib.rs

@@ -125,7 +125,7 @@ use frame_support::{
 	parameter_types, IsSubType,
 	weights::DispatchInfo,
 };
-use frame_support::traits::{OnKilledAccount, OnUnbalanced, Currency, Get, Time, Randomness};
+use frame_support::traits::{OnUnbalanced, Currency, Get, Time, Randomness};
 use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root};
 use sp_core::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX;
 use pallet_contracts_primitives::{RentProjection, ContractAccessError};
@@ -941,18 +941,6 @@ decl_storage! {
 	}
 }
 
-// TODO: this should be removed in favour of a self-destruct contract host function allowing the
-// contract to delete all storage and the `ContractInfoOf` key and transfer remaining balance to
-// some other account. As it stands, it's an economic insecurity on any smart-contract chain.
-// https://github.com/paritytech/substrate/issues/4952
-impl<T: Trait> OnKilledAccount<T::AccountId> for Module<T> {
-	fn on_killed_account(who: &T::AccountId) {
-		if let Some(ContractInfo::Alive(info)) = <ContractInfoOf<T>>::take(who) {
-			child::kill_storage(&info.trie_id, info.child_trie_unique_id());
-		}
-	}
-}
-
 /// In-memory cache of configuration values.
 ///
 /// We assume that these values can't be changed in the

substrate/frame/contracts/src/tests.rs

@@ -117,7 +117,7 @@ impl frame_system::Trait for Test {
 	type ModuleToIndex = ();
 	type AccountData = pallet_balances::AccountData<u64>;
 	type OnNewAccount = ();
-	type OnKilledAccount = Contracts;
+	type OnKilledAccount = ();
 }
 impl pallet_balances::Trait for Test {
 	type Balance = u64;
@@ -308,7 +308,7 @@ fn refunds_unused_gas() {
 }
 
 #[test]
-fn account_removal_removes_storage() {
+fn account_removal_does_not_remove_storage() {
 	ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
 		let trie_id1 = <Test as Trait>::TrieIdGenerator::trie_id(&1);
 		let trie_id2 = <Test as Trait>::TrieIdGenerator::trie_id(&2);
@@ -351,14 +351,22 @@ fn account_removal_removes_storage() {
 		// Transfer funds from account 1 of such amount that after this transfer
 		// the balance of account 1 will be below the existential threshold.
 		//
-		// This should lead to the removal of all storage associated with this account.
+		// This does not remove the contract storage as we are not notified about a
+		// account removal. This cannot happen in reality because a contract can only
+		// remove itself by `ext_terminate`. There is no external event that can remove
+		// the account appart from that.
 		assert_ok!(Balances::transfer(Origin::signed(1), 2, 20));
 
-		// Verify that all entries from account 1 is removed, while
-		// entries from account 2 is in place.
+		// Verify that no entries are removed.
 		{
-			assert!(<dyn AccountDb<Test>>::get_storage(&DirectAccountDb, &1, Some(&trie_id1), key1).is_none());
-			assert!(<dyn AccountDb<Test>>::get_storage(&DirectAccountDb, &1, Some(&trie_id1), key2).is_none());
+			assert_eq!(
+				<dyn AccountDb<Test>>::get_storage(&DirectAccountDb, &1, Some(&trie_id1), key1),
+				Some(b"1".to_vec())
+			);
+			assert_eq!(
+				<dyn AccountDb<Test>>::get_storage(&DirectAccountDb, &1, Some(&trie_id1), key2),
+				Some(b"2".to_vec())
+			);
 
 			assert_eq!(
 				<dyn AccountDb<Test>>::get_storage(&DirectAccountDb, &2, Some(&trie_id2), key1),