fix(assets-controller): improve SnapDataSource chain discovery and balance delivery#8857
fix(assets-controller): improve SnapDataSource chain discovery and balance delivery#8857salimtb wants to merge 2 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
1 similar comment
|
@metamaskbot publish-preview |
Fixes three related bugs that caused snap chain balances (Tron, Bitcoin, Solana, etc.) to never appear in AssetsController state. **Bug 1 — Chain discovery ignored `endowment:keyring` chainIds caveat** `#discoverKeyringSnaps` only read `chainIds` from `endowment:assets`. Snaps that declare supported chains solely on `endowment:keyring` (e.g. many wallet snaps) produced an empty `activeChains` after discovery. Fix: `#getChainIdsFromSnapPermissions` now prefers `endowment:assets` and falls back to `endowment:keyring` when the assets caveat is absent. **Bug 2 — Snap subscriptions never created (activeSubscriptions always 0)** `#subscribeAssetsBalance` in AssetsController passed only `#enabledChains` (EVM networks from NetworkController) to `#buildChainToAccountsMap`. Snap chains (`bip122:*`, `solana:*`, `tron:*`) were never in that set, so `remainingChains` had no overlap with SnapDataSource's `activeChains` → `assignedChains.length === 0` every call → SnapDataSource was immediately unsubscribed → `activeSubscriptions` stayed 0 and every push balance event was dispatched into nothing. Fix: augment the chain set with `snapDataSource.getActiveChainsSync()` so snap chains enter the assignment loop and are routed to SnapDataSource. **Bug 3 — Snaps with no `chainIds` caveat silently dropped (e.g. Tron)** When a snap has `endowment:keyring` but no `chainIds` caveat on either permission, discovery returns `null` and skips the snap. Tron balances arrived with `chainAllowed=false` and were discarded on every event. Fix: `#handleSnapBalancesUpdated` now detects a non-EVM chain that arrived after discovery completed but isn't registered, adds it to `activeChains` via `updateActiveChains`, and triggers `onActiveChainsUpdated` to re-subscribe AssetsController. EVM chains are explicitly excluded from this path — they belong to RpcDataSource. **Additional fixes** - Retry discovery in `#handleSnapBalancesUpdated` when a balance push arrives before `SnapController`/`PermissionController` are ready. - Fail-open in `subscribe` when discovery has not completed: register the subscription using requested chains so push events can be delivered once discovery finishes. - Subscribe to `PermissionController:stateChanged` (correct BaseController v2 event) instead of the deprecated `stateChange`.
salimtb
force-pushed
the
fix/snap-data-source-keyring-chain-discovery
branch
from
0fd18ce to
d219a72
Compare
| const chainToAccounts = this.#buildChainToAccountsMap( | ||
| accounts, | ||
| new Set(chainIds), | ||
| new Set([...chainIds, ...snapActiveChains]), |
There was a problem hiding this comment.
is this change needed?
There was a problem hiding this comment.
no i don't think so , i'm cleaning now
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 d219a72. Configure here.
| // Guard: never learn EVM chains — those belong to RpcDataSource. | ||
| if (discoveryReady && !chainAllowed && !chainId.startsWith('eip155:')) { | ||
| const previous = [...this.state.activeChains]; | ||
| this.state.chainToSnap[chainId] = 'discovered-from-event'; |
There was a problem hiding this comment.
Sentinel discovered-from-event breaks fetch routing for learned chains
Medium Severity
When a balance event arrives for an undiscovered non-EVM chain, chainToSnap[chainId] is set to the literal string 'discovered-from-event'. This sentinel never matches any real snap ID, so fetch() always skips accounts for these chains (the check this.state.chainToSnap[chainId] === snapId fails). The code explicitly triggers re-subscription via onActiveChainsUpdated, but the fetch call within that subscription returns empty results because routing is broken. Only push-based updates work, leaving initial and update fetches data-less for these chains.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d219a72. Configure here.
There was a problem hiding this comment.
change is a bit larger than I initially thought. I thought the hotfix would just be.
// Rediscover keyring snaps when any snap gets installed
this.messenger.subscribe('SnapController:snapInstalled', () => {
this.#discoverKeyringSnaps();
})There was a problem hiding this comment.
this will only fix the timing race condition , when i added logs i found more than just that , happy to keep it small but keep in mind that
- tron had no chainIds caveat on either permission
- activeSubscriptions=0 even for chains that were correctly discovered
the fact that chainId is not correctly received can cause other issues
There was a problem hiding this comment.
Confirmed, this is also a valid fix - unless we need these additional checks?
There was a problem hiding this comment.
Hmm okay... just cautious on complexity increase. If this is valid, happy to keep... but wondering if there is smaller changes needed or required.
Just feels like death by 1000 cuts, we keep piling on more and more complexity for every edge case, and this feels impossible to reason the full system with.
3 participants
Read chainIds from endowment:keyring when assets permission lacks them, retry discovery on balance pushes, fail-open until activeChains populate, and allow subscriptions before discovery so snap balance events reach consumers. Subscribe to PermissionController:stateChanged. Remove debug console logging.
Explanation
References
Checklist
Note
Medium Risk
Adjusts snap chain discovery and subscription behavior for non-EVM balances, which can change when/which balance updates propagate and how chains are assigned between Snap and RPC sources.
Overview
Improves
SnapDataSourceresiliency so non-EVM snap balances are delivered even when snap/permission discovery is delayed or incomplete. Discovery now readschainIdsfromendowment:keyringwhen missing onendowment:assets, retries discovery on earlyAccountsController:accountBalancesUpdatedevents, and preserves last known-good discovery state on rediscovery failures.Subscriptions now fail-open before discovery completes (registering requested chains so push events reach
onAssetsUpdate), and the data source can learn non-EVM chains from balance events (while explicitly excludingeip155:*).AssetsControlleralso includesSnapDataSourceactive chains when building chain→account subscriptions so snap-backed accounts on non-enabled chains still receive balance subscriptions. Tests and changelog were updated accordingly, including requiring delegation ofPermissionController:stateChange.Reviewed by Cursor Bugbot for commit d219a72. Bugbot is set up for automated code reviews on this repo. Configure here.