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

Pool Cleanup #652

Merged
merged 13 commits into from
Jan 6, 2022
103 changes: 103 additions & 0 deletions x/gamm/keeper/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,109 @@ func (k Keeper) SetPool(ctx sdk.Context, pool types.PoolI) error {
return nil
}

func (k Keeper) DeletePool(ctx sdk.Context, poolId uint64) error {
store := ctx.KVStore(k.storeKey)
poolKey := types.GetKeyPrefixPools(poolId)
if !store.Has(poolKey) {
return fmt.Errorf("pool with ID %d does not exist", poolId)
}

store.Delete(poolKey)
return nil
}

// CleanupBalancerPool destructs a pool and refund all the assets according to
// the shares held by the accounts. CleanupBalancerPool should not be called during
// the chain execution time, as it iterates the entire account balances.
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
// TODO: once SDK v0.46.0, use https://github.com/cosmos/cosmos-sdk/pull/9611
//
// All locks on this pool share must be unlocked prior to execution. Use LockupKeeper.ForceUnlock
// on remaining locks before calling this function.
func (k Keeper) CleanupBalancerPool(ctx sdk.Context, poolIds []uint64, excludedModules []string) (err error) {
pools := make(map[string]types.PoolI)
totalShares := make(map[string]sdk.Int)
for _, poolId := range poolIds {
pool, err := k.GetPool(ctx, poolId)
if err != nil {
return err
}
shareDenom := pool.GetTotalShares().Denom
pools[shareDenom] = pool
totalShares[shareDenom] = pool.GetTotalShares().Amount
}

moduleAccounts := make(map[string]string)
for _, module := range excludedModules {
moduleAccounts[string(authtypes.NewModuleAddress(module))] = module
}
Comment on lines +114 to +117
Copy link
Member

Choose a reason for hiding this comment

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

Nice!


// first iterate through the share holders and burn them
k.bankKeeper.IterateAllBalances(ctx, func(addr sdk.AccAddress, coin sdk.Coin) (stop bool) {
Copy link
Member

@ValarDragon ValarDragon Dec 21, 2021

Choose a reason for hiding this comment

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

Lets file an issue to revisit performance of this. Either we:

Copy link
Member

Choose a reason for hiding this comment

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

Wait aren't we going to be iterating over IBC's module accounts like this? If so, we will be making IBC'd tokens never redeemable. Do we need to care about this, cc @sunnya97 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the original issue I think the IBC'd LP tokens are never redeemable for now(also there is no use case of IBCing out LP tokens yet). I could apply the first and the third suggestion now.

if coin.Amount.IsZero() {
return
}

pool, ok := pools[coin.Denom]
if !ok {
return
}

// track the iterated shares
pool.SubTotalShares(coin.Amount)
pools[coin.Denom] = pool

// check if the shareholder is a module
if _, ok = moduleAccounts[coin.Denom]; ok {
return
}

// Burn the share tokens
err = k.bankKeeper.SendCoinsFromAccountToModule(ctx, addr, types.ModuleName, sdk.Coins{coin})
if err != nil {
return true
}

err = k.bankKeeper.BurnCoins(ctx, types.ModuleName, sdk.Coins{coin})
if err != nil {
return true
}

// Refund assets
for _, asset := range pool.GetAllPoolAssets() {
// lpShareEquivalentTokens = (amount in pool) * (your shares) / (total shares)
lpShareEquivalentTokens := asset.Token.Amount.Mul(coin.Amount).Quo(totalShares[coin.Denom])
if lpShareEquivalentTokens.IsZero() {
continue
}
err = k.bankKeeper.SendCoins(
ctx, pool.GetAddress(), addr, sdk.Coins{{asset.Token.Denom, lpShareEquivalentTokens}})
if err != nil {
return true
}
}

return false
})

if err != nil {
return err
}

for _, pool := range pools {
// sanity check
if !pool.GetTotalShares().IsZero() {
panic("pool total share should be zero after cleanup")
}

err = k.DeletePool(ctx, pool.GetId())
if err != nil {
return err
}
}

return nil
}

// newBalancerPool is an internal function that creates a new Balancer Pool object with the provided
// parameters, initial assets, and future governor.
func (k Keeper) newBalancerPool(ctx sdk.Context, balancerPoolParams balancer.BalancerPoolParams, assets []types.PoolAsset, futureGovernor string) (types.PoolI, error) {
Expand Down
222 changes: 222 additions & 0 deletions x/gamm/keeper/pool_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
package keeper_test

import (
"math/rand"
"time"

"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/osmosis-labs/osmosis/x/gamm/types"
)

func (suite *KeeperTestSuite) TestCleanupPool() {
// Mint some assets to the accounts.
for _, acc := range []sdk.AccAddress{acc1, acc2, acc3} {
err := simapp.FundAccount(
suite.app.BankKeeper,
suite.ctx,
acc,
sdk.NewCoins(
sdk.NewCoin("uosmo", sdk.NewInt(1000000000)),
sdk.NewCoin("foo", sdk.NewInt(1000)),
sdk.NewCoin("bar", sdk.NewInt(1000)),
sdk.NewCoin("baz", sdk.NewInt(1000)),
),
)
if err != nil {
panic(err)
}
}

poolId, err := suite.app.GAMMKeeper.CreateBalancerPool(suite.ctx, acc1, defaultBalancerPoolParams, []types.PoolAsset{
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("foo", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("bar", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("baz", sdk.NewInt(1000)),
},
}, "")
suite.NoError(err)

for _, acc := range []sdk.AccAddress{acc2, acc3} {
err = suite.app.GAMMKeeper.JoinPool(suite.ctx, acc, poolId, types.OneShare.MulRaw(100), sdk.NewCoins(
sdk.NewCoin("foo", sdk.NewInt(1000)),
sdk.NewCoin("bar", sdk.NewInt(1000)),
sdk.NewCoin("baz", sdk.NewInt(1000)),
))
suite.NoError(err)
}

pool, err := suite.app.GAMMKeeper.GetPool(suite.ctx, poolId)
suite.NoError(err)
denom := pool.GetTotalShares().Denom
totalAmount := sdk.ZeroInt()
for _, acc := range []sdk.AccAddress{acc1, acc2, acc3} {
coin := suite.app.BankKeeper.GetBalance(suite.ctx, acc, denom)
suite.True(coin.Amount.Equal(types.OneShare.MulRaw(100)))
totalAmount = totalAmount.Add(coin.Amount)
}
suite.True(totalAmount.Equal(types.OneShare.MulRaw(300)))

err = suite.app.GAMMKeeper.CleanupBalancerPool(suite.ctx, []uint64{poolId}, []string{})
suite.NoError(err)
for _, acc := range []sdk.AccAddress{acc1, acc2, acc3} {
for _, denom := range []string{"foo", "bar", "baz"} {
amt := suite.app.BankKeeper.GetBalance(suite.ctx, acc, denom)
suite.True(amt.Amount.Equal(sdk.NewInt(1000)),
"Expected equal %s: %d, %d", amt.Denom, amt.Amount.Int64(), 1000)
}
}
}

func (suite *KeeperTestSuite) TestCleanupPoolRandomized() {
// address => deposited coins
coinOf := make(map[string]sdk.Coins)
denoms := []string{"foo", "bar", "baz"}

// Mint some assets to the accounts.
for _, acc := range []sdk.AccAddress{acc1, acc2, acc3} {
coins := make(sdk.Coins, 3)
for i := range coins {
amount := sdk.NewInt(rand.Int63n(1000))
// give large amount of coins to the pool creator
if i == 0 {
amount = amount.MulRaw(10000)
}
coins[i] = sdk.Coin{denoms[i], amount}
}
coinOf[acc.String()] = coins
coins = append(coins, sdk.NewCoin("uosmo", sdk.NewInt(1000000000)))

err := simapp.FundAccount(
suite.app.BankKeeper,
suite.ctx,
acc,
coins.Sort(),
)
if err != nil {
panic(err)
}
}

initialAssets := []types.PoolAsset{}
for _, coin := range coinOf[acc1.String()] {
initialAssets = append(initialAssets, types.PoolAsset{Weight: types.OneShare.MulRaw(100), Token: coin})
}
poolId, err := suite.app.GAMMKeeper.CreateBalancerPool(suite.ctx, acc1, defaultBalancerPoolParams, initialAssets, "")
suite.NoError(err)

for _, acc := range []sdk.AccAddress{acc2, acc3} {
err = suite.app.GAMMKeeper.JoinPool(suite.ctx, acc, poolId, types.OneShare, coinOf[acc.String()])
suite.NoError(err)
}

err = suite.app.GAMMKeeper.CleanupBalancerPool(suite.ctx, []uint64{poolId}, []string{})
suite.NoError(err)
for _, acc := range []sdk.AccAddress{acc1, acc2, acc3} {
for _, coin := range coinOf[acc.String()] {
amt := suite.app.BankKeeper.GetBalance(suite.ctx, acc, coin.Denom)
// the refund could have rounding error
suite.True(amt.Amount.Equal(coin.Amount) || amt.Amount.Equal(coin.Amount.SubRaw(1)),
"Expected equal %s: %d, %d", amt.Denom, amt.Amount.Int64(), coin.Amount.Int64())
}
}
}

func (suite *KeeperTestSuite) TestCleanupPoolErrorOnSwap() {
suite.ctx = suite.ctx.WithBlockTime(time.Unix(1000, 1000))
err := simapp.FundAccount(
suite.app.BankKeeper,
suite.ctx,
acc1,
sdk.NewCoins(
sdk.NewCoin("uosmo", sdk.NewInt(1000000000)),
sdk.NewCoin("foo", sdk.NewInt(1000)),
sdk.NewCoin("bar", sdk.NewInt(1000)),
sdk.NewCoin("baz", sdk.NewInt(1000)),
),
)
if err != nil {
panic(err)
}

poolId, err := suite.app.GAMMKeeper.CreateBalancerPool(suite.ctx, acc1, defaultBalancerPoolParams, []types.PoolAsset{
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("foo", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("bar", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("baz", sdk.NewInt(1000)),
},
}, "")
suite.NoError(err)

err = suite.app.GAMMKeeper.CleanupBalancerPool(suite.ctx, []uint64{poolId}, []string{})
suite.NoError(err)

_, _, err = suite.app.GAMMKeeper.SwapExactAmountIn(suite.ctx, acc1, poolId, sdk.NewCoin("foo", sdk.NewInt(1)), "bar", sdk.NewInt(1))
suite.Error(err)
}

func (suite *KeeperTestSuite) TestCleanupPoolWithLockup() {
suite.ctx = suite.ctx.WithBlockTime(time.Unix(1000, 1000))
err := simapp.FundAccount(
suite.app.BankKeeper,
suite.ctx,
acc1,
sdk.NewCoins(
sdk.NewCoin("uosmo", sdk.NewInt(1000000000)),
sdk.NewCoin("foo", sdk.NewInt(1000)),
sdk.NewCoin("bar", sdk.NewInt(1000)),
sdk.NewCoin("baz", sdk.NewInt(1000)),
),
)
if err != nil {
panic(err)
}

poolId, err := suite.app.GAMMKeeper.CreateBalancerPool(suite.ctx, acc1, defaultBalancerPoolParams, []types.PoolAsset{
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("foo", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("bar", sdk.NewInt(1000)),
},
{
Weight: sdk.NewInt(100),
Token: sdk.NewCoin("baz", sdk.NewInt(1000)),
},
}, "")
suite.NoError(err)

_, err = suite.app.LockupKeeper.LockTokens(suite.ctx, acc1, sdk.Coins{sdk.NewCoin(types.GetPoolShareDenom(poolId), types.InitPoolSharesSupply)}, time.Hour)
suite.NoError(err)

for _, lock := range suite.app.LockupKeeper.GetLocksDenom(suite.ctx, types.GetPoolShareDenom(poolId)) {
err = suite.app.LockupKeeper.ForceUnlock(suite.ctx, lock)
suite.NoError(err)
}

err = suite.app.GAMMKeeper.CleanupBalancerPool(suite.ctx, []uint64{poolId}, []string{})
suite.NoError(err)
for _, coin := range []string{"foo", "bar", "baz"} {
amt := suite.app.BankKeeper.GetBalance(suite.ctx, acc1, coin)
// the refund could have rounding error
suite.True(amt.Amount.Equal(sdk.NewInt(1000)) || amt.Amount.Equal(sdk.NewInt(1000).SubRaw(1)),
"Expected equal %s: %d, %d", amt.Denom, amt.Amount.Int64(), sdk.NewInt(1000).Int64())
}
}
17 changes: 9 additions & 8 deletions x/gamm/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ import sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

// x/gamm module sentinel errors
var (
ErrPoolNotFound = sdkerrors.Register(ModuleName, 1, "pool not found")
ErrPoolAlreadyExist = sdkerrors.Register(ModuleName, 2, "pool already exist")
ErrPoolLocked = sdkerrors.Register(ModuleName, 3, "pool is locked")
ErrTooFewPoolAssets = sdkerrors.Register(ModuleName, 4, "pool should have at least 2 assets, as they must be swapping between at least two assets")
ErrTooManyPoolAssets = sdkerrors.Register(ModuleName, 5, "pool has too many assets (currently capped at 8 assets per pool)")
ErrLimitMaxAmount = sdkerrors.Register(ModuleName, 6, "calculated amount is larger than max amount")
ErrLimitMinAmount = sdkerrors.Register(ModuleName, 7, "calculated amount is lesser than min amount")
ErrInvalidMathApprox = sdkerrors.Register(ModuleName, 8, "invalid calculated result")
ErrPoolNotFound = sdkerrors.Register(ModuleName, 1, "pool not found")
ErrPoolAlreadyExist = sdkerrors.Register(ModuleName, 2, "pool already exist")
ErrPoolLocked = sdkerrors.Register(ModuleName, 3, "pool is locked")
ErrTooFewPoolAssets = sdkerrors.Register(ModuleName, 4, "pool should have at least 2 assets, as they must be swapping between at least two assets")
ErrTooManyPoolAssets = sdkerrors.Register(ModuleName, 5, "pool has too many assets (currently capped at 8 assets per pool)")
ErrLimitMaxAmount = sdkerrors.Register(ModuleName, 6, "calculated amount is larger than max amount")
ErrLimitMinAmount = sdkerrors.Register(ModuleName, 7, "calculated amount is lesser than min amount")
ErrInvalidMathApprox = sdkerrors.Register(ModuleName, 8, "invalid calculated result")
ErrAlreadyInvalidPool = sdkerrors.Register(ModuleName, 9, "destruction on already invalid pool")

ErrEmptyRoutes = sdkerrors.Register(ModuleName, 21, "routes not defined")
ErrEmptyPoolAssets = sdkerrors.Register(ModuleName, 22, "PoolAssets not defined")
Expand Down
1 change: 1 addition & 0 deletions x/gamm/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ type BankKeeper interface {
GetSupply(ctx sdk.Context, denom string) sdk.Coin
UndelegateCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
DelegateCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
IterateAllBalances(ctx sdk.Context, callback func(addr sdk.AccAddress, coin sdk.Coin) (stop bool))
}

// DistrKeeper defines the contract needed to be fulfilled for distribution keeper
Expand Down
Loading