srml-contracts: Refactor to remove duplication between call and create code paths (#3209)

* srml-contracts: Change Ext interface to pass-by-value.

* srml-contracts: Refactor ExecutionContext to reduce duplication.

* srml-contracts: Refactor contracts Module to reduce duplication.

* Bump node runtime impl version.

substrate/node/runtime/src/lib.rs

@@ -80,7 +80,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
 	spec_version: 122,
-	impl_version: 122,
+	impl_version: 123,
 	apis: RUNTIME_API_VERSIONS,
 };
 

substrate/srml/contracts/src/exec.rs

@@ -227,7 +227,7 @@ pub trait Vm<T: Trait> {
 	fn execute<E: Ext<T = T>>(
 		&self,
 		exec: &Self::Executable,
-		ext: &mut E,
+		ext: E,
 		input_data: &[u8],
 		empty_output_buf: EmptyOutputBuf,
 		gas_meter: &mut GasMeter<T>,
@@ -294,13 +294,13 @@ where
 		}
 	}
 
-	fn nested(&self, overlay: OverlayAccountDb<'a, T>, dest: T::AccountId, trie_id: Option<TrieId>)
-		-> Self
+	fn nested<'b, 'c: 'b>(&'c self, dest: T::AccountId, trie_id: Option<TrieId>)
+		-> ExecutionContext<'b, T, V, L>
 	{
 		ExecutionContext {
 			self_trie_id: trie_id,
 			self_account: dest,
-			overlay,
+			overlay: OverlayAccountDb::new(&self.overlay),
 			depth: self.depth + 1,
 			events: Vec::new(),
 			calls: Vec::new(),
@@ -342,54 +342,41 @@ where
 			return Err("contract has been evicted");
 		};
 
-		let mut output_data = Vec::new();
-
-		let (change_set, events, calls) = {
-			let mut nested = self.nested(
-				OverlayAccountDb::new(&self.overlay),
-				dest.clone(),
-				contract_info.and_then(|i| i.as_alive().map(|i| i.trie_id.clone()))
-			);
+		let caller = self.self_account.clone();
+		let dest_trie_id = contract_info.and_then(|i| i.as_alive().map(|i| i.trie_id.clone()));
 
+		let output_data = self.with_nested_context(dest.clone(), dest_trie_id, |nested| {
 			if value > BalanceOf::<T>::zero() {
 				transfer(
 					gas_meter,
 					TransferCause::Call,
-					&self.self_account,
+					&caller,
 					&dest,
 					value,
-					&mut nested,
+					nested,
 				)?;
 			}
 
 			// If code_hash is not none, then the destination account is a live contract, otherwise
 			// it is a regular account since tombstone accounts have already been rejected.
-			if let Some(dest_code_hash) = self.overlay.get_code_hash(&dest) {
-				let executable = self.loader.load_main(&dest_code_hash)?;
-				output_data = self
-					.vm
-					.execute(
-						&executable,
-						&mut CallContext {
-							ctx: &mut nested,
-							caller: self.self_account.clone(),
-							value_transferred: value,
-							timestamp: self.timestamp.clone(),
-							block_number: self.block_number.clone(),
-						},
-						input_data,
-						empty_output_buf,
-						gas_meter,
-					)
-					.into_result()?;
-			}
+			let output_data = match nested.overlay.get_code_hash(&dest) {
+				Some(dest_code_hash) => {
+					let executable = nested.loader.load_main(&dest_code_hash)?;
+					nested.vm
+						.execute(
+							&executable,
+							nested.new_call_context(caller, value),
+							input_data,
+							empty_output_buf,
+							gas_meter,
+						)
+						.into_result()?
+				}
+				None => Vec::new(),
+			};
 
-			(nested.overlay.into_change_set(), nested.events, nested.calls)
-		};
-
-		self.overlay.commit(change_set);
-		self.events.extend(events);
-		self.calls.extend(calls);
+			Ok(output_data)
+		})?;
 
 		Ok(CallReceipt { output_data })
 	}
@@ -412,42 +399,35 @@ where
 			return Err("not enough gas to pay base instantiate fee");
 		}
 
+		let caller = self.self_account.clone();
 		let dest = T::DetermineContractAddress::contract_address_for(
 			code_hash,
 			input_data,
-			&self.self_account,
+			&caller,
 		);
 
-		let (change_set, events, calls) = {
-			let mut overlay = OverlayAccountDb::new(&self.overlay);
+		// TrieId has not been generated yet and storage is empty since contract is new.
+		let dest_trie_id = None;
 
-			overlay.create_contract(&dest, code_hash.clone())?;
-
-			// TrieId has not been generated yet and storage is empty since contract is new.
-			let mut nested = self.nested(overlay, dest.clone(), None);
+		let _ = self.with_nested_context(dest.clone(), dest_trie_id, |nested| {
+			nested.overlay.create_contract(&dest, code_hash.clone())?;
 
 			// Send funds unconditionally here. If the `endowment` is below existential_deposit
 			// then error will be returned here.
 			transfer(
 				gas_meter,
 				TransferCause::Instantiate,
-				&self.self_account,
+				&caller,
 				&dest,
 				endowment,
-				&mut nested,
+				nested,
 			)?;
 
-			let executable = self.loader.load_init(&code_hash)?;
-			self.vm
+			let executable = nested.loader.load_init(&code_hash)?;
+			nested.vm
 				.execute(
 					&executable,
-					&mut CallContext {
-						ctx: &mut nested,
-						caller: self.self_account.clone(),
-						value_transferred: endowment,
-						timestamp: self.timestamp.clone(),
-						block_number: self.block_number.clone(),
-					},
+					nested.new_call_context(caller.clone(), endowment),
 					input_data,
 					EmptyOutputBuf::new(),
 					gas_meter,
@@ -456,18 +436,45 @@ where
 
 			// Deposit an instantiation event.
 			nested.events.push(IndexedEvent {
-				event: RawEvent::Instantiated(self.self_account.clone(), dest.clone()),
+				event: RawEvent::Instantiated(caller.clone(), dest.clone()),
 				topics: Vec::new(),
 			});
 
-			(nested.overlay.into_change_set(), nested.events, nested.calls)
+			Ok(Vec::new())
+		})?;
+
+		Ok(InstantiateReceipt { address: dest })
+	}
+
+	fn new_call_context<'b>(&'b mut self, caller: T::AccountId, value: BalanceOf<T>)
+		-> CallContext<'b, 'a, T, V, L>
+	{
+		let timestamp = self.timestamp.clone();
+		let block_number = self.block_number.clone();
+		CallContext {
+			ctx: self,
+			caller,
+			value_transferred: value,
+			timestamp,
+			block_number,
+		}
+	}
+
+	fn with_nested_context<F>(&mut self, dest: T::AccountId, trie_id: Option<TrieId>, func: F)
+		-> Result<Vec<u8>, &'static str>
+		where F: FnOnce(&mut ExecutionContext<T, V, L>) -> Result<Vec<u8>, &'static str>
+	{
+		let (output_data, change_set, events, calls) = {
+			let mut nested = self.nested(dest, trie_id);
+			let output_data = func(&mut nested)?;
+			(output_data, nested.overlay.into_change_set(), nested.events, nested.calls)
 		};
 
 		self.overlay.commit(change_set);
 		self.events.extend(events);
 		self.calls.extend(calls);
 
-		Ok(InstantiateReceipt { address: dest })
+		Ok(output_data)
 	}
 }
 
@@ -806,13 +813,13 @@ mod tests {
 		fn execute<E: Ext<T = Test>>(
 			&self,
 			exec: &MockExecutable,
-			ext: &mut E,
+			mut ext: E,
 			input_data: &[u8],
 			empty_output_buf: EmptyOutputBuf,
 			gas_meter: &mut GasMeter<Test>,
 		) -> VmExecResult {
 			(exec.0)(MockCtx {
-				ext,
+				ext: &mut ext,
 				input_data,
 				empty_output_buf: Some(empty_output_buf),
 				gas_meter,

substrate/srml/contracts/src/lib.rs

@@ -91,7 +91,8 @@ mod tests;
 
 use crate::exec::ExecutionContext;
 use crate::account_db::{AccountDb, DirectAccountDb};
-pub use crate::gas::Gas;
+pub use crate::gas::{Gas, GasMeter};
+use crate::wasm::{WasmLoader, WasmVm};
 
 #[cfg(feature = "std")]
 use serde::{Serialize, Deserialize};
@@ -558,45 +559,9 @@ decl_module! {
 			let origin = ensure_signed(origin)?;
 			let dest = T::Lookup::lookup(dest)?;
 
-			// Pay for the gas upfront.
-			//
-			// NOTE: it is very important to avoid any state changes before
-			// paying for the gas.
-			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
-
-			let cfg = Config::preload();
-			let vm = crate::wasm::WasmVm::new(&cfg.schedule);
-			let loader = crate::wasm::WasmLoader::new(&cfg.schedule);
-			let mut ctx = ExecutionContext::top_level(origin.clone(), &cfg, &vm, &loader);
-
-			let result = ctx.call(dest, value, &mut gas_meter, &data, exec::EmptyOutputBuf::new());
-
-			if let Ok(_) = result {
-				// Commit all changes that made it thus far into the persistent storage.
-				DirectAccountDb.commit(ctx.overlay.into_change_set());
-
-				// Then deposit all events produced.
-				ctx.events.into_iter().for_each(|indexed_event| {
-					<system::Module<T>>::deposit_event_indexed(
-						&*indexed_event.topics,
-						<T as Trait>::Event::from(indexed_event.event).into(),
-					);
-				});
-			}
-
-			// Refund cost of the unused gas.
-			//
-			// NOTE: This should go after the commit to the storage, since the storage changes
-			// can alter the balance of the caller.
-			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
-
-			// Dispatch every recorded call with an appropriate origin.
-			ctx.calls.into_iter().for_each(|(who, call)| {
-				let result = call.dispatch(RawOrigin::Signed(who.clone()).into());
-				Self::deposit_event(RawEvent::Dispatched(who, result.is_ok()));
-			});
-
-			result.map(|_| ())
+			Self::execute_wasm(origin, gas_limit, |ctx, gas_meter| {
+				ctx.call(dest, value, gas_meter, &data, exec::EmptyOutputBuf::new()).map(|_| ())
+			})
 		}
 
 		/// Creates a new contract from the `codehash` generated by `put_code`, optionally transferring some balance.
@@ -618,44 +583,9 @@ decl_module! {
 		) -> Result {
 			let origin = ensure_signed(origin)?;
 
-			// Commit the gas upfront.
-			//
-			// NOTE: It is very important to avoid any state changes before
-			// paying for the gas.
-			let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
-
-			let cfg = Config::preload();
-			let vm = crate::wasm::WasmVm::new(&cfg.schedule);
-			let loader = crate::wasm::WasmLoader::new(&cfg.schedule);
-			let mut ctx = ExecutionContext::top_level(origin.clone(), &cfg, &vm, &loader);
-			let result = ctx.instantiate(endowment, &mut gas_meter, &code_hash, &data);
-
-			if let Ok(_) = result {
-				// Commit all changes that made it thus far into the persistent storage.
-				DirectAccountDb.commit(ctx.overlay.into_change_set());
-
-				// Then deposit all events produced.
-				ctx.events.into_iter().for_each(|indexed_event| {
-					<system::Module<T>>::deposit_event_indexed(
-						&*indexed_event.topics,
-						<T as Trait>::Event::from(indexed_event.event).into(),
-					);
-				});
-			}
-
-			// Refund cost of the unused gas.
-			//
-			// NOTE: This should go after the commit to the storage, since the storage changes
-			// can alter the balance of the caller.
-			gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
-
-			// Dispatch every recorded call with an appropriate origin.
-			ctx.calls.into_iter().for_each(|(who, call)| {
-				let result = call.dispatch(RawOrigin::Signed(who.clone()).into());
-				Self::deposit_event(RawEvent::Dispatched(who, result.is_ok()));
-			});
-
-			result.map(|_| ())
+			Self::execute_wasm(origin, gas_limit, |ctx, gas_meter| {
+				ctx.instantiate(endowment, gas_meter, &code_hash, &data).map(|_| ())
+			})
 		}
 
 		/// Allows block producers to claim a small reward for evicting a contract. If a block producer
@@ -775,6 +705,54 @@ decl_module! {
 	}
 }
 
+impl<T: Trait> Module<T> {
+	fn execute_wasm(
+		origin: T::AccountId,
+		gas_limit: Gas,
+		func: impl FnOnce(&mut ExecutionContext<T, WasmVm, WasmLoader>, &mut GasMeter<T>) -> Result
+	) -> Result {
+		// Pay for the gas upfront.
+		//
+		// NOTE: it is very important to avoid any state changes before
+		// paying for the gas.
+		let (mut gas_meter, imbalance) = gas::buy_gas::<T>(&origin, gas_limit)?;
+
+		let cfg = Config::preload();
+		let vm = WasmVm::new(&cfg.schedule);
+		let loader = WasmLoader::new(&cfg.schedule);
+		let mut ctx = ExecutionContext::top_level(origin.clone(), &cfg, &vm, &loader);
+
+		let result = func(&mut ctx, &mut gas_meter);
+
+		if result.is_ok() {
+			// Commit all changes that made it thus far into the persistent storage.
+			DirectAccountDb.commit(ctx.overlay.into_change_set());
+
+			// Then deposit all events produced.
+			ctx.events.into_iter().for_each(|indexed_event| {
+				<system::Module<T>>::deposit_event_indexed(
+					&*indexed_event.topics,
+					<T as Trait>::Event::from(indexed_event.event).into(),
+				);
+			});
+		}
+
+		// Refund cost of the unused gas.
+		//
+		// NOTE: This should go after the commit to the storage, since the storage changes
+		// can alter the balance of the caller.
+		gas::refund_unused_gas::<T>(&origin, gas_meter, imbalance);
+
+		// Dispatch every recorded call with an appropriate origin.
+		ctx.calls.into_iter().for_each(|(who, call)| {
+			let result = call.dispatch(RawOrigin::Signed(who.clone()).into());
+			Self::deposit_event(RawEvent::Dispatched(who, result.is_ok()));
+		});
+
+		result
+	}
+}
+
 decl_event! {
 	pub enum Event<T>
 	where

substrate/srml/contracts/src/wasm/mod.rs

@@ -109,7 +109,7 @@ impl<'a, T: Trait> crate::exec::Vm<T> for WasmVm<'a> {
 	fn execute<E: Ext<T = T>>(
 		&self,
 		exec: &WasmExecutable,
-		ext: &mut E,
+		mut ext: E,
 		input_data: &[u8],
 		empty_output_buf: EmptyOutputBuf,
 		gas_meter: &mut GasMeter<E::T>,
@@ -133,7 +133,7 @@ impl<'a, T: Trait> crate::exec::Vm<T> for WasmVm<'a> {
 		});
 
 		let mut runtime = Runtime::new(
-			ext,
+			&mut ext,
 			input_data.to_vec(),
 			empty_output_buf,
 			&self.schedule,
@@ -299,12 +299,79 @@ mod tests {
 
 		fn max_value_size(&self) -> u32 { 16_384 }
 	}
+	impl Ext for &mut MockExt {
+		type T = <MockExt as Ext>::T;
+
+		fn get_storage(&self, key: &[u8; 32]) -> Option<Vec<u8>> {
+			(**self).get_storage(key)
+		}
+		fn set_storage(&mut self, key: [u8; 32], value: Option<Vec<u8>>)
+			-> Result<(), &'static str>
+		{
+			(**self).set_storage(key, value)
+		}
+		fn instantiate(
+			&mut self,
+			code: &CodeHash<Test>,
+			value: u64,
+			gas_meter: &mut GasMeter<Test>,
+			input_data: &[u8]
+		) -> Result<InstantiateReceipt<u64>, &'static str> {
+			(**self).instantiate(code, value, gas_meter, input_data)
+		}
+		fn call(
+			&mut self,
+			to: &u64,
+			value: u64,
+			gas_meter: &mut GasMeter<Test>,
+			input_data: &[u8],
+			empty_output_buf: EmptyOutputBuf
+		) -> Result<CallReceipt, &'static str> {
+			(**self).call(to, value, gas_meter, input_data, empty_output_buf)
+		}
+		fn note_dispatch_call(&mut self, call: Call) {
+			(**self).note_dispatch_call(call)
+		}
+		fn caller(&self) -> &u64 {
+			(**self).caller()
+		}
+		fn address(&self) -> &u64 {
+			(**self).address()
+		}
+		fn balance(&self) -> u64 {
+			(**self).balance()
+		}
+		fn value_transferred(&self) -> u64 {
+			(**self).value_transferred()
+		}
+		fn now(&self) -> &u64 {
+			(**self).now()
+		}
+		fn random(&self, subject: &[u8]) -> H256 {
+			(**self).random(subject)
+		}
+		fn deposit_event(&mut self, topics: Vec<H256>, data: Vec<u8>) {
+			(**self).deposit_event(topics, data)
+		}
+		fn set_rent_allowance(&mut self, rent_allowance: u64) {
+			(**self).set_rent_allowance(rent_allowance)
+		}
+		fn rent_allowance(&self) -> u64 {
+			(**self).rent_allowance()
+		}
+		fn block_number(&self) -> u64 {
+			(**self).block_number()
+		}
+		fn max_value_size(&self) -> u32 {
+			(**self).max_value_size()
+		}
+	}
 
 	fn execute<E: Ext>(
 		wat: &str,
 		input_data: &[u8],
 		output_data: &mut Vec<u8>,
-		ext: &mut E,
+		ext: E,
 		gas_meter: &mut GasMeter<E::T>,
 	) -> Result<(), &'static str> {
 		use crate::exec::Vm;
@@ -598,7 +665,7 @@ mod tests {
 			CODE_GET_STORAGE,
 			&[],
 			&mut return_buf,
-			&mut mock_ext,
+			mock_ext,
 			&mut GasMeter::with_limit(50_000, 1),
 		)
 		.unwrap();
@@ -660,12 +727,11 @@ mod tests {
 
 	#[test]
 	fn caller() {
-		let mut mock_ext = MockExt::default();
 		execute(
 			CODE_CALLER,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut GasMeter::with_limit(50_000, 1),
 		)
 		.unwrap();
@@ -725,12 +791,11 @@ mod tests {
 
 	#[test]
 	fn address() {
-		let mut mock_ext = MockExt::default();
 		execute(
 			CODE_ADDRESS,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut GasMeter::with_limit(50_000, 1),
 		)
 		.unwrap();
@@ -787,13 +852,12 @@ mod tests {
 
 	#[test]
 	fn balance() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 		execute(
 			CODE_BALANCE,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -850,13 +914,12 @@ mod tests {
 
 	#[test]
 	fn gas_price() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1312);
 		execute(
 			CODE_GAS_PRICE,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -911,7 +974,6 @@ mod tests {
 
 	#[test]
 	fn gas_left() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1312);
 
 		let mut return_buf = Vec::new();
@@ -919,7 +981,7 @@ mod tests {
 			CODE_GAS_LEFT,
 			&[],
 			&mut return_buf,
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -980,13 +1042,12 @@ mod tests {
 
 	#[test]
 	fn value_transferred() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 		execute(
 			CODE_VALUE_TRANSFERRED,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -1057,13 +1118,12 @@ mod tests {
 
 	#[test]
 	fn return_from_start_fn() {
-		let mut mock_ext = MockExt::default();
 		let mut output_data = Vec::new();
 		execute(
 			CODE_RETURN_FROM_START_FN,
 			&[],
 			&mut output_data,
-			&mut mock_ext,
+			MockExt::default(),
 			&mut GasMeter::with_limit(50_000, 1),
 		)
 		.unwrap();
@@ -1122,13 +1182,12 @@ mod tests {
 
 	#[test]
 	fn now() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 		execute(
 			CODE_TIMESTAMP_NOW,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -1193,7 +1252,6 @@ mod tests {
 
 	#[test]
 	fn random() {
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 
 		let mut return_buf = Vec::new();
@@ -1201,7 +1259,7 @@ mod tests {
 			CODE_RANDOM,
 			&[],
 			&mut return_buf,
-			&mut mock_ext,
+			MockExt::default(),
 			&mut gas_meter,
 		)
 		.unwrap();
@@ -1291,7 +1349,6 @@ mod tests {
 	#[test]
 	fn deposit_event_max_topics() {
 		// Checks that the runtime traps if there are more than `max_topic_events` topics.
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 
 		assert_eq!(
@@ -1299,7 +1356,7 @@ mod tests {
 				CODE_DEPOSIT_EVENT_MAX_TOPICS,
 				&[],
 				&mut Vec::new(),
-				&mut mock_ext,
+				MockExt::default(),
 				&mut gas_meter
 			),
 			Err("during execution"),
@@ -1335,7 +1392,6 @@ mod tests {
 	#[test]
 	fn deposit_event_duplicates() {
 		// Checks that the runtime traps if there are duplicates.
-		let mut mock_ext = MockExt::default();
 		let mut gas_meter = GasMeter::with_limit(50_000, 1);
 
 		assert_eq!(
@@ -1343,7 +1399,7 @@ mod tests {
 				CODE_DEPOSIT_EVENT_DUPLICATES,
 				&[],
 				&mut Vec::new(),
-				&mut mock_ext,
+				MockExt::default(),
 				&mut gas_meter
 			),
 			Err("during execution"),
@@ -1404,12 +1460,11 @@ mod tests {
 
 	#[test]
 	fn block_number() {
-		let mut mock_ext = MockExt::default();
 		execute(
 			CODE_BLOCK_NUMBER,
 			&[],
 			&mut Vec::new(),
-			&mut mock_ext,
+			MockExt::default(),
 			&mut GasMeter::with_limit(50_000, 1),
 		)
 		.unwrap();