Replace useSafeGasEstimatePolling with usePolling #23010

jiexi · 2024-02-16T17:53:03Z

Description

Added usePolling hook and used it within useSafeGasEstimatePolling hook.

This replaces where gas estimates are coming from for the confirmation flow.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2014

Manual testing steps

Hit send button
get to confirmation screen
see that gas estimation updates

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-02-16T17:53:16Z

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.

codecov · 2024-02-16T18:06:23Z

Codecov Report

Attention: Patch coverage is 94.78261% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 68.77%. Comparing base (6c810aa) to head (d7b1e5e).

Files	Patch %	Lines
ui/ducks/metamask/metamask.js	90.48%	2 Missing ⚠️
ui/store/actions.ts	77.78%	2 Missing ⚠️
app/scripts/metamask-controller.js	0.00%	1 Missing ⚠️
ui/pages/swaps/prepare-swap-page/review-quote.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #23010      +/-   ##
===========================================
+ Coverage    68.70%   68.77%   +0.07%     
===========================================
  Files         1120     1120              
  Lines        43509    43527      +18     
  Branches     11636    11654      +18     
===========================================
+ Hits         29891    29933      +42     
+ Misses       13618    13594      -24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ui/hooks/useGasFeeEstimates.js

ui/ducks/metamask/metamask.js

...components/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js

adonesky1 · 2024-02-20T15:30:18Z

ui/hooks/useGasFeeEstimates.js

