Add new token added event (duplicating the existing event structure) when collectible is manually added #14279

segun · 2022-03-30T20:29:31Z

Add new token added event (duplicating the existing event structure) when collectible is manually added

Set source property to custom on the new token added event,

More information

Progresses Add event metrics on NFT support in extension #13477

github-actions · 2022-03-30T20:29:49Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

brad-decker · 2022-03-30T20:46:36Z

ui/pages/add-collectible/add-collectible.js

+      event: 'Token Added',
+      category: 'Wallet',
+      sensitiveProperties: {
+        token_symbol: '',


best to leave these empty strings as undefined (just omit the properties), segment will strip them anyways.

You could add this field back now since some NFTs have a symbol and it is returned by the getTokenStandardAndDetails method that you are using now.

cc @segun ☝️

Somehow missed that....fixed here

@segun that commit doesn't see to re-add the token_symbol to the event?

metamaskbot · 2022-03-30T21:15:37Z

Builds ready [7b46558]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1445 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	150	100	20	9
		domContentLoaded	1215	1687	1435	114	55
		load	1215	1687	1445	119	57
		domInteractive	1215	1687	1435	114	55

digiwand

LGTM after updating the code based on what Brad commented

I see that the reused pattern from the codebase has been designed to support more events 👍 Can we get a reference to the related, event tracking pattern you found in the codebase?

digiwand

LGTM! left just a minor suggestion related to lines 95 & 96

digiwand · 2022-04-04T23:06:08Z

ui/pages/add-collectible/add-collectible.js

+      sensitiveProperties: {
+        token_contract_address: newCollectible.address,        
+        tokenId: newCollectible.tokenId,
+        name: newCollectible.name ? newCollectible.name : null,


Suggested change

name: newCollectible.name ? newCollectible.name : null,

name: newCollectible.name || null,

nit: we could shorten this. Same comment for the description prop below

metamaskbot · 2022-04-05T09:16:17Z

Builds ready [64993fa]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1315 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	78	126	95	13	6
		domContentLoaded	1183	1525	1293	99	47
		load	1197	1532	1315	95	45
		domInteractive	1183	1525	1293	99	47

adonesky1 · 2022-04-05T15:01:36Z

ui/pages/add-collectible/add-collectible.js

+
+    const newCollectible = collectibles.find(
+      (collectible) =>
+        collectible.address === address &&


might be worth making this a case-insensitive check until we have thoroughly standardized storing addresses as checksummed across controllers.

adonesky1 · 2022-04-05T15:02:06Z

ui/pages/add-collectible/add-collectible.js

+    }
+
+    const alreadyTracked = eventTracked.find(
+      (et) => et.address === address && et.tracked === true,


metamaskbot · 2022-04-05T16:04:37Z

Builds ready [d68dc95]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1324 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	79	132	96	11	5
		domContentLoaded	1184	1437	1312	64	31
		load	1198	1437	1324	66	31
		domInteractive	1184	1437	1312	64	31

digiwand

Awesome, LGTM!

Good to know about @adonesky1's comment above:

might be worth making this a case-insensitive check until we have thoroughly standardized storing addresses as checksummed across controllers.

now I'm wondering if we want to normalize the addresses here and/or through Segment.
cc: @brad-decker

adonesky1 · 2022-04-06T16:36:29Z

ui/pages/add-collectible/add-collectible.js

+
+    const newCollectible = collectibles.find(
+      (collectible) =>
+        collectible.address.toLowerCase() === address.toLowerCase() &&


[nit] - you could/should use the isEqualCaseInsensitive util method

brad-decker

potentially blocking question: ... Is this tracking token added when the token is detected or when the token is added to the wallet? :

adonesky1 · 2022-04-06T17:29:03Z

ui/pages/add-collectible/add-collectible.js

@@ -46,8 +50,68 @@ export default function AddCollectible() {
  );
  const [tokenId, setTokenId] = useState('');
  const [disabled, setDisabled] = useState(true);
+  const [eventTracked, setEventTracked] = useState([
+    {
+      event: 'Token Added',


Should this be collectible/NFT added? Sorry I don't have context on the schema we're using, are we somehow indicating that this is an collectible/NFT elsewhere as a subcategory of token?

@brad-decker do you agree to change this to "Collectible Added"

No we should use the asset_type and token_standard properties that match the transaction events to indicate that this 'Added Token' is a collectible/NFT

adonesky1 · 2022-04-06T17:34:34Z

ui/pages/add-collectible/add-collectible.js

+        tokenId: newCollectible.tokenId,
+        name: newCollectible.name || null,
+        description: newCollectible.description || null,
+        source: 'detected',


A couple of broader questions adding to @brad-decker's point.

This is not a "detected" add token event. This is a manually added token.

and

I'm a bit confused about why this track event is being called in a useEffect on the page where the user is adding the collectible. Is the idea that the value of collectibles will change when the user submits the form to add the collectible and this effect will happen before the component dismounts from the history.push event called on line 132 (at the end of the submission handler)?

For 2: Yes. I did test this. The useEffect is called before the component dismounts.
For 1: Do we have a "detected" flow for NFTs?

oh. interesting. I was also confused by the original request, "when auto detection occurs for NFTs".

The existing patterns for "Token Added" do not live in useEffect hooks. If we were following the pattern used for "Token Added", we could call trackEvent in the same method or callback that adds the token. In this case, could we call track in handleAddCollectible? I'm wondering where you found this pattern original pattern with useEffect

segun · 2022-04-06T18:12:58Z

potentially blocking question: ... Is this tracking token added when the token is detected or when the token is added to the wallet? :

It's added when the token is added to the wallet.

metamaskbot · 2022-04-06T19:02:23Z

Builds ready [0e9facd]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1323 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	123	100	11	5
		domContentLoaded	1232	1476	1315	64	31
		load	1233	1476	1323	64	31
		domInteractive	1232	1476	1315	64	31

metamaskbot · 2022-04-07T11:41:07Z

Builds ready [cbd6e1a]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1288 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	1181	150	237	114
		domContentLoaded	1186	1385	1274	54	26
		load	1197	1395	1288	51	24
		domInteractive	1186	1385	1274	54	26

brad-decker · 2022-04-08T16:15:47Z

ui/pages/add-collectible/add-collectible.js

+      sensitiveProperties: {
+        token_contract_address: address,
+        tokenId: tokenId.toString(),
+        source: 'manual',


we're missing properties here. asset_type, token_standard... and source should 'custom' (if this is when the user manually types in all the details of the token)

adonesky1 · 2022-04-12T14:41:21Z

ui/pages/add-collectible/add-collectible.js

@@ -47,6 +52,30 @@ export default function AddCollectible() {
  const [tokenId, setTokenId] = useState('');
  const [disabled, setDisabled] = useState(true);
  const [collectibleAddFailed, setCollectibleAddFailed] = useState(false);
+  const trackEvent = useContext(MetaMetricsContext);
+
+  const getCollectibleStandard = async () => {


There is an existing method getTokenStandardAndDetails on the AssetsContractController that we currently use elsewhere in the extension that could provide the standard as a returned value. It will query for more than just the standard, but perhaps we want to include more data like the asset name when available? cc @brad-decker @kevinghim

metamaskbot · 2022-04-12T15:22:36Z

Builds ready [a82b0a8]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1532 ± 122 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	1142	180	225	108
		domContentLoaded	1144	2087	1509	257	123
		load	1171	2087	1532	255	122
		domInteractive	1144	2087	1509	257	123

highlights:

storybook

adonesky1 · 2022-04-12T18:38:56Z

ui/pages/add-collectible/add-collectible.js

      address,
+      selectedAddress,


We actually don't need this here since it is not used for non-ERC20 tokens. Its a weird function signature that we will change (to accept an object) but for now you can and should put the second arg as null and no longer need to get the selectedAddress from state.

adonesky1 · 2022-04-13T17:12:11Z

@segun just want to make sure you see my comment here

…when auto detection occurs for NFTs Set source property to detected on the new token added event, Signed-off-by: Akintayo A. Olusegun <[email protected]> leave undefined properties undefined (not empty strings) Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]> Shorten checks. Signed-off-by: Akintayo A. Olusegun <[email protected]> Case insensitive address comparison Signed-off-by: Akintayo A. Olusegun <[email protected]> Use the isEqualCaseInsensitive of string-utils. Signed-off-by: Akintayo A. Olusegun <[email protected]> Change event name to Collectible Added and source to manual Signed-off-by: Akintayo A. Olusegun <[email protected]> Add collectible on handleAddCollectible. Signed-off-by: Akintayo A. Olusegun <[email protected]> Remove un-used code. Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]>

Signed-off-by: Akintayo A. Olusegun <[email protected]>

metamaskbot · 2022-04-15T16:51:54Z

Builds ready [2aa736a]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1717 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	198	129	23	11
		domContentLoaded	1505	1880	1693	112	54
		load	1506	1967	1717	116	56
		domInteractive	1505	1880	1693	112	54

adonesky1 · 2022-04-15T16:54:58Z

@segun another comment still to address here: #14279 (comment)

adonesky1 · 2022-04-18T14:13:33Z

ui/pages/add-collectible/add-collectible.js

+        tokenId: tokenId.toString(),
+        asset_type: ASSET_TYPES.COLLECTIBLE,
+        token_standard: tokenDetails?.standard,
+        source: 'custom',


just a question what does source: 'custom' mean?

custom means its added manually. possible options right now are 'custom' | 'dapp' | 'list' | 'detected'

adonesky1

On non blocking question but LGTM

digiwand

👍 can we update the description too please?

currently, it says:

Set source property to manual on the new token added event

and we've updated the code to set the source to "custom"

adonesky1 · 2022-04-21T18:15:22Z

@brad-decker have your "requested changes" been addressed? Currently this PR is only blocked by that request.

brad-decker · 2022-04-21T18:24:19Z

me: Welp, this is good to go..why aren't they merging it.
also me: .... 🤦🏻‍♂️

segun requested a review from a team as a code owner March 30, 2022 20:29

segun requested a review from brad-decker March 30, 2022 20:29

segun self-assigned this Mar 30, 2022

segun marked this pull request as draft March 30, 2022 20:33

brad-decker reviewed Mar 30, 2022

View reviewed changes

digiwand reviewed Mar 31, 2022

View reviewed changes

segun force-pushed the dev-olu-add-collectible-event-metrics branch from 1fd0762 to 2f959d8 Compare April 4, 2022 17:22

segun marked this pull request as ready for review April 4, 2022 17:22

digiwand previously approved these changes Apr 4, 2022

View reviewed changes

digiwand mentioned this pull request Apr 4, 2022

Add event metrics on NFT support in extension #13477

Closed

12 tasks

segun dismissed digiwand’s stale review via 221d864 April 5, 2022 07:46

segun force-pushed the dev-olu-add-collectible-event-metrics branch from 221d864 to 64993fa Compare April 5, 2022 08:27

adonesky1 reviewed Apr 5, 2022

View reviewed changes

digiwand previously approved these changes Apr 5, 2022

View reviewed changes

segun requested review from adonesky1 and brad-decker April 5, 2022 21:20

jpuri previously approved these changes Apr 6, 2022

View reviewed changes

adonesky1 reviewed Apr 6, 2022

View reviewed changes

brad-decker suggested changes Apr 6, 2022

View reviewed changes

adonesky1 reviewed Apr 6, 2022

View reviewed changes

segun dismissed stale reviews from jpuri and digiwand via 0e9facd April 6, 2022 18:16

segun force-pushed the dev-olu-add-collectible-event-metrics branch from e7cba3e to cbd6e1a Compare April 7, 2022 10:51

segun changed the title ~~Add new token added event (duplicating the existing event structure) whe collectible is manually added~~ Add new token added event (duplicating the existing event structure) when collectible is manually added Apr 8, 2022

brad-decker suggested changes Apr 8, 2022

View reviewed changes

adonesky1 reviewed Apr 12, 2022

View reviewed changes

segun requested review from adonesky1, jpuri, digiwand and brad-decker April 15, 2022 16:03

segun added 6 commits April 15, 2022 20:04

Add asset type and token standard to events

af534cc

Signed-off-by: Akintayo A. Olusegun <[email protected]>

use already existing getTokenStandardAndDetails method.

5df0336

Signed-off-by: Akintayo A. Olusegun <[email protected]>

Use Added Token + type/standard

e43fa86

Signed-off-by: Akintayo A. Olusegun <[email protected]>

getTokenStandardAndDetails can accept null as user address.

c704cf9

Signed-off-by: Akintayo A. Olusegun <[email protected]>

Lint fix

2aa736a

Signed-off-by: Akintayo A. Olusegun <[email protected]>

segun force-pushed the dev-olu-add-collectible-event-metrics branch from b2b69c0 to 2aa736a Compare April 15, 2022 16:04

adonesky1 reviewed Apr 18, 2022

View reviewed changes

adonesky1 approved these changes Apr 18, 2022

View reviewed changes

digiwand approved these changes Apr 19, 2022

View reviewed changes

brad-decker approved these changes Apr 21, 2022

View reviewed changes

adonesky1 merged commit 095cc94 into develop Apr 21, 2022

adonesky1 deleted the dev-olu-add-collectible-event-metrics branch April 21, 2022 18:29

github-actions bot locked and limited conversation to collaborators Apr 21, 2022

	name: newCollectible.name ? newCollectible.name : null,
	name: newCollectible.name \|\| null,

Add new token added event (duplicating the existing event structure) when collectible is manually added #14279

Add new token added event (duplicating the existing event structure) when collectible is manually added #14279

Conversation

segun commented Mar 30, 2022 • edited Loading

More information

github-actions bot commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Mar 30, 2022

digiwand left a comment • edited Loading

Choose a reason for hiding this comment

digiwand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Apr 5, 2022

adonesky1 Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Apr 5, 2022

digiwand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brad-decker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segun commented Apr 6, 2022

metamaskbot commented Apr 6, 2022

metamaskbot commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Apr 12, 2022

highlights:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 commented Apr 13, 2022

metamaskbot commented Apr 15, 2022

adonesky1 commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

digiwand left a comment • edited Loading

Choose a reason for hiding this comment

adonesky1 commented Apr 21, 2022

brad-decker commented Apr 21, 2022

segun commented Mar 30, 2022 •

edited

Loading

digiwand left a comment •

edited

Loading

adonesky1 Apr 5, 2022 •

edited

Loading

adonesky1 Apr 6, 2022 •

edited

Loading

adonesky1 Apr 12, 2022 •

edited

Loading

digiwand left a comment •

edited

Loading