-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: eip-1559 fixed gas prices #925
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Pull Request Test Coverage Report for Build 5927002806
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a test from "view" perspective – if a a fixed1559
gas price is set, how is it rendered in the response.
src/chains/models.py
Outdated
exactly_one_variant = reduce( | ||
xor, [fixed_wei_defined, fixed1559_defined, oracle_defined] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work as it is since True XOR True XOR True
becomes False XOR True
which becomes True
and that won't match with "exactly one variant".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like:
exactly_one_variant = [fixed_wei_defined, fixed1559_defined, oracle_defined].count(True) == 1
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops I missed that one. Your suggestions sounds good. I will change it to that :)
src/chains/models.py
Outdated
"oracle_uri": "An oracle uri, fixed gas price or maxFeePerGas and maxPriorityFeePerGas should be \ | ||
provided (but not multiple)", | ||
"fixed_wei_value": "An oracle uri, fixed gas price or maxFeePerGas and maxPriorityFeePerGas \ | ||
should be provided (but not multiple)", | ||
"max_fee_per_gas": "An oracle uri, fixed gas price or maxFeePerGas and maxPriorityFeePerGas \ | ||
should be provided (but not multiple)", | ||
"max_priority_fee_per_gas": "An oracle uri, fixed gas price or maxFeePerGas and \ | ||
maxPriorityFeePerGas should be provided (but not multiple)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: the error detail can be extracted to a variable and referenced (seems to be the same for very field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
What this PR resolves
Resolves #924
How it resolves it
GasPrice
model:maxFeePerGas
(in Wei)maxPriorityFeePerGas
(in Wei)