fixes maxPriorityFeePerGas in _handleTxPricing #5018
Conversation
There was a problem hiding this comment.
@mathewmeconry Thanks for your contributions, could you check and fix as following tests are failing:
2 failing
1) typical usage
with methods
should sendTransaction and fill in default gasPrice:
Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/web3.js/web3.js/test/contract.js)
at listOnTimeout (internal/timers.js:557:17)
at processTimers (internal/timers.js:500:7)
2) standalone usage
with methods
should sendTransaction and fill in default gasPrice:
Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/web3.js/web3.js/test/contract.js)
at listOnTimeout (internal/timers.js:557:17)
at processTimers (internal/timers.js:500:7)
hey @jdevcs |
Pull Request Test Coverage Report for Build 2305841789
💛 - Coveralls |
|
|
||
| // if no priority fee is set in the tx try to get it from the rpc | ||
| if(maxPriorityFeePerGas === '0x9502F900') { | ||
| const feeHistory = await getFeeHistory(1); |
There was a problem hiding this comment.
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.
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.
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.
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 for maxPriorityFeePerGas 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.
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.
@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.
@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.
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.
so I guess we can close this pull request cause the eth_feeHistory is already exported by the web3-eth module
https://github.com/ChainSafe/web3.js/blob/2c0324af2da467ee1acb72452f20000e916e4306/packages/web3-eth/src/index.js#L426
?
|
|
||
| // if no priority fee is set in the tx try to get it from the rpc | ||
| if(maxPriorityFeePerGas === '0x9502F900') { | ||
| const feeHistory = await getFeeHistory(1); |
There was a problem hiding this comment.
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.
Closing this PR in favor of using https://web3js.readthedocs.io/en/v1.7.3/web3-eth.html#getfeehistory for estimation of maxPriorityFeePerGas and setting Priority Fee via tx.maxPriorityFeePerGas explicitly if user dnt want to default 2.5 Gwei.
|
Hej guys! We followed your advice to create a 'local fix' for the issue of tx's failing on Polygon with a fixed 2.5 gwei priorityFee. Here's the PR of @mathewmeconry: aragon/client#1753 Thanks for your input and feedback, and looking forward to 4.0! |
Description
web3-core-method._handleTxPricinguses noweth_feeHistoryfor priority fee calculation on EIP-1559 compatible chainsI have no idea to get the
Deploy Previewto check if it looks correct...Fixes #4857
Type of change
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:covand my test cases cover all the lines and branches of the added code.npm run buildwith success.dist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.