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

WT-1760 Return resolved dex quotes if any reject #945

Merged
merged 8 commits into from
Oct 10, 2023
Merged

WT-1760 Return resolved dex quotes if any reject #945

merged 8 commits into from
Oct 10, 2023

Conversation

imx-mikhala
Copy link
Contributor

@imx-mikhala imx-mikhala commented Oct 5, 2023

Summary

This PR ensures that if any of the dex quotes reject then the resolved ones will still return.

Also adds in a check to make sure we do not call the dex with a matching to/from token as this is not a valid dex call.

@imx-mikhala imx-mikhala requested a review from a team as a code owner October 5, 2023 06:26
@@ -25,6 +25,7 @@ export const quoteFetcher = async (

// Create a quote for each swappable token
for (const swappableToken of swappableTokens) {
if (swappableToken === requiredToken.address) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

@imx-mikhala shouldn't this at the start of the quoteFetcher function?

Copy link
Contributor Author

@imx-mikhala imx-mikhala Oct 9, 2023

Choose a reason for hiding this comment

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

nope, we are looping through an array of swappable tokens and checking that the swappable token for that loop is not the same as the required token we want to get token pair quotes for

dexTransactionResponse.forEach((response, index) => {
if (response.status === 'rejected') return; // Ignore any requests to dex that failed to resolve
Copy link
Contributor

Choose a reason for hiding this comment

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

@imx-mikhala this is 👍 however shall we expose the reason for this promise to be rejected (e.g. missing liquidity pool)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do you think is best to surface this? just by logging?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are fetching quotes as a best effort thing. No need to really tell the user if one quote failed to resolve. I'd think about what the output of this should be if all quote responses were erroring.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ZacharyCouchman I think if there is an error that could be mitigated by the user and we couldn't find any swap options then responding with that might give them a pathway forward.. not sure what those error types would be though. But otherwise might just be too much noise as it is best effort.

@imx-mikhala imx-mikhala merged commit 7efffe3 into main Oct 10, 2023
6 checks passed
@imx-mikhala imx-mikhala deleted the WT-1760 branch October 10, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants