diff --git a/x/amm/keeper/grpc_query.go b/x/amm/keeper/grpc_query.go index 5805efda..f571d7b3 100644 --- a/x/amm/keeper/grpc_query.go +++ b/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") diff --git a/x/amm/keeper/invariants.go b/x/amm/keeper/invariants.go index eb61b26b..47bdd50e 100644 --- a/x/amm/keeper/invariants.go +++ b/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 := "" diff --git a/x/amm/keeper/position.go b/x/amm/keeper/position.go index 47429816..2da28007 100644 --- a/x/amm/keeper/position.go +++ b/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 } diff --git a/x/amm/keeper/store.go b/x/amm/keeper/store.go index 8572373d..11d2f5c2 100644 --- a/x/amm/keeper/store.go +++ b/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 diff --git a/x/exchange/keeper/batch.go b/x/exchange/keeper/batch.go index ab22811f..66bf4b43 100644 --- a/x/exchange/keeper/batch.go +++ b/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. diff --git a/x/exchange/keeper/grpc_query.go b/x/exchange/keeper/grpc_query.go index 42236342..f825fe69 100644 --- a/x/exchange/keeper/grpc_query.go +++ b/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, diff --git a/x/exchange/keeper/matching.go b/x/exchange/keeper/matching.go index f93875d8..7e78d63e 100644 --- a/x/exchange/keeper/matching.go +++ b/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 diff --git a/x/exchange/keeper/order.go b/x/exchange/keeper/order.go index d3f422fb..c157bdd1 100644 --- a/x/exchange/keeper/order.go +++ b/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) { diff --git a/x/liquidamm/keeper/auction.go b/x/liquidamm/keeper/auction.go index 3bf90129..ef8253b2 100644 --- a/x/liquidamm/keeper/auction.go +++ b/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 diff --git a/x/liquidamm/keeper/grpc_query.go b/x/liquidamm/keeper/grpc_query.go index 4766d450..06f854f8 100644 --- a/x/liquidamm/keeper/grpc_query.go +++ b/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") diff --git a/x/liquidamm/keeper/public_position.go b/x/liquidamm/keeper/public_position.go index 0b53db99..bf7ffb1a 100644 --- a/x/liquidamm/keeper/public_position.go +++ b/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 } diff --git a/x/liquidamm/keeper/store.go b/x/liquidamm/keeper/store.go index aac2819e..910443a9 100644 --- a/x/liquidamm/keeper/store.go +++ b/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))