[WIP] Extend GasFeeController to poll for network status #609
Conversation
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
mcmire
force-pushed
the
add-network-congestion-gauge
branch
3 times, most recently
from
d02f93f
to
118fda6
Compare
wachunei
reviewed
Sep 29, 2021
mcmire
force-pushed
the
add-network-congestion-gauge
branch
from
118fda6
to
ab9a949
Compare
mcmire
commented
Sep 29, 2021
mcmire
commented
Sep 29, 2021
mcmire
commented
Sep 30, 2021
| const currentAccountIsEIP1559Compatible = | ||
| this.getCurrentAccountEIP1559Compatibility?.() ?? true; | ||
| private async getEIP1559Compatibility(): Promise<boolean> { | ||
| try { |
There was a problem hiding this comment.
This try/catch was copied from _fetchGasFeeEstimateData so that we could use the same behavior in poll.
mcmire
force-pushed
the
add-network-congestion-gauge
branch
from
5867e8f
to
7ce8597
Compare
mcmire
commented
Oct 14, 2021
|
|
The tests for GasFeeController are currently lacking in a few ways: * `nock` is being used to test that requests are being used to obtain data; however, GasFeeController takes functions as arguments which can be used to stub out those requests, thereby removing the need for `nock`. * The logic within `getGasFeeEstimatesAndStartPolling` is not being fully exercised, specifically the polling code. * There are no tests for `disconnectPoller`. * There are no explicit tests for `stopPolling`. * The logic within `_fetchGasFeeEstimateData` is not being fully exercised, specifically with regard to how the result of `getChainId` changes the URL that ends up being hit for both EIP-1559 and non-EIP-1559 flows. * There are tests categorized under a `getChainId` describe block which are actually for `_fetchGasFeeEstimateData`. This commit attempts to address these issues so that future changes to GasFeeController do not break existing behavior.
We would like to introduce another fallback to how we estimate gas fees
in a future commit — namely, we want to use `eth_feeHistory` for
EIP-1559 networks if the usual API is down. To do this, the existing
code around gas fee estimates needs to be refactored. This is the first
in a two-part refactor, extracting the majority of the code in
`GasFeeController._fetchGasFeeEstimateData` to `gas-util`. In addition
to making room for more changes, this also allows us to test this code
in isolation from the polling code that already exists in
GasFeeController.
Note that I am using the term "suggestions" to encompass the data that
`_fetchGasFeeEstimateData` returns. This is because `gasFeeEstimates` is
an intermediate value that can also be packaged with
`estimatedGasFeeTimeBounds`, so "estimates" could be confusing. Also, I
replaced "fetch" with "determine" as in the future we may be performing
calculations that APIs have done for us in the past ("fetch" seems like
a more low-level action).
mcmire
force-pushed
the
add-network-congestion-gauge
branch
from
7ce8597
to
e655c35
Compare
mcmire
changed the base branch from
extract-gas-fee-recommendations
to
add-eth-fee-history-fallback-for-gas-estimates
mcmire
changed the title
mcmire
commented
Oct 30, 2021
| @@ -0,0 +1,28 @@ | |||
| import { BN } from 'ethereumjs-util'; | |||
There was a problem hiding this comment.
See https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/017436f628b2d5967f6e8734780a9114f9e58af9/src/server.ts#L186 and https://gitlab.com/ConsenSys/codefi/products/metaswap/gas-api/-/blob/017436f628b2d5967f6e8734780a9114f9e58af9/src/eip1559/feeHistory.ts#L152 for the original source.
The tests for GasFeeController are currently lacking in a few ways: * `nock` is being used to test that requests are being used to obtain data; however, GasFeeController takes functions as arguments which can be used to stub out those requests, thereby removing the need for `nock`. * The logic within `getGasFeeEstimatesAndStartPolling` is not being fully exercised, specifically the polling code. * There are no tests for `disconnectPoller`. * There are no explicit tests for `stopPolling`. * The logic within `_fetchGasFeeEstimateData` is not being fully exercised, specifically with regard to how the result of `getChainId` changes the URL that ends up being hit for both EIP-1559 and non-EIP-1559 flows. * There are tests categorized under a `getChainId` describe block which are actually for `_fetchGasFeeEstimateData`. This commit attempts to address these issues so that future changes to GasFeeController do not break existing behavior.
mcmire
force-pushed
the
add-eth-fee-history-fallback-for-gas-estimates
branch
from
b864f9e
to
6873f34
Compare
mcmire
force-pushed
the
add-network-congestion-gauge
branch
from
e655c35
to
a741072
Compare
The tests for GasFeeController are currently lacking in a few ways: * `nock` is being used to test that requests are being used to obtain data; however, GasFeeController takes functions as arguments which can be used to stub out those requests, thereby removing the need for `nock`. * The logic within `getGasFeeEstimatesAndStartPolling` is not being fully exercised, specifically the polling code. * There are no tests for `disconnectPoller`. * There are no explicit tests for `stopPolling`. * The logic within `_fetchGasFeeEstimateData` is not being fully exercised, specifically with regard to how the result of `getChainId` changes the URL that ends up being hit for both EIP-1559 and non-EIP-1559 flows. * There are tests categorized under a `getChainId` describe block which are actually for `_fetchGasFeeEstimateData`. This commit attempts to address these issues so that future changes to GasFeeController do not break existing behavior.
mcmire
force-pushed
the
add-eth-fee-history-fallback-for-gas-estimates
branch
from
6873f34
to
709d386
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
mcmire
force-pushed
the
add-eth-fee-history-fallback-for-gas-estimates
branch
from
709d386
to
b4f23df
Compare
When a user is about to send a transaction or is looking at a swap quote, we would like to inform them if the network is busy so that we can push them to use a lower fee setting. GasFeeController already provides a way to poll for fee estimates, which we employ on transaction preview screens. This commit updates the polling code so that we also gauge network status as we pull estimates. This is done by hitting another endpoint in the MetaSwap API which specifically gives us a base fee threshold we can use to determine whether the network is busy (falling back to using `eth_feeHistory` if the API is not available).
mcmire
force-pushed
the
add-network-congestion-gauge
branch
2 times, most recently
from
5e18a39
to
888b63d
Compare
|
This may end up being obsoleted by #632. I'll leave this open until that's merged. |
mcmire
changed the title
mcmire
force-pushed
the
add-eth-fee-history-fallback-for-gas-estimates
branch
4 times, most recently
from
fbbc906
to
46c3788
Compare
Base automatically changed from
add-eth-fee-history-fallback-for-gas-estimates
to
eip-1559-v2
December 2, 2021 16:57
|
Closing since we can now use |
sambacha
added a commit
to sambacha/gas-reporting
that referenced
this pull request
Dec 21, 2021
## network Congestion weight this adds the new field used by metamask for network congestion ### metamask references https://github.com/MetaMask/controllers/pull/632/files MetaMask/core#609 MetaMask/metamask-extension#13044
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When a user is about to send a transaction or is looking at a swap
quote, we would like to inform them if the network is busy so that we
can push them to use a lower fee setting. GasFeeController already
provides a way to poll for fee estimates, which we employ on transaction
preview screens. This commit updates the polling code so that we also
gauge network status as we pull estimates. This is done by hitting
another endpoint in the MetaSwap API which specifically gives us a base
fee threshold we can use to determine whether the network is busy
(falling back to using
eth_feeHistoryif the API is not available).References MetaMask/metamask-extension#11307.