Mint shares at the end depositCore; not inside the loop

Much better gas efficiency since we don't need to perform duplicate lookups for each share issuance Also add additional check for duplicate deposits. This should exist regardless but also required for share math to work if we aren't issuing shares within the deposit loop
neutron-org · Nov 1, 2023 · ffc8bd2 · ffc8bd2
1 parent ee0ef23
commit ffc8bd2
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 8 deletions.
diff --git a/x/dex/keeper/core.go b/x/dex/keeper/core.go
@@ -77,9 +77,6 @@ func (k Keeper) DepositCore(
 			return nil, nil, nil, types.ErrDepositShareUnderflow
 		}
 
-		if err := k.MintShares(ctx, receiverAddr, outShares); err != nil {
-			return nil, nil, nil, err
-		}
 		sharesIssued = append(sharesIssued, outShares)
 
 		amounts0Deposited[i] = inAmount0
@@ -103,6 +100,7 @@ func (k Keeper) DepositCore(
 	// At this point shares issued is not sorted and may have duplicates
 	// we must sanitize to convert it to a valid set of coins
 	sharesIssued = utils.SanitizeCoins(sharesIssued)
+
 	if totalAmountReserve0.IsPositive() {
 		coin0 := sdk.NewCoin(pairID.Token0, totalAmountReserve0)
 		if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, callerAddr, types.ModuleName, sdk.Coins{coin0}); err != nil {
@@ -117,6 +115,10 @@ func (k Keeper) DepositCore(
 		}
 	}
 
+	if err := k.MintShares(ctx, receiverAddr, sharesIssued); err != nil {
+		return nil, nil, nil, err
+	}
+
 	return amounts0Deposited, amounts1Deposited, sharesIssued, nil
 }
 

diff --git a/x/dex/keeper/core_helper.go b/x/dex/keeper/core_helper.go
@@ -77,9 +77,8 @@ func (k Keeper) ValidateFee(ctx sdk.Context, fee uint64) error {
 //                            TOKENIZER UTILS                                //
 ///////////////////////////////////////////////////////////////////////////////
 
-func (k Keeper) MintShares(ctx sdk.Context, addr sdk.AccAddress, shareCoin sdk.Coin) error {
+func (k Keeper) MintShares(ctx sdk.Context, addr sdk.AccAddress, sharesCoins sdk.Coins) error {
 	// mint share tokens
-	sharesCoins := sdk.Coins{shareCoin}
 	if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sharesCoins); err != nil {
 		return err
 	}

diff --git a/x/dex/keeper/core_helper_test.go b/x/dex/keeper/core_helper_test.go
@@ -62,7 +62,7 @@ func (s *CoreHelpersTestSuite) setLPAtFee1Pool(tickIndex int64, amountA, amountB
 
 	totalShares := pool.CalcSharesMinted(amountAInt, amountBInt, existingShares)
 
-	err = s.app.DexKeeper.MintShares(s.ctx, s.alice, totalShares)
+	err = s.app.DexKeeper.MintShares(s.ctx, s.alice, sdk.NewCoins(totalShares))
 	s.Require().NoError(err)
 
 	lowerTick.ReservesMakerDenom = amountAInt

diff --git a/x/dex/keeper/msg_server_test.go b/x/dex/keeper/msg_server_test.go
@@ -543,7 +543,7 @@ func (s *DexTestSuite) deposits(
 		s.Assert().Fail("Only 1 pairID can be provided")
 	}
 
-	_, err := s.msgServer.Deposit(s.GoCtx, &types.MsgDeposit{
+	msg := &types.MsgDeposit{
 		Creator:         account.String(),
 		Receiver:        account.String(),
 		TokenA:          tokenA,
@@ -553,7 +553,10 @@ func (s *DexTestSuite) deposits(
 		TickIndexesAToB: tickIndexes,
 		Fees:            fees,
 		Options:         options,
-	})
+	}
+	err := msg.ValidateBasic()
+	s.Assert().NoError(err)
+	_, err = s.msgServer.Deposit(s.GoCtx, msg)
 	s.Assert().Nil(err)
 }
 

diff --git a/x/dex/types/errors.go b/x/dex/types/errors.go
@@ -164,4 +164,9 @@ var (
 		1149,
 		"Invalid Address",
 	)
+	ErrDuplicatePoolDeposit = sdkerrors.Register(
+		ModuleName,
+		1150,
+		"Can only provide a single deposit amount for each tick, fee pair",
+	)
 )
diff --git a/x/dex/types/message_deposit.go b/x/dex/types/message_deposit.go
@@ -1,6 +1,8 @@
 package types
 
 import (
+	"fmt"
+
 	sdkerrors "cosmossdk.io/errors"
 	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -78,7 +80,13 @@ func (msg *MsgDeposit) ValidateBasic() error {
 		return ErrZeroDeposit
 	}
 
+	poolsDeposited := make(map[string]bool)
 	for i := 0; i < numDeposits; i++ {
+		poolStr := fmt.Sprintf("%d-%d", msg.TickIndexesAToB[i], msg.Fees[i])
+		if _, ok := poolsDeposited[poolStr]; ok {
+			return ErrDuplicatePoolDeposit
+		}
+		poolsDeposited[poolStr] = true
 		if msg.AmountsA[i].LT(math.ZeroInt()) || msg.AmountsB[i].LT(math.ZeroInt()) {
 			return ErrZeroDeposit
 		}

diff --git a/x/dex/types/message_deposit_test.go b/x/dex/types/message_deposit_test.go
@@ -99,6 +99,18 @@ func TestMsgDeposit_ValidateBasic(t *testing.T) {
 			},
 			err: ErrZeroDeposit,
 		},
+		{
+			name: "invalid duplicate deposit",
+			msg: MsgDeposit{
+				Creator:         sample.AccAddress(),
+				Receiver:        sample.AccAddress(),
+				Fees:            []uint64{1, 2, 1},
+				TickIndexesAToB: []int64{0, 0, 0},
+				AmountsA:        []math.Int{math.OneInt(), math.OneInt(), math.OneInt()},
+				AmountsB:        []math.Int{math.OneInt(), math.OneInt(), math.OneInt()},
+			},
+			err: ErrDuplicatePoolDeposit,
+		},
 		{
 			name: "invalid no deposit",
 			msg: MsgDeposit{