Merge pull request #1048 from neutron-org/chore/better_tranche_share_calc

Add decSharesWithdrawn field for more accurate share calculation

Author: pr0n00gler

proto/neutron/dex/limit_order_tranche_user.proto

@@ -18,7 +18,10 @@ message LimitOrderTrancheUser {
     (gogoproto.nullable) = false,
     (gogoproto.jsontag) = "shares_owned"
   ];
+
+  // DEPRECATED: shares_withdrawn will be removed in a future release, `dec_shares_withdrawn` should be used
   string shares_withdrawn = 6 [
+    deprecated = true,
     (gogoproto.moretags) = "yaml:\"shares_withdrawn\"",
     (gogoproto.customtype) = "cosmossdk.io/math.Int",
     (gogoproto.nullable) = false,
@@ -32,4 +35,10 @@ message LimitOrderTrancheUser {
     (gogoproto.jsontag) = "shares_cancelled"
   ];
   LimitOrderType order_type = 8;
+  string dec_shares_withdrawn = 9 [
+    (gogoproto.moretags) = "yaml:\"dec_shares_withdrawn\"",
+    (gogoproto.customtype) = "github.com/neutron-org/neutron/v10/utils/math.PrecDec",
+    (gogoproto.nullable) = false,
+    (gogoproto.jsontag) = "dec_shares_withdrawn"
+  ];
 }

tests/dex/state_withdraw_limit_order_test.go

@@ -187,8 +187,10 @@ func (s *DexStateTestSuite) assertWithdrawFilledAmount(params withdrawLimitOrder
 			s.False(found)
 		} else {
 			s.True(found)
-			remainingShares := ut.SharesOwned.Sub(ut.SharesWithdrawn)
-			s.True(expectedBalanceA.Equal(remainingShares), "Expected Balance A %v != Actual %v", expectedBalanceA, remainingShares)
+			sharesOwnedDec := math_utils.NewPrecDecFromInt(ut.SharesOwned)
+			remainingShares := sharesOwnedDec.Sub(ut.DecSharesWithdrawn)
+			expectedBalanceADec := math_utils.NewPrecDecFromInt(expectedBalanceA)
+			s.True(expectedBalanceADec.Equal(remainingShares), "Expected Balance A %v != Actual %v", expectedBalanceA, remainingShares)
 		}
 	}
 }

x/dex/genesis_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/neutron-org/neutron/v10/testutil/common/nullify"
 	keepertest "github.com/neutron-org/neutron/v10/testutil/dex/keeper"
+	math_utils "github.com/neutron-org/neutron/v10/utils/math"
 	"github.com/neutron-org/neutron/v10/x/dex"
 	"github.com/neutron-org/neutron/v10/x/dex/types"
 )
@@ -26,7 +27,7 @@ func TestGenesis(t *testing.T) {
 				TrancheKey:            "0",
 				Address:               "fakeAddr",
 				SharesOwned:           math.NewInt(10),
-				SharesWithdrawn:       math.NewInt(0),
+				DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 			},
 			{
 				TradePairId: &types.TradePairID{
@@ -37,7 +38,7 @@ func TestGenesis(t *testing.T) {
 				TrancheKey:            "0",
 				Address:               "fakeAddr",
 				SharesOwned:           math.NewInt(10),
-				SharesWithdrawn:       math.NewInt(0),
+				DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 			},
 		},
 		TickLiquidityList: []*types.TickLiquidity{

x/dex/keeper/cancel_limit_order.go

@@ -91,8 +91,8 @@ func (k Keeper) ExecuteCancelLimitOrder(
 	tranche.TotalMakerDenom = tranche.TotalMakerDenom.Sub(trancheUser.SharesOwned)
 
 	// Calculate total number of shares removed previously withdrawn by the user (denominated in takerDenom)
-	sharesWithdrawnTakerDenom := math_utils.NewPrecDecFromInt(trancheUser.SharesWithdrawn).
-		Quo(tranche.PriceTakerToMaker)
+	sharesWithdrawnTakerDenom := trancheUser.DecSharesWithdrawn.
+		Mul(tranche.MakerPrice)
 
 	// Calculate the total amount removed including prior withdrawals (denominated in takerDenom)
 	totalAmountOutTakerDenom := sharesWithdrawnTakerDenom.Add(takerAmountOut)
@@ -101,7 +101,7 @@ func (k Keeper) ExecuteCancelLimitOrder(
 	tranche.SetTotalTakerDenom(tranche.DecTotalTakerDenom.Sub(totalAmountOutTakerDenom))
 
 	// Set TrancheUser to 100% shares withdrawn
-	trancheUser.SharesWithdrawn = trancheUser.SharesOwned
+	trancheUser.SetSharesWithdrawn(math_utils.NewPrecDecFromInt(trancheUser.SharesOwned))
 
 	if !makerAmountToReturn.IsPositive() && !takerAmountOut.IsPositive() {
 		return types.PrecDecCoin{}, types.PrecDecCoin{}, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey)

x/dex/keeper/grpc_query_limit_order_tranche_user.go

@@ -3,13 +3,13 @@ package keeper
 import (
 	"context"
 
-	"cosmossdk.io/math"
 	"cosmossdk.io/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/query"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
+	math_utils "github.com/neutron-org/neutron/v10/utils/math"
 	"github.com/neutron-org/neutron/v10/x/dex/types"
 )
 
@@ -47,7 +47,7 @@ func (k Keeper) LimitOrderTrancheUserAll(
 	}, nil
 }
 
-func (k Keeper) CalcWithdrawableShares(ctx sdk.Context, trancheUser types.LimitOrderTrancheUser) (amount math.Int, err error) {
+func (k Keeper) CalcWithdrawableShares(ctx sdk.Context, trancheUser types.LimitOrderTrancheUser) (amount math_utils.PrecDec, err error) {
 	tradePairID, tickIndex := trancheUser.TradePairId, trancheUser.TickIndexTakerToMaker
 
 	tranche, _, found := k.FindLimitOrderTranche(
@@ -60,7 +60,7 @@ func (k Keeper) CalcWithdrawableShares(ctx sdk.Context, trancheUser types.LimitO
 	)
 
 	if !found {
-		return math.ZeroInt(), status.Error(codes.NotFound, "Tranche not found")
+		return math_utils.ZeroPrecDec(), status.Error(codes.NotFound, "Tranche not found")
 	}
 	withdrawableShares, _ := tranche.CalcWithdrawAmount(&trancheUser)
 
@@ -89,7 +89,8 @@ func (k Keeper) LimitOrderTrancheUser(c context.Context,
 		if err != nil {
 			return nil, err
 		}
-		resp.WithdrawableShares = &withdrawAmt
+		withdrawAmtInt := withdrawAmt.TruncateInt()
+		resp.WithdrawableShares = &withdrawAmtInt
 	}
 
 	return resp, nil

x/dex/keeper/integration_cancellimitorder_test.go

@@ -532,9 +532,9 @@ func (s *DexTestSuite) TestWithdrawThenCancelLowTick() {
 	s.assertAliceBalancesInt(sdkmath.NewInt(13058413), sdkmath.NewInt(4999999))
 
 	s.bobWithdrawsLimitSell(trancheKey)
-	s.assertBobBalancesInt(sdkmath.ZeroInt(), sdkmath.NewInt(4999999))
+	s.assertBobBalances(0, 5)
 	s.bobCancelsLimitSell(trancheKey)
-	s.assertBobBalancesInt(sdkmath.NewInt(13058413), sdkmath.NewInt(4999999))
+	s.assertBobBalancesInt(sdkmath.NewInt(13058413), sdkmath.NewInt(5_000_000))
 }
 
 func (s *DexTestSuite) TestWrongSharesProtectionCancel() {
@@ -553,3 +553,51 @@ func (s *DexTestSuite) TestWrongSharesProtectionCancel() {
 
 	s.assertAliceBalances(1, 0)
 }
+
+func (s *DexTestSuite) TestCanceLimitOrderClearsPosition() {
+	s.fundAccountBalancesInt(s.alice, sdkmath.NewInt(10_000), sdkmath.ZeroInt())
+	s.fundAccountBalancesInt(s.bob, sdkmath.NewInt(500_000), sdkmath.ZeroInt())
+	s.fundAccountBalancesInt(s.carol, sdkmath.ZeroInt(), sdkmath.NewInt(22_015))
+
+	// GIVEN alice and bob place GTC limit sells at tick -100000
+	trancheKey := s.limitSellsIntSuccess(s.alice, "TokenA", -100000, sdkmath.NewInt(10_000))
+	s.limitSellsIntSuccess(s.bob, "TokenA", -100000, sdkmath.NewInt(500_000))
+
+	// AND carol swaps 22,015 TokenB for TokeA
+	s.limitSellsIntSuccess(s.carol, "TokenB", -100001, sdkmath.NewInt(22_015), types.LimitOrderType_FILL_OR_KILL)
+
+	// WHEN Alice withdraws then cancels
+	s.aliceWithdrawsLimitSell(trancheKey)
+	s.aliceCancelsLimitSell(trancheKey)
+
+	// THEN totalTakerDenom == ReservesTakerDenom
+	tranche, _, found := s.App.DexKeeper.FindLimitOrderTranche(
+		s.Ctx,
+		&types.LimitOrderTrancheKey{
+			TradePairId:           types.MustNewTradePairID("TokenB", "TokenA"),
+			TickIndexTakerToMaker: 100000,
+			TrancheKey:            trancheKey,
+		},
+	)
+	s.True(found)
+
+	s.Equal(tranche.DecTotalTakerDenom, tranche.DecReservesTakerDenom)
+
+	// WHEN bob cancels his limit order, the tranche only contains dust
+	s.bobCancelsLimitSell(trancheKey)
+
+	// Final tranche state
+	tranche, _, found = s.App.DexKeeper.FindLimitOrderTranche(
+		s.Ctx,
+		&types.LimitOrderTrancheKey{
+			TradePairId:           types.MustNewTradePairID("TokenB", "TokenA"),
+			TickIndexTakerToMaker: 100000,
+			TrancheKey:            trancheKey,
+		},
+	)
+	s.True(found)
+
+	// NOTE: we are using the Int fields just to ensure only dust is left
+	s.Assert().Equal(tranche.ReservesTakerDenom, sdkmath.ZeroInt())
+	s.Assert().Equal(tranche.TotalTakerDenom, sdkmath.ZeroInt())
+}

x/dex/keeper/limit_order_tranche_user.go

@@ -7,6 +7,7 @@ import (
 	storetypes "cosmossdk.io/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	math_utils "github.com/neutron-org/neutron/v10/utils/math"
 	"github.com/neutron-org/neutron/v10/x/dex/types"
 )
 
@@ -33,6 +34,7 @@ func (k Keeper) GetOrInitLimitOrderTrancheUser(
 		Address:               receiver,
 		SharesOwned:           math.ZeroInt(),
 		SharesWithdrawn:       math.ZeroInt(),
+		DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 		TickIndexTakerToMaker: tickIndex,
 		TradePairId:           tradePairID,
 		OrderType:             orderType,

x/dex/keeper/limit_order_tranche_user_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/neutron-org/neutron/v10/testutil/common/nullify"
 	keepertest "github.com/neutron-org/neutron/v10/testutil/dex/keeper"
+	math_utils "github.com/neutron-org/neutron/v10/utils/math"
 	"github.com/neutron-org/neutron/v10/x/dex/keeper"
 	"github.com/neutron-org/neutron/v10/x/dex/types"
 )
@@ -24,7 +25,7 @@ func createNLimitOrderTrancheUser(keeper *keeper.Keeper, ctx sdk.Context, n int)
 			TradePairId:           &types.TradePairID{MakerDenom: "TokenA", TakerDenom: "TokenB"},
 			TickIndexTakerToMaker: int64(i),
 			SharesOwned:           math.NewInt(100),
-			SharesWithdrawn:       math.ZeroInt(),
+			DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 		}
 		items[i] = val
 		keeper.SetLimitOrderTrancheUser(ctx, items[i])
@@ -42,7 +43,7 @@ func createNLimitOrderTrancheUserWithAddress(keeper *keeper.Keeper, ctx sdk.Cont
 			TradePairId:           &types.TradePairID{MakerDenom: "TokenA", TakerDenom: "TokenB"},
 			TickIndexTakerToMaker: 0,
 			SharesOwned:           math.ZeroInt(),
-			SharesWithdrawn:       math.ZeroInt(),
+			DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 		}
 		items[i] = val
 		keeper.SetLimitOrderTrancheUser(ctx, items[i])
@@ -91,7 +92,8 @@ func (s *DexTestSuite) TestGetAllLimitOrders() {
 		TrancheKey:            trancheKeyA,
 		Address:               s.alice.String(),
 		SharesOwned:           math.NewInt(10_000_000),
-		SharesWithdrawn:       math.NewInt(0),
+		SharesWithdrawn:       math.ZeroInt(),
+		DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 		SharesCancelled:       math.ZeroInt(),
 	},
 		LOList[0],
@@ -102,7 +104,8 @@ func (s *DexTestSuite) TestGetAllLimitOrders() {
 		TrancheKey:            trancheKeyB,
 		Address:               s.alice.String(),
 		SharesOwned:           math.NewInt(10_000_000),
-		SharesWithdrawn:       math.NewInt(0),
+		SharesWithdrawn:       math.ZeroInt(),
+		DecSharesWithdrawn:    math_utils.ZeroPrecDec(),
 		SharesCancelled:       math.ZeroInt(),
 	},
 		LOList[1],

x/dex/keeper/migrations.go

@@ -7,6 +7,7 @@ import (
 	v4 "github.com/neutron-org/neutron/v10/x/dex/migrations/v4"
 	v5 "github.com/neutron-org/neutron/v10/x/dex/migrations/v5"
 	v6 "github.com/neutron-org/neutron/v10/x/dex/migrations/v6"
+	v7 "github.com/neutron-org/neutron/v10/x/dex/migrations/v7"
 )
 
 // Migrator is a struct for handling in-place store migrations.
@@ -38,3 +39,8 @@ func (m Migrator) Migrate4to5(ctx sdk.Context) error {
 func (m Migrator) Migrate5to6(ctx sdk.Context) error {
 	return v6.MigrateStore(ctx, m.keeper.cdc, m.keeper.storeKey)
 }
+
+// Migrate6to7 migrates from version 6 to 7.
+func (m Migrator) Migrate6to7(ctx sdk.Context) error {
+	return v7.MigrateStore(ctx, m.keeper.cdc, m.keeper.storeKey)
+}

x/dex/keeper/withdraw_filled_limit_order.go

@@ -4,7 +4,6 @@ import (
 	"context"
 
 	sdkerrors "cosmossdk.io/errors"
-	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	math_utils "github.com/neutron-org/neutron/v10/utils/math"
@@ -82,7 +81,7 @@ func (k Keeper) ExecuteWithdrawFilledLimitOrder(
 	remainingTokenIn := math_utils.ZeroPrecDec()
 	// It's possible that a TrancheUser exists but tranche does not if LO was filled entirely through a swap
 	if found {
-		var amountOutTokenIn math.Int
+		var amountOutTokenIn math_utils.PrecDec
 		amountOutTokenIn, amountOutTokenOut, err = tranche.Withdraw(trancheUser)
 		if err != nil {
 			return types.PrecDecCoin{}, types.PrecDecCoin{}, err
@@ -97,12 +96,13 @@ func (k Keeper) ExecuteWithdrawFilledLimitOrder(
 			k.UpdateInactiveTranche(ctx, tranche)
 
 			// Since the order has already been filled we treat this as a complete withdrawal
-			trancheUser.SharesWithdrawn = trancheUser.SharesOwned
+			trancheUser.SetSharesWithdrawn(math_utils.NewPrecDecFromInt(trancheUser.SharesOwned))
 
 		} else {
 			// This was an active tranche (still has MakerReserves) and we have only removed TakerReserves; we will save it as an active tranche
 			k.UpdateTranche(ctx, tranche)
-			trancheUser.SharesWithdrawn = trancheUser.SharesWithdrawn.Add(amountOutTokenIn)
+			totalSharesWithdrawn := trancheUser.DecSharesWithdrawn.Add(amountOutTokenIn)
+			trancheUser.SetSharesWithdrawn(totalSharesWithdrawn)
 		}
 
 	}