+  useEffect(() => {
+    getNetworkConfigurationByNetworkClientId(networkClientId).then(
+      (networkConfig) => {
+        if (networkConfig) {


could this be undefined?

doesn't seem like we have any plan in the case that it is... should we throw an error since this would be unexpected?

yeah, i suppose we should throw an error, but i think that's tricky to do from inside an effect

If its undefined we could set some error state that we could return from the hook. The reason I think we probably need to do something like this is because it won't be apparent what's wrong and why polling isn't working if this failed... Though I admit I don't see why it would fail

ui/hooks/useGasFeeEstimates.js

adonesky1 · 2024-03-05T18:40:22Z

I am able to repro, I see errors both from StaticIntervalPollingController and PolliognBlockTracker after I switch networks.

Steps:

Add ganache (server is on)
Perform a tx
Switch to sepolia

I agree this is should be addressed separately but I'm confused why I'm not seeing the errors you describe. I followed the steps above and this is what I'm seeing:

Screen.Recording.2024-03-05.at.12.37.45.PM.mov

…Safe

adonesky1 · 2024-03-05T18:55:24Z

ui/pages/confirmations/components/gas-timing/gas-timing.component.js

@@ -65,6 +65,7 @@ export default function GasTiming({
  const previousIsUnknownLow = usePrevious(isUnknownLow);


Looks like we're not reading gasFeeEstimates and gasEstimateType from the gasFeeEstimatesByChainId state yet on this component. Is that intentional?

i believe values derived from gasFeeEstimatesByChainId are used here

…Safe

ui/hooks/usePolling.test.js

jpuri · 2024-03-06T06:13:05Z

ui/hooks/usePolling.ts

+      JSON.stringify(
+        usePollingOptions.options,
+        Object.keys(usePollingOptions.options).sort(),
+      ),


Wondering if its possible to simplify above conditional.

This logic is to ensure that if the usePollingOptions.options is present we can guarantee the order of the keys in the option object so that the hook doesn't rerun unnecessarily. I'm not sure if there is a better way to express that conditional.

jpuri · 2024-03-06T06:16:19Z

...ts/advanced-gas-fee-popover/advanced-gas-fee-inputs/priority-fee-input/priority-fee-input.js

+  const [priorityFee, setPriorityFee] = useState(defaultPriorityFee);
+  useEffect(() => {
+    setPriorityFee(defaultPriorityFee);
+  }, [defaultPriorityFee, setPriorityFee]);


Here we did calculation in useState as this should be done only once. defaultPriorityFee should not change.

I believe this is necessary because with the new polling hook some of the values from useGasFeeContext used to derive defaultPriorityFee will be undefined on first render. @jiexi can maybe add some color here

But we need to check scenario if user has set a custom value for priority fee, can this override that ?

User set Max Base Fee and Priority Fee are not reset/changed by this. I have verified by delaying the gas estimate state update in the hook and seeing that the Max Fee does not change and that of course the Max Base Fee and Priority Fee inputs do not change either

…Safe

metamaskbot · 2024-03-06T17:43:51Z

Builds ready [37ef9ac]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (857 ± 451 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	86	238	136	43	20
		domContentLoaded	15	86	34	19	9
		load	77	2761	857	939	451
		domInteractive	15	85	34	19	9

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 278 Bytes (0.01%)
ui: 2.14 KiB (0.03%)
common: 0 Bytes (0.00%)

…Safe

metamaskbot · 2024-03-06T20:09:32Z

Builds ready [974acba]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1093 ± 393 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	211	133	33	16
		domContentLoaded	10	71	32	16	8
		load	80	2007	1093	818	393
		domInteractive	9	71	32	16	8

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 278 Bytes (0.01%)
ui: 2.14 KiB (0.03%)
common: 0 Bytes (0.00%)

…Safe

metamaskbot · 2024-03-08T14:58:48Z

Builds ready [d7b1e5e]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1058 ± 431 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	293	138	56	27
		domContentLoaded	10	74	27	15	7
		load	58	2408	1058	897	431
		domInteractive	10	74	27	15	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 278 Bytes (0.01%)
ui: 2.14 KiB (0.03%)
common: 0 Bytes (0.00%)

…tworkClientId (#24065)  ## **Description** #23010 made initial changes to replace the GasFeeController's legacy gas fee polling in favor of polling via networkClientId. It was discovered that there are still lingering instances of the legacy gas fee polling which were causing double polling to occur since the legacy and networkClientId based polling run on separate loops. This PR fixes this by replacing those remaining legacy gas fee polling usages. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24065?quickstart=1) ## **Related issues** Fixes: #23010 ## **Manual testing steps** 1. Open background console -> Network tab 2. Open Extension Popup -> send -> enter address and amount -> next 3. Background console network tab should now start showing one single network request every 10s to the gas api endpoint ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Alex <[email protected]>

shanejonas and others added 7 commits February 15, 2024 15:53

Added generic polling hook

556a765

Added fast stable stringify

5706baa

Removed fast stringify for normal json stringify to not deal with ESM

a9a2303

Removed fast-json-stable-stringify

591c934

Fixed issue if usePollingOptions.options is not defined

d8bb41d

Merge branch 'develop' into sj/added-generic-polling-hook

1c6d5ea

Replace useSafeGasEstimatePolling with usePolling

cf501b9

update specs

57a0469

shanejonas reviewed Feb 16, 2024

View reviewed changes

ui/hooks/useGasFeeEstimates.js Outdated Show resolved Hide resolved

jiexi added 2 commits February 16, 2024 13:47

update useGasFeeEstimates to use usePolling hook

3d48452

fix undefined dereference estimatedBaseFee

cabd6db

shanejonas reviewed Feb 16, 2024

View reviewed changes

ui/ducks/metamask/metamask.js Outdated Show resolved Hide resolved

shanejonas previously approved these changes Feb 16, 2024

View reviewed changes

Fix base-fee-input

3ec134e

jiexi commented Feb 16, 2024

View reviewed changes

...components/advanced-gas-fee-popover/advanced-gas-fee-inputs/base-fee-input/base-fee-input.js Show resolved Hide resolved

jiexi added 2 commits February 16, 2024 15:54

jsdoc

9a641c9

remove .tool-versions

cd086c4

adonesky1 reviewed Feb 20, 2024

View reviewed changes

ui/hooks/useGasFeeEstimates.js Show resolved Hide resolved

jiexi force-pushed the jl/added-generic-polling-hook-replace-useSafe branch from 96dee07 to b91d6f9 Compare February 20, 2024 18:03

fix advanced-gas-fee-gas-limit.test.js

6398d06

jiexi force-pushed the jl/added-generic-polling-hook-replace-useSafe branch from b91d6f9 to 6398d06 Compare February 20, 2024 18:06

jiexi added 6 commits February 20, 2024 10:13

Fix priority fee

c99ff3b

fix base fee import act

db00da5

fix advanced fee

40bf680

fix priority fee input

3173387

fix gas fee popover

b2d3f31

lint

b3160b8

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

168fc81

…Safe

adonesky1 reviewed Mar 5, 2024

View reviewed changes

jiexi added 2 commits March 5, 2024 14:00

update confirm-send-ether.test.js snapshot

7cf4cc1

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

ec6c828

…Safe

adonesky1 previously approved these changes Mar 5, 2024

View reviewed changes

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

c246019

…Safe

jpuri reviewed Mar 6, 2024

View reviewed changes

ui/hooks/usePolling.test.js Outdated Show resolved Hide resolved

jpuri reviewed Mar 6, 2024

View reviewed changes

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

928c60b

…Safe

adonesky1 dismissed their stale review via 928c60b March 6, 2024 15:04

adonesky1 added 2 commits March 6, 2024 09:47

use renderHookWithProvider

02a0c73

fix broken swaps flow

37ef9ac

adonesky1 force-pushed the jl/added-generic-polling-hook-replace-useSafe branch from b627c45 to 37ef9ac Compare March 6, 2024 17:01

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

64c3fd5

…Safe

adonesky1 approved these changes Mar 6, 2024

View reviewed changes

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

974acba

…Safe

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

56fedb4

…Safe

jpuri approved these changes Mar 7, 2024

View reviewed changes

Merge branch 'develop' into jl/added-generic-polling-hook-replace-use…

d7b1e5e

…Safe

Mrtenz approved these changes Mar 8, 2024

View reviewed changes

adonesky1 merged commit 1f89a65 into develop Mar 8, 2024
76 checks passed

adonesky1 deleted the jl/added-generic-polling-hook-replace-useSafe branch March 8, 2024 15:00

github-actions bot locked and limited conversation to collaborators Mar 8, 2024

metamaskbot added the release-11.14.0 Issue or pull request that will be included in release 11.14.0 label Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace useSafeGasEstimatePolling with usePolling #23010

Replace useSafeGasEstimatePolling with usePolling #23010

jiexi commented Feb 16, 2024 •

edited by shanejonas

Loading

github-actions bot commented Feb 16, 2024

codecov bot commented Feb 16, 2024 •

edited

Loading

adonesky1 Feb 20, 2024

adonesky1 Feb 20, 2024 •

edited

Loading

jiexi Feb 20, 2024

adonesky1 Feb 28, 2024

adonesky1 commented Mar 5, 2024

adonesky1 Mar 5, 2024

jiexi Mar 5, 2024

jpuri Mar 6, 2024

adonesky1 Mar 6, 2024

jpuri Mar 6, 2024

adonesky1 Mar 6, 2024

jpuri Mar 6, 2024

jiexi Mar 6, 2024

metamaskbot commented Mar 6, 2024

metamaskbot commented Mar 6, 2024

metamaskbot commented Mar 8, 2024

		@@ -65,6 +65,7 @@ export default function GasTiming({
		const previousIsUnknownLow = usePrevious(isUnknownLow);

Replace useSafeGasEstimatePolling with usePolling #23010

Replace useSafeGasEstimatePolling with usePolling #23010

Conversation

jiexi commented Feb 16, 2024 • edited by shanejonas Loading

Description

Related issues

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Feb 16, 2024

codecov bot commented Feb 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

adonesky1 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Mar 6, 2024

metamaskbot commented Mar 6, 2024

metamaskbot commented Mar 8, 2024

jiexi commented Feb 16, 2024 •

edited by shanejonas

Loading

codecov bot commented Feb 16, 2024 •

edited

Loading

adonesky1 Feb 20, 2024 •

edited

Loading