[pallet-revive] Move to_account_id host function to System pre-compile (#9455)

Part of closing #8572.

cc @athei @pgherveou

---------

Co-authored-by: xermicus <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
(cherry picked from commit 8744f5e)

Author: 3 people

prdoc/pr_9455.prdoc

@@ -0,0 +1,12 @@
+title: '[pallet-revive] Migrate `to_account_id` from host function to pre-compile'
+doc:
+- audience: Runtime Dev
+  description: Moves the unstable host function `to_account_id` to the 
+    `System` pre-compile.
+crates:
+- name: pallet-revive
+  bump: patch
+- name: pallet-revive-fixtures
+  bump: patch
+- name: pallet-revive-uapi
+  bump: major

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

@@ -23,11 +23,14 @@ use crate::{
 	evm::runtime::GAS_PRICE,
 	exec::{Key, MomentOf, PrecompileExt},
 	limits,
-	precompiles::{self, run::builtin as run_builtin_precompile},
+	precompiles::{
+		self, run::builtin as run_builtin_precompile, BenchmarkSystem, BuiltinPrecompile, ISystem,
+	},
 	storage::WriteOutcome,
 	Pallet as Contracts, *,
 };
 use alloc::{vec, vec::Vec};
+use alloy_core::sol_types::SolInterface;
 use codec::{Encode, MaxEncodedLen};
 use frame_benchmarking::v2::*;
 use frame_support::{
@@ -548,35 +551,40 @@ mod benchmarks {
 	}
 
 	#[benchmark(pov_mode = Measured)]
-	fn seal_to_account_id() {
+	fn to_account_id() {
 		// use a mapped address for the benchmark, to ensure that we bench the worst
 		// case (and not the fallback case).
+		let account_id = account("precompile_to_account_id", 0, 0);
 		let address = {
-			let caller = account("seal_to_account_id", 0, 0);
-			T::Currency::set_balance(&caller, caller_funding::<T>());
-			T::AddressMapper::map(&caller).unwrap();
-			T::AddressMapper::to_address(&caller)
+			T::Currency::set_balance(&account_id, caller_funding::<T>());
+			T::AddressMapper::map(&account_id).unwrap();
+			T::AddressMapper::to_address(&account_id)
 		};
 
-		let len = <T::AccountId as MaxEncodedLen>::max_encoded_len();
-		build_runtime!(runtime, memory: [vec![0u8; len], address.0, ]);
+		let input_bytes = ISystem::ISystemCalls::toAccountId(ISystem::toAccountIdCall {
+			input: address.0.into(),
+		})
+		.abi_encode();
+
+		let mut call_setup = CallSetup::<T>::default();
+		let (mut ext, _) = call_setup.ext();
 
 		let result;
 		#[block]
 		{
-			result = runtime.bench_to_account_id(memory.as_mut_slice(), len as u32, 0);
+			result = run_builtin_precompile(
+				&mut ext,
+				H160(BenchmarkSystem::<T>::MATCHER.base_address()).as_fixed_bytes(),
+				input_bytes,
+			);
 		}
-
-		assert_ok!(result);
+		let data = result.unwrap().data;
 		assert_ne!(
-			memory.as_slice()[20..32],
+			data.as_slice()[20..32],
 			[0xEE; 12],
 			"fallback suffix found where none should be"
 		);
-		assert_eq!(
-			T::AccountId::decode(&mut memory.as_slice()),
-			Ok(runtime.ext().to_account_id(&address))
-		);
+		assert_eq!(T::AccountId::decode(&mut data.as_slice()), Ok(account_id),);
 	}
 
 	#[benchmark(pov_mode = Measured)]
@@ -1969,9 +1977,6 @@ mod benchmarks {
 	// `n`: Input to hash in bytes
 	#[benchmark(pov_mode = Measured)]
 	fn hash_blake2_256(n: Linear<0, { limits::code::BLOB_BYTES }>) {
-		use crate::precompiles::{BenchmarkSystem, BuiltinPrecompile, ISystem};
-		use alloy_core::sol_types::SolInterface;
-
 		let input = vec![0u8; n as usize];
 		let input_bytes = ISystem::ISystemCalls::hashBlake256(ISystem::hashBlake256Call {
 			input: input.clone().into(),

substrate/frame/revive/src/benchmarking.rs

@@ -324,9 +324,6 @@ pub trait PrecompileExt: sealing::Sealed {
 	/// Return the origin of the whole call stack.
 	fn origin(&self) -> &Origin<Self::T>;
 
-	/// Returns the account id for the given `address`.
-	fn to_account_id(&self, address: &H160) -> AccountIdOf<Self::T>;
-
 	/// Returns the code hash of the contract for the given `address`.
 	/// If not a contract but account exists then `keccak_256([])` is returned, otherwise `zero`.
 	fn code_hash(&self, address: &H160) -> H256;
@@ -1939,10 +1936,6 @@ where
 		&self.origin
 	}
 
-	fn to_account_id(&self, address: &H160) -> T::AccountId {
-		T::AddressMapper::to_account_id(address)
-	}
-
 	fn code_hash(&self, address: &H160) -> H256 {
 		if let Some(code) = <AllPrecompiles<T>>::code(address.as_fixed_bytes()) {
 			return sp_io::hashing::keccak_256(code).into()

substrate/frame/revive/src/exec.rs

@@ -793,40 +793,6 @@ fn origin_returns_proper_values() {
 	assert_eq!(WitnessedCallerCharlie::get(), Some(ALICE_ADDR));
 }
 
-#[test]
-fn to_account_id_returns_proper_values() {
-	let bob_code_hash = MockLoader::insert(Call, |ctx, _| {
-		let alice_account_id = <Test as Config>::AddressMapper::to_account_id(&ALICE_ADDR);
-		assert_eq!(ctx.ext.to_account_id(&ALICE_ADDR), alice_account_id);
-
-		const UNMAPPED_ADDR: H160 = H160([99u8; 20]);
-		let mut unmapped_fallback_account_id = [0xEE; 32];
-		unmapped_fallback_account_id[..20].copy_from_slice(UNMAPPED_ADDR.as_bytes());
-		assert_eq!(
-			ctx.ext.to_account_id(&UNMAPPED_ADDR),
-			AccountId32::new(unmapped_fallback_account_id)
-		);
-
-		exec_success()
-	});
-
-	ExtBuilder::default().build().execute_with(|| {
-		place_contract(&BOB, bob_code_hash);
-		let origin = Origin::from_account_id(ALICE);
-		let mut storage_meter = storage::meter::Meter::new(0);
-		let result = MockStack::run_call(
-			origin,
-			BOB_ADDR,
-			&mut GasMeter::<Test>::new(GAS_LIMIT),
-			&mut storage_meter,
-			U256::zero(),
-			vec![0],
-			false,
-		);
-		assert_matches!(result, Ok(_));
-	});
-}
-
 #[test]
 fn code_hash_returns_proper_values() {
 	let bob_code_hash = MockLoader::insert(Call, |ctx, _| {

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

@@ -31,6 +31,16 @@ sol! {
 	interface ISystem {
 		/// Computes the BLAKE2 256-bit hash on the given input.
 		function hashBlake256(bytes memory input) external pure returns (bytes32 digest);
+		/// Retrieve the account id for a specified `H160` address.
+		///
+		/// Calling this function on a native `H160` chain (`type AccountId = H160`)
+		/// does not make sense, as it would just return the `address` that it was
+		/// called with.
+		///
+		/// # Note
+		///
+		/// If no mapping exists for `addr`, the fallback account id will be returned.
+		function toAccountId(address input) external view returns (bytes memory account_id);
 	}
 }
 
@@ -53,17 +63,95 @@ impl<T: Config> BuiltinPrecompile for System<T> {
 				let output = sp_io::hashing::blake2_256(input.as_bytes_ref());
 				Ok(output.to_vec())
 			},
+			ISystemCalls::toAccountId(ISystem::toAccountIdCall { input }) => {
+				use crate::address::AddressMapper;
+				use codec::Encode;
+				env.gas_meter_mut().charge(RuntimeCosts::ToAccountId)?;
+				let account_id =
+					T::AddressMapper::to_account_id(&crate::H160::from_slice(input.as_slice()));
+				Ok(account_id.encode())
+			},
 		}
 	}
 }
 
 #[cfg(test)]
 mod tests {
-	use super::*;
-	use crate::{precompiles::tests::run_test_vectors, tests::Test};
+	use super::{ISystem, *};
+	use crate::{
+		address::AddressMapper,
+		call_builder::{caller_funding, CallSetup},
+		pallet,
+		precompiles::{tests::run_test_vectors, BuiltinPrecompile},
+		tests::{ExtBuilder, Test},
+		H160,
+	};
+	use codec::Decode;
+	use frame_support::traits::fungible::Mutate;
 
 	#[test]
 	fn test_system_precompile() {
 		run_test_vectors::<System<Test>>(include_str!("testdata/900-blake2_256.json"));
+		run_test_vectors::<System<Test>>(include_str!("testdata/900-to_account_id.json"));
+	}
+
+	#[test]
+	fn test_system_precompile_unmapped_account() {
+		ExtBuilder::default().build().execute_with(|| {
+			// given
+			let mut call_setup = CallSetup::<Test>::default();
+			let (mut ext, _) = call_setup.ext();
+			let unmapped_address = H160::zero();
+
+			// when
+			let input = ISystem::ISystemCalls::toAccountId(ISystem::toAccountIdCall {
+				input: unmapped_address.0.into(),
+			});
+			let expected_fallback_account_id =
+				<System<Test>>::call(&<System<Test>>::MATCHER.base_address(), &input, &mut ext)
+					.unwrap();
+
+			// then
+			assert_eq!(
+				expected_fallback_account_id[20..32],
+				[0xEE; 12],
+				"no fallback suffix found where one should be"
+			);
+		})
+	}
+
+	#[test]
+	fn test_system_precompile_mapped_account() {
+		use crate::test_utils::EVE;
+		ExtBuilder::default().build().execute_with(|| {
+			// given
+			let mapped_address = {
+				<Test as pallet::Config>::Currency::set_balance(&EVE, caller_funding::<Test>());
+				let _ = <Test as pallet::Config>::AddressMapper::map(&EVE);
+				<Test as pallet::Config>::AddressMapper::to_address(&EVE)
+			};
+
+			let mut call_setup = CallSetup::<Test>::default();
+			let (mut ext, _) = call_setup.ext();
+
+			// when
+			let input = ISystem::ISystemCalls::toAccountId(ISystem::toAccountIdCall {
+				input: mapped_address.0.into(),
+			});
+			let data =
+				<System<Test>>::call(&<System<Test>>::MATCHER.base_address(), &input, &mut ext)
+					.unwrap();
+
+			// then
+			assert_ne!(
+				data.as_slice()[20..32],
+				[0xEE; 12],
+				"fallback suffix found where none should be"
+			);
+			assert_eq!(
+				<Test as frame_system::Config>::AccountId::decode(&mut data.as_slice()),
+				Ok(EVE),
+			);
+		})
 	}
 }

substrate/frame/revive/src/precompiles/builtin/system.rs

@@ -0,0 +1,9 @@
+[
+  {
+    "Input": "cf5231cc00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000",
+    "Expected": "0000000000000000000000000000000000000020eeeeeeeeeeeeeeeeeeeeeeee",
+    "Name": "vector 1",
+    "Gas": 8000000,
+    "NoBenchmark": false
+  }
+]

substrate/frame/revive/src/precompiles/builtin/testdata/900-to_account_id.json

@@ -3913,43 +3913,6 @@ fn origin_api_works() {
 	});
 }
 
-#[test]
-fn to_account_id_works() {
-	let (code_hash_code, _) = compile_module("to_account_id").unwrap();
-
-	ExtBuilder::default().existential_deposit(1).build().execute_with(|| {
-		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
-		let _ = <Test as Config>::Currency::set_balance(&EVE, 1_000_000);
-
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code_hash_code)).build_and_unwrap_contract();
-
-		// mapped account
-		<Pallet<Test>>::map_account(RuntimeOrigin::signed(EVE)).unwrap();
-		let expected_mapped_account_id = &<Test as Config>::AddressMapper::to_account_id(&EVE_ADDR);
-		assert_ne!(
-			expected_mapped_account_id.encode()[20..32],
-			[0xEE; 12],
-			"fallback suffix found where none should be"
-		);
-		assert_ok!(builder::call(addr)
-			.data((EVE_ADDR, expected_mapped_account_id).encode())
-			.build());
-
-		// fallback for unmapped accounts
-		let expected_fallback_account_id =
-			&<Test as Config>::AddressMapper::to_account_id(&BOB_ADDR);
-		assert_eq!(
-			expected_fallback_account_id.encode()[20..32],
-			[0xEE; 12],
-			"no fallback suffix found where one should be"
-		);
-		assert_ok!(builder::call(addr)
-			.data((BOB_ADDR, expected_fallback_account_id).encode())
-			.build());
-	});
-}
-
 #[test]
 fn code_hash_works() {
 	use crate::precompiles::{Precompile, EVM_REVERT};

substrate/frame/revive/src/tests.rs

@@ -2123,25 +2123,4 @@ pub mod env {
 			already_charged,
 		)?)
 	}
-
-	/// Retrieves the account id for a specified contract address.
-	///
-	/// See [`pallet_revive_uapi::HostFn::to_account_id`].
-	fn to_account_id(
-		&mut self,
-		memory: &mut M,
-		addr_ptr: u32,
-		out_ptr: u32,
-	) -> Result<(), TrapReason> {
-		self.charge_gas(RuntimeCosts::ToAccountId)?;
-		let address = memory.read_h160(addr_ptr)?;
-		let account_id = self.ext.to_account_id(&address);
-		Ok(self.write_fixed_sandbox_output(
-			memory,
-			out_ptr,
-			&account_id.encode(),
-			false,
-			already_charged,
-		)?)
-	}
 }