-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fixes maxPriorityFeePerGas in _handleTxPricing #5018
Closed
mathewmeconry
wants to merge
6
commits into
web3:1.x
from
mathewmeconry:fix/priority_fee_calculation
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f0c6085
fixes maxPriorityFeePerGas in _handleTxPricing
mathewmeconry 4242abb
updates changelog
mathewmeconry 09fa3b0
adds missing async flag in _handleTxPricing
mathewmeconry 4dc61bb
fixes test with new eth_feeHistory request
mathewmeconry c7b868d
improves maxPriorityFeePerGas in _handleTxPricing
mathewmeconry 482786b
fixes variable scope in _handleTxPricing
mathewmeconry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There can be optimized maxPriorityFeePerGas calculation, and users can always calculate and set it explicitly via
tx.maxPriorityFeePerGas
. We always defaults to 2.5 Gwei if user is not setting it.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 would second @jdevcs comment, we should not make these changes. We can develop a feature to inject custom fee calculation algorithm to the library but in some 4.x releases.
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.
personally, I think the hardcoded value of 2.5 gwei is a mistake because the chains differ and as we see on polygon mainnet the transactions fail because of underpricing.
The same thing would from my point of view also apply to the gas calculation which could be done by the user and set with
maxFeePerGas
but this is done in this library to make the usage easier.If you don't agree then please close the pull request and we make the change for all of our clients
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.
As there can be several ways of estimation of
maxPriorityFeePerGas
based on how fast user want his tx to include in next block. So instead of defaulting to single way of estimation in this function, another path could be adding a standalone function formaxPriorityFeePerGas
estimation for users facilitation, and allowing users to use that function for explicitly doing estimation and setting tx.maxPriorityFeePerGas @mathewmeconry ?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.
not a bad idea and would make total sense.
but as I have seen in the code so far there is also only one way implemented for the gas estimation and this one also defines how "fast" a transaction goes through.
I just try to understand why the code change isn't ok for the priority fee but doing the same with the gas estimation is fine....
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.
@mathewmeconry our team is occupied for 4.x work so if you have time could you go ahead and add maxPriorityFeePerGas estimation function? Thanks
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.
@jdevcs sure. in which package should this function live?
so I am gonna remove this improvement again and only add a function to return the estimated priority fee?
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.
Yes,
web3.eth
package, once its added in 1.x could you also add this in 4.x for future use.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.
so I guess we can close this pull request cause the
eth_feeHistory
is already exported by theweb3-eth
modulehttps://github.com/ChainSafe/web3.js/blob/2c0324af2da467ee1acb72452f20000e916e4306/packages/web3-eth/src/index.js#L426
?