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

Avoid showing "Gas price extremely low" warning in advanced tab for testnets #11111

Merged
merged 3 commits into from
May 20, 2021

Conversation

NiranjanaBinoy
Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy commented May 16, 2021

Fixes: #11076

Explanation:
Problem: While speeding up or canceling a transaction on testnets, the gasprice on the advanced tab is showing "Gas price extremely low" warning no matter what price is entered.

Fix: On testnets, we don't use the gas prices API to get slow, medium, and fast estimates. Instead, we just get a single estimate via eth_gasprice. So the isCustomPriceSafe() will be called only for Mainnet and e2e test and a default true value will be sent for testnets.

Manual testing steps:

@NiranjanaBinoy NiranjanaBinoy requested a review from danjm May 16, 2021 23:06
@NiranjanaBinoy NiranjanaBinoy self-assigned this May 16, 2021
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner May 16, 2021 23:06
@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.

@NiranjanaBinoy NiranjanaBinoy changed the title Avoid showing "Gas price extremely low" warning for speedup in testnets Avoid showing "Gas price extremely low" warning in advanced tab in gas-modals for testnets May 16, 2021
@NiranjanaBinoy NiranjanaBinoy changed the title Avoid showing "Gas price extremely low" warning in advanced tab in gas-modals for testnets Avoid showing "Gas price extremely low" warning in advanced tab for testnets May 16, 2021
Gudahtt
Gudahtt previously approved these changes May 17, 2021
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! This is at least a solid improvement.

I think there remains an issue when the gas estimation API is offline. If we don't have any value cached for the safe low price, this problem will continue to affect users on mainnet I think?

I haven't thought this through, so I don't know how we might fix that issue. If a solution isn't clear, perhaps we can open a separate issue for it, and merge this for now.

@NiranjanaBinoy
Copy link
Contributor Author

NiranjanaBinoy commented May 17, 2021

LGTM! This is at least a solid improvement.

I think there remains an issue when the gas estimation API is offline. If we don't have any value cached for the safe low price, this problem will continue to affect users on mainnet I think?

I haven't thought this through, so I don't know how we might fix that issue. If a solution isn't clear, perhaps we can open a separate issue for it, and merge this for now.

PR #10767 is handling the gas API failure scenarios, currently, if the API fetch fails we are fetching gas price from eth_gasprice and warning the user of the same, and failure of both the service will disable the transaction submission and shows an error message.

I have updated to set customPriceIsSafe to true in the API fetch failure for speedup and cancellation. Is this a suitable solution for this problem?

@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2021
@metamaskbot
Copy link
Collaborator

Builds ready [6d11781]
Page Load Metrics (584 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448359105
domContentLoaded3597505839847
load3607515849847
domInteractive3587505839847

@Gudahtt
Copy link
Member

Gudahtt commented May 17, 2021

Ah, great idea! I hadn't seen this basicEstimateStatus.

Though, I think we'll need to use a different selector. This one will return false if the gas estimates are still loading for example, wrongly displaying this warning until they finish. It'll also show the warning if the gas estimates failed to load completely.

Maybe we should only show the warning if basicEstimateStatus is READY, and the estimateSource is MetaSwaps? (or not eth_gasprice, alternatively)

@metamaskbot
Copy link
Collaborator

Builds ready [b8d37a5]
Page Load Metrics (603 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448064115
domContentLoaded3737206028240
load3747216038240
domInteractive3727196028240

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!

@NiranjanaBinoy NiranjanaBinoy merged commit 2972e78 into develop May 20, 2021
@NiranjanaBinoy NiranjanaBinoy deleted the gasprice-low-warning branch May 20, 2021 15:32
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.

"Gas price extremely low" warning showing on speed ups on test nets
3 participants