feat: adds market insights logic in ai-digest-ctrl#7930
feat: adds market insights logic in ai-digest-ctrl#7930
Conversation
|
@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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
zone-live
changed the title
| */ | ||
| export type MarketInsightsEntry = { | ||
| /** CAIP-19 asset identifier */ | ||
| caip19Id: string; |
There was a problem hiding this comment.
WYT of using the type CaipAssetType?
There was a problem hiding this comment.
Yap, will update 👍🏼
|
|
||
| export type AiDigestControllerState = { | ||
| digests: Record<string, DigestEntry>; | ||
| marketInsights: Record<string, MarketInsightsEntry>; |
There was a problem hiding this comment.
Do we actually need to manage cache ourselves? 🤔
The API returns responses with a header 'Cache-Control': 'public, max-age=600', so we get cache management at browser-level out of the box (assuming there's a browser in mobile. Is that a thing?)
There was a problem hiding this comment.
Yeah we're not sure if its a thing for mobile, and since it didn't hurt to add this simple management, its there. So I would keep controller-level cache, likely in a RN app the fetch is not browser cached like in the web since app behavior changes between iOS and Android.
View it as an other layer of control, for max entries, persisted state, TTL, etc.
| async fetchMarketInsights( | ||
| caip19Id: string, | ||
| ): Promise<MarketInsightsReport | null> { | ||
| const existing = this.state.marketInsights[caip19Id]; |
There was a problem hiding this comment.
I think we should ensure that caip19Id is valid (using isCaipAssetType). This will avoid an unnecssary call to the API in case an invalid caip asset type is passed
There was a problem hiding this comment.
Well pointed, will update 👍🏼
| * @returns The market insights report, or `null` if none exists. | ||
| */ | ||
| async fetchMarketInsights( | ||
| caip19Id: string, |
There was a problem hiding this comment.
What do you think of naming it caipAssetType? It would avoid confusion with caip asset id
| export type AiDigestControllerClearMarketInsightsAction = { | ||
| type: `${typeof controllerName}:clearMarketInsights`; | ||
| handler: AiDigestController['clearMarketInsights']; | ||
| }; |
There was a problem hiding this comment.
Does we really need to expose an action to clear the internal state? 🤔
It's a leaky abstraction
There was a problem hiding this comment.
True, we don't, will update 👍🏼
| const data: MarketInsightsApiResponse = await response.json(); | ||
|
|
||
| // API currently returns an envelope with the report under `digest`. | ||
| // Keep backward compatibility if the report is returned directly. |
There was a problem hiding this comment.
I don't think we need ensure backward compat here. The feature is not live, so it just feels like free tech debt as makes the code and type MarketInsightsApiResponse unnecessarily complex. WYT?
There was a problem hiding this comment.
Had the same doubt :D and you just confirmed it, so yeah, I'll remove it. 👍🏼
| const response = await fetch( | ||
| `${this.#baseUrl}/digests?caipAssetType=${encodeURIComponent(caip19Id)}`, |
There was a problem hiding this comment.
Should we add some response schema validation here, or leave it for the frontend?
An argument in favour of leaving it to the frontend is that any changes in the API would just be forwarded by core, and only the frontend would need to be updated — since core isn't checking the schema at runtime, it wouldn't throw an error.
That said, core is using the MarketInsightsReport type, so a breaking API change would still require a change in core anyway. Even though the integration wouldn't break at runtime, we'd be operating with incorrect static types, which kind of undermines the benefit of skipping validation here.
| async fetchDigest(assetId: string): Promise<DigestData> { | ||
| const response = await fetch( | ||
| `${this.#baseUrl}/digests/assets/${encodeURIComponent(assetId)}/latest`, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error( | ||
| `${AiDigestControllerErrorMessage.API_REQUEST_FAILED}: ${response.status}`, | ||
| ); | ||
| } | ||
|
|
||
| const data: DigestData = await response.json(); | ||
|
|
||
| if (!data.success) { | ||
| throw new Error( | ||
| data.error ?? AiDigestControllerErrorMessage.API_RETURNED_ERROR, | ||
| ); | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
We can delete this right @zone-live ? We're only using searchDigests?
There was a problem hiding this comment.
It was added by Devin, but meant as just a placeholder I think, so yes... we can always add it back later if neded.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| * @param caip19Id - The CAIP-19 identifier of the asset. | ||
| * @returns The market insights report, or `null` if none exists (404). | ||
| */ | ||
| async searchDigests(caip19Id: string): Promise<MarketInsightsReport | null> { |
There was a problem hiding this comment.
NIT: I think this function should be renamed to searchDigest (singular) .
The issue is that the endpoint suggests this returns a collection [] which is then filtered by the query parameter, but actually we're only using a query parameter because of CAIP19 not being safe to pass as a URL parameter and the endpoint always returns a single object, not a collection.
There was a problem hiding this comment.
I mean, it's not really a NIT, I think it's important but not a blocker, we can fix this later.
3 participants
Explanation
Extended
AiDigestControllerwithfetchMarketInsightsandclearMarketInsightsactions that search for AI generated market insights by asset. The service currently returns mock data but when the API is live, the real fetch to it will be added.References
Checklist
Note
Medium Risk
Introduces new persisted controller state and a new network-facing API path with CAIP validation and cache eviction logic; issues could affect state persistence or API error handling but are covered by unit tests.
Overview
Adds Market Insights support to
AiDigestControllerby introducing a persistedmarketInsightscache in state plus newfetchMarketInsights(CAIP-19 validated, TTL + max-size eviction, clears cache onnull/404) andclearMarketInsightsAPIs, and wiresfetchMarketInsightsinto the messenger action handlers.Extends
AiDigestServicewithsearchDigests(caipAssetType)to callGET /digests?caipAssetType=...(returningnullon 404), adds the correspondingMarketInsights*types and a newINVALID_CAIP_ASSET_TYPEerror constant, and updates tests/changelog/deps (adds@metamask/utils).Written by Cursor Bugbot for commit 16624fa. This will update automatically on new commits. Configure here.