Skip to content

fetch both selectedcurrency and usd prices#8123

Open
bergarces wants to merge 3 commits intomainfrom
assets-controller-usd-prices
Open

fetch both selectedcurrency and usd prices#8123
bergarces wants to merge 3 commits intomainfrom
assets-controller-usd-prices

Conversation

@bergarces
Copy link
Contributor

@bergarces bergarces commented Mar 5, 2026

Explanation

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Medium risk because it changes the AssetPrice/FungibleAssetPrice type shape (adds assetPriceType/usdPrice and narrows the union), which may be breaking for downstream consumers, and it increases Price API usage by fetching USD in addition to the selected currency.

Overview
Price fetching now queries the spot-prices API for both the selected currency and USD (in parallel when needed) and returns a normalized assetsPrice map that includes price (selected currency), usdPrice, lastUpdated, and assetPriceType: 'fungible'.

Types were updated to make prices a discriminated union (assetPriceType), factor shared fields into BaseAssetPrice, add usdPrice to FungibleAssetPrice, and narrow AssetPrice to FungibleAssetPrice | NFTAssetPrice.

Written by Cursor Bugbot for commit 647c5e1. This will update automatically on new commits. Configure here.

@bergarces bergarces requested a review from a team as a code owner March 5, 2026 16:03
}

const prices: Record<Caip19AssetId, AssetPrice> = {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the logic that validates the result from the API here and added an extra check to verify that the USD price is also there.

This also allows us to remove this logic from the two places that were calling this private method.

export type AssetPrice =
| FungibleAssetPrice
| NFTAssetPrice
| (BaseAssetPrice & { [key: string]: Json });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made some minor adjustment to this type, which also addresses this TODO I added to the client (http://github.com/MetaMask/metamask-extension/blob/main/shared/modules/selectors/assets-migration.ts#L715).

It's very convenient to have a type discriminator field with unions of very different types, as that allows typescript to to perform type guards easily.

For that reason, I added an assetPriceType with a specific value for each type and removed the generic (BaseAssetPrice & { [key: string]: Json }), as it is not used anywhere and it's unlikely it'll ever be needed. If it ever is needed, we can add it then and create a new type for it.

Copy link

@cursor cursor bot left a 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.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

@@ -190,7 +190,7 @@ export type AssetMetadata =
* Base price attributes.
*/
export type BaseAssetPrice = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make all the price types extend this one, which is the absolutely bare minimum needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant