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: fix ConnectPage when a non-EVM account is selected #28436

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Nov 13, 2024

Description

The currently selected non-EVM account was part of the requested account on during the account connection flow. The ConnectPage was allowing to "confirm" the request even if the UI was showing 0 account.

To fix this, we no longer uses the currently selected account if this account is not EVM-compatible.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/670

Manual testing steps

  1. yarn start:flask
  2. Enable Bitcoin support
  3. Create a Bitcoin account
  4. Select this account
  5. Go to: https://metamask.github.io/test-dapp/
  6. Try to connect your accounts
    • None accounts should be selected by default
    • You should not be able to "confirm" the dialog

Screenshots/Recordings

Before

Screenshot 2024-11-13 at 14 57 11

If you try to connect, you will get:

Screenshot 2024-11-13 at 14 57 44

After

Screenshot 2024-11-13 at 15 01 39

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.

@ccharly ccharly self-assigned this Nov 13, 2024
@ccharly ccharly requested a review from a team as a code owner November 13, 2024 13:02
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.

@metamaskbot
Copy link
Collaborator

Builds ready [3a8d449]
Page Load Metrics (1898 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17172180189811354
domContentLoaded17042170186710450
load17162183189811455
domInteractive21256655727
backgroundConnect779332110
firstReactRender8911910173
getState56121199
initialActions01000
loadScripts1222167013709345
setupStore65612147
uiStartup19122393210112359
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. Not approving yet since there are unresolved comments

@metamaskbot
Copy link
Collaborator

Builds ready [6b8c02a]
Page Load Metrics (2231 ± 119 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28030731936719345
domContentLoaded187230622193251120
load192330812231247119
domInteractive29151573115
backgroundConnect980352311
firstReactRender782761214522
getState781282512
initialActions01000
loadScripts134423471606213102
setupStore65816168
uiStartup216633672490267128
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [a9c8cfb]
Page Load Metrics (2031 ± 242 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39526621676567272
domContentLoaded162537302005499240
load164737552031504242
domInteractive23142533416
backgroundConnect9101332813
firstReactRender682411254019
getState45213126
initialActions01000
loadScripts115029861481428205
setupStore675222311
uiStartup184240832289552265
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@danroc danroc added this pull request to the merge queue Nov 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 25, 2024
@ccharly ccharly added this pull request to the merge queue Nov 25, 2024
Merged via the queue into develop with commit eeb085f Nov 25, 2024
75 checks passed
@ccharly ccharly deleted the fix/non-evm-connect-page branch November 25, 2024 17:41
@github-actions github-actions bot locked and limited conversation to collaborators Nov 25, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants