[pallet-revive] eth-rpc fixes (#6453)

- Breaking down the integration-test into multiple tests
- Fix tx hash to use expected keccak-256
- Add option to ethers.js example to connect to westend and use a
private key

---------

Co-authored-by: GitHub Action <action@github.com>

.config/nextest.toml

@@ -124,3 +124,10 @@ serial-integration = { max-threads = 1 }
 [[profile.default.overrides]]
 filter = 'test(/(^ui$|_ui|ui_)/)'
 test-group = 'serial-integration'
+
+# Running eth-rpc tests sequentially
+# These tests rely on a shared resource (the RPC and Node) 
+# and would cause race conditions due to transaction nonces if run in parallel.
+[[profile.default.overrides]]
+filter = 'package(pallet-revive-eth-rpc) and test(/^tests::/)'
+test-group = 'serial-integration'

prdoc/pr_6453.prdoc

@@ -0,0 +1,7 @@
+title: '[pallet-revive] breakdown integration tests'
+doc:
+- audience: Runtime Dev
+  description: Break down the single integration tests into multiple tests, use keccak-256 for tx.hash
+crates:
+- name: pallet-revive-eth-rpc
+  bump: minor

substrate/frame/revive/rpc/Cargo.toml

@@ -74,6 +74,7 @@ ethabi = { version = "18.0.0" }
 example = ["hex-literal", "rlp", "secp256k1", "subxt-signer"]
 
 [dev-dependencies]
+static_init = { workspace = true }
 hex-literal = { workspace = true }
 pallet-revive-fixtures = { workspace = true }
 substrate-cli-test-utils = { workspace = true }

substrate/frame/revive/rpc/examples/js/src/lib.ts

@@ -4,15 +4,34 @@ import {
 	JsonRpcProvider,
 	TransactionReceipt,
 	TransactionResponse,
+	Wallet,
 } from 'ethers'
 import { readFileSync } from 'node:fs'
 import type { compile } from '@parity/revive'
 import { spawn } from 'node:child_process'
+import { parseArgs } from 'node:util'
 
 type CompileOutput = Awaited<ReturnType<typeof compile>>
 type Abi = CompileOutput['contracts'][string][string]['abi']
 
-const geth = process.argv.includes('--geth')
+const {
+	values: { geth, westend, ['private-key']: privateKey },
+} = parseArgs({
+	args: process.argv.slice(2),
+	options: {
+		['private-key']: {
+			type: 'string',
+			short: 'k',
+		},
+		geth: {
+			type: 'boolean',
+		},
+		westend: {
+			type: 'boolean',
+		},
+	},
+})
+
 if (geth) {
 	console.log('Testing with Geth')
 	const child = spawn(
@@ -31,13 +50,15 @@ if (geth) {
 	)
 
 	process.on('exit', () => child.kill())
-
 	child.unref()
 	await new Promise((resolve) => setTimeout(resolve, 500))
 }
 
-const provider = new JsonRpcProvider('http://localhost:8545')
-const signer = await provider.getSigner()
+const provider = new JsonRpcProvider(
+	westend ? 'https://westend-asset-hub-eth-rpc.polkadot.io' : 'http://localhost:8545'
+)
+
+const signer = privateKey ? new Wallet(privateKey, provider) : await provider.getSigner()
 console.log(`Signer address: ${await signer.getAddress()}, Nonce: ${await signer.getNonce()}`)
 
 /**

substrate/frame/revive/rpc/src/client.rs

@@ -35,6 +35,7 @@ use pallet_revive::{
 	},
 	EthContractResult,
 };
+use sp_core::keccak_256;
 use sp_weights::Weight;
 use std::{
 	collections::{HashMap, VecDeque},
@@ -278,6 +279,7 @@ impl ClientInner {
 		// Filter extrinsics from pallet_revive
 		let extrinsics = extrinsics.iter().flat_map(|ext| {
 			let call = ext.as_extrinsic::<EthTransact>().ok()??;
+			let transaction_hash = H256(keccak_256(&call.payload));
 			let tx = rlp::decode::<TransactionLegacySigned>(&call.payload).ok()?;
 			let from = tx.recover_eth_address().ok()?;
 			let contract_address = if tx.transaction_legacy_unsigned.to.is_none() {
@@ -286,12 +288,12 @@ impl ClientInner {
 				None
 			};
 
-			Some((from, tx, contract_address, ext))
+			Some((from, tx, transaction_hash, contract_address, ext))
 		});
 
 		// Map each extrinsic to a receipt
 		stream::iter(extrinsics)
-			.map(|(from, tx, contract_address, ext)| async move {
+			.map(|(from, tx, transaction_hash, contract_address, ext)| async move {
 				let events = ext.events().await?;
 				let tx_fees =
 					events.find_first::<TransactionFeePaid>()?.ok_or(ClientError::TxFeeNotFound)?;
@@ -305,7 +307,6 @@ impl ClientInner {
 				let transaction_index = ext.index();
 				let block_hash = block.hash();
 				let block_number = block.number().into();
-				let transaction_hash= ext.hash();
 
 				// get logs from ContractEmitted event
 				let logs = events.iter()

substrate/frame/revive/rpc/src/lib.rs

@@ -24,7 +24,7 @@ use jsonrpsee::{
 	types::{ErrorCode, ErrorObjectOwned},
 };
 use pallet_revive::{evm::*, EthContractResult};
-use sp_core::{H160, H256, U256};
+use sp_core::{keccak_256, H160, H256, U256};
 use thiserror::Error;
 
 pub mod cli;
@@ -135,6 +135,8 @@ impl EthRpcServer for EthRpcServerImpl {
 	}
 
 	async fn send_raw_transaction(&self, transaction: Bytes) -> RpcResult<H256> {
+		let hash = H256(keccak_256(&transaction.0));
+
 		let tx = rlp::decode::<TransactionLegacySigned>(&transaction.0).map_err(|err| {
 			log::debug!(target: LOG_TARGET, "Failed to decode transaction: {err:?}");
 			EthRpcError::from(err)
@@ -167,7 +169,7 @@ impl EthRpcServer for EthRpcServerImpl {
 			gas_required.into(),
 			storage_deposit,
 		);
-		let hash = self.client.submit(call).await?;
+		self.client.submit(call).await?;
 		log::debug!(target: LOG_TARGET, "send_raw_transaction hash: {hash:?}");
 		Ok(hash)
 	}

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

@@ -27,6 +27,7 @@ use pallet_revive::{
 	create1,
 	evm::{Account, BlockTag, U256},
 };
+use static_init::dynamic;
 use std::thread;
 use substrate_cli_test_utils::*;
 
@@ -60,6 +61,52 @@ fn get_contract(name: &str) -> anyhow::Result<(Vec<u8>, ethabi::Contract)> {
 	Ok((bytecode, contract))
 }
 
+struct SharedResources {
+	_node_handle: std::thread::JoinHandle<()>,
+	_rpc_handle: std::thread::JoinHandle<()>,
+}
+
+impl SharedResources {
+	fn start() -> Self {
+		// Start the node.
+		let _node_handle = thread::spawn(move || {
+			if let Err(e) = start_node_inline(vec![
+				"--dev",
+				"--rpc-port=45789",
+				"--no-telemetry",
+				"--no-prometheus",
+				"-lerror,evm=debug,sc_rpc_server=info,runtime::revive=trace",
+			]) {
+				panic!("Node exited with error: {e:?}");
+			}
+		});
+
+		// Start the rpc server.
+		let args = CliCommand::parse_from([
+			"--dev",
+			"--rpc-port=45788",
+			"--node-rpc-url=ws://localhost:45789",
+			"--no-prometheus",
+			"-linfo,eth-rpc=debug",
+		]);
+
+		let _rpc_handle = thread::spawn(move || {
+			if let Err(e) = cli::run(args) {
+				panic!("eth-rpc exited with error: {e:?}");
+			}
+		});
+
+		Self { _node_handle, _rpc_handle }
+	}
+
+	async fn client() -> WsClient {
+		ws_client_with_retry("ws://localhost:45788").await
+	}
+}
+
+#[dynamic(lazy)]
+static mut SHARED_RESOURCES: SharedResources = SharedResources::start();
+
 macro_rules! unwrap_call_err(
 	($err:expr) => {
 		match $err.downcast_ref::<jsonrpsee::core::client::Error>().unwrap() {
@@ -70,41 +117,42 @@ macro_rules! unwrap_call_err(
 );
 
 #[tokio::test]
-async fn test_jsonrpsee_server() -> anyhow::Result<()> {
-	// Start the node.
-	let _ = thread::spawn(move || {
-		if let Err(e) = start_node_inline(vec![
-			"--dev",
-			"--rpc-port=45789",
-			"--no-telemetry",
-			"--no-prometheus",
-			"-lerror,evm=debug,sc_rpc_server=info,runtime::revive=trace",
-		]) {
-			panic!("Node exited with error: {e:?}");
-		}
-	});
-
-	// Start the rpc server.
-	let args = CliCommand::parse_from([
-		"--dev",
-		"--rpc-port=45788",
-		"--node-rpc-url=ws://localhost:45789",
-		"--no-prometheus",
-		"-linfo,eth-rpc=debug",
-	]);
-	let _ = thread::spawn(move || {
-		if let Err(e) = cli::run(args) {
-			panic!("eth-rpc exited with error: {e:?}");
-		}
-	});
+async fn transfer() -> anyhow::Result<()> {
+	let _lock = SHARED_RESOURCES.write();
+	let client = SharedResources::client().await;
 
-	let client = ws_client_with_retry("ws://localhost:45788").await;
+	let ethan = Account::from(subxt_signer::eth::dev::ethan());
+	let initial_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?;
+
+	let value = 1_000_000_000_000_000_000_000u128.into();
+	let hash = TransactionBuilder::default()
+		.value(value)
+		.to(ethan.address())
+		.send(&client)
+		.await?;
+
+	let receipt = wait_for_successful_receipt(&client, hash).await?;
+	assert_eq!(
+		Some(ethan.address()),
+		receipt.to,
+		"Receipt should have the correct contract address."
+	);
+
+	let increase =
+		client.get_balance(ethan.address(), BlockTag::Latest.into()).await? - initial_balance;
+	assert_eq!(value, increase);
+	Ok(())
+}
+
+#[tokio::test]
+async fn deploy_and_call() -> anyhow::Result<()> {
+	let _lock = SHARED_RESOURCES.write();
+	let client = SharedResources::client().await;
 	let account = Account::default();
 
 	// Balance transfer
 	let ethan = Account::from(subxt_signer::eth::dev::ethan());
-	let ethan_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?;
-	assert_eq!(U256::zero(), ethan_balance);
+	let initial_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?;
 
 	let value = 1_000_000_000_000_000_000_000u128.into();
 	let hash = TransactionBuilder::default()
@@ -120,8 +168,8 @@ async fn test_jsonrpsee_server() -> anyhow::Result<()> {
 		"Receipt should have the correct contract address."
 	);
 
-	let ethan_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?;
-	assert_eq!(value, ethan_balance, "ethan's balance should be the same as the value sent.");
+	let updated_balance = client.get_balance(ethan.address(), BlockTag::Latest.into()).await?;
+	assert_eq!(value, updated_balance - initial_balance);
 
 	// Deploy contract
 	let data = b"hello world".to_vec();
@@ -169,15 +217,19 @@ async fn test_jsonrpsee_server() -> anyhow::Result<()> {
 	wait_for_successful_receipt(&client, hash).await?;
 	let increase = client.get_balance(contract_address, BlockTag::Latest.into()).await? - balance;
 	assert_eq!(value, increase, "contract's balance should have increased by the value sent.");
+	Ok(())
+}
 
-	// Deploy revert
+#[tokio::test]
+async fn revert_call() -> anyhow::Result<()> {
+	let _lock = SHARED_RESOURCES.write();
+	let client = SharedResources::client().await;
 	let (bytecode, contract) = get_contract("revert")?;
 	let receipt = TransactionBuilder::default()
 		.input(contract.constructor.clone().unwrap().encode_input(bytecode, &[]).unwrap())
 		.send_and_wait_for_receipt(&client)
 		.await?;
 
-	// Call doRevert
 	let err = TransactionBuilder::default()
 		.to(receipt.contract_address.unwrap())
 		.input(contract.function("doRevert")?.encode_input(&[])?.to_vec())
@@ -187,25 +239,36 @@ async fn test_jsonrpsee_server() -> anyhow::Result<()> {
 
 	let call_err = unwrap_call_err!(err.source().unwrap());
 	assert_eq!(call_err.message(), "Execution reverted: revert message");
+	Ok(())
+}
 
-	// Deploy event
+#[tokio::test]
+async fn event_logs() -> anyhow::Result<()> {
+	let _lock = SHARED_RESOURCES.write();
+	let client = SharedResources::client().await;
 	let (bytecode, contract) = get_contract("event")?;
 	let receipt = TransactionBuilder::default()
 		.input(bytecode)
 		.send_and_wait_for_receipt(&client)
 		.await?;
 
-	// Call triggerEvent
 	let receipt = TransactionBuilder::default()
 		.to(receipt.contract_address.unwrap())
 		.input(contract.function("triggerEvent")?.encode_input(&[])?.to_vec())
 		.send_and_wait_for_receipt(&client)
 		.await?;
 	assert_eq!(receipt.logs.len(), 1, "There should be one log.");
+	Ok(())
+}
+
+#[tokio::test]
+async fn invalid_transaction() -> anyhow::Result<()> {
+	let _lock = SHARED_RESOURCES.write();
+	let client = SharedResources::client().await;
+	let ethan = Account::from(subxt_signer::eth::dev::ethan());
 
-	// Invalid transaction
 	let err = TransactionBuilder::default()
-		.value(value)
+		.value(U256::from(1_000_000_000_000u128))
 		.to(ethan.address())
 		.mutate(|tx| tx.chain_id = Some(42u32.into()))
 		.send(&client)