Skip to content
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

fix: eip1559 validation incorrectly adds gas price field #2099

Merged
merged 1 commit into from Aug 10, 2021

Conversation

banteg
Copy link
Contributor

@banteg banteg commented Aug 7, 2021

What was wrong?

When passing a correct EIP-1559 transaction with:

{'maxFeePerGas': '0x174876e800', 'maxPriorityFeePerGas': '0x77359400', 'type': '0x2'}

validate_transaction_params appends gasPrice to it, which causes the subsequent check for mixed transaction type to fail.

{'maxFeePerGas': '0x174876e800', 'maxPriorityFeePerGas': '0x77359400', 'type': '0x2', 'gasPrice': '0x6977bbb6'}

How was it fixed?

Expanded the condition to skip adding gasPrice when maxPriorityFeePerGas key is present.

Todo:

if (
generated_gas_price is not None
and 'gasPrice' not in transaction
and 'maxPriorityFeePerGas' not in transaction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this addition... so when we have a gas price strategy set but want to send an EIP-1559 transaction, we can still do so. I think I'd like to expand this to something like:

Suggested change
and 'maxPriorityFeePerGas' not in transaction
and all(_ not in transaction for _ in ('maxFeePerGas', 'maxPriorityFeePerGas'))

This way, if only maxFeePerGas is specified, we end up with a better message on why the exception was thrown. It won't be confusing that a gas price was added and conflicted, instead it will be that the maxPriorityFeePerGas needs to be explicit for an EIP-1559 transaction.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also am going to encourage adding a test to eth_module for this. If you'd like I can throw one in on top of this change. Let me know.

@@ -0,0 +1 @@
Validation function was incorrectly adding `gasPrice` field to EIP-1559 transactions, which made them fail the mixed transaction type check.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add here that this was only when a gas price strategy is explicitly set

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This newsfragment also needs to be in .rst format. This is why the build failed.

@fselmo fselmo force-pushed the fix/eip1559-validation branch 2 times, most recently from 4abd314 to a4559a4 Compare August 9, 2021 21:32
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fselmo LGTM once the error variants of the test are split out like we talked about! 🚢

@fselmo fselmo force-pushed the fix/eip1559-validation branch 3 times, most recently from 4f3a7fd to 18f6ba8 Compare August 9, 2021 22:38
@fselmo fselmo merged commit 7f53f5d into ethereum:master Aug 10, 2021
kanban [experimental] automation moved this from In progress to Done Aug 10, 2021
@fselmo
Copy link
Collaborator

fselmo commented Aug 10, 2021

@banteg thanks for the PR. I went ahead and added the tests I mentioned and merged so we can get this out in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants