fix: reviews for x/amm, x/exchange, x/liquidamm

x/amm/keeper/grpc_query.go

@@ -179,6 +179,7 @@ func (k Querier) PositionAssets(c context.Context, req *types.QueryPositionAsset
 	return &types.QueryPositionAssetsResponse{Coin0: coin0, Coin1: coin1}, nil
 }
 
+// The structure of simulation-related functions as queries is strange.
 func (k Querier) AddLiquiditySimulation(c context.Context, req *types.QueryAddLiquiditySimulationRequest) (*types.QueryAddLiquiditySimulationResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")
@@ -222,6 +223,7 @@ func (k Querier) AddLiquiditySimulation(c context.Context, req *types.QueryAddLi
 	}, nil
 }
 
+// The structure of simulation-related functions as queries is strange.
 func (k Querier) RemoveLiquiditySimulation(c context.Context, req *types.QueryRemoveLiquiditySimulationRequest) (*types.QueryRemoveLiquiditySimulationResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")

x/amm/keeper/invariants.go

@@ -131,6 +131,8 @@ func CanCollectInvariant(k Keeper) sdk.Invariant {
 	}
 }
 
+// A nested iterator was used. Since there is no part of it that changes values, I don't think it will cause any problems internally.
+// However, an implementation that avoids nested iterators, like PoolCurrentLiquidityInvariant, would be better.
 func PoolCurrentLiquidityInvariant(k Keeper) sdk.Invariant {
 	return func(ctx sdk.Context) (string, bool) {
 		msg := ""

x/amm/keeper/position.go

@@ -156,7 +156,7 @@ func (k Keeper) Collect(
 
 	if position.Liquidity.IsPositive() {
 		var err error
-		position, _, err = k.RemoveLiquidity(ctx, ownerAddr, toAddr, positionId, utils.ZeroInt)
+		position, _, err = k.RemoveLiquidity(ctx, ownerAddr, toAddr, positionId, utils.ZeroInt) // It would be un-natural to call a function named RemovePosition for a state check. It also doesn't use a CacheContext, so it raises unnecessary events.
 		if err != nil {
 			return err
 		}
@@ -203,8 +203,8 @@ func (k Keeper) PositionAssets(ctx sdk.Context, positionId uint64) (coin0, coin1
 		coin1 = sdk.NewInt64Coin(pool.Denom1, 0)
 		return
 	}
-	ctx, _ = ctx.CacheContext()
-	_, amt0, amt1 := k.modifyPosition(
+	ctx, _ = ctx.CacheContext()        // It takes a CacheContext and doesn't call writeCache, so nothing is written to the context.
+	_, amt0, amt1 := k.modifyPosition( // However, it is un-natural to call a function named modifyPosition in a function that is meant to get information.
 		ctx, pool, position.MustGetOwnerAddress(), position.LowerTick, position.UpperTick, position.Liquidity.Neg())
 	amt0, amt1 = amt0.Neg(), amt1.Neg()
 	coin0 = sdk.NewCoin(pool.Denom0, amt0)
@@ -217,10 +217,10 @@ func (k Keeper) CollectibleCoins(ctx sdk.Context, positionId uint64) (fee, farmi
 	if !found {
 		return nil, nil, sdkerrors.Wrap(sdkerrors.ErrNotFound, "position not found")
 	}
-	ctx, _ = ctx.CacheContext()
+	ctx, _ = ctx.CacheContext() // It takes a CacheContext and doesn't call writeCache, so nothing is written to the context or an event is fired.
 	ownerAddr := position.MustGetOwnerAddress()
 	if position.Liquidity.IsPositive() {
-		position, _, err = k.RemoveLiquidity(ctx, ownerAddr, ownerAddr, positionId, utils.ZeroInt)
+		position, _, err = k.RemoveLiquidity(ctx, ownerAddr, ownerAddr, positionId, utils.ZeroInt) // However, it is un-natural to call a function named RemoveLiquidity in a function that is meant to get information.
 		if err != nil {
 			return nil, nil, err
 		}

x/amm/keeper/store.go

@@ -235,7 +235,7 @@ func (k Keeper) IterateAllPositions(ctx sdk.Context, cb func(position types.Posi
 	iter := sdk.KVStorePrefixIterator(store, types.PositionKeyPrefix)
 	defer iter.Close()
 	for ; iter.Valid(); iter.Next() {
-		var pool types.Position
+		var pool types.Position // pool -> position(mis-spell)
 		k.cdc.MustUnmarshal(iter.Value(), &pool)
 		if cb(pool) {
 			break

x/exchange/keeper/batch.go

@@ -6,6 +6,10 @@ import (
 	"github.com/crescent-network/crescent/v5/x/exchange/types"
 )
 
+// Inside the getBestPrice function, it is calling the ConstructMemOrderBookSide function,
+// which means that the ConstructMemOrderBookSide function is called 4 times in this RunBatchMatching logic.
+// Since this logic is called for every market in the midblocker of every block,
+// it can act as a significant overhead. How about improving the logic?
 func (k Keeper) RunBatchMatching(ctx sdk.Context, market types.Market) (err error) {
 	// Find the best buy(bid) and sell(ask) prices to limit the price to load
 	// on the other side.

x/exchange/keeper/grpc_query.go

@@ -244,7 +244,7 @@ func (k Querier) MakeOrderBooks(ctx sdk.Context, market types.Market, lastPrice
 	smallestPriceInterval := types.PriceIntervalAtTick(types.TickAtPrice(highestPrice))
 
 	var orderBooks []types.OrderBook
-	for _, p := range []int{1, 10, 100} {
+	for _, p := range []int{1, 10, 100} { // If create and return 1, 10, and 100 all at once, the query may not perform well. It could also be used in a DOS attack to intentionally degrade the performance of a node. Wouldn't it be better to create only 1 and throw it to FE to process and use?
 		priceInterval := smallestPriceInterval.MulInt64(int64(p))
 		ob := types.OrderBook{
 			PriceInterval: priceInterval,

x/exchange/keeper/matching.go

@@ -38,6 +38,7 @@ func (k Keeper) ConstructMemOrderBookSide(
 			panic(err)
 		}
 	}
+	// You can optimize performance by only accepting up to opts.MaxNumPriceLevels for each user order and order source order, and then applying a limit.
 	if opts.MaxNumPriceLevels > 0 {
 		// TODO: can refund?
 		obs.Limit(opts.MaxNumPriceLevels)
@@ -116,7 +117,7 @@ func (k Keeper) finalizeMatching(ctx sdk.Context, market types.Market, orders []
 				}
 				// Update user orders
 				executableQty := order.ExecutableQuantity()
-				if executableQty.TruncateDec().IsZero() ||
+				if executableQty.TruncateDec().IsZero() || // How about adding order.IsBuy? And it works fine in the intended order, but adding parentheses seems like a good idea.
 					!order.IsBuy && executableQty.MulTruncate(order.Price).TruncateDec().IsZero() {
 					if err := k.cancelOrder(ctx, market, order); err != nil {
 						return err

x/exchange/keeper/order.go

@@ -331,6 +331,8 @@ func (k Keeper) cancelOrder(ctx sdk.Context, market types.Market, order types.Or
 	return nil
 }
 
+// A nested iterator has been used. Since it is changing the kv store inside the cancelOrder function,
+// there is a possibility of unintended behavior.
 func (k Keeper) CancelExpiredOrders(ctx sdk.Context) (err error) {
 	blockTime := ctx.BlockTime()
 	k.IterateAllMarkets(ctx, func(market types.Market) (stop bool) {

x/liquidamm/keeper/auction.go

@@ -101,6 +101,7 @@ func (k Keeper) GetLastRewardsAuction(ctx sdk.Context, publicPositionId uint64)
 	return k.GetRewardsAuction(ctx, publicPosition.Id, publicPosition.LastRewardsAuctionId)
 }
 
+// I think we can combine it with GetLastRewardsAuction to create a generalized function.
 func (k Keeper) GetPreviousRewardsAuction(ctx sdk.Context, publicPosition types.PublicPosition) (auction types.RewardsAuction, found bool) {
 	if publicPosition.LastRewardsAuctionId <= 1 {
 		return
@@ -134,7 +135,7 @@ func (k Keeper) AdvanceRewardsAuctions(ctx sdk.Context, nextEndTime time.Time) (
 			if auction.Id+uint64(maxNumRecentAuctions) >= lastAuctionId {
 				return true
 			}
-			k.DeleteRewardsAuction(ctx, auction)
+			k.DeleteRewardsAuction(ctx, auction) // Modifying kv store inside an iterator is undesirable. And the key prefix used by the iterator and DeleteRewardsAuction is the same.
 			return false
 		})
 		return false

x/liquidamm/keeper/grpc_query.go

@@ -203,6 +203,8 @@ func (k Querier) RewardsAuction(c context.Context, req *types.QueryRewardsAuctio
 }
 
 // Bids queries all Bid objects.
+// In the kv store, there are only bids for auctions that are currently in progress.
+// If req.AuctionId is not PublicPosition.LastRewardsAuctionId, the query is meaningless.
 func (k Querier) Bids(c context.Context, req *types.QueryBidsRequest) (*types.QueryBidsResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")

x/liquidamm/keeper/public_position.go

@@ -96,6 +96,8 @@ func (k Keeper) BurnShare(
 		return
 	}
 
+	// Requires a denom check for the share.
+
 	if err = k.bankKeeper.SendCoinsFromAccountToModule(ctx, senderAddr, types.ModuleName, sdk.NewCoins(share)); err != nil {
 		return
 	}

x/liquidamm/keeper/store.go

@@ -248,6 +248,8 @@ func (k Keeper) IterateAllBids(ctx sdk.Context, cb func(bid types.Bid) (stop boo
 	}
 }
 
+// In the kv store, there are only bids for auctions that are currently in progress.
+// If req.AuctionId is not PublicPosition.LastRewardsAuctionId, the iterate is meaningless.
 func (k Keeper) IterateBidsByRewardsAuction(ctx sdk.Context, publicPositionId, auctionId uint64, cb func(bid types.Bid) (stop bool)) {
 	store := ctx.KVStore(k.storeKey)
 	iterator := sdk.KVStorePrefixIterator(store, types.GetBidsByRewardsAuctionIteratorPrefix(publicPositionId, auctionId))