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 parallel calls for quotes in swaps #12484

Merged
merged 10 commits into from
Oct 28, 2021
Merged

Fix parallel calls for quotes in swaps #12484

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d018eb5
Add a counter for how many quotes are being fetched in parallel
dan437 Oct 25, 2021
6652f55
Fix fetching quotes in parallel
dan437 Oct 26, 2021
200aa3c
Merge branch 'develop' of https://github.com/MetaMask/metamask-extens…
dan437 Oct 26, 2021
a21ea60
Remove this.activeFetchingQuotesCounter
dan437 Oct 26, 2021
1af0bf5
Merge branch 'develop' of https://github.com/MetaMask/metamask-extens…
dan437 Oct 26, 2021
ca92992
Merge branch 'develop' of https://github.com/MetaMask/metamask-extens…
dan437 Oct 27, 2021
a5e69d2
Update when we call the "this.setIsFetchingQuotes" fn, refactoring
dan437 Oct 27, 2021
304ebba
Revert "Update when we call the "this.setIsFetchingQuotes" fn, refact…
dan437 Oct 27, 2021
5e3e053
Merge branch 'develop' of https://github.com/MetaMask/metamask-extens…
dan437 Oct 27, 2021
63b7598
isFetchingQuotesEnabled -> saveFetchedQuotes
dan437 Oct 27, 2021
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
25 changes: 16 additions & 9 deletions app/scripts/controllers/swaps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const initialState = {
routeState: '',
swapsFeatureIsLive: true,
useNewSwapsApi: false,
isFetchingQuotes: false,
isFetchingQuotesEnabled: false,
swapsQuoteRefreshTime: FALLBACK_QUOTE_REFRESH_TIME,
swapsQuotePrefetchingRefreshTime: FALLBACK_QUOTE_REFRESH_TIME,
},
Expand Down Expand Up @@ -209,7 +209,11 @@ export default class SwapsController {
) {
const { chainId } = fetchParamsMetaData;
const {
swapsState: { useNewSwapsApi, quotesPollingLimitEnabled },
swapsState: {
useNewSwapsApi,
quotesPollingLimitEnabled,
isFetchingQuotesEnabled,
},
} = this.store.getState();

if (!fetchParams) {
Expand All @@ -230,7 +234,9 @@ export default class SwapsController {
const indexOfCurrentCall = this.indexOfNewestCallInFlight + 1;
this.indexOfNewestCallInFlight = indexOfCurrentCall;

this.setIsFetchingQuotes(true);
if (!isFetchingQuotesEnabled) {
this.setIsFetchingQuotesEnabled(true);
}

let [newQuotes] = await Promise.all([
this._fetchTradesInfo(fetchParams, {
Expand All @@ -241,18 +247,19 @@ export default class SwapsController {
]);

const {
swapsState: { isFetchingQuotes },
swapsState: {
isFetchingQuotesEnabled: isFetchingQuotesEnabledAfterResponse,
},
} = this.store.getState();

// If isFetchingQuotes is false, it means a user left Swaps (we cleaned the state)
// If isFetchingQuotesEnabledAfterResponse is false, it means a user left Swaps (we cleaned the state)
// and we don't want to set any API response with quotes into state.
if (!isFetchingQuotes) {
if (!isFetchingQuotesEnabledAfterResponse) {
return [
{}, // quotes
null, // selectedAggId
];
}
this.setIsFetchingQuotes(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this line removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was the problem if 2 calls are being made in parallel. Both of them set it to true before the API call, then when the first one finished, it set it to false and when the second one finished, it was already false, so we returned empty quotes and showed the "No quotes found" page. Without this line 2 parallel calls work correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

but what is the consequence of never setting it to false? was this needed before?

Copy link
Contributor

Choose a reason for hiding this comment

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

should we use this.indexOfNewestCallInFlight === indexOfCurrentCall to determine whether to call this.setIsFetchingQuotes(false);

if (this.indexOfNewestCallInFlight === indexOfCurrentCall) {
this.setIsFetchingQuotes(false);
}

This would ensure that this.setIsFetchingQuotes(false); still gets called, but not if there is another call happening in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it's only set to false from initialState when a user leaves Swaps. We don't really need to set it to false here. However, I've used the code you suggested and it works well with it, so I've already included it in this PR.


newQuotes = mapValues(newQuotes, (quote) => ({
...quote,
Expand Down Expand Up @@ -559,10 +566,10 @@ export default class SwapsController {
this.store.updateState({ swapsState: { ...swapsState, routeState } });
}

setIsFetchingQuotes(status) {
setIsFetchingQuotesEnabled(status) {
const { swapsState } = this.store.getState();
this.store.updateState({
swapsState: { ...swapsState, isFetchingQuotes: status },
swapsState: { ...swapsState, isFetchingQuotesEnabled: status },
});
}

Expand Down
2 changes: 1 addition & 1 deletion app/scripts/controllers/swaps.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ const EMPTY_INIT_STATE = {
swapsQuoteRefreshTime: 60000,
swapsQuotePrefetchingRefreshTime: 60000,
swapsUserFeeLevel: '',
isFetchingQuotes: false,
isFetchingQuotesEnabled: false,
},
};

Expand Down