revive: Fix dust & child contract calls

prdoc/pr_10192.prdoc

@@ -0,0 +1,21 @@
+title: 'revive: Fix dust & child contract calls'
+doc:
+- audience: Runtime Dev
+  description: |-
+    When transferring to self we should just return early as it's a noop.
+    Not doing so cause bug in `transfer_with_dust`
+
+    as we do
+    ```
+    from.dust -= from.dust
+    to.dust += to.dust
+    ```
+
+    We end up with a value of dust - planck (that we burnt from to create dust amount) on the account
+
+    fix https://github.com/paritytech/contract-issues/issues/211
+crates:
+- name: pallet-revive-fixtures
+  bump: patch
+- name: pallet-revive
+  bump: patch

substrate/frame/revive/fixtures/contracts/call_self_with_dust.sol

@@ -0,0 +1,7 @@
+contract CallSelfWithDust {
+    function f() external payable {}
+
+    function call() public payable {
+        this.f{value: 10}();
+    }
+}

substrate/frame/revive/src/exec.rs

@@ -1001,7 +1001,7 @@ where
 							return Ok(None);
 						},
 					(None, Some(precompile)) if precompile.has_contract_info() => {
-						log::trace!(target: crate::LOG_TARGET, "found precompile for address {address:?}");
+						log::trace!(target: LOG_TARGET, "found precompile for address {address:?}");
 						if let Some(info) = AccountInfo::<T>::load_contract(&address) {
 							CachedContract::Cached(info)
 						} else {
@@ -1526,16 +1526,14 @@ where
 			to: &AccountIdOf<T>,
 			value: BalanceWithDust<BalanceOf<T>>,
 		) -> DispatchResult {
-			let (value, dust) = value.deconstruct();
-
 			fn transfer_balance<T: Config>(
 				from: &AccountIdOf<T>,
 				to: &AccountIdOf<T>,
 				value: BalanceOf<T>,
 			) -> DispatchResult {
 				T::Currency::transfer(from, to, value, Preservation::Preserve)
 				.map_err(|err| {
-					log::debug!(target: crate::LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}");
+					log::debug!(target: LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}");
 					Error::<T>::TransferFailed
 				})?;
 				Ok(())
@@ -1552,13 +1550,21 @@ where
 				Ok(())
 			}
 
+			let from_addr = <T::AddressMapper as AddressMapper<T>>::to_address(from);
+			let mut from_info = AccountInfoOf::<T>::get(&from_addr).unwrap_or_default();
+
+			if from_info.balance(from) < value {
+				log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from));
+				return Err(Error::<T>::TransferFailed.into())
+			} else if from == to || value.is_zero() {
+				return Ok(())
+			}
+
+			let (value, dust) = value.deconstruct();
 			if dust.is_zero() {
 				return transfer_balance::<T>(from, to, value)
 			}
 
-			let from_addr = <T::AddressMapper as AddressMapper<T>>::to_address(from);
-			let mut from_info = AccountInfoOf::<T>::get(&from_addr).unwrap_or_default();
-
 			let to_addr = <T::AddressMapper as AddressMapper<T>>::to_address(to);
 			let mut to_info = AccountInfoOf::<T>::get(&to_addr).unwrap_or_default();
 
@@ -1573,7 +1579,7 @@ where
 					Fortitude::Polite,
 				)
 				.map_err(|err| {
-					log::debug!(target: crate::LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}");
+					log::debug!(target: LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}");
 					Error::<T>::TransferFailed
 				})?;
 
@@ -1673,7 +1679,7 @@ where
 
 	/// Returns the *free* balance of the supplied AccountId.
 	fn account_balance(&self, who: &T::AccountId) -> U256 {
-		let balance = AccountInfo::<T>::balance(AccountIdOrAddress::AccountId(who.clone()));
+		let balance = AccountInfo::<T>::balance_of(AccountIdOrAddress::AccountId(who.clone()));
 		crate::Pallet::<T>::convert_native_to_evm(balance)
 	}
 
@@ -1732,7 +1738,7 @@ where
 			delete_code,
 		);
 
-		log::debug!(target: crate::LOG_TARGET, "Contract at {contract_address:?} registered termination. Beneficiary: {beneficiary_address:?}, delete_code: {delete_code}");
+		log::debug!(target: LOG_TARGET, "Contract at {contract_address:?} registered termination. Beneficiary: {beneficiary_address:?}, delete_code: {delete_code}");
 	}
 
 	/// Returns true if the current context has contract info.

substrate/frame/revive/src/lib.rs

@@ -1860,7 +1860,7 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Returns the spendable balance excluding the existential deposit.
 	pub fn evm_balance(address: &H160) -> U256 {
-		let balance = AccountInfo::<T>::balance((*address).into());
+		let balance = AccountInfo::<T>::balance_of((*address).into());
 		Self::convert_native_to_evm(balance)
 	}
 

substrate/frame/revive/src/primitives.rs

@@ -96,7 +96,7 @@ pub enum BalanceConversionError {
 
 /// A Balance amount along with some "dust" to represent the lowest decimals that can't be expressed
 /// in the native currency
-#[derive(Default, Clone, Copy, Eq, PartialEq, Debug)]
+#[derive(Default, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Debug)]
 pub struct BalanceWithDust<Balance> {
 	/// The value expressed in the native currency
 	value: Balance,

substrate/frame/revive/src/storage.rs

@@ -32,6 +32,10 @@ use codec::{Decode, Encode, MaxEncodedLen};
 use core::marker::PhantomData;
 use frame_support::{
 	storage::child::{self, ChildInfo},
+	traits::{
+		fungible::Inspect,
+		tokens::{Fortitude::Polite, Preservation::Preserve},
+	},
 	weights::{Weight, WeightMeter},
 	CloneNoBound, DebugNoBound, DefaultNoBound,
 };
@@ -157,15 +161,15 @@ impl<T: Config> AccountInfo<T> {
 	}
 
 	/// Returns the balance of the account at the given address.
-	pub fn balance(account: AccountIdOrAddress<T>) -> BalanceWithDust<BalanceOf<T>> {
-		use frame_support::traits::{
-			fungible::Inspect,
-			tokens::{Fortitude::Polite, Preservation::Preserve},
-		};
+	pub fn balance_of(account: AccountIdOrAddress<T>) -> BalanceWithDust<BalanceOf<T>> {
+		let info = <AccountInfoOf<T>>::get(account.address()).unwrap_or_default();
+		info.balance(&account.account_id())
+	}
 
-		let value = T::Currency::reducible_balance(&account.account_id(), Preserve, Polite);
-		let dust = <AccountInfoOf<T>>::get(account.address()).map(|a| a.dust).unwrap_or_default();
-		BalanceWithDust::new_unchecked::<T>(value, dust)
+	/// Returns the balance of this account info.
+	pub fn balance(&self, account: &AccountIdOf<T>) -> BalanceWithDust<BalanceOf<T>> {
+		let value = T::Currency::reducible_balance(account, Preserve, Polite);
+		BalanceWithDust::new_unchecked::<T>(value, self.dust)
 	}
 
 	/// Loads the contract information for a given address.

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

@@ -71,86 +71,158 @@ use sp_runtime::{
 fn transfer_with_dust_works() {
 	struct TestCase {
 		description: &'static str,
+		from: H160,
+		to: H160,
 		from_balance: BalanceWithDust<u64>,
 		to_balance: BalanceWithDust<u64>,
 		amount: BalanceWithDust<u64>,
 		expected_from_balance: BalanceWithDust<u64>,
 		expected_to_balance: BalanceWithDust<u64>,
 		total_issuance_diff: i64,
+		expected_error: Option<DispatchError>,
 	}
 
 	let plank: u32 = <Test as Config>::NativeToEthRatio::get();
 
 	let test_cases = vec![
 		TestCase {
 			description: "without dust",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, 0),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, 0),
 			amount: BalanceWithDust::new_unchecked::<Test>(1, 0),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(99, 0),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(1, 0),
 			total_issuance_diff: 0,
+			expected_error: None,
 		},
 		TestCase {
 			description: "with dust",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, 0),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, 0),
 			amount: BalanceWithDust::new_unchecked::<Test>(1, 10),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(98, plank - 10),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(1, 10),
 			total_issuance_diff: 1,
+			expected_error: None,
 		},
 		TestCase {
 			description: "just dust",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, 0),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, 0),
 			amount: BalanceWithDust::new_unchecked::<Test>(0, 10),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(99, plank - 10),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
 			total_issuance_diff: 1,
+			expected_error: None,
 		},
 		TestCase {
 			description: "with existing dust",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, 5),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, plank - 5),
 			amount: BalanceWithDust::new_unchecked::<Test>(1, 10),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(98, plank - 5),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(2, 5),
 			total_issuance_diff: 0,
+			expected_error: None,
 		},
 		TestCase {
 			description: "with enough existing dust",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, 10),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, plank - 10),
 			amount: BalanceWithDust::new_unchecked::<Test>(1, 10),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(99, 0),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(2, 0),
 			total_issuance_diff: -1,
