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 permissions not correctly being updated when all network clients are removed for a chainId #29855

Merged

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jan 22, 2025

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.

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.

@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
Collaborator

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%)

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

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%)

Copy link
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
71 checks passed
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.12.0 Issue or pull request that will be included in release 12.12.0 team-wallet-api-platform team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants