diff --git a/packages/solana-wallet-snap/CHANGELOG.md b/packages/solana-wallet-snap/CHANGELOG.md index 72bfc3a52..e866bf4fb 100644 --- a/packages/solana-wallet-snap/CHANGELOG.md +++ b/packages/solana-wallet-snap/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `keyring_getAccountBalances` now fails when an RPC fetch for a requested network fails, instead of returning a partial or empty balance, so the wallet shows the balance as unavailable rather than stale ([#527](https://github.com/MetaMask/internal-snaps/pull/527)) - Preserve accounts loaded from address lookup tables when decoding transaction instructions, preventing incomplete account lists and parsing errors during transaction history synchronization. ([#568](https://github.com/MetaMask/internal-snaps/pull/568)) ## [7.3.0] diff --git a/packages/solana-wallet-snap/snap.manifest.json b/packages/solana-wallet-snap/snap.manifest.json index f3051dc35..e07f6fb32 100644 --- a/packages/solana-wallet-snap/snap.manifest.json +++ b/packages/solana-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "Dr868Npsp/5wyPjq3W8nLkJF4MDZupuJO8yV8z8aJz4=", + "shasum": "5juA4eLDLJwq36TEKUVSaLNRG5dMuSCMTPLir8kJ1js=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.test.ts b/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.test.ts index 953ffb129..a1d3b9ca4 100644 --- a/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.test.ts +++ b/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.test.ts @@ -20,6 +20,7 @@ import type { KeyringAccountMonitor, TransactionsService, } from '../../services'; +import { AccountAssetsFetchError } from '../../services/assets/adapters/SnapAssetsAdapter'; import type { ConfirmationHandler } from '../../services/confirmation/ConfirmationHandler'; import { DEFAULT_UNENCRYPTED_STATE } from '../../services/state/stateTypes'; import type { UnencryptedStateValue } from '../../services/state/stateTypes'; @@ -396,6 +397,7 @@ describe('SolanaKeyring', () => { expect(mockAssetsService.fetchAccountAssetsFromRpc).toHaveBeenCalledWith( MOCK_SOLANA_KEYRING_ACCOUNT_0, + { failOnNetworks: [MOCK_ASSET_ENTITY_1.assetType.split('/')[0]] }, ); expect(result).toStrictEqual({ [MOCK_ASSET_ENTITY_1.assetType]: { @@ -418,6 +420,30 @@ describe('SolanaKeyring', () => { ).rejects.toThrow('network error'); }); + it('fails instead of answering with a partial result when an RPC fetch for a requested network failed (v3)', async () => { + vi.spyOn( + mockAssetsService, + 'fetchAccountAssetsFromRpc', + ).mockRejectedValue( + new AccountAssetsFetchError([ + { + network: Network.Mainnet, + reason: new TypeError('Failed to fetch'), + }, + ]), + ); + + await expect( + keyring.getAccountBalances(MOCK_SOLANA_KEYRING_ACCOUNT_1.id, [ + KnownCaip19Id.SolMainnet, + ]), + ).rejects.toThrow('Failed to fetch account assets (1 failed)'); + expect(mockAssetsService.fetchAccountAssetsFromRpc).toHaveBeenCalledWith( + expect.anything(), + { failOnNetworks: [KnownCaip19Id.SolMainnet.split('/')[0]] }, + ); + }); + it('removes token assets with zero balance', async () => { vi.spyOn( mockAssetsService, diff --git a/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.ts b/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.ts index c78730f88..24aed5201 100644 --- a/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.ts +++ b/packages/solana-wallet-snap/src/core/handlers/onKeyringRequest/Keyring.ts @@ -71,6 +71,7 @@ import { } from '../../utils/deriveSolanaKeypair'; import { trackError } from '../../utils/errors'; import { getBip32Entropy } from '../../utils/getBip32Entropy'; +import { getNetworkFromToken } from '../../utils/getNetworkFromToken'; import { endTrace, listEntropySources, @@ -457,8 +458,16 @@ export class SolanaKeyring implements KeyringSnapRpc { const account = await this.getAccountOrThrow(accountId); - const assetsList = - await this.#assetsService.fetchAccountAssetsFromRpc(account); + // A failed RPC fetch for a requested network must fail the request: + // answering with the partial (or empty) result would make the client + // keep showing a stale balance with no error. + const failOnNetworks = [ + ...new Set(assets.map((asset) => getNetworkFromToken(asset))), + ]; + const assetsList = await this.#assetsService.fetchAccountAssetsFromRpc( + account, + { failOnNetworks }, + ); const assetsToUse = assetsList .filter((asset) => assets.includes(asset.assetType)) diff --git a/packages/solana-wallet-snap/src/core/services/assets/AssetsService.test.ts b/packages/solana-wallet-snap/src/core/services/assets/AssetsService.test.ts index facce4ab9..096fb0e8f 100644 --- a/packages/solana-wallet-snap/src/core/services/assets/AssetsService.test.ts +++ b/packages/solana-wallet-snap/src/core/services/assets/AssetsService.test.ts @@ -221,9 +221,24 @@ describe('AssetsService', () => { MOCK_SOLANA_KEYRING_ACCOUNT_0, ); - expect(fetchSpy).toHaveBeenCalledWith(MOCK_SOLANA_KEYRING_ACCOUNT_0); + expect(fetchSpy).toHaveBeenCalledWith(MOCK_SOLANA_KEYRING_ACCOUNT_0, {}); expect(assets).toStrictEqual(MOCK_ASSET_ENTITIES); }); + + it('forwards failOnNetworks to the Snap adapter', async () => { + const fetchSpy = vi + .spyOn(snapAssetsAdapter, 'fetchAccountAssets') + .mockResolvedValue(MOCK_ASSET_ENTITIES); + + await assetsService.fetchAccountAssetsFromRpc( + MOCK_SOLANA_KEYRING_ACCOUNT_0, + { failOnNetworks: [Network.Mainnet] }, + ); + + expect(fetchSpy).toHaveBeenCalledWith(MOCK_SOLANA_KEYRING_ACCOUNT_0, { + failOnNetworks: [Network.Mainnet], + }); + }); }); describe('getAssetsMetadata', () => { diff --git a/packages/solana-wallet-snap/src/core/services/assets/AssetsService.ts b/packages/solana-wallet-snap/src/core/services/assets/AssetsService.ts index 824d3f2e8..6c9566899 100644 --- a/packages/solana-wallet-snap/src/core/services/assets/AssetsService.ts +++ b/packages/solana-wallet-snap/src/core/services/assets/AssetsService.ts @@ -11,6 +11,7 @@ import type { import type { CaipAssetType, CaipChainId } from '@metamask/utils'; import type { AssetEntity } from '../../../entities'; +import type { Network } from '../../constants/solana'; import type { CoreAssetsAdapter } from './adapters/CoreAssetsAdapter'; import { SnapAssetsAdapter } from './adapters/SnapAssetsAdapter'; import type { AssetMetadata } from './types'; @@ -92,12 +93,16 @@ export class AssetsService { * asset fetching: it never consults the assets migration feature flag. * * @param account - The account to fetch live assets for. + * @param options - Options. + * @param options.failOnNetworks - Networks whose fetch failures must throw + * instead of being skipped (see the adapter's `fetchAccountAssets`). * @returns The live assets. */ async fetchAccountAssetsFromRpc( account: ExtendedKeyringAccount, + options: { failOnNetworks?: Network[] } = {}, ): Promise { - return this.#snapAdapter.fetchAccountAssets(account); + return this.#snapAdapter.fetchAccountAssets(account, options); } async save(asset: AssetEntity): Promise { diff --git a/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.test.ts b/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.test.ts index 49e286370..278874eba 100644 --- a/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.test.ts +++ b/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.test.ts @@ -32,7 +32,10 @@ import type { AccountsService } from '../../accounts/AccountsService'; import type { ConfigProvider } from '../../config'; import type { SolanaConnection } from '../../connection'; import type { AssetsRepository } from '../AssetsRepository'; -import { SnapAssetsAdapter } from './SnapAssetsAdapter'; +import { + AccountAssetsFetchError, + SnapAssetsAdapter, +} from './SnapAssetsAdapter'; describe('SnapAssetsAdapter', () => { let snapAssetsAdapter: SnapAssetsAdapter; @@ -258,4 +261,84 @@ describe('SnapAssetsAdapter', () => { ); }); }); + + describe('fetchAccountAssets with failOnNetworks (v3: SOL must not fail silently)', () => { + beforeEach(() => { + (mockConfigProvider.getActiveNetworks as Mock).mockResolvedValue([ + Network.Mainnet, + ]); + }); + + const mockRpc = ({ + balanceFails = false, + tokensFail = false, + }: { + balanceFails?: boolean; + tokensFail?: boolean; + }): void => { + (mockConnection.getRpc as Mock).mockReturnValue({ + getBalance: vi.fn().mockReturnValue({ + send: balanceFails + ? vi.fn().mockRejectedValue(new TypeError('Failed to fetch')) + : vi + .fn() + .mockResolvedValue(MOCK_SOLANA_RPC_GET_BALANCE_AS_SDK_RESPONSE), + }), + getTokenAccountsByOwner: vi.fn().mockReturnValue({ + send: tokensFail + ? vi.fn().mockRejectedValue(new TypeError('Failed to fetch')) + : vi + .fn() + .mockResolvedValue( + MOCK_SOLANA_RPC_GET_TOKEN_ACCOUNTS_BY_OWNER_AS_SDK_RESPONSE, + ), + }), + }); + }; + + it('throws AccountAssetsFetchError when the native balance fetch of a required network fails', async () => { + mockRpc({ balanceFails: true }); + + const promise = snapAssetsAdapter.fetchAccountAssets( + MOCK_SOLANA_KEYRING_ACCOUNT_0, + { failOnNetworks: [Network.Mainnet] }, + ); + + await expect(promise).rejects.toBeInstanceOf(AccountAssetsFetchError); + await expect(promise).rejects.toThrow( + `Failed to fetch account assets (1 failed): ${Network.Mainnet}: Failed to fetch`, + ); + }); + + it('throws when token accounts of a required network cannot be fetched', async () => { + mockRpc({ tokensFail: true }); + + await expect( + snapAssetsAdapter.fetchAccountAssets(MOCK_SOLANA_KEYRING_ACCOUNT_0, { + failOnNetworks: [Network.Mainnet], + }), + ).rejects.toThrow('Failed to fetch account assets (2 failed)'); + }); + + it('ignores failures on networks that are not required', async () => { + mockRpc({ balanceFails: true }); + + const assets = await snapAssetsAdapter.fetchAccountAssets( + MOCK_SOLANA_KEYRING_ACCOUNT_0, + { failOnNetworks: [Network.Devnet] }, + ); + + expect(Array.isArray(assets)).toBe(true); + }); + + it('keeps the lenient behaviour without failOnNetworks (sync path)', async () => { + mockRpc({ balanceFails: true, tokensFail: true }); + + const assets = await snapAssetsAdapter.fetchAccountAssets( + MOCK_SOLANA_KEYRING_ACCOUNT_0, + ); + + expect(assets).toStrictEqual([]); + }); + }); }); diff --git a/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.ts b/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.ts index 99dc41460..4d52197c5 100644 --- a/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.ts +++ b/packages/solana-wallet-snap/src/core/services/assets/adapters/SnapAssetsAdapter.ts @@ -65,6 +65,38 @@ type TokenAccountWithMetadata = { keyringAccount: ExtendedKeyringAccount; } & Serializable; +/** + * A failed RPC fetch while collecting an account's assets. + */ +export type AccountAssetsFetchFailure = { + network: Network; + reason: unknown; +}; + +/** + * Thrown when an account's assets cannot be fetched completely for the + * networks a caller needs (see `fetchAccountAssets`'s `failOnNetworks`). + */ +export class AccountAssetsFetchError extends Error { + readonly failures: AccountAssetsFetchFailure[]; + + constructor(failures: AccountAssetsFetchFailure[]) { + const reasons = failures + .map(({ network, reason }) => { + const message = + reason instanceof Error ? reason.message : String(reason); + return `${network}: ${message}`; + }) + .join('; '); + super( + `Failed to fetch account assets (${failures.length} failed): ${reasons}`, + { cause: failures[0]?.reason }, + ); + this.name = 'AccountAssetsFetchError'; + this.failures = failures; + } +} + export class SnapAssetsAdapter { readonly #logger: Logger; @@ -280,12 +312,14 @@ export class SnapAssetsAdapter { * @param accounts - The owners of the token accounts. * @param programIds - The program ids to fetch the token accounts for. * @param scopes - The networks to fetch the token accounts for. + * @param failures - Collects the per-network fetch failures, if given. * @returns The token accounts augmented with the scope and the caip-19 asset type for convenience. */ async #fetchTokenAccountsMultiple( accounts: ExtendedKeyringAccount[], programIds: Address[] = [TOKEN_PROGRAM_ADDRESS, TOKEN_2022_PROGRAM_ADDRESS], scopes: Network[] = [Network.Mainnet], + failures?: AccountAssetsFetchFailure[], ): Promise { if (programIds.length === 0 || scopes.length === 0) { return []; @@ -332,6 +366,13 @@ export class SnapAssetsAdapter { combinations.map(({ account }) => account.id), ); + responses.forEach((item, index) => { + const combination = combinations[index]; + if (item.status === 'rejected' && combination) { + failures?.push({ network: combination.scope, reason: item.reason }); + } + }); + return responses.flatMap((item) => item.status === 'fulfilled' ? item.value : [], ); @@ -383,18 +424,45 @@ export class SnapAssetsAdapter { * @param account - The account to get the assets and balances for. * @returns The live assets and balances of the account. */ + /** + * Fetches the native and token assets of an account from the RPC. + * + * Individual fetch failures are reported to Sentry and left out of the + * result. Pass `failOnNetworks` when a partial result must not be used as a + * balance (e.g. `keyring_getAccountBalances`): if any fetch for one of those + * networks failed, an {@link AccountAssetsFetchError} is thrown instead, so + * the client flags the balance unavailable rather than keeping a stale one. + * + * @param account - The account. + * @param options - Options. + * @param options.failOnNetworks - Networks whose fetch failures must throw. + * @returns The account assets. + */ async fetchAccountAssets( account: ExtendedKeyringAccount, + options: { failOnNetworks?: Network[] } = {}, ): Promise { + const failures: AccountAssetsFetchFailure[] = []; const [nativeAssets, tokenAccounts] = await Promise.all([ - this.#fetchNativeAssets(account), + this.#fetchNativeAssets(account, failures), this.#fetchTokenAccountsMultiple( [account], [TOKEN_PROGRAM_ADDRESS, TOKEN_2022_PROGRAM_ADDRESS], await this.#configProvider.getActiveNetworks(), + failures, ), ]); + const { failOnNetworks } = options; + if (failOnNetworks?.length) { + const blocking = failures.filter(({ network }) => + failOnNetworks.includes(network), + ); + if (blocking.length > 0) { + throw new AccountAssetsFetchError(blocking); + } + } + const assetTypes = tokenAccounts.map( (tokenAccount) => tokenAccount.assetType, ); @@ -458,6 +526,7 @@ export class SnapAssetsAdapter { async #fetchNativeAssets( account: ExtendedKeyringAccount, + failures?: AccountAssetsFetchFailure[], ): Promise { const nativeAssetsTypes = await this.getNativeAssetTypes(); @@ -489,6 +558,16 @@ export class SnapAssetsAdapter { results.map(() => account.id), ); + results.forEach((item, index) => { + const assetType = nativeAssetsTypes[index]; + if (item.status === 'rejected' && assetType) { + failures?.push({ + network: getNetworkFromToken(assetType), + reason: item.reason, + }); + } + }); + return results.flatMap((item) => item.status === 'fulfilled' ? item.value : [], );