Skip to content
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

Add event metrics on NFT support in extension #13477

Closed
12 tasks done
kevinghim opened this issue Jan 31, 2022 · 22 comments · Fixed by #14252, #14253 or #14495
Closed
12 tasks done

Add event metrics on NFT support in extension #13477

kevinghim opened this issue Jan 31, 2022 · 22 comments · Fixed by #14252, #14253 or #14495
Labels
area-metrics area-NFTs team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead

Comments

@kevinghim
Copy link
Contributor

kevinghim commented Jan 31, 2022

Describe the bug

Notion reference: https://www.notion.so/NFT-support-in-extension-v1-844f8c3e72e044c4b8d6f7da7e24b646#c9bf0bdb95e044378b6b382953e76a20

Acceptance Criteria

1. Usage of NFTs. Goal: create asset value per user metric. i.e. NFT: $15 per user, Non-NFT: $80 per user
a. Assets vs NFTs
i. Asset types and value
ii. Number of NFTs and NFT types (floor value if possible)
b. Number of NFT collections

nft 1a
nft 1b

2. Detection utility. Goal: elevate usage of detection that’ll lead to improvements of detection overall.
a. NFT detection allowed vs disabled
i. Centralized detection
ii. Non-centralized detection

nft 2a

b. NFTs added via detection
i. Added via NFT detection
ii. Added via Token detection

nft 2b
nft 2bii

c. Manually added

nft 2c

  1. Flow analysis. Goal: track flow of user journey to determine drop-offs and exits
    a. Adding flow
    i. NFT detected (2bii) → NFT detection (centralized (2ai) / non-centralized (2aii)) → Added via NFT detection (2bi)
    ii. Manual add (2c) → Added via manual
    b. Sending flow
    i. NFT Details
    ii. Address Added
    iii. NFT Sent

nft 3bi
nft 3bii
nft 3biii

Tasks

Collectibles state help

Collectibles state comes from the CollectiblesController from metamask/controllers. The key in our state tree we'll want to use is allCollectibles. So in the handleMetaMaskUpdate function on the MetaMetrics controller introduced in #14192 you will add some new keys to the traits object for the various tasks above specifying add new user trait.

/**
 * @type Collectible
 *
 * Collectible representation
 * @property address - Hex address of a ERC721 contract
 * @property description - The collectible description
 * @property image - URI of custom collectible image associated with this tokenId
 * @property name - Name associated with this tokenId and contract address
 * @property tokenId - The collectible identifier
 * @property numberOfSales - Number of sales
 * @property backgroundColor - The background color to be displayed with the item
 * @property imagePreview - URI of a smaller image associated with this collectible
 * @property imageThumbnail - URI of a thumbnail image associated with this collectible
 * @property imageOriginal - URI of the original image associated with this collectible
 * @property animation - URI of a animation associated with this collectible
 * @property animationOriginal - URI of the original animation associated with this collectible
 * @property externalLink - External link containing additional information
 * @property creator - The collectible owner information object
 * @property isCurrentlyOwned - Boolean indicating whether the address/chainId combination where it's currently stored currently owns this collectible
 */

All collectibles has this shape:

 allCollectibles: { [key: string (account)]: { [key: string (chainId)]: Collectible[] } };

This is an object whose keys are accounts and the values are objects where the keys are chainIds. So, for example, in order to get an aggregate number of NFTs imported you'll have to go through each chainId in each account in this object and sum the length of the Collectibles array.

In order to get the number of collections you'll need to do the same sort of thing but you'll want the number of unique 'address' properties on the entries. Each collectible has an address property that indicates which collection it is a part of.

@danjm
Copy link
Contributor

danjm commented Feb 1, 2022

Note that we are not doing #1 for first release

@danjm
Copy link
Contributor

danjm commented Feb 1, 2022

@kevinghim For 2a, you just want us to track an event when the either of the options is toggled, correct?

@danjm
Copy link
Contributor

danjm commented Feb 1, 2022

@kevinghim What do you want tracked for 2bii? That seems to not be related to NFTs

@danjm
Copy link
Contributor

danjm commented Feb 1, 2022

@adonesky1 For 2bi, we just need to track an event whenever the polling results in a successful add

@danjm
Copy link
Contributor

danjm commented Feb 1, 2022

@brad-decker I guess you can advise on how to handle 3?

@adonesky1
Copy link
Contributor

adonesky1 commented Feb 1, 2022

What do you want tracked for 2bii? That seems to not be related to NFTs

This actually may make sense to track as part of NFT metrics. It occurs when users attempt to add an NFT in the Import-Token (ERC20) flow and get the blocking error message that redirects them to the Import NFT flow.

@bschorchit
Copy link

@kevinghim For 2a, you just want us to track an event when the either of the options is toggled, correct?

Should 2a be a user trait? @brad-decker

@brad-decker
Copy link
Contributor

Usage of NFTs. Goal: create asset value per user metric. i.e. NFT: $15 per user, Non-NFT: $80 per user
a. Assets vs NFTs
i. Asset types and value
ii. Number of NFTs and NFT types (floor value if possible)
b. number of NFT collections

These all make sense as user traits instead of events. We don't yet have the methods required for tracking user traits built, but it should be a high priority item as we are rapidly increasing our metrics and we are in the position to create more analytics debt if we don't do this now.

Detection utility. Goal: elevate usage of detection that’ll lead to improvements of detection overall.
a. NFT detection allowed vs disabled
i. Centralized detection
ii. Non-centralized detection

Again these are user traits versus events.

b. NFTs added via detection
i. Added via NFT detection
ii. Added via Token detection

Might i suggest modifying the "Token added" event with a "type" property set to "ERC-20, ERC-721, or ERC-1155" ? This allows us to capture future "token" types that are possibly not NFTs or ERC-20s without changing our schema.

c. Manually added

Also in terms of Manually vs detection don't have two separate events. We already have a "source" property on the "Token Added" that is one of "List | Dapp | Custom" with the last option being manual entry. Lets add a fourth option "Detected" to capture the additional source.

Flow analysis. Goal: track flow of user journey to determine drop-offs and exits
a. Adding flow
i. NFT detected (2bii) → NFT detection (centralized (2ai) / non-centralized (2aii)) → Added via NFT detection (2bi)
ii. Manual add (2c) → Added via manual
b. Sending flow
i. NFT Details
ii. Address Added
iii. NFT Sent

We already have events for sending transactions, and in #13423 we are adding "is_base_asset" property to transactions. Maybe we should revise that with asset_type: "native | erc-20 | erc-721 | erc-1155" which would then give us the ability to break down the normal send flow and segment by Asset type.

As for the add flow, the entire add process is captured with the "Token Added" event with the parameter i suggested.

@kevinghim @bschorchit @adonesky1 @danjm thoughts?

@kevinghim
Copy link
Contributor Author

kevinghim commented Feb 2, 2022

@brad-decker agree with your comments.

Might i suggest modifying the "Token added" event with a "type" property set to "ERC-20, ERC-721, or ERC-1155" ? This allows us to capture future "token" types that are possibly not NFTs or ERC-20s without changing our schema.

This makes sense.

We already have a "source" property on the "Token Added" that is one of "List | Dapp | Custom" with the last option being manual entry. Lets add a fourth option "Detected" to capture the additional source.

Yes, this also makes sense.

Maybe we should revise that with asset_type: "native | erc-20 | erc-721 | erc-1155" which would then give us the ability to break down the normal send flow and segment by Asset type.

This would be in addition to the "Token Added" flow on the "is _base_asset" transaction properties, right? I agree.

@adonesky1
Copy link
Contributor

Might i suggest modifying the "Token added" event with a "type" property set to "ERC-20, ERC-721, or ERC-1155" ? This allows us to capture future "token" types that are possibly not NFTs or ERC-20s without changing our schema.

Agreed. In general I feel like we should be future proofing our functional division of erc20 tokens and NFTs a lot more, but that's a separate conversation.

But yes I totally agree with @brad-decker that we should try to keep as much of the collectible/NFT metrics within the family of our broader token events as possible.

@digiwand
Copy link
Contributor

digiwand commented Apr 6, 2022

Hi Team,

Question about the user trait naming. I am working on adding:

  • 'NFT Autodetection Enabled'
  • 'OpenSeas API Enabled'
  • 'Number of NFTs'
  • 'Number of Tokens'
  • 'Number of NFT collections'

Our current convention to send string values to Segment uses snake case. This would mean the above would translate to:

  • nft_autodetection_enabled
  • opensea_api_enabled (I removed the extra "s". Please let me know if someone feels strongly against this. I am matching what the setting is called in the codebase.)
  • number_of_nfts
  • number_of_tokens
  • number_of_nft_collections
  1. Is this the right conversion?

  2. Have we considered prepending preference/settings traits with settings_ or enabled_ instead of the opposite way around? This could provide some ease to search for settings in our analytic platforms.

cc: @brad-decker @segun @kevinghim

@digiwand
Copy link
Contributor

digiwand commented Apr 20, 2022

I think we can remove

[ ] Set source property to detected on the new token added event

from the "Tasks" list. Can someone confirm this?

As discussed in #14279 (comment),

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

cc: @brad-decker @segun

@adonesky1
Copy link
Contributor

@digiwand @brad-decker @segun are we adding tracking to the CollectibleDetectionController for detection events yet?

@digiwand
Copy link
Contributor

oops, I believe this closed automatically after I directly linked the related PR to this issue and merged that PR in. Reopening

@digiwand digiwand reopened this Apr 21, 2022
@adonesky1
Copy link
Contributor

@digiwand did you end up creating a ticket for tracking collectible detection events?

@digiwand
Copy link
Contributor

@adonesky1

Hello 👋 I mentioned to @brad-decker, @danjm, and @segun last Friday about speaking to you and then realizing the need to update the task list.

After you inquired about adding tracking for CollectionDetectionController and mentioned that TokenDetectionV2 is believed to handle tracking for only for ERC20 tokens, I realized we were missing task(s) to handle this following acceptance criteria:

#3 Flow analysis. Goal: track flow of user journey to determine drop-offs and exits

I have updated the previous task item:

[ ] Set source property to detected on the new token added event

with

[ ] Add metric tracking for NFT detection. See CollectionDetectionController

to support the acceptance criteria #3.

Thank you for the catch here @adonesky1! Will you still be available to meet tomorrow or later this week about handling this metric tracking for NFT detection?

@adonesky1
Copy link
Contributor

@digiwand awesome thanks! Yes available to discuss anytime this week!

@digiwand
Copy link
Contributor

@adonesky1 Thank you! I will Slack you to schedule a time.

In regards to your question about creating an issue for this new task, there have been no individual issues created for any of these tasks. All of the specs live in this issue ticket.

@brad-decker
Copy link
Contributor

@digiwand we want BOTH of those things not one or the other. We need an event for when a token is detected and a property on the Token Added for if it was added as a result of being detected

@brad-decker
Copy link
Contributor

@NiranjanaBinoy is doing the same thing on TokenDetection V2 with "Token Detected" occurring in the detect tokens controller

@digiwand
Copy link
Contributor

@brad-decker Oh, thank you for the clarification! I was confused by this because I'm not familiar with the token detection systems yet. Re-adding the task to the task list now

@hilvmason hilvmason added the team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead label Feb 6, 2023
@kevinghim
Copy link
Contributor Author

Closing this as completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-metrics area-NFTs team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
7 participants