Merge pull request #647 from neutron-org/feat/fix-cancel-lo-logic

Feat: fix cancel limit order logic

proto/neutron/dex/limit_order_tranche_user.proto

@@ -24,6 +24,7 @@ message LimitOrderTrancheUser {
     (gogoproto.nullable) = false,
     (gogoproto.jsontag) = "shares_withdrawn"
   ];
+  // TODO: remove this in next release. It is no longer used
   string shares_cancelled = 7 [
     (gogoproto.moretags) = "yaml:\"shares_cancelled\"",
     (gogoproto.customtype) = "cosmossdk.io/math.Int",

x/dex/genesis_test.go

@@ -27,7 +27,6 @@ func TestGenesis(t *testing.T) {
 				Address:               "fakeAddr",
 				SharesOwned:           math.NewInt(10),
 				SharesWithdrawn:       math.NewInt(0),
-				SharesCancelled:       math.NewInt(0),
 			},
 			{
 				TradePairId: &types.TradePairID{
@@ -39,7 +38,6 @@ func TestGenesis(t *testing.T) {
 				Address:               "fakeAddr",
 				SharesOwned:           math.NewInt(10),
 				SharesWithdrawn:       math.NewInt(0),
-				SharesCancelled:       math.NewInt(0),
 			},
 		},
 		TickLiquidityList: []*types.TickLiquidity{

x/dex/keeper/cancel_limit_order.go

@@ -17,16 +17,17 @@ func (k Keeper) CancelLimitOrderCore(
 ) error {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	coinOut, tradePairID, err := k.ExecuteCancelLimitOrder(ctx, trancheKey, callerAddr)
+	makerCoinOut, takerCoinOut, tradePairID, err := k.ExecuteCancelLimitOrder(ctx, trancheKey, callerAddr)
 	if err != nil {
 		return err
 	}
 
+	coinsOut := sdk.NewCoins(makerCoinOut, takerCoinOut)
 	err = k.bankKeeper.SendCoinsFromModuleToAccount(
 		ctx,
 		types.ModuleName,
 		callerAddr,
-		sdk.Coins{coinOut},
+		coinsOut,
 	)
 	if err != nil {
 		return err
@@ -40,7 +41,8 @@ func (k Keeper) CancelLimitOrderCore(
 		pairID.Token1,
 		tradePairID.MakerDenom,
 		tradePairID.TakerDenom,
-		coinOut.Amount,
+		makerCoinOut.Amount,
+		takerCoinOut.Amount,
 		trancheKey,
 	))
 
@@ -54,10 +56,10 @@ func (k Keeper) ExecuteCancelLimitOrder(
 	ctx sdk.Context,
 	trancheKey string,
 	callerAddr sdk.AccAddress,
-) (coinOut sdk.Coin, tradePairID *types.TradePairID, error error) {
+) (makerCoinOut, takerCoinOut sdk.Coin, tradePairID *types.TradePairID, error error) {
 	trancheUser, found := k.GetLimitOrderTrancheUser(ctx, callerAddr.String(), trancheKey)
 	if !found {
-		return sdk.Coin{}, nil, types.ErrActiveLimitOrderNotFound
+		return sdk.Coin{}, sdk.Coin{}, nil, types.ErrActiveLimitOrderNotFound
 	}
 
 	tradePairID, tickIndex := trancheUser.TradePairId, trancheUser.TickIndexTakerToMaker
@@ -70,14 +72,20 @@ func (k Keeper) ExecuteCancelLimitOrder(
 		},
 	)
 	if tranche == nil {
-		return sdk.Coin{}, nil, types.ErrActiveLimitOrderNotFound
+		return sdk.Coin{}, sdk.Coin{}, nil, types.ErrActiveLimitOrderNotFound
 	}
 
-	amountToCancel := tranche.RemoveTokenIn(trancheUser)
-	trancheUser.SharesCancelled = trancheUser.SharesCancelled.Add(amountToCancel)
+	makerAmountToReturn := tranche.RemoveTokenIn(trancheUser)
+	_, takerAmountOut := tranche.Withdraw(trancheUser)
 
-	if !amountToCancel.IsPositive() {
-		return sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey)
+	trancheUser.SharesWithdrawn = trancheUser.SharesOwned
+
+	// Remove the canceled shares from the limitOrder
+	tranche.TotalMakerDenom = tranche.TotalMakerDenom.Sub(trancheUser.SharesOwned)
+	tranche.TotalTakerDenom = tranche.TotalTakerDenom.Sub(takerAmountOut)
+
+	if !makerAmountToReturn.IsPositive() && !takerAmountOut.IsPositive() {
+		return sdk.Coin{}, sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey)
 	}
 
 	k.SaveTrancheUser(ctx, trancheUser)
@@ -86,7 +94,9 @@ func (k Keeper) ExecuteCancelLimitOrder(
 	if trancheUser.OrderType.HasExpiration() {
 		k.RemoveLimitOrderExpiration(ctx, *tranche.ExpirationTime, tranche.Key.KeyMarshal())
 	}
-	coinOut = sdk.NewCoin(tradePairID.MakerDenom, amountToCancel)
 
-	return coinOut, tradePairID, nil
+	makerCoinOut = sdk.NewCoin(tradePairID.MakerDenom, makerAmountToReturn)
+	takerCoinOut = sdk.NewCoin(tradePairID.TakerDenom, takerAmountOut)
+
+	return makerCoinOut, takerCoinOut, tradePairID, nil
 }

x/dex/keeper/integration_cancellimitorder_test.go

@@ -191,9 +191,39 @@ func (s *DexTestSuite) TestCancelPartiallyFilled() {
 	// WHEN alice cancels her limit order
 	s.aliceCancelsLimitSell(trancheKey)
 
-	// Then alice gets back remaining 25 TokenA LO reserves
-	s.assertAliceBalances(25, 0)
-	s.assertDexBalances(0, 25)
+	// Then alice gets back remaining 25 TokenA LO reserves & 25 TokenB taker tokens
+	s.assertAliceBalances(25, 25)
+	s.assertDexBalances(0, 0)
+
+	// Assert that the LimitOrderTrancheUser has been deleted
+	_, found := s.App.DexKeeper.GetLimitOrderTrancheUser(s.Ctx, s.alice.String(), trancheKey)
+	s.Assert().False(found)
+}
+
+func (s *DexTestSuite) TestCancelPartiallyFilledWithdrawFails() {
+	s.fundAliceBalances(50, 0)
+	s.fundBobBalances(0, 10)
+
+	// GIVEN alice limit sells 50 TokenA
+	trancheKey := s.aliceLimitSells("TokenA", 2000, 50)
+	// Bob swaps 10 TokenB for TokenA
+	s.bobLimitSells("TokenB", -2001, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	s.assertDexBalancesInt(sdkmath.NewInt(37786095), sdkmath.NewInt(10000000))
+	s.assertAliceBalances(0, 0)
+
+	// WHEN alice cancels her limit order
+	s.aliceCancelsLimitSell(trancheKey)
+
+	// Then alice gets back remaining ~37 BIGTokenA LO reserves & 10 BIGTokenB taker tokens
+	s.assertAliceBalancesInt(sdkmath.NewInt(37786094), sdkmath.NewInt(10000000))
+	s.assertDexBalancesInt(sdkmath.OneInt(), sdkmath.ZeroInt())
+
+	// Assert that the LimitOrderTrancheUser has been deleted
+	_, found := s.App.DexKeeper.GetLimitOrderTrancheUser(s.Ctx, s.alice.String(), trancheKey)
+	s.Assert().False(found)
+
+	s.aliceWithdrawLimitSellFails(types.ErrValidLimitOrderTrancheNotFound, trancheKey)
 }
 
 func (s *DexTestSuite) TestCancelPartiallyFilledMultiUser() {
@@ -215,12 +245,82 @@ func (s *DexTestSuite) TestCancelPartiallyFilledMultiUser() {
 	s.aliceCancelsLimitSell(trancheKey)
 	s.carolCancelsLimitSell(trancheKey)
 
-	// THEN alice gets back ~41 BIGTokenA (125 * 1/3)
-	s.assertAliceBalancesInt(sdkmath.NewInt(41_666_666), sdkmath.ZeroInt())
+	// THEN alice gets back ~41 BIGTokenA (125 * 1/3) & ~8.3 BIGTokenB Taker tokens (25 * 1/3)
+	s.assertAliceBalancesInt(sdkmath.NewInt(41_666_666), sdkmath.NewInt(8333333))
+
+	// Carol gets back 83 TokenA (125 * 2/3) & ~16.6 BIGTokenB Taker tokens (25 * 2/3)
+	s.assertCarolBalancesInt(sdkmath.NewInt(83_333_333), sdkmath.NewInt(16666667))
+	s.assertDexBalancesInt(sdkmath.OneInt(), sdkmath.ZeroInt())
+
+	// Assert that the LimitOrderTrancheUsers has been deleted
+	_, found := s.App.DexKeeper.GetLimitOrderTrancheUser(s.Ctx, s.alice.String(), trancheKey)
+	s.Assert().False(found)
+	_, found = s.App.DexKeeper.GetLimitOrderTrancheUser(s.Ctx, s.carol.String(), trancheKey)
+	s.Assert().False(found)
+}
+
+func (s *DexTestSuite) TestCancelPartiallyFilledMultiUser2() {
+	s.fundAliceBalances(50, 0)
+	s.fundBobBalances(50, 0)
+	s.fundCarolBalances(0, 40)
+
+	// // GIVEN alice and bob each limit sells 50 TokenA
+	trancheKey := s.aliceLimitSells("TokenA", 2000, 50)
+	s.bobLimitSells("TokenA", 2000, 50)
+	// carol swaps 20 TokenB for TokenA
+	s.carolLimitSells("TokenB", -2001, 20, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice cancels her limit order
+	s.aliceCancelsLimitSell(trancheKey)
+
+	// THEN alice gets back remaining ~38 BIGTokenA LO reserves & 10 BIGTokenB taker tokens
+	s.assertAliceBalancesInt(sdkmath.NewInt(37786094), sdkmath.NewInt(10000000))
+	s.assertDexBalancesInt(sdkmath.NewInt(37786096), sdkmath.NewInt(10000000))
+
+	// THEN carol swap through more of the limitorder
+	s.carolLimitSells("TokenB", -2001, 20, types.LimitOrderType_FILL_OR_KILL)
+
+	// And bob can withdraw his portion
+	s.bobWithdrawsLimitSell(trancheKey)
+	s.assertBobBalancesInt(sdkmath.ZeroInt(), sdkmath.NewInt(30000000))
+}
+
+func (s *DexTestSuite) TestCancelFirstMultiCancel() {
+	s.fundAliceBalances(50, 0)
+	s.fundBobBalances(50, 0)
+	s.fundCarolBalances(0, 40)
+
+	// // GIVEN alice and bob each limit sells 50 TokenA
+	trancheKey := s.aliceLimitSells("TokenA", 0, 50)
+	s.bobLimitSells("TokenA", 0, 50)
+	s.bobCancelsLimitSell(trancheKey)
+	// carol swaps 10 TokenB for TokenA
+	s.carolLimitSells("TokenB", -1, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice cancels her limit order
+	s.aliceCancelsLimitSell(trancheKey)
+
+	// THEN alice gets back remaining 40 tokenA  10 TokenB taker tokens
+	s.assertAliceBalances(40, 10)
+}
+
+func (s *DexTestSuite) TestCancelFirstMultiWithdraw() {
+	s.fundAliceBalances(50, 0)
+	s.fundBobBalances(50, 0)
+	s.fundCarolBalances(0, 40)
+
+	// // GIVEN alice and bob each limit sells 50 TokenA
+	trancheKey := s.aliceLimitSells("TokenA", 0, 50)
+	s.bobLimitSells("TokenA", 0, 50)
+	s.bobCancelsLimitSell(trancheKey)
+	// carol swaps 10 TokenB for TokenA
+	s.carolLimitSells("TokenB", -1, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice withdraws her limit order
+	s.aliceWithdrawsLimitSell(trancheKey)
 
-	// Carol gets back 83 TokenA (125 * 2/3)
-	s.assertCarolBalancesInt(sdkmath.NewInt(83_333_333), sdkmath.ZeroInt())
-	s.assertDexBalancesInt(sdkmath.OneInt(), sdkmath.NewInt(25_000_000))
+	// THEN alice gets 10 TokenB
+	s.assertAliceBalances(0, 10)
 }
 
 func (s *DexTestSuite) TestCancelGoodTil() {

x/dex/keeper/integration_placelimitorder_test.go

@@ -271,31 +271,19 @@ func (s *DexTestSuite) TestLimitOrderPartialFillDepositCancel() {
 
 	s.aliceCancelsLimitSell(trancheKey0)
 
-	s.assertAliceBalances(100, 40)
+	s.assertAliceBalances(110, 40)
 	s.assertBobBalances(90, 110)
-	s.assertDexBalances(10, 50)
+	s.assertDexBalances(0, 50)
 	s.assertCurrentTicks(math.MinInt64, 0)
 
 	s.bobLimitSells("TokenA", 10, 10, types.LimitOrderType_FILL_OR_KILL)
 
-	s.assertAliceBalances(100, 40)
+	s.assertAliceBalances(110, 40)
 	s.assertBobBalances(80, 120)
-	s.assertDexBalances(20, 40)
+	s.assertDexBalances(10, 40)
 
 	s.aliceCancelsLimitSell(trancheKey1)
 
-	s.assertAliceBalances(100, 80)
-	s.assertBobBalances(80, 120)
-	s.assertDexBalances(20, 0)
-
-	s.aliceWithdrawsLimitSell(trancheKey0)
-
-	s.assertAliceBalances(110, 80)
-	s.assertBobBalances(80, 120)
-	s.assertDexBalances(10, 0)
-
-	s.aliceWithdrawsLimitSell(trancheKey1)
-
 	s.assertAliceBalances(120, 80)
 	s.assertBobBalances(80, 120)
 	s.assertDexBalances(0, 0)

x/dex/keeper/integration_withdrawfilled_test.go

@@ -339,9 +339,6 @@ func (s *DexTestSuite) TestWithdrawPartiallyFilledCancelled() {
 
 	// AND alice cancels the remainder
 	s.aliceCancelsLimitSell(trancheKey)
-
-	// THEN she can withdraw the unused portion
-	s.aliceWithdrawsLimitSell(trancheKey)
 	s.assertAliceBalances(1, 1)
 
 	// AND her LimitOrderTrancheUser is removed

x/dex/keeper/limit_order_tranche_user.go

@@ -25,7 +25,6 @@ func (k Keeper) GetOrInitLimitOrderTrancheUser(
 			Address:               receiver,
 			SharesOwned:           math.ZeroInt(),
 			SharesWithdrawn:       math.ZeroInt(),
-			SharesCancelled:       math.ZeroInt(),
 			TickIndexTakerToMaker: tickIndex,
 			TradePairId:           tradePairID,
 			OrderType:             orderType,

x/dex/keeper/limit_order_tranche_user_test.go

@@ -24,7 +24,6 @@ func createNLimitOrderTrancheUser(keeper *keeper.Keeper, ctx sdk.Context, n int)
 			TickIndexTakerToMaker: int64(i),
 			SharesOwned:           math.NewInt(100),
 			SharesWithdrawn:       math.ZeroInt(),
-			SharesCancelled:       math.ZeroInt(),
 		}
 		items[i] = val
 		keeper.SetLimitOrderTrancheUser(ctx, items[i])
@@ -43,7 +42,6 @@ func createNLimitOrderTrancheUserWithAddress(keeper *keeper.Keeper, ctx sdk.Cont
 			TickIndexTakerToMaker: 0,
 			SharesOwned:           math.ZeroInt(),
 			SharesWithdrawn:       math.ZeroInt(),
-			SharesCancelled:       math.ZeroInt(),
 		}
 		items[i] = val
 		keeper.SetLimitOrderTrancheUser(ctx, items[i])
@@ -93,7 +91,7 @@ func (s *DexTestSuite) TestGetAllLimitOrders() {
 		Address:               s.alice.String(),
 		SharesOwned:           math.NewInt(10_000_000),
 		SharesWithdrawn:       math.NewInt(0),
-		SharesCancelled:       math.NewInt(0),
+		SharesCancelled:       math.ZeroInt(),
 	},
 		LOList[0],
 	)
@@ -104,7 +102,7 @@ func (s *DexTestSuite) TestGetAllLimitOrders() {
 		Address:               s.alice.String(),
 		SharesOwned:           math.NewInt(10_000_000),
 		SharesWithdrawn:       math.NewInt(0),
-		SharesCancelled:       math.NewInt(0),
+		SharesCancelled:       math.ZeroInt(),
 	},
 		LOList[1],
 	)

x/dex/keeper/withdraw_filled_limit_order.go

@@ -88,14 +88,14 @@ func (k Keeper) ExecuteWithdrawFilledLimitOrder(
 			remainingTokenIn = tranche.RemoveTokenIn(trancheUser)
 			k.SaveInactiveTranche(ctx, tranche)
 
-			// Treat the removed tokenIn as cancelled shares
-			trancheUser.SharesCancelled = trancheUser.SharesCancelled.Add(remainingTokenIn)
+			// Since the order has already been filled we treat this as a complete withdrawal
+			trancheUser.SharesWithdrawn = trancheUser.SharesOwned
 
 		} else {
 			k.SetLimitOrderTranche(ctx, tranche)
+			trancheUser.SharesWithdrawn = trancheUser.SharesWithdrawn.Add(amountOutTokenIn)
 		}
 
-		trancheUser.SharesWithdrawn = trancheUser.SharesWithdrawn.Add(amountOutTokenIn)
 	}
 
 	k.SaveTrancheUser(ctx, trancheUser)

x/dex/types/events.go

@@ -156,8 +156,9 @@ func CancelLimitOrderEvent(
 	token0 string,
 	token1 string,
 	makerDenom string,
-	tokenOut string,
-	amountOut math.Int,
+	takerDenom string,
+	makerAmountOut math.Int,
+	takerAmountOut math.Int,
 	trancheKey string,
 ) sdk.Event {
 	attrs := []sdk.Attribute{
@@ -167,8 +168,9 @@ func CancelLimitOrderEvent(
 		sdk.NewAttribute(CancelLimitOrderEventToken0, token0),
 		sdk.NewAttribute(CancelLimitOrderEventToken1, token1),
 		sdk.NewAttribute(CancelLimitOrderEventTokenIn, makerDenom),
-		sdk.NewAttribute(CancelLimitOrderEventTokenOut, tokenOut),
-		sdk.NewAttribute(CancelLimitOrderEventAmountOut, amountOut.String()),
+		sdk.NewAttribute(CancelLimitOrderEventTokenOut, takerDenom),
+		sdk.NewAttribute(CancelLimitOrderEventTokenInAmountOut, makerAmountOut.String()),
+		sdk.NewAttribute(CancelLimitOrderEventTokenOutAmountOut, takerAmountOut.String()),
 		sdk.NewAttribute(CancelLimitOrderEventTrancheKey, trancheKey),
 	}
 

x/dex/types/keys.go

@@ -265,14 +265,15 @@ const (
 
 // Cancel LimitOrder Event Attributes
 const (
-	CancelLimitOrderEventKey        = "CancelLimitOrder"
-	CancelLimitOrderEventCreator    = "Creator"
-	CancelLimitOrderEventToken0     = "TokenZero"
-	CancelLimitOrderEventToken1     = "TokenOne"
-	CancelLimitOrderEventTokenIn    = "TokenIn"
-	CancelLimitOrderEventTokenOut   = "TokenOut"
-	CancelLimitOrderEventTrancheKey = "TrancheKey"
-	CancelLimitOrderEventAmountOut  = "AmountOut"
+	CancelLimitOrderEventKey               = "CancelLimitOrder"
+	CancelLimitOrderEventCreator           = "Creator"
+	CancelLimitOrderEventToken0            = "TokenZero"
+	CancelLimitOrderEventToken1            = "TokenOne"
+	CancelLimitOrderEventTokenIn           = "TokenIn"
+	CancelLimitOrderEventTokenOut          = "TokenOut"
+	CancelLimitOrderEventTrancheKey        = "TrancheKey"
+	CancelLimitOrderEventTokenInAmountOut  = "TokenInAmountOut"
+	CancelLimitOrderEventTokenOutAmountOut = "TokenOutAmountOut"
 )
 
 // Tick Update Event Attributes