fix: Retry requests from provider after connection is ready, on all builds by jiexi · Pull Request #24142 · MetaMask/metamask-extension

jiexi · 2024-04-19T20:19:51Z

Description

Currently, requests from the dapp can hang if:

they are sent before the extension is fully initialized / before the extension is ready to process requests from that specific dapp connection
they get swallowed do to an issue in chromium with streams on prerendered pages breaking when they become visible to the user

The inpage provider does have a retry mechanism, but currently that only works on MV3 builds. Additionally, is not a 100% reliable trigger as the underlying metamask_chainChanged message that it relies on is tied to MetaMaskController mem state changes, and is not fired off each time a dapp connection is established.

This PR attempts to fix request retries by:

Making the retry message triggered by metamask_chainChanged work on all builds, not just MV3 builds
Firing metamask_chainChanged every time a new dapp connection is established

Open concern:
There may be an edge case when a request with a confirmation is sent by the dapp, then received and shown to the user in the wallet, but swallowed by prerender. I believe my proposed changes in this PR would cause the confirmation to get resent even though there may still be a confirmation being shown to the user. I haven't been able to test this properly as debugging the prerender issue in general is difficult.

This would require a dapp to send a request requiring confirmation pretty much immediately after page load, implying without user interaction.

Related issues

See: #11069
See: #23329
See: #24025

Manual testing steps

NOTE: these testing steps require that you add an artificial 5s delay to the background connection handling. See the comments below to find the commit where this delay was included in this PR before being removed for merging

Verify this fixes the `metamask_sendDomainMetadata` issue

Restart extension
Open any page
Quickly open the wallet popup
Notice that there is no favicon
After 5s have elapsed since the page was opened, the favicon should populate in the wallet popup

Verify this fixes retrying messages before extension is ready

Open any page (restarting extension not required)
Quickly open console and type await window.ethereum.request({method: 'eth_chainId'})
Notice that the response doesn't come immediately
After 5s have elapsed since the page was opened, the response should be shown in console

Verify this fixes retrying messages after broken prerender stream

Not easy to verify this unfortunately, you would need to break the prerender stream while a request was being made

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-04-19T20:20:03Z

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.

sentry · 2024-04-19T20:20:06Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: app/scripts/background.js

Function	Unhandled Issue
`setupController`	TypeError: Cannot read properties of undefined (reading 'getCurrentBlock') new exports.default(app/scrip... `Event Count:` 13 `Affected Users:` 0
`setupController`	new MetamaskController(app/scri... `Event Count:` 4 `Affected Users:` 0

_{Did you find this useful? React with a 👍 or 👎}

app/scripts/metamask-controller.js

jiexi · 2024-04-19T21:30:41Z

app/scripts/background.js

+        console.log(`delaying connection to ${remotePort.sender.url} for 5s`);
+        await new Promise((resolve) => setTimeout(resolve, 5000));
+


this is here to make testing easier. It delays the handling of the dapp responses over the port for 5s.

TODO: Remove this before approval, of course

This has been removed, but must be added back locally if you are testing manually

jiexi · 2024-05-30T17:57:00Z

Following change in web3-stream-provider needed MetaMask/web3-stream-provider#16

metamaskbot · 2024-05-30T23:34:33Z

Builds ready [9e29f0e]

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, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2874 ± 885 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	135	475	220	83	40
		domContentLoaded	18	273	61	65	31
		load	115	5064	2874	1843	885
		domInteractive	18	273	61	65	31

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

background: 477 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

codecov · 2024-05-30T23:34:39Z

Codecov Report

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

Project coverage is 65.69%. Comparing base (0b0bf11) to head (6a8083b).

Files	Patch %	Lines
app/scripts/metamask-controller.js	76.92%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #24142      +/-   ##
===========================================
- Coverage    65.70%   65.69%   -0.01%     
===========================================
  Files         1366     1366              
  Lines        54361    54391      +30     
  Branches     14141    14144       +3     
===========================================
+ Hits         35716    35728      +12     
- Misses       18645    18663      +18

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

metamaskbot · 2024-05-31T04:39:08Z

Builds ready [6513795]

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, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (206 ± 223 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	109	86	11	6
		domContentLoaded	9	27	12	4	2
		load	42	1609	206	464	223
		domInteractive	9	27	12	4	2

metamaskbot · 2024-05-31T16:39:22Z

Builds ready [6921a09]

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, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (51 ± 4 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	69	98	82	8	4
		domContentLoaded	8	22	10	3	1
		load	42	66	51	8	4
		domInteractive	8	22	10	3	1

metamaskbot · 2024-06-01T01:50:02Z

Builds ready [6a8083b]

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, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (52 ± 3 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	70	110	86	10	5
		domContentLoaded	9	25	12	4	2
		load	43	73	52	7	3
		domInteractive	9	25	12	4	2

adonesky1 · 2024-06-03T22:02:51Z

I'm not sure what this is about but I'm seeing some strange inexplicable inconsistency with favicons:
https://github.com/MetaMask/metamask-extension/assets/34557516/87cf7091-fd41-4c42-ab8c-d1c9082a0a6e

adonesky1

I'm not seeing the issue with the favicon anymore. I've tested all scenarios (with and without the added delay) and they all look good. Code looks good to me!

Gudahtt

LGTM!

jiexi added 3 commits April 19, 2024 09:26

Remove mv3 guard on METAMASK_EXTENSION_CONNECT_CAN_RETRY

3e31dab

emit metamask_chainChanged on every new connection

95d7062

Merge branch 'develop' into jl/retry-requests-provider-all-builds

2828fbc

jiexi commented Apr 19, 2024

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

jiexi commented Apr 19, 2024

View reviewed changes

jiexi added team-extension-platform Extension Platform team team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead labels Apr 19, 2024

jiexi changed the title ~~WIP: Retry requests on provider for all builds~~ fix: Retry requests from provider after connection is ready, on all builds Apr 19, 2024

jiexi marked this pull request as ready for review April 19, 2024 22:03

jiexi requested a review from a team as a code owner April 19, 2024 22:03

Merge branch 'develop' into jl/retry-requests-provider-all-builds

4e30e01

jiexi mentioned this pull request Apr 23, 2024

fix: Disable preload on chromium #24201

Closed

7 tasks

adonesky1 mentioned this pull request Apr 23, 2024

[Bug] subjectMetadata doesn't return the active Tab metaData #24025

Open

Merge branch 'develop' into jl/retry-requests-provider-all-builds

149fda5

jpuri previously approved these changes Apr 25, 2024

View reviewed changes

jiexi added 2 commits April 26, 2024 09:31

Merge branch 'develop' into jl/retry-requests-provider-all-builds

9bbd55c

Merge branch 'develop' into jl/retry-requests-provider-all-builds

cc76bd8

jiexi dismissed jpuri’s stale review via cc76bd8 May 1, 2024 23:02

Notify single connection

f90b220

jiexi mentioned this pull request May 3, 2024

[Bug]: Calling request too early after the browser start leads to a Promise which is never settled #23336

Closed

jiexi and others added 7 commits May 3, 2024 12:11

Merge branch 'develop' into jl/retry-requests-provider-all-builds

dc2e923

Merge branch 'develop' into jl/retry-requests-provider-all-builds

b96dafe

Merge branch 'develop' into jl/retry-requests-provider-all-builds

11a0279

Merge branch 'develop' into jl/retry-requests-provider-all-builds

989740f

Merge branch 'develop' into jl/retry-requests-provider-all-builds

c5d969d

Merge branch 'develop' into jl/retry-requests-provider-all-builds

7171c50

Merge branch 'develop' into jl/retry-requests-provider-all-builds

dfe2978

jiexi requested a review from Gudahtt May 29, 2024 21:17

jiexi requested review from adonesky1 and shanejonas May 29, 2024 21:17

fix spec

f4d20be

jiexi added 4 commits May 30, 2024 13:37

Merge branch 'develop' into jl/retry-requests-provider-all-builds

de6647a

only send liveliness notification to non-internal connections

66bf7a3

Merge branch 'develop' into jl/retry-requests-provider-all-builds

926bf14

Add notifyConnections try catch

7ae80ba

jiexi force-pushed the jl/retry-requests-provider-all-builds branch from b503c8d to 7ae80ba Compare May 30, 2024 22:17

jiexi added 2 commits May 30, 2024 15:19

Merge branch 'develop' into jl/retry-requests-provider-all-builds

d6e1542

remove 5s delay

9e29f0e

Merge branch 'develop' into jl/retry-requests-provider-all-builds

6513795

Merge branch 'develop' into jl/retry-requests-provider-all-builds

6921a09

Merge branch 'develop' into jl/retry-requests-provider-all-builds

6a8083b

adonesky1 approved these changes Jun 6, 2024

View reviewed changes

shanejonas approved these changes Jun 12, 2024

View reviewed changes

Gudahtt approved these changes Jun 12, 2024

View reviewed changes

jiexi merged commit dc5247f into develop Jun 12, 2024

jiexi deleted the jl/retry-requests-provider-all-builds branch June 12, 2024 17:28

github-actions bot locked and limited conversation to collaborators Jun 12, 2024

metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 12, 2024

		console.log(`delaying connection to ${remotePort.sender.url} for 5s`);
		await new Promise((resolve) => setTimeout(resolve, 5000));

Conversation

jiexi commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Verify this fixes the metamask_sendDomainMetadata issue

Verify this fixes retrying messages before extension is ready

Verify this fixes retrying messages after broken prerender stream

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Apr 19, 2024

Uh oh!

sentry bot commented Apr 19, 2024

🔍 Existing Issues For Review

Uh oh!

Uh oh!

jiexi Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

jiexi Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

jiexi May 31, 2024

Choose a reason for hiding this comment

Uh oh!

jiexi commented May 30, 2024

Uh oh!

metamaskbot commented May 30, 2024

Uh oh!

codecov bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented May 31, 2024

Uh oh!

metamaskbot commented May 31, 2024

Uh oh!

metamaskbot commented Jun 1, 2024

Uh oh!

adonesky1 commented Jun 3, 2024

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jiexi commented Apr 19, 2024 •

edited

Loading

Verify this fixes the `metamask_sendDomainMetadata` issue

codecov bot commented May 30, 2024 •

edited

Loading