-
-
Notifications
You must be signed in to change notification settings - Fork 256
feat: price API v3 upgrade #7119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
950baef to
82b6f2e
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
944dfe3 to
6dd22b8
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
6dd22b8 to
afa0dd6
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
afa0dd6 to
373429e
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
373429e to
3c9e945
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| '0x63564c40': 'eip155:1666600000/slip44:1023', // Harmony Mainnet Shard 0 - Native symbol: ONE | ||
| '0x279f': 'eip155:143/slip44:268435779', // Monad Testnet - Native symbol: MON | ||
| '0x3e7': 'eip155:999/slip44:2457', // HyperEVM - Native symbol: ETH | ||
| } as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any entry here with null, means that the support for price-api v2 works but it doesn't for v3, as price-api does not recognise the native asset slip44 reference, which we need to fetch prices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking with platform-api. They have confirmed that if the entry worked with the zero address on v2, it should work by passing zero address as a token for v3. Not having the slip44 reference is not an issue then, as we are only using this internally.
There are, however, 2 chains for which that is not true: Meter and Polis.
There's also Monad, which does not seem to work at all, but that's probably because it's not on until next week.
I'm keeping the fallback to v2 for those chains, but as soon as they are sorted at backend level we can get rid of any v2 code.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
nice test coverage |
salimtb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tested this on extension and it looks good to me , TY for the clean up
| const contractInformations = await this.#fetchAndMapExchangeRates({ | ||
| tokenAddresses, | ||
| chainId, | ||
| const promises = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the number of calls/promises we are generating here?
If this is large, thoughts on using reduceInBatchesSerially or one of the other batching mechanisms we have to process multiple promises in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At most, one for every different currency. Up until now, we were generating one for every different chain and awaiting them as well with Promise.allSettled.
In a standard scenario with all the popular chains enabled, we'll be sending 5 promises in parallel (ETH, POL, BNB, SEI and AVAX).
I think we are still far from the number of chains that would require careful batching.
| {} as Record<SupportedChainId, Hex[]>, | ||
| ); | ||
|
|
||
| const promises = Object.entries(assetsByChainId).map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Are there many promises in parallel here?
If yes, thoughts on using the reduceInBatchesSerially or other batching utils so we process certain amount at a time?
Also unsure if we should add a timeout mechanism safelyExecuteWithTimeout, for if this API hangs for a long time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maximum of 3, assuming that the user has Monad, Meter and Polis networks enabled (the last two are rather obscure).
We should also be expecting to remove this fallback soon, because we will be able to test Monad for the correct caip19 asset when it becomes available next week. The other two chains I have already reported them to platform-api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are called with this.#policy.execute, which has its own for retries, duration, etc.
Prithpal-Sooriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with 2 comments on adding some batching logic on our parallel promises
e306214
Explanation
Upgrade to Price API v3
Preview PR for extension: MetaMask/metamask-extension#37741
Preview PR for mobile: MetaMask/metamask-mobile#22876
References
Fixes https://consensyssoftware.atlassian.net/browse/ASSETS-1746
Checklist
Note
Upgrades token price fetching to Price API v3, updates service/types and controllers to new assets-based API, and removes legacy polling paths.
CodefiTokenPricesServiceV2: add v3/v3/spot-priceswith CAIP-19assetId, fallback to v2 per-chain endpoint; expandSUPPORTED_CURRENCIES; introduceSPOT_PRICES_SUPPORT_INFO; handle native token addresses; circuit-breaker/degraded handling retained.AbstractTokenPricesService.fetchTokenPricesnow acceptsassets: { chainId, tokenAddress }[]and returnsEvmAssetWithMarketData[](replaces address->object map and addsassetId).EvmAssetAddressWithChain,EvmAssetWithId,EvmAssetWithMarketData.TokenRatesController: fetch prices grouped by native currency; support unsupported native currencies via USD conversion; include native token in queries; remove legacy polling/state/event deps; handle network deletions; enable/disable gating maintained.CurrencyRateController: on API failure, fallback to spot prices viafetchTokenPricesusing native token asset; map multiple networks by native currency.TokenSearchDiscoveryDataController: adapt to newfetchTokenPricesresponse (array) and asset inputs.assetsUtil.fetchTokenContractExchangeRates: switch to assets-based batching (includes native token) and map array response.Written by Cursor Bugbot for commit e306214. This will update automatically on new commits. Configure here.