Lots of comments and better update pool/tranche/trancheUser fn naming

x/dex/keeper/cancel_limit_order.go

@@ -88,8 +88,11 @@ func (k Keeper) ExecuteCancelLimitOrder(
 		return sdk.Coin{}, sdk.Coin{}, nil, sdkerrors.Wrapf(types.ErrCancelEmptyLimitOrder, "%s", tranche.Key.TrancheKey)
 	}
 
-	k.SaveOrRemoveTrancheUser(ctx, trancheUser)
-	k.SaveOrMakeInactiveTranche(ctx, tranche)
+	// This will ALWAYS result in a deletion of the TrancheUser, but we still use UpdateTranche user so that the relevant events will be emitted
+	k.UpdateTrancheUser(ctx, trancheUser)
+
+	// If there is still liquidity from other shareholders we will either save the tranche as active or delete it entirely
+	k.UpdateTranche(ctx, tranche)
 
 	if trancheUser.OrderType.HasExpiration() {
 		k.RemoveLimitOrderExpiration(ctx, *tranche.ExpirationTime, tranche.Key.KeyMarshal())

x/dex/keeper/core_helper_test.go

@@ -72,7 +72,7 @@ func (s *CoreHelpersTestSuite) setLPAtFee1Pool(tickIndex int64, amountA, amountB
 
 	lowerTick.ReservesMakerDenom = amountAInt
 	upperTick.ReservesMakerDenom = amountBInt
-	s.app.DexKeeper.SaveOrRemovePool(s.ctx, pool)
+	s.app.DexKeeper.UpdatePool(s.ctx, pool)
 }
 
 // FindNextTick ////////////////////////////////////////////////////

x/dex/keeper/deposit.go

@@ -128,7 +128,8 @@ func (k Keeper) ExecuteDeposit(
 
 		inAmount0, inAmount1, outShares := pool.Deposit(amount0, amount1, existingShares, autoswap)
 
-		k.SaveOrRemovePool(ctx, pool)
+		// Save updates to both sides of the pool
+		k.UpdatePool(ctx, pool)
 
 		if inAmount0.IsZero() && inAmount1.IsZero() {
 			return nil, nil, math.ZeroInt(), math.ZeroInt(), nil, nil, nil, types.ErrZeroTrueDeposit

x/dex/keeper/inactive_limit_order_tranche.go

@@ -58,13 +58,14 @@ func (k Keeper) GetAllInactiveLimitOrderTranche(ctx sdk.Context) (list []*types.
 	return
 }
 
-func (k Keeper) SaveOrRemoveInactiveTranche(sdkCtx sdk.Context, tranche *types.LimitOrderTranche) {
+// UpdateInactiveTranche handles the logic for all updates to InactiveLimitOrderTranches
+// It will delete an InactiveTranche if there is no remaining MakerReserves or TakerReserves
+func (k Keeper) UpdateInactiveTranche(sdkCtx sdk.Context, tranche *types.LimitOrderTranche) {
 	if tranche.HasTokenIn() || tranche.HasTokenOut() {
+		// There are still reserves to be removed. Save the tranche as is.
 		k.SetInactiveLimitOrderTranche(sdkCtx, tranche)
 	} else {
-		k.RemoveInactiveLimitOrderTranche(
-			sdkCtx,
-			tranche.Key,
-		)
+		// There is nothing left to remove, we can safely remove the tranche entirely.
+		k.RemoveInactiveLimitOrderTranche(sdkCtx, tranche.Key)
 	}
 }

x/dex/keeper/limit_order_tranche.go

@@ -51,12 +51,27 @@ func (k Keeper) FindLimitOrderTranche(
 	return nil, false, false
 }
 
-func (k Keeper) SaveOrMakeInactiveTranche(ctx sdk.Context, tranche *types.LimitOrderTranche) {
-	if tranche.HasTokenIn() {
+// UpdateTranche handles the logic for all updates to active LimitOrderTranches in the KV Store.
+// NOTE: This method should always be called even if not all logic branches are applicable.
+// It avoids unnecessary repetition of logic and provides a single place to attach update event handlers.
+func (k Keeper) UpdateTranche(ctx sdk.Context, tranche *types.LimitOrderTranche) {
+	switch {
+
+	// Tranche still has TokenIn (ReservesMakerDenom) ==> Just save the update
+	case tranche.HasTokenIn():
 		k.SetLimitOrderTranche(ctx, tranche)
-	} else {
-		k.SaveOrRemoveInactiveTranche(ctx, tranche)
+
+	// There is no TokenIn but there is still withdrawable TokenOut (ReservesTakerDenom) ==> Remove the active tranche and create a new inactive tranche
+	case tranche.HasTokenOut():
+		k.SetInactiveLimitOrderTranche(ctx, tranche)
+		k.RemoveLimitOrderTranche(ctx, tranche.Key)
+		// We are removing liquidity from the orderbook so we emit an event
+		ctx.EventManager().EmitEvents(types.GetEventsDecTotalOrders(tranche.Key.TradePairId))
+
+	// There is no TokenIn or Token Out ==> We can delete the tranche entirely
+	default:
 		k.RemoveLimitOrderTranche(ctx, tranche.Key)
+		// We are removing liquidity from the orderbook so we emit an event
 		ctx.EventManager().EmitEvents(types.GetEventsDecTotalOrders(tranche.Key.TradePairId))
 	}
 

x/dex/keeper/limit_order_tranche_user.go

@@ -87,10 +87,15 @@ func (k Keeper) RemoveLimitOrderTrancheUser(ctx sdk.Context, trancheUser *types.
 	)
 }
 
-func (k Keeper) SaveOrRemoveTrancheUser(ctx sdk.Context, trancheUser *types.LimitOrderTrancheUser) {
+// UpdateTrancheUser handles the logic for all updates to LimitOrderTrancheUsers in the KV Store.
+// NOTE: This method should always be called even if not all logic branches are applicable.
+// It avoids unnecessary repetition of logic and provides a single place to attach update event handlers.
+func (k Keeper) UpdateTrancheUser(ctx sdk.Context, trancheUser *types.LimitOrderTrancheUser) {
 	if trancheUser.IsEmpty() {
+		// The trancheUser has no remaining withdrawable shares and can be deleted
 		k.RemoveLimitOrderTrancheUser(ctx, trancheUser)
 	} else {
+		// The trancheUser has withdrawable shares; it should be saved
 		k.SetLimitOrderTrancheUser(ctx, trancheUser)
 	}
 }

x/dex/keeper/liquidity.go

@@ -110,9 +110,11 @@ func (k Keeper) SwapWithCache(
 func (k Keeper) SaveLiquidity(sdkCtx sdk.Context, liquidityI types.Liquidity) {
 	switch liquidity := liquidityI.(type) {
 	case *types.LimitOrderTranche:
-		k.SaveOrMakeInactiveTranche(sdkCtx, liquidity)
+		// If there is still makerReserves we will save the tranche as active, if not, we will move it to inactive
+		k.UpdateTranche(sdkCtx, liquidity)
 	case *types.PoolLiquidity:
-		k.SaveOrRemovePool(sdkCtx, liquidity.Pool)
+		// Save updated to both sides of the pool. If one of the sides is empty it will be deleted
+		k.UpdatePool(sdkCtx, liquidity.Pool)
 	default:
 		panic("Invalid liquidity type")
 	}

x/dex/keeper/liquidity_test.go

@@ -556,7 +556,7 @@ func (s *DexTestSuite) addDeposit(deposit *Deposit) {
 	s.Assert().NoError(err)
 	pool.LowerTick0.ReservesMakerDenom = pool.LowerTick0.ReservesMakerDenom.Add(deposit.AmountA)
 	pool.UpperTick1.ReservesMakerDenom = pool.UpperTick1.ReservesMakerDenom.Add(deposit.AmountB)
-	s.App.DexKeeper.SaveOrRemovePool(s.Ctx, pool)
+	s.App.DexKeeper.UpdatePool(s.Ctx, pool)
 }
 
 func (s *DexTestSuite) addDeposits(deposits ...*Deposit) {
@@ -581,7 +581,7 @@ func (s *DexTestSuite) placeGTCLimitOrder(
 	)
 	s.Assert().NoError(err)
 	tranche.PlaceMakerLimitOrder(sdkmath.NewInt(amountIn).Mul(denomMultiple))
-	s.App.DexKeeper.SaveOrMakeInactiveTranche(s.Ctx, tranche)
+	s.App.DexKeeper.UpdateTranche(s.Ctx, tranche)
 }
 
 func (s *DexTestSuite) swapInt(

x/dex/keeper/place_limit_order.go

@@ -174,13 +174,17 @@ func (k Keeper) ExecutePlaceLimitOrder(
 			ctx.GasMeter().ConsumeGas(types.ExpiringLimitOrderGas, "Expiring LimitOrder Fee")
 		}
 
-		k.SaveOrMakeInactiveTranche(ctx, placeTranche)
+		// This update will ALWAYS save the tranche as active.
+		// But we use the general updateTranche function so the correct events are emitted
+		k.UpdateTranche(ctx, placeTranche)
 
 		totalIn = totalIn.Add(amountLeft)
 		sharesIssued = amountLeft
 	}
 
-	k.SaveOrRemoveTrancheUser(ctx, trancheUser)
+	// This update will ALWAYS save the trancheUser as active.
+	// But we use the general updateTranche function so the correct events are emitted
+	k.UpdateTrancheUser(ctx, trancheUser)
 
 	if orderType.IsJIT() {
 		err = k.AssertCanPlaceJIT(ctx)

x/dex/keeper/pool.go

@@ -135,25 +135,15 @@ func (k Keeper) GetPoolIDByParams(
 	return poolID, true
 }
 
-func (k Keeper) SaveOrRemovePool(ctx sdk.Context, pool *types.Pool) {
-	k.saveOrRemovePoolReserves(ctx, pool.LowerTick0)
-	k.saveOrRemovePoolReserves(ctx, pool.UpperTick1)
+// UpdatePool handles the logic for all updates to Pools in the KV Store.
+// It provides a convenient way to save both sides of the pool reserves.
+func (k Keeper) UpdatePool(ctx sdk.Context, pool *types.Pool) {
+	k.UpdatePoolReserves(ctx, pool.LowerTick0)
+	k.UpdatePoolReserves(ctx, pool.UpperTick1)
 
-	// TODO: this will create a bit of extra noise since not every Save is updating both ticks
-	// This should be solved upstream by better tracking of dirty ticks
-	ctx.EventManager().EmitEvent(types.CreateTickUpdatePoolReserves(*pool.LowerTick0))
 	ctx.EventManager().EmitEvent(types.CreateTickUpdatePoolReserves(*pool.UpperTick1))
 }
 
-func (k Keeper) saveOrRemovePoolReserves(ctx sdk.Context, reserves *types.PoolReserves) {
-	if reserves.HasToken() {
-		k.SetPoolReserves(ctx, reserves)
-	} else {
-		ctx.EventManager().EmitEvents(types.GetEventsDecTotalPoolReserves(*reserves.Key.TradePairId.MustPairID()))
-		k.RemovePoolReserves(ctx, reserves.Key)
-	}
-}
-
 // GetPoolCount get the total number of pools
 func (k Keeper) GetPoolCount(ctx sdk.Context) uint64 {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{})

x/dex/keeper/pool_reserves.go

@@ -40,3 +40,22 @@ func (k Keeper) RemovePoolReserves(ctx sdk.Context, poolReservesID *types.PoolRe
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefix(types.TickLiquidityKeyPrefix))
 	store.Delete(poolReservesID.KeyMarshal())
 }
+
+// UpdatePoolReserves handles the logic for all updates to PoolReserves in the KV Store.
+// NOTE: This method should always be called even if not all logic branches are applicable.
+// It avoids unnecessary repetition of logic and provides a single place to attach update event handlers.
+func (k Keeper) UpdatePoolReserves(ctx sdk.Context, reserves *types.PoolReserves) {
+	if reserves.HasToken() {
+		// The pool still has ReservesMakerDenom; save it as is
+		k.SetPoolReserves(ctx, reserves)
+	} else {
+		ctx.EventManager().EmitEvents(types.GetEventsDecTotalPoolReserves(*reserves.Key.TradePairId.MustPairID()))
+		// The pool is empty (ie. ReservesMakerDenom == 0); it can be safely deleted
+		k.RemovePoolReserves(ctx, reserves.Key)
+	}
+
+	// TODO: This will create a bit of extra noise since UpdatePoolReserves is called for both sides of the pool,
+	// but not in some cases only one side has been updated
+	// This should be solved upstream by better tracking of dirty ticks
+	ctx.EventManager().EmitEvent(types.CreateTickUpdatePoolReserves(*reserves))
+}

x/dex/keeper/pool_test.go

@@ -20,7 +20,7 @@ func createNPools(k *keeper.Keeper, ctx sdk.Context, n int) []*types.Pool {
 			panic("failed to create pool")
 		}
 		pool.Deposit(math.NewInt(10), math.NewInt(0), math.ZeroInt(), true)
-		k.SaveOrRemovePool(ctx, pool)
+		k.UpdatePool(ctx, pool)
 		items[i] = pool
 	}
 
@@ -33,7 +33,7 @@ func TestPoolInit(t *testing.T) {
 	pool, err := keeper.InitPool(ctx, defaultPairID, 0, 1)
 	require.NoError(t, err)
 	pool.Deposit(math.NewInt(1000), math.NewInt(1000), math.NewInt(0), true)
-	keeper.SaveOrRemovePool(ctx, pool)
+	keeper.UpdatePool(ctx, pool)
 
 	dbPool, found := keeper.GetPool(ctx, defaultPairID, 0, 1)
 

x/dex/keeper/withdraw.go

@@ -116,7 +116,9 @@ func (k Keeper) ExecuteWithdraw(
 		}
 
 		outAmount0, outAmount1 := pool.Withdraw(sharesToRemove, totalShares)
-		k.SaveOrRemovePool(ctx, pool)
+
+		// Save both sides of the pool. If one or both sides are empty they will be deleted.
+		k.UpdatePool(ctx, pool)
 
 		totalReserve0ToRemove = totalReserve0ToRemove.Add(outAmount0)
 		totalReserve1ToRemove = totalReserve1ToRemove.Add(outAmount1)

x/dex/keeper/withdraw_filled_limit_order.go

@@ -86,19 +86,20 @@ func (k Keeper) ExecuteWithdrawFilledLimitOrder(
 		if wasFilled {
 			// This is only relevant for inactive JIT and GoodTil limit orders
 			remainingTokenIn = tranche.RemoveTokenIn(trancheUser)
-			k.SaveOrRemoveInactiveTranche(ctx, tranche)
+			k.UpdateInactiveTranche(ctx, tranche)
 
 			// Since the order has already been filled we treat this as a complete withdrawal
 			trancheUser.SharesWithdrawn = trancheUser.SharesOwned
 
 		} else {
-			k.SetLimitOrderTranche(ctx, tranche)
+			// 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)
 		}
 
 	}
-
-	k.SaveOrRemoveTrancheUser(ctx, trancheUser)
+	// Save the tranche user
+	k.UpdateTrancheUser(ctx, trancheUser)
 
 	if !amountOutTokenOut.IsPositive() && !remainingTokenIn.IsPositive() {
 		return math.ZeroInt(), math.ZeroInt(), tradePairID, types.ErrWithdrawEmptyLimitOrder