Skip to content

cherrypick: fix: wallet_addEthereumChain does not attach a result under certain conditions (#26726)#26733

Merged
adonesky1 merged 1 commit intoVersion-v12.1.1from
ad/add-wallet_addEthereumChain-fix
Aug 29, 2024
Merged

cherrypick: fix: wallet_addEthereumChain does not attach a result under certain conditions (#26726)#26733
adonesky1 merged 1 commit intoVersion-v12.1.1from
ad/add-wallet_addEthereumChain-fix

Conversation

@adonesky1
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 commented Aug 28, 2024

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.

Open in GitHub Codespaces

Related issues

Fixes: #26706

Manual testing steps

  1. Go to https://chainlist.org/?chain=56&search=blast
  2. 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)
  3. After successfully adding the network, attempt to add Blast again. Nothing should happen.
  4. Then go to https://faucet.quicknode.com/blast/sepolia (or any dapp where your wallet isn't already connected) and attempt to connect
  5. 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

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.

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

…in conditions (#26726)

## **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.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26726?quickstart=1)

## **Related issues**

Fixes: #26706

## **Manual testing steps**

1. Go to https://chainlist.org/?chain=56&search=blast
2. 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)
3. After successfully adding the network, attempt to add Blast again.
Nothing should happen.
4. Then go to https://faucet.quicknode.com/blast/sepolia (or any dapp
where your wallet isn't already connected) and attempt to connect
5. You should be able to connect as usual

## **Screenshots/Recordings**

### **Before**


https://github.com/user-attachments/assets/b997027f-1c62-4279-87c6-0fe70989abb3


### **After**


https://github.com/user-attachments/assets/08307a88-8f6f-44cd-9b75-877aadb1805a


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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
- [ ] 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.
@adonesky1 adonesky1 requested a review from a team as a code owner August 28, 2024 22:16
@github-actions
Copy link
Copy Markdown
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.

@metamaskbot metamaskbot added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Aug 28, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.80%. Comparing base (cd89937) to head (2d4c53c).
Report is 1 commits behind head on Version-v12.1.1.

Additional details and impacted files
@@               Coverage Diff                @@
##           Version-v12.1.1   #26733   +/-   ##
================================================
  Coverage            69.80%   69.80%           
================================================
  Files                 1370     1370           
  Lines                48739    48740    +1     
  Branches             13438    13438           
================================================
+ Hits                 34019    34022    +3     
+ Misses               14720    14718    -2     

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

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [2d4c53c]
Page Load Metrics (66 ± 16 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint59287974722
domContentLoaded95622147
load39191663416
domInteractive95622147

@adonesky1 adonesky1 requested review from Gudahtt and danjm August 29, 2024 14:16
Copy link
Copy Markdown
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! I went through the manual testing steps, and it seems to work as expected

@adonesky1 adonesky1 merged commit 006f607 into Version-v12.1.1 Aug 29, 2024
@adonesky1 adonesky1 deleted the ad/add-wallet_addEthereumChain-fix branch August 29, 2024 16:50
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants