fetch both selectedcurrency and usd prices#8123
Conversation
| } | ||
|
|
||
| const prices: Record<Caip19AssetId, AssetPrice> = {}; | ||
|
|
There was a problem hiding this comment.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| export type AssetPrice = | ||
| | FungibleAssetPrice | ||
| | NFTAssetPrice | ||
| | (BaseAssetPrice & { [key: string]: Json }); |
There was a problem hiding this comment.
I have made some minor adjustment to this type, which also addresses this TODO I added to the client (https://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.
| @@ -190,7 +190,7 @@ export type AssetMetadata = | |||
| * Base price attributes. | |||
| */ | |||
| export type BaseAssetPrice = { | |||
There was a problem hiding this comment.
Make all the price types extend this one, which is the absolutely bare minimum needed.
bergarces
force-pushed
the
assets-controller-usd-prices
branch
from
860f2a6 to
635bdf9
Compare
| data !== null && | ||
| 'price' in data && | ||
| typeof (data as SpotPriceMarketData).price === 'number' | ||
| typeof data.price === 'number' |
There was a problem hiding this comment.
Removed unnecessary casting.
In any case, this function does not validate well enough the result from fetch. If we have any package to validate schemas we should consider using it instead.
There was a problem hiding this comment.
++ on validation 🤞🏾
Otherwise can be a nice improvement
(personal thoughts - we have openAPI specs on our services. This is a source of truth that can drive backend and front-end changes. It is totally doable to perform type-gen and validators from openAPI specs)
| response.assetsPrice = { | ||
| ...(response.assetsPrice ?? {}), | ||
| ...spotPrices, | ||
| }; |
There was a problem hiding this comment.
Validation is moved to #fetchSpotPrices
| response.assetsPrice = { | ||
| ...(response.assetsPrice ?? {}), | ||
| ...spotPrices, | ||
| }; |
There was a problem hiding this comment.
Validation is moved to ``#fetchSpotPrices`
| }); | ||
|
|
||
| it('skips entries with invalid or negative price', () => { | ||
| it('skips entries with negative price', () => { |
There was a problem hiding this comment.
We cannot not trust and verify for a second type every piece of data we have in our controller state. If price data is not valid, we are not storing it..
| acc[assetId as Caip19AssetId] = priceData; | ||
| } | ||
| return acc; | ||
| }, {}); |
There was a problem hiding this comment.
Filter in fungible asset prices, as those are the only that we need and have the type signature we require.
| marketData: {}, | ||
| currentCurrency: selectedCurrency, | ||
| }; | ||
| } |
There was a problem hiding this comment.
We cannot just return usd prices if the selected currency is not in that list, as it might display wrong currency symbols with a price that does not correspond.
I have checked, and the list on that constant coincides with the currencies we offer in the client.
| const conversionTime = | ||
| lastUpdated > 1e12 ? lastUpdated / 1000 : lastUpdated; | ||
| const expirationTime = conversionTime + expirationOffset; | ||
| const lastUpdatedInSeconds = lastUpdated / 1000; |
There was a problem hiding this comment.
Price data is always in ms as it is always built with Date.now(), so we should always divide it by 1000 to get the value in seconds.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| : null; | ||
| if (!baseMeta) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We do not need to double-check the type here. If it comes from our controller and it matches the type, it is correct.
bergarces
force-pushed
the
assets-controller-usd-prices
branch
from
4919168 to
0c1e527
Compare
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| ### Fixed | ||
|
|
||
| - `getStateForTransactionPay()` and `formatStateForTransactionPay()` now accept optional `usdToSelectedCurrencyRate` (rate: 1 USD = N units of selected currency). When `selectedCurrency` is not USD, passing this rate ensures `currencyRates` and `marketData` use correct user-currency vs USD values; previously they incorrectly used USD price for both | ||
| - `formatExchangeRatesForBridge` and `formatStateForTransactionPay` now read both `price` (selected currency) and `usdPrice` (USD) directly from asset price data ([#8123](https://github.com/MetaMask/core/pull/8123)) |
There was a problem hiding this comment.
Since we haven't released this yet, it is not breaking. I'm just amending the changelog.
| @@ -621,99 +634,33 @@ describe('AssetsController', () => { | |||
| }); | |||
|
|
|||
| describe('getExchangeRatesForBridge', () => { | |||
There was a problem hiding this comment.
We do not need to re-test formatExchangeRatesForBridge logic here. That function is imported from the utils module and is already being tested, we just need to check that it is being called with the correct parameters.
| getExchangeRatesForBridge(options?: { | ||
| usdToSelectedCurrencyRate?: number; | ||
| }): BridgeExchangeRatesFormat { | ||
| getExchangeRatesForBridge(): BridgeExchangeRatesFormat { |
There was a problem hiding this comment.
this is good and exactly what we need here 👍
3 participants
Explanation
Starts including
usdPricefor fungible asset prices. This might mean an extra call to price api if the selected currency is different.It also updates functions that make use of the prices for state migration.
References
Checklist
Note
Medium Risk
Changes price fetching and exchange-rate formatting used by bridge and transaction-pay consumers; incorrect assumptions about
usdPrice,lastUpdatedunits, or missing network/native-currency config could cause missing or wrong rates in non-USD scenarios.Overview
Updates asset price data to always carry both the selected-currency price (
price) and the corresponding USD price (usdPrice), and formalizes price shapes viaassetPriceType(e.g.FungibleAssetPrice).PriceDataSourcenow fetches spot prices in the selected currency and USD (in parallel when needed) and merges results into responses/state.Refactors
formatExchangeRatesForBridge(and the transaction-pay legacy formatter viaformatStateForTransactionPay) to stop usingusdToSelectedCurrencyRateand instead readprice/usdPricedirectly; it also tightens behavior by returning empty rates for unknown fiat currencies and skipping EVM entries when required native-currency/network data is missing. Controller methodsgetExchangeRatesForBridgeandgetStateForTransactionPaydrop the optional conversion-rate parameter, and tests are updated accordingly.Written by Cursor Bugbot for commit 84240e4. This will update automatically on new commits. Configure here.