prefer chainId over networkId in most cases#10594
Conversation
|
When we say "in most cases", what does that represent? We might want to document that. |
|
@darkwing https://github.com/MetaMask/metamask-extension/pull/10594/files#diff-ad1f68882a19fa4247caaa355b64dd6a6ae6c4e3c67b51c5a23e4727a0c91c01R86-R93 I added some inline documentation to the selector for networkId, but it might not be sufficient. I'll update the PR description to reflect this as well, but did you have some thoughts on where this documentation should live? |
8dc0e44 to
1d181b8
Compare
Builds ready [1d181b8]
Page Load Metrics (642 ± 25 ms)
|
1d181b8 to
452e9f2
Compare
Builds ready [452e9f2]
Page Load Metrics (622 ± 15 ms)
|
app/scripts/metamask-controller.js
Outdated
| properties: { | ||
| action: 'Transactions', | ||
| errorMessage: txMeta.simulationFails?.reason, | ||
| numberOfTokens: metamaskState.tokens?.length ?? 0, |
There was a problem hiding this comment.
Doesn't tokens default to a []? Is there a case where that property doesn't exist?
There was a problem hiding this comment.
background on this change: I saw that we had a 'getBackgroundMetaMetrics' selector in the UI folder that is only called from the background and I wanted to kill it with fire. I copied the intent of the selector 'getNumberOfTokens' from the frontend here which checks if tokens is specified.
app/scripts/metamask-controller.js
Outdated
| action: 'Transactions', | ||
| errorMessage: txMeta.simulationFails?.reason, | ||
| numberOfTokens: metamaskState.tokens?.length ?? 0, | ||
| numberOfAccounts: Object.keys(metamaskState.accounts ?? {}) |
There was a problem hiding this comment.
Same as above with accounts and {}
There was a problem hiding this comment.
Same as above, but here I did add something that didn't exist before. I'll check into both of these and simplify the selectors if they can be.
|
I added a commit that changes the name of
between this and the @deprecated flag, I feel like there is a strong signal to not use this. |
Builds ready [7212382]
Page Load Metrics (587 ± 9 ms)
|
7212382 to
0066733
Compare
Builds ready [0066733]
Page Load Metrics (626 ± 18 ms)
|
|
@ryanml ready for another when you can. |
Bug Fixes