From 86bee379ced817fdb9d8adcc8f128041194440c9 Mon Sep 17 00:00:00 2001 From: Felipe Selmo Date: Mon, 18 Oct 2021 20:48:28 -0600 Subject: [PATCH] more refactoring before next team review --- eth_tester/backends/mock/factory.py | 49 +++++++++++++---------------- eth_tester/backends/pyevm/main.py | 17 +++++----- eth_tester/utils/backend_testing.py | 5 ++- eth_tester/validation/common.py | 2 +- eth_tester/validation/inbound.py | 30 ++++++++++-------- 5 files changed, 51 insertions(+), 52 deletions(-) diff --git a/eth_tester/backends/mock/factory.py b/eth_tester/backends/mock/factory.py index 2fad3897..ecd31589 100644 --- a/eth_tester/backends/mock/factory.py +++ b/eth_tester/backends/mock/factory.py @@ -91,52 +91,45 @@ def _fill_transaction(transaction, block, transaction_index, is_pending, overrid if overrides is None: overrides = {} - is_dynamic_fee_transaction = ( - 'gas_price' not in transaction and any( - _ in transaction for _ in ('max_fee_per_gas', 'max_priority_fee_per_gas') - ) - ) - - if 'hash' in overrides: + if 'hash' in overrides: # else calculate hash after all fields are filled yield 'hash', overrides['hash'] - else: - # calculate hash after all fields are filled - pass - # this pattern isn't the nicest to read but it keeps things clean. Here, we yield the overrides - # value if it exists, else either the transaction value if that exists, or a default value + # Here, we yield the key with the overrides value if it exists, else either the transaction + # value if it exists or a default value yield 'nonce', overrides.get('nonce', 0) yield 'from', overrides.get('from', transaction.get('from')) yield 'to', overrides.get('to', transaction.get('to', b'')) yield 'data', overrides.get('data', transaction.get('data', b'')) yield 'value', overrides.get('value', transaction.get('value', 0)) yield 'gas', overrides.get('gas', transaction.get('gas')) + yield 'r', overrides.get('r', transaction.get('r', 12345)) + yield 's', overrides.get('s', transaction.get('s', 67890)) - if 'gas_price' in transaction or 'gas_price' in overrides: - # gas price is 1 gwei in order to be at least the base fee at the (London) genesis block - yield 'gas_price', overrides.get('gas_price', transaction.get('gas_price', 1000000000)) - else: - # if no gas_price specified, default to dynamic fee txn parameters - is_dynamic_fee_transaction = True - - if is_dynamic_fee_transaction: + if 'gas_price' not in transaction: + # dynamic fee transaction (type = 2) yield 'max_fee_per_gas', overrides.get( 'max_fee_per_gas', transaction.get('max_fee_per_gas', 1000000000) ) yield 'max_priority_fee_per_gas', overrides.get( 'max_priority_fee_per_gas', transaction.get('max_priority_fee_per_gas', 1000000000) ) + yield from _yield_typed_transaction_fields(overrides, transaction) - if is_dynamic_fee_transaction or 'access_list' in transaction: - # if is typed transaction (dynamic fee or access list transaction) - yield 'chain_id', overrides.get('chain_id', transaction.get('chain_id', 131277322940537)) - yield 'access_list', overrides.get('access_list', transaction.get('access_list', [])) - yield 'y_parity', overrides.get('y_parity', transaction.get('y_parity', 0)) else: - yield 'v', overrides.get('v', transaction.get('v', 27)) + yield 'gas_price', overrides.get('gas_price', transaction.get('gas_price')) + if 'access_list' in transaction: + # access list transaction (type = 1) + yield from _yield_typed_transaction_fields(overrides, transaction) - yield 'r', overrides.get('r', transaction.get('r', 12345)) - yield 's', overrides.get('s', transaction.get('s', 67890)) + else: + # legacy transaction + yield 'v', overrides.get('v', transaction.get('v', 27)) + + +def _yield_typed_transaction_fields(overrides, transaction): + yield 'chain_id', overrides.get('chain_id', transaction.get('chain_id', 131277322940537)) + yield 'access_list', overrides.get('access_list', transaction.get('access_list', [])) + yield 'y_parity', overrides.get('y_parity', transaction.get('y_parity', 0)) @to_dict diff --git a/eth_tester/backends/pyevm/main.py b/eth_tester/backends/pyevm/main.py index f15c7700..be9b95c1 100644 --- a/eth_tester/backends/pyevm/main.py +++ b/eth_tester/backends/pyevm/main.py @@ -438,7 +438,8 @@ def get_base_fee(self, block_number='latest'): # @to_dict def _normalize_transaction(self, transaction, block_number='latest'): - is_dynamic_fee_transaction = False + is_dynamic_fee_transaction = 'gas_price' not in transaction # default to dynamic fee txn + for key in transaction: if key == 'from': continue @@ -447,19 +448,21 @@ def _normalize_transaction(self, transaction, block_number='latest'): yield 'nonce', self.get_nonce(transaction['from'], block_number) if 'data' not in transaction: yield 'data', b'' - if 'gas_price' not in transaction: - is_dynamic_fee_transaction = True + if 'value' not in transaction: + yield 'value', 0 + if 'to' not in transaction: + yield 'to', b'' + + if is_dynamic_fee_transaction: if not any(_ in transaction for _ in ('max_fee_per_gas', 'max_priority_fee_per_gas')): yield 'max_fee_per_gas', 1 * 10**9 yield 'max_priority_fee_per_gas', 1 * 10**9 elif 'max_priority_fee_per_gas' in transaction and 'max_fee_per_gas' not in transaction: base_fee = self.get_base_fee(block_number) yield 'max_fee_per_gas', transaction['max_priority_fee_per_gas'] + 2 * base_fee - if 'value' not in transaction: - yield 'value', 0 - if 'to' not in transaction: - yield 'to', b'' + if is_dynamic_fee_transaction or 'access_list' in transaction: + # typed transaction if 'access_list' not in transaction: yield 'access_list', [] if 'chain_id' not in transaction: diff --git a/eth_tester/utils/backend_testing.py b/eth_tester/utils/backend_testing.py index 095dbb4e..a650f48a 100644 --- a/eth_tester/utils/backend_testing.py +++ b/eth_tester/utils/backend_testing.py @@ -348,7 +348,7 @@ def test_send_access_list_transaction(self, eth_tester): } self._send_and_check_transaction(eth_tester, access_list_transaction, accounts[0]) - # with access list + # with non-empty access list access_list_transaction['access_list'] = ( { 'address': '0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae', @@ -376,11 +376,10 @@ def test_send_dynamic_fee_transaction(self, eth_tester): 'gas': 40000, 'max_fee_per_gas': 2000000000, 'max_priority_fee_per_gas': 1000000000, - 'access_list': [], } self._send_and_check_transaction(eth_tester, dynamic_fee_transaction, accounts[0]) - # with access list + # with non-empty access list dynamic_fee_transaction['access_list'] = ( { 'address': '0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae', diff --git a/eth_tester/validation/common.py b/eth_tester/validation/common.py index f7673061..266d02dd 100644 --- a/eth_tester/validation/common.py +++ b/eth_tester/validation/common.py @@ -34,7 +34,7 @@ def validate_positive_integer(value): error_message = "Value must be positive integers. Got: {}".format( value, ) - if not is_integer(value) or is_boolean(value): + if not is_integer(value): raise ValidationError(error_message) elif value < 0: raise ValidationError(error_message) diff --git a/eth_tester/validation/inbound.py b/eth_tester/validation/inbound.py index a4d3a31a..f4b14b85 100644 --- a/eth_tester/validation/inbound.py +++ b/eth_tester/validation/inbound.py @@ -147,8 +147,6 @@ def validate_private_key(value): TRANSACTION_KEYS = { - # note that the transaction 'type' is not included, though it is a valid param, because it will - # never be passed to the backend and thus is not necessary. 'chain_id', 'from', 'to', @@ -165,8 +163,8 @@ def validate_private_key(value): SIGNED_TRANSACTION_KEYS = { 'r', 's', - 'v', - 'y_parity', + 'v', # legacy transactions only + 'y_parity', # typed transactions only, instead of 'v' } TRANSACTION_INTERNAL_TYPE_INFO = { @@ -179,6 +177,12 @@ def validate_private_key(value): ALLOWED_TRANSACTION_INTERNAL_TYPES = set(TRANSACTION_INTERNAL_TYPE_INFO.keys()) +def _is_typed_transaction(txn_dict: dict) -> bool: + return any(_ in txn_dict.keys() for _ in ( + 'max_fee_per_gas', 'max_priority_fee_per_gas', 'access_list' + )) + + def validate_transaction(value, txn_internal_type): if txn_internal_type not in ALLOWED_TRANSACTION_INTERNAL_TYPES: raise TypeError("the `txn_internal_type` parameter must be one of send/call/estimate") @@ -201,12 +205,11 @@ def validate_transaction(value, txn_internal_type): elif txn_internal_type == 'send_signed': signed_transaction_keys = SIGNED_TRANSACTION_KEYS.copy() - if any(_ in value.keys() for _ in ( - 'max_fee_per_gas', 'max_priority_fee_per_gas', 'access_list' - )): - signed_transaction_keys.remove('v') # typed txn + if _is_typed_transaction(value): + signed_transaction_keys.remove('v') else: - signed_transaction_keys.remove('y_parity') # legacy txn + # legacy transaction + signed_transaction_keys.remove('y_parity') required_keys = {'from', 'gas'} | signed_transaction_keys elif txn_internal_type in {'estimate', 'call'}: @@ -272,10 +275,11 @@ def validate_transaction(value, txn_internal_type): if txn_internal_type == 'send_signed': validate_uint256(value['r']) validate_uint256(value['s']) - try: - validate_uint8(value['v']) # legacy transaction - except KeyError: - validate_uint8(value['y_parity']) # typed transaction + if _is_typed_transaction(value): + validate_uint8(value['y_parity']) + else: + # legacy transaction + validate_uint8(value['v']) def _validate_inbound_access_list(access_list):