fix: Pool Coin Decimal Truncation During Deposit (tendermint#446)

* fix: wip poc for reproduce and fix poolcoin truncation

* fix: simplify calculation logic and add more tests

WIP

* fix: use equality check in MintingPoolCoinsInvariant

* test: fix expected value

due to the change in deposit logic, expected value in test
also changed

* test: add test for MintingPoolCoinsInvariant

* fix: update deposit truncation logic and simulation ordering

* docs: update changelog and readme

* fix: revert MulTruncate to Mul on Deposit

Co-authored-by: Hanjun Kim <hallazzang@gmail.com>

CHANGELOG.md

@@ -35,6 +35,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 # Changelog
 
+## v1.4.x - 2021.10.xx
+
+* [\#455](https://github.com/tendermint/liquidity/pull/455) (sdk) Bump SDK version to [v0.44.2](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.44.2)
+* [\#446](https://github.com/tendermint/liquidity/pull/446) Fix: Pool Coin Decimal Truncation During Deposit
+
 ## [Unreleased]
 
 ## [v1.4.0](https://github.com/tendermint/liquidity/releases/tag/v1.4.0) - 2021.09.07

README.md

@@ -35,7 +35,7 @@ For details, see the [Liquidity Module Light Paper](doc/LiquidityModuleLightPape
 Requirement | Notes
 ----------- | -----------------
 Go version  | Go1.15 or higher
-Cosmos SDK  | v0.44.0 or higher
+Cosmos SDK  | v0.44.2 or higher
 
 ### Get Liquidity Module source code
 

app/app.go

@@ -381,8 +381,8 @@ func NewLiquidityApp(
 		slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
 		params.NewAppModule(app.ParamsKeeper),
 		evidence.NewAppModule(app.EvidenceKeeper),
-		authzmodule.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
 		liquidity.NewAppModule(appCodec, app.LiquidityKeeper, app.AccountKeeper, app.BankKeeper, app.DistrKeeper),
+		authzmodule.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
 	)
 
 	// register the store decoders for simulation tests

x/liquidity/keeper/invariants.go

@@ -94,10 +94,15 @@ func MintingPoolCoinsInvariant(poolCoinTotalSupply, mintPoolCoin, depositCoinA,
 	expectedMintPoolCoinAmtBasedA := depositCoinARatio.MulInt(poolCoinTotalSupply).TruncateInt()
 	expectedMintPoolCoinAmtBasedB := depositCoinBRatio.MulInt(poolCoinTotalSupply).TruncateInt()
 
-	// NewPoolCoinAmount / LastPoolCoinSupply <= AfterRefundedDepositCoinA / LastReserveCoinA
-	// NewPoolCoinAmount / LastPoolCoinSupply <= AfterRefundedDepositCoinA / LastReserveCoinB
-	if depositCoinARatio.LT(poolCoinRatio) || depositCoinBRatio.LT(poolCoinRatio) {
-		panic("invariant check fails due to incorrect ratio of pool coins")
+	// NewPoolCoinAmount / LastPoolCoinSupply == AfterRefundedDepositCoinA / LastReserveCoinA
+	// NewPoolCoinAmount / LastPoolCoinSupply == AfterRefundedDepositCoinA / LastReserveCoinB
+	if depositCoinA.GTE(coinAmountThreshold) && depositCoinB.GTE(coinAmountThreshold) &&
+		lastReserveCoinA.GTE(coinAmountThreshold) && lastReserveCoinB.GTE(coinAmountThreshold) &&
+		mintPoolCoin.GTE(coinAmountThreshold) && poolCoinTotalSupply.GTE(coinAmountThreshold) {
+		if errorRate(depositCoinARatio, poolCoinRatio).GT(errorRateThreshold) ||
+			errorRate(depositCoinBRatio, poolCoinRatio).GT(errorRateThreshold) {
+			panic("invariant check fails due to incorrect ratio of pool coins")
+		}
 	}
 
 	if mintPoolCoin.GTE(coinAmountThreshold) &&

x/liquidity/keeper/invariants_test.go

@@ -21,6 +21,62 @@ func TestWithdrawRatioInvariant(t *testing.T) {
 	})
 }
 
+func TestMintingPoolCoinsInvariant(t *testing.T) {
+	for _, tc := range []struct {
+		poolCoinSupply  int64
+		mintingPoolCoin int64
+		reserveA        int64
+		depositA        int64
+		refundedA       int64
+		reserveB        int64
+		depositB        int64
+		refundedB       int64
+		expectPanic     bool
+	}{
+		{
+			10000, 1000,
+			100000, 10000, 0,
+			100000, 10000, 0,
+			false,
+		},
+		{
+			10000, 1000,
+			100000, 10000, 5000,
+			100000, 10000, 300,
+			true,
+		},
+		{
+			3000000, 100,
+			100000000, 4000, 667,
+			200000000, 8000, 1334,
+			false,
+		},
+		{
+			3000000, 100,
+			100000000, 4000, 0,
+			200000000, 8000, 1334,
+			true,
+		},
+	} {
+		f := require.NotPanics
+		if tc.expectPanic {
+			f = require.Panics
+		}
+		f(t, func() {
+			keeper.MintingPoolCoinsInvariant(
+				sdk.NewInt(tc.poolCoinSupply),
+				sdk.NewInt(tc.mintingPoolCoin),
+				sdk.NewInt(tc.depositA),
+				sdk.NewInt(tc.depositB),
+				sdk.NewInt(tc.reserveA),
+				sdk.NewInt(tc.reserveB),
+				sdk.NewInt(tc.refundedA),
+				sdk.NewInt(tc.refundedB),
+			)
+		})
+	}
+}
+
 func TestLiquidityPoolsEscrowAmountInvariant(t *testing.T) {
 	simapp, ctx := app.CreateTestInput()
 

x/liquidity/keeper/liquidity_pool.go

@@ -188,9 +188,6 @@ func (k Keeper) ExecuteDeposit(ctx sdk.Context, msg types.DepositMsgState, batch
 
 	params := k.GetParams(ctx)
 
-	var inputs []banktypes.Input
-	var outputs []banktypes.Output
-
 	reserveCoins := k.GetReserveCoins(ctx, pool)
 
 	// reinitialize pool if the pool is depleted
@@ -240,77 +237,53 @@ func (k Keeper) ExecuteDeposit(ctx sdk.Context, msg types.DepositMsgState, batch
 		return nil
 	}
 
-	// only two coins are acceptable
-	if reserveCoins.Len() != msg.Msg.DepositCoins.Len() {
-		return types.ErrNumOfReserveCoin
-	}
-
 	reserveCoins.Sort()
 
-	// Decimal Error, divide the Int coin amount by the Decimal Rate and erase the decimal point to deposit a lower value
-	lastReserveCoinA := reserveCoins[0].Amount
-	lastReserveCoinB := reserveCoins[1].Amount
-	lastReserveRatio := lastReserveCoinA.ToDec().QuoTruncate(lastReserveCoinB.ToDec())
+	lastReserveCoinA := reserveCoins[0]
+	lastReserveCoinB := reserveCoins[1]
 
 	depositCoinA := depositCoins[0]
 	depositCoinB := depositCoins[1]
-	depositCoinAmountA := depositCoinA.Amount
-	depositCoinAmountB := depositCoinB.Amount
-	depositableCoinAmountA := depositCoinB.Amount.ToDec().MulTruncate(lastReserveRatio).TruncateInt()
-
-	refundedCoins := sdk.NewCoins()
-	refundedCoinA := sdk.ZeroInt()
-	refundedCoinB := sdk.ZeroInt()
 
-	var acceptedCoins sdk.Coins
-	// handle when depositing coin A amount is less than, greater than or equal to depositable amount
-	if depositCoinA.Amount.LT(depositableCoinAmountA) {
-		depositCoinAmountB = depositCoinA.Amount.ToDec().QuoTruncate(lastReserveRatio).TruncateInt()
-		acceptedCoins = sdk.NewCoins(depositCoinA, sdk.NewCoin(depositCoinB.Denom, depositCoinAmountB))
-
-		inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, acceptedCoins))
-		outputs = append(outputs, banktypes.NewOutput(reserveAcc, acceptedCoins))
-
-		refundedCoinB = depositCoinB.Amount.Sub(depositCoinAmountB)
-
-		if refundedCoinB.IsPositive() {
-			refundedCoins = sdk.NewCoins(sdk.NewCoin(depositCoinB.Denom, refundedCoinB))
-			inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, refundedCoins))
-			outputs = append(outputs, banktypes.NewOutput(depositor, refundedCoins))
-		}
-	} else if depositCoinA.Amount.GT(depositableCoinAmountA) {
-		depositCoinAmountA = depositCoinB.Amount.ToDec().MulTruncate(lastReserveRatio).TruncateInt()
-		acceptedCoins = sdk.NewCoins(depositCoinB, sdk.NewCoin(depositCoinA.Denom, depositCoinAmountA))
-
-		inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, acceptedCoins))
-		outputs = append(outputs, banktypes.NewOutput(reserveAcc, acceptedCoins))
+	poolCoinTotalSupply := k.GetPoolCoinTotalSupply(ctx, pool)
+	poolCoinMintAmt := sdk.MinDec(
+		poolCoinTotalSupply.ToDec().MulTruncate(depositCoinA.Amount.ToDec()).QuoTruncate(lastReserveCoinA.Amount.ToDec()),
+		poolCoinTotalSupply.ToDec().MulTruncate(depositCoinB.Amount.ToDec()).QuoTruncate(lastReserveCoinB.Amount.ToDec()),
+	)
+	mintRate := poolCoinMintAmt.TruncateDec().QuoTruncate(poolCoinTotalSupply.ToDec())
+	acceptedCoins := sdk.NewCoins(
+		sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().Mul(mintRate).TruncateInt()),
+		sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().Mul(mintRate).TruncateInt()),
+	)
+	refundedCoins := depositCoins.Sub(acceptedCoins)
+	refundedCoinA := sdk.NewCoin(depositCoinA.Denom, refundedCoins.AmountOf(depositCoinA.Denom))
+	refundedCoinB := sdk.NewCoin(depositCoinB.Denom, refundedCoins.AmountOf(depositCoinB.Denom))
 
-		refundedCoinA = depositCoinA.Amount.Sub(depositCoinAmountA)
+	mintPoolCoin := sdk.NewCoin(pool.PoolCoinDenom, poolCoinMintAmt.TruncateInt())
+	mintPoolCoins := sdk.NewCoins(mintPoolCoin)
 
-		if refundedCoinA.IsPositive() {
-			refundedCoins = sdk.NewCoins(sdk.NewCoin(depositCoinA.Denom, refundedCoinA))
-			inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, refundedCoins))
-			outputs = append(outputs, banktypes.NewOutput(depositor, refundedCoins))
-		}
-	} else {
-		acceptedCoins = sdk.NewCoins(depositCoinA, depositCoinB)
-		inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, acceptedCoins))
-		outputs = append(outputs, banktypes.NewOutput(reserveAcc, acceptedCoins))
+	if mintPoolCoins.IsZero() || acceptedCoins.IsZero() {
+		return fmt.Errorf("pool coin truncated, no accepted coin, refund")
 	}
 
-	// calculate pool token mint amount
-	poolCoinTotalSupply := k.GetPoolCoinTotalSupply(ctx, pool)
-	poolCoinAmt := sdk.MinInt(
-		poolCoinTotalSupply.ToDec().MulTruncate(depositCoinAmountA.ToDec()).QuoTruncate(reserveCoins[0].Amount.ToDec()).TruncateInt(),
-		poolCoinTotalSupply.ToDec().MulTruncate(depositCoinAmountB.ToDec()).QuoTruncate(reserveCoins[1].Amount.ToDec()).TruncateInt())
-	mintPoolCoin := sdk.NewCoin(pool.PoolCoinDenom, poolCoinAmt)
-	mintPoolCoins := sdk.NewCoins(mintPoolCoin)
-
-	// mint pool token to the depositor
 	if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, mintPoolCoins); err != nil {
 		return err
 	}
 
