-
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
fix: undefined market data selector #26264
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ import { | |
checkNetworkAndAccountSupports1559, | ||
getCurrentChainId, | ||
getMetaMaskAccounts, | ||
getTokenExchangeRates, | ||
} from './selectors'; | ||
import { | ||
getUnapprovedTransactions, | ||
|
@@ -153,19 +154,6 @@ export const txDataSelector = (state) => state.confirmTransaction.txData; | |
const tokenDataSelector = (state) => state.confirmTransaction.tokenData; | ||
const tokenPropsSelector = (state) => state.confirmTransaction.tokenProps; | ||
|
||
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; | ||
}, | ||
{}, | ||
); | ||
}; | ||
Comment on lines
-156
to
-167
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This chunk was identical to the |
||
|
||
const tokenDecimalsSelector = createSelector( | ||
tokenPropsSelector, | ||
(tokenProps) => tokenProps && tokenProps.decimals, | ||
|
@@ -218,7 +206,7 @@ export const sendTokenTokenAmountAndToAddressSelector = createSelector( | |
); | ||
|
||
export const contractExchangeRateSelector = createSelector( | ||
contractExchangeRatesSelector, | ||
(state) => getTokenExchangeRates(state), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd think
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error is caused by the function being 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. |
||
tokenAddressSelector, | ||
(contractExchangeRates, tokenAddress) => { | ||
return contractExchangeRates[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code below could probably look up via the checksummed address instead of the O(n) case insensitive search |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.