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: eth_accounts / accountsChanged behavior when wallet is locked #30067

Conversation

ffmcgee725
Copy link
Member

@ffmcgee725 ffmcgee725 commented Feb 3, 2025

Description

Refer to [#3853](https://github.com/MetaMask/MetaMask-planning/issues/3853)

-->

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page
  2. Open dev tools
  3. Enter await window.ethereum.on('accountsChanged', console.log) on console
  4. Lock / unlock wallet and make sure event is not emitted

Screenshots/Recordings

Before

After

Screen.Recording.2025-02-03.at.15.44.50.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.

@metamaskbot
Copy link
Collaborator

Builds ready [5e5f41e]
Page Load Metrics (1709 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14892001171014469
domContentLoaded14771978168013665
load14911990170914368
domInteractive2691452311
backgroundConnect1172302110
firstReactRender1689332512
getState57316199
initialActions01000
loadScripts10361428122011756
setupStore891282613
uiStartup172126301990210101
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -89 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2025
}
return []; // changing this is a breaking change
return this.getPermittedAccounts(innerOrigin);
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is breaking and will result in unintended behavior. We still need to return empty array for accounts in this hook when the wallet is locked

Copy link
Member Author

Choose a reason for hiding this comment

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

@adonesky1 can you chime in here ?

@ffmcgee725 ffmcgee725 reopened this Feb 3, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [5e5f41e]
Page Load Metrics (1709 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14892001171014469
domContentLoaded14771978168013665
load14911990170914368
domInteractive2691452311
backgroundConnect1172302110
firstReactRender1689332512
getState57316199
initialActions01000
loadScripts10361428122011756
setupStore891282613
uiStartup172126301990210101
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -89 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [418f591]
Page Load Metrics (1605 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14541910160712359
domContentLoaded14151887158411957
load14531957160512660
domInteractive24124413014
backgroundConnect97825199
firstReactRender1489402813
getState45111136
initialActions01000
loadScripts9601395113711957
setupStore65615168
uiStartup16422118185514469
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -89 Bytes (-0.00%)
  • ui: -367 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

@ffmcgee725 ffmcgee725 closed this Feb 4, 2025
@ffmcgee725 ffmcgee725 deleted the fix/locked-wallet-behaviour-eth_accounts-accountsChanged branch February 4, 2025 19:32
@ffmcgee725
Copy link
Member Author

ffmcgee725 commented Feb 4, 2025

closed, as #29936 supersedes this work

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants