contracts: Passing zero as deposit_limit to call_v2 correctly limits …

…the deposit to nothing (#3392) Only affects unstable `v2` versions of `call` and `instantiate`. Also only when `0` was passed as `desposit_limit`. Old behaviour was allowing all the remaining balance. New correct behaviour is to not allow any deposit. Otherwise both passing the `SENTINEL` and encoding `0` would mean the same thing.
paritytech · Feb 19, 2024 · 5a3b03e · 5a3b03e
1 parent e96f7c5
commit 5a3b03e
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 90 deletions.
diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs
@@ -144,7 +144,7 @@ pub trait Ext: sealing::Sealed {
 	fn call(
 		&mut self,
 		gas_limit: Weight,
-		deposit_limit: BalanceOf<Self::T>,
+		deposit_limit: Option<BalanceOf<Self::T>>,
 		to: AccountIdOf<Self::T>,
 		value: BalanceOf<Self::T>,
 		input_data: Vec<u8>,
@@ -168,7 +168,7 @@ pub trait Ext: sealing::Sealed {
 	fn instantiate(
 		&mut self,
 		gas_limit: Weight,
-		deposit_limit: BalanceOf<Self::T>,
+		deposit_limit: Option<BalanceOf<Self::T>>,
 		code: CodeHash<Self::T>,
 		value: BalanceOf<Self::T>,
 		input_data: Vec<u8>,
@@ -747,7 +747,7 @@ where
 			gas_meter,
 			Weight::zero(),
 			storage_meter,
-			BalanceOf::<T>::zero(),
+			None,
 			determinism,
 		)?;
 
@@ -779,7 +779,7 @@ where
 		gas_meter: &mut GasMeter<T>,
 		gas_limit: Weight,
 		storage_meter: &mut storage::meter::GenericMeter<T, S>,
-		deposit_limit: BalanceOf<T>,
+		deposit_limit: Option<BalanceOf<T>>,
 		determinism: Determinism,
 	) -> Result<(Frame<T>, E, Option<u64>), ExecError> {
 		let (account_id, contract_info, executable, delegate_caller, entry_point, nonce) =
@@ -848,7 +848,7 @@ where
 		frame_args: FrameArgs<T, E>,
 		value_transferred: BalanceOf<T>,
 		gas_limit: Weight,
-		deposit_limit: BalanceOf<T>,
+		deposit_limit: Option<BalanceOf<T>>,
 	) -> Result<E, ExecError> {
 		if self.frames.len() == T::CallStack::size() {
 			return Err(Error::<T>::MaxCallDepthReached.into())
@@ -1185,7 +1185,7 @@ where
 	fn call(
 		&mut self,
 		gas_limit: Weight,
-		deposit_limit: BalanceOf<T>,
+		deposit_limit: Option<BalanceOf<T>>,
 		to: T::AccountId,
 		value: BalanceOf<T>,
 		input_data: Vec<u8>,
@@ -1246,15 +1246,15 @@ where
 			},
 			value,
 			Weight::zero(),
-			BalanceOf::<T>::zero(),
+			None,
 		)?;
 		self.run(executable, input_data)
 	}
 
 	fn instantiate(
 		&mut self,
 		gas_limit: Weight,
-		deposit_limit: BalanceOf<Self::T>,
+		deposit_limit: Option<BalanceOf<Self::T>>,
 		code_hash: CodeHash<T>,
 		value: BalanceOf<T>,
 		input_data: Vec<u8>,
@@ -2091,7 +2091,7 @@ mod tests {
 		let value = Default::default();
 		let recurse_ch = MockLoader::insert(Call, |ctx, _| {
 			// Try to call into yourself.
-			let r = ctx.ext.call(Weight::zero(), BalanceOf::<Test>::zero(), BOB, 0, vec![], true);
+			let r = ctx.ext.call(Weight::zero(), None, BOB, 0, vec![], true);
 
 			ReachedBottom::mutate(|reached_bottom| {
 				if !*reached_bottom {
@@ -2149,11 +2149,7 @@ mod tests {
 			});
 
 			// Call into CHARLIE contract.
-			assert_matches!(
-				ctx.ext
-					.call(Weight::zero(), BalanceOf::<Test>::zero(), CHARLIE, 0, vec![], true),
-				Ok(_)
-			);
+			assert_matches!(ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], true), Ok(_));
 			exec_success()
 		});
 		let charlie_ch = MockLoader::insert(Call, |ctx, _| {
@@ -2297,8 +2293,7 @@ mod tests {
 			// ALICE is the origin of the call stack
 			assert!(ctx.ext.caller_is_origin());
 			// BOB calls CHARLIE
-			ctx.ext
-				.call(Weight::zero(), BalanceOf::<Test>::zero(), CHARLIE, 0, vec![], true)
+			ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], true)
 		});
 
 		ExtBuilder::default().build().execute_with(|| {
@@ -2396,8 +2391,7 @@ mod tests {
 			// root is the origin of the call stack.
 			assert!(ctx.ext.caller_is_root());
 			// BOB calls CHARLIE.
-			ctx.ext
-				.call(Weight::zero(), BalanceOf::<Test>::zero(), CHARLIE, 0, vec![], true)
+			ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], true)
 		});
 
 		ExtBuilder::default().build().execute_with(|| {
@@ -2430,11 +2424,7 @@ mod tests {
 			assert_eq!(*ctx.ext.address(), BOB);
 
 			// Call into charlie contract.
-			assert_matches!(
-				ctx.ext
-					.call(Weight::zero(), BalanceOf::<Test>::zero(), CHARLIE, 0, vec![], true),
-				Ok(_)
-			);
+			assert_matches!(ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], true), Ok(_));
 			exec_success()
 		});
 		let charlie_ch = MockLoader::insert(Call, |ctx, _| {
@@ -2609,7 +2599,7 @@ mod tests {
 					.ext
 					.instantiate(
 						Weight::zero(),
-						BalanceOf::<Test>::zero(),
+						None,
 						dummy_ch,
 						<Test as Config>::Currency::minimum_balance(),
 						vec![],
@@ -2685,7 +2675,7 @@ mod tests {
 				assert_matches!(
 					ctx.ext.instantiate(
 						Weight::zero(),
-						BalanceOf::<Test>::zero(),
+						None,
 						dummy_ch,
 						<Test as Config>::Currency::minimum_balance(),
 						vec![],
@@ -2794,25 +2784,15 @@ mod tests {
 				assert_eq!(info.storage_byte_deposit, 0);
 				info.storage_byte_deposit = 42;
 				assert_eq!(
-					ctx.ext.call(
-						Weight::zero(),
-						BalanceOf::<Test>::zero(),
-						CHARLIE,
-						0,
-						vec![],
-						true
-					),
+					ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], true),
 					exec_trapped()
 				);
 				assert_eq!(ctx.ext.contract_info().storage_byte_deposit, 42);
 			}
 			exec_success()
 		});
 		let code_charlie = MockLoader::insert(Call, |ctx, _| {
-			assert!(ctx
-				.ext
-				.call(Weight::zero(), BalanceOf::<Test>::zero(), BOB, 0, vec![99], true)
-				.is_ok());
+			assert!(ctx.ext.call(Weight::zero(), None, BOB, 0, vec![99], true).is_ok());
 			exec_trapped()
 		});
 
@@ -2844,7 +2824,7 @@ mod tests {
 	fn recursive_call_during_constructor_fails() {
 		let code = MockLoader::insert(Constructor, |ctx, _| {
 			assert_matches!(
-				ctx.ext.call(Weight::zero(), BalanceOf::<Test>::zero(), ctx.ext.address().clone(), 0, vec![], true),
+				ctx.ext.call(Weight::zero(), None, ctx.ext.address().clone(), 0, vec![], true),
 				Err(ExecError{error, ..}) if error == <Error<Test>>::ContractNotFound.into()
 			);
 			exec_success()
@@ -2994,7 +2974,7 @@ mod tests {
 		// call the contract passed as input with disabled reentry
 		let code_bob = MockLoader::insert(Call, |ctx, _| {
 			let dest = Decode::decode(&mut ctx.input_data.as_ref()).unwrap();
-			ctx.ext.call(Weight::zero(), BalanceOf::<Test>::zero(), dest, 0, vec![], false)
+			ctx.ext.call(Weight::zero(), None, dest, 0, vec![], false)
 		});
 
 		let code_charlie = MockLoader::insert(Call, |_, _| exec_success());
@@ -3043,16 +3023,15 @@ mod tests {
 	fn call_deny_reentry() {
 		let code_bob = MockLoader::insert(Call, |ctx, _| {
 			if ctx.input_data[0] == 0 {
-				ctx.ext
-					.call(Weight::zero(), BalanceOf::<Test>::zero(), CHARLIE, 0, vec![], false)
+				ctx.ext.call(Weight::zero(), None, CHARLIE, 0, vec![], false)
 			} else {
 				exec_success()
 			}
 		});
 
 		// call BOB with input set to '1'
 		let code_charlie = MockLoader::insert(Call, |ctx, _| {
-			ctx.ext.call(Weight::zero(), BalanceOf::<Test>::zero(), BOB, 0, vec![1], true)
+			ctx.ext.call(Weight::zero(), None, BOB, 0, vec![1], true)
 		});
 
 		ExtBuilder::default().build().execute_with(|| {
@@ -3248,7 +3227,7 @@ mod tests {
 			ctx.ext
 				.instantiate(
 					Weight::zero(),
-					BalanceOf::<Test>::zero(),
+					None,
 					fail_code,
 					ctx.ext.minimum_balance() * 100,
 					vec![],
@@ -3262,7 +3241,7 @@ mod tests {
 				.ext
 				.instantiate(
 					Weight::zero(),
-					BalanceOf::<Test>::zero(),
+					None,
 					success_code,
 					ctx.ext.minimum_balance() * 100,
 					vec![],
@@ -3271,9 +3250,7 @@ mod tests {
 				.unwrap();
 
 			// a plain call should not influence the account counter
-			ctx.ext
-				.call(Weight::zero(), BalanceOf::<Test>::zero(), account_id, 0, vec![], false)
-				.unwrap();
+			ctx.ext.call(Weight::zero(), None, account_id, 0, vec![], false).unwrap();
 
 			exec_success()
 		});
@@ -3791,31 +3768,15 @@ mod tests {
 			assert_eq!(ctx.ext.nonce(), 1);
 			// Should not change with a failed instantiation
 			assert_err!(
-				ctx.ext.instantiate(
-					Weight::zero(),
-					BalanceOf::<Test>::zero(),
-					fail_code,
-					0,
-					vec![],
-					&[],
-				),
+				ctx.ext.instantiate(Weight::zero(), None, fail_code, 0, vec![], &[],),
 				ExecError {
 					error: <Error<Test>>::ContractTrapped.into(),
 					origin: ErrorOrigin::Callee
 				}
 			);
 			assert_eq!(ctx.ext.nonce(), 1);
 			// Successful instantiation increments
-			ctx.ext
-				.instantiate(
-					Weight::zero(),
-					BalanceOf::<Test>::zero(),
-					success_code,
-					0,
-					vec![],
-					&[],
-				)
-				.unwrap();
+			ctx.ext.instantiate(Weight::zero(), None, success_code, 0, vec![], &[]).unwrap();
 			assert_eq!(ctx.ext.nonce(), 2);
 			exec_success()
 		});

diff --git a/substrate/frame/contracts/src/storage/meter.rs b/substrate/frame/contracts/src/storage/meter.rs
@@ -283,21 +283,21 @@ where
 	E: Ext<T>,
 	S: State + Default + Debug,
 {
-	/// Create a new child that has its `limit`.
-	/// Passing `0` as the limit is interpreted as to take whatever is remaining from its parent.
+	/// Create a new child with an optional `limit` on how much deposit it can consume.
 	///
 	/// This is called whenever a new subcall is initiated in order to track the storage
 	/// usage for this sub call separately. This is necessary because we want to exchange balance
 	/// with the current contract we are interacting with.
-	pub fn nested(&self, limit: BalanceOf<T>) -> RawMeter<T, E, Nested> {
+	pub fn nested(&self, limit: Option<BalanceOf<T>>) -> RawMeter<T, E, Nested> {
 		debug_assert!(matches!(self.contract_state(), ContractState::Alive));
-		// If a special limit is specified higher than it is available,
-		// we want to enforce the lesser limit to the nested meter, to fail in the sub-call.
-		let limit = self.available().min(limit);
-		if limit.is_zero() {
-			RawMeter { limit: self.available(), ..Default::default() }
+		if let Some(limit) = limit {
+			RawMeter {
+				limit: self.available().min(limit),
+				nested: Nested::OwnLimit,
+				..Default::default()
+			}
 		} else {
-			RawMeter { limit, nested: Nested::OwnLimit, ..Default::default() }
+			RawMeter { limit: self.available(), ..Default::default() }
 		}
 	}
 
@@ -750,7 +750,7 @@ mod tests {
 		assert_eq!(meter.available(), 1_000);
 
 		// an empty charge does not create a `Charge` entry
-		let mut nested0 = meter.nested(BalanceOf::<Test>::zero());
+		let mut nested0 = meter.nested(None);
 		nested0.charge(&Default::default());
 		meter.absorb(nested0, &BOB, None);
 
@@ -812,7 +812,7 @@ mod tests {
 				bytes_deposit: 100,
 				items_deposit: 10,
 			});
-			let mut nested0 = meter.nested(BalanceOf::<Test>::zero());
+			let mut nested0 = meter.nested(None);
 			nested0.charge(&Diff {
 				bytes_added: 108,
 				bytes_removed: 5,
@@ -827,7 +827,7 @@ mod tests {
 				bytes_deposit: 100,
 				items_deposit: 20,
 			});
-			let mut nested1 = nested0.nested(BalanceOf::<Test>::zero());
+			let mut nested1 = nested0.nested(None);
 			nested1.charge(&Diff { items_removed: 5, ..Default::default() });
 			nested0.absorb(nested1, &CHARLIE, Some(&mut nested1_info));
 
@@ -837,7 +837,7 @@ mod tests {
 				bytes_deposit: 100,
 				items_deposit: 20,
 			});
-			let mut nested2 = nested0.nested(BalanceOf::<Test>::zero());
+			let mut nested2 = nested0.nested(None);
 			nested2.charge(&Diff { items_removed: 7, ..Default::default() });
 			nested0.absorb(nested2, &CHARLIE, Some(&mut nested2_info));
 
@@ -891,7 +891,7 @@ mod tests {
 			let mut meter = TestMeter::new(&test_case.origin, Some(1_000), 0).unwrap();
 			assert_eq!(meter.available(), 1_000);
 
-			let mut nested0 = meter.nested(BalanceOf::<Test>::zero());
+			let mut nested0 = meter.nested(None);
 			nested0.charge(&Diff {
 				bytes_added: 5,
 				bytes_removed: 1,
@@ -906,7 +906,7 @@ mod tests {
 				bytes_deposit: 100,
 				items_deposit: 20,
 			});
-			let mut nested1 = nested0.nested(BalanceOf::<Test>::zero());
+			let mut nested1 = nested0.nested(None);
 			nested1.charge(&Diff { items_removed: 5, ..Default::default() });
 			nested1.charge(&Diff { bytes_added: 20, ..Default::default() });
 			nested1.terminate(&nested1_info, CHARLIE);

diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs
@@ -549,7 +549,7 @@ mod tests {
 		fn call(
 			&mut self,
 			_gas_limit: Weight,
-			_deposit_limit: BalanceOf<Self::T>,
+			_deposit_limit: Option<BalanceOf<Self::T>>,
 			to: AccountIdOf<Self::T>,
 			value: u64,
 			data: Vec<u8>,
@@ -569,7 +569,7 @@ mod tests {
 		fn instantiate(
 			&mut self,
 			gas_limit: Weight,
-			_deposit_limit: BalanceOf<Self::T>,
+			_deposit_limit: Option<BalanceOf<Self::T>>,
 			code_hash: CodeHash<Test>,
 			value: u64,
 			data: Vec<u8>,