Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/assets-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add `getAsset(accountId, assetId)` method and `AssetsController:getAsset` messenger action that returns the combined `Asset` (balance, metadata, price) for a single account/asset pair from controller state, or `undefined` when a complete renderable asset is not available ([#9521](https://github.com/MetaMask/core/pull/9521))

### Changed

- Bump `@metamask/network-enablement-controller` from `^5.5.0` to `^5.6.0` ([#9520](https://github.com/MetaMask/core/pull/9520))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,29 @@ export type AssetsControllerGetAssetMetadataAction = {
handler: AssetsController['getAssetMetadata'];
};

/**
* Get a single combined asset (balance + metadata + price + computed
* `fiatValue`) for an account directly from controller state.
*
* Reuses the same state-composition and filtering logic as `getAssets`
* (balance and metadata are required, a missing price falls back to
* `{ price: 0, lastUpdated: 0 }` with `fiatValue: 0`, and hidden or
* otherwise filtered assets are excluded) so the returned shape never
* drifts from `getAssets`. Reads from current state only and does not
* trigger a data-source refresh.
*
* @param accountId - The account ID (`InternalAccount.id`, not an address).
* @param assetId - The CAIP-19 asset ID including chain scope
* (e.g. `eip155:1/erc20:0x...`).
* @returns The combined `Asset`, or `undefined` when no complete
* renderable asset (balance + metadata) exists for the account/asset pair.
* @throws If `accountId` is empty or `assetId` is not a valid CAIP-19 asset ID.
*/
export type AssetsControllerGetAssetAction = {
type: `AssetsController:getAsset`;
handler: AssetsController['getAsset'];
};

export type AssetsControllerGetAssetsPriceAction = {
type: `AssetsController:getAssetsPrice`;
handler: AssetsController['getAssetsPrice'];
Expand Down Expand Up @@ -132,6 +155,7 @@ export type AssetsControllerMethodActions =
| AssetsControllerGetAssetsAction
| AssetsControllerGetAssetsBalanceAction
| AssetsControllerGetAssetMetadataAction
| AssetsControllerGetAssetAction
| AssetsControllerGetAssetsPriceAction
| AssetsControllerGetExchangeRatesForBridgeAction
| AssetsControllerGetStateForTransactionPayAction
Expand Down
148 changes: 148 additions & 0 deletions packages/assets-controller/src/AssetsController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,154 @@ describe('AssetsController', () => {
});
});

describe('getAsset', () => {

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.

🌶️ this test file is too big, impossible to read!

Thoughts on:

  1. Making this spec more dry. I think we can leverage test tables (it.each)
  2. spicer -- separate test for each method, truly isolated, easy to read 😈

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.

I'm fine keeping this as is... but I'd like to pencil in test cleanup.

const metadata = {
type: 'erc20' as const,
symbol: 'USDC',
name: 'USD Coin',
decimals: 6,
};

it('returns the combined asset with computed fiatValue', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '100' } },
},
assetsPrice: { [MOCK_ASSET_ID]: { price: 2, lastUpdated: 123 } },
};

await withController({ state: initialState }, ({ controller }) => {
const asset = controller.getAsset(MOCK_ACCOUNT_ID, MOCK_ASSET_ID);

expect(asset).toStrictEqual({
id: MOCK_ASSET_ID,
chainId: 'eip155:1',
balance: { amount: '100' },
metadata,
price: { price: 2, lastUpdated: 123 },
fiatValue: 200,
});
});
});

it('normalizes a lowercase EVM asset ID before lookup', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '1' } },
},
assetsPrice: { [MOCK_ASSET_ID]: { price: 1, lastUpdated: 1 } },
};

await withController({ state: initialState }, ({ controller }) => {
const asset = controller.getAsset(
MOCK_ACCOUNT_ID,
MOCK_ASSET_ID_LOWERCASE,
);

expect(asset?.id).toBe(MOCK_ASSET_ID);
});
});

it('returns undefined when the balance is missing', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
};

await withController({ state: initialState }, ({ controller }) => {
expect(
controller.getAsset(MOCK_ACCOUNT_ID, MOCK_ASSET_ID),
).toBeUndefined();
});
});

it('returns undefined when the metadata is missing', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '100' } },
},
};

await withController({ state: initialState }, ({ controller }) => {
expect(
controller.getAsset(MOCK_ACCOUNT_ID, MOCK_ASSET_ID),
).toBeUndefined();
});
});

it('falls back to a zero price and fiatValue when the price is missing', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '100' } },
},
};

await withController({ state: initialState }, ({ controller }) => {
const asset = controller.getAsset(MOCK_ACCOUNT_ID, MOCK_ASSET_ID);

expect(asset?.price).toStrictEqual({ price: 0, lastUpdated: 0 });
expect(asset?.fiatValue).toBe(0);
});
});

it('returns undefined for a hidden asset', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '100' } },
},
assetPreferences: { [MOCK_ASSET_ID]: { hidden: true } },
};

await withController({ state: initialState }, ({ controller }) => {
expect(
controller.getAsset(MOCK_ACCOUNT_ID, MOCK_ASSET_ID),
).toBeUndefined();
});
});

it('throws when accountId is empty', async () => {
await withController(({ controller }) => {
expect(() =>
controller.getAsset('' as AccountId, MOCK_ASSET_ID),
).toThrow('accountId must be a non-empty string');
});
});

