-
-
Notifications
You must be signed in to change notification settings - Fork 267
fix(AssetController): refactor Snap data source #7764
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
base: main
Are you sure you want to change the base?
Conversation
7238184 to
58befda
Compare
58befda to
edad4fe
Compare
| const snapSupportsRequestedChains = request.chainIds.some( | ||
| (chainId) => this.state.chainToSnap[chainId] === snapId, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand how this is supposed to work. What if the chainToSnap mapping isn't already populated? Are the inputs BIP-44 account groups or individual accounts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chainToSnap is populated at the initialization of MetaMask, much before that we are fetching assets. So What if the chainToSnap mapping isn't already populated? is unlikely to say impossible.
Are the inputs BIP-44 account groups or individual accounts?
request.accounts is the selected account group
edad4fe to
b6be0e0
Compare
There was a problem hiding this 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 1 potential issue.
9bc40a5 to
bbc3ea1
Compare
bbc3ea1 to
92d01fb
Compare
Prithpal-Sooriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving on first pass. This data source is completely new and not in production (we have time to refine).
Lets have the snaps team to review before merging. Maybe a short loom/recording to walkthrough changes?
Explanation
This PR simplifies and cleans up
SnapDataSourceby:Dynamic snap discovery via PermissionController: Instead of hardcoded snap registry entries,
SnapDataSourcenow dynamically discovers installed snaps that have theendowment:keyringpermission and extracts their supported chain IDs from permission caveats. This makes the data source more flexible and extensible for future snaps.Simplified fetch routing: Changed from complex chain-based routing to account-based routing using
account.metadata.snap.id. This aligns with howMultichainBalancesControllerhandles snap accounts and is more direct since each account already knows which snap it belongs to.Added
KeyringClient: Replaced manualSnapController:handleRequestcalls withKeyringClientfrom@metamask/keyring-snap-client, which provides cleaner typed methods (listAccountAssets,getAccountBalances). This follows the same pattern used inMultichainBalancesController.Removed unused/redundant code:
DEFAULT_SNAP_POLL_INTERVALconstant (never used)#groupChainsBySnapIdmethod (no longer needed with account-based routing)#fetchFromSnapByIdmethod (inlined intofetch)#accountSupportsChainmethod (unused after simplification)fetch(already done by middleware/subscribe callers)InternalAccountimportUsed proper types: Replaced inline type
Record<string, { amount: string; unit: string }>withGetAccountBalancesResponseandBalancefrom@metamask/keyring-api.Dependencies added:
@metamask/keyring-api: ForBalanceandCaipAssetTypetypes@metamask/keyring-snap-client: ForKeyringClientto make typed snap callsReferences
SnapDataSourcedynamic snap discovery refactoringMultichainBalancesControllerandMultichainAssetsControllerimplementationsChecklist
Note
Medium Risk
Moderate risk: replaces core snap discovery/routing logic and the snap-call transport, which can change which chains/accounts are handled and how balances are fetched/filtered.
Overview
SnapDataSourceis refactored from a hardcoded Solana/Bitcoin/Tron registry to dynamic snap discovery: it now queriesSnapController:getRunnableSnapsandPermissionController:getPermissionsto find keyring snaps (endowment:keyring) and derive supportedchainIdsfrom theendowment:assetscaveat, populatingactiveChainsand a newchainToSnapmap.Balance fetching is simplified to be account-driven using
account.metadata.snap.idandKeyringClient(viaSnapController:handleRequest) instead of directwallet_getSnaps/wallet_invokeSnapcalls; related chain-management APIs/exports for predefined networks and snap types are removed. Messenger initialization is updated to delegate the new Snap/Permission controller actions, and tests are rewritten accordingly. Dependencies/tsconfig are updated to include the keyring and permission/snaps packages.Written by Cursor Bugbot for commit 92d01fb. This will update automatically on new commits. Configure here.