fix: `wallet_addEthereumChain` does not attach a `result` under certain conditions #26726

adonesky1 · 2024-08-28T18:24:42Z

Description

Fix issue where wallet_addEthereumChain does not attach a result to the response object when the currently selected rpcUrl matches the request.

This would cause the request to get "stuck" in the QueuedRequestController queue, preventing the queue from progressing and causing confusing behavior.

Related issues

Fixes: #26706

Manual testing steps

Go to https://chainlist.org/?chain=56&search=blast
Connect the wallet and add Blast Mainnet
(For other chains it appears Chainlist cycles to the next rpcUrl you don't have which avoids this bug, so use Blast)
After successfully adding the network, attempt to add Blast again. Nothing should happen.
Then go to https://faucet.quicknode.com/blast/sepolia (or any dapp where your wallet isn't already connected) and attempt to connect
You should be able to connect as usual

Screenshots/Recordings

Before

Screen.Recording.2024-08-28.at.1.37.50.PM.mov

After

Screen.Recording.2024-08-28.at.1.33.16.PM.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension 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.

…he response object when the currently selected rpcUrl matches the request

github-actions · 2024-08-28T18:24:56Z

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.

Gudahtt · 2024-08-28T18:49:20Z

This change looks correct to me, but I am curious how it might have caused this problem. Wouldn't end() have triggered the QueuedRequestController to consider this as resolved? Why do we need result to be set for it to be resolved there?

adonesky1 · 2024-08-28T18:53:51Z

This change looks correct to me, but I am curious how it might have caused this problem. Wouldn't end() have triggered the QueuedRequestController to consider this as resolved? Why do we need result to be set for it to be resolved there.

I would have thought so too... which is certainly why this bug was introduced and unnoticed. And even when debugging I stared at it for a while thinking this should resolve the request. But apparently the rpc engine needs a result if an error is not specified in the end function... something to probably look into modifying in the JSON RPC Engine

Gudahtt · 2024-08-28T18:54:25Z

Oh I see, I recall that now. Makes sense.

Gudahtt

LGTM!

adonesky1 · 2024-08-28T18:56:19Z

seems like the chrome e2e test failure might relate to these brownouts? All the tests seem to be passing...

FrederikBolding · 2024-08-28T18:59:18Z

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

@@ -139,6 +139,7 @@ async function addEthereumChainHandler(
      currentChainIdForDomain === chainId &&
      currentRpcUrl === firstValidRPCUrl
    ) {
+      res.result = null;


Not a blocker, but curious how the JsonRpcEngine didn't end up throwing for this. AFAIK if we process all middlewares without a result we end up throwing an error?

It did throw an error in this case, and it was returned to the dapp. But the QueuedRequestController missed it for some reason.

But yeah, good point, how was this missed 🤔

It actually did throw an error to the dapp as you can see in the after video

But the QueuedRequestController missed it for some reason.

Yeah... we should investigate this. I can create a ticket

https://app.zenhub.com/workspaces/wallet-api-platform-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/3096

Gudahtt · 2024-08-28T18:59:53Z

Possibly; the initial failure is one we've always seen occasionally on CircleCI. The "Rerun failed tests" button always fails unless test artifacts have been uploaded during the failed run, so "Rerun failed workflows" needs to be used instead. I've just triggered that.

codecov · 2024-08-28T19:07:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.08%. Comparing base (d9e989e) to head (c4c185b).
Report is 15 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26726      +/-   ##
===========================================
- Coverage    70.09%   70.08%   -0.01%     
===========================================
  Files         1413     1413              
  Lines        49255    49318      +63     
  Branches     13768    13780      +12     
===========================================
+ Hits         34524    34562      +38     
- Misses       14731    14756      +25

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

metamaskbot · 2024-08-28T19:19:20Z

Builds ready [2cf7859]

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

Page Load Metrics (71 ± 7 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	154	94	19	9
		domContentLoaded	42	103	68	13	6
		load	49	103	71	14	7
		domInteractive	11	55	26	10	5

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

background: 14 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

jiexi · 2024-08-28T21:04:07Z

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.test.js

+    jest.spyOn(chainUtils, 'findExistingNetwork').mockReturnValue({
+      chainId: '0x1',
+      rpcUrl: 'https://mainnet.infura.io/v3/',
+      ticker: 'ETH',
+    });


guessing you can just modify the findNetworkConfigurationBy mock result because findExistingNetwork also checks the built in networks constant?

if so, can we modify this test to use a non-built-in network instead?

if it wasnt* clear, this is a nit and totally ignorable. I'm mainly asking so we can avoid having to import and mock chainutils

addressed here: 4c93640

jiexi

approving to avoid being a blocker. feel free to ignore nit

sonarcloud · 2024-08-28T21:50:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-08-28T22:21:36Z

Builds ready [c4c185b]

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

Page Load Metrics (82 ± 10 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	77	191	111	24	12
		domContentLoaded	53	151	78	23	11
		load	58	151	82	22	10
		domInteractive	16	83	32	15	7

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

background: 14 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

fix issue where wallet_addEthereumChain does not attach a result to t…

2cf7859

…he response object when the currently selected rpcUrl matches the request

github-actions bot added the team-wallet-api-platform label Aug 28, 2024

adonesky1 marked this pull request as ready for review August 28, 2024 18:38

adonesky1 requested a review from a team as a code owner August 28, 2024 18:38

adonesky1 requested review from Gudahtt and seaona August 28, 2024 18:44

Gudahtt previously approved these changes Aug 28, 2024

View reviewed changes

FrederikBolding reviewed Aug 28, 2024

View reviewed changes

jiexi reviewed Aug 28, 2024

View reviewed changes

jiexi previously approved these changes Aug 28, 2024

View reviewed changes

improve test

4c93640

adonesky1 dismissed stale reviews from jiexi and Gudahtt via 4c93640 August 28, 2024 21:24

adonesky1 requested review from Gudahtt and jiexi August 28, 2024 21:24

jiexi previously approved these changes Aug 28, 2024

View reviewed changes

Gudahtt previously approved these changes Aug 28, 2024

View reviewed changes

unfocus test

c4c185b

adonesky1 dismissed stale reviews from Gudahtt and jiexi via c4c185b August 28, 2024 21:48

adonesky1 requested review from jiexi and Gudahtt August 28, 2024 21:48

Gudahtt approved these changes Aug 28, 2024

View reviewed changes

jiexi approved these changes Aug 28, 2024

View reviewed changes

adonesky1 merged commit fb61b0f into develop Aug 28, 2024
77 of 78 checks passed

adonesky1 deleted the ad/fix-wallet_addEthereumChain-handler branch August 28, 2024 22:11

github-actions bot locked and limited conversation to collaborators Aug 28, 2024

metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 28, 2024

gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `wallet_addEthereumChain` does not attach a `result` under certain conditions #26726

fix: `wallet_addEthereumChain` does not attach a `result` under certain conditions #26726

adonesky1 commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024

Gudahtt commented Aug 28, 2024 •

edited

Loading

adonesky1 commented Aug 28, 2024

Gudahtt commented Aug 28, 2024

Gudahtt left a comment

adonesky1 commented Aug 28, 2024

FrederikBolding Aug 28, 2024

Gudahtt Aug 28, 2024

adonesky1 Aug 28, 2024

adonesky1 Aug 28, 2024

adonesky1 Aug 28, 2024

Gudahtt commented Aug 28, 2024

codecov bot commented Aug 28, 2024 •

edited

Loading

metamaskbot commented Aug 28, 2024

jiexi Aug 28, 2024

jiexi Aug 28, 2024 •

edited

Loading

adonesky1 Aug 28, 2024

jiexi left a comment

sonarcloud bot commented Aug 28, 2024

metamaskbot commented Aug 28, 2024

fix: wallet_addEthereumChain does not attach a result under certain conditions #26726

fix: wallet_addEthereumChain does not attach a result under certain conditions #26726

Conversation

adonesky1 commented Aug 28, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 28, 2024

Gudahtt commented Aug 28, 2024 • edited Loading

adonesky1 commented Aug 28, 2024

Gudahtt commented Aug 28, 2024

Gudahtt left a comment

Choose a reason for hiding this comment

adonesky1 commented Aug 28, 2024

FrederikBolding Aug 28, 2024

Choose a reason for hiding this comment

Gudahtt Aug 28, 2024

Choose a reason for hiding this comment

adonesky1 Aug 28, 2024

Choose a reason for hiding this comment

adonesky1 Aug 28, 2024

Choose a reason for hiding this comment

adonesky1 Aug 28, 2024

Choose a reason for hiding this comment

Gudahtt commented Aug 28, 2024

codecov bot commented Aug 28, 2024 • edited Loading

Codecov Report

metamaskbot commented Aug 28, 2024

jiexi Aug 28, 2024

Choose a reason for hiding this comment

jiexi Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

adonesky1 Aug 28, 2024

Choose a reason for hiding this comment

jiexi left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 28, 2024

Quality Gate passed

metamaskbot commented Aug 28, 2024

fix: `wallet_addEthereumChain` does not attach a `result` under certain conditions #26726

fix: `wallet_addEthereumChain` does not attach a `result` under certain conditions #26726

adonesky1 commented Aug 28, 2024 •

edited

Loading

Gudahtt commented Aug 28, 2024 •

edited

Loading

codecov bot commented Aug 28, 2024 •

edited

Loading

jiexi Aug 28, 2024 •

edited

Loading