From 909cbae0601fd34589a16c9bd926865d050f4c3e Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 16:40:36 +0200
Subject: [PATCH 01/18] wip

---
 Cargo.lock                                    |  1 +
 frame/contracts/Cargo.toml                    |  1 +
 .../fixtures/call_runtime_and_call.wat        | 56 +++++++++++
 frame/contracts/src/lib.rs                    | 15 ++-
 frame/contracts/src/storage/meter.rs          | 64 ++++---------
 frame/contracts/src/tests.rs                  | 96 +++++++++++++++++++
 6 files changed, 185 insertions(+), 48 deletions(-)
 create mode 100644 frame/contracts/fixtures/call_runtime_and_call.wat

diff --git a/Cargo.lock b/Cargo.lock
index aa7d22daccb80..a14afdeb69359 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -5922,6 +5922,7 @@ dependencies = [
  "pallet-contracts-primitives",
  "pallet-contracts-proc-macro",
  "pallet-insecure-randomness-collective-flip",
+ "pallet-proxy",
  "pallet-timestamp",
  "pallet-utility",
  "parity-scale-codec",
diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml
index 6ab60846fd412..edb6d294cfcd5 100644
--- a/frame/contracts/Cargo.toml
+++ b/frame/contracts/Cargo.toml
@@ -59,6 +59,7 @@ pallet-balances = { version = "4.0.0-dev", path = "../balances" }
 pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" }
 pallet-insecure-randomness-collective-flip = { version = "4.0.0-dev", path = "../insecure-randomness-collective-flip" }
 pallet-utility = { version = "4.0.0-dev", path = "../utility" }
+pallet-proxy = { version = "4.0.0-dev", path = "../proxy" }
 sp-keystore = { version = "0.13.0", path = "../../primitives/keystore" }
 
 [features]
diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat
new file mode 100644
index 0000000000000..4a6197fc1cb0d
--- /dev/null
+++ b/frame/contracts/fixtures/call_runtime_and_call.wat
@@ -0,0 +1,56 @@
+(module
+	(import "seal0" "call_runtime" (func $call_runtime (param i32 i32) (result i32)))
+	(import "seal1" "seal_call" (func $seal_call (param i32 i32 i64 i32 i32 i32 i32 i32) (result i32)))
+	(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
+	(import "seal0" "seal_return" (func $seal_return (param i32 i32 i32)))
+	(import "env" "memory" (memory 1 1))
+
+	(func $assert (param i32)
+		(block $ok
+			(br_if $ok (get_local 0))
+			(unreachable)
+		)
+	)
+
+	(func (export "call")
+	    ;; store available input size at offset 0
+        (i32.store (i32.const 0) (i32.const 512))
+
+		;; read input data
+        (call $seal_input (i32.const 4) (i32.const 0))
+
+		;; input data layout
+		;; [0..4) - size of the call
+		;; [4..8) - storage length
+		;; [8..40) - hash code of the callee
+		;; [40..n) - encoded runtime call
+
+		;; invoke call_runtime with the encoded call passed to this contract
+		(call $assert (i32.eqz
+			(call $call_runtime
+				(i32.const 40)                ;; Pointer where the call is stored
+				(i32.sub
+					(i32.load (i32.const 0))  ;; Size of the call
+					(i32.const 36)            ;; Size of the call without the call_runtime
+				)
+			)
+		))
+
+		;; call passed contract
+		(call $assert (i32.eqz
+			(call $seal_call
+				(i32.const 0) ;; No flags
+				(i32.const 8)  ;; Pointer to "callee" address.
+				(i64.const 0)  ;; How much gas to devote for the execution. 0 = all.
+				(i32.const 512) ;; Pointer to the buffer with value to transfer
+				(i32.const 4) ;; Pointer to input data buffer address
+				(i32.const 4)  ;; Length of input data buffer
+				(i32.const 4294967295) ;; u32 max value is the sentinel value: do not copy output
+				(i32.const 0) ;; Length is ignored in this case
+			)
+		))
+	)
+
+	(func (export "deploy"))
+)
+
diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
index fa230a8cade14..4a3b0d20b7453 100644
--- a/frame/contracts/src/lib.rs
+++ b/frame/contracts/src/lib.rs
@@ -1033,7 +1033,15 @@ impl<T: Config> Invokable<T> for CallInput<T> {
 			debug_message,
 			*determinism,
 		);
-		InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(&origin), result }
+
+		match storage_meter.try_into_deposit(&origin) {
+			Ok(storage_deposit) => InternalOutput { gas_meter, result, storage_deposit },
+			Err(err) => InternalOutput {
+				result: Err(err.into()),
+				gas_meter,
+				storage_deposit: Default::default(),
+			},
+		}
 	}
 }
 
