contracts: Refactor instantiate with code (paritytech#14503)

* wip

* fixes

* rm comment

* join fns

* clippy

* Fix limits

* reduce diff

* fix

* fix

* fix typo

* refactor store to  use self

* refactor run to take self by value

* pass tests

* rm comment

* fixes

* fix typo

* rm

* fix fmt

* clippy

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* Update frame/contracts/src/lib.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Update frame/contracts/src/wasm/mod.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Update frame/contracts/src/wasm/mod.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* PR review, rm duplicate increment_refcount

* PR review

* Update frame/contracts/src/wasm/prepare.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Add test for failing storage_deposit

* fix lint

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

---------

Co-authored-by: command-bot <>
Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

Author: pgherveou

frame/contracts/primitives/src/lib.rs

@@ -161,12 +161,6 @@ pub enum Code<Hash> {
 	Existing(Hash),
 }
 
-impl<T: Into<Vec<u8>>, Hash> From<T> for Code<Hash> {
-	fn from(from: T) -> Self {
-		Code::Upload(from.into())
-	}
-}
-
 /// The amount of balance that was either charged or refunded in order to pay for storage.
 #[derive(Eq, PartialEq, Ord, PartialOrd, Encode, Decode, RuntimeDebug, Clone, TypeInfo)]
 pub enum StorageDeposit<Balance> {

frame/contracts/src/lib.rs

@@ -75,8 +75,8 @@
 //!   allowed to code owner.
 //! * [`Pallet::set_code`] - Changes the code of an existing contract. Only allowed to `Root`
 //!   origin.
-//! * [`Pallet::migrate`] - Runs migration steps of curent multi-block migration in priority, before
-//!   [`Hooks::on_idle`][frame_support::traits::Hooks::on_idle] activates.
+//! * [`Pallet::migrate`] - Runs migration steps of current multi-block migration in priority,
+//!   before [`Hooks::on_idle`][frame_support::traits::Hooks::on_idle] activates.
 //!
 //! ## Usage
 //!
@@ -105,7 +105,7 @@ use crate::{
 	exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, Stack as ExecStack},
 	gas::GasMeter,
 	storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager},
-	wasm::{CodeInfo, TryInstantiate, WasmBlob},
+	wasm::{CodeInfo, WasmBlob},
 };
 use codec::{Codec, Decode, Encode, HasCompact};
 use environmental::*;
