Update geth test fixture to v1.10.8 + some refactoring #2118
Conversation
fselmo
force-pushed
the
geth-fixture-bump-1.10.8
branch
5 times, most recently
from
b19f73c
to
db0287e
Compare
web3/_utils/transactions.py
Outdated
| if all(_ is not None for _ in ( | ||
| extracted_params.get('maxFeePerGas'), extracted_params.get('maxPriorityFeePerGas')) | ||
| ): |
There was a problem hiding this comment.
This is bending my brain a little 😆 maybe worth pulling out into it's own function or assigning to a variable?
There was a problem hiding this comment.
agreed... I split it into two if statements rather than just one to try to clarify it but I still wasn't happy with how this looks. Let me see if I can make this readable 🤔
- Minor fix on formatting in contracts.rst docs
Geth now adds a gasPrice to dynamic fee transactions. The value for gasPrice that is set is equal to the maxFeePerGas value. Changes were made to modify_transaction since we build a new transaction based on values from the original, pending transaction. This transaction would now have all 3 fields and eth_sendTransaction would fail. We need to pop this value back out before replacing the transaction if it is equal to the expected value (maxFeePerGas).
fselmo
force-pushed
the
geth-fixture-bump-1.10.8
branch
from
db0287e
to
0ba2aab
Compare
fselmo
changed the title
fselmo
changed the title
fselmo
force-pushed
the
geth-fixture-bump-1.10.8
branch
3 times, most recently
from
5b0199d
to
d524aa2
Compare
|
@kclowes As I mentioned I kind of went far in refactoring some things but I wanted to cut through most of the noise by highlighting the most important changes:
The rest should all be clarifying name changes for variables in the |
fselmo
force-pushed
the
geth-fixture-bump-1.10.8
branch
from
d524aa2
to
d6a2d4c
Compare
Added utility helper functions to look for certain values among dict-like objects to improve readability among the code base. Added the DYNAMIC_FEE_TXN_PARAMS constant since they are increasingly being used across our code base for validation. Did some general refactoring along the way and renamed any EIP-1559 transaction usage to dynamic fee transaction to keep the naming consistency across code bases since this seems to be the adopted name for the new type=2 transactions. This did not include messages within exceptions since this would be a breaking change.
fselmo
force-pushed
the
geth-fixture-bump-1.10.8
branch
from
d6a2d4c
to
b22bc1f
Compare
| if key == 'gasPrice' and any(_ in transaction for _ in ( | ||
| 'maxFeePerGas', 'maxPriorityFeePerGas' | ||
| )): # if EIP-1559 params in transaction, do not set a default gasPrice if missing | ||
| if key == 'gasPrice' and any_in_dict(DYNAMIC_FEE_TXN_PARAMS, transaction): |
There was a problem hiding this comment.
This is much better 😆 Nice!
| @@ -26,40 +34,38 @@ | |||
|
|
|||
|
|
|||
| def validate_transaction_params( | |||
| transaction: TxParams, latest_block: BlockData, generated_gas_price: Wei | |||
| transaction: TxParams, latest_block: BlockData, strategy_based_gas_price: Wei | |||
There was a problem hiding this comment.
In this case it doesn't matter because I believe this is just an internal function that gets called by the middleware below, but... since python allows users to pass in arguments either positionally or by name, changing the names of the parameters can be a breaking change if it's part of the public API. Just something to watch out for :)
What was wrong?
How was it fixed?
gasPriceto pending dynamic fee transactions that is equal to the value of the providedmaxFeePerGas. This broke some tests that needed to be fixed as well as the validation formodify_transactionsince we pull the params from the pending transaction and that now included agasPriceTodo:
Cute Animal Picture