Skip to content

fix: fix permissions not correctly being updated when all network clients are removed for a chainId#29855

Merged
adonesky1 merged 13 commits intomainfrom
jl/mmp-3735/fix-permissions-removing-only-network-client
Jan 23, 2025
Merged

fix: fix permissions not correctly being updated when all network clients are removed for a chainId#29855
adonesky1 merged 13 commits intomainfrom
jl/mmp-3735/fix-permissions-removing-only-network-client

Conversation

@jiexi
Copy link
Copy Markdown
Member

@jiexi jiexi commented Jan 22, 2025 •
edited by github-actions bot
Loading

Description

Fixes a bug where permissions are not being updated when all network clients for a chainId are removed. This results in permittedChains permissions referencing chainIds that are no longer supported and makes it so that the existing permittedChains permissions cannot be modified in any way until either the permission is revoked entirely or the missing network is readded.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3735

Requires: MetaMask/core#5183

Manual testing steps

  1. Add a new RPC endpoint for a chainId you do not have network clients for already
  2. Add another different RPC endpoint for that same chainId above
  3. Permit a dapp to access that chainId
  4. Check that you can modify the permitted chains vs the wallet UI
  5. Make sure that calling wallet_getPermissions shows the chainId in the endowment:permitted-chains permission
  6. Remove one RPC endpoint for that chainId
  7. Check that you can modify the permitted chains vs the wallet UI
  8. Make sure that calling wallet_getPermissions shows the chainId in the endowment:permitted-chains permission
  9. Remove the chainId entirely
  10. Check that you can modify the permitted chains vs the wallet UI
  11. Make sure that calling wallet_getPermissions shows that the chainId is no longer in the endowment:permitted-chains permission

Screenshots/Recordings

Before

After

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 22, 2025

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 metamaskbot added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Jan 22, 2025
@jiexi jiexi changed the title Fix: fix permissions not correctly being updated when all network clients are removed for a chainId fix: fix permissions not correctly being updated when all network clients are removed for a chainId Jan 22, 2025
@jiexi jiexi marked this pull request as ready for review January 22, 2025 23:57
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 23, 2025

Builds ready [a535556]
Page Load Metrics (1595 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint13741939159414670
domContentLoaded13681916156614469
load13731931159514771
domInteractive217832147
backgroundConnect795312210
firstReactRender16175473718
getState45812147
initialActions01000
loadScripts9501411114211756
setupStore5629126
uiStartup160123781850223107
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 120 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

adonesky1
adonesky1 approved these changes Jan 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 23, 2025

Builds ready [0928320]
Page Load Metrics (1706 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14462132171220599
domContentLoaded14312069168119393
load14452092170619895
domInteractive2492422110
backgroundConnect76328178
firstReactRender1677382211
getState45314147
initialActions01000
loadScripts10471585123716579
setupStore6471194
uiStartup161924241911229110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 120 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Gudahtt
Gudahtt approved these changes Jan 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! Won't we also need a migration to fully resolve this bug though? Certainly we can do it in a separate PR, but I would expect there to be chainIds from removed networks already in permission state.

@adonesky1 adonesky1 added this pull request to the merge queue Jan 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025
@adonesky1 adonesky1 added this pull request to the merge queue Jan 23, 2025
Merged via the queue into main with commit 305ea7e Jan 23, 2025
@adonesky1 adonesky1 deleted the jl/mmp-3735/fix-permissions-removing-only-network-client branch January 23, 2025 21:42
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2025
@metamaskbot metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 23, 2025
@hesterbruikman
Copy link
Copy Markdown

hesterbruikman commented Feb 5, 2025

@jiexi it seems we have the same issue on Mobile. Beyond the controller fix, do you think there needs to be a fix similar to this one on Mobile?

MetaMask/metamask-mobile#12684

cc @sleepytanya

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

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@adonesky1 adonesky1 adonesky1 approved these changes

Assignees

No one assigned

Labels

release-12.12.0 Issue or pull request that will be included in release 12.12.0 team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jiexi @metamaskbot @hesterbruikman @Gudahtt @adonesky1 @desi