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

set maxFeePerGas if not provided #2055

Merged
merged 2 commits into from
Jun 30, 2021
Merged

Conversation

wolovim
Copy link
Member

@wolovim wolovim commented Jun 29, 2021

What was wrong?

One of the 1559 TODOs was to determine an appropriate maxFeePerGas if one was not provided, but a maxPriorityFeePerGas was. (Related to #2054)

How was it fixed?

This follows Geth's behavior to set the maxFeePerGas to maxPriorityFeePerGas + 2*baseFee.

Todo:

Cute Animal Picture

@wolovim wolovim requested review from fselmo and kclowes June 29, 2021 17:19
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.

Not a blocker to merging, but was curious about how we might be able to narrow down the except Exception line, or if that is the best option there.

max_fee_per_gas = priority_fee + 2 * base_fee
transaction = assoc(transaction, 'maxFeePerGas', hex(max_fee_per_gas))
return make_request(method, [transaction])
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would we be unable to calculate maxFeePerGas? This except Exception seems pretty broad - I wonder if we could narrow it down some?

priority_fee = int(transaction['maxPriorityFeePerGas'], 16)
max_fee_per_gas = priority_fee + 2 * base_fee
transaction = assoc(transaction, 'maxFeePerGas', hex(max_fee_per_gas))
return make_request(method, [transaction])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@marcgarreau If we fail at the make_request call, we will have already added the maxFeePerGas field to the transaction, based on the line:

transaction = assoc(transaction, 'maxFeePerGas', hex(max_fee_per_gas))

We should probably make sure we've removed that field [the maxPriorityFeePerGas] at the except block, otherwise we will ultimately be making the same call that we did in the try block.

We can have a test case for this with an alphabetical string as the value for maxPriorityFeePerGas to make sure the try block fails. This test should fail... but if we add transaction.pop('maxPriorityFeePerGas') in the except block, it should pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correction on the above... I don't think we can actually catch any exception from the make_request call since it feeds back to this method and it will now have both params.

If we fail, it will be because the maxPriorityFeePerGas is invalid and therefore that is what we should pop in the except block. I will edit the above comment.

Copy link

Choose a reason for hiding this comment

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

So by just putting the 2 wei on the max priority fee solves it?

Is this really optimal solution?

@fselmo fselmo merged commit 03be8a3 into ethereum:london Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants