-
Notifications
You must be signed in to change notification settings - Fork 191
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: trade impact is including protocol fees for chainflip #8556
Conversation
98fa0b8
to
59292a5
Compare
99bf34d
to
0cec0f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Math does look sane/r than before (i.e use the input/output fiat rate to calculate a pseudo amount before fees as a prorate of sorts) for smolish amounts, tested against develop (right):
Added some suggested changes re: dependency-injecting the fiat rate between sell/buy ourselves so we don't rely on upstream with market-data discrepancies which could cause issues in calculations (either being off at best, or having a negative number at worst), though happy to have that as a follow-up 🙏🏽
Description
Chainflip price impact was wrong because amount out was excluding fees
I used a shortcut by calculating the inAmount with the rate chainflip send to us, so the price impact is nearer from the right one now
If you want to verify other price impacts, keep in mind that we do with what we have, some quotes provide great data to have an accurate price impact, some others are pretty hard to provide an accurate price impact and it's more an estimation than a precise price impact
For chainflip it's pretty close to the true now
Issue (if applicable)
closes #8514
Risk
Low
Testing
Engineering
n/a
Operations
n/a
Screenshots (if applicable)