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.

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

Merged
merged 10 commits into from
Oct 28, 2021
Merged

Conversation

dan437
Copy link
Contributor

@dan437 dan437 commented Oct 26, 2021

Description

Sometimes our UI triggered 2 API calls for quotes in parallel, which resulted in "No quotes found" page. This PR fixes it.

@dan437 dan437 requested a review from a team as a code owner October 26, 2021 09:19
@github-actions
Copy link
Contributor

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.

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.

meppsilon
meppsilon previously approved these changes Oct 26, 2021
danjm
danjm previously approved these changes Oct 27, 2021
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM

darkwing
darkwing previously approved these changes Oct 27, 2021
@dan437 dan437 dismissed stale reviews from darkwing and danjm via 5e3e053 October 27, 2021 15:22
@dan437 dan437 merged commit e85248a into MetaMask:develop Oct 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2021
@dan437 dan437 deleted the fix-parallel-calls branch July 24, 2023 11:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants