Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

fix: cowswap buyAmountCryptoBaseUnit logic #1230

Merged
merged 3 commits into from
Apr 3, 2023
Merged

Conversation

0xApotheosis
Copy link
Contributor

@0xApotheosis 0xApotheosis commented Apr 3, 2023

There is a logic error in the getCowSwapTradeQuote function.

If we attempt to sell an amount that is less than the CoW Swap minimum, we get a quote using the minimum amount to ensure we still get a quote back.

The issue was that we are always passing the buy asset amount that this minimum sell amount would buy (from the quote response) in the buyAmountCryptoBaseUnit field, regardless of if we made the minimum or not.

This is leading to inflated CoW Swap ratios which were rugging swapper selection UI, among a host of other bugs relating to minimum amounts.

To test this you'll need to be running shapeshift/web#4177 on web, then:

  1. Attempt to replicate the bug in Miner Fee trading out of FOX is $0.00 when entering a USD amount web#4168
  2. With REACT_APP_FEATURE_TRADE_RATES enabled, we should no longer see negative amounts for CoW Swap, and it should be in it's correct place in the list (i.e. not "best" when it's worse than 0x) - trading a very small amount of FOX for ETH is a good way to get rates from CoW Swap, THORSwap and 0x and test this.

Closes shapeshift/web#4168

Note for you @gomesalexandre, as I know you'll find it(!) - this does not yet fix the minimum amounts issue, which is separate, and fixed in shapeshift/web#4178.

@0xApotheosis 0xApotheosis requested a review from a team as a code owner April 3, 2023 06:05
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested in conjunction with shapeshift/web#4177:

  1. With an amount lower than the minimum, CoW swapper is in its correct position (i.e not best, since a trade with this amount would fail so the receive amount is effectively 0)

image

  1. Miner fees are correctly displayed, see above

  2. When selecting CoW, the receive amount (both in the input and min. expected after fees) shown doesn't account for the fee (i.e should be zero), though I believe this is a web concern and fixed in a follow-up PRs in the stack?

image

image

  1. As noted in the description, minimum amounts are still incorrect, i.e 1400 FOX shows "Insufficient sell amount" currently but 1500 doesn't

image

According to CoWSwap's UI, the minimum should be ~126 FOX (though with very unfavourable receive amount where fees make up for ~87% of the sell amount, I believe with our heuristics the actual amount we deem to be minimum should be around 350 FOX e.g fees being ~40% of the sell amount)

image

image

If we confirm 3. isn't a matter of this PR, happy to get it and its matching web PR in

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Confirmed with big @0xApotheosis 3. isn't a concern of this PR (doesn't bring any regression and doesn't look like a discrepancy as long as the flag is off)

@gomesalexandre gomesalexandre enabled auto-merge (squash) April 3, 2023 09:03
@gomesalexandre gomesalexandre merged commit c24b9e1 into main Apr 3, 2023
@gomesalexandre gomesalexandre deleted the fix-cowswap-logic branch April 3, 2023 09:03
shapeshift-ci-bot pushed a commit that referenced this pull request Apr 3, 2023
# [@shapeshiftoss/swapper-v17.6.2](https://github.com/shapeshift/lib/compare/@shapeshiftoss/swapper-v17.6.1...@shapeshiftoss/swapper-v17.6.2) (2023-04-03)

### Bug Fixes

* cowswap buyAmountCryptoBaseUnit logic ([#1230](#1230)) ([c24b9e1](c24b9e1))
@shapeshift-ci-bot
Copy link
Member

🎉 This PR is included in version @shapeshiftoss/swapper-v17.6.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Miner Fee trading out of FOX is $0.00 when entering a USD amount
3 participants