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 handling of permissions of removed accounts #8803

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jun 12, 2020

Imported accounts can be removed, but the permissions controller is not informed when this happens. Permissions are now removed as part of the account removal process.

Additionally, the getPermittedIdentitiesForCurrentTab selector now filters out any non-existent accounts, in case a render occurs in the middle of an account removal.

This was resulting in a render crash upon opening the popup on a site that was connected to the removed account.

@Gudahtt Gudahtt force-pushed the fix-handling-of-removed-account-permissions branch from 31b1479 to 592c57d Compare June 12, 2020 22:48
Imported accounts can be removed, but the permissions controller is not
informed when this happens. Permissions are now removed as part of the
account removal process.

Additionally, the `getPermittedIdentitiesForCurrentTab` selector now
filters out any non-existent accounts, in case a render occurs in the
middle of an account removal.

This was resulting in a render crash upon opening the popup on a site
that was connected to the removed account.
@Gudahtt Gudahtt force-pushed the fix-handling-of-removed-account-permissions branch from 592c57d to 879eeef Compare June 13, 2020 02:26
@Gudahtt Gudahtt marked this pull request as ready for review June 13, 2020 02:44
@Gudahtt Gudahtt requested a review from a team as a code owner June 13, 2020 02:44
@metamaskbot
Copy link
Collaborator

Builds ready [879eeef]
Page Load Metrics (685 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318243147
domContentLoaded4158386848038
load4178406858038
domInteractive4158386838038

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Great catch!

@Gudahtt Gudahtt merged commit 6ca18c3 into develop Jun 15, 2020
@Gudahtt Gudahtt deleted the fix-handling-of-removed-account-permissions branch June 15, 2020 13:27
Gudahtt added a commit that referenced this pull request Jun 16, 2020
* origin/develop: (58 commits)
  Fix site icon fallback letter (#8815)
  add hover style to list-item (#8813)
  Fix site icon size (#8814)
  Consolidate connected account alerts (#8802)
  lowercase web3
  Use markdown-to-jsx@6.11.4 (#8809)
  Update app/_locales/en/messages.json
  Update app/_locales/en/messages.json
  also remove 'dapp' from descriptions
  remove all user-facing instances of 'dapp'
  update button styling on home/asset page (#8800)
  Fix handling of permissions of removed accounts (#8803)
  Clear permssions during createNewVaultAndRestore (#8804)
  Hide token transfers on ETH asset page (#8799)
  Fix account name editing (#8801)
  Fix connect flow account list height (#8798)
  Update color of menu item icons (#8797)
  Update "Connected accounts" empty description (#8796)
  Stop reporting failed transactions to Sentry (#8795)
  Omit state snapshot from Sentry errors (#8794)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants