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.

Sign up for GitHub

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
Merged

Pool Cleanup #652

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
485f4b4
add pool cleanup
mconcat Dec 10, 2021
cc252f2
add pool destruction on cleanup
mconcat Dec 10, 2021
f5b02a7
gofmt
mconcat Dec 10, 2021
bc1859e
Update x/gamm/keeper/pool.go
mconcat Dec 14, 2021
1d272cd
Merge branch 'main' of github.com:osmosis-labs/osmosis into mconcat/5…
mconcat Dec 14, 2021
f52ab62
fix test
mconcat Dec 14, 2021
e1d9bcb
add ForceUnlock
mconcat Dec 17, 2021
3b6af5c
Merge branch 'mconcat/502-pool-cleanup' of github.com:osmosis-labs/os…
mconcat Dec 17, 2021
1436202
move to batch processing cleanup, exclude module accounts
mconcat Dec 23, 2021
57304a8
Update x/lockup/keeper/lock.go
mconcat Dec 23, 2021
c2e0c28
fix lock iteration and reuse unlock logic
mconcat Jan 6, 2022
0bd19df
Merge branch 'main' of github.com:osmosis-labs/osmosis into mconcat/5…
mconcat Jan 6, 2022
3f4526f
Minor grammar fixes
ValarDragon Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions proto/osmosis/gamm/v1beta1/balancerPool.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,9 @@ message BalancerPool {
(gogoproto.moretags) = "yaml:\"total_weight\"",
(gogoproto.nullable) = false
];

google.protobuf.Timestamp destructionTime = 8[
(gogoproto.stdtime) = true,
(gogoproto.moretags) = "yaml:\"destruction_time\""
];
}
66 changes: 66 additions & 0 deletions x/gamm/keeper/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,72 @@ func (k Keeper) SetPool(ctx sdk.Context, pool types.PoolI) error {
return nil
}

// CleanupBalancerPool destructs a pool and refund all the assets according to
// the shares held by the accounts. CleanupBalancerPool should be called not during
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
// the chain execution time, as it iterates the entire account balances.
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) CleanupBalancerPool(ctx sdk.Context, poolId uint64) (err error) {
pool, err := k.GetPool(ctx, poolId)
if err != nil {
return err
}

totalShares := pool.GetTotalShares().Amount
shareDenom := pool.GetTotalShares().Denom
poolAssets := pool.GetAllPoolAssets()

// 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 •
edited
Loading

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.Denom != shareDenom || coin.Amount.IsZero() {
return
}

shareAmount := coin.Amount
Copy link
Member

Choose a reason for hiding this comment

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

lets alias coin to userPoolShare here. I think it makes a lot of subsequent lines easier to read


// 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
pool.SubTotalShares(shareAmount)
for _, asset := range poolAssets {
err = k.bankKeeper.SendCoins(
ctx, pool.GetAddress(), addr, sdk.Coins{{asset.Token.Denom, asset.Token.Amount.Mul(shareAmount).Quo(totalShares)}})
if err != nil {
return true
}
}

return false
})

if err != nil {
return err
}

// sanity check
if !pool.GetTotalShares().IsZero() {
panic("pool total share should be zero after cleanup")
}
Copy link
Member

Choose a reason for hiding this comment

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

Can zero share be assured everytime(division factor etc.)?
My only concern is that it might not: considering the fact that I remember that somewhere in the code what we did was iterate through coins and send a part of it every iteration, and at the last iteration, we would send the all the remainings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point, I originally thought the total shares are only added/subtracted, not divided; I will check if there are any case of division.

Copy link
Member

Choose a reason for hiding this comment

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

Is this fixed? / checked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because shares are just a sdk.Coin, it could be IBC'd out or sent to module logics, and stay under the module account. I am pretty sure if we count all the address held shares and module held shares, it would sum up to GetTotalShares(). Let me add the logic for module accounts to(or, is it already iterated by GetAllBalances?)

Copy link
Member

Choose a reason for hiding this comment

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

Can you investigate if module accounts are iterated over? I think we should definitely exclude certain module accounts, e.g. IBC

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The iteration do include the module accounts. I will exclude the module account held LP tokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, annoyingly, there is no way to distinguish module address and normal address by only using the address itself. We could read all the module address and compare to them each time iterating over the list of address, but at this stage it feels little adhoc. Let me think about some alternatives a bit.


err = pool.SetDestruction(ctx.BlockTime())
if err != nil {
return err
}
err = k.SetPool(ctx, pool)
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 types.BalancerPoolParams, assets []types.PoolAsset, futureGovernor string) (types.PoolI, error) {
Expand Down
169 changes: 169 additions & 0 deletions x/gamm/keeper/pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
package keeper_test

import (
"math/rand"
"time"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"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 := suite.app.BankKeeper.AddCoins(
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, poolId)
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 := suite.app.BankKeeper.AddCoins(
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, poolId)
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 := suite.app.BankKeeper.AddCoins(
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, poolId)
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)
suite.Equal(err.Error(), sdkerrors.Wrapf(types.ErrPoolLocked, "swap on inactive pool").Error())
}
Loading