From 6173a3ce9bd86f2d13aceeba2513252afeed3556 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Mon, 19 Oct 2020 12:31:57 +0200 Subject: [PATCH] Fix eth_estimateGas and eth_call to match with Ethereum impl (#168) * move estimate_gas method into frame_ethereum and handle exit_reason * replace estimate_gas with generic ethereum call, so both and can use it * cleanup * revert some changes * refactor * add test case --- Cargo.lock | 1 + frame/ethereum/src/lib.rs | 227 ++++++++++++++++++++++-------------- frame/ethereum/src/tests.rs | 63 +++++++++- rpc/Cargo.toml | 1 + rpc/primitives/src/lib.rs | 6 +- rpc/src/eth.rs | 68 +++++++---- rpc/src/lib.rs | 14 ++- template/runtime/src/lib.rs | 32 +---- 8 files changed, 263 insertions(+), 149 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3fc40ef18..7d9d12dc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1465,6 +1465,7 @@ dependencies = [ "pallet-ethereum 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec", "rlp", + "rustc-hex", "sc-client-api", "sc-network", "sc-rpc", diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index e937b074d..a35c9550b 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -24,7 +24,7 @@ use frame_support::{ decl_module, decl_storage, decl_error, decl_event, - traits::Get, traits::FindAuthor + traits::Get, traits::FindAuthor, }; use sp_std::prelude::*; use frame_system::ensure_none; @@ -49,6 +49,12 @@ mod tests; #[cfg(all(feature = "std", test))] mod mock; +#[derive(Eq, PartialEq, Clone, sp_runtime::RuntimeDebug)] +pub enum ReturnValue { + Bytes(Vec), + Hash(H160), +} + /// A type alias for the balance type from this pallet's point of view. pub type BalanceOf = ::Balance; @@ -172,7 +178,7 @@ decl_module! { #[repr(u8)] enum TransactionValidationError { - #[allow (dead_code)] + #[allow(dead_code)] UnknownError, InvalidChainId, InvalidSignature, @@ -219,7 +225,6 @@ impl frame_support::unsigned::ValidateUnsigned for Module { } impl Module { - fn recover_signer(transaction: ðereum::Transaction) -> Option { let mut sig = [0u8; 65]; let mut msg = [0u8; 32]; @@ -321,78 +326,52 @@ impl Module { } /// Execute an Ethereum transaction, ignoring transaction signatures. - pub fn execute(source: H160, transaction: ethereum::Transaction) -> DispatchResult { + pub fn execute(from: H160, transaction: ethereum::Transaction) -> DispatchResult { let transaction_hash = H256::from_slice( Keccak256::digest(&rlp::encode(&transaction)).as_slice() ); let transaction_index = Pending::get().len() as u32; - let (status, gas_used) = match transaction.action { - ethereum::TransactionAction::Call(target) => { - let (_, _, gas_used, logs) = Self::handle_exec( - pallet_evm::Module::::execute_call( - source, - target, - transaction.input.clone(), - transaction.value, - transaction.gas_limit.low_u32(), - transaction.gas_price, - Some(transaction.nonce), - true, - )? - )?; - - (TransactionStatus { - transaction_hash, - transaction_index, - from: source, - to: Some(target), - contract_address: None, - logs: { - logs.into_iter() - .map(|log| { - Log { - address: log.address, - topics: log.topics, - data: log.data - } - }).collect() - }, - logs_bloom: Bloom::default(), // TODO: feed in bloom. - }, gas_used) - }, - ethereum::TransactionAction::Create => { - let (_, contract_address, gas_used, logs) = Self::handle_exec( - pallet_evm::Module::::execute_create( - source, - transaction.input.clone(), - transaction.value, - transaction.gas_limit.low_u32(), - transaction.gas_price, - Some(transaction.nonce), - true, - )? - )?; - - (TransactionStatus { - transaction_hash, - transaction_index, - from: source, - to: None, - contract_address: Some(contract_address), - logs: { - logs.into_iter() - .map(|log| { - Log { - address: log.address, - topics: log.topics, - data: log.data - } - }).collect() - }, - logs_bloom: Bloom::default(), // TODO: feed in bloom. - }, gas_used) + let (exit_reason, returned_value, gas_used, logs) = Self::execute_evm( + from, + transaction.input.clone(), + transaction.value, + transaction.gas_limit.low_u32(), + transaction.gas_price, + Some(transaction.nonce), + transaction.action, + true, + )?; + + Self::handle_exit_reason(exit_reason)?; + + let to = match transaction.action { + TransactionAction::Create => None, + TransactionAction::Call(to) => Some(to) + }; + + let contract_address = match returned_value { + ReturnValue::Bytes(_) => None, + ReturnValue::Hash(addr) => Some(addr) + }; + + let status = TransactionStatus { + transaction_hash, + transaction_index, + from, + to, + contract_address, + logs: { + logs.into_iter() + .map(|log| { + Log { + address: log.address, + topics: log.topics, + data: log.data, + } + }).collect() }, + logs_bloom: Bloom::default(), // TODO: feed in bloom. }; let receipt = ethereum::Receipt { @@ -407,16 +386,86 @@ impl Module { Ok(()) } - fn handle_exec(res: (ExitReason, R, U256, Vec)) - -> Result<(ExitReason, R, U256, Vec), Error> { - match res.0 { - ExitReason::Succeed(_s) => Ok(res), + /// Execute an EVM call or create + pub fn call( + source: H160, + data: Vec, + value: U256, + gas_limit: u32, + gas_price: U256, + nonce: Option, + action: TransactionAction, + ) -> Result<(Vec, U256), (sp_runtime::DispatchError, Vec)> { + let (exit_reason, returned_value, gas_used, _) = Self::execute_evm( + source, + data, + value, + gas_limit, + gas_price, + nonce, + action, + false, + ).map_err(|err| (err.into(), Vec::new()))?; + + let returned_value = match returned_value.clone() { + ReturnValue::Bytes(data) => data, + ReturnValue::Hash(_) => Vec::new() + }; + + Self::handle_exit_reason(exit_reason) + .map_err(|err| (err.into(), returned_value.clone()))?; + + Ok((returned_value, gas_used)) + } +} + +impl Module { + fn execute_evm( + source: H160, + data: Vec, + value: U256, + gas_limit: u32, + gas_price: U256, + nonce: Option, + action: TransactionAction, + apply_state: bool, + ) -> Result<(ExitReason, ReturnValue, U256, Vec), pallet_evm::Error> { + match action { + TransactionAction::Call(target) => pallet_evm::Module::::execute_call( + source, + target, + data, + value, + gas_limit, + gas_price, + nonce, + apply_state, + ).map(|(exit_reason, returned_value, gas_used, logs)| { + (exit_reason, ReturnValue::Bytes(returned_value), gas_used, logs) + }), + TransactionAction::Create => pallet_evm::Module::::execute_create( + source, + data, + value, + gas_limit, + gas_price, + nonce, + apply_state, + ).map(|(exit_reason, returned_value, gas_used, logs)| { + (exit_reason, ReturnValue::Hash(returned_value), gas_used, logs) + }) + } + } + + fn handle_exit_reason(exit_reason: ExitReason) -> Result<(), Error> { + match exit_reason { + ExitReason::Succeed(_s) => Ok(()), ExitReason::Error(e) => Err(Self::parse_exit_error(e)), ExitReason::Revert(e) => { match e { ExitRevert::Reverted => Err(Error::::Reverted), } - }, + } ExitReason::Fatal(e) => { match e { ExitFatal::NotSupported => Err(Error::::NotSupported), @@ -424,26 +473,26 @@ impl Module { ExitFatal::CallErrorAsFatal(e_error) => Err(Self::parse_exit_error(e_error)), ExitFatal::Other(_s) => Err(Error::::ExitFatalOther), } - }, + } } } fn parse_exit_error(exit_error: ExitError) -> Error { match exit_error { - ExitError::StackUnderflow => return Error::::StackUnderflow, - ExitError::StackOverflow => return Error::::StackOverflow, - ExitError::InvalidJump => return Error::::InvalidJump, - ExitError::InvalidRange => return Error::::InvalidRange, - ExitError::DesignatedInvalid => return Error::::DesignatedInvalid, - ExitError::CallTooDeep => return Error::::CallTooDeep, - ExitError::CreateCollision => return Error::::CreateCollision, - ExitError::CreateContractLimit => return Error::::CreateContractLimit, - ExitError::OutOfOffset => return Error::::OutOfOffset, - ExitError::OutOfGas => return Error::::OutOfGas, - ExitError::OutOfFund => return Error::::OutOfFund, - ExitError::PCUnderflow => return Error::::PCUnderflow, - ExitError::CreateEmpty => return Error::::CreateEmpty, - ExitError::Other(_s) => return Error::::ExitErrorOther, + ExitError::StackUnderflow => Error::::StackUnderflow, + ExitError::StackOverflow => Error::::StackOverflow, + ExitError::InvalidJump => Error::::InvalidJump, + ExitError::InvalidRange => Error::::InvalidRange, + ExitError::DesignatedInvalid => Error::::DesignatedInvalid, + ExitError::CallTooDeep => Error::::CallTooDeep, + ExitError::CreateCollision => Error::::CreateCollision, + ExitError::CreateContractLimit => Error::::CreateContractLimit, + ExitError::OutOfOffset => Error::::OutOfOffset, + ExitError::OutOfGas => Error::::OutOfGas, + ExitError::OutOfFund => Error::::OutOfFund, + ExitError::PCUnderflow => Error::::PCUnderflow, + ExitError::CreateEmpty => Error::::CreateEmpty, + ExitError::Other(_s) => Error::::ExitErrorOther, } } } diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index 8a0a7ce31..f960e3af2 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -19,7 +19,7 @@ use super::*; use mock::*; -use rustc_hex::FromHex; +use rustc_hex::{FromHex, ToHex}; use std::str::FromStr; use ethereum::TransactionSignature; use frame_support::{ @@ -146,7 +146,6 @@ fn contract_constructor_should_get_executed() { assert_eq!(Evm::account_storages( erc20_address, alice_storage_address ), H256::from_str("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()) - }); } @@ -218,3 +217,63 @@ fn transaction_should_generate_correct_gas_used() { assert_eq!(Pending::get()[0].2.used_gas, expected_gas); }); } + +#[test] +fn call_should_handle_errors() { + // pragma solidity ^0.6.6; + // contract Test { + // function foo() external pure returns (bool) { + // return true; + // } + // function bar() external pure { + // require(false, "error_msg"); + // } + // } + let contract: &str = "608060405234801561001057600080fd5b50610113806100206000396000f3fe6080604052348015600f57600080fd5b506004361060325760003560e01c8063c2985578146037578063febb0f7e146057575b600080fd5b603d605f565b604051808215151515815260200191505060405180910390f35b605d6068565b005b60006001905090565b600060db576040517f08c379a00000000000000000000000000000000000000000000000000000000081526004018080602001828103825260098152602001807f6572726f725f6d7367000000000000000000000000000000000000000000000081525060200191505060405180910390fd5b56fea2646970667358221220fde68a3968e0e99b16fabf9b2997a78218b32214031f8e07e2c502daf603a69e64736f6c63430006060033"; + + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + assert_ok!(Ethereum::execute( + alice.address, + UnsignedTransaction { + nonce: U256::zero(), + gas_price: U256::from(1), + gas_limit: U256::from(0x100000), + action: ethereum::TransactionAction::Create, + value: U256::zero(), + input: FromHex::from_hex(contract).unwrap(), + }.sign(&alice.private_key), + )); + + let contract_address: Vec = FromHex::from_hex("32dcab0ef3fb2de2fce1d2e0799d36239671f04a").unwrap(); + let foo: Vec = FromHex::from_hex("c2985578").unwrap(); + let bar: Vec = FromHex::from_hex("febb0f7e").unwrap(); + + // calling foo will succeed + let (returned, _) = Ethereum::call( + alice.address, + foo, + U256::zero(), + 1048576, + U256::from(1), + None, + TransactionAction::Call(H160::from_slice(&contract_address)), + ).unwrap(); + assert_eq!(returned.to_hex::(), "0000000000000000000000000000000000000000000000000000000000000001".to_owned()); + + // calling bar will revert + let (error, returned) = Ethereum::call( + alice.address, + bar, + U256::zero(), + 1048576, + U256::from(1), + None, + TransactionAction::Call(H160::from_slice(&contract_address)) + ).err().unwrap(); + assert_eq!(error, Error::::Reverted.into()); + assert_eq!(returned.to_hex::(), "08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000096572726f725f6d73670000000000000000000000000000000000000000000000".to_owned()); + }); +} diff --git a/rpc/Cargo.toml b/rpc/Cargo.toml index 56e4a7406..0e57cd482 100644 --- a/rpc/Cargo.toml +++ b/rpc/Cargo.toml @@ -32,3 +32,4 @@ rlp = "0.4" pallet-ethereum = "0.1" futures = { version = "0.3.1", features = ["compat"] } sha3 = "0.8" +rustc-hex = { version = "2.1.0", default-features = false } diff --git a/rpc/primitives/src/lib.rs b/rpc/primitives/src/lib.rs index 59167598d..7fa51059d 100644 --- a/rpc/primitives/src/lib.rs +++ b/rpc/primitives/src/lib.rs @@ -18,7 +18,7 @@ use sp_core::{H160, H256, U256}; use ethereum::{ - Log, Block as EthereumBlock, TransactionAction + Log, Block as EthereumBlock, TransactionAction, }; use ethereum_types::Bloom; use codec::{Encode, Decode}; @@ -64,7 +64,7 @@ sp_api::decl_runtime_apis! { fn author() -> H160; /// For a given account address and index, returns pallet_evm::AccountStorages. fn storage_at(address: H160, index: U256) -> H256; - /// Returns a pallet_evm::execute_call response. + /// Returns a frame_ethereum::call response. fn call( from: H160, data: Vec, @@ -73,7 +73,7 @@ sp_api::decl_runtime_apis! { gas_price: Option, nonce: Option, action: TransactionAction - ) -> Result<(Vec, U256), sp_runtime::DispatchError>; + ) -> Result<(Vec, U256), (sp_runtime::DispatchError, Vec)>; /// Return the current block. fn current_block() -> Option; /// Return the current receipt. diff --git a/rpc/src/eth.rs b/rpc/src/eth.rs index a4d4da157..2df738981 100644 --- a/rpc/src/eth.rs +++ b/rpc/src/eth.rs @@ -35,7 +35,7 @@ use frontier_rpc_core::types::{ SyncStatus, SyncInfo, Transaction, Work, Rich, Block, BlockTransactions, VariadicValue }; use frontier_rpc_primitives::{EthereumRuntimeRPCApi, ConvertTransaction, TransactionStatus}; -use crate::internal_err; +use crate::{internal_err, handle_call_error}; pub use frontier_rpc_core::{EthApiServer, NetApiServer}; @@ -489,27 +489,37 @@ impl EthApiT for EthApi where fn call(&self, request: CallRequest, _: Option) -> Result { let hash = self.client.info().best_hash; - let from = request.from.unwrap_or_default(); - let to = request.to.unwrap_or_default(); - let gas_price = request.gas_price; - let gas_limit = request.gas.unwrap_or(U256::max_value()); - let value = request.value.unwrap_or_default(); - let data = request.data.map(|d| d.0).unwrap_or_default(); - let nonce = request.nonce; + let CallRequest { + from, + to, + gas_price, + gas, + value, + data, + nonce + } = request; + + let gas_limit = gas.unwrap_or(U256::max_value()); // TODO: set a limit + let data = data.map(|d| d.0).unwrap_or_default(); + + let action = match to { + Some(to) => ethereum::TransactionAction::Call(to), + _ => ethereum::TransactionAction::Create, + }; let (ret, _) = self.client.runtime_api() .call( &BlockId::Hash(hash), - from, + from.unwrap_or_default(), data, - value, + value.unwrap_or_default(), gas_limit, gas_price, nonce, - ethereum::TransactionAction::Call(to) + action ) .map_err(|err| internal_err(format!("internal error: {:?}", err)))? - .map_err(|err| internal_err(format!("executing call failed: {:?}", err)))?; + .map_err(handle_call_error)?; Ok(Bytes(ret)) } @@ -517,29 +527,37 @@ impl EthApiT for EthApi where fn estimate_gas(&self, request: CallRequest, _: Option) -> Result { let hash = self.client.info().best_hash; - let from = request.from.unwrap_or_default(); - let gas_price = request.gas_price; - let gas_limit = request.gas.unwrap_or(U256::max_value()); // TODO: this isn't safe - let value = request.value.unwrap_or_default(); - let data = request.data.map(|d| d.0).unwrap_or_default(); - let nonce = request.nonce; + let CallRequest { + from, + to, + gas_price, + gas, + value, + data, + nonce + } = request; + + let gas_limit = gas.unwrap_or(U256::max_value()); // TODO: set a limit + let data = data.map(|d| d.0).unwrap_or_default(); + + let action = match to { + Some(to) => ethereum::TransactionAction::Call(to), + _ => ethereum::TransactionAction::Create, + }; let (_, used_gas) = self.client.runtime_api() .call( &BlockId::Hash(hash), - from, + from.unwrap_or_default(), data, - value, + value.unwrap_or_default(), gas_limit, gas_price, nonce, - match request.to { - Some(to) => ethereum::TransactionAction::Call(to), - _ => ethereum::TransactionAction::Create, - } + action ) .map_err(|err| internal_err(format!("internal error: {:?}", err)))? - .map_err(|err| internal_err(format!("executing call failed: {:?}", err)))?; + .map_err(handle_call_error)?; Ok(used_gas) } diff --git a/rpc/src/lib.rs b/rpc/src/lib.rs index 62d4d6ef1..7444fa1c2 100644 --- a/rpc/src/lib.rs +++ b/rpc/src/lib.rs @@ -20,7 +20,8 @@ mod eth_pubsub; pub use eth::{EthApi, EthApiServer, NetApi, NetApiServer}; pub use eth_pubsub::{EthPubSubApi, EthPubSubApiServer}; -use jsonrpc_core::{ErrorCode, Error}; +use jsonrpc_core::{ErrorCode, Error, Value}; +use rustc_hex::ToHex; pub fn internal_err(message: T) -> Error { Error { @@ -29,3 +30,14 @@ pub fn internal_err(message: T) -> Error { data: None } } + +pub fn handle_call_error((err, data): (sp_runtime::DispatchError, Vec)) -> Error { + let error: &'static str = err.into(); + // TODO: trim error message + let msg = String::from_utf8_lossy(&data); + Error { + code: ErrorCode::InternalError, + message: format!("{}: {}", error.to_lowercase(), msg), + data: Some(Value::String(data.to_hex())) + } +} diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 8fe4886e6..64666004b 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -511,35 +511,9 @@ impl_runtime_apis! { gas_limit: U256, gas_price: Option, nonce: Option, - action: frame_ethereum::TransactionAction, - ) -> Result<(Vec, U256), sp_runtime::DispatchError> { - match action { - frame_ethereum::TransactionAction::Call(to) => - EVM::execute_call( - from, - to, - data, - value, - gas_limit.low_u32(), - gas_price.unwrap_or_default(), - nonce, - false, - ) - .map(|(_, ret, gas, _)| (ret, gas)) - .map_err(|err| err.into()), - frame_ethereum::TransactionAction::Create => - EVM::execute_create( - from, - data, - value, - gas_limit.low_u32(), - gas_price.unwrap_or_default(), - nonce, - false, - ) - .map(|(_, _, gas, _)| (vec![], gas)) - .map_err(|err| err.into()), - } + action: frame_ethereum::TransactionAction + ) -> Result<(Vec, U256), (sp_runtime::DispatchError, Vec)> { + >::call(from, data, value, gas_limit.low_u32(), gas_price.unwrap_or_default(), nonce, action) } fn current_transaction_statuses() -> Option> {