+			expected_error: None,
 		},
 		TestCase {
 			description: "receiver dust less than 1 plank",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
 			from_balance: BalanceWithDust::new_unchecked::<Test>(100, plank / 10),
 			to_balance: BalanceWithDust::new_unchecked::<Test>(0, plank / 2),
 			amount: BalanceWithDust::new_unchecked::<Test>(1, plank / 10 * 3),
 			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(98, plank / 10 * 8),
 			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(1, plank / 10 * 8),
 			total_issuance_diff: 1,
+			expected_error: None,
+		},
+		TestCase {
+			description: "insufficient balance",
+			from: ALICE_ADDR,
+			to: BOB_ADDR,
+			from_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			to_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			amount: BalanceWithDust::new_unchecked::<Test>(20, 0),
+			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			total_issuance_diff: 0,
+			expected_error: Some(Error::<Test>::TransferFailed.into()),
+		},
+		TestCase {
+			description: "from = to with insufficient balance",
+			from: ALICE_ADDR,
+			to: ALICE_ADDR,
+			from_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			to_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			amount: BalanceWithDust::new_unchecked::<Test>(20, 0),
+			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(10, 0),
+			total_issuance_diff: 0,
+			expected_error: Some(Error::<Test>::TransferFailed.into()),
+		},
+		TestCase {
+			description: "from = to with insufficient balance",
+			from: ALICE_ADDR,
+			to: ALICE_ADDR,
+			from_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			to_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			amount: BalanceWithDust::new_unchecked::<Test>(0, 20),
+			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			total_issuance_diff: 0,
+			expected_error: Some(Error::<Test>::TransferFailed.into()),
+		},
+		TestCase {
+			description: "from = to",
+			from: ALICE_ADDR,
+			to: ALICE_ADDR,
+			from_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			to_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			amount: BalanceWithDust::new_unchecked::<Test>(0, 5),
+			expected_from_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			expected_to_balance: BalanceWithDust::new_unchecked::<Test>(0, 10),
+			total_issuance_diff: 0,
+			expected_error: None,
 		},
 	];
 
 	for TestCase {
 		description,
+		from,
+		to,
 		from_balance,
 		to_balance,
 		amount,
 		expected_from_balance,
 		expected_to_balance,
 		total_issuance_diff,
+		expected_error,
 	} in test_cases.into_iter()
 	{
 		ExtBuilder::default().build().execute_with(|| {
-			set_balance_with_dust(&ALICE_ADDR, from_balance);
-			set_balance_with_dust(&BOB_ADDR, to_balance);
+			set_balance_with_dust(&from, from_balance);
+			set_balance_with_dust(&to, to_balance);
 
 			let total_issuance = <Test as Config>::Currency::total_issuance();
 			let evm_value = Pallet::<Test>::convert_native_to_evm(amount);
@@ -159,18 +231,22 @@ fn transfer_with_dust_works() {
 			assert_eq!(Pallet::<Test>::has_dust(evm_value), !dust.is_zero());
 			assert_eq!(Pallet::<Test>::has_balance(evm_value), !value.is_zero());
 
-			let result =
-				builder::bare_call(BOB_ADDR).evm_value(evm_value).build_and_unwrap_result();
-			assert_eq!(result, Default::default(), "{description} tx failed");
+			let result = builder::bare_call(to).evm_value(evm_value).build();
+
+			if let Some(expected_error) = expected_error {
+				assert_err!(result.result, expected_error);
+			} else {
+				assert_eq!(result.result.unwrap(), Default::default(), "{description} tx failed");
+			}
 
 			assert_eq!(
-				Pallet::<Test>::evm_balance(&ALICE_ADDR),
+				Pallet::<Test>::evm_balance(&from),
 				Pallet::<Test>::convert_native_to_evm(expected_from_balance),
 				"{description}: invalid from balance"
 			);
 
 			assert_eq!(
-				Pallet::<Test>::evm_balance(&BOB_ADDR),
+				Pallet::<Test>::evm_balance(&to),
 				Pallet::<Test>::convert_native_to_evm(expected_to_balance),
 				"{description}: invalid to balance"
 			);

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

@@ -31,6 +31,7 @@ use alloy_core::sol_types::{SolCall, SolInterface};
 use frame_support::{assert_err, assert_ok, traits::fungible::Mutate};
 use pallet_revive_fixtures::{compile_module_with_type, Fibonacci, FixtureType};
 use pretty_assertions::assert_eq;
+use test_case::test_case;
 
 use revm::bytecode::opcode::*;
 
@@ -240,3 +241,27 @@ fn upload_evm_runtime_code_works() {
 		assert_eq!(55u64, decoded, "Contract should correctly compute fibonacci(10)");
 	});
 }
+
+#[test_case(FixtureType::Solc)]
+#[test_case(FixtureType::Resolc)]
+fn dust_work_with_child_calls(fixture_type: FixtureType) {
+	use pallet_revive_fixtures::CallSelfWithDust;
+	let (code, _) = compile_module_with_type("CallSelfWithDust", fixture_type).unwrap();
+
+	ExtBuilder::default().build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
+		let Contract { addr, .. } =
+			builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();
+
+		let value = 1_000_000_000.into();
+		builder::bare_call(addr)
+			.data(
+				CallSelfWithDust::CallSelfWithDustCalls::call(CallSelfWithDust::callCall {})
+					.abi_encode(),
+			)
+			.evm_value(value)
+			.build_and_unwrap_result();
+
+		assert_eq!(crate::Pallet::<Test>::evm_balance(&addr), value);
+	});
+}