Skip to content

Ensure swaps gas prices are fetched from the correct chain specific endpoint#10744

Merged
danjm merged 3 commits intodevelopfrom
swaps-gas-prices-non-mainnet
Mar 28, 2021
Merged

Ensure swaps gas prices are fetched from the correct chain specific endpoint#10744
danjm merged 3 commits intodevelopfrom
swaps-gas-prices-non-mainnet

Conversation

@danjm
Copy link
Contributor

@danjm danjm commented Mar 26, 2021

Fixes an issue found by @tmashuang when QAing v9.3.0

The chainId was not being passed to the fetchSwapsGasPrices function, resulting in the incorrect gas prices being fetched.

@danjm danjm requested a review from a team as a code owner March 26, 2021 22:41
@danjm danjm requested a review from brad-decker March 26, 2021 22:41
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

priceEstimates = await fetchSwapsGasPrices(chainId);
} else {
const cachedPriceEstimates = await getStorageItem(
'METASWAP_GAS_PRICE_ESTIMATES',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cache isn't chainId-aware either. It needs to be, otherwise cached values from the wrong chain will end up being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was chainId aware, to the extent that a different url will be used on a different chain. fetchWithCache keys the local storage cache by const cacheKey = cachedFetch:${url};, so a different url's for different chains would get different values from the cache, or trigger fresh requests if no key matching the url is already in the cache.

Copy link
Member

@Gudahtt Gudahtt Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but look at this line though. getStorageItem is only being passed METASWAP_GAS_PRICE_ESTIMATES. No URL, no chainId.

This wasn't cached by ChainID before because it used to only be used for one.

The METASWAP_GAS_PRICE_ESTIMATES_LAST_RETRIEVED entry has the same issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the best solution here would be to get rid of METASWAP_GAS_PRICE_ESTIMATES and METASWAP_GAS_PRICE_ESTIMATES_LAST_RETRIEVED, and rely upon fetchWithCache for our cache.

That would require passing through the desired cache interval to fetchSwapsGasPrices, or changing it to be 30000`, if that's what's desired (that's what we're using now).

@metamaskbot
Copy link
Collaborator

Builds ready [23e0ca4]
Page Load Metrics (606 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded39681260412058
load39781760612158
domInteractive39581260412058

…ectly using storage in getSwapsPriceEstimatesLastRetrieved
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [23e0ca4]
Page Load Metrics (540 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint437851105
domContentLoaded3376215388139
load3436225408039
domInteractive3366215388139

@metamaskbot
Copy link
Collaborator

Builds ready [9996f7a]
Page Load Metrics (1004 ± 185 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44230894220
domContentLoaded36816731003385185
load36916741004386185
domInteractive36816721002385185

@danjm danjm merged commit cc19d25 into develop Mar 28, 2021
@danjm danjm deleted the swaps-gas-prices-non-mainnet branch March 28, 2021 13:18
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants