Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Pool Coin Decimal Truncation During Deposit #446

Merged
15 changes: 11 additions & 4 deletions x/liquidity/keeper/invariants.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ func errorRate(expected, actual sdk.Dec) sdk.Dec {
return actual.Sub(expected).Quo(expected).Abs()
}

// TODO: update
// MintingPoolCoinsInvariant checks the correct ratio of minting amount of pool coins.
func MintingPoolCoinsInvariant(poolCoinTotalSupply, mintPoolCoin, depositCoinA, depositCoinB, lastReserveCoinA, lastReserveCoinB, refundedCoinA, refundedCoinB sdk.Int) {
if !refundedCoinA.IsZero() {
Expand All @@ -94,10 +95,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")
hallazzang marked this conversation as resolved.
Show resolved Hide resolved
}
}

if mintPoolCoin.GTE(coinAmountThreshold) &&
Expand All @@ -107,6 +113,7 @@ func MintingPoolCoinsInvariant(poolCoinTotalSupply, mintPoolCoin, depositCoinA,
}
}

// TODO: update
// DepositInvariant checks after deposit amounts.
func DepositInvariant(lastReserveCoinA, lastReserveCoinB, depositCoinA, depositCoinB, afterReserveCoinA, afterReserveCoinB, refundedCoinA, refundedCoinB sdk.Int) {
depositCoinA = depositCoinA.Sub(refundedCoinA)
Expand Down
101 changes: 37 additions & 64 deletions x/liquidity/keeper/liquidity_pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()),
Comment on lines +255 to +256
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().Mul(mintRate).TruncateInt()),
sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().Mul(mintRate).TruncateInt()),
sdk.NewCoin(depositCoins[0].Denom, lastReserveCoinA.Amount.ToDec().MulTruncate(mintRate).TruncateInt()),
sdk.NewCoin(depositCoins[1].Denom, lastReserveCoinB.Amount.ToDec().MulTruncate(mintRate).TruncateInt()),

What do you think about using MulTruncate instead of Mul here? @hallazzang @typark391 @kogisin
I think both will have the same result but that is more intuitive, or is there a case that is not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using MulTruncate here can cause(very rarely) a depositor to take advantage against the pool.
Since the result of MulTruncate is always less or equal than the result of Mul, I suggest to use Mul here.
How do others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. Let's keep it Mul first, reproduce the corresponding edge case, analyze it, and specify it on spec through other PRs.

)
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("no minted coins") // TODO: use proper error type
}

// 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))

Expand All @@ -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(
Expand All @@ -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(
Expand Down
143 changes: 140 additions & 3 deletions x/liquidity/keeper/liquidity_pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,143 @@ func TestExecuteDeposit(t *testing.T) {
require.Equal(t, poolCoin.Sub(poolCoinBefore), depositorBalance.Amount)
}

func TestExecuteDepositTruncation(t *testing.T) {
simapp, ctx := createTestInput()
simapp.LiquidityKeeper.SetParams(ctx, types.DefaultParams())
params := simapp.LiquidityKeeper.GetParams(ctx)

poolTypeID := types.DefaultPoolTypeID
addrs := app.AddTestAddrs(simapp, ctx, 4, params.PoolCreationFee)

denomA := "uETH"
denomB := "uUSD"
denomA, denomB = types.AlphabeticalDenomPair(denomA, denomB)

initDeposit := sdk.NewCoins(sdk.NewCoin(denomA, sdk.NewInt(10000000*1000000)), sdk.NewCoin(denomB, sdk.NewInt(200000000*1000000)))
app.SaveAccount(simapp, ctx, addrs[0], initDeposit)
app.SaveAccount(simapp, ctx, addrs[1], initDeposit)

depositA := simapp.BankKeeper.GetBalance(ctx, addrs[0], denomA)
depositB := simapp.BankKeeper.GetBalance(ctx, addrs[0], denomB)
depositBalance := sdk.NewCoins(depositA, depositB)

require.Equal(t, initDeposit, depositBalance)

depositA = simapp.BankKeeper.GetBalance(ctx, addrs[1], denomA)
depositB = simapp.BankKeeper.GetBalance(ctx, addrs[1], denomB)
depositBalance = sdk.NewCoins(depositA, depositB)

require.Equal(t, initDeposit, depositBalance)

createMsg := types.NewMsgCreatePool(addrs[0], poolTypeID, depositBalance)

_, err := simapp.LiquidityKeeper.CreatePool(ctx, createMsg)
require.NoError(t, err)

pools := simapp.LiquidityKeeper.GetAllPools(ctx)
pool := pools[0]

poolCoinBefore := simapp.LiquidityKeeper.GetPoolCoinTotalSupply(ctx, pool)

deposit := sdk.NewCoins(sdk.NewCoin(denomA, sdk.NewInt(19*1000000)), sdk.NewCoin(denomB, sdk.NewInt(400*1000000)))
depositMsg := types.NewMsgDepositWithinBatch(addrs[1], pool.Id, deposit)
_, err = simapp.LiquidityKeeper.DepositWithinBatch(ctx, depositMsg)
require.NoError(t, err)

poolBatch, found := simapp.LiquidityKeeper.GetPoolBatch(ctx, depositMsg.PoolId)
require.True(t, found)
msgs := simapp.LiquidityKeeper.GetAllPoolBatchDepositMsgs(ctx, poolBatch)
require.Equal(t, 1, len(msgs))

err = simapp.LiquidityKeeper.ExecuteDeposit(ctx, msgs[0], poolBatch)
require.NoError(t, err)

poolCoin := simapp.LiquidityKeeper.GetPoolCoinTotalSupply(ctx, pool)
depositorBalancePoolCoin := simapp.BankKeeper.GetBalance(ctx, addrs[1], pool.PoolCoinDenom)
depositorBalanceDenomA := simapp.BankKeeper.GetBalance(ctx, addrs[1], denomA)
depositorBalanceDenomB := simapp.BankKeeper.GetBalance(ctx, addrs[1], denomB)
require.Equal(t, depositBalance.AmountOf(denomA).Sub(sdk.NewInt(10*1000000)), depositorBalanceDenomA.Amount)
require.Equal(t, depositBalance.AmountOf(denomB).Sub(sdk.NewInt(200*1000000)), depositorBalanceDenomB.Amount)
require.Equal(t, depositorBalancePoolCoin.Amount, sdk.OneInt())
require.Equal(t, poolCoin.Sub(poolCoinBefore), depositorBalancePoolCoin.Amount)
}

func TestDepositDecimalTruncation(t *testing.T) {
simapp, ctx := createTestInput()
params := simapp.LiquidityKeeper.GetParams(ctx)
params.WithdrawFeeRate = sdk.ZeroDec()

// Suppose that atom price is $40.
denomA := "uatom"
denomB := "uusd"

addrs := app.AddTestAddrs(simapp, ctx, 2, sdk.NewCoins())
creator, depositor := addrs[0], addrs[1]

// Create a pool with initial value of $10M.
depositCoins := sdk.NewCoins(sdk.NewInt64Coin(denomA, 125000*1000000), sdk.NewInt64Coin(denomB, 5000000*1000000))
err := app.FundAccount(simapp, ctx, creator, depositCoins.Add(params.PoolCreationFee...))
require.NoError(t, err)
pool, err := simapp.LiquidityKeeper.CreatePool(ctx, types.NewMsgCreatePool(creator, types.DefaultPoolTypeID, depositCoins))
require.NoError(t, err)

// Deposit 19$.
depositCoins = sdk.NewCoins(sdk.NewInt64Coin(denomA, 0.2375*1000000), sdk.NewInt64Coin(denomB, 9.5*1000000))
err = app.FundAccount(simapp, ctx, depositor, depositCoins)
require.NoError(t, err)
_, err = simapp.LiquidityKeeper.DepositWithinBatch(ctx, types.NewMsgDepositWithinBatch(depositor, pool.Id, depositCoins))
require.NoError(t, err)
liquidity.BeginBlocker(ctx, simapp.LiquidityKeeper)
liquidity.EndBlocker(ctx, simapp.LiquidityKeeper)

depositorPoolCoin := simapp.BankKeeper.GetBalance(ctx, depositor, pool.PoolCoinDenom)
require.True(sdk.IntEq(t, sdk.OneInt(), depositorPoolCoin.Amount))
require.True(t, simapp.BankKeeper.GetBalance(ctx, depositor, denomA).Amount.IsPositive())
require.True(t, simapp.BankKeeper.GetBalance(ctx, depositor, denomB).Amount.IsPositive())

// Withdraw.
_, err = simapp.LiquidityKeeper.WithdrawWithinBatch(ctx, types.NewMsgWithdrawWithinBatch(depositor, pool.Id, depositorPoolCoin))
require.NoError(t, err)
liquidity.BeginBlocker(ctx, simapp.LiquidityKeeper)
liquidity.EndBlocker(ctx, simapp.LiquidityKeeper)
dongsam marked this conversation as resolved.
Show resolved Hide resolved

depositorCoinsDelta := depositCoins.Sub(simapp.BankKeeper.GetAllBalances(ctx, depositor))
require.True(t, depositorCoinsDelta.IsZero())
}

func TestDepositDecimalTruncation2(t *testing.T) {
simapp, ctx := createTestInput()
params := simapp.LiquidityKeeper.GetParams(ctx)
params.WithdrawFeeRate = sdk.ZeroDec()

// Suppose that atom price is $40.
denomA := "uatom"
denomB := "uusd"

addrs := app.AddTestAddrs(simapp, ctx, 2, sdk.NewCoins())
creator, depositor := addrs[0], addrs[1]

// Create a pool with initial value of $10M.
depositCoins := sdk.NewCoins(sdk.NewInt64Coin(denomA, 125000*1000000), sdk.NewInt64Coin(denomB, 5000000*1000000))
err := app.FundAccount(simapp, ctx, creator, depositCoins.Add(params.PoolCreationFee...))
require.NoError(t, err)
pool, err := simapp.LiquidityKeeper.CreatePool(ctx, types.NewMsgCreatePool(creator, types.DefaultPoolTypeID, depositCoins))
require.NoError(t, err)

// Deposit 9$.
depositCoins = sdk.NewCoins(sdk.NewInt64Coin(denomA, 0.1125*1000000), sdk.NewInt64Coin(denomB, 4.5*1000000))
err = app.FundAccount(simapp, ctx, depositor, depositCoins)
require.NoError(t, err)
_, err = simapp.LiquidityKeeper.DepositWithinBatch(ctx, types.NewMsgDepositWithinBatch(depositor, pool.Id, depositCoins))
require.NoError(t, err)
liquidity.BeginBlocker(ctx, simapp.LiquidityKeeper)
liquidity.EndBlocker(ctx, simapp.LiquidityKeeper)

depositorPoolCoin := simapp.BankKeeper.GetBalance(ctx, depositor, pool.PoolCoinDenom)
require.True(sdk.IntEq(t, sdk.ZeroInt(), depositorPoolCoin.Amount))
require.True(t, simapp.BankKeeper.GetAllBalances(ctx, depositor).IsEqual(depositCoins))
}

func TestReserveCoinLimit(t *testing.T) {
simapp, ctx := createTestInput()
params := types.DefaultParams()
Expand Down Expand Up @@ -860,10 +997,10 @@ func TestDepositWithCoinsSent(t *testing.T) {
liquidity.EndBlocker(ctx, simapp.LiquidityKeeper)

reserveCoins = simapp.LiquidityKeeper.GetReserveCoins(ctx, pool)
require.True(sdk.IntEq(t, sdk.NewInt(3999999), reserveCoins.AmountOf(DenomX))) // This is because of decimal truncation error.
require.True(sdk.IntEq(t, sdk.NewInt(4000000), reserveCoins.AmountOf(DenomX)))
require.True(sdk.IntEq(t, sdk.NewInt(6000000), reserveCoins.AmountOf(DenomY)))
balances := simapp.BankKeeper.GetAllBalances(ctx, addr)
require.True(sdk.IntEq(t, sdk.NewInt(1000001), balances.AmountOf(DenomX)))
require.True(sdk.IntEq(t, sdk.NewInt(1000000), balances.AmountOf(DenomX)))
require.True(sdk.IntEq(t, sdk.NewInt(0), balances.AmountOf(DenomY)))
require.True(sdk.IntEq(t, sdk.NewInt(999999), balances.AmountOf(pool.PoolCoinDenom)))
require.True(sdk.IntEq(t, sdk.NewInt(1000000), balances.AmountOf(pool.PoolCoinDenom)))
}