Skip to content
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.

Sign up for GitHub

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

Feat: Add minAverageSellPrice for LO #736

Merged
merged 10 commits into from
Oct 14, 2024
Merged

Feat: Add minAverageSellPrice for LO #736

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1d1c83a
add minAverageSellPrice
jcompagni10 Sep 20, 2024
4cf7dd3
comments + fmt
jcompagni10 Sep 23, 2024
744bfc8
Merge remote-tracking branch 'origin/main' into feat/lo_slop_tolerance
jcompagni10 Sep 24, 2024
0abd365
Add param to SimulatePlaceLimitOrder
jcompagni10 Sep 24, 2024
9e6a99b
fix merge
jcompagni10 Sep 24, 2024
1848d68
fix tests
jcompagni10 Sep 24, 2024
f289c3e
fix: better test naming
jcompagni10 Sep 25, 2024
a5c3314
Merge remote-tracking branch 'origin/main' into feat/lo_slop_tolerance
jcompagni10 Oct 11, 2024
78730d7
fix proto version
jcompagni10 Oct 11, 2024
4fbfc01
Add additional test for MinAveragePrice > LimitSellPrice
jcompagni10 Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions proto/neutron/dex/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,15 @@ message MsgPlaceLimitOrder {
(gogoproto.nullable) = true,
(gogoproto.jsontag) = "limit_sell_price"
];
// min_average_sell_price is an optional parameter that sets a required minimum average price for the entire trade.
// if the min_average_sell_price is not met the trade will fail.
// If min_average_sell_price is omitted limit_sell_price will be used instead
string min_average_sell_price = 12 [
(gogoproto.moretags) = "yaml:\"min_average_sell_price\"",
(gogoproto.customtype) = "github.com/neutron-org/neutron/v4/utils/math.PrecDec",
(gogoproto.nullable) = true,
(gogoproto.jsontag) = "min_average_sell_price"
];
}

message MsgPlaceLimitOrderResponse {
Expand Down
1 change: 1 addition & 0 deletions x/dex/keeper/grpc_query_estimate_place_limit_order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func (k Keeper) EstimatePlaceLimitOrder(
req.OrderType,
req.ExpirationTime,
req.MaxAmountOut,
nil,
callerAddr,
receiverAddr,
)
Expand Down
1 change: 1 addition & 0 deletions x/dex/keeper/grpc_query_simulate_place_limit_order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func (k Keeper) SimulatePlaceLimitOrder(
msg.OrderType,
msg.ExpirationTime,
msg.MaxAmountOut,
msg.MinAverageSellPrice,
receiverAddr,
)
if err != nil {
Expand Down
93 changes: 93 additions & 0 deletions x/dex/keeper/integration_placelimitorder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,57 @@ func (s *DexTestSuite) TestPlaceLimitOrderInsufficientFunds() {
s.assertAliceLimitSellFails(err, "TokenA", 0, 10)
}

func (s *DexTestSuite) TestPlaceLimitOrderGTCWithDustMinAvgPriceFails() {
s.fundAliceBalances(1, 0)
s.fundBobBalances(0, 1)
// GIVEN LP liq at 148.37-148.42 (with dust)
s.bobDeposits(
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_000, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_001, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_002, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(10), 50_003, 1),
)
// THEN alice GTC limitOrder minAvgPrice == limitPrice fails
limitPrice := math_utils.MustNewPrecDecFromStr("0.006737")
_, err := s.msgServer.PlaceLimitOrder(s.Ctx, &types.MsgPlaceLimitOrder{
Creator: s.alice.String(),
Receiver: s.alice.String(),
TokenIn: "TokenA",
TokenOut: "TokenB",
LimitSellPrice: &limitPrice,
AmountIn: sdkmath.NewInt(2000),
OrderType: types.LimitOrderType_GOOD_TIL_CANCELLED,
MinAverageSellPrice: &limitPrice,
})
s.ErrorIs(err, types.ErrLimitPriceNotSatisfied)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error you are getting this place not because of the new logic, but because of the rounding nature. And thats why

  1. 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
  2. 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

  1. with bigger deposits and swaps amounts
  2. with a large spread between ticks to make the meaning of MinAverageSellPrice more obvious

Copy link
Contributor Author

@jcompagni10 jcompagni10 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

func (s *DexTestSuite) TestPlaceLimitOrderGTCWithDustMinAvgPrice() {
s.fundAliceBalances(1, 0)
s.fundBobBalances(0, 1)
// GIVEN LP liq at 148.37-148.42 (with dust)
s.bobDeposits(
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_000, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_001, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_002, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(10), 50_003, 1),
)
// THEN alice IoC limitOrder with lower min avg prices success
limitPrice := math_utils.MustNewPrecDecFromStr("0.006737")
minAvgPrice := math_utils.MustNewPrecDecFromStr("0.006728")
_, err := s.msgServer.PlaceLimitOrder(s.Ctx, &types.MsgPlaceLimitOrder{
Creator: s.alice.String(),
Receiver: s.alice.String(),
TokenIn: "TokenA",
TokenOut: "TokenB",
LimitSellPrice: &limitPrice,
AmountIn: sdkmath.NewInt(2000),
OrderType: types.LimitOrderType_GOOD_TIL_CANCELLED,
MinAverageSellPrice: &minAvgPrice,
})
s.NoError(err)
}

func (s *DexTestSuite) TestLimitOrderPartialFillDepositCancel() {
s.fundAliceBalances(100, 100)
s.fundBobBalances(100, 100)
Expand Down Expand Up @@ -303,6 +354,48 @@ func (s *DexTestSuite) TestPlaceLimitOrderWithDustHitsTruePriceLimit() {
s.assertAliceLimitSellFails(types.ErrLimitPriceNotSatisfied, "TokenA", 20005, 1, types.LimitOrderType_IMMEDIATE_OR_CANCEL)
}

func (s *DexTestSuite) TestPlaceLimitOrderIOCWithDustMinAvgPriceFails() {
s.fundAliceBalances(1, 0)
s.fundBobBalances(0, 1)
// GIVEN LP liq at 148.37-148.42 (with dust)
s.bobDeposits(
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_000, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_001, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_002, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(10), 50_003, 1),
)
// THEN alice IoC limitOrder minAvgPrice == limitPrice fails
_, err := s.aliceLimitSellsWithMinAvgPrice(
"TokenA",
math_utils.MustNewPrecDecFromStr("0.006730"),
1,
math_utils.MustNewPrecDecFromStr("0.006730"),
types.LimitOrderType_IMMEDIATE_OR_CANCEL,
)
s.ErrorIs(err, types.ErrLimitPriceNotSatisfied)
}

func (s *DexTestSuite) TestPlaceLimitOrderIOCWithDustMinAvgPrice() {
s.fundAliceBalances(1, 0)
s.fundBobBalances(0, 1)
// GIVEN LP liq at 148.37-148.42 (with dust)
s.bobDeposits(
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_000, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_001, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(1), 50_002, 1),
NewDepositInt(sdkmath.ZeroInt(), sdkmath.NewInt(10), 50_003, 1),
)
// THEN alice IoC limitOrder with lower min avg prices success
_, err := s.aliceLimitSellsWithMinAvgPrice(
"TokenA",
math_utils.MustNewPrecDecFromStr("0.006730"),
1,
math_utils.MustNewPrecDecFromStr("0.006727"),
types.LimitOrderType_IMMEDIATE_OR_CANCEL,
)
s.NoError(err)
}

func (s *DexTestSuite) TestPlaceLimitOrderWithDust() {
s.fundAliceBalances(1, 0)
s.fundBobBalances(0, 1)
Expand Down
8 changes: 5 additions & 3 deletions x/dex/keeper/liquidity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func (k Keeper) TakerLimitOrderSwap(
amountIn math.Int,
maxAmountOut *math.Int,
limitPrice math_utils.PrecDec,
minAvgSellPrice math_utils.PrecDec,
orderType types.LimitOrderType,
) (totalInCoin, totalOutCoin sdk.Coin, err error) {
totalInCoin, totalOutCoin, orderFilled, err := k.SwapWithCache(
Expand All @@ -150,7 +151,7 @@ func (k Keeper) TakerLimitOrderSwap(

truePrice := math_utils.NewPrecDecFromInt(totalOutCoin.Amount).QuoInt(totalInCoin.Amount)

if truePrice.LT(limitPrice) {
if truePrice.LT(minAvgSellPrice) {
return sdk.Coin{}, sdk.Coin{}, types.ErrLimitPriceNotSatisfied
}

Expand All @@ -164,6 +165,7 @@ func (k Keeper) MakerLimitOrderSwap(
tradePairID types.TradePairID,
amountIn math.Int,
limitPrice math_utils.PrecDec,
minAvgSellPrice math_utils.PrecDec,
) (totalInCoin, totalOutCoin sdk.Coin, filled bool, err error) {
totalInCoin, totalOutCoin, filled, err = k.SwapWithCache(
ctx,
Expand All @@ -178,11 +180,11 @@ func (k Keeper) MakerLimitOrderSwap(

if totalInCoin.Amount.IsPositive() {
remainingIn := amountIn.Sub(totalInCoin.Amount)
expectedOutMakerPortion := limitPrice.MulInt(remainingIn).Ceil()
expectedOutMakerPortion := limitPrice.MulInt(remainingIn)
totalExpectedOut := expectedOutMakerPortion.Add(math_utils.NewPrecDecFromInt(totalOutCoin.Amount))
truePrice := totalExpectedOut.QuoInt(amountIn)

if truePrice.LT(limitPrice) {
if truePrice.LT(minAvgSellPrice) {
return sdk.Coin{}, sdk.Coin{}, false, types.ErrLimitPriceNotSatisfied
}
}
Expand Down
1 change: 1 addition & 0 deletions x/dex/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ func (k MsgServer) PlaceLimitOrder(
msg.OrderType,
msg.ExpirationTime,
msg.MaxAmountOut,
msg.MinAverageSellPrice,
callerAddr,
receiverAddr,
)
Expand Down
47 changes: 47 additions & 0 deletions x/dex/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,39 @@ func (s *DexTestSuite) limitSellsWithMaxOut(
return msg.TrancheKey
}

func (s *DexTestSuite) aliceLimitSellsWithMinAvgPrice(
selling string,
limitPrice math_utils.PrecDec,
amountIn int,
minAvgPrice math_utils.PrecDec,
orderType types.LimitOrderType,
) (*types.MsgPlaceLimitOrderResponse, error) {
return s.limitSellsWithMinAvgPrice(s.alice, selling, limitPrice, amountIn, minAvgPrice, orderType)
}

func (s *DexTestSuite) limitSellsWithMinAvgPrice(
account sdk.AccAddress,
tokenIn string,
limitPrice math_utils.PrecDec,
amountIn int,
minAvgPrice math_utils.PrecDec,
orderType types.LimitOrderType,
) (*types.MsgPlaceLimitOrderResponse, error) {
tokenIn, tokenOut := dexkeeper.GetInOutTokens(tokenIn, "TokenA", "TokenB")

return s.msgServer.PlaceLimitOrder(s.Ctx, &types.MsgPlaceLimitOrder{
Creator: account.String(),
Receiver: account.String(),
TokenIn: tokenIn,
TokenOut: tokenOut,
TickIndexInToOut: 0,
LimitSellPrice: &limitPrice,
AmountIn: sdkmath.NewInt(int64(amountIn)).Mul(denomMultiple),
OrderType: orderType,
MinAverageSellPrice: &minAvgPrice,
})
}

func (s *DexTestSuite) limitSellsWithPrice(
account sdk.AccAddress,
tokenIn string,
Expand Down Expand Up @@ -2006,6 +2039,7 @@ func TestMsgPlaceLimitOrderValidate(t *testing.T) {

ZEROINT := sdkmath.ZeroInt()
ONEINT := sdkmath.OneInt()
ZERODEC := math_utils.ZeroPrecDec()
TINYDEC := math_utils.MustNewPrecDecFromStr("0.000000000000000000000000494")
HUGEDEC := math_utils.MustNewPrecDecFromStr("2020125331305056766452345.127500016657360222036663652")
FIVEDEC := math_utils.NewPrecDec(5)
Expand Down Expand Up @@ -2177,6 +2211,19 @@ func TestMsgPlaceLimitOrderValidate(t *testing.T) {
},
types.ErrInvalidPriceAndTick,
},
{
"invalid zero min average sell price",
types.MsgPlaceLimitOrder{
Creator: sample.AccAddress(),
Receiver: sample.AccAddress(),
TokenIn: "TokenA",
TokenOut: "TokenB",
LimitSellPrice: &FIVEDEC,
AmountIn: sdkmath.OneInt(),
MinAverageSellPrice: &ZERODEC,
},
types.ErrZeroMinAverageSellPrice,
},
}

for _, tt := range tests {
Expand Down
14 changes: 12 additions & 2 deletions x/dex/keeper/place_limit_order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func (k Keeper) PlaceLimitOrderCore(
orderType types.LimitOrderType,
goodTil *time.Time,
maxAmountOut *math.Int,
minAvgSellPrice *math_utils.PrecDec,
callerAddr sdk.AccAddress,
receiverAddr sdk.AccAddress,
) (trancheKey string, totalInCoin, swapInCoin, swapOutCoin sdk.Coin, err error) {
Expand All @@ -38,6 +39,7 @@ func (k Keeper) PlaceLimitOrderCore(
orderType,
goodTil,
maxAmountOut,
minAvgSellPrice,
receiverAddr,
)
if err != nil {
Expand Down Expand Up @@ -100,16 +102,24 @@ func (k Keeper) ExecutePlaceLimitOrder(
orderType types.LimitOrderType,
goodTil *time.Time,
maxAmountOut *math.Int,
minAvgSellPriceP *math_utils.PrecDec,
receiverAddr sdk.AccAddress,
) (trancheKey string, totalIn math.Int, swapInCoin, swapOutCoin sdk.Coin, sharesIssued math.Int, err error) {
amountLeft := amountIn

var limitPrice math_utils.PrecDec
limitPrice, err = types.CalcPrice(tickIndexInToOut)

if err != nil {
return trancheKey, totalIn, swapInCoin, swapOutCoin, math.ZeroInt(), err
}

// Use limitPrice for minAvgSellPrice if it has not be specified
minAvgSellPrice := limitPrice
if minAvgSellPriceP != nil {
minAvgSellPrice = *minAvgSellPriceP
}

// Ensure that after rounding user will get at least 1 token out.
err = types.ValidateFairOutput(amountIn, limitPrice)
if err != nil {
Expand All @@ -118,9 +128,9 @@ func (k Keeper) ExecutePlaceLimitOrder(

var orderFilled bool
if orderType.IsTakerOnly() {
swapInCoin, swapOutCoin, err = k.TakerLimitOrderSwap(ctx, *takerTradePairID, amountIn, maxAmountOut, limitPrice, orderType)
swapInCoin, swapOutCoin, err = k.TakerLimitOrderSwap(ctx, *takerTradePairID, amountIn, maxAmountOut, limitPrice, minAvgSellPrice, orderType)
} else {
swapInCoin, swapOutCoin, orderFilled, err = k.MakerLimitOrderSwap(ctx, *takerTradePairID, amountIn, limitPrice)
swapInCoin, swapOutCoin, orderFilled, err = k.MakerLimitOrderSwap(ctx, *takerTradePairID, amountIn, limitPrice, minAvgSellPrice)
}
if err != nil {
return trancheKey, totalIn, swapInCoin, swapOutCoin, math.ZeroInt(), err
Expand Down
5 changes: 5 additions & 0 deletions x/dex/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,9 @@ var (
1163,
"Cannot convert price to int64 tick value",
)
ErrZeroMinAverageSellPrice = sdkerrors.Register(
ModuleName,
1164,
"MinAverageSellPrice must be nil or > 0.",
)
)
4 changes: 4 additions & 0 deletions x/dex/types/message_place_limit_order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ func (msg *MsgPlaceLimitOrder) Validate() error {
return ErrInvalidPriceAndTick
}

if msg.MinAverageSellPrice != nil && msg.MinAverageSellPrice.IsZero() {
return ErrZeroMinAverageSellPrice
}

return nil
}

Expand Down
Loading
Loading