Contracts: Remove ED from base deposit (#3536)

- Update internal logic so that the storage_base_deposit does not
include ED
- add v16 migration to update ContractInfo struct with this change

Before:
<img width="820" alt="Screenshot 2024-03-21 at 11 23 29"
src="https://github.com/paritytech/polkadot-sdk/assets/521091/a0a8df0d-e743-42c5-9e16-cf2ec1aa949c">

After:
![Screenshot 2024-03-21 at 11 23
42](https://github.com/paritytech/polkadot-sdk/assets/521091/593235b0-b866-4915-b653-2071d793228b)

---------

Co-authored-by: Cyrill Leutwiler <cyrill@parity.io>
Co-authored-by: command-bot <>

substrate/frame/contracts/src/benchmarking/mod.rs

@@ -32,7 +32,7 @@ use self::{
 use crate::{
 	exec::Key,
 	migration::{
-		codegen::LATEST_MIGRATION_VERSION, v09, v10, v11, v12, v13, v14, v15, MigrationStep,
+		codegen::LATEST_MIGRATION_VERSION, v09, v10, v11, v12, v13, v14, v15, v16, MigrationStep,
 	},
 	Pallet as Contracts, *,
 };
@@ -331,6 +331,26 @@ mod benchmarks {
 		Ok(())
 	}
 
+	// This benchmarks the v16 migration step (Remove ED from base_deposit).
+	#[benchmark(pov_mode = Measured)]
+	fn v16_migration_step() -> Result<(), BenchmarkError> {
+		let contract =
+			<Contract<T>>::with_caller(whitelisted_caller(), WasmModule::dummy(), vec![])?;
+
+		let info = contract.info()?;
+		let base_deposit = v16::store_old_contract_info::<T>(contract.account_id.clone(), &info);
+		let mut m = v16::Migration::<T>::default();
+
+		#[block]
+		{
+			m.step(&mut WeightMeter::new());
+		}
+		let ed = Pallet::<T>::min_balance();
+		let info = v16::ContractInfoOf::<T>::get(&contract.account_id).unwrap();
+		assert_eq!(info.storage_base_deposit, base_deposit - ed);
+		Ok(())
+	}
+
 	// This benchmarks the weight of executing Migration::migrate to execute a noop migration.
 	#[benchmark(pov_mode = Measured)]
 	fn migration_noop() {

substrate/frame/contracts/src/lib.rs

@@ -244,7 +244,7 @@ pub mod pallet {
 	use sp_runtime::Perbill;
 
 	/// The in-code storage version.
-	pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(15);
+	pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(16);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]

substrate/frame/contracts/src/migration.rs

@@ -64,6 +64,7 @@ pub mod v12;
 pub mod v13;
 pub mod v14;
 pub mod v15;
+pub mod v16;
 include!(concat!(env!("OUT_DIR"), "/migration_codegen.rs"));
 
 use crate::{weights::WeightInfo, Config, Error, MigrationInProgress, Pallet, Weight, LOG_TARGET};

substrate/frame/contracts/src/migration/v16.rs

@@ -0,0 +1,107 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Remove ED from storage base deposit.
+//! See <https://github.com/paritytech/polkadot-sdk/pull/3536>.
+
+use crate::{
+	migration::{IsFinished, MigrationStep},
+	weights::WeightInfo,
+	BalanceOf, CodeHash, Config, Pallet, TrieId, Weight, WeightMeter, LOG_TARGET,
+};
+use codec::{Decode, Encode};
+use frame_support::{pallet_prelude::*, storage_alias, DefaultNoBound};
+use sp_runtime::{BoundedBTreeMap, Saturating};
+use sp_std::prelude::*;
+
+#[cfg(feature = "runtime-benchmarks")]
+pub fn store_old_contract_info<T: Config>(
+	account: T::AccountId,
+	info: &crate::ContractInfo<T>,
+) -> BalanceOf<T> {
+	let storage_base_deposit = Pallet::<T>::min_balance() + 1u32.into();
+	ContractInfoOf::<T>::insert(
+		account,
+		ContractInfo {
+			trie_id: info.trie_id.clone(),
+			code_hash: info.code_hash,
+			storage_bytes: Default::default(),
+			storage_items: Default::default(),
+			storage_byte_deposit: Default::default(),
+			storage_item_deposit: Default::default(),
+			storage_base_deposit,
+			delegate_dependencies: Default::default(),
+		},
+	);
+
+	storage_base_deposit
+}
+
+#[storage_alias]
+pub type ContractInfoOf<T: Config> =
+	StorageMap<Pallet<T>, Twox64Concat, <T as frame_system::Config>::AccountId, ContractInfo<T>>;
+
+#[derive(Encode, Decode, CloneNoBound, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+#[scale_info(skip_type_params(T))]
+pub struct ContractInfo<T: Config> {
+	trie_id: TrieId,
+	code_hash: CodeHash<T>,
+	storage_bytes: u32,
+	storage_items: u32,
+	storage_byte_deposit: BalanceOf<T>,
+	storage_item_deposit: BalanceOf<T>,
+	pub storage_base_deposit: BalanceOf<T>,
+	delegate_dependencies: BoundedBTreeMap<CodeHash<T>, BalanceOf<T>, T::MaxDelegateDependencies>,
+}
+
+#[derive(Encode, Decode, MaxEncodedLen, DefaultNoBound)]
+pub struct Migration<T: Config> {
+	last_account: Option<T::AccountId>,
+}
+
+impl<T: Config> MigrationStep for Migration<T> {
+	const VERSION: u16 = 16;
+
+	fn max_step_weight() -> Weight {
+		T::WeightInfo::v16_migration_step()
+	}
+
+	fn step(&mut self, meter: &mut WeightMeter) -> IsFinished {
+		let mut iter = if let Some(last_account) = self.last_account.take() {
+			ContractInfoOf::<T>::iter_keys_from(ContractInfoOf::<T>::hashed_key_for(last_account))
+		} else {
+			ContractInfoOf::<T>::iter_keys()
+		};
+
+		if let Some(key) = iter.next() {
+			log::debug!(target: LOG_TARGET, "Migrating contract {:?}", key);
+			ContractInfoOf::<T>::mutate(key.clone(), |info| {
+				let ed = Pallet::<T>::min_balance();
+				let mut updated_info = info.take().expect("Item exists; qed");
+				updated_info.storage_base_deposit.saturating_reduce(ed);
+				*info = Some(updated_info);
+			});
+			self.last_account = Some(key);
+			meter.consume(T::WeightInfo::v16_migration_step());
+			IsFinished::No
+		} else {
+			log::debug!(target: LOG_TARGET, "No more contracts to migrate");
+			meter.consume(T::WeightInfo::v16_migration_step());
+			IsFinished::Yes
+		}
+	}
+}

substrate/frame/contracts/src/storage.rs

@@ -23,7 +23,7 @@ use crate::{
 	exec::{AccountIdOf, Key},
 	weights::WeightInfo,
 	BalanceOf, CodeHash, CodeInfo, Config, ContractInfoOf, DeletionQueue, DeletionQueueCounter,
-	Error, Pallet, TrieId, SENTINEL,
+	Error, TrieId, SENTINEL,
 };
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
@@ -125,9 +125,7 @@ impl<T: Config> ContractInfo<T> {
 
 	/// Same as [`Self::extra_deposit`] but including the base deposit.
 	pub fn total_deposit(&self) -> BalanceOf<T> {
-		self.extra_deposit()
-			.saturating_add(self.storage_base_deposit)
-			.saturating_sub(Pallet::<T>::min_balance())
+		self.extra_deposit().saturating_add(self.storage_base_deposit)
 	}
 
 	/// Returns the storage base deposit of the contract.
@@ -213,7 +211,6 @@ impl<T: Config> ContractInfo<T> {
 	/// The base deposit is updated when the `code_hash` of the contract changes, as it depends on
 	/// the deposit paid to upload the contract's code.
 	pub fn update_base_deposit(&mut self, code_info: &CodeInfo<T>) -> BalanceOf<T> {
-		let ed = Pallet::<T>::min_balance();
 		let info_deposit =
 			Diff { bytes_added: self.encoded_size() as u32, items_added: 1, ..Default::default() }
 				.update_contract::<T>(None)
@@ -224,11 +221,7 @@ impl<T: Config> ContractInfo<T> {
 		// to prevent abuse.
 		let upload_deposit = T::CodeHashLockupDepositPercent::get().mul_ceil(code_info.deposit());
 
-		// Instantiate needs to transfer at least the minimum balance in order to pull the
-		// contract's own account into existence, as the deposit itself does not contribute to the
-		// `ed`.
-		let deposit = info_deposit.saturating_add(upload_deposit).saturating_add(ed);
-
+		let deposit = info_deposit.saturating_add(upload_deposit);
 		self.storage_base_deposit = deposit;
 		deposit
 	}

substrate/frame/contracts/src/storage/meter.rs

@@ -435,22 +435,12 @@ where
 		contract: &T::AccountId,
 		contract_info: &mut ContractInfo<T>,
 		code_info: &CodeInfo<T>,
-	) -> Result<DepositOf<T>, DispatchError> {
+	) -> Result<(), DispatchError> {
 		debug_assert!(matches!(self.contract_state(), ContractState::Alive));
-		let ed = Pallet::<T>::min_balance();
-
-		let deposit = contract_info.update_base_deposit(&code_info);
-		if deposit > self.limit {
-			return Err(<Error<T>>::StorageDepositLimitExhausted.into())
-		}
-
-		let deposit = Deposit::Charge(deposit);
-
-		// We do not increase `own_contribution` because this will be charged later when the
-		// contract execution does conclude and hence would lead to a double charge.
-		self.total_deposit = Deposit::Charge(ed);
 
 		// We need to make sure that the contract's account exists.
+		let ed = Pallet::<T>::min_balance();
+		self.total_deposit = Deposit::Charge(ed);
 		T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;
 
 		// A consumer is added at account creation and removed it on termination, otherwise the
@@ -458,9 +448,11 @@ where
 		// With the consumer, a correct runtime cannot remove the account.
 		System::<T>::inc_consumers(contract)?;
 
-		self.charge_deposit(contract.clone(), deposit.saturating_sub(&Deposit::Charge(ed)));
+		let deposit = contract_info.update_base_deposit(&code_info);
+		let deposit = Deposit::Charge(deposit);
 
-		Ok(deposit)
+		self.charge_deposit(contract.clone(), deposit);
+		Ok(())
 	}
 
 	/// Call to tell the meter that the currently executing contract was terminated.
@@ -859,14 +851,14 @@ mod tests {
 		let test_cases = vec![
 			ChargingTestCase {
 				origin: Origin::<Test>::from_account_id(ALICE),
-				deposit: Deposit::Refund(107),
+				deposit: Deposit::Refund(108),
 				expected: TestExt {
 					limit_checks: vec![LimitCheck { origin: ALICE, limit: 1_000, min_leftover: 0 }],
 					charges: vec![
 						Charge {
 							origin: ALICE,
 							contract: CHARLIE,
-							amount: Deposit::Refund(119),
+							amount: Deposit::Refund(120),
 							state: ContractState::Terminated { beneficiary: CHARLIE },
 						},
 						Charge {
@@ -915,7 +907,6 @@ mod tests {
 
 			meter.absorb(nested0, &BOB, None);
 			assert_eq!(meter.try_into_deposit(&test_case.origin).unwrap(), test_case.deposit);
-
 			assert_eq!(TestExtTestValue::get(), test_case.expected)
 		}
 	}

substrate/frame/contracts/src/tests.rs

@@ -3817,7 +3817,7 @@ fn locking_delegate_dependency_works() {
 				&HoldReason::StorageDepositReserve.into(),
 				&addr_caller
 			),
-			dependency_deposit + contract.storage_base_deposit() - ED
+			dependency_deposit + contract.storage_base_deposit()
 		);
 
 		// Removing the code should fail, since we have added a dependency.
@@ -3856,7 +3856,7 @@ fn locking_delegate_dependency_works() {
 				&HoldReason::StorageDepositReserve.into(),
 				&addr_caller
 			),
-			contract.storage_base_deposit() - ED
+			contract.storage_base_deposit()
 		);
 
 		// Removing a nonexistent dependency should fail.
@@ -3888,7 +3888,7 @@ fn locking_delegate_dependency_works() {
 		assert_ok!(call(&addr_caller, &terminate_input).result);
 		assert_eq!(
 			test_utils::get_balance(&ALICE),
-			balance_before + contract.storage_base_deposit() + dependency_deposit
+			ED + balance_before + contract.storage_base_deposit() + dependency_deposit
 		);
 
 		// Terminate should also remove the dependency, so we can remove the code.
@@ -3904,13 +3904,9 @@ fn native_dependency_deposit_works() {
 	// Set hash lock up deposit to 30%, to test deposit calculation.
 	CODE_HASH_LOCKUP_DEPOSIT_PERCENT.with(|c| *c.borrow_mut() = Perbill::from_percent(30));
 
-	// Set a low existential deposit so that the base storage deposit is based on the contract
-	// storage deposit rather than the existential deposit.
-	const ED: u64 = 10;
-
 	// Test with both existing and uploaded code
 	for code in [Code::Upload(wasm.clone()), Code::Existing(code_hash)] {
-		ExtBuilder::default().existential_deposit(ED).build().execute_with(|| {
+		ExtBuilder::default().build().execute_with(|| {
 			let _ = Balances::set_balance(&ALICE, 1_000_000);
 			let lockup_deposit_percent = CodeHashLockupDepositPercent::get();
 
@@ -3942,16 +3938,16 @@ fn native_dependency_deposit_works() {
 			let res = builder::bare_instantiate(code).build();
 
 			let addr = res.result.unwrap().account_id;
-			let base_deposit = ED + test_utils::contract_info_storage_deposit(&addr);
+			let base_deposit = test_utils::contract_info_storage_deposit(&addr);
 			let upload_deposit = test_utils::get_code_deposit(&code_hash);
 			let extra_deposit = add_upload_deposit.then(|| upload_deposit).unwrap_or_default();
 
 			// Check initial storage_deposit
-			// The base deposit should be: ED + contract_info_storage_deposit + 30% * deposit
+			// The base deposit should be: contract_info_storage_deposit + 30% * deposit
 			let deposit =
 				extra_deposit + base_deposit + lockup_deposit_percent.mul_ceil(upload_deposit);
 
-			assert_eq!(res.storage_deposit.charge_or_zero(), deposit);
+			assert_eq!(res.storage_deposit.charge_or_zero(), deposit + Contracts::min_balance());
 
 			// call set_code_hash
 			builder::bare_call(addr.clone())
@@ -3962,9 +3958,10 @@ fn native_dependency_deposit_works() {
 			let code_deposit = test_utils::get_code_deposit(&dummy_code_hash);
 			let deposit = base_deposit + lockup_deposit_percent.mul_ceil(code_deposit);
 			assert_eq!(test_utils::get_contract(&addr).storage_base_deposit(), deposit);
+
 			assert_eq!(
 				test_utils::get_balance_on_hold(&HoldReason::StorageDepositReserve.into(), &addr),
-				deposit - ED
+				deposit
 			);
 		});
 	}

substrate/frame/contracts/src/weights.rs

@@ -58,6 +58,7 @@ pub trait WeightInfo {
 	fn v13_migration_step() -> Weight;
 	fn v14_migration_step() -> Weight;
 	fn v15_migration_step() -> Weight;
+	fn v16_migration_step() -> Weight;
 	fn migration_noop() -> Weight;
 	fn migrate() -> Weight;
 	fn on_runtime_upgrade_noop() -> Weight;
@@ -271,6 +272,17 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().reads(4_u64))
 			.saturating_add(T::DbWeight::get().writes(2_u64))
 	}
+	/// Storage: `Contracts::ContractInfoOf` (r:2 w:1)
+	/// Proof: `Contracts::ContractInfoOf` (`max_values`: None, `max_size`: Some(1795), added: 4270, mode: `Measured`)
+	fn v16_migration_step() -> Weight {
+		// Proof Size summary in bytes:
+		//  Measured:  `409`
+		//  Estimated: `6349`
+		// Minimum execution time: 12_595_000 picoseconds.
+		Weight::from_parts(13_059_000, 6349)
+			.saturating_add(T::DbWeight::get().reads(2_u64))
+			.saturating_add(T::DbWeight::get().writes(1_u64))
+	}
 	/// Storage: `Contracts::MigrationInProgress` (r:1 w:1)
 	/// Proof: `Contracts::MigrationInProgress` (`max_values`: Some(1), `max_size`: Some(1026), added: 1521, mode: `Measured`)
 	fn migration_noop() -> Weight {
@@ -1540,6 +1552,17 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().reads(4_u64))
 			.saturating_add(RocksDbWeight::get().writes(2_u64))
 	}
+	/// Storage: `Contracts::ContractInfoOf` (r:2 w:1)
+	/// Proof: `Contracts::ContractInfoOf` (`max_values`: None, `max_size`: Some(1795), added: 4270, mode: `Measured`)
+	fn v16_migration_step() -> Weight {
+		// Proof Size summary in bytes:
+		//  Measured:  `409`
+		//  Estimated: `6349`
+		// Minimum execution time: 12_595_000 picoseconds.
+		Weight::from_parts(13_059_000, 6349)
+			.saturating_add(RocksDbWeight::get().reads(2_u64))
+			.saturating_add(RocksDbWeight::get().writes(1_u64))
+	}
 	/// Storage: `Contracts::MigrationInProgress` (r:1 w:1)
 	/// Proof: `Contracts::MigrationInProgress` (`max_values`: Some(1), `max_size`: Some(1026), added: 1521, mode: `Measured`)
 	fn migration_noop() -> Weight {