Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Wallet API - When I'm conneting to the test dapp for the first time, I'm switched to Mainnet automatically, despite not having this network selected #27891

Closed
seaona opened this issue Oct 16, 2024 · 2 comments · Fixed by #27980
Closed

[Bug]: Wallet API - When I'm conneting to the test dapp for the first time, I'm switched to Mainnet automatically, despite not having this network selected #27891

seaona opened this issue Oct 16, 2024 · 2 comments · Fixed by #27980
Labels
regression-main Regression bug that was found on main branch, but not yet present in production regression-RC-12.7.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 release-12.7.0 Issue or pull request that will be included in release 12.7.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform team-wallet-ux type-bug

Comments

@seaona
Copy link
Contributor

seaona commented Oct 16, 2024 •
edited
Loading

Describe the bug

When I'm conneting to the test dapp for the first time, I'm switched to Mainnet automatically, despite not having this network selected.
This is not happening in current production, but it has changed recently (happening on develop, meaning it will appear in 12.7.0).
This also makes fail the snaps test very frequently for this reason, see here.

Expected behavior

Preserve my selected network, like we did before?

Screenshots/Recordings

See current behaviour (switched automatically) and production behaviour (preserving my selected network)

switched-to-not-selected-network.mp4

Steps to reproduce

  1. Select localhost network
  2. Go to the test dapp
  3. Click connect
  4. Connect
  5. See you are switched to Mainnet automatically

Error messages or log output

No response

Detection stage

On the development branch

Version

12.7.0

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Oct 16, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Oct 16, 2024
@seaona seaona added the release-blocker This bug is blocking the next release label Oct 16, 2024
@seaona seaona mentioned this issue Oct 16, 2024
Merged
7 tasks
@seaona seaona added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 16, 2024
@metamaskbot metamaskbot added the regression-main Regression bug that was found on main branch, but not yet present in production label Oct 16, 2024
@seaona seaona added regression-RC-12.7.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform team-wallet-ux labels Oct 16, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 16, 2024
@seaona
fix: flaky test `Snap Account Signatures and Disconnects can connect …
bb2e2a9 
…to the Test Dapp, then #signTypedDataV3, disconnect then connect, then #signTypedDataV4 (async flow approve)` (#27887)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
After connecting to the test dapp we are automatically switched to
Mainnet (this is a recent change, didn't happen before). Then there is a
race condition where the network the dapp sees is different from the
wallet one, causing a miss-match and making the signature fail never
opening the dialog.

This seems an issue on the wallet side, as we should preserve the
selected network after connecting, as we used to do before.

I've opened an issue for that
[here](#27891).

This just fixes the issue on the e2e side, but it needs to be fixed on
the wallet side and remove the extra step again once the issue is fixed.

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

## **Related issues**

Fixes: #27892

## **Manual testing steps**

1. Check ci


## **Screenshots/Recordings**
See how you are automatically switched to mainnet


https://github.com/user-attachments/assets/d8fa53f5-b207-46bc-8e54-073245eff7b7



## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] 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.
@jiexi
Copy link
Contributor

jiexi commented Oct 18, 2024

In the first part of the reproduction when you are connecting, only mainnet and linea are selected. For ganache to remain the selected network for the dapp after connection, ganache must also be selected at the time of connection

@seaona
Copy link
Contributor Author

seaona commented Oct 18, 2024 •
edited
Loading

hey @jiexi , from the slack thread, the solution should be:

Feels like an easy win to add the currently selected network to the list of preselected networks. Also makes it seem less like a regression / more backwards compatible

@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
@NidhiKJha NidhiKJha mentioned this issue Oct 23, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Oct 29, 2024
@NidhiKJha
feat: added test network as selected network if globally selected for…
03196a8 
… connection Request (#27980)

This PR is to select a test network in the default selected networks
list if its the globally selected network at the time of connection
request.

## **Related issues**

Fixes:
[#27891](#27891)
## **Manual testing steps**

1. Run extension with yarn start
2. Switch to Sepolia
3. Go to test-dapp, click on connect. 
4. In the connections page, check Sepolia is the selected along with non
test networks
5. Click confirm, dapp should be connected to MM and user should be on
Sepolia network in MM.

## **Screenshots/Recordings**


### **Before**



https://github.com/user-attachments/assets/127fc7bb-2e68-411a-b407-7f6d5158e911


### **After**



https://github.com/user-attachments/assets/dd0b5aa6-404a-421f-93a4-67cab43d60c6


## **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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Oct 29, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Oct 29, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 29, 2024
NidhiKJha added a commit that referenced this issue Oct 29, 2024
@NidhiKJha
feat: added test network as selected network if globally selected for…
1eb190f 
… connection Request (#27980)

This PR is to select a test network in the default selected networks
list if its the globally selected network at the time of connection
request.

## **Related issues**

Fixes:
[#27891](#27891)
## **Manual testing steps**

1. Run extension with yarn start
2. Switch to Sepolia
3. Go to test-dapp, click on connect. 
4. In the connections page, check Sepolia is the selected along with non
test networks
5. Click confirm, dapp should be connected to MM and user should be on
Sepolia network in MM.

## **Screenshots/Recordings**


### **Before**



https://github.com/user-attachments/assets/127fc7bb-2e68-411a-b407-7f6d5158e911


### **After**



https://github.com/user-attachments/assets/dd0b5aa6-404a-421f-93a4-67cab43d60c6


## **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.
@NidhiKJha NidhiKJha mentioned this issue Oct 29, 2024
Merged
14 tasks
danjm pushed a commit that referenced this issue Oct 30, 2024
@NidhiKJha
feat (cherry-pick): added test network as selected network if globall…
ac1173b 
…y selected for… (#28139)

… connection Request (#27980)

This PR is to select a test network in the default selected networks
list if its the globally selected network at the time of connection
request.

## **Related issues**

Fixes:
[#27891](#27891) ##
**Manual testing steps**

1. Run extension with yarn start
2. Switch to Sepolia
3. Go to test-dapp, click on connect. 
4. In the connections page, check Sepolia is the selected along with non
test networks
5. Click confirm, dapp should be connected to MM and user should be on
Sepolia network in MM.

## **Screenshots/Recordings**


### **Before**




https://github.com/user-attachments/assets/127fc7bb-2e68-411a-b407-7f6d5158e911


### **After**




https://github.com/user-attachments/assets/dd0b5aa6-404a-421f-93a4-67cab43d60c6


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


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

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

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
regression-main Regression bug that was found on main branch, but not yet present in production regression-RC-12.7.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 release-12.7.0 Issue or pull request that will be included in release 12.7.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform team-wallet-ux type-bug
Projects
Bugs by severity
Archived in project
Milestone
No milestone
4 participants
@jiexi @gauthierpetetin @metamaskbot @seaona