[pallet-revive] rework balance transfers

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -45,7 +45,10 @@ use frame_support::{
 	ord_parameter_types, parameter_types,
 	traits::{
 		fungible, fungibles,
-		tokens::{imbalance::ResolveAssetTo, nonfungibles_v2::Inspect},
+		tokens::{
+			imbalance::ResolveAssetTo, nonfungibles_v2::Inspect, Fortitude::Polite,
+			Preservation::Expendable,
+		},
 		AsEnsureOriginWithArg, ConstBool, ConstU128, ConstU32, ConstU64, ConstU8, InstanceFilter,
 		Nothing, TransformOrigin,
 	},
@@ -2073,8 +2076,9 @@ impl_runtime_apis! {
 	impl pallet_revive::ReviveApi<Block, AccountId, Balance, Nonce, BlockNumber, EventRecord> for Runtime
 	{
 		fn balance(address: H160) -> Balance {
+			use frame_support::traits::fungible::Inspect;
 			let account = <Runtime as pallet_revive::Config>::AddressMapper::to_account_id(&address);
-			Balances::usable_balance(account)
+			Balances::reducible_balance(&account, Expendable, Polite)
 		}
 
 		fn nonce(address: H160) -> Nonce {

prdoc/pr_6187.prdoc

@@ -0,0 +1,16 @@
+title: '[pallet-revive] rework balance transfers'
+doc:
+- audience: Runtime Dev
+  description: |-
+    This PR removes the `transfer` syscall and changes balance transfers to make the existential deposit (ED) fully transparent for contracts.
+
+    The `transfer` API is removed since there is no corresponding EVM opcode and transferring via a call introduces barely any overhead.
+
+    We make the ED transparent to contracts by transferring the ED from the call origin to nonexistent accounts. Without this change, transfers to nonexistant accounts will transfer the supplied value minus the ED from the contracts viewpoint, and consequentially fail if the supplied value lies below the ED. Changing this behavior removes the need for contract code to handle this rather annoying corner case and aligns better with the EVM. The EVM charges a similar deposit from the gas meter, so transferring the ED from the call origin is practically the same as the call origin pays for gas.
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-fixtures
+  bump: patch
+- name: pallet-revive-uapi
+  bump: major

substrate/bin/node/runtime/src/lib.rs

@@ -47,7 +47,7 @@ use frame_support::{
 		},
 		tokens::{
 			imbalance::ResolveAssetTo, nonfungibles_v2::Inspect, pay::PayAssetFromAccount,
-			GetSalary, PayFromAccount,
+			Fortitude::Polite, GetSalary, PayFromAccount, Preservation::Preserve,
 		},
 		AsEnsureOriginWithArg, ConstBool, ConstU128, ConstU16, ConstU32, ConstU64, Contains,
 		Currency, EitherOfDiverse, EnsureOriginWithArg, EqualPrivilegeOnly, Imbalance, InsideBoth,
@@ -3165,8 +3165,9 @@ impl_runtime_apis! {
 	impl pallet_revive::ReviveApi<Block, AccountId, Balance, Nonce, BlockNumber, EventRecord> for Runtime
 	{
 		fn balance(address: H160) -> Balance {
+			use frame_support::traits::fungible::Inspect;
 			let account = <Runtime as pallet_revive::Config>::AddressMapper::to_account_id(&address);
-			Balances::usable_balance(account)
+			Balances::reducible_balance(&account, Preserve, Polite)
 		}
 
 		fn nonce(address: H160) -> Nonce {

substrate/frame/revive/fixtures/contracts/drain.rs

@@ -36,6 +36,15 @@ pub extern "C" fn call() {
 
 	// Try to self-destruct by sending more balance to the 0 address.
 	// The call will fail because a contract transfer has a keep alive requirement.
-	let res = api::transfer(&[0u8; 20], &u256_bytes(balance));
+	let res = api::call(
+		uapi::CallFlags::empty(),
+		&[0u8; 20],
+		0,
+		0,
+		None,
+		&u256_bytes(balance),
+		&[],
+		None,
+	);
 	assert!(matches!(res, Err(uapi::ReturnErrorCode::TransferFailed)));
 }

substrate/frame/revive/fixtures/contracts/return_data_api.rs

@@ -35,11 +35,11 @@ static INPUT_DATA: [u8; INPUT_BUF_SIZE] = [0xFF; INPUT_BUF_SIZE];
 const OUTPUT_BUF_SIZE: usize = INPUT_BUF_SIZE - 4;
 static OUTPUT_DATA: [u8; OUTPUT_BUF_SIZE] = [0xEE; OUTPUT_BUF_SIZE];
 
+/// Assert correct return data after calls and finally reset the return data.
 fn assert_return_data_after_call(input: &[u8]) {
 	assert_return_data_size_of(OUTPUT_BUF_SIZE as u64);
-	assert_plain_transfer_does_not_reset(OUTPUT_BUF_SIZE as u64);
 	assert_return_data_copy(&input[4..]);
-	reset_return_data();
+	assert_balance_transfer_does_reset();
 }
 
 /// Assert that what we get from [api::return_data_copy] matches `whole_return_data`,
@@ -73,34 +73,18 @@ fn recursion_guard() -> [u8; 20] {
 	own_address
 }
 
-/// Call ourselves recursively, which panics the callee and thus resets the return data.
-fn reset_return_data() {
-	api::call(
-		uapi::CallFlags::ALLOW_REENTRY,
-		&recursion_guard(),
-		0u64,
-		0u64,
-		None,
-		&[0u8; 32],
-		&[0u8; 32],
-		None,
-	)
-	.unwrap_err();
-	assert_return_data_size_of(0);
-}
-
 /// Assert [api::return_data_size] to match the `expected` value.
 fn assert_return_data_size_of(expected: u64) {
 	let mut return_data_size = [0xff; 32];
 	api::return_data_size(&mut return_data_size);
 	assert_eq!(return_data_size, u256_bytes(expected));
 }
 
-/// Assert [api::return_data_size] to match the `expected` value after a plain transfer
-/// (plain transfers don't issue a call and so should not reset the return data)
-fn assert_plain_transfer_does_not_reset(expected: u64) {
-	api::transfer(&[0; 20], &u256_bytes(128)).unwrap();
-	assert_return_data_size_of(expected);
+/// Assert the return data to be reset after a balance transfer.
+fn assert_balance_transfer_does_reset() {
+	api::call(uapi::CallFlags::empty(), &[0u8; 20], 0, 0, None, &u256_bytes(128), &[], None)
+		.unwrap();
+	assert_return_data_size_of(0);
 }
 
 #[no_mangle]

substrate/frame/revive/fixtures/contracts/transfer_return_code.rs

@@ -28,7 +28,16 @@ pub extern "C" fn deploy() {}
 #[no_mangle]
 #[polkavm_derive::polkavm_export]
 pub extern "C" fn call() {
-	let ret_code = match api::transfer(&[0u8; 20], &u256_bytes(100u64)) {
+	let ret_code = match api::call(
+		uapi::CallFlags::empty(),
+		&[0u8; 20],
+		0,
+		0,
+		None,
+		&u256_bytes(100u64),
+		&[],
+		None,
+	) {
 		Ok(_) => 0u32,
 		Err(code) => code as u32,
 	};

substrate/frame/revive/src/benchmarking/mod.rs

@@ -169,7 +169,7 @@ where
 				};
 
 				if key == &key_new {
-					continue
+					continue;
 				}
 				child::put_raw(&child_trie_info, &key_new, &value);
 			}
@@ -1507,36 +1507,6 @@ mod benchmarks {
 		Ok(())
 	}
 
-	// We transfer to unique accounts.
-	#[benchmark(pov_mode = Measured)]
-	fn seal_transfer() {
-		let account = account::<T::AccountId>("receiver", 0, 0);
-		let value = Pallet::<T>::min_balance();
-		assert!(value > 0u32.into());
-
-		let mut setup = CallSetup::<T>::default();
-		setup.set_balance(value);
-		let (mut ext, _) = setup.ext();
-		let mut runtime = crate::wasm::Runtime::<_, [u8]>::new(&mut ext, vec![]);
-
-		let account_bytes = account.encode();
-		let account_len = account_bytes.len() as u32;
-		let value_bytes = Into::<U256>::into(value).encode();
-		let mut memory = memory!(account_bytes, value_bytes,);
-
-		let result;
-		#[block]
-		{
-			result = runtime.bench_transfer(
-				memory.as_mut_slice(),
-				0,           // account_ptr
-				account_len, // value_ptr
-			);
-		}
-
-		assert_ok!(result);
-	}
-
 	// t: with or without some value to transfer
 	// i: size of the input data
 	#[benchmark(pov_mode = Measured)]