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.

Sign up for GitHub

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

Jump to bottom

fixes maxPriorityFeePerGas in _handleTxPricing #5018

Closed
wants to merge 6 commits into from
Closed

fixes maxPriorityFeePerGas in _handleTxPricing #5018

Show file tree
Hide file tree
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 May 10, 2022
4242abb
updates changelog
mathewmeconry May 10, 2022
09fa3b0
adds missing async flag in _handleTxPricing
mathewmeconry May 10, 2022
4dc61bb
fixes test with new eth_feeHistory request
mathewmeconry May 10, 2022
c7b868d
improves maxPriorityFeePerGas in _handleTxPricing
mathewmeconry May 11, 2022
482786b
fixes variable scope in _handleTxPricing
mathewmeconry May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,10 +548,12 @@ Released with 1.0.0-beta.37 code base.
- Fix static tuple encoding (#4673) (#4884)
- Fix bug in handleRevert logic for eth_sendRawTransaction (#4902)
- Fix resolve type of getBlock function (#4911)
- Fix resolve issue with to low priority fee on polygon (#4857)

### Changed
- Replace deprecated String.prototype.substr() (#4855)
- Exporting AbiCoder as coder (#4937)
- `web3-core-method._handleTxPricing` uses now `eth_feeHistory` for priority fee calculation on EIP-1559 compatible chains (#5018)

### Added
- Exposing `web3.eth.Contract.setProvider()` as per public documentation (#4822) (#5001)
Expand Down
25 changes: 23 additions & 2 deletions packages/web3-core-method/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,11 +861,23 @@ function _handleTxPricing(method, tx) {
call: 'eth_gasPrice',
params: 0
})).createFunction(method.requestManager);
var getFeeHistory = new Method({
name: "getFeeHistory",
call: "eth_feeHistory",
params: 3,
inputFormatter: [
utils.numberToHex,
function (blockNumber) {
return blockNumber ? utils.toHex(blockNumber) : "latest";
},
null,
],
}).createFunction(method.requestManager);

Promise.all([
getBlockByNumber(),
getGasPrice()
]).then(responses => {
]).then(async responses => {
const [block, gasPrice] = responses;
if (
(tx.type === '0x2' || tx.type === undefined) &&
Expand All @@ -883,7 +895,16 @@ function _handleTxPricing(method, tx) {
maxFeePerGas = tx.gasPrice;
delete tx.gasPrice;
} else {
maxPriorityFeePerGas = tx.maxPriorityFeePerGas || '0x9502F900'; // 2.5 Gwei
maxPriorityFeePerGas = tx.maxPriorityFeePerGas || '0x9502F900'; // 2.5 Gwei

// if no priority fee is set in the tx try to get it from the rpc
if(maxPriorityFeePerGas === '0x9502F900') {
const feeHistory = await getFeeHistory(1);
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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 for maxPriorityFeePerGas estimation for users facilitation, and allowing users to use that function for explicitly doing estimation and setting tx.maxPriorityFeePerGas @mathewmeconry ?

Copy link
Author

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....

Copy link
Contributor

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

Copy link
Author

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?

Copy link
Contributor

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.

Copy link
Author

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 the web3-eth module
https://github.com/ChainSafe/web3.js/blob/2c0324af2da467ee1acb72452f20000e916e4306/packages/web3-eth/src/index.js#L426
?

if(feeHistory && feeHistory.baseFeePerGas) {
const [baseFee] = feeHistory.baseFeePerGas;
maxPriorityFeePerGas = utils.numberToHex(utils.hexToNumber(gasPrice) - utils.hexToNumber(baseFee));
}
}
maxFeePerGas = tx.maxFeePerGas ||
utils.toHex(
utils.toBN(block.baseFeePerGas)
Expand Down
4 changes: 4 additions & 0 deletions test/contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2684,6 +2684,10 @@ var runTests = function(contractFactory) {

provider.injectResult('0x45656456456456');

provider.injectValidation(function (payload) {
assert.equal(payload.method, 'eth_feeHistory')
assert.deepEqual(payload.params, ['0x1', 'latest', null])
})

provider.injectValidation(function (payload) {
assert.equal(payload.method, 'eth_sendTransaction');
Expand Down