+	var inputs []banktypes.Input
+	var outputs []banktypes.Output
+
+	if !refundedCoins.IsZero() {
+		// refund truncated deposit coins
+		inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, refundedCoins))
+		outputs = append(outputs, banktypes.NewOutput(depositor, refundedCoins))
+	}
+
+	// send accepted deposit coins
+	inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, acceptedCoins))
+	outputs = append(outputs, banktypes.NewOutput(reserveAcc, acceptedCoins))
+
+	// send minted pool coins
 	inputs = append(inputs, banktypes.NewInput(batchEscrowAcc, mintPoolCoins))
 	outputs = append(outputs, banktypes.NewOutput(depositor, mintPoolCoins))
 
@@ -329,9 +302,9 @@ func (k Keeper) ExecuteDeposit(ctx sdk.Context, msg types.DepositMsgState, batch
 		afterReserveCoinB := afterReserveCoins[1].Amount
 
 		MintingPoolCoinsInvariant(poolCoinTotalSupply, mintPoolCoin.Amount, depositCoinA.Amount, depositCoinB.Amount,
-			lastReserveCoinA, lastReserveCoinB, refundedCoinA, refundedCoinB)
-		DepositInvariant(lastReserveCoinA, lastReserveCoinB, depositCoinA.Amount, depositCoinB.Amount,
-			afterReserveCoinA, afterReserveCoinB, refundedCoinA, refundedCoinB)
+			lastReserveCoinA.Amount, lastReserveCoinB.Amount, refundedCoinA.Amount, refundedCoinB.Amount)
+		DepositInvariant(lastReserveCoinA.Amount, lastReserveCoinB.Amount, depositCoinA.Amount, depositCoinB.Amount,
+			afterReserveCoinA, afterReserveCoinB, refundedCoinA.Amount, refundedCoinB.Amount)
 	}
 
 	ctx.EventManager().EmitEvent(
@@ -350,7 +323,7 @@ func (k Keeper) ExecuteDeposit(ctx sdk.Context, msg types.DepositMsgState, batch
 	)
 
 	reserveCoins = k.GetReserveCoins(ctx, pool)
-	lastReserveRatio = sdk.NewDecFromInt(reserveCoins[0].Amount).Quo(sdk.NewDecFromInt(reserveCoins[1].Amount))
+	lastReserveRatio := sdk.NewDecFromInt(reserveCoins[0].Amount).Quo(sdk.NewDecFromInt(reserveCoins[1].Amount))
 
 	logger := k.Logger(ctx)
 	logger.Debug(