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.

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

fix: remove methods from array used to determine which requests should be enqueued because they can be safely passed through #27315

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Sep 20, 2024

Description

Fix issues that arise when a STX is initated in a dapp and subsequent method calls were being unnecessarily queued until the STX was complete.

The following methods can be safely removed from the list of methods we use to determine whether a request should be queued or executed immediately:

  • 'wallet_addEthereumChain'
  • 'wallet_requestPermissions',
  • 'wallet_requestSnaps',
  • 'eth_decrypt',
  • 'eth_requestAccounts',
  • 'eth_getEncryptionPublicKey',

Open in GitHub Codespaces

Related issues

Fixes: #27098

Manual testing steps

  1. Make sure you have STX enabled from settings
  2. Navigate to https://docs.metamask.io/wallet/reference/eth_sendtransaction/
  3. Connect the wallet and switch networks to Sepolia
  4. Trigger a TX (call run request)
  5. Confirm the transaction and see the STX pending screen
  6. Go to test dapp
  7. Click Connect --> this needs to happen while STX is pending
  8. See that you are able to connect

Screenshots/Recordings

Before

Before-wallet_requestSnaps.mov

After

wallet_requestPermissions:

Screen.Recording.2024-09-20.at.1.11.57.PM.mov

wallet_requestSnaps

Screen.Recording.2024-09-20.at.1.41.04.PM.mov

eth_requestAccounts

Screen.Recording.2024-09-20.at.1.14.44.PM.mov

wallet_addEthereumChain

Screen.Recording.2024-09-20.at.1.01.45.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.

Copy link
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.

@adonesky1 adonesky1 changed the title Remove methods from array used to determine which requests should be enqueued because they can be safely passed through Fix: remove methods from array used to determine which requests should be enqueued because they can be safely passed through Sep 20, 2024
@adonesky1 adonesky1 changed the title Fix: remove methods from array used to determine which requests should be enqueued because they can be safely passed through fix: remove methods from array used to determine which requests should be enqueued because they can be safely passed through Sep 20, 2024
@jiexi
Copy link
Contributor

jiexi commented Sep 20, 2024

LGTM after the eth_accounts special handling is removed from shouldEnqueueRequest

@adonesky1 adonesky1 marked this pull request as ready for review September 20, 2024 19:38
@adonesky1 adonesky1 requested a review from a team as a code owner September 20, 2024 19:38
@adonesky1 adonesky1 force-pushed the ad/fix-unnecessary-enqueuing branch 2 times, most recently from 0d5c9e6 to 37667fa Compare September 23, 2024 20:08
jiexi
jiexi previously approved these changes Sep 23, 2024
dbrans
dbrans previously approved these changes Sep 24, 2024
Copy link
Contributor

@dbrans dbrans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adonesky1, fantastic investigation and documentation of the changes!

@adonesky1 adonesky1 dismissed stale reviews from dbrans and jiexi via e7d6712 September 24, 2024 15:36
Copy link

sonarcloud bot commented Sep 24, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [e7d6712]
Page Load Metrics (1858 ± 122 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint56627021795375180
domContentLoaded155525191802216104
load156327251858254122
domInteractive156732189
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -322 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@adonesky1 adonesky1 merged commit 63c5da7 into develop Sep 24, 2024
77 checks passed
@adonesky1 adonesky1 deleted the ad/fix-unnecessary-enqueuing branch September 24, 2024 16:37
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
@metamaskbot metamaskbot added release-12.6.0 Issue or pull request that will be included in release 12.6.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 24, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-12.3.0 on PR. Adding release label release-12.3.0 on PR and removing other release labels(release-12.6.0), as PR was cherry-picked in branch 12.3.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-wallet-api-platform
Projects
None yet
4 participants