Skip to content

feat: Use AccountGroup metadata for sorting in core permission helpers#29295

Open
jiexi wants to merge 46 commits into
mainfrom
jl/use-account-group-metadata-for-sorting-in-core-permission
Open

feat: Use AccountGroup metadata for sorting in core permission helpers#29295
jiexi wants to merge 46 commits into
mainfrom
jl/use-account-group-metadata-for-sorting-in-core-permission

Conversation

@jiexi
Copy link
Copy Markdown
Member

@jiexi jiexi commented Apr 23, 2026

Description

Replaces EVM InternalAccount based sorting with AccountGroup based sorting in the core permission helpers.

NOTE: This does not update the selectors which still rely on the EVM InternalAccount

Changelog

CHANGELOG entry: null

Related issues

Fixes:

Manual testing steps

Nothing should be different. Use the IAB to connect to a dapp, then change the selected account around and ensure the ordering is correct

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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.

Note

Medium Risk
Changes account-ordering behavior used when returning/announcing permitted accounts and removes a delay in wallet_sessionChanged emission, which could affect dapp-observed ordering/timing across lock/unlock and account-group changes.

Overview
Updates permission helpers to sort permitted EVM and CAIP-10 account lists using AccountTreeController/AccountGroup metadata.lastSelected (with caching and fallbacks) instead of InternalAccount metadata, and removes the sortEvmAccountsByLastSelected path.

BackgroundBridge.handleSessionChangedFromSelectedAccountGroupChanges now emits wallet_sessionChanged immediately using the current CAIP-25 caveat (no setTimeout/refetch), and tests/mocks are updated to cover the new sorting behavior, fallback cases, and error handling.

Reviewed by Cursor Bugbot for commit 94f075e. Bugbot is set up for automated code reviews on this repo. Configure here.

@metamaskbotv2 metamaskbotv2 Bot added the team-wallet-integrations Wallet Integrations team label Apr 23, 2026
Comment thread app/core/Permissions/index.ts
@jiexi jiexi requested a review from a team as a code owner April 23, 2026 22:02
Base automatically changed from jl/WAPI-1387/multichain-api-accounts-ordering to main April 28, 2026 14:59
@adonesky1 adonesky1 dismissed their stale review April 28, 2026 14:59

The base branch was changed.

@jiexi jiexi requested a review from a team as a code owner April 28, 2026 14:59
Comment thread app/core/Permissions/index.test.ts Outdated
Comment thread app/core/Permissions/index.test.ts Outdated
Comment thread app/core/Permissions/index.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can remove if in fact it's unused

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread app/core/Permissions/index.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can remove if in fact it's unused

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

jiexi and others added 2 commits April 29, 2026 15:57
Co-authored-by: ffmcgee <51971598+ffmcgee725@users.noreply.github.com>
Co-authored-by: ffmcgee <51971598+ffmcgee725@users.noreply.github.com>
Comment thread app/core/Permissions/index.ts
adonesky1
adonesky1 previously approved these changes May 4, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2341564. Configure here.

Comment thread app/core/BackgroundBridge/BackgroundBridge.test.js Outdated
jest.advanceTimersByTime(1000);
expect(PermissionController.getCaveat).toHaveBeenCalledTimes(1);
expect(notifySpy).toHaveBeenCalledWith(mockCaveatValue);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test name contradicts its actual assertion

Medium Severity

The test is named "does not call notifyCaipAuthorizationChange if the caveat is revoked before the setTimeout fires" but line 1830 asserts expect(notifySpy).toHaveBeenCalledWith(mockCaveatValue), confirming it WAS called. Since the setTimeout was removed and the function now fires synchronously, the assertion body is correct for the new behavior, but the test name claims the opposite — providing false documentation of the contract.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2341564. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeMultiChainAPI, SmokeNetworkExpansion, SmokeNetworkAbstractions, SmokeConfirmations, SmokeBrowser
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: high
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes touch two critical core files:

  1. BackgroundBridge.js: Removes a 1-second setTimeout workaround in handleSessionChangedFromSelectedAccountGroupChanges. Previously, the code waited 1 second and re-fetched the CAIP-25 caveat before calling notifyCaipAuthorizationChange. Now it calls synchronously with the initially-fetched caveat. This directly affects how dApps receive account/session change notifications via CAIP-25, impacting multi-chain session management.

  2. Permissions/index.ts: Major refactoring of account sorting logic. The sortEvmAccountsByLastSelected export is removed and replaced with a private sortAddressesByLastSelected that uses AccountTreeController (via getAccountContext and getAccountGroupObject) instead of AccountsController.listAccounts. The new implementation gracefully handles missing accounts (returns 0 rank) instead of throwing errors. This affects how permitted accounts are ordered when returned to dApps via eth_accounts and CAIP-25 sessions.

Affected E2E areas:

  • SmokeMultiChainAPI: CAIP-25 session management (wallet_createSession, wallet_getSession, wallet_sessionChanged) is directly affected by both changes. The synchronous notification change and account sorting changes affect session state reporting.
  • SmokeNetworkExpansion: Solana dApp connections use BackgroundBridge for account notifications and CAIP-25 sessions. Account switching notifications to dApps are affected.
  • SmokeNetworkAbstractions: Chain permission system uses getPermittedAccounts which now uses the refactored sorting logic.
  • SmokeConfirmations: Required as dependent tag when selecting SmokeNetworkExpansion for Solana flows.
  • SmokeBrowser: dApp connections go through BrowserTab → BackgroundBridge. The account permission sorting affects what accounts are exposed to dApps in the browser.

Dependent tags per descriptions:

  • SmokeMultiChainAPI → SmokeNetworkAbstractions + SmokeNetworkExpansion
  • SmokeNetworkExpansion (Solana) → SmokeConfirmations
  • SmokeNetworkAbstractions → SmokeNetworkExpansion + SmokeMultiChainAPI

Performance Test Selection:
The changes are focused on correctness fixes (removing a setTimeout workaround, refactoring account sorting logic) rather than performance-sensitive rendering or data loading paths. The removal of the 1-second setTimeout actually improves responsiveness but doesn't affect the performance test scenarios which measure UI rendering, account list loading, login times, etc. No performance test tags are warranted.

View GitHub Actions results

@sonarqubecloud
Copy link
Copy Markdown

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

Labels

size-L team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants