Merge pull request #690 from neutron-org/fix/lo_cancel_accounting

Fix: Limit order cancel accounting

x/dex/keeper/cancel_limit_order.go

@@ -6,6 +6,7 @@ import (
 	sdkerrors "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	math_utils "github.com/neutron-org/neutron/v4/utils/math"
 	"github.com/neutron-org/neutron/v4/x/dex/types"
 )
 
@@ -78,11 +79,22 @@ func (k Keeper) ExecuteCancelLimitOrder(
 	makerAmountToReturn := tranche.RemoveTokenIn(trancheUser)
 	_, takerAmountOut := tranche.Withdraw(trancheUser)
 
-	trancheUser.SharesWithdrawn = trancheUser.SharesOwned
-
-	// Remove the canceled shares from the limitOrder
+	// Remove the canceled shares from the maker side of the limitOrder
 	tranche.TotalMakerDenom = tranche.TotalMakerDenom.Sub(trancheUser.SharesOwned)
-	tranche.TotalTakerDenom = tranche.TotalTakerDenom.Sub(takerAmountOut)
+
+	// Calculate total number of shares removed previously withdrawn by the user (denominated in takerDenom)
+	sharesWithdrawnTakerDenom := math_utils.NewPrecDecFromInt(trancheUser.SharesWithdrawn).
+		Quo(tranche.PriceTakerToMaker).
+		TruncateInt()
+
+	// Calculate the total amount removed including prior withdrawals (denominated in takerDenom)
+	totalAmountOutTakerDenom := sharesWithdrawnTakerDenom.Add(takerAmountOut)
+
+	// Decrease the tranche TotalTakerDenom by the amount being removed
+	tranche.TotalTakerDenom = tranche.TotalTakerDenom.Sub(totalAmountOutTakerDenom)
+
+	// Set TrancheUser to 100% shares withdrawn
+	trancheUser.SharesWithdrawn = trancheUser.SharesOwned
 
 	if !makerAmountToReturn.IsPositive() && !takerAmountOut.IsPositive() {
 		return sdk.Coin{}, sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey)

x/dex/keeper/integration_cancellimitorder_test.go

@@ -384,3 +384,67 @@ func (s *DexTestSuite) TestCancelJITNextBlock() {
 	s.aliceCancelsLimitSellFails(trancheKey, types.ErrActiveLimitOrderNotFound)
 	s.assertAliceBalances(0, 0)
 }
+
+func (s *DexTestSuite) TestWithdrawThenCancel() {
+	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.carolLimitSells("TokenB", -1, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice withdraws and  cancels her limit order
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.aliceCancelsLimitSell(trancheKey)
+	s.assertAliceBalances(45, 5)
+
+	s.bobWithdrawsLimitSell(trancheKey)
+	s.assertBobBalances(0, 5)
+	s.bobCancelsLimitSell(trancheKey)
+	s.assertBobBalances(45, 5)
+}
+
+func (s *DexTestSuite) TestWithdrawThenCancel2() {
+	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.carolLimitSells("TokenB", -1, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice withdraws and  cancels her limit order
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.aliceCancelsLimitSell(trancheKey)
+	s.assertAliceBalances(45, 5)
+
+	s.bobCancelsLimitSell(trancheKey)
+	s.assertBobBalances(45, 5)
+}
+
+func (s *DexTestSuite) TestWithdrawThenCancelLowTick() {
+	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", 20000, 50)
+	s.bobLimitSells("TokenA", 20000, 50)
+
+	s.carolLimitSells("TokenB", -20001, 10, types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN alice withdraws and  cancels her limit order
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.aliceCancelsLimitSell(trancheKey)
+	s.assertAliceBalancesInt(sdkmath.NewInt(13058413), sdkmath.NewInt(4999999))
+
+	s.bobWithdrawsLimitSell(trancheKey)
+	s.assertBobBalancesInt(sdkmath.ZeroInt(), sdkmath.NewInt(4999999))
+	s.bobCancelsLimitSell(trancheKey)
+	s.assertBobBalancesInt(sdkmath.NewInt(13058413), sdkmath.NewInt(4999999))
+}

x/dex/keeper/integration_withdrawfilled_test.go

@@ -386,8 +386,27 @@ func (s *DexTestSuite) TestWithdrawPartiallyGTTFilledCancelled() {
 	s.False(found, "Alice's LimitOrderTrancheUser not removed")
 }
 
-// testcancel unfilled
+func (s *DexTestSuite) TestWithdrawInactive() {
+	s.fundAliceBalances(10, 0)
+	s.fundBobBalances(0, 20)
 
-// test withdraw expired
+	// GIVEN Alice places an expiring limit order of A
+	trancheKey := s.aliceLimitSellsGoodTil("TokenA", 0, 10, time.Now())
+
+	// Bob trades through half of it
+	s.bobLimitSells("TokenB", -1, 5)
+
+	// Alice withdraws the profits
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.assertAliceBalances(0, 5)
+
+	// bob swap through more
+	s.bobLimitSells("TokenB", -1, 4)
 
-// how does cancel withdraw work does it call into was filled
+	// WHEN it is purged
+	s.App.DexKeeper.PurgeExpiredLimitOrders(s.Ctx, time.Now())
+
+	// THEN alice can withdraw the expected amount
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.assertAliceBalances(1, 9)
+}