-
Notifications
You must be signed in to change notification settings - Fork 102
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: estimated execution price #5308
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
192dad2
to
e2c0feb
Compare
98def08
to
ef9f307
Compare
let sellAmount: string | ||
let partiallyFillable = isPartiallyFillable | ||
|
||
if (order) { |
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.
Here we handle the case when the order exists
// Check what's left to sell, discounting the surplus, if any | ||
sellAmount = getRemainderAmountsWithoutSurplus(order).sellAmount | ||
partiallyFillable = order.partiallyFillable | ||
} else { |
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.
Here is the case when we have the order parameters only
return new Price(order.inputToken, order.outputToken, '0', '0') | ||
let feasibleExecutionPrice: Price<Currency, Currency> | undefined = undefined | ||
|
||
if (partiallyFillable) { |
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.
Here we apply the fee multiplier strategy for a fixed estimated fill price
// Regular case, when the order is fill or kill OR the fill amount is smaller than the threshold set | ||
if (feasibleExecutionPrice === undefined) { |
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.
Here's the case for fill or kills, or small partial fill amounts
*/ | ||
feasibleExecutionPrice = new Price(inputToken, outputToken, denominator.quotient, numerator.quotient) | ||
|
||
console.log(`getEstimatedExecutionPrice: fill or kill`, { |
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.
TODO: remove logs before merging
// // Make the fill price a bit worse to account for the fee | ||
const newFillPrice = | ||
extrapolatePriceBasedOnFeeAmount(feeAmount, remainingSellAmount, fillPrice, inputToken, outputToken) || fillPrice |
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.
And here we make the fill price slightly worse to account for the fee
newSellAmount.subtract(feeAmount).quotient, // TODO: should we use the fee with margin here? | ||
buyAmount.quotient, | ||
) | ||
console.log(`getEstimatedExecutionPrice: partially fillable`, { |
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.
TODO: remove before merging
useSafeEffect(() => { | ||
updateLimitRateState({ feeAmount }) | ||
}, [feeAmount, updateLimitRateState]) |
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.
We need to have the fee stored in the state for the estimated execution price calculation
Note: this fee is volume aware! In the future or as follow up, we could quote a fixed USD amount for partial fills.
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.
this fee is volume aware
I think it worth it to reflect this fact in the value name.
feeAmount
-> feeAmountFromQuote
or smth else
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.
Hey @alfetopito , thank you!
- But the app is crashed in this PR:
- I connected to this account on Ethereum 0x9FA3c00a92Ec5f96B1Ad2527ab41B3932EFEDa58
- Swap container was prefilled with selling 456 WETH per USDC
- Then I opened Limit orders page and it crashed
Also, I can catch a crash when pick a different token pair (COW/WETH as an example), and start edititng the sell amount.
341ea58
to
1eca8a1
Compare
apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTableContainer/OrderRow/index.tsx
Outdated
Show resolved
Hide resolved
* fix: memoize and error handle market price calculation on ExecutionPriceUpdater * fix: error handling SpotPriceUpdater * fix: avoid division by 0 * fix: don't be stupid and create invalid fractions * fix: use console.error for errors instead of debug * fix: numerator and denominator are inverted in the price constructor * fix: skip est. exec. price display when it's 0 * fix: use est. exec. price calculated locally
* Limit price: 182000 / 100 = 1820 USDC per 1 WETH | ||
* | ||
* Fee with margin: 0.002 + 5% = 0.0021 WETH | ||
* Executes at: 182000 / (100 - 0.0021) = 1820.038 USDC per 1 WETH |
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.
Thanks for the example!
It makes it clear!
Summary
Overhaul estimated fill price in the form (and a bit in the table)
Overloaded
getEstimatedExecutionPrice
to be used in theExecutionPriceUpdater
It now can also take order parameters without having an existing order
partially fillable
Now it'll use a fixed sell amount to make estimation stable regardless of order size
The amount is currently set to
50x1000xfeeAmount
From there, we use the chosen limit price (already discounted for potential fees) to derive the buy amount
Lastly, the
feeAmount
is deduced from the calculated sell amount, to give us the new estimated priceIf the order is fill or kill OR the amount calculated above is bigger than actual sell amount, use the old way considering the whole sell amount
One last adjustment, is to use the same method above to add the fee to the market price as well
We know we cannot execute exactly at the spot price because we need to take fees into account
So the same
50x1000x the fee strategy is applied to the spot price in case the limit price is below market.To Test
fill or kill
and partially fillableTODO