fix: Price service caching logic for spot price#49
fix: Price service caching logic for spot price#49stanleyyconsensys wants to merge 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts PriceService.getSpotPrices caching to cache spot prices per asset ID + quote currency, enabling partial cache hits (fetch only missing assets) and batch cache operations.
Changes:
- Replaced
useCache-based spot price caching with a custommget/msetstrategy keyed per asset andvsCurrency. - Expanded
PriceServiceunit tests to cover deduplication, defaultvsCurrency, partial hits, and cache isolation across currencies. - Enhanced the in-memory cache test fixture to implement
mget/msetagainst the backing store map.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/snap/src/services/price/PriceService.ts | Implements per-asset spot price cache reads/writes using mget/mset and partial-miss fetching. |
| packages/snap/src/services/price/PriceService.test.ts | Updates and expands spot price caching tests to match the new per-asset caching behavior. |
| packages/snap/src/services/cache/mocks/cache.fixtures.ts | Implements mget/mset behavior in the test cache fixture so tests can validate batch caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const setupTest = () => { | ||
| const getSpotPricesSpy = jest.spyOn( | ||
| PriceApiClient.prototype, | ||
| 'getSpotPrices', | ||
| ); | ||
| getSpotPricesSpy.mockReset(); | ||
| getSpotPricesSpy.mockResolvedValue({ [stellarClassicUsdc]: null }); | ||
|
|
||
| const getFiatExchangeRatesSpy = jest.spyOn( | ||
| PriceApiClient.prototype, | ||
| 'getFiatExchangeRates', | ||
| ); | ||
| getFiatExchangeRatesSpy.mockReset(); | ||
| getFiatExchangeRatesSpy.mockResolvedValue(fiatExchangeRatesBody); | ||
|
|
||
| const getHistoricalPricesSpy = jest.spyOn( | ||
| PriceApiClient.prototype, | ||
| 'getHistoricalPrices', | ||
| ); | ||
| getHistoricalPricesSpy.mockReset(); | ||
| getHistoricalPricesSpy.mockResolvedValue( | ||
| GET_HISTORICAL_PRICES_RESPONSE_NULL_OBJECT, | ||
| ); | ||
|
|
||
| return { | ||
| getSpotPricesSpy, | ||
| getFiatExchangeRatesSpy, | ||
| getHistoricalPricesSpy, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
restoreAllMocks is no need, as our jest setup reseted mock
| vsCurrency: string, | ||
| ) => | ||
| `PriceService:getSpotPrices:${JSON.stringify(serialize(assetIds))}:${JSON.stringify(serialize(vsCurrency))}`; | ||
| const SPOT_PRICES_CACHE_KEY_PREFIX = 'PriceApiClient:getSpotPrices' as const; |
There was a problem hiding this comment.
For consistency with the other cache namespaces in this class (PriceService:getFiatExchangeRates, PriceService:getHistoricalPrices), rename the prefix from PriceApiClient:getSpotPrices to PriceService:getSpotPrices
| @@ -70,32 +69,40 @@ const cacheKeyHistoricalPrices = (params: { | |||
| }) => `PriceService:getHistoricalPrices:${JSON.stringify(serialize(params))}`; | |||
|
|
|||
| describe('PriceService', () => { | |||
There was a problem hiding this comment.
NIT: I think better to add a top-level afterEach for any future test that forgets setupTest()
afterEach(() => {
jest.restoreAllMocks();
});
There was a problem hiding this comment.
we dont need, i have set the restore on jest config level
and i try to remove all after each, before each to align the MM test standard
Explanation
This PR adjusts PriceService.getSpotPrices caching to cache spot prices per asset ID + quote currency, enabling partial cache hits (fetch only missing assets) and batch cache operations.
before change
if the asset id is "1,2,3" and if the price for "1,2,3" is $1, $2, $3,
we cache the price in that way
key "1,2,3" = "$1,$2,$3"
if another request asset id "2,3" come in, we will have to request the API again, as we didnt cache "2,3"
after change
if the asset id is "1,2,3" and if the price for "1,2,3" is $1, $2, $3,
we cache the price in that way
key "1" = "$1"
key "2" = "$2"
key "3" = "$3"
if another request asset id "2,3" come in, we will no longer need to fetch from the api
References
Checklist