feat: Add firstTimeInteraction to transactionMeta
#4895
+320
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@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. |
…ess-for-the-first-time
|
@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. |
7 tasks
matthewwalsh0
requested changes
Nov 13, 2024
| import { FirstTimeInteractionError } from '../errors'; | ||
| import { projectLogger } from '../logger'; | ||
|
|
||
| export const BASE_URL = 'https://accounts.api.cx.metamask.io'; |
There was a problem hiding this comment.
Should the name of this file reflect the API itself so accounts-api.ts?
I've also added this same util (for a different endpoint) for the ongoing incoming transactions refactor, so should we match the name and location to simplify the merge?
| ): Promise<FirstTimeInteractionResponse> { | ||
| const url = await getFirstTimeInteractionUrl(request); | ||
|
|
||
| log('Sending request', url, request); |
There was a problem hiding this comment.
As we'll use additional endpoints for incoming transactions, should we keep logs more specific so Sending first time request?
| if (responseJson.error) { | ||
| const { message, code } = responseJson.error; | ||
|
|
||
| if (message === FAILED_TO_PARSE_MESSAGE) { |
There was a problem hiding this comment.
Is there an error code we could use instead to keep it less coupled?
| if (responseJson.error) { | ||
| const { message, code } = responseJson.error; | ||
|
|
||
| if (message === FAILED_TO_PARSE_MESSAGE) { |
There was a problem hiding this comment.
Should we add a comment to clarify why we can assume this specific error means it's a first time interaction?
|
|
||
| return { | ||
| isFirstTimeInteraction: | ||
| responseJson?.count === 0 || responseJson?.count === undefined, |
There was a problem hiding this comment.
Will the second condition mean we return true if there is no response JSON at all?
| log('Sending request', url, request); | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'GET', |
There was a problem hiding this comment.
I can't find the name currently, but there is a specific header we should use so the API team can track the source of the requests.
| let firstTimeInteractionResponse: FirstTimeInteractionResponse; | ||
|
|
||
| try { | ||
| validateFirstTimeInteraction(request); |
There was a problem hiding this comment.
The params are validated when we first add the transaction in addTransaction, so why do we need to validate an object generated internally?
| @@ -3614,6 +3627,73 @@ export class TransactionController extends BaseController< | |||
| return transactionMeta; | |||
| } | |||
|
|
|||
| async #updateFirstInteraction( | |||
There was a problem hiding this comment.
I don't think we should do anything now, but if these API usages grow, we could standardise this pattern to reduce some duplication.
| /** | ||
| * Whether the transaction is the first time interaction. | ||
| */ | ||
| firstTimeInteraction?: boolean; |
There was a problem hiding this comment.
Do we also need a firstTimeInteractionLoading property so the UI can more easily differentiate loading and disabled?
|
|
||
| firstTimeInteractionResponse = await this.#trace( | ||
| { name: 'FirstTimeInteraction', parentContext: traceContext }, | ||
| () => getFirstTimeInteraction(request), |
There was a problem hiding this comment.
Do we need a toggle to disable this or is that a future ticket?
At what point is it easier to provide a single isExternalApiDisabled callback?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR adds
firstTimeInteractiontotransactionMetaby callinghttps://primitives.api.cx.metamask.io/APIrelationshipsendpoint.For more information please see docs here: https://primitives.api.cx.metamask.io/docs#/Account%20%7C%20Relationships/IndexedChainDataAPIController_getAccountAddressRelationship
References
Related to: https://github.com/MetaMask/MetaMask-planning/issues/3040
Changelog
@metamask/transaction-controllerfirstTimeInteractiontotransactionMetahttps://primitives.api.cx.metamask.iorelationshipsendpoint.Checklist