-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Use eth_feeHistory as a fallback for gas estimates #614
Use eth_feeHistory as a fallback for gas estimates #614
Conversation
@@ -20,50 +18,6 @@ const GAS_PRICE = 'gasPrice'; | |||
const FAIL = 'lol'; | |||
const PASS = '0x1'; | |||
|
|||
const mockFlags: { [key: string]: any } = { |
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 am not sure why these were here. I assume that they existed once to test out the query
function, but we don't actually need a real EthQuery object to test that.
a886e92
to
cbcf79f
Compare
@@ -69,7 +75,12 @@ export default async function determineGasFeeSuggestions({ | |||
|
|||
if (isEIP1559Compatible) { | |||
strategies.push(async () => { | |||
const estimates = await fetchGasEstimates(fetchGasEstimatesUrl, clientId); | |||
let estimates: GasFeeEstimates; |
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.
@danjm Is this what you had in mind? i.e., do we still want to use eth_feeHistory
as a fallback for the EIP-1559 Metaswap API, and then continue to use eth_gasPrice
as an ultimate fallback whether EIP-1559 or not?
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, this looks right
if (typeof ethQuery[method] === 'function') { | ||
ethQuery[method](...args, cb); | ||
} else { | ||
ethQuery.sendAsync({ method, params: args }, cb); |
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.
Is this need because ethQuery
doesn't yet have explicit support for eth_feeHistory
?
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, exactly. web3.js
does, but eth-query
does not. A bit annoying :/
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 suppose we could just use sendAsync
directly in this case without "patching" eth-query
, but eth-query
's sendAsync
doesn't return a promise, it only accepts a callback, which is another thing that this query
function tries to paper over.
e7359cd
to
5cef2af
Compare
4d5a23b
to
ecac47e
Compare
5cef2af
to
883b2fe
Compare
Will update this with some new changes today, stay tuned. |
6873f34
to
709d386
Compare
709d386
to
b4f23df
Compare
@@ -76,6 +76,7 @@ | |||
"@metamask/eslint-config-nodejs": "^9.0.0", | |||
"@metamask/eslint-config-typescript": "^9.0.1", | |||
"@types/jest": "^26.0.22", | |||
"@types/jest-when": "^2.7.3", |
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.
jest-when
lets you make assertions on different invocations of mock functions (e.g. let the return value be Y only when X is given). See the README for more: https://www.npmjs.com/package/jest-when
? await query(ethQuery, 'blockNumber') | ||
: givenEndBlock; | ||
|
||
const requestChunkSpecifiers = determineRequestChunkSpecifiers( |
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.
In case the numberOfBlocks
is more than 1024, this function will make multiple calls to eth_feeHistory
in order to gather all of the desired blocks. This comes in handy later when we want to provide a fallback to Sam's "busy threshold" endpoint, which requires fetching 20,000 blocks.
type Percentile = typeof PRIORITY_LEVEL_PERCENTILES[number]; | ||
|
||
// This code is translated from the MetaSwap API: | ||
// <https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/017436f628b2d5967f6e8734780a9114f9e58af9/src/eip1559/feeHistory.ts> |
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.
Note that this file is not a direct translation of the MetaSwap API. I rearranged the code because it made more sense to me and was easier to follow when presented this way (also, fetchBlockFeeHistory
returns data in a different format than eth_feeHistory
).
This is now ready for review. |
fe524df
to
809566b
Compare
6c94a80
to
47e0fa0
Compare
47e0fa0
to
fbbc906
Compare
If we are on an EIP-1559-supported network and the Metaswap API fails for some reason, fall back to using `eth_feeHistory` to calculate gas estimates (which the API uses anyway). This code is more or less taken from the code for the API ([1]). [1]: https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/eae6927b1a0c445e02cb3cba9e9e6b0f35857a12/src/eip1559/feeHistory.ts
fbbc906
to
46c3788
Compare
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.
👍 great work
If we are on an EIP-1559-supported network and the Metaswap API fails for some reason, fall back to using `eth_feeHistory` to calculate gas estimates (which the API uses anyway). This code is more or less taken from the code for the API ([1]). [1]: https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/eae6927b1a0c445e02cb3cba9e9e6b0f35857a12/src/eip1559/feeHistory.ts
If we are on an EIP-1559-supported network and the Metaswap API fails for some reason, fall back to using `eth_feeHistory` to calculate gas estimates (which the API uses anyway). This code is more or less taken from the code for the API ([1]). [1]: https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/eae6927b1a0c445e02cb3cba9e9e6b0f35857a12/src/eip1559/feeHistory.ts
If we are on an EIP-1559-supported network and the Metaswap API fails for some reason, fall back to using `eth_feeHistory` to calculate gas estimates (which the API uses anyway). This code is more or less taken from the code for the API ([1]). [1]: https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/eae6927b1a0c445e02cb3cba9e9e6b0f35857a12/src/eip1559/feeHistory.ts
If we are on an EIP-1559-supported network and the Metaswap API fails
for some reason, fall back to using
eth_feeHistory
to calculate gasestimates (which the API uses anyway). This code is more or less taken
from the code for the API (1).
Fixes #600.