fix: undefined market data selector #26264
Conversation
|
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. |
| const contractExchangeRatesSelector = (state) => { | ||
| const chainId = getCurrentChainId(state); | ||
| const contractMarketData = state.metamask.marketData?.[chainId]; | ||
|
|
||
| return Object.entries(contractMarketData).reduce( | ||
| (acc, [address, marketData]) => { | ||
| acc[address] = marketData?.price ?? null; | ||
| return acc; | ||
| }, | ||
| {}, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This chunk was identical to the getTokenExchangeRates selector, except it did not include a ?? {} to handle the undefined case. So I just made it re-use that selector instead.
| @@ -218,7 +206,7 @@ export const sendTokenTokenAmountAndToAddressSelector = createSelector( | |||
| ); | |||
|
|
|||
| export const contractExchangeRateSelector = createSelector( | |||
| contractExchangeRatesSelector, | |||
| (state) => getTokenExchangeRates(state), | |||
There was a problem hiding this comment.
You'd think getTokenExchangeRates would work here instead of a lambda, but the tests don't like it. I suspect that's why the original code duplicated the selector.
FAIL app/scripts/metamask-controller.actions.test.js
● Test suite failed to run
Selector creators expect all input-selectors to be functions, instead received the following types: [undefined, function]
206 | );
207 |
> 208 | export const contractExchangeRateSelector = createSelector(
| ^
209 | getTokenExchangeRates,
I've encountered this situation a few times, and never discovered why it happens. I think you can also fix it by stubbing the selector in the tests. But the lambda seems to work too.
There was a problem hiding this comment.
This error is caused by the function being undefined at the point in time this is executed. This could happen because the function is defined later in the module (as a variable, so not hoisted to the top like function declarations are), but more often I've seen this happen due to a circular dependency between the current file and the file we're importing the selector from.
One of the tech debt tasks in-progress now is an effort to remove all circular dependencies in our codebase and implement a lint rule preventing their re-introduction, so hopefully we can resolve this soon. But in the meantime this solution works.
| @@ -218,7 +206,7 @@ export const sendTokenTokenAmountAndToAddressSelector = createSelector( | |||
| ); | |||
|
|
|||
| export const contractExchangeRateSelector = createSelector( | |||
| contractExchangeRatesSelector, | |||
| (state) => getTokenExchangeRates(state), | |||
| tokenAddressSelector, | |||
| (contractExchangeRates, tokenAddress) => { | |||
| return contractExchangeRates[ | |||
There was a problem hiding this comment.
Code below could probably look up via the checksummed address instead of the O(n) case insensitive search
Builds ready [c4ea0a6]
Page Load Metrics (215 ± 225 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26264 +/- ##
===========================================
- Coverage 69.95% 69.94% -0.01%
===========================================
Files 1411 1411
Lines 49963 49966 +3
Branches 13800 13801 +1
===========================================
- Hits 34948 34947 -1
- Misses 15015 15019 +4 ☔ View full report in Codecov by Sentry. |
|
Something fishier may be going on. This selector should run when there's a pending transaction, not when importing a token. |
|
@bergeron I still the 'Cannot convert undefined or null to object' error |
| @@ -137,7 +137,7 @@ export const ImportTokensModal = ({ onClose }) => { | |||
| const accounts = useSelector(getInternalAccounts); | |||
| const tokens = useSelector((state) => state.metamask.tokens); | |||
| const rpcPrefs = useSelector(getRpcPrefsForCurrentProvider); | |||
| const contractExchangeRates = useSelector(contractExchangeRateSelector); | |||
| const contractExchangeRates = useSelector(getTokenExchangeRates); | |||
There was a problem hiding this comment.
I don't think the previous selector made sense. It only returned the exchange rate of the current token being transacted. Whereas the variable is accessed like an object containing rates for multiple tokens.
With what repro steps |
|
@bergeron I'm using this build #26264 (comment) |
|
I can't reproduce with those steps on that build |
Builds ready [241c39d]
Page Load Metrics (352 ± 305 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
|
Builds ready [e5c9a75]
Page Load Metrics (133 ± 147 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
metamaskbot
added
the
release-12.4.0
gauthierpetetin
added
release-12.3.0
Description
Fixes an issue when
Show balance and token price checkeris turned off in settings, certain actions like sending, importing, or swapping a token crash metamask.Related issues
Fixes: #26243
Manual testing steps
Show balance and token price checkerScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist