-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Comments
Note that we are not doing #1 for first release |
@kevinghim For 2a, you just want us to track an event when the either of the options is toggled, correct? |
@kevinghim What do you want tracked for 2bii? That seems to not be related to NFTs |
@adonesky1 For 2bi, we just need to track an event whenever the polling results in a successful add |
@brad-decker I guess you can advise on how to handle 3? |
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. |
Should 2a be a user trait? @brad-decker |
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.
Again these are user traits versus events.
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.
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.
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? |
@brad-decker agree with your comments.
This makes sense.
Yes, this also makes sense.
This would be in addition to the "Token Added" flow on the "is _base_asset" transaction properties, right? I agree. |
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. |
Hi Team, Question about the user trait naming. I am working on adding:
Our current convention to send string values to Segment uses snake case. This would mean the above would translate to:
|
I think we can remove
from the "Tasks" list. Can someone confirm this? As discussed in #14279 (comment),
cc: @brad-decker @segun |
@digiwand @brad-decker @segun are we adding tracking to the |
oops, I believe this closed automatically after I directly linked the related PR to this issue and merged that PR in. Reopening |
@digiwand did you end up creating a ticket for tracking collectible detection events? |
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
I have updated the previous task item:
with
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? |
@digiwand awesome thanks! Yes available to discuss anytime this week! |
@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. |
@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 |
@NiranjanaBinoy is doing the same thing on TokenDetection V2 with "Token Detected" occurring in the detect tokens controller |
@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 |
Closing this as completed |
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 usera. Assets vs NFTs
i. Asset types and value
ii. Number of NFTs and NFT types (floor value if possible)
b. Number of NFT collections
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
b. NFTs added via detection
i. Added via NFT detection
ii. Added via Token detection
c. Manually added
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
Tasks
asset_type
usingASSET_TYPES
from transactions consttoken_standard
usingTOKEN_STANDARDS
from 'ui/helpers/constants/common'CollectionDetectionController
and acceptance criteria # 3aCollectionDetectionController
and acceptance criteria # 3basset_type
to transaction events (done in add asset_type to transactions #13858)token_standard
to transaction events (done in add asset_type to transactions #13858)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.All collectibles has this shape:
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.
The text was updated successfully, but these errors were encountered: