Feat: Add minAverageSellPrice for LO #736
Conversation
| @@ -243,6 +243,57 @@ func (s *DexTestSuite) TestPlaceLimitOrderInsufficientFunds() { | |||
| s.assertAliceLimitSellFails(err, "TokenA", 0, 10) | |||
| } | |||
|
|
|||
| func (s *DexTestSuite) TestPlaceLimitOrderWithDustMinAvgPriceFails() { | |||
There was a problem hiding this comment.
world's smallest nit, but naming these
TestPlaceLimitOrderGTCWithDustMinAvgPriceFails
TestPlaceLimitOrderGTCWithDustMinAvgPrice
for consistency with the IOC tests
| OrderType: types.LimitOrderType_GOOD_TIL_CANCELLED, | ||
| MinAverageSellPrice: &limitPrice, | ||
| }) | ||
| s.ErrorIs(err, types.ErrLimitPriceNotSatisfied) |
There was a problem hiding this comment.
I think the error you are getting this place not because of the new logic, but because of the rounding nature. And thats why
- LimitPrice converted from price to tick to tick and back in a msgHandler, and in the code limitPrice lower than MinAverageSellPrice (despite the fact that you set them the same)
006736936405289611298728644<0.006737 - On every swap iteration we check
if limitPrice != nil && liq.Price().LT(*limitPrice) {
break
}
in other words we swap only and only if liq.Price>limitPrice
If every price > limitPrice, it's impossible the avg deal true price < limitPrice.
swap debug via 4 deposits:
price: 0.006738957688326027277211891
inAmount: 149 outAmount: 1
price: 0.006738283859940033273884502
inAmount: 149 outAmount: 1
price: 0.006737610098930140259858517
inAmount: 149 outAmount: 1
price: 0.006736936405289611298728644
inAmount: 1485 outAmount: 10
seems like only because of accumulated rounding errors we got realsellprice 13/1932=0.006728778467908902691511387 < limitPrice
I would suggest you to add two (at least) test cases, i mean and fail and success
- with bigger deposits and swaps amounts
- with a large spread between ticks to make the meaning of
MinAverageSellPricemore obvious
There was a problem hiding this comment.
This is the intended functionality. The goal was to be able to trade through a specific tick but tolerate a certain amount of price increase due to rounding.
There is another possible use case, which I believe you are describing where MinAverageSellPrice > LimitSellPrice. This would allow trading through a certain tick, but require the average price to be better than the last tick traded through. I will add some tests for this.
There was a problem hiding this comment.
Oh, I see. I thought the "another possible use case" is the intended functionality :) Something like: "I want to sell my 10BTC for 500USDT and i do not care about highest and lowest price, i just want my average". Confused with the name. If you wanna just be tolerant to the rounding error, did you think about another approach, e.g. instead of 'MinAverageSellPrice', introduce the bool flag "beRoundingTolerant" and do not check at all truePrice if true, or may be roundingTollerance as offset ticks, i.e. the limit of rounding error which is allowed to bypass, in the nutshell the second approach is close to what you did, but it looks more intuitive for me.
There was a problem hiding this comment.
We went back and forth on this a lot. Just being ok with all rounding allows the possibility of front running, since there is no cap on how bad rounding can get. So it's better Passing an explicit value is much safer. Both use cases are valid. I will update docs to better explain the usage for this param
| NewDeposit(0, 10, 70, 1), | ||
| NewDeposit(0, 10, 80, 1), | ||
| ) | ||
| // THEN alice places IOC limitOrder with very low MinAveragePrice Fails |
There was a problem hiding this comment.
With very high? (not "low")
| OrderType: types.LimitOrderType_GOOD_TIL_CANCELLED, | ||
| MinAverageSellPrice: &limitPrice, | ||
| }) | ||
| s.ErrorIs(err, types.ErrLimitPriceNotSatisfied) |
There was a problem hiding this comment.
Oh, I see. I thought the "another possible use case" is the intended functionality :) Something like: "I want to sell my 10BTC for 500USDT and i do not care about highest and lowest price, i just want my average". Confused with the name. If you wanna just be tolerant to the rounding error, did you think about another approach, e.g. instead of 'MinAverageSellPrice', introduce the bool flag "beRoundingTolerant" and do not check at all truePrice if true, or may be roundingTollerance as offset ticks, i.e. the limit of rounding error which is allowed to bypass, in the nutshell the second approach is close to what you did, but it looks more intuitive for me.
Adds a new field for placing a limit order
MinAverageSellPrice. This provides more control for users when placing a limit order and resolves a number of edge cases. Currently, we uselimitSellPriceto set the highest tick we try to trade through, and as part of a final check to make sure the average trade price for the entire trade is above thelimitSellPrice.MinAverageSellPriceis a new optional parameter that allows the user to set a limit for the final aggregate/average price different from what is specified by thelimitSellPrice.