MetaMetrics: Identify 'number_of_tokens' user trait #14427
Conversation
digiwand
requested review from
georgewrmarshall,
brad-decker and
segun
and removed request for
a team
|
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. |
| _getNumberOfTokens(metamaskState) { | ||
| const allUniqueAddresses = Object.values(metamaskState.allTokens).reduce( | ||
| (result, tokensByAccountByChain) => { | ||
| Object.values(tokensByAccountByChain).forEach((tokensByAccount) => { | ||
| tokensByAccount.forEach((token) => { | ||
| result.add(token.address); | ||
| }); | ||
| }); | ||
|
|
||
| return result; | ||
| }, | ||
| new Set(), | ||
| ); | ||
|
|
||
| return allUniqueAddresses.size; |
There was a problem hiding this comment.
Its not necessary to do a unique check here, The addresses represent contracts, even if a token manages to have the same address on many different networks (possible, I think, but difficult) the user would have different balances on each chain effectively making them different tokens. It's also possible for two different tokens to have the same address on different chains.
There was a problem hiding this comment.
😮 Thank you for the catch and for explaining this. Maybe there is a guide I can read to help me better understand the structure of token contracts
Updating the code to not do the unique filter here.
There was a problem hiding this comment.
@brad-decker updated here: 0bd0772
| ); | ||
|
|
||
| return allUniqueAddresses.size; | ||
| } |
There was a problem hiding this comment.
I think I got this right... "tokensByAccountByChain" is what I presumed from looking through the TokensController and the data produced. Please double-check me here 🙏
I am also wondering two other things:
- How large could the data set get, and would it be worth it to use lodash memoize?
- May we want to move this logic into the TokensController someday?
There was a problem hiding this comment.
@brad-decker Thoughts here? I'm not familiar enough with this controller to answer these questions.
There was a problem hiding this comment.
- memoizing functions might be worth while, but that can happen down the road. We should discuss the proper level of memoization (should we memoize the entire build user trait function, only portions of the traits, etc)
- No, although it seems like we will end up with a bunch of methods related to other parts of state, they are building user traits which is specific to meta metrics. I feel all user traits should be built and established inside the metametrics controller regardless of where in state they come from
There was a problem hiding this comment.
Thank you for the thoughts!
- 👍
- I thought, maybe, the total number of tokens could be shown in other places. Thanks for confirming that this method wouldn't be desirable in the TokenController. I am all for keeping the user trait logic inside the MetaMetrics controller
Builds ready [2447399]Page Load Metrics (1270 ± 41 ms)
|
Builds ready [0bd0772]Page Load Metrics (1252 ± 47 ms)
|
do not filter by unique addresses. Each token contract x chain id combo is a unique contract
- add number_of_tokens
digiwand
force-pushed
the
metametrics/identify-number-of-tokens
branch
from
5be5191 to
1227724
Compare
|
There was a moment I forgot this branch was branching off #14367. After I force pushed on #14367, I had duplicate commits that I had to clean up. I force pushed a new version. There have been no changes in logic in the last couple of days; cc: @brad-decker |
Builds ready [76cdece]Page Load Metrics (1378 ± 55 ms)
|
Explanation
Add two new user traits to Segment:
number_of_tokens - Number of Tokens
Note: We want the count of all of the tokens across all of the chains as mentioned by @brad-decker here: #14421 (comment)
More information
Progresses #13477