@@ -1094,9 +1102,8 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 				&salt,
 				debug_message,
 			);
-			storage_deposit = storage_meter
-				.into_deposit(&origin)
-				.saturating_add(&StorageDeposit::Charge(extra_deposit));
+
+			storage_deposit = storage_meter.try_into_deposit(&origin)?;
 			result
 		};
 		InternalOutput { result: try_exec(), gas_meter, storage_deposit }
diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs
index 497ee03ac319a..7f0b12d3be6ba 100644
--- a/frame/contracts/src/storage/meter.rs
+++ b/frame/contracts/src/storage/meter.rs
@@ -82,7 +82,7 @@ pub trait Ext<T: Config> {
 		deposit_account: &DepositAccount<T>,
 		amount: &DepositOf<T>,
 		terminated: bool,
-	);
+	) -> Result<(), DispatchError>;
 }
 
 /// This [`Ext`] is used for actual on-chain execution when balance needs to be charged.
@@ -343,14 +343,14 @@ where
 	///
 	/// This drops the root meter in order to make sure it is only called when the whole
 	/// execution did finish.
-	pub fn into_deposit(self, origin: &T::AccountId) -> DepositOf<T> {
+	pub fn try_into_deposit(self, origin: &T::AccountId) -> Result<DepositOf<T>, DispatchError> {
 		for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) {
-			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
+			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
 		}
 		for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) {
-			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
+			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
 		}
-		self.total_deposit
+		Ok(self.total_deposit)
 	}
 }
 
@@ -405,7 +405,8 @@ where
 			info.deposit_account(),
 			&deposit.saturating_sub(&Deposit::Charge(ed)),
 			false,
-		);
+		)?;
+
 		System::<T>::inc_consumers(info.deposit_account())?;
 
 		// We also need to make sure that the contract's account itself exists.
@@ -476,41 +477,14 @@ impl<T: Config> Ext<T> for ReservingExt {
 		deposit_account: &DepositAccount<T>,
 		amount: &DepositOf<T>,
 		terminated: bool,
-	) {
-		// There is nothing we can do when this fails as this constitutes a bug in the runtime.
-		// We need to settle for emitting an error log in this case.
-		//
-		// # Note
-		//
-		// This is infallible because it is called in a part of the execution where we cannot
-		// simply roll back. It might make sense to do some refactoring to move the deposit
-		// collection to the fallible part of execution.
+	) -> Result<(), DispatchError> {
 		match amount {
-			Deposit::Charge(amount) => {
-				// This will never fail because a deposit account is required to exist
-				// at all times. The pallet enforces this invariant by holding a consumer reference
-				// on the deposit account as long as the contract exists.
-				//
-				// The sender always has enough balance because we checked that it had enough
-				// balance when instantiating the storage meter. There is no way for the sender
-				// which is a plain account to send away this balance in the meantime.
-				let result = T::Currency::transfer(
-					origin,
-					deposit_account,
-					*amount,
-					ExistenceRequirement::KeepAlive,
-				);
-				if let Err(err) = result {
-					log::error!(
-						target: "runtime::contracts",
-						"Failed to transfer storage deposit {:?} from origin {:?} to deposit account {:?}: {:?}",
-						amount, origin, deposit_account, err,
-					);
-					if cfg!(debug_assertions) {
-						panic!("Unable to collect storage deposit. This is a bug.");
-					}
-				}
-			},
+			Deposit::Charge(amount) => T::Currency::transfer(
+				origin,
+				deposit_account,
+				*amount,
+				ExistenceRequirement::KeepAlive,
+			),
 			// The receiver always exists because the initial value transfer from the
 			// origin to the contract has a keep alive existence requirement. When taking a deposit
 			// we make sure to leave at least the ed in the free balance.
@@ -539,8 +513,9 @@ impl<T: Config> Ext<T> for ReservingExt {
 						panic!("Unable to refund storage deposit. This is a bug.");
 					}
 				}
+				Ok(())
 			},