@@ -695,24 +695,40 @@ pub mod pallet {
 			salt: Vec<u8>,
 		) -> DispatchResultWithPostInfo {
 			Migration::<T>::ensure_migrated()?;
+			let origin = ensure_signed(origin)?;
 			let code_len = code.len() as u32;
+
+			let (module, upload_deposit) = Self::try_upload_code(
+				origin.clone(),
+				code,
+				storage_deposit_limit.clone().map(Into::into),
+				Determinism::Enforced,
+				None,
+			)?;
+
+			// Reduces the storage deposit limit by the amount that was reserved for the upload.
+			let storage_deposit_limit =
+				storage_deposit_limit.map(|limit| limit.into().saturating_sub(upload_deposit));
+
 			let data_len = data.len() as u32;
 			let salt_len = salt.len() as u32;
 			let common = CommonInput {
-				origin: Origin::from_runtime_origin(origin)?,
+				origin: Origin::from_account_id(origin),
 				value,
 				data,
 				gas_limit,
-				storage_deposit_limit: storage_deposit_limit.map(Into::into),
+				storage_deposit_limit,
 				debug_message: None,
 			};
+
 			let mut output =
-				InstantiateInput::<T> { code: Code::Upload(code), salt }.run_guarded(common);
+				InstantiateInput::<T> { code: WasmCode::Wasm(module), salt }.run_guarded(common);
 			if let Ok(retval) = &output.result {
 				if retval.1.did_revert() {
 					output.result = Err(<Error<T>>::ContractReverted.into());
 				}
 			}
+
 			output.gas_meter.into_dispatch_result(
 				output.result.map(|(_address, result)| result),
 				T::WeightInfo::instantiate_with_code(code_len, data_len, salt_len),
@@ -748,8 +764,8 @@ pub mod pallet {
 				storage_deposit_limit: storage_deposit_limit.map(Into::into),
 				debug_message: None,
 			};
-			let mut output =
-				InstantiateInput::<T> { code: Code::Existing(code_hash), salt }.run_guarded(common);
+			let mut output = InstantiateInput::<T> { code: WasmCode::CodeHash(code_hash), salt }
+				.run_guarded(common);
 			if let Ok(retval) = &output.result {
 				if retval.1.did_revert() {
 					output.result = Err(<Error<T>>::ContractReverted.into());
@@ -1052,9 +1068,15 @@ struct CallInput<T: Config> {
 	determinism: Determinism,
 }
 
+/// Reference to an existing code hash or a new wasm module.
+enum WasmCode<T: Config> {
+	Wasm(WasmBlob<T>),
+	CodeHash(CodeHash<T>),
+}
+
 /// Input specific to a contract instantiation invocation.
 struct InstantiateInput<T: Config> {
-	code: Code<CodeHash<T>>,
+	code: WasmCode<T>,
 	salt: Vec<u8>,
 }
 
@@ -1099,7 +1121,7 @@ struct InternalOutput<T: Config, O> {
 
 /// Helper trait to wrap contract execution entry points into a single function
 /// [`Invokable::run_guarded`].
-trait Invokable<T: Config> {
+trait Invokable<T: Config>: Sized {
 	/// What is returned as a result of a successful invocation.
 	type Output;
 
@@ -1111,7 +1133,7 @@ trait Invokable<T: Config> {
 	///
 	/// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a
 	/// global reference.
-	fn run_guarded(&self, common: CommonInput<T>) -> InternalOutput<T, Self::Output> {
+	fn run_guarded(self, common: CommonInput<T>) -> InternalOutput<T, Self::Output> {
 		// Set up a global reference to the boolean flag used for the re-entrancy guard.
 		environmental!(executing_contract: bool);
 
@@ -1159,11 +1181,8 @@ trait Invokable<T: Config> {
 	/// contract or a instantiation of a new one.
 	///
 	/// Called by dispatchables and public functions through the [`Invokable::run_guarded`].
-	fn run(
-		&self,
-		common: CommonInput<T>,
-		gas_meter: GasMeter<T>,
-	) -> InternalOutput<T, Self::Output>;
+	fn run(self, common: CommonInput<T>, gas_meter: GasMeter<T>)
+		-> InternalOutput<T, Self::Output>;
 
 	/// This method ensures that the given `origin` is allowed to invoke the current `Invokable`.
 	///
@@ -1175,7 +1194,7 @@ impl<T: Config> Invokable<T> for CallInput<T> {
 	type Output = ExecReturnValue;
 
 	fn run(
-		&self,
+		self,
 		common: CommonInput<T>,
 		mut gas_meter: GasMeter<T>,
 	) -> InternalOutput<T, Self::Output> {
@@ -1201,7 +1220,7 @@ impl<T: Config> Invokable<T> for CallInput<T> {
 			value,
 			data.clone(),
 			debug_message,
-			*determinism,
+			determinism,
 		);
 
 		match storage_meter.try_into_deposit(&origin) {
@@ -1223,8 +1242,8 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 	type Output = (AccountIdOf<T>, ExecReturnValue);
 
 	fn run(
-		&self,
-		mut common: CommonInput<T>,
+		self,
+		common: CommonInput<T>,
 		mut gas_meter: GasMeter<T>,
 	) -> InternalOutput<T, Self::Output> {
 		let mut storage_deposit = Default::default();
@@ -1233,37 +1252,15 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 			let InstantiateInput { salt, .. } = self;
 			let CommonInput { origin: contract_origin, .. } = common;
 			let origin = contract_origin.account_id()?;
-			let (extra_deposit, executable) = match &self.code {
-				Code::Upload(binary) => {
-					let executable = WasmBlob::from_code(
-						binary.clone(),
-						&schedule,
-						origin.clone(),
-						Determinism::Enforced,
-						TryInstantiate::Skip,
-					)
-					.map_err(|(err, msg)| {
-						common
-							.debug_message
-							.as_mut()
-							.map(|buffer| buffer.try_extend(&mut msg.bytes()));
-						err
-					})?;
-					// The open deposit will be charged during execution when the
-					// uploaded module does not already exist. This deposit is not part of the
-					// storage meter because it is not transferred to the contract but
-					// reserved on the uploading account.
-					(executable.open_deposit(&executable.code_info()), executable)
-				},
-				Code::Existing(hash) =>
-					(Default::default(), WasmBlob::from_storage(*hash, &mut gas_meter)?),
+
+			let executable = match self.code {
+				WasmCode::Wasm(module) => module,
+				WasmCode::CodeHash(code_hash) => WasmBlob::from_storage(code_hash, &mut gas_meter)?,
 			};
+
 			let contract_origin = Origin::from_account_id(origin.clone());
-			let mut storage_meter = StorageMeter::new(
-				&contract_origin,
-				common.storage_deposit_limit,
-				common.value.saturating_add(extra_deposit),
-			)?;
+			let mut storage_meter =
+				StorageMeter::new(&contract_origin, common.storage_deposit_limit, common.value)?;
 			let CommonInput { value, data, debug_message, .. } = common;
 			let result = ExecStack::<T, WasmBlob<T>>::run_instantiate(
 				origin.clone(),
@@ -1277,9 +1274,7 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 				debug_message,
 			);
 
-			storage_deposit = storage_meter
-				.try_into_deposit(&contract_origin)?
-				.saturating_add(&StorageDeposit::Charge(extra_deposit));
+			storage_deposit = storage_meter.try_into_deposit(&contract_origin)?;
 			result
 		};
 		InternalOutput { result: try_exec(), gas_meter, storage_deposit }
@@ -1383,7 +1378,7 @@ impl<T: Config> Pallet<T> {
 		origin: T::AccountId,
 		value: BalanceOf<T>,
 		gas_limit: Weight,
-		storage_deposit_limit: Option<BalanceOf<T>>,
+		mut storage_deposit_limit: Option<BalanceOf<T>>,
 		code: Code<CodeHash<T>>,
 		data: Vec<u8>,
 		salt: Vec<u8>,
@@ -1397,6 +1392,45 @@ impl<T: Config> Pallet<T> {
 		} else {
 			None
 		};
+		// collect events if CollectEvents is UnsafeCollect
+		let events = || {
+			if collect_events == CollectEvents::UnsafeCollect {
+				Some(System::<T>::read_events_no_consensus().map(|e| *e).collect())
+			} else {
+				None
+			}
+		};
+
+		let (code, upload_deposit): (WasmCode<T>, BalanceOf<T>) = match code {
+			Code::Upload(code) => {
+				let result = Self::try_upload_code(
+					origin.clone(),
+					code,
+					storage_deposit_limit.map(Into::into),
+					Determinism::Enforced,
+					debug_message.as_mut(),
+				);
+
+				let (module, deposit) = match result {
+					Ok(result) => result,
+					Err(error) =>
+						return ContractResult {
+							gas_consumed: Zero::zero(),
+							gas_required: Zero::zero(),
+							storage_deposit: Default::default(),
+							debug_message: debug_message.unwrap_or(Default::default()).into(),
+							result: Err(error),
+							events: events(),
+						},
+				};
+
+				storage_deposit_limit =
+					storage_deposit_limit.map(|l| l.saturating_sub(deposit.into()));
+				(WasmCode::Wasm(module), deposit)
+			},
+			Code::Existing(hash) => (WasmCode::CodeHash(hash), Default::default()),
+		};
+
 		let common = CommonInput {
 			origin: Origin::from_account_id(origin),
 			value,
@@ -1405,23 +1439,20 @@ impl<T: Config> Pallet<T> {
 			storage_deposit_limit,
 			debug_message: debug_message.as_mut(),
 		};
+
 		let output = InstantiateInput::<T> { code, salt }.run_guarded(common);
-		// collect events if CollectEvents is UnsafeCollect
-		let events = if collect_events == CollectEvents::UnsafeCollect {
-			Some(System::<T>::read_events_no_consensus().map(|e| *e).collect())
-		} else {
-			None
-		};
 		ContractInstantiateResult {
 			result: output
 				.result
 				.map(|(account_id, result)| InstantiateReturnValue { result, account_id })
 				.map_err(|e| e.error),
 			gas_consumed: output.gas_meter.gas_consumed(),
 			gas_required: output.gas_meter.gas_required(),
-			storage_deposit: output.storage_deposit,
+			storage_deposit: output
+				.storage_deposit
+				.saturating_add(&StorageDeposit::Charge(upload_deposit)),
 			debug_message: debug_message.unwrap_or_default().to_vec(),
-			events,
+			events: events(),
 		}
 	}
 
@@ -1436,17 +1467,31 @@ impl<T: Config> Pallet<T> {
 		determinism: Determinism,
 	) -> CodeUploadResult<CodeHash<T>, BalanceOf<T>> {
 		Migration::<T>::ensure_migrated()?;
+		let (module, deposit) =
+			Self::try_upload_code(origin, code, storage_deposit_limit, determinism, None)?;
+		Ok(CodeUploadReturnValue { code_hash: *module.code_hash(), deposit })
+	}
+
+	/// Uploads new code and returns the Wasm blob and deposit amount collected.
+	fn try_upload_code(
+		origin: T::AccountId,
+		code: Vec<u8>,
+		storage_deposit_limit: Option<BalanceOf<T>>,
+		determinism: Determinism,
+		mut debug_message: Option<&mut DebugBufferVec<T>>,
+	) -> Result<(WasmBlob<T>, BalanceOf<T>), DispatchError> {
 		let schedule = T::Schedule::get();
-		let module =
-			WasmBlob::from_code(code, &schedule, origin, determinism, TryInstantiate::Instantiate)
-				.map_err(|(err, _)| err)?;
-		let deposit = module.open_deposit(&module.code_info());
+		let mut module =
+			WasmBlob::from_code(code, &schedule, origin, determinism).map_err(|(err, msg)| {
+				debug_message.as_mut().map(|d| d.try_extend(msg.bytes()));
+				err
+			})?;
+		let deposit = module.store_code()?;
 		if let Some(storage_deposit_limit) = storage_deposit_limit {
 			ensure!(storage_deposit_limit >= deposit, <Error<T>>::StorageDepositLimitExhausted);
 		}
-		let result = CodeUploadReturnValue { code_hash: *module.code_hash(), deposit };
-		module.store()?;
-		Ok(result)
+
+		Ok((module, deposit))
 	}
 
 	/// Query storage of a specified contract under a specified key.
@@ -1490,7 +1535,7 @@ impl<T: Config> Pallet<T> {
 		owner: T::AccountId,
 	) -> frame_support::dispatch::DispatchResult {
 		let schedule = T::Schedule::get();
-		WasmBlob::store_code_unchecked(code, &schedule, owner)?;
+		WasmBlob::<T>::from_code_unchecked(code, &schedule, owner)?.store_code()?;
 		Ok(())
 	}
 

frame/contracts/src/migration/v11.rs

@@ -80,7 +80,9 @@ impl<T: Config> MigrationStep for Migration<T> {
 	}
 
 	fn step(&mut self) -> (IsFinished, Weight) {
-		let Some(old_queue) = old::DeletionQueue::<T>::take() else { return (IsFinished::Yes, Weight::zero()) };
+		let Some(old_queue) = old::DeletionQueue::<T>::take() else {
+			return (IsFinished::Yes, Weight::zero())
+		};
 		let len = old_queue.len();
 
 		log::debug!(