From d48f8a80a299b1905bce1832f29890e4c99ccb1d Mon Sep 17 00:00:00 2001 From: Grigoriy Simonov Date: Tue, 17 Oct 2023 14:09:28 +0000 Subject: [PATCH 1/3] feat: add `OnCheckEvmTransaction` callback --- frame/ethereum/src/lib.rs | 38 +++-- frame/ethereum/src/mock.rs | 1 + frame/evm/precompile/dispatch/src/mock.rs | 1 + frame/evm/src/lib.rs | 25 ++- frame/evm/src/mock.rs | 2 + frame/evm/src/runner/stack.rs | 31 ++-- precompiles/tests-external/lib.rs | 1 + primitives/evm/src/validation.rs | 177 +++++++++++++--------- template/runtime/src/lib.rs | 1 + 9 files changed, 179 insertions(+), 98 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index e9aa870f11..416ec87910 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -65,7 +65,9 @@ use fp_evm::{ }; pub use fp_rpc::TransactionStatus; use fp_storage::{EthereumStorageSchema, PALLET_ETHEREUM_SCHEMA}; -use pallet_evm::{BlockHashMapping, FeeCalculator, GasWeightMapping, Runner}; +use pallet_evm::{ + BlockHashMapping, FeeCalculator, GasWeightMapping, OnCheckEvmTransaction, Runner, +}; #[derive(Clone, Eq, PartialEq, RuntimeDebug)] #[derive(Encode, Decode, MaxEncodedLen, TypeInfo)] @@ -494,7 +496,8 @@ impl Pallet { let (base_fee, _) = T::FeeCalculator::min_gas_price(); let (who, _) = pallet_evm::Pallet::::account_basic(&origin); - let _ = CheckEvmTransaction::::new( + let mut v = CheckEvmTransaction::::new( + who.clone(), CheckEvmTransactionConfig { evm_config: T::config(), block_gas_limit: T::BlockGasLimit::get(), @@ -505,13 +508,19 @@ impl Pallet { transaction_data.clone().into(), weight_limit, proof_size_base_cost, + ); + + T::OnCheckEvmTransaction::::on_check_evm_transaction( + &mut v, &origin, ) - .validate_in_pool_for(&who) - .and_then(|v| v.with_chain_id()) - .and_then(|v| v.with_base_fee()) - .and_then(|v| v.with_balance_for(&who)) .map_err(|e| e.0)?; + v.validate_in_pool_for() + .and_then(|v| v.with_chain_id()) + .and_then(|v| v.with_base_fee()) + .and_then(|v| v.with_balance()) + .map_err(|e| e.0)?; + // EIP-3607: https://eips.ethereum.org/EIPS/eip-3607 // Do not allow transactions for which `tx.sender` has any code deployed. // @@ -862,7 +871,8 @@ impl Pallet { let (base_fee, _) = T::FeeCalculator::min_gas_price(); let (who, _) = pallet_evm::Pallet::::account_basic(&origin); - let _ = CheckEvmTransaction::::new( + let mut v = CheckEvmTransaction::::new( + who, CheckEvmTransactionConfig { evm_config: T::config(), block_gas_limit: T::BlockGasLimit::get(), @@ -873,13 +883,19 @@ impl Pallet { transaction_data.into(), weight_limit, proof_size_base_cost, + ); + + T::OnCheckEvmTransaction::::on_check_evm_transaction( + &mut v, &origin, ) - .validate_in_block_for(&who) - .and_then(|v| v.with_chain_id()) - .and_then(|v| v.with_base_fee()) - .and_then(|v| v.with_balance_for(&who)) .map_err(|e| TransactionValidityError::Invalid(e.0))?; + v.validate_in_block() + .and_then(|v| v.with_chain_id()) + .and_then(|v| v.with_base_fee()) + .and_then(|v| v.with_balance()) + .map_err(|e| TransactionValidityError::Invalid(e.0))?; + Ok(()) } diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 9eeb8f7e7b..49f0490726 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -177,6 +177,7 @@ impl pallet_evm::Config for Test { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = (); + type OnCheckEvmTransaction> = (); } parameter_types! { diff --git a/frame/evm/precompile/dispatch/src/mock.rs b/frame/evm/precompile/dispatch/src/mock.rs index 1c55895817..c71a225f37 100644 --- a/frame/evm/precompile/dispatch/src/mock.rs +++ b/frame/evm/precompile/dispatch/src/mock.rs @@ -157,6 +157,7 @@ impl pallet_evm::Config for Test { type GasLimitPovSizeRatio = (); type Timestamp = Timestamp; type WeightInfo = (); + type OnCheckEvmTransaction> = (); } pub(crate) struct MockHandle { diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs index 59945a4fd1..13b215c9c4 100644 --- a/frame/evm/src/lib.rs +++ b/frame/evm/src/lib.rs @@ -98,9 +98,10 @@ use sp_std::{cmp::min, collections::btree_map::BTreeMap, vec::Vec}; use fp_account::AccountId20; use fp_evm::GenesisAccount; pub use fp_evm::{ - Account, CallInfo, CreateInfo, ExecutionInfoV2 as ExecutionInfo, FeeCalculator, - IsPrecompileResult, LinearCostPrecompile, Log, Precompile, PrecompileFailure, PrecompileHandle, - PrecompileOutput, PrecompileResult, PrecompileSet, TransactionValidationError, Vicinity, + Account, CallInfo, CheckEvmTransaction, CreateInfo, ExecutionInfoV2 as ExecutionInfo, + FeeCalculator, IsPrecompileResult, LinearCostPrecompile, Log, Precompile, PrecompileFailure, + PrecompileHandle, PrecompileOutput, PrecompileResult, PrecompileSet, + TransactionValidationError, Vicinity, }; pub use self::{ @@ -182,6 +183,12 @@ pub mod pallet { fn config() -> &'static EvmConfig { &SHANGHAI_CONFIG } + + // Called when transaction info for validation is created + type OnCheckEvmTransaction>: OnCheckEvmTransaction< + Self, + E, + >; } #[pallet::call] @@ -581,7 +588,7 @@ pub type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; /// Type alias for negative imbalance during fees -type NegativeImbalanceOf = +pub type NegativeImbalanceOf = ::AccountId>>::NegativeImbalance; #[derive( @@ -1089,3 +1096,13 @@ impl OnCreate for Tuple { )*) } } + +pub trait OnCheckEvmTransaction> { + fn on_check_evm_transaction(v: &mut CheckEvmTransaction, origin: &H160) -> Result<(), E>; +} + +impl> OnCheckEvmTransaction for () { + fn on_check_evm_transaction(_v: &mut CheckEvmTransaction, _origin: &H160) -> Result<(), E> { + Ok(()) + } +} diff --git a/frame/evm/src/mock.rs b/frame/evm/src/mock.rs index 9e1e481d5d..3c7facb314 100644 --- a/frame/evm/src/mock.rs +++ b/frame/evm/src/mock.rs @@ -32,6 +32,7 @@ use sp_std::{boxed::Box, prelude::*, str::FromStr}; use crate::{ EnsureAddressNever, EnsureAddressRoot, FeeCalculator, IdentityAddressMapping, IsPrecompileResult, Precompile, PrecompileHandle, PrecompileResult, PrecompileSet, + TransactionValidationError, }; frame_support::construct_runtime! { @@ -155,6 +156,7 @@ impl crate::Config for Test { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = (); + type OnCheckEvmTransaction> = (); } /// Example PrecompileSet with only Identity precompile. diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 3d20134c2d..e08ff1313f 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -17,6 +17,11 @@ //! EVM stack-based runner. +use crate::{ + runner::Runner as RunnerT, AccountCodes, AccountCodesMetadata, AccountStorages, AddressMapping, + BalanceOf, BlockHashMapping, Config, Error, Event, FeeCalculator, OnChargeEVMTransaction, + OnCheckEvmTransaction, OnCreate, Pallet, RunnerError, +}; use evm::{ backend::Backend as BackendT, executor::stack::{Accessed, StackExecutor, StackState as StackStateT, StackSubstateMetadata}, @@ -47,12 +52,6 @@ use fp_evm::{ ACCOUNT_STORAGE_PROOF_SIZE, IS_EMPTY_CHECK_PROOF_SIZE, WRITE_PROOF_SIZE, }; -use crate::{ - runner::Runner as RunnerT, AccountCodes, AccountCodesMetadata, AccountStorages, AddressMapping, - BalanceOf, BlockHashMapping, Config, Error, Event, FeeCalculator, OnChargeEVMTransaction, - OnCreate, Pallet, RunnerError, -}; - #[cfg(feature = "forbid-evm-reentrancy")] environmental::thread_local_impl!(static IN_EVM: environmental::RefCell = environmental::RefCell::new(false)); @@ -376,8 +375,10 @@ where let (base_fee, mut weight) = T::FeeCalculator::min_gas_price(); let (source_account, inner_weight) = Pallet::::account_basic(&source); weight = weight.saturating_add(inner_weight); + let nonce = nonce.unwrap_or(source_account.nonce); - let _ = fp_evm::CheckEvmTransaction::::new( + let mut v = fp_evm::CheckEvmTransaction::::new( + source_account, fp_evm::CheckEvmTransactionConfig { evm_config, block_gas_limit: T::BlockGasLimit::get(), @@ -389,7 +390,7 @@ where chain_id: Some(T::ChainId::get()), to: target, input, - nonce: nonce.unwrap_or(source_account.nonce), + nonce, gas_limit: gas_limit.into(), gas_price: None, max_fee_per_gas, @@ -399,11 +400,15 @@ where }, weight_limit, proof_size_base_cost, - ) - .validate_in_block_for(&source_account) - .and_then(|v| v.with_base_fee()) - .and_then(|v| v.with_balance_for(&source_account)) - .map_err(|error| RunnerError { error, weight })?; + ); + + T::OnCheckEvmTransaction::>::on_check_evm_transaction(&mut v, &source) + .map_err(|error| RunnerError { error, weight })?; + + v.validate_in_block() + .and_then(|v| v.with_base_fee()) + .and_then(|v| v.with_balance()) + .map_err(|error| RunnerError { error, weight })?; Ok(()) } diff --git a/precompiles/tests-external/lib.rs b/precompiles/tests-external/lib.rs index 75199c1cee..2a69928be7 100644 --- a/precompiles/tests-external/lib.rs +++ b/precompiles/tests-external/lib.rs @@ -239,6 +239,7 @@ impl pallet_evm::Config for Runtime { type Runner = pallet_evm::runner::stack::Runner; type OnChargeTransaction = (); type OnCreate = (); + type OnCheckEvmTransaction = (); type FindAuthor = (); type GasLimitPovSizeRatio = GasLimitPovSizeRatio; type SuicideQuickClearLimit = SuicideQuickClearLimit; diff --git a/primitives/evm/src/validation.rs b/primitives/evm/src/validation.rs index 405f09a4b3..e9300ef56d 100644 --- a/primitives/evm/src/validation.rs +++ b/primitives/evm/src/validation.rs @@ -47,6 +47,7 @@ pub struct CheckEvmTransactionConfig<'config> { #[derive(Debug)] pub struct CheckEvmTransaction<'config, E: From> { + pub who: Account, pub config: CheckEvmTransactionConfig<'config>, pub transaction: CheckEvmTransactionInput, pub weight_limit: Option, @@ -85,12 +86,14 @@ pub enum TransactionValidationError { impl<'config, E: From> CheckEvmTransaction<'config, E> { pub fn new( + who: Account, config: CheckEvmTransactionConfig<'config>, transaction: CheckEvmTransactionInput, weight_limit: Option, proof_size_base_cost: Option, ) -> Self { CheckEvmTransaction { + who, config, transaction, weight_limit, @@ -99,17 +102,17 @@ impl<'config, E: From> CheckEvmTransaction<'config, } } - pub fn validate_in_pool_for(&self, who: &Account) -> Result<&Self, E> { - if self.transaction.nonce < who.nonce { + pub fn validate_in_pool_for(&self) -> Result<&Self, E> { + if self.transaction.nonce < self.who.nonce { return Err(TransactionValidationError::TxNonceTooLow.into()); } self.validate_common() } - pub fn validate_in_block_for(&self, who: &Account) -> Result<&Self, E> { - if self.transaction.nonce > who.nonce { + pub fn validate_in_block(&self) -> Result<&Self, E> { + if self.transaction.nonce > self.who.nonce { return Err(TransactionValidationError::TxNonceTooHigh.into()); - } else if self.transaction.nonce < who.nonce { + } else if self.transaction.nonce < self.who.nonce { return Err(TransactionValidationError::TxNonceTooLow.into()); } self.validate_common() @@ -137,7 +140,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, Ok(self) } - pub fn with_balance_for(&self, who: &Account) -> Result<&Self, E> { + pub fn with_balance(&self) -> Result<&Self, E> { // Get fee data from either a legacy or typed transaction input. let (max_fee_per_gas, _) = self.transaction_fee_input()?; @@ -153,7 +156,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, let fee = max_fee_per_gas.saturating_mul(self.transaction.gas_limit); if self.config.is_transactional || fee > U256::zero() { let total_payment = self.transaction.value.saturating_add(fee); - if who.balance < total_payment { + if self.who.balance < total_payment { return Err(TransactionValidationError::BalanceTooLow.into()); } } @@ -163,7 +166,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, // Returns the max_fee_per_gas (or gas_price for legacy txns) as well as an optional // effective_gas_price for EIP-1559 transactions. effective_gas_price represents // the total (fee + tip) that would be paid given the current base_fee. - fn transaction_fee_input(&self) -> Result<(U256, Option), E> { + pub fn transaction_fee_input(&self) -> Result<(U256, Option), E> { match ( self.transaction.gas_price, self.transaction.max_fee_per_gas, @@ -297,6 +300,7 @@ mod tests { pub value: U256, pub weight_limit: Option, pub proof_size_base_cost: Option, + pub who: Account, } impl Default for TestCase { @@ -315,6 +319,7 @@ mod tests { value: U256::from(1u8), weight_limit: None, proof_size_base_cost: None, + who: Account::default(), } } } @@ -334,8 +339,10 @@ mod tests { value, weight_limit, proof_size_base_cost, + who, } = input; CheckEvmTransaction::::new( + who, CheckEvmTransactionConfig { evm_config: &SHANGHAI_CONFIG, block_gas_limit: blockchain_gas_limit, @@ -362,18 +369,22 @@ mod tests { // Transaction settings fn default_transaction<'config>( + who: Account, is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, is_transactional, ..Default::default() }) } fn transaction_gas_limit_low<'config>( + who: Account, is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, gas_limit: U256::from(1u8), is_transactional, ..Default::default() @@ -381,9 +392,11 @@ mod tests { } fn transaction_gas_limit_low_proof_size<'config>( + who: Account, is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, weight_limit: Some(Weight::from_parts(1, 1)), proof_size_base_cost: Some(2), is_transactional, @@ -391,31 +404,40 @@ mod tests { }) } - fn transaction_gas_limit_high<'config>() -> CheckEvmTransaction<'config, TestError> { + fn transaction_gas_limit_high<'config>( + who: Account, + ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, blockchain_gas_limit: U256::from(1u8), ..Default::default() }) } - fn transaction_nonce_high<'config>() -> CheckEvmTransaction<'config, TestError> { + fn transaction_nonce_high<'config>(who: Account) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, nonce: U256::from(10u8), ..Default::default() }) } - fn transaction_invalid_chain_id<'config>() -> CheckEvmTransaction<'config, TestError> { + fn transaction_invalid_chain_id<'config>( + who: Account, + ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, chain_id: Some(555u64), ..Default::default() }) } fn transaction_none_fee<'config>( + who: Account, is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, max_fee_per_gas: None, max_priority_fee_per_gas: None, is_transactional, @@ -427,6 +449,7 @@ mod tests { is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who: Account::default(), max_fee_per_gas: Some(U256::from(1u8)), max_priority_fee_per_gas: None, is_transactional, @@ -438,14 +461,19 @@ mod tests { is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who: Account::default(), max_priority_fee_per_gas: Some(U256::from(1_100_000_000)), is_transactional, ..Default::default() }) } - fn transaction_max_fee_high<'config>(tip: bool) -> CheckEvmTransaction<'config, TestError> { + fn transaction_max_fee_high<'config>( + who: Account, + tip: bool, + ) -> CheckEvmTransaction<'config, TestError> { let mut input = TestCase { + who, max_fee_per_gas: Some(U256::from(5_000_000_000u128)), ..Default::default() }; @@ -455,8 +483,9 @@ mod tests { test_env(input) } - fn legacy_transaction<'config>() -> CheckEvmTransaction<'config, TestError> { + fn legacy_transaction<'config>(who: Account) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, gas_price: Some(U256::from(1_000_000_000u128)), max_fee_per_gas: None, max_priority_fee_per_gas: None, @@ -465,9 +494,11 @@ mod tests { } fn invalid_transaction_mixed_fees<'config>( + who: Account, is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { test_env(TestCase { + who, gas_price: Some(U256::from(1_000_000_000u128)), max_fee_per_gas: Some(U256::from(1_000_000_000u128)), max_priority_fee_per_gas: None, @@ -483,11 +514,11 @@ mod tests { balance: U256::from(1_000_000u128), nonce: U256::zero(), }; - let test = default_transaction(true); + let test = default_transaction(who, true); // Pool - assert!(test.validate_in_pool_for(&who).is_ok()); + assert!(test.validate_in_pool_for().is_ok()); // Block - assert!(test.validate_in_block_for(&who).is_ok()); + assert!(test.validate_in_block().is_ok()); } // Nonce too low fails in pool and in block. @@ -497,13 +528,13 @@ mod tests { balance: U256::from(1_000_000u128), nonce: U256::from(1u8), }; - let test = default_transaction(true); + let test = default_transaction(who, true); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::TxNonceTooLow); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::TxNonceTooLow); } @@ -515,8 +546,8 @@ mod tests { balance: U256::from(1_000_000u128), nonce: U256::from(1u8), }; - let test = transaction_nonce_high(); - let res = test.validate_in_pool_for(&who); + let test = transaction_nonce_high(who); + let res = test.validate_in_pool_for(); assert!(res.is_ok()); } @@ -527,8 +558,8 @@ mod tests { balance: U256::from(1_000_000u128), nonce: U256::from(1u8), }; - let test = transaction_nonce_high(); - let res = test.validate_in_block_for(&who); + let test = transaction_nonce_high(who); + let res = test.validate_in_block(); assert!(res.is_err()); } @@ -540,13 +571,13 @@ mod tests { nonce: U256::zero(), }; let is_transactional = true; - let test = transaction_gas_limit_low(is_transactional); + let test = transaction_gas_limit_low(who, is_transactional); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); } @@ -559,12 +590,12 @@ mod tests { nonce: U256::zero(), }; let is_transactional = false; - let test = transaction_gas_limit_low(is_transactional); + let test = transaction_gas_limit_low(who, is_transactional); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_ok()); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_ok()); } @@ -576,13 +607,13 @@ mod tests { nonce: U256::zero(), }; let is_transactional = true; - let test = transaction_gas_limit_low_proof_size(is_transactional); + let test = transaction_gas_limit_low_proof_size(who, is_transactional); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); } @@ -595,12 +626,12 @@ mod tests { nonce: U256::zero(), }; let is_transactional = false; - let test = transaction_gas_limit_low_proof_size(is_transactional); + let test = transaction_gas_limit_low_proof_size(who, is_transactional); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_ok()); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_ok()); } @@ -611,13 +642,13 @@ mod tests { balance: U256::from(1_000_000u128), nonce: U256::zero(), }; - let test = transaction_gas_limit_high(); + let test = transaction_gas_limit_high(who); // Pool - let res = test.validate_in_pool_for(&who); + let res = test.validate_in_pool_for(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooHigh); // Block - let res = test.validate_in_block_for(&who); + let res = test.validate_in_block(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::GasLimitTooHigh); } @@ -625,7 +656,8 @@ mod tests { // Valid chain id succeeds. #[test] fn validate_chain_id_succeeds() { - let test = default_transaction(true); + let who = Account::default(); + let test = default_transaction(who, true); let res = test.with_chain_id(); assert!(res.is_ok()); } @@ -633,7 +665,8 @@ mod tests { // Invalid chain id fails. #[test] fn validate_chain_id_fails() { - let test = transaction_invalid_chain_id(); + let who = Account::default(); + let test = transaction_invalid_chain_id(who); let res = test.with_chain_id(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::InvalidChainId); @@ -642,12 +675,13 @@ mod tests { // Valid max fee per gas succeeds. #[test] fn validate_base_fee_succeeds() { + let who = Account::default(); // Transactional - let test = default_transaction(true); + let test = default_transaction(who.clone(), true); let res = test.with_base_fee(); assert!(res.is_ok()); // Non-transactional - let test = default_transaction(false); + let test = default_transaction(who, false); let res = test.with_base_fee(); assert!(res.is_ok()); } @@ -655,7 +689,8 @@ mod tests { // Transactional call with unset fee data fails. #[test] fn validate_base_fee_with_none_fee_fails() { - let test = transaction_none_fee(true); + let who = Account::default(); + let test = transaction_none_fee(who, true); let res = test.with_base_fee(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); @@ -664,7 +699,8 @@ mod tests { // Non-transactional call with unset fee data succeeds. #[test] fn validate_base_fee_with_none_fee_non_transactional_succeeds() { - let test = transaction_none_fee(false); + let who = Account::default(); + let test = transaction_none_fee(who, false); let res = test.with_base_fee(); assert!(res.is_ok()); } @@ -707,12 +743,12 @@ mod tests { nonce: U256::zero(), }; // Transactional - let test = default_transaction(true); - let res = test.with_balance_for(&who); + let test = default_transaction(who.clone(), true); + let res = test.with_balance(); assert!(res.is_ok()); // Non-transactional - let test = default_transaction(false); - let res = test.with_balance_for(&who); + let test = default_transaction(who, false); + let res = test.with_balance(); assert!(res.is_ok()); } @@ -724,13 +760,13 @@ mod tests { nonce: U256::zero(), }; // Transactional - let test = default_transaction(true); - let res = test.with_balance_for(&who); + let test = default_transaction(who.clone(), true); + let res = test.with_balance(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); // Non-transactional - let test = default_transaction(false); - let res = test.with_balance_for(&who); + let test = default_transaction(who, false); + let res = test.with_balance(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); } @@ -742,8 +778,8 @@ mod tests { balance: U256::from(21_000_000_000_001u128), nonce: U256::zero(), }; - let test = transaction_none_fee(true); - let res = test.with_balance_for(&who); + let test = transaction_none_fee(who, true); + let res = test.with_balance(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); } @@ -755,8 +791,8 @@ mod tests { balance: U256::from(0u8), nonce: U256::zero(), }; - let test = transaction_none_fee(false); - let res = test.with_balance_for(&who); + let test = transaction_none_fee(who, false); + let res = test.with_balance(); assert!(res.is_ok()); } @@ -769,8 +805,8 @@ mod tests { nonce: U256::zero(), }; let with_tip = false; - let test = transaction_max_fee_high(with_tip); - let res = test.with_balance_for(&who); + let test = transaction_max_fee_high(who, with_tip); + let res = test.with_balance(); assert!(res.is_err()); } @@ -783,8 +819,8 @@ mod tests { nonce: U256::zero(), }; let with_tip = true; - let test = transaction_max_fee_high(with_tip); - let res = test.with_balance_for(&who); + let test = transaction_max_fee_high(who, with_tip); + let res = test.with_balance(); assert!(res.is_err()); } @@ -795,8 +831,8 @@ mod tests { balance: U256::from(21_000_000_000_001u128), nonce: U256::zero(), }; - let test = legacy_transaction(); - let res = test.with_balance_for(&who); + let test = legacy_transaction(who); + let res = test.with_balance(); assert!(res.is_ok()); } @@ -807,8 +843,8 @@ mod tests { balance: U256::from(21_000_000_000_000u128), nonce: U256::zero(), }; - let test = legacy_transaction(); - let res = test.with_balance_for(&who); + let test = legacy_transaction(who); + let res = test.with_balance(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); } @@ -822,14 +858,14 @@ mod tests { }; // Fails for transactional. let is_transactional = true; - let test = invalid_transaction_mixed_fees(is_transactional); - let res = test.with_balance_for(&who); + let test = invalid_transaction_mixed_fees(who.clone(), is_transactional); + let res = test.with_balance(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); // Succeeds for non-transactional. let is_transactional = false; - let test = invalid_transaction_mixed_fees(is_transactional); - let res = test.with_balance_for(&who); + let test = invalid_transaction_mixed_fees(who, is_transactional); + let res = test.with_balance(); assert!(res.is_ok()); } @@ -838,13 +874,14 @@ mod tests { fn validate_base_fee_with_invalid_fee_input() { // Fails for transactional. let is_transactional = true; - let test = invalid_transaction_mixed_fees(is_transactional); + let who = Account::default(); + let test = invalid_transaction_mixed_fees(who.clone(), is_transactional); let res = test.with_base_fee(); assert!(res.is_err()); assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); // Succeeds for non-transactional. let is_transactional = false; - let test = invalid_transaction_mixed_fees(is_transactional); + let test = invalid_transaction_mixed_fees(who, is_transactional); let res = test.with_base_fee(); assert!(res.is_ok()); } diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 5995ec780a..2bb290ed06 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -349,6 +349,7 @@ impl pallet_evm::Config for Runtime { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = pallet_evm::weights::SubstrateWeight; + type OnCheckEvmTransaction> = (); } parameter_types! { From 213a8c6e3ace6faa21aa706fe168aaf9225867aa Mon Sep 17 00:00:00 2001 From: Grigoriy Simonov Date: Tue, 17 Oct 2023 14:09:28 +0000 Subject: [PATCH 2/3] fix: remove generic error from `OnCheckEvmTransaction` --- frame/ethereum/src/lib.rs | 22 +-- frame/ethereum/src/mock.rs | 2 +- frame/evm/precompile/dispatch/src/mock.rs | 2 +- frame/evm/src/lib.rs | 19 ++- frame/evm/src/mock.rs | 3 +- frame/evm/src/runner/stack.rs | 15 +- primitives/evm/src/validation.rs | 194 ++++++++++------------ template/runtime/src/lib.rs | 2 +- 8 files changed, 123 insertions(+), 136 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index 416ec87910..a313a85a7b 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -496,7 +496,7 @@ impl Pallet { let (base_fee, _) = T::FeeCalculator::min_gas_price(); let (who, _) = pallet_evm::Pallet::::account_basic(&origin); - let mut v = CheckEvmTransaction::::new( + let mut v = CheckEvmTransaction::new( who.clone(), CheckEvmTransactionConfig { evm_config: T::config(), @@ -510,16 +510,14 @@ impl Pallet { proof_size_base_cost, ); - T::OnCheckEvmTransaction::::on_check_evm_transaction( - &mut v, &origin, - ) - .map_err(|e| e.0)?; + T::OnCheckEvmTransaction::on_check_evm_transaction(&mut v, &origin) + .map_err(|e| InvalidTransactionWrapper::from(e).0)?; - v.validate_in_pool_for() + v.validate_in_pool() .and_then(|v| v.with_chain_id()) .and_then(|v| v.with_base_fee()) .and_then(|v| v.with_balance()) - .map_err(|e| e.0)?; + .map_err(|e| InvalidTransactionWrapper::from(e).0)?; // EIP-3607: https://eips.ethereum.org/EIPS/eip-3607 // Do not allow transactions for which `tx.sender` has any code deployed. @@ -871,7 +869,7 @@ impl Pallet { let (base_fee, _) = T::FeeCalculator::min_gas_price(); let (who, _) = pallet_evm::Pallet::::account_basic(&origin); - let mut v = CheckEvmTransaction::::new( + let mut v = CheckEvmTransaction::new( who, CheckEvmTransactionConfig { evm_config: T::config(), @@ -885,16 +883,14 @@ impl Pallet { proof_size_base_cost, ); - T::OnCheckEvmTransaction::::on_check_evm_transaction( - &mut v, &origin, - ) - .map_err(|e| TransactionValidityError::Invalid(e.0))?; + T::OnCheckEvmTransaction::on_check_evm_transaction(&mut v, &origin) + .map_err(|e| TransactionValidityError::Invalid(InvalidTransactionWrapper::from(e).0))?; v.validate_in_block() .and_then(|v| v.with_chain_id()) .and_then(|v| v.with_base_fee()) .and_then(|v| v.with_balance()) - .map_err(|e| TransactionValidityError::Invalid(e.0))?; + .map_err(|e| TransactionValidityError::Invalid(InvalidTransactionWrapper::from(e).0))?; Ok(()) } diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 49f0490726..f55b2604e7 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -177,7 +177,7 @@ impl pallet_evm::Config for Test { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = (); - type OnCheckEvmTransaction> = (); + type OnCheckEvmTransaction = (); } parameter_types! { diff --git a/frame/evm/precompile/dispatch/src/mock.rs b/frame/evm/precompile/dispatch/src/mock.rs index c71a225f37..3e92aad7de 100644 --- a/frame/evm/precompile/dispatch/src/mock.rs +++ b/frame/evm/precompile/dispatch/src/mock.rs @@ -157,7 +157,7 @@ impl pallet_evm::Config for Test { type GasLimitPovSizeRatio = (); type Timestamp = Timestamp; type WeightInfo = (); - type OnCheckEvmTransaction> = (); + type OnCheckEvmTransaction = (); } pub(crate) struct MockHandle { diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs index 13b215c9c4..1a484eb69a 100644 --- a/frame/evm/src/lib.rs +++ b/frame/evm/src/lib.rs @@ -185,10 +185,7 @@ pub mod pallet { } // Called when transaction info for validation is created - type OnCheckEvmTransaction>: OnCheckEvmTransaction< - Self, - E, - >; + type OnCheckEvmTransaction: OnCheckEvmTransaction; } #[pallet::call] @@ -1097,12 +1094,18 @@ impl OnCreate for Tuple { } } -pub trait OnCheckEvmTransaction> { - fn on_check_evm_transaction(v: &mut CheckEvmTransaction, origin: &H160) -> Result<(), E>; +pub trait OnCheckEvmTransaction { + fn on_check_evm_transaction( + v: &mut CheckEvmTransaction, + origin: &H160, + ) -> Result<(), TransactionValidationError>; } -impl> OnCheckEvmTransaction for () { - fn on_check_evm_transaction(_v: &mut CheckEvmTransaction, _origin: &H160) -> Result<(), E> { +impl OnCheckEvmTransaction for () { + fn on_check_evm_transaction( + _v: &mut CheckEvmTransaction, + _origin: &H160, + ) -> Result<(), TransactionValidationError> { Ok(()) } } diff --git a/frame/evm/src/mock.rs b/frame/evm/src/mock.rs index 3c7facb314..65bbb19482 100644 --- a/frame/evm/src/mock.rs +++ b/frame/evm/src/mock.rs @@ -32,7 +32,6 @@ use sp_std::{boxed::Box, prelude::*, str::FromStr}; use crate::{ EnsureAddressNever, EnsureAddressRoot, FeeCalculator, IdentityAddressMapping, IsPrecompileResult, Precompile, PrecompileHandle, PrecompileResult, PrecompileSet, - TransactionValidationError, }; frame_support::construct_runtime! { @@ -156,7 +155,7 @@ impl crate::Config for Test { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = (); - type OnCheckEvmTransaction> = (); + type OnCheckEvmTransaction = (); } /// Example PrecompileSet with only Identity precompile. diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index e08ff1313f..bf829e3f1e 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -377,7 +377,7 @@ where weight = weight.saturating_add(inner_weight); let nonce = nonce.unwrap_or(source_account.nonce); - let mut v = fp_evm::CheckEvmTransaction::::new( + let mut v = fp_evm::CheckEvmTransaction::new( source_account, fp_evm::CheckEvmTransactionConfig { evm_config, @@ -402,13 +402,20 @@ where proof_size_base_cost, ); - T::OnCheckEvmTransaction::>::on_check_evm_transaction(&mut v, &source) - .map_err(|error| RunnerError { error, weight })?; + T::OnCheckEvmTransaction::on_check_evm_transaction(&mut v, &source).map_err(|error| { + RunnerError { + error: error.into(), + weight, + } + })?; v.validate_in_block() .and_then(|v| v.with_base_fee()) .and_then(|v| v.with_balance()) - .map_err(|error| RunnerError { error, weight })?; + .map_err(|error| RunnerError { + error: error.into(), + weight, + })?; Ok(()) } diff --git a/primitives/evm/src/validation.rs b/primitives/evm/src/validation.rs index e9300ef56d..55f1b569e7 100644 --- a/primitives/evm/src/validation.rs +++ b/primitives/evm/src/validation.rs @@ -46,18 +46,17 @@ pub struct CheckEvmTransactionConfig<'config> { } #[derive(Debug)] -pub struct CheckEvmTransaction<'config, E: From> { +pub struct CheckEvmTransaction<'config> { pub who: Account, pub config: CheckEvmTransactionConfig<'config>, pub transaction: CheckEvmTransactionInput, pub weight_limit: Option, pub proof_size_base_cost: Option, - _marker: sp_std::marker::PhantomData, } /// Transaction validation errors #[repr(u8)] -#[derive(num_enum::FromPrimitive, num_enum::IntoPrimitive, Debug)] +#[derive(num_enum::FromPrimitive, num_enum::IntoPrimitive, Debug, PartialEq)] pub enum TransactionValidationError { /// The transaction gas limit is too low GasLimitTooLow, @@ -84,7 +83,7 @@ pub enum TransactionValidationError { UnknownError, } -impl<'config, E: From> CheckEvmTransaction<'config, E> { +impl<'config> CheckEvmTransaction<'config> { pub fn new( who: Account, config: CheckEvmTransactionConfig<'config>, @@ -98,49 +97,48 @@ impl<'config, E: From> CheckEvmTransaction<'config, transaction, weight_limit, proof_size_base_cost, - _marker: Default::default(), } } - pub fn validate_in_pool_for(&self) -> Result<&Self, E> { + pub fn validate_in_pool(&self) -> Result<&Self, TransactionValidationError> { if self.transaction.nonce < self.who.nonce { - return Err(TransactionValidationError::TxNonceTooLow.into()); + return Err(TransactionValidationError::TxNonceTooLow); } self.validate_common() } - pub fn validate_in_block(&self) -> Result<&Self, E> { + pub fn validate_in_block(&self) -> Result<&Self, TransactionValidationError> { if self.transaction.nonce > self.who.nonce { - return Err(TransactionValidationError::TxNonceTooHigh.into()); + return Err(TransactionValidationError::TxNonceTooHigh); } else if self.transaction.nonce < self.who.nonce { - return Err(TransactionValidationError::TxNonceTooLow.into()); + return Err(TransactionValidationError::TxNonceTooLow); } self.validate_common() } - pub fn with_chain_id(&self) -> Result<&Self, E> { + pub fn with_chain_id(&self) -> Result<&Self, TransactionValidationError> { // Chain id matches the one in the signature. if let Some(chain_id) = self.transaction.chain_id { if chain_id != self.config.chain_id { - return Err(TransactionValidationError::InvalidChainId.into()); + return Err(TransactionValidationError::InvalidChainId); } } Ok(self) } - pub fn with_base_fee(&self) -> Result<&Self, E> { + pub fn with_base_fee(&self) -> Result<&Self, TransactionValidationError> { // Get fee data from either a legacy or typed transaction input. let (gas_price, _) = self.transaction_fee_input()?; if self.config.is_transactional || gas_price > U256::zero() { // Transaction max fee is at least the current base fee. if gas_price < self.config.base_fee { - return Err(TransactionValidationError::GasPriceTooLow.into()); + return Err(TransactionValidationError::GasPriceTooLow); } } Ok(self) } - pub fn with_balance(&self) -> Result<&Self, E> { + pub fn with_balance(&self) -> Result<&Self, TransactionValidationError> { // Get fee data from either a legacy or typed transaction input. let (max_fee_per_gas, _) = self.transaction_fee_input()?; @@ -157,7 +155,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, if self.config.is_transactional || fee > U256::zero() { let total_payment = self.transaction.value.saturating_add(fee); if self.who.balance < total_payment { - return Err(TransactionValidationError::BalanceTooLow.into()); + return Err(TransactionValidationError::BalanceTooLow); } } Ok(self) @@ -166,7 +164,9 @@ impl<'config, E: From> CheckEvmTransaction<'config, // Returns the max_fee_per_gas (or gas_price for legacy txns) as well as an optional // effective_gas_price for EIP-1559 transactions. effective_gas_price represents // the total (fee + tip) that would be paid given the current base_fee. - pub fn transaction_fee_input(&self) -> Result<(U256, Option), E> { + pub fn transaction_fee_input( + &self, + ) -> Result<(U256, Option), TransactionValidationError> { match ( self.transaction.gas_price, self.transaction.max_fee_per_gas, @@ -181,7 +181,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, // EIP-1559 tip. (None, Some(max_fee_per_gas), Some(max_priority_fee_per_gas)) => { if max_priority_fee_per_gas > max_fee_per_gas { - return Err(TransactionValidationError::PriorityFeeTooHigh.into()); + return Err(TransactionValidationError::PriorityFeeTooHigh); } let effective_gas_price = self .config @@ -193,7 +193,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, } _ => { if self.config.is_transactional { - Err(TransactionValidationError::InvalidFeeInput.into()) + Err(TransactionValidationError::InvalidFeeInput) } else { // Allow non-set fee input for non-transactional calls. Ok((U256::zero(), None)) @@ -202,7 +202,7 @@ impl<'config, E: From> CheckEvmTransaction<'config, } } - pub fn validate_common(&self) -> Result<&Self, E> { + pub fn validate_common(&self) -> Result<&Self, TransactionValidationError> { if self.config.is_transactional { // Try to subtract the proof_size_base_cost from the Weight proof_size limit or fail. // Validate the weight limit can afford recording the proof size cost. @@ -234,12 +234,12 @@ impl<'config, E: From> CheckEvmTransaction<'config, }; if gasometer.record_transaction(transaction_cost).is_err() { - return Err(TransactionValidationError::GasLimitTooLow.into()); + return Err(TransactionValidationError::GasLimitTooLow); } // Transaction gas limit is within the upper bound block gas limit. if self.transaction.gas_limit > self.config.block_gas_limit { - return Err(TransactionValidationError::GasLimitTooHigh.into()); + return Err(TransactionValidationError::GasLimitTooHigh); } } @@ -251,41 +251,8 @@ impl<'config, E: From> CheckEvmTransaction<'config, mod tests { use super::*; - #[derive(Debug, PartialEq)] - pub enum TestError { - GasLimitTooLow, - GasLimitTooHigh, - GasPriceTooLow, - PriorityFeeTooHigh, - BalanceTooLow, - TxNonceTooLow, - TxNonceTooHigh, - InvalidFeeInput, - InvalidChainId, - InvalidSignature, - UnknownError, - } - static SHANGHAI_CONFIG: evm::Config = evm::Config::shanghai(); - impl From for TestError { - fn from(e: TransactionValidationError) -> Self { - match e { - TransactionValidationError::GasLimitTooLow => TestError::GasLimitTooLow, - TransactionValidationError::GasLimitTooHigh => TestError::GasLimitTooHigh, - TransactionValidationError::GasPriceTooLow => TestError::GasPriceTooLow, - TransactionValidationError::PriorityFeeTooHigh => TestError::PriorityFeeTooHigh, - TransactionValidationError::BalanceTooLow => TestError::BalanceTooLow, - TransactionValidationError::TxNonceTooLow => TestError::TxNonceTooLow, - TransactionValidationError::TxNonceTooHigh => TestError::TxNonceTooHigh, - TransactionValidationError::InvalidFeeInput => TestError::InvalidFeeInput, - TransactionValidationError::InvalidChainId => TestError::InvalidChainId, - TransactionValidationError::InvalidSignature => TestError::InvalidSignature, - TransactionValidationError::UnknownError => TestError::UnknownError, - } - } - } - struct TestCase { pub blockchain_gas_limit: U256, pub blockchain_base_fee: U256, @@ -324,7 +291,7 @@ mod tests { } } - fn test_env<'config>(input: TestCase) -> CheckEvmTransaction<'config, TestError> { + fn test_env<'config>(input: TestCase) -> CheckEvmTransaction<'config> { let TestCase { blockchain_gas_limit, blockchain_base_fee, @@ -341,7 +308,7 @@ mod tests { proof_size_base_cost, who, } = input; - CheckEvmTransaction::::new( + CheckEvmTransaction::new( who, CheckEvmTransactionConfig { evm_config: &SHANGHAI_CONFIG, @@ -371,7 +338,7 @@ mod tests { fn default_transaction<'config>( who: Account, is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who, is_transactional, @@ -382,7 +349,7 @@ mod tests { fn transaction_gas_limit_low<'config>( who: Account, is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who, gas_limit: U256::from(1u8), @@ -394,7 +361,7 @@ mod tests { fn transaction_gas_limit_low_proof_size<'config>( who: Account, is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who, weight_limit: Some(Weight::from_parts(1, 1)), @@ -404,9 +371,7 @@ mod tests { }) } - fn transaction_gas_limit_high<'config>( - who: Account, - ) -> CheckEvmTransaction<'config, TestError> { + fn transaction_gas_limit_high<'config>(who: Account) -> CheckEvmTransaction<'config> { test_env(TestCase { who, blockchain_gas_limit: U256::from(1u8), @@ -414,7 +379,7 @@ mod tests { }) } - fn transaction_nonce_high<'config>(who: Account) -> CheckEvmTransaction<'config, TestError> { + fn transaction_nonce_high<'config>(who: Account) -> CheckEvmTransaction<'config> { test_env(TestCase { who, nonce: U256::from(10u8), @@ -422,9 +387,7 @@ mod tests { }) } - fn transaction_invalid_chain_id<'config>( - who: Account, - ) -> CheckEvmTransaction<'config, TestError> { + fn transaction_invalid_chain_id<'config>(who: Account) -> CheckEvmTransaction<'config> { test_env(TestCase { who, chain_id: Some(555u64), @@ -435,7 +398,7 @@ mod tests { fn transaction_none_fee<'config>( who: Account, is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who, max_fee_per_gas: None, @@ -445,9 +408,7 @@ mod tests { }) } - fn transaction_max_fee_low<'config>( - is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + fn transaction_max_fee_low<'config>(is_transactional: bool) -> CheckEvmTransaction<'config> { test_env(TestCase { who: Account::default(), max_fee_per_gas: Some(U256::from(1u8)), @@ -459,7 +420,7 @@ mod tests { fn transaction_priority_fee_high<'config>( is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who: Account::default(), max_priority_fee_per_gas: Some(U256::from(1_100_000_000)), @@ -468,10 +429,7 @@ mod tests { }) } - fn transaction_max_fee_high<'config>( - who: Account, - tip: bool, - ) -> CheckEvmTransaction<'config, TestError> { + fn transaction_max_fee_high<'config>(who: Account, tip: bool) -> CheckEvmTransaction<'config> { let mut input = TestCase { who, max_fee_per_gas: Some(U256::from(5_000_000_000u128)), @@ -483,7 +441,7 @@ mod tests { test_env(input) } - fn legacy_transaction<'config>(who: Account) -> CheckEvmTransaction<'config, TestError> { + fn legacy_transaction<'config>(who: Account) -> CheckEvmTransaction<'config> { test_env(TestCase { who, gas_price: Some(U256::from(1_000_000_000u128)), @@ -496,7 +454,7 @@ mod tests { fn invalid_transaction_mixed_fees<'config>( who: Account, is_transactional: bool, - ) -> CheckEvmTransaction<'config, TestError> { + ) -> CheckEvmTransaction<'config> { test_env(TestCase { who, gas_price: Some(U256::from(1_000_000_000u128)), @@ -516,7 +474,7 @@ mod tests { }; let test = default_transaction(who, true); // Pool - assert!(test.validate_in_pool_for().is_ok()); + assert!(test.validate_in_pool().is_ok()); // Block assert!(test.validate_in_block().is_ok()); } @@ -530,13 +488,13 @@ mod tests { }; let test = default_transaction(who, true); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::TxNonceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::TxNonceTooLow); // Block let res = test.validate_in_block(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::TxNonceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::TxNonceTooLow); } // Nonce too high succeeds in pool. @@ -547,7 +505,7 @@ mod tests { nonce: U256::from(1u8), }; let test = transaction_nonce_high(who); - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_ok()); } @@ -573,13 +531,13 @@ mod tests { let is_transactional = true; let test = transaction_gas_limit_low(who, is_transactional); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasLimitTooLow); // Block let res = test.validate_in_block(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasLimitTooLow); } // Gas limit too low non-transactional succeeds in pool and in block. @@ -592,7 +550,7 @@ mod tests { let is_transactional = false; let test = transaction_gas_limit_low(who, is_transactional); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_ok()); // Block let res = test.validate_in_block(); @@ -609,13 +567,13 @@ mod tests { let is_transactional = true; let test = transaction_gas_limit_low_proof_size(who, is_transactional); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasLimitTooLow); // Block let res = test.validate_in_block(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasLimitTooLow); } // Gas limit too low non-transactional succeeds in pool and in block. @@ -628,7 +586,7 @@ mod tests { let is_transactional = false; let test = transaction_gas_limit_low_proof_size(who, is_transactional); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_ok()); // Block let res = test.validate_in_block(); @@ -644,13 +602,19 @@ mod tests { }; let test = transaction_gas_limit_high(who); // Pool - let res = test.validate_in_pool_for(); + let res = test.validate_in_pool(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooHigh); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::GasLimitTooHigh + ); // Block let res = test.validate_in_block(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasLimitTooHigh); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::GasLimitTooHigh + ); } // Valid chain id succeeds. @@ -669,7 +633,7 @@ mod tests { let test = transaction_invalid_chain_id(who); let res = test.with_chain_id(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::InvalidChainId); + assert_eq!(res.unwrap_err(), TransactionValidationError::InvalidChainId); } // Valid max fee per gas succeeds. @@ -693,7 +657,10 @@ mod tests { let test = transaction_none_fee(who, true); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::InvalidFeeInput + ); } // Non-transactional call with unset fee data succeeds. @@ -712,12 +679,12 @@ mod tests { let test = transaction_max_fee_low(true); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasPriceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasPriceTooLow); // Non-transactional let test = transaction_max_fee_low(false); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::GasPriceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::GasPriceTooLow); } // Priority fee too high fails. @@ -727,12 +694,18 @@ mod tests { let test = transaction_priority_fee_high(true); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::PriorityFeeTooHigh); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::PriorityFeeTooHigh + ); // Non-transactional let test = transaction_priority_fee_high(false); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::PriorityFeeTooHigh); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::PriorityFeeTooHigh + ); } // Sufficient balance succeeds. @@ -763,12 +736,12 @@ mod tests { let test = default_transaction(who.clone(), true); let res = test.with_balance(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::BalanceTooLow); // Non-transactional let test = default_transaction(who, false); let res = test.with_balance(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::BalanceTooLow); } // Fee not set on transactional fails. @@ -781,7 +754,10 @@ mod tests { let test = transaction_none_fee(who, true); let res = test.with_balance(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::InvalidFeeInput + ); } // Fee not set on non-transactional succeeds. @@ -846,7 +822,7 @@ mod tests { let test = legacy_transaction(who); let res = test.with_balance(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::BalanceTooLow); + assert_eq!(res.unwrap_err(), TransactionValidationError::BalanceTooLow); } // Transaction with invalid fee input - mixing gas_price and max_fee_per_gas. @@ -861,7 +837,10 @@ mod tests { let test = invalid_transaction_mixed_fees(who.clone(), is_transactional); let res = test.with_balance(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::InvalidFeeInput + ); // Succeeds for non-transactional. let is_transactional = false; let test = invalid_transaction_mixed_fees(who, is_transactional); @@ -878,7 +857,10 @@ mod tests { let test = invalid_transaction_mixed_fees(who.clone(), is_transactional); let res = test.with_base_fee(); assert!(res.is_err()); - assert_eq!(res.unwrap_err(), TestError::InvalidFeeInput); + assert_eq!( + res.unwrap_err(), + TransactionValidationError::InvalidFeeInput + ); // Succeeds for non-transactional. let is_transactional = false; let test = invalid_transaction_mixed_fees(who, is_transactional); diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 2bb290ed06..3ab389ce07 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -349,7 +349,7 @@ impl pallet_evm::Config for Runtime { type SuicideQuickClearLimit = SuicideQuickClearLimit; type Timestamp = Timestamp; type WeightInfo = pallet_evm::weights::SubstrateWeight; - type OnCheckEvmTransaction> = (); + type OnCheckEvmTransaction = (); } parameter_types! { From 23b2427c7a9c767af6d67221b94fb50e27a9c558 Mon Sep 17 00:00:00 2001 From: Grigoriy Simonov Date: Tue, 17 Oct 2023 14:09:28 +0000 Subject: [PATCH 3/3] docs: add documentation to `OnCheckEvmTransaction` trait --- frame/evm/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs index 1a484eb69a..ca84f5c033 100644 --- a/frame/evm/src/lib.rs +++ b/frame/evm/src/lib.rs @@ -1094,13 +1094,20 @@ impl OnCreate for Tuple { } } +/// Implements additional EVM transaction validation logic pub trait OnCheckEvmTransaction { + /// Validate EVM transaction. + /// + /// This method should be called before frontier's built-in validations. + /// + /// - `v`: Transaction data to validate. Method can modify transaction data before frontier's built-in validations. fn on_check_evm_transaction( v: &mut CheckEvmTransaction, origin: &H160, ) -> Result<(), TransactionValidationError>; } +/// Implementation for () does not specify any additional validations. impl OnCheckEvmTransaction for () { fn on_check_evm_transaction( _v: &mut CheckEvmTransaction,