fix(AssetController): refactor Snap data source#7764
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.
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.
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
bbc3ea1 to
92d01fb
Compare
Prithpal-Sooriya
left a comment
There was a problem hiding this comment.
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?
| const configuredNetworks = | ||
| options.configuredNetworks ?? ALL_DEFAULT_NETWORKS; | ||
|
|
||
| super(SNAP_DATA_SOURCE_NAME, { |
There was a problem hiding this comment.
Not necessarily Snaps-related, but I think you should loop in @mcmire to look at the DataSource architecture. We should ensure that this pattern is aligned with the approach we generally want for data services.
There was a problem hiding this comment.
@FrederikBolding: Thanks for the mention, @Kriys94 messaged me privately about the DataSource code in AssetsController code. I'm looking through it now to understand how it all fits together. @Kriys94: I'll let you know when I have something to share.
There was a problem hiding this comment.
I have a diagram about what we are trying to build https://consensyssoftware.atlassian.net/wiki/spaces/PRDC/pages/400284352527/Architecture+proposal+34+AssetsController+the+cross-network-asset-type+controller#High-Level-Architecture
Here we call "data source" a piece of logic that extends the AssetsController and implements a specific interface to work with external services (Backend REST APIs, Snap, RPC). The code is related to the business logic, here assets balance, price and metadata.
For @mcmire and this ticket https://consensyssoftware.atlassian.net/browse/WPC-26, I think it refers more to a new service/client I just implemented in core-backend https://github.com/MetaMask/core/tree/main/packages/core-backend#http-api . The code in core-backend is agnostic form business logic.
There was a problem hiding this comment.
In any case, feel free to comment on the overall code :) It's not released yet so making a braking change is easy :)
92d01fb to
82d66c0
Compare
82d66c0 to
1d3ed84
Compare
1d3ed84 to
ad81bb1
Compare
| const cachedClient = this.#keyringClientCache.get(snapId); | ||
| if (cachedClient) { | ||
| return cachedClient; | ||
| } |
15b4bd4 to
ad81bb1
Compare
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
High Risk
High risk because it changes SnapDataSource discovery/routing behavior and removes the
snapProviderdependency and many exported constants/types, which may break consumers and alter which chains/accounts get queried for balances.Overview
Refactors
SnapDataSourceto use dynamic snap discovery instead of a hardcoded Solana/Bitcoin/Tron registry: it now queriesSnapController:getRunnableSnapsandPermissionController:getPermissionsto build achainToSnapmap from theendowment:assetschainIdscaveat, and keepsactiveChainsempty until discovery succeeds (re-running onPermissionController:stateChange).Simplifies balance fetching to be account-driven by using
account.metadata.snap.idand calling snap keyring methods viaKeyringClient(@metamask/keyring-snap-client) overSnapController:handleRequest, plus updates event filtering and cleanup/unsubscribe logic accordingly.Updates initialization to delegate the new Snap/Permission controller actions/events, removes
snapProviderfrominitDataSourcesoptions/tests, trims Snap exports fromindex.ts, and adds new dependencies/tsconfig references for permission/keyring snap support.Written by Cursor Bugbot for commit 0376a35. This will update automatically on new commits. Configure here.