Record ed as part of the storage deposit

prdoc/pr_8718.prdoc

@@ -0,0 +1,7 @@
+title: Record ed as part of the storage deposit
+doc:
+- audience: Runtime Dev
+  description: Fixes https://github.com/paritytech/contract-issues/issues/38
+crates:
+- name: pallet-revive
+  bump: patch

substrate/frame/revive/src/exec.rs

@@ -761,14 +761,14 @@ where
 	pub fn run_call(
 		origin: Origin<T>,
 		dest: H160,
-		gas_meter: &'a mut GasMeter<T>,
-		storage_meter: &'a mut storage::meter::Meter<T>,
+		gas_meter: &mut GasMeter<T>,
+		storage_meter: &mut storage::meter::Meter<T>,
 		value: U256,
 		input_data: Vec<u8>,
 		skip_transfer: bool,
 	) -> ExecResult {
 		let dest = T::AddressMapper::to_account_id(&dest);
-		if let Some((mut stack, executable)) = Self::new(
+		if let Some((mut stack, executable)) = Stack::<'_, T, E>::new(
 			FrameArgs::Call { dest: dest.clone(), cached_info: None, delegated_call: None },
 			origin.clone(),
 			gas_meter,
@@ -778,7 +778,7 @@ where
 		)? {
 			stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output)
 		} else {
-			let result = Self::transfer_from_origin(&origin, &origin, &dest, value);
+			let result = Self::transfer_from_origin(&origin, &origin, &dest, value, storage_meter);
 			if_tracing(|t| {
 				t.enter_child_span(
 					origin.account_id().map(T::AddressMapper::to_address).unwrap_or_default(),
@@ -807,15 +807,15 @@ where
 	pub fn run_instantiate(
 		origin: T::AccountId,
 		executable: E,
-		gas_meter: &'a mut GasMeter<T>,
-		storage_meter: &'a mut storage::meter::Meter<T>,
+		gas_meter: &mut GasMeter<T>,
+		storage_meter: &mut storage::meter::Meter<T>,
 		value: U256,
 		input_data: Vec<u8>,
 		salt: Option<&[u8; 32]>,
 		skip_transfer: bool,
 		nonce_already_incremented: NonceAlreadyIncremented,
 	) -> Result<(H160, ExecReturnValue), ExecError> {
-		let (mut stack, executable) = Self::new(
+		let (mut stack, executable) = Stack::<'_, T, E>::new(
 			FrameArgs::Instantiate {
 				sender: origin.clone(),
 				executable,
@@ -1165,6 +1165,7 @@ where
 					&caller,
 					account_id,
 					frame.value_transferred,
+					&mut frame.nested_storage,
 				)?;
 			}
 
@@ -1365,11 +1366,12 @@ where
 	/// not exist, the transfer does fail and nothing will be sent to `to` if either `origin` can
 	/// not provide the ED or transferring `value` from `from` to `to` fails.
 	/// Note: This will also fail if `origin` is root.
-	fn transfer(
+	fn transfer<S: storage::meter::State + Default + Debug>(
 		origin: &Origin<T>,
 		from: &T::AccountId,
 		to: &T::AccountId,
 		value: U256,
+		storage_meter: &mut storage::meter::GenericMeter<T, S>,
 	) -> ExecResult {
 		let value = crate::Pallet::<T>::convert_evm_to_native(value, ConversionPrecision::Exact)?;
 		if value.is_zero() {
@@ -1391,18 +1393,24 @@ where
 					T::Currency::transfer(from, to, value, Preservation::Preserve)
 						.map_err(|_| Error::<T>::TransferFailed.into())
 				}) {
-				Ok(_) => TransactionOutcome::Commit(Ok(Default::default())),
+				Ok(_) => {
+					// ed is taken from the transaction signer so it should be
+					// limited by the storage deposit
+					storage_meter.record_charge(&StorageDeposit::Charge(ed));
+					TransactionOutcome::Commit(Ok(Default::default()))
+				},
 				Err(err) => TransactionOutcome::Rollback(Err(err)),
 			}
 		})
 	}
 
 	/// Same as `transfer` but `from` is an `Origin`.
-	fn transfer_from_origin(
+	fn transfer_from_origin<S: storage::meter::State + Default + Debug>(
 		origin: &Origin<T>,
 		from: &Origin<T>,
 		to: &T::AccountId,
 		value: U256,
+		storage_meter: &mut storage::meter::GenericMeter<T, S>,
 	) -> ExecResult {
 		// If the from address is root there is no account to transfer from, and therefore we can't
 		// take any `value` other than 0.
@@ -1411,7 +1419,7 @@ where
 			Origin::Root if value.is_zero() => return Ok(Default::default()),
 			Origin::Root => return Err(DispatchError::RootNotAllowed.into()),
 		};
-		Self::transfer(origin, from, to, value)
+		Self::transfer(origin, from, to, value, storage_meter)
 	}
 
 	/// Reference to the current (top) frame.
@@ -1764,11 +1772,14 @@ where
 				} else if is_read_only {
 					Err(Error::<T>::StateChangeDenied.into())
 				} else {
+					let account_id = self.account_id().clone();
+					let frame = top_frame_mut!(self);
 					Self::transfer_from_origin(
 						&self.origin,
-						&Origin::from_account_id(self.account_id().clone()),
+						&Origin::from_account_id(account_id),
 						&dest,
 						value,
+						&mut frame.nested_storage,
 					)
 				};
 

substrate/frame/revive/src/exec/tests.rs

@@ -235,12 +235,19 @@ fn transfer_works() {
 		set_balance(&ALICE, 100);
 		set_balance(&BOB, 0);
 
+		let value = 55;
 		let origin = Origin::from_account_id(ALICE);
-		MockStack::transfer(&origin, &ALICE, &BOB, 55u64.into()).unwrap();
+		let mut storage_meter = storage::meter::Meter::new(u64::MAX);
+		MockStack::transfer(&origin, &ALICE, &BOB, value.into(), &mut storage_meter).unwrap();
 
 		let min_balance = <Test as Config>::Currency::minimum_balance();
-		assert_eq!(get_balance(&ALICE), 45 - min_balance);
-		assert_eq!(get_balance(&BOB), 55 + min_balance);
+		assert!(min_balance > 0);
+		assert_eq!(get_balance(&ALICE), 100 - value - min_balance);
+		assert_eq!(get_balance(&BOB), min_balance + value);
+		assert_eq!(
+			storage_meter.try_into_deposit(&Origin::from_account_id(ALICE), false).unwrap(),
+			StorageDeposit::Charge(min_balance)
+		);
 	});
 }
 
@@ -252,6 +259,7 @@ fn transfer_to_nonexistent_account_works() {
 	ExtBuilder::default().build().execute_with(|| {
 		let ed = <Test as Config>::Currency::minimum_balance();
 		let value = 1024;
+		let mut storage_meter = storage::meter::Meter::new(u64::MAX);
 
 		// Transfers to nonexistant accounts should work
 		set_balance(&ALICE, ed * 2);
@@ -261,7 +269,8 @@ fn transfer_to_nonexistent_account_works() {
 			&Origin::from_account_id(ALICE),
 			&BOB,
 			&CHARLIE,
-			value.into()
+			value.into(),
+			&mut storage_meter,
 		));
 		assert_eq!(get_balance(&ALICE), ed);
 		assert_eq!(get_balance(&BOB), ed);
@@ -271,15 +280,27 @@ fn transfer_to_nonexistent_account_works() {
 		set_balance(&ALICE, ed);
 		set_balance(&BOB, ed + value);
 		assert_err!(
-			MockStack::transfer(&Origin::from_account_id(ALICE), &BOB, &DJANGO, value.into()),
+			MockStack::transfer(
+				&Origin::from_account_id(ALICE),
+				&BOB,
+				&DJANGO,
+				value.into(),
+				&mut storage_meter
+			),
 			<Error<Test>>::StorageDepositNotEnoughFunds,
 		);
 
 		// Do not reap the sender account
 		set_balance(&ALICE, ed * 2);
 		set_balance(&BOB, value);
 		assert_err!(
-			MockStack::transfer(&Origin::from_account_id(ALICE), &BOB, &EVE, value.into()),
+			MockStack::transfer(
+				&Origin::from_account_id(ALICE),
+				&BOB,
+				&EVE,
+				value.into(),
+				&mut storage_meter
+			),
 			<Error<Test>>::TransferFailed
 		);
 		// The ED transfer would work. But it should only be executed with the actual transfer
@@ -444,9 +465,15 @@ fn balance_too_low() {
 		let ed = <Test as Config>::Currency::minimum_balance();
 		set_balance(&ALICE, ed * 2);
 		set_balance(&from, ed + 99);
+		let mut storage_meter = storage::meter::Meter::new(u64::MAX);
 
-		let result =
-			MockStack::transfer(&Origin::from_account_id(ALICE), &from, &dest, 100u64.into());
+		let result = MockStack::transfer(
+			&Origin::from_account_id(ALICE),
+			&from,
+			&dest,
+			100u64.into(),
+			&mut storage_meter,
+		);
 
 		assert_eq!(result, Err(Error::<Test>::TransferFailed.into()));
 		assert_eq!(get_balance(&ALICE), ed * 2);

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

@@ -304,6 +304,14 @@ where
 		}
 	}
 
+	/// Record a charge that has taken place externally.
+	///
+	/// This will not perform a charge. It just records it to reflect it in the
+	/// total amount of storage required for a transaction.
+	pub fn record_charge(&mut self, amount: &DepositOf<T>) {
+		self.total_deposit = self.total_deposit.saturating_add(&amount);
+	}
+
 	/// The amount of balance that is still available from the original `limit`.
 	fn available(&self) -> BalanceOf<T> {
 		self.total_deposit.available(&self.limit)
@@ -397,14 +405,6 @@ impl<T: Config, E: Ext<T>> RawMeter<T, E, Nested> {
 		self.charges.push(Charge { contract, amount, state: ContractState::Alive });
 	}
 
-	/// Record a charge that has taken place externally.
-	///
-	/// This will not perform a charge. It just records it to reflect it in the
-	/// total amount of storage required for a transaction.
-	pub fn record_charge(&mut self, amount: &DepositOf<T>) {
-		self.total_deposit = self.total_deposit.saturating_add(&amount);
-	}
-
 	/// Call to tell the meter that the currently executing contract was terminated.
 	///
 	/// This will manipulate the meter so that all storage deposit accumulated in