-		};
+		}
 	}
 }
 
@@ -613,7 +588,7 @@ mod tests {
 			contract: &DepositAccount<Test>,
 			amount: &DepositOf<Test>,
 			terminated: bool,
-		) {
+		) -> Result<(), DispatchError> {
 			TestExtTestValue::mutate(|ext| {
 				ext.charges.push(Charge {
 					origin: origin.clone(),
@@ -622,6 +597,7 @@ mod tests {
 					terminated,
 				})
 			});
+			Ok(())
 		}
 	}
 
@@ -719,7 +695,7 @@ mod tests {
 		nested0.enforce_limit(Some(&mut nested0_info)).unwrap();
 		meter.absorb(nested0, DepositAccount(BOB), Some(&mut nested0_info));
 
-		meter.into_deposit(&ALICE);
+		meter.try_into_deposit(&ALICE).unwrap();
 
 		assert_eq!(nested0_info.extra_deposit(), 112);
 		assert_eq!(nested1_info.extra_deposit(), 110);
@@ -779,7 +755,7 @@ mod tests {
 		nested0.absorb(nested1, DepositAccount(CHARLIE), None);
 
 		meter.absorb(nested0, DepositAccount(BOB), None);
-		meter.into_deposit(&ALICE);
+		meter.try_into_deposit(&ALICE).unwrap();
 
 		assert_eq!(
 			TestExtTestValue::get(),
diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 4e6c468f19c81..ffc9e408a5944 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -70,6 +70,7 @@ frame_support::construct_runtime!(
 		Randomness: pallet_insecure_randomness_collective_flip::{Pallet, Storage},
 		Utility: pallet_utility::{Pallet, Call, Storage, Event},
 		Contracts: pallet_contracts::{Pallet, Call, Storage, Event<T>},
+		Proxy: pallet_proxy::{Pallet, Call, Storage, Event<T>},
 	}
 );
 
@@ -337,6 +338,22 @@ impl pallet_utility::Config for Test {
 	type PalletsOrigin = OriginCaller;
 	type WeightInfo = ();
 }
+
+impl pallet_proxy::Config for Test {
+	type RuntimeEvent = RuntimeEvent;
+	type RuntimeCall = RuntimeCall;
+	type Currency = Balances;
+	type ProxyType = ();
+	type ProxyDepositBase = ConstU64<1>;
+	type ProxyDepositFactor = ConstU64<1>;
+	type MaxProxies = ConstU32<32>;
+	type WeightInfo = ();
+	type MaxPending = ConstU32<32>;
+	type CallHasher = BlakeTwo256;
+	type AnnouncementDepositBase = ConstU64<1>;
+	type AnnouncementDepositFactor = ConstU64<1>;
+}
+
 parameter_types! {
 	pub MySchedule: Schedule<Test> = {
 		let mut schedule = <Schedule<Test>>::default();
@@ -2966,6 +2983,85 @@ fn sr25519_verify() {
 	})
 }
 
+#[test]
+fn test_failed_charge_should_roll_back_call() {
+	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
+	let (wasm_callee, _) = compile_module::<Test>("store").unwrap();
+
+	ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
+		let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+
+		// Instantiate both contracts.
+		let addr_caller = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Upload(wasm_caller),
+			vec![],
+			vec![],
+			false,
+		)
+		.result
+		.unwrap()
+		.account_id;
+		let addr_callee = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Upload(wasm_callee),
+			vec![],
+			vec![],
+			false,
+		)
+		.result
+		.unwrap()
+		.account_id;
+
+		// Give caller proxy access to caller
+		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
+
+		// create a Proxy call that will create a transfer onbehalf of Alice
+		let value = pallet_balances::Pallet::<Test>::free_balance(&ALICE) -
+			<Test as Config>::Currency::minimum_balance();
+
+		let transfer_call =
+			Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
+				dest: CHARLIE,
+				value,
+			}));
+
+		// wrap the transfer call in a proxy call
+		let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy {
+			real: ALICE,
+			force_proxy_type: Some(()),
+			call: transfer_call,
+		});
+
+		// encode inputs data
+		let data = (
+			30u32, // storage length
+			addr_callee.clone(),
+			transfer_proxy_call,
+		);
+
+		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 1_000);
+
+		// dispatch the call
+		let result = <Pallet<Test>>::call(
+			RuntimeOrigin::signed(ALICE),
+			addr_caller.clone(),
+			0,
+			GAS_LIMIT,
+			None, //Some(5_000),
+			data.encode(),
+		);
+
+		assert_err_ignore_postinfo!(result, TokenError::FundsUnavailable);
+	})
+}
+
 #[test]
 fn upload_code_works() {
 	let (wasm, code_hash) = compile_module::<Test>("dummy").unwrap();

From fb0cc688737198b320eabca0cc14e2753231727d Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 16:51:27 +0200
Subject: [PATCH 02/18] add comments

---
 frame/contracts/src/tests.rs | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index ffc9e408a5944..d669505e8ed54 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3022,14 +3022,13 @@ fn test_failed_charge_should_roll_back_call() {
 		// Give caller proxy access to caller
 		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
 
-		// create a Proxy call that will create a transfer onbehalf of Alice
-		let value = pallet_balances::Pallet::<Test>::free_balance(&ALICE) -
-			<Test as Config>::Currency::minimum_balance();
-
+		// create a Proxy call that will attempt to transfer away's Alice balance, so we can test a
+		// failed charge
+		let min_balance = <Test as Config>::Currency::minimum_balance();
 		let transfer_call =
 			Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
 				dest: CHARLIE,
-				value,
+				value: pallet_balances::Pallet::<Test>::free_balance(&ALICE) - min_balance,
 			}));
 
 		// wrap the transfer call in a proxy call
@@ -3039,14 +3038,15 @@ fn test_failed_charge_should_roll_back_call() {
 			call: transfer_call,
 		});
 
-		// encode inputs data
+		// inputs data
 		let data = (
-			30u32, // storage length
+			100u32, // storage length
 			addr_callee.clone(),
 			transfer_proxy_call,
 		);
 
-		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 1_000);
+		// bump the deposit per byte to a high value to trigger a FundsUnavailable error
+		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 100);
 
 		// dispatch the call
 		let result = <Pallet<Test>>::call(
@@ -3054,7 +3054,7 @@ fn test_failed_charge_should_roll_back_call() {
 			addr_caller.clone(),
 			0,
 			GAS_LIMIT,
-			None, //Some(5_000),
+			None,
 			data.encode(),
 		);
 

From 81859b611c54df64fd22fa3933255648a3ff42d9 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 16:59:21 +0200
Subject: [PATCH 03/18] fix comment

---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index d669505e8ed54..698402cf83ea9 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3019,7 +3019,7 @@ fn test_failed_charge_should_roll_back_call() {
 		.unwrap()
 		.account_id;
 
-		// Give caller proxy access to caller
+		// Give caller proxy access to Alice
 		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
 
 		// create a Proxy call that will attempt to transfer away's Alice balance, so we can test a

From 0ccd804801199cd714ee2e9698e7c9258c7651c3 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 17:01:39 +0200
Subject: [PATCH 04/18] comments

---
 frame/contracts/src/tests.rs | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 698402cf83ea9..e67d74024d475 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3019,11 +3019,11 @@ fn test_failed_charge_should_roll_back_call() {
 		.unwrap()
 		.account_id;
 
-		// Give caller proxy access to Alice
+		// Give caller proxy access to Alice.
 		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
 
-		// create a Proxy call that will attempt to transfer away's Alice balance, so we can test a
-		// failed charge
+		// Create a Proxy call that will attempt to transfer away's Alice balance, so we can test a
+		// failed charge.
 		let min_balance = <Test as Config>::Currency::minimum_balance();
 		let transfer_call =
 			Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
@@ -3031,24 +3031,22 @@ fn test_failed_charge_should_roll_back_call() {
 				value: pallet_balances::Pallet::<Test>::free_balance(&ALICE) - min_balance,
 			}));
 
-		// wrap the transfer call in a proxy call
+		// Wrap the transfer call in a proxy call.
 		let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy {
 			real: ALICE,
 			force_proxy_type: Some(()),
 			call: transfer_call,
 		});
 
-		// inputs data
+		// bump the deposit per byte to a high value to trigger a FundsUnavailable error.
+		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 100);
+
 		let data = (
 			100u32, // storage length
-			addr_callee.clone(),
+			addr_callee,
 			transfer_proxy_call,
 		);
 
-		// bump the deposit per byte to a high value to trigger a FundsUnavailable error
-		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 100);
-
-		// dispatch the call
 		let result = <Pallet<Test>>::call(
 			RuntimeOrigin::signed(ALICE),
 			addr_caller.clone(),

From a5a63da2213d1f56c7e95fbb99578d6fda070a88 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 17:03:24 +0200
Subject: [PATCH 05/18] comments

---
 frame/contracts/fixtures/call_runtime_and_call.wat | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat
index 4a6197fc1cb0d..b8225efe9b099 100644
--- a/frame/contracts/fixtures/call_runtime_and_call.wat
+++ b/frame/contracts/fixtures/call_runtime_and_call.wat
@@ -13,19 +13,19 @@
 	)
 
 	(func (export "call")
-	    ;; store available input size at offset 0
+	    ;; Store available input size at offset 0.
         (i32.store (i32.const 0) (i32.const 512))
 
 		;; read input data
         (call $seal_input (i32.const 4) (i32.const 0))
 
-		;; input data layout
+		;; Input data layout.
 		;; [0..4) - size of the call
 		;; [4..8) - storage length
 		;; [8..40) - hash code of the callee
 		;; [40..n) - encoded runtime call
 
-		;; invoke call_runtime with the encoded call passed to this contract
+		;; Invoke call_runtime with the encoded call passed to this contract.
 		(call $assert (i32.eqz
 			(call $call_runtime
 				(i32.const 40)                ;; Pointer where the call is stored

From 21e2fc10b516e5f73cac8960bc472b4abdc1eb6e Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Mon, 24 Apr 2023 18:15:39 +0200
Subject: [PATCH 06/18] PR comment

---
 frame/contracts/src/lib.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
index 4a3b0d20b7453..5329768e6375e 100644
--- a/frame/contracts/src/lib.rs
+++ b/frame/contracts/src/lib.rs
@@ -1103,7 +1103,9 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 				debug_message,
 			);
 
-			storage_deposit = storage_meter.try_into_deposit(&origin)?;
+			storage_deposit = storage_meter
+				.try_into_deposit(&origin)?
+				.saturating_add(&StorageDeposit::Charge(extra_deposit));
 			result
 		};
 		InternalOutput { result: try_exec(), gas_meter, storage_deposit }

From c81bde429d81d80d4a2c048ca2fe14c3865583a8 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 09:32:48 +0200
Subject: [PATCH 07/18] field orders

---
 frame/contracts/src/lib.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
index 5329768e6375e..f710a08359d0e 100644
--- a/frame/contracts/src/lib.rs
+++ b/frame/contracts/src/lib.rs
@@ -1035,11 +1035,11 @@ impl<T: Config> Invokable<T> for CallInput<T> {
 		);
 
 		match storage_meter.try_into_deposit(&origin) {
-			Ok(storage_deposit) => InternalOutput { gas_meter, result, storage_deposit },
+			Ok(storage_deposit) => InternalOutput { gas_meter, storage_deposit, result },
 			Err(err) => InternalOutput {
-				result: Err(err.into()),
 				gas_meter,
 				storage_deposit: Default::default(),
+				result: Err(err.into()),
 			},
 		}
 	}

From 60c2bf47bca3ee07011ef54343d89d784fd55b7f Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 10:06:27 +0200
Subject: [PATCH 08/18] Update frame/contracts/src/tests.rs

---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index e67d74024d475..d9083cdf6e212 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3038,7 +3038,7 @@ fn test_failed_charge_should_roll_back_call() {
 			call: transfer_call,
 		});
 
