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: hexidecimal number for eth_feehistory blockcount #25

Merged
merged 1 commit into from
Nov 10, 2022
Merged

fix: hexidecimal number for eth_feehistory blockcount #25

merged 1 commit into from
Nov 10, 2022

Conversation

alexandre-abrioux
Copy link
Contributor

@alexandre-abrioux alexandre-abrioux commented Feb 8, 2022

Hi!

First of all, thank you for creating this tool!
Its really great and has been working like a charm in projects that I've included it in.

However I had an issue running it against Quicknode's RPC servers.
You can test it with this simple demo.ts:

import { JsonRpcProvider } from '@ethersproject/providers';
import { suggestFees } from './src';

const main = async () => {
  try {
    const provider = new JsonRpcProvider("QUICKNODE_URL");
    const fee = await suggestFees(provider);
    // eslint-disable-next-line no-console
    console.log(fee);
  } catch (e) {
    // eslint-disable-next-line no-console
    console.error(e);
  }
};
main();

You will get this server error:

{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params: invalid type: integer `100`, expected a 0x-prefixed hex string with length between (0; 64]."},"id":42}

It looks like Infura and Alchemy both allow the use of an integer for the blockCount argument of eth_feeHistory.
But the doc actually specifies that it should be an hexadecimal number:
https://infura.io/docs/ethereum#operation/eth_feeHistory

This change fixes it by using ethers' hexadecimal utils.

@alexandre-abrioux
Copy link
Contributor Author

alexandre-abrioux commented Feb 14, 2022

PS: Here is the link of the official spec, described in the ethereum/execution-apis repository. You will find that blockCount should satisfy this regex: ^0x([1-9a-f]+[0-9a-f]*|0)$.

@welps welps requested a review from estebanmino March 5, 2022 14:56
@alexandre-abrioux
Copy link
Contributor Author

Hi @estebanmino! Any change of this being merged? It would be greatly appreciated! Thanks 🙂

@alexandre-abrioux
Copy link
Contributor Author

alexandre-abrioux commented Oct 5, 2022

Updates

1/ I released a branch in my fork that contains the (fixed) compiled JS files and that you can directly use in your packages.json:

"eip1559-fee-suggestions-ethers": "git+https://github.com/alexandre-abrioux/fee-suggestions#hex-feehistory-dist"

2/ Quicknode now supports integers for the blockCount argument of eth_feeHistory, so this specific provider is not impacted by the issue anymore. However, it would be great if this could still be merged, as the current code does not work with RPCs that strictly follow the specs (hexadecimals only).

@welps welps merged commit 980e6a6 into rainbow-me:main Nov 10, 2022
@alexandre-abrioux
Copy link
Contributor Author

Thank you very much @welps for the review and the merge! Do you think it could be released as a minor version for the npm package?

@welps
Copy link
Member

welps commented Nov 14, 2022

Thank you very much @welps for the review and the merge! Do you think it could be released as a minor version for the npm package?

@alexandre-abrioux Yes, I don't have access to do this so I'm gonna be looking to get someone to do it this week

@alexandre-abrioux
Copy link
Contributor Author

Thank you @welps 🙂

@welps
Copy link
Member

welps commented Nov 17, 2022

@alexandre-abrioux Likely will release next week, I understand why #23 is happening and am trying to figure out a good solution so I can fit in a fix.

@alexandre-abrioux
Copy link
Contributor Author

Great! Thanks again

@welps
Copy link
Member

welps commented Nov 22, 2022

@alexandre-abrioux This has been released under 2.1.0 on npm

In regard to #23, see #23 (comment)

@alexandre-abrioux alexandre-abrioux deleted the hex-feehistory branch March 10, 2023 11:19
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.

2 participants