Fix #10081 - Use fetchWithCache to retrieve and store basic gas estim…#10384
Fix #10081 - Use fetchWithCache to retrieve and store basic gas estim…#10384darkwing merged 1 commit intoMetaMask:developfrom
Conversation
|
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. |
f7354e7 to
879454c
Compare
879454c to
8e9dfa1
Compare
67263d7 to
a0fad33
Compare
app/scripts/migrations/052.js
Outdated
There was a problem hiding this comment.
I tried testing this locally, and it seems as though this call is stalling, not failing. It hangs forever. It happens with the benchmark and the metric tests because it only affects the first pageload. When you refresh, it's fine.
I found some localForage issues that talk about it hanging when trying to open an existing IndexedDB database in a web worker... maybe something similar is happening here? This is the first time we've used this API in the background. It's only been used in the front-end up to now. 🤔
There was a problem hiding this comment.
One solution would be to not await this call. Just keep going. That would get past this problem, but it seems a bit... sketchy 😬 . Is it possible it would mess with future IndexedDB calls? Who knows.
Maybe we can remove the migration altogether? Having junk data leftover is less bad than freezing the page load. Not ideal, but less bad.
There was a problem hiding this comment.
Or we could just remove these from local storage, and add a comment to remind us to find a way to remove it from local storage later at some point. Maybe we'll need front-end migrations 🤔
There was a problem hiding this comment.
Made an update to see if a value check before attempting to remove will help.
There was a problem hiding this comment.
It was freezing when opening the database, so I'd be surprised if that helped.
614481d to
b0c16bd
Compare
…are now minimal and moved corresponding code to fetchEthGasPriceEstimates
…are now minimal and moved corresponding code to fetchEthGasPriceEstimates
…are now minimal and moved corresponding code to fetchEthGasPriceEstimates
…are now minimal and moved corresponding code to fetchEthGasPriceEstimates
|
It appears fab22ee makes this PR no longer viable, as we're specific to chains now, so I'll close. |
|
I don't believe fab22ee makes this PR any less viable. In fact it was updated specifically to reduce conflicts with this PR. Custom networks won't benefit from this, as we're not using our gas estimation API for those. But these changes remain just as applicable for Mainnet. |
a14e2cb to
cbd5e3b
Compare
ui/lib/storage-helpers.js
Outdated
There was a problem hiding this comment.
Nit: This function is no longer used.
|
The CI failure was already fixed on |
ecd5513 to
d6d97f7
Compare
d6d97f7 to
9632805
Compare
Fixes: #10081
Explanation:
Supercedes #10098 to avoid huge rebase.
It appears we're currently fetching basic gas estimates and storing the result along with last fetched time, and then doing expiration validation, when fetchWithCache can do all of this for us.