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

v2 quote with fee-on-transfer tax considerations #144

Merged
merged 11 commits into from
Sep 11, 2023

Conversation

jsy1218
Copy link
Member

@jsy1218 jsy1218 commented Sep 8, 2023

When we quote against v2 pairs, if the token(s) is/are fee-on-transfer tokens, we essentially do not take them off the amountIn/amountOut. This is incorrect, and we want to consider the fee-on-transfer tokens when calculating the quotes in smart-order-router quote-provider getQuotes.

One thing is that the reserves are not updated. Most likely fee-on-transfer tokens do deplete the reserves, so the correct calculations should take off of the reserves. However since smart-order-router refreshes reserves per block, and router doesn't persisted its own reserve state, we don't have to update the reserve calculation here.

@jsy1218 jsy1218 self-assigned this Sep 8, 2023
Copy link

@cgkol cgkol left a comment

Choose a reason for hiding this comment

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

awesome commments!! left a piece of feedback need to review further

src/entities/pair.ts Show resolved Hide resolved
src/entities/pair.ts Show resolved Hide resolved
Copy link
Contributor

@marktoda marktoda left a comment

Choose a reason for hiding this comment

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

the logic LGTM, nice work!

src/entities/pair.ts Outdated Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/entities/pair.ts Outdated Show resolved Hide resolved
src/entities/pair.ts Outdated Show resolved Hide resolved
@jsy1218 jsy1218 force-pushed the jsy1218/v2-pair-quote-consider-fot-fees branch from 38d19d2 to ff3671b Compare September 11, 2023 18:54
@jsy1218 jsy1218 merged commit ff80edf into main Sep 11, 2023
1 check passed
@jsy1218 jsy1218 deleted the jsy1218/v2-pair-quote-consider-fot-fees branch September 11, 2023 19:03
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