-		// bump the deposit per byte to a high value to trigger a FundsUnavailable error.
+		// Bump the deposit per byte to a high value to trigger a FundsUnavailable error.
 		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 100);
 
 		let data = (

From cd177fcc12e4394d7c802da2d723424a70b5adb8 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 23:01:47 +0200
Subject: [PATCH 09/18] Update
 frame/contracts/fixtures/call_runtime_and_call.wat

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
---
 frame/contracts/fixtures/call_runtime_and_call.wat | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat
index b8225efe9b099..8b08c98b2dc5d 100644
--- a/frame/contracts/fixtures/call_runtime_and_call.wat
+++ b/frame/contracts/fixtures/call_runtime_and_call.wat
@@ -21,7 +21,7 @@
 
 		;; Input data layout.
 		;; [0..4) - size of the call
-		;; [4..8) - storage length
+		;; [4..8) - how many bytes to add to storage
 		;; [8..40) - hash code of the callee
 		;; [40..n) - encoded runtime call
 

From 672070bdcc9caa5401ec09087e7aee1a1689a39a Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 23:02:13 +0200
Subject: [PATCH 10/18] Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
---
 frame/contracts/fixtures/call_runtime_and_call.wat | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat
index 8b08c98b2dc5d..3320922d9e2cb 100644
--- a/frame/contracts/fixtures/call_runtime_and_call.wat
+++ b/frame/contracts/fixtures/call_runtime_and_call.wat
@@ -22,7 +22,7 @@
 		;; Input data layout.
 		;; [0..4) - size of the call
 		;; [4..8) - how many bytes to add to storage
-		;; [8..40) - hash code of the callee
+		;; [8..40) - address of the callee
 		;; [40..n) - encoded runtime call
 
 		;; Invoke call_runtime with the encoded call passed to this contract.
@@ -31,7 +31,7 @@
 				(i32.const 40)                ;; Pointer where the call is stored
 				(i32.sub
 					(i32.load (i32.const 0))  ;; Size of the call
-					(i32.const 36)            ;; Size of the call without the call_runtime
+					(i32.const 36)            ;; Subtract size of the subcall-related part: 4 bytes for storage length to add + 32 bytes of the callee address 
 				)
 			)
 		))

From ead8ba3d8ea2e43d3bc9ff5c17d45cacf4cd113a Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 23:03:41 +0200
Subject: [PATCH 11/18] Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index d9083cdf6e212..5af0e4a5a3a05 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -2984,7 +2984,7 @@ fn sr25519_verify() {
 }
 
 #[test]
-fn test_failed_charge_should_roll_back_call() {
+fn failed_deposit_charge_should_roll_back_call() {
 	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
 	let (wasm_callee, _) = compile_module::<Test>("store").unwrap();
 

From 826bfa76bbcc6cce82d088d94ce1cc3a491bcc14 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Tue, 25 Apr 2023 23:03:59 +0200
Subject: [PATCH 12/18] Update frame/contracts/src/tests.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 5af0e4a5a3a05..80457d999b2ae 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3022,7 +3022,7 @@ fn failed_deposit_charge_should_roll_back_call() {
 		// Give caller proxy access to Alice.
 		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
 
-		// Create a Proxy call that will attempt to transfer away's Alice balance, so we can test a
+		// Create a Proxy call that will attempt to transfer away Alice's balance, so we can test a
 		// failed charge.
 		let min_balance = <Test as Config>::Currency::minimum_balance();
 		let transfer_call =

From faf1b125583a3c97265c72d579b63add435a41f6 Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Wed, 26 Apr 2023 09:22:19 +0200
Subject: [PATCH 13/18] Validate fees of failed call

---
 frame/contracts/src/tests.rs | 136 ++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 66 deletions(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 80457d999b2ae..061a4fe67c2a8 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -32,7 +32,7 @@ use crate::{
 use assert_matches::assert_matches;
 use codec::Encode;
 use frame_support::{
-	assert_err, assert_err_ignore_postinfo, assert_noop, assert_ok,
+	assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok,
 	dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo},
 	parameter_types,
 	storage::child,
@@ -2987,77 +2987,81 @@ fn sr25519_verify() {
 fn failed_deposit_charge_should_roll_back_call() {
 	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
 	let (wasm_callee, _) = compile_module::<Test>("store").unwrap();
+	const ED: u64 = 200;
 
-	ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
-		let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+	let execute = || {
+		ExtBuilder::default().existential_deposit(ED).build().execute_with(|| {
+			let _ = Balances::deposit_creating(&ALICE, 1_000_000);
 
-		// Instantiate both contracts.
-		let addr_caller = Contracts::bare_instantiate(
-			ALICE,
-			0,
-			GAS_LIMIT,
-			None,
-			Code::Upload(wasm_caller),
-			vec![],
-			vec![],
-			false,
-		)
-		.result
-		.unwrap()
-		.account_id;
-		let addr_callee = Contracts::bare_instantiate(
-			ALICE,
-			0,
-			GAS_LIMIT,
-			None,
-			Code::Upload(wasm_callee),
-			vec![],
-			vec![],
-			false,
-		)
-		.result
-		.unwrap()
-		.account_id;
-
-		// Give caller proxy access to Alice.
-		assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
-
-		// Create a Proxy call that will attempt to transfer away Alice's balance, so we can test a
-		// failed charge.
-		let min_balance = <Test as Config>::Currency::minimum_balance();
-		let transfer_call =
-			Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
-				dest: CHARLIE,
-				value: pallet_balances::Pallet::<Test>::free_balance(&ALICE) - min_balance,
-			}));
-
-		// Wrap the transfer call in a proxy call.
-		let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy {
-			real: ALICE,
-			force_proxy_type: Some(()),
-			call: transfer_call,
-		});
+			// Instantiate both contracts.
+			let addr_caller = Contracts::bare_instantiate(
+				ALICE,
+				0,
+				GAS_LIMIT,
+				None,
+				Code::Upload(wasm_caller.clone()),
+				vec![],
+				vec![],
+				false,
+			)
+			.result
+			.unwrap()
+			.account_id;
+			let addr_callee = Contracts::bare_instantiate(
+				ALICE,
+				0,
+				GAS_LIMIT,
+				None,
+				Code::Upload(wasm_callee.clone()),
+				vec![],
+				vec![],
+				false,
+			)
+			.result
+			.unwrap()
+			.account_id;
 
-		// Bump the deposit per byte to a high value to trigger a FundsUnavailable error.
-		DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 100);
+			// Give caller proxy access to Alice.
+			assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
+
+			// Create a Proxy call that will attempt to transfer away Alice's balance
+			let transfer_call =
+				Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
+					dest: CHARLIE,
+					value: pallet_balances::Pallet::<Test>::free_balance(&ALICE) - 2 * ED,
+				}));
+
+			// Wrap the transfer call in a proxy call.
+			let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy {
+				real: ALICE,
+				force_proxy_type: Some(()),
+				call: transfer_call,
+			});
+
+			let data = (
+				(ED - DepositPerItem::get()) as u32, // storage length
+				addr_callee,
+				transfer_proxy_call,
+			);
+
+			<Pallet<Test>>::call(
+				RuntimeOrigin::signed(ALICE),
+				addr_caller.clone(),
+				0,
+				GAS_LIMIT,
+				None,
+				data.encode(),
+			)
+		})
+	};
 
-		let data = (
-			100u32, // storage length
-			addr_callee,
-			transfer_proxy_call,
-		);
+	// With a low enough deposit per byte, the call should succeed.
+	let result = execute().unwrap();
 
-		let result = <Pallet<Test>>::call(
-			RuntimeOrigin::signed(ALICE),
-			addr_caller.clone(),
-			0,
-			GAS_LIMIT,
-			None,
-			data.encode(),
-		);
+	// Bump the deposit per byte to a high value to trigger a FundsUnavailable error.
+	DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = ED);
 
-		assert_err_ignore_postinfo!(result, TokenError::FundsUnavailable);
-	})
+	assert_err_with_weight!(execute(), TokenError::FundsUnavailable, result.actual_weight);
 }
 
 #[test]

From fe799018d0c81c7371a23677e9368a41b4c3efa0 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Wed, 26 Apr 2023 10:24:34 +0200
Subject: [PATCH 14/18] Update frame/contracts/src/tests.rs

---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 061a4fe67c2a8..b5ffb32daec79 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3024,7 +3024,7 @@ fn failed_deposit_charge_should_roll_back_call() {
 			// Give caller proxy access to Alice.
 			assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
 
-			// Create a Proxy call that will attempt to transfer away Alice's balance
+			// Create a Proxy call that will attempt to transfer away Alice's balance.
 			let transfer_call =
 				Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
 					dest: CHARLIE,

From 2bed075cc7be5bcbcdf6e7462a59117c287dc1dc Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Wed, 26 Apr 2023 10:25:35 +0200
Subject: [PATCH 15/18] Update frame/contracts/src/tests.rs

---
 frame/contracts/src/tests.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index b5ffb32daec79..9c20a02d0f895 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3060,7 +3060,6 @@ fn failed_deposit_charge_should_roll_back_call() {
 
 	// Bump the deposit per byte to a high value to trigger a FundsUnavailable error.
 	DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = ED);
-
 	assert_err_with_weight!(execute(), TokenError::FundsUnavailable, result.actual_weight);
 }
 

From badc93cc855bd560101efeb3058a0c85d4ba4ea0 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Thu, 27 Apr 2023 11:38:57 +0200
Subject: [PATCH 16/18] Update frame/contracts/src/tests.rs

---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 76ad7c5b09b24..4c90e3501b0a8 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3003,7 +3003,7 @@ fn sr25519_verify() {
 #[test]
 fn failed_deposit_charge_should_roll_back_call() {
 	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
-	let (wasm_callee, _) = compile_module::<Test>("store").unwrap();
+	let (wasm_callee, _) = compile_module::<Test>("call_store").unwrap();
 	const ED: u64 = 200;
 
 	let execute = || {

From 22cea6e74e65b2f29d3580a74affd77336189cae Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Fri, 28 Apr 2023 15:38:48 +0200
Subject: [PATCH 17/18] bubble up refund error

---
 frame/contracts/src/storage/meter.rs | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs
index fb85231dc7871..73794b59774d4 100644
--- a/frame/contracts/src/storage/meter.rs
+++ b/frame/contracts/src/storage/meter.rs
@@ -19,7 +19,7 @@
 
 use crate::{
 	storage::{ContractInfo, DepositAccount},
-	BalanceOf, Config, Error, Inspect, Pallet, System, LOG_TARGET,
+	BalanceOf, Config, Error, Inspect, Pallet, System,
 };
 use codec::Encode;
 use frame_support::{
@@ -523,35 +523,17 @@ impl<T: Config> Ext<T> for ReservingExt {
 				*amount,
 				ExistenceRequirement::KeepAlive,
 			),
-			// The receiver always exists because the initial value transfer from the
-			// origin to the contract has a keep alive existence requirement. When taking a deposit
-			// we make sure to leave at least the ed in the free balance.
-			//
-			// The sender always has enough balance because we track it in the `ContractInfo` and
-			// never send more back than we have. No one has access to the deposit account. Hence no
-			// other interaction with this account takes place.
 			Deposit::Refund(amount) => {
 				if terminated {
 					System::<T>::dec_consumers(&deposit_account);
 				}
-				let result = T::Currency::transfer(
+				T::Currency::transfer(
 					deposit_account,
 					origin,
 					*amount,
 					// We can safely use `AllowDeath` because our own consumer prevents an removal.
 					ExistenceRequirement::AllowDeath,
-				);
-				if matches!(result, Err(_)) {
-					log::error!(
-						target: LOG_TARGET,
-						"Failed to refund storage deposit {:?} from deposit account {:?} to origin {:?}: {:?}",
-						amount, deposit_account, origin, result,
-					);
-					if cfg!(debug_assertions) {
-						panic!("Unable to refund storage deposit. This is a bug.");
-					}
-				}
-				Ok(())
+				)
 			},
 		}
 	}

From 45aae4d7024e14289612a974122b168d93d8e6dc Mon Sep 17 00:00:00 2001
From: pgherveou <pgherveou@gmail.com>
Date: Fri, 28 Apr 2023 15:42:20 +0200
Subject: [PATCH 18/18] rename fixture file

---
 frame/contracts/src/tests.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
index 08513cbb2edd6..c202d3796e3ed 100644
--- a/frame/contracts/src/tests.rs
+++ b/frame/contracts/src/tests.rs
@@ -3003,7 +3003,7 @@ fn sr25519_verify() {
 #[test]
 fn failed_deposit_charge_should_roll_back_call() {
 	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
-	let (wasm_callee, _) = compile_module::<Test>("call_store").unwrap();
+	let (wasm_callee, _) = compile_module::<Test>("store_call").unwrap();
 	const ED: u64 = 200;
 
 	let execute = || {