Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: trying to access an undefined object in swaps review quote compo… #27708

Merged
merged 4 commits into from
Oct 9, 2024
+3 −1
Merged

fix: trying to access an undefined object in swaps review quote compo… #27708

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05c0651
fix: trying to access an undefined object in swaps review quote compo…
jclancy93 Oct 8, 2024
461c2f5
chore: minimize diff and remove log
jclancy93 Oct 9, 2024
039ba76
Merge branch 'develop' into MMS-1569-swap-quote-crash
jclancy93 Oct 9, 2024
7f128cf
chore: remove unused import
jclancy93 Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ui/pages/swaps/prepare-swap-page/prepare-swap-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
getTransactionSettingsOpened,
setTransactionSettingsOpened,
getLatestAddedTokenTo,
getUsedQuote,
} from '../../../ducks/swaps/swaps';
import {
getSwapsDefaultToken,
Expand Down Expand Up @@ -190,9 +191,10 @@ export default function PrepareSwapPage({
const rpcPrefs = useSelector(getRpcPrefsForCurrentProvider, shallowEqual);
const tokenList = useSelector(getTokenList, isEqual);
const quotes = useSelector(getQuotes, isEqual);
const usedQuote = useSelector(getUsedQuote, isEqual);
const latestAddedTokenTo = useSelector(getLatestAddedTokenTo, isEqual);
const numberOfQuotes = Object.keys(quotes).length;
const areQuotesPresent = numberOfQuotes > 0;
const areQuotesPresent = numberOfQuotes > 0 && usedQuote;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions, possibly not worth digging into right now but things we could consider trying to straighten out in the future:
(1) What's the reason usedQuote may not exist if numberOfQuotes > 0?
(2) Is numberOfQuotes > 0 adding anything here or would we just want to use the presence/absence of usedQuote?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea for sure, good questions:

1.

This is what I touched on a bit in the PR description usedQuote is derived from getSelectedQuote or getTopQuote, which both use a key to access the quotes object selectedAggId or topAggId. After many changes, there is a single or a couple of render cycles where this is quotes are defined, but usedQuote is not. usedQuote should never be undefined if quotes are defined, but that does not seem to be true always, which is the root cause of this bug. I tried finding out why, but it took me deep into the extension redux setup, which I'm not too familiar with. That's why I opted for a more bandaid fix like this

2.

I think we could probably remove the length check here, but since the checks accomplish similar things to usedQuote and I'd rather minimize the changes required for the PR, I decided to keep it in. If you think it's worth keeping the checks simpler, I can try it out though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it in for now - no harm in it 🤷

const swapsErrorKey = useSelector(getSwapsErrorKey);
const aggregatorMetadata = useSelector(getAggregatorMetadata, shallowEqual);
const transactionSettingsOpened = useSelector(
Expand Down