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]: Queued request controller not working as expected in mmi build #28441

Open
hjetpoluru opened this issue Nov 13, 2024 · 1 comment
Open

[Bug]: Queued request controller not working as expected in mmi build #28441

hjetpoluru opened this issue Nov 13, 2024 · 1 comment
Labels
regression-main Regression bug that was found on main branch, but not yet present in production release-12.7.0 Issue or pull request that will be included in release 12.7.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform type-bug Something isn't working

Comments

@hjetpoluru
Copy link
Contributor

Describe the bug

I was trying to fix the flaky test for the switchEthereum function, and there is inconsistent behavior where switching to the network does not happen.

Github ticket - #28195
Test Path - /test/e2e/json-rpc/switchEthereumChain.spec.js
Test scenario - Switch Ethereum Chain for two dapps queues switchEthereumChain request from second dapp after send tx request

Thanks to @seaona for mentioning about the defect #27977.
Later, I realized that Alex has made a fix #28371 and linking the Slack discussion in v12.7.0 RC Thread but it requires additional effort for the mmi I build due to the code fence.
Hence, I have created this ticket for tracking purposes.

Expected behavior

No response

Screenshots/Recordings

Screen.Recording.2024-11-13.at.9.40.57.AM.mov

Steps to reproduce

Perquisite: mmi build is needed

  1. Open the extension app and Toggle off the request queue setting.
  2. Open dApp with the Ethereum network
  3. Initiate a send transaction on the dApp.
  4. Switch to dApp to localhost using the switchNetwork command below in console log

await window.ethereum.request({"method":"wallet_switchEthereumChain",params: [{ chainId: '0x539' }],})
5. In the send transaction, switch and confirm the queued notification for switchEthereumChain.

Error messages or log output

No response

Detection stage

On the development branch

Version

12.7.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response
@hjetpoluru hjetpoluru added type-bug Something isn't working Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform labels Nov 13, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Nov 13, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Nov 13, 2024
@hjetpoluru hjetpoluru added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Nov 13, 2024
@metamaskbot metamaskbot added the regression-main Regression bug that was found on main branch, but not yet present in production label Nov 13, 2024
@adonesky1
Copy link
Contributor

Hmmm so this "bug" is happening on the normal extension build too, but I think instead of "fixing" it we should
a) switch this test to not turn off queueing
b) accelerate removing the toggle to turn off queuing which we've meant to do for a while now.

This was referenced Nov 20, 2024
Merged
Draft
adonesky1 added a commit to MetaMask/core that referenced this issue Nov 22, 2024
@adonesky1
chore: Remove useRequestQueue toggle to turn on/off Per Dapp Select…
4814cf1 
…ed Network Feature (#4941)

## Explanation

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes the togglability of per dapp selected network from the
`SelectedNetworkController` and `QueuedRequestMiddleware`

The extension to integrate this change will also have to implement a
migration to remove this state.

Another reason we should remove this now is that having this setting
turned off is [causing a
bug](MetaMask/metamask-extension#28441) to
result with `wallet_switchEthereumChain` and the interaction between the
new chain permissions feature and the absence of queuing.

## References

Extension PR w/ preview builds of this PR (plus full removal of the
toggle): MetaMask/metamask-extension#28577

When integrated in the extension will resolve this issue:
MetaMask/metamask-extension#28441

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/queued-request-controller`

- **REMOVED**: `createQueuedRequestMiddleware` no longer expects a
`useRequestQueue` property in its param options.


### `@metamask/selected-network-controller`

- **REMOVED**: SelectedNetworkController constructure no longer expects
either `useRequestQueuePreference` or `onPreferencesStateChange` params

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
@adonesky1 adonesky1 mentioned this issue Dec 17, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Jan 9, 2025
@adonesky1
chore: Remove toggle to turn on/off Per Dapp Selected Network Feature (…
4cbce35 
…#29301)

## **Description**

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes this toggle ~and integrates new versions of the
QueuedRequestController and SelectedNetworkController which remove the
backend logic it operated.~
- We have delayed the updates to the controllers side because of some
mobile side requirements. See [this
PR](MetaMask/core#5065 (comment)) for
more context:

> We are not yet ready to release per-dapp selected network
functionality on the mobile client and with this change there is no
clean way to
MetaMask/metamask-mobile#12434 (comment)
in the mobile client without having the Domains state starting to
populate and possibly become corrupt since its not being consumed
by/updated by the frontend in the expected way and may need to be
migrated away when its time to actually start using the controller.

> Without this revert the @MetaMask/wallet-framework team is blocked
from completing their goal to get both clients up to the latest versions
of all controllers.

Beyond the fact that this removal is overdue, another reason we should
remove this now is that having this setting when turned off is [causing
a bug](#28441) with
`wallet_switchEthereumChain` and the interaction with the new chain
permissions feature.

## **Related issues**

Fixes: #2844

## **Manual testing steps**

1. Go to experimental tab of settings
2. See that there is no longer a toggleable preference called "Selected
Networks for each site"
3. See that Per Dapp Selected Network Functionality is still on by
default

## **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/main/.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/main/.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 release-12.7.0 Issue or pull request that will be included in release 12.7.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform type-bug Something isn't working
Projects
Bugs by severity
Status: To be fixed
Bugs by team
Status: To be fixed
Milestone
No milestone
Development

No branches or pull requests
3 participants
@adonesky1 @metamaskbot @hjetpoluru