it('throws when assetId is not a valid CAIP-19 asset ID', async () => {
await withController(({ controller }) => {
expect(() =>
controller.getAsset(
MOCK_ACCOUNT_ID,
'not-a-caip-19' as Caip19AssetId,
),
).toThrow('invalid CAIP-19 assetId');
});
});

it('is exposed as the AssetsController:getAsset messenger action', async () => {
const initialState: Partial<AssetsControllerState> = {
assetsInfo: { [MOCK_ASSET_ID]: metadata },
assetsBalance: {
[MOCK_ACCOUNT_ID]: { [MOCK_ASSET_ID]: { amount: '100' } },
},
assetsPrice: { [MOCK_ASSET_ID]: { price: 2, lastUpdated: 123 } },
};

await withController({ state: initialState }, ({ messenger }) => {
const asset = messenger.call(
'AssetsController:getAsset',
MOCK_ACCOUNT_ID,
MOCK_ASSET_ID,
);

expect(asset?.fiatValue).toBe(200);
});
});
});

describe('getAssets', () => {
it('returns empty object when no balances exist', async () => {
await withController(async ({ controller }) => {
Expand Down
49 changes: 48 additions & 1 deletion packages/assets-controller/src/AssetsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@
'getAssets',
'getAssetsBalance',
'getAssetMetadata',
'getAsset',
'getAssetsPrice',
'getExchangeRatesForBridge',
'getStateForTransactionPay',
Expand Down Expand Up @@ -1775,6 +1776,51 @@
return this.state.assetsInfo[assetId] as AssetMetadata | undefined;
}

/**
* Get a single combined asset (balance + metadata + price + computed
* `fiatValue`) for an account directly from controller state.
*
* Reuses the same state-composition and filtering logic as `getAssets`
* (balance and metadata are required, a missing price falls back to
* `{ price: 0, lastUpdated: 0 }` with `fiatValue: 0`, and hidden or
* otherwise filtered assets are excluded) so the returned shape never
* drifts from `getAssets`. Reads from current state only and does not
* trigger a data-source refresh.
*
* @param accountId - The account ID (`InternalAccount.id`, not an address).
* @param assetId - The CAIP-19 asset ID including chain scope
* (e.g. `eip155:1/erc20:0x...`).
* @returns The combined `Asset`, or `undefined` when no complete
* renderable asset (balance + metadata) exists for the account/asset pair.
* @throws If `accountId` is empty or `assetId` is not a valid CAIP-19 asset ID.
*/
getAsset(accountId: AccountId, assetId: Caip19AssetId): Asset | undefined {
if (typeof accountId !== 'string' || accountId.length === 0) {
throw new Error(
'AssetsController.getAsset: accountId must be a non-empty string',
);
}

let normalizedAssetId: Caip19AssetId;
try {
normalizedAssetId = normalizeAssetId(assetId);
} catch {
throw new Error(
`AssetsController.getAsset: invalid CAIP-19 assetId "${assetId}"`,
);
}

const chainId = extractChainId(normalizedAssetId);

const assets = this.#getAssetsFromState(
[{ id: accountId }],
[chainId],
['fungible'],
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Skips enabled-chain filtering

Medium Severity

getAsset passes only the asset’s own chain into #getAssetsFromState, while default getAssets filters with #enabledChains. A balance on a disabled network can be returned by getAsset but omitted from getAssets, breaking the documented parity between the two APIs.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6aa280d. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well this function will be used by Snaps independently from the enabledChains

Comment on lines +1815 to +1819

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.

nit - A tad nuts that we are calculating all assets, only to return a single asset here.

This is okay for now, we can raise/improve moving forward.


return assets[accountId]?.[normalizedAssetId];
}

async getAssetsPrice(
accounts: InternalAccount[],
options?: {
Expand Down Expand Up @@ -2698,7 +2744,7 @@
}

#getAssetsFromState(
accounts: InternalAccount[],
accounts: Pick<InternalAccount, 'id'>[],

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.

Very good! Keep interface minimal!
Easier to test and use.

chainIds: ChainId[],
assetTypes: AssetType[],
): Record<AccountId, Record<Caip19AssetId, Asset>> {
Expand All @@ -2725,7 +2771,7 @@
const metadata = metadataRaw;

// Skip hidden assets (assetPreferences)
const prefs = this.state.assetPreferences[typedAssetId];

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
if (prefs?.hidden) {
continue;
}
Expand Down Expand Up @@ -3671,6 +3717,7 @@
this.messenger.unregisterActionHandler('AssetsController:getAssets');
this.messenger.unregisterActionHandler('AssetsController:getAssetsBalance');
this.messenger.unregisterActionHandler('AssetsController:getAssetMetadata');
this.messenger.unregisterActionHandler('AssetsController:getAsset');
this.messenger.unregisterActionHandler('AssetsController:getAssetsPrice');
this.messenger.unregisterActionHandler(
'AssetsController:getExchangeRatesForBridge',
Expand Down
1 change: 1 addition & 0 deletions packages/assets-controller/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export type {
AssetsControllerGetAssetsAction,
AssetsControllerGetAssetsBalanceAction,
AssetsControllerGetAssetMetadataAction,
AssetsControllerGetAssetAction,
AssetsControllerGetAssetsPriceAction,
AssetsControllerAddCustomAssetAction,
AssetsControllerRemoveCustomAssetAction,
Expand Down