Remove use of ethgasstation; use metaswap /gasPrices api for gas price estimates by danjm · Pull Request #9867 · MetaMask/metamask-extension

danjm · 2020-11-13T10:16:26Z

This PR removes our reliance on ethgasstation in favour of the metaswap /gasPrices api.

For now, to make this change, we have to remove our time estimate functionality. Most of the PR is deletion of code.

github-actions · 2020-11-13T10:16:42Z

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.

metamaskbot · 2020-11-13T12:01:03Z

Builds ready [ccb3c62]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (618 ± 116 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	300	1134	615	241	116
		load	302	1141	618	242	116
		domInteractive	300	1134	615	241	116

kumavis · 2020-11-13T12:52:51Z

sad to see the price/time chart gone
glad to see quick fix for gas pricing

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js

rekmarks

Great job getting this in. I left a handful of inline comments.

I agree that getting rid of the chart and times is worth it to get this feature in, and the reason we have to do that is because the MetaSwap API doesn't have time estimates yet?

Edit: Left one question here, too: #9867 (comment)

ui/app/ducks/gas/gas.duck.js

metamaskbot · 2020-11-13T16:37:59Z

Builds ready [8ab0573]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (372 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	46	37	4	2
		domContentLoaded	256	595	370	112	54
		load	258	597	372	112	54
		domInteractive	256	594	370	112	54

rekmarks · 2020-11-13T16:45:07Z

For future reference: #9870

ui/app/hooks/useTokenTracker.js

rekmarks

LGTM!

metamaskbot · 2020-11-13T17:53:10Z

Builds ready [b185603]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (319 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	52	38	6	3
		domContentLoaded	259	437	317	50	24
		load	260	438	319	50	24
		domInteractive	259	437	317	50	24

Gudahtt · 2020-11-13T17:54:59Z

The gas price chart appears to now be unused, but it hasn't been deleted

danjm · 2020-11-13T18:15:37Z

@Gudahtt The gas price chart was deleted in 106a631

brad-decker

LGTM. So long time estimates 👋 😢

...ustomization/gas-modal-page-container/advanced-tab-content/advanced-tab-content.component.js

metamaskbot · 2020-11-13T18:57:37Z

Builds ready [26ce08c]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (371 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	28	40	33	4	2
		domContentLoaded	239	587	369	116	56
		load	240	589	371	116	56
		domInteractive	238	587	369	116	56

metamaskbot · 2020-11-13T20:26:42Z

Builds ready [394f93b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (325 ± 35 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	41	36	3	1
		domContentLoaded	260	579	323	73	35
		load	262	580	325	73	35
		domInteractive	260	579	323	73	35

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js

...n/gas-modal-page-container/advanced-tab-content/tests/advanced-tab-content-component.test.js

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js

.../gas-customization/gas-modal-page-container/tests/gas-modal-page-container-component.test.js

.../gas-customization/gas-modal-page-container/tests/gas-modal-page-container-container.test.js

ui/app/components/app/gas-customization/gas-price-chart/gas-price-chart.utils.js

ui/app/hooks/tests/useRetryTransaction.test.js

ui/app/ducks/gas/gas.duck.js

ui/app/pages/settings/advanced-tab/advanced-tab.container.js

ui/app/selectors/custom-gas.js

danjm · 2020-11-23T14:51:03Z

@Gudahtt I believe I have addressed all your latest comments, and removed other code as well, in the latest commit

Gudahtt · 2020-11-23T20:00:43Z

The d3/c3 removal requires build script changes, as they are explicitly pulled into a ui-libs bundle.

metamaskbot · 2020-11-24T10:52:56Z

Builds ready [cfa2657]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (385 ± 61 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	81	44	15	7
		domContentLoaded	258	729	384	128	61
		load	259	731	385	128	61
		domInteractive	258	729	383	128	61

danjm · 2020-11-25T14:40:00Z

@Gudahtt this should be good to go now

metamaskbot · 2020-11-25T15:01:19Z

Builds ready [60b820f]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (359 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	75	50	16	8
		domContentLoaded	265	504	358	92	44
		load	266	506	359	92	44
		domInteractive	264	504	358	92	44

Gudahtt

LGTM, but needs a rebase to resolve the lockfile conflict

…estimates

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

danjm · 2020-12-02T16:53:30Z

@Gudahtt this has been rebased

metamaskbot · 2020-12-02T17:22:39Z

Builds ready [3ad445a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (388 ± 38 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	84	53	19	9
		domContentLoaded	290	550	386	80	39
		load	292	551	388	80	38
		domInteractive	290	550	386	80	39

danjm requested a review from a team as a code owner November 13, 2020 10:16

danjm requested a review from rekmarks November 13, 2020 10:16

danjm force-pushed the replace-ethgasstation branch from 253fef0 to 9d45175 Compare November 13, 2020 11:19

danjm closed this Nov 13, 2020

danjm force-pushed the replace-ethgasstation branch from 9d45175 to 67303b7 Compare November 13, 2020 11:41

github-actions bot locked and limited conversation to collaborators Nov 13, 2020

danjm reopened this Nov 13, 2020

rekmarks reviewed Nov 13, 2020

View reviewed changes

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js Show resolved Hide resolved

MetaMask unlocked this conversation Nov 13, 2020

rekmarks requested changes Nov 13, 2020

View reviewed changes

ui/app/ducks/gas/gas.duck.js Outdated Show resolved Hide resolved

ui/app/ducks/gas/gas.duck.js Outdated Show resolved Hide resolved

rekmarks requested changes Nov 13, 2020

View reviewed changes

ui/app/hooks/useTokenTracker.js Outdated Show resolved Hide resolved

rekmarks previously approved these changes Nov 13, 2020

View reviewed changes

danjm dismissed rekmarks’s stale review via 106a631 November 13, 2020 18:15

brad-decker previously approved these changes Nov 13, 2020

View reviewed changes

danjm dismissed brad-decker’s stale review via 26ce08c November 13, 2020 18:37

Gudahtt reviewed Nov 13, 2020

View reviewed changes

...ustomization/gas-modal-page-container/advanced-tab-content/advanced-tab-content.component.js Show resolved Hide resolved

Gudahtt reviewed Nov 16, 2020

View reviewed changes

omnat mentioned this pull request Nov 16, 2020

Update gas estimation API (across the app) MetaMask/metamask-mobile#1998

Closed

brad-decker assigned Gudahtt and brad-decker Nov 17, 2020

brad-decker assigned rekmarks Nov 17, 2020

danjm force-pushed the replace-ethgasstation branch from 394f93b to 0064827 Compare November 23, 2020 14:49

danjm force-pushed the replace-ethgasstation branch from 0064827 to d2c5c52 Compare November 23, 2020 15:08

danjm force-pushed the replace-ethgasstation branch from cfa2657 to 60b820f Compare November 25, 2020 14:39

danjm requested a review from kumavis as a code owner November 25, 2020 14:39

Gudahtt requested changes Dec 1, 2020

View reviewed changes

danjm and others added 9 commits December 2, 2020 13:20

Remove use of ethgassthat; use metaswap /gasPrices api for gas price …

a7eb931

…estimates

Remove references to ethgasstation

bfd71fe

Pass base to BigNumber constructor in fetchExternalBasicGasEstimates

b2360c6

Update ui/app/hooks/useTokenTracker.js

1fda01d

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

Delete gas price chart

e9b6a40

Remove price chart css import

2170738

Delete additional fee chart code

bb6fca1

Lint fix

e572a36

Delete more code no longer used after ethgasstation removal

3ad445a

danjm force-pushed the replace-ethgasstation branch from 60b820f to 3ad445a Compare December 2, 2020 16:53

Gudahtt approved these changes Dec 2, 2020

View reviewed changes

danjm merged commit 97d268c into develop Dec 2, 2020

danjm deleted the replace-ethgasstation branch December 2, 2020 23:25

github-actions bot locked and limited conversation to collaborators Dec 2, 2020

Conversation

danjm commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 13, 2020

Uh oh!

metamaskbot commented Nov 13, 2020

Uh oh!

kumavis commented Nov 13, 2020

Uh oh!

Uh oh!

rekmarks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Nov 13, 2020

Uh oh!

rekmarks commented Nov 13, 2020

Uh oh!

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Nov 13, 2020

Uh oh!

Gudahtt commented Nov 13, 2020

Uh oh!

danjm commented Nov 13, 2020

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metamaskbot commented Nov 13, 2020

Uh oh!

metamaskbot commented Nov 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danjm commented Nov 23, 2020

Uh oh!

Gudahtt commented Nov 23, 2020

Uh oh!

metamaskbot commented Nov 24, 2020

Uh oh!

danjm commented Nov 25, 2020

Uh oh!

metamaskbot commented Nov 25, 2020

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

danjm commented Dec 2, 2020

Uh oh!

metamaskbot commented Dec 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danjm commented Nov 13, 2020 •

edited

Loading

rekmarks left a comment •

edited

Loading