Pool Cleanup #652

mconcat · 2021-12-10T17:26:35Z

Closes: #502

Description

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

x/gamm/keeper/pool.go

Co-authored-by: Sunny Aggarwal <[email protected]>

…02-pool-cleanup

ValarDragon · 2021-12-14T06:54:33Z

Is the idea of setting a destruction time that I can make a pool get deleted in the current block (e.g. at upgrade), and schedule a deletion in the future?

Shouldn't we make it so that the pool deletion also deletes the pool from state, or is it a goal / low cost to keep the literal pool in state as long as its marked inactive for integrators?

mattverse · 2021-12-15T04:53:56Z

x/gamm/keeper/pool.go

+	if !pool.GetTotalShares().IsZero() {
+		panic("pool total share should be zero after cleanup")
+	}


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.

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

Is this fixed? / checked?

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

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

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

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.

mattverse · 2021-12-15T05:06:51Z

Similar but different to @ValarDragon 's comment, I'm curious when this CleanupBalancerPool is designed to be ran

mattverse · 2021-12-15T05:07:20Z

A friendly reminder to fix test!

mconcat · 2021-12-16T07:00:38Z

I intended pool destruction in order to make it able to resurrected in the future, but I think we don't need to store the actual time here. However, I agree that it does not really make sense to just pause the pool in this use case, as we completely destruct the pool and refund the whole assets. I will change it to state removal.

Is the idea of setting a destruction time that I can make a pool get deleted in the current block (e.g. at upgrade), and schedule a deletion in the future?

Shouldn't we make it so that the pool deletion also deletes the pool from state, or is it a goal / low cost to keep the literal pool in state as long as its marked inactive for integrators?

It is designed to ran on the chain upgrade time, so we don't have a lot of computation time restriction.

Similar but different to @ValarDragon 's comment, I'm curious when this CleanupBalancerPool is designed to be ran

…mosis into mconcat/502-pool-cleanup

codecov-commenter · 2021-12-17T06:33:11Z

Codecov Report

Merging #652 (0bd19df) into main (8342388) will increase coverage by 0.04%.
The diff coverage is 30.58%.

@@            Coverage Diff             @@
##             main     #652      +/-   ##
==========================================
+ Coverage   18.69%   18.73%   +0.04%     
==========================================
  Files         171      171              
  Lines       23816    23901      +85     
==========================================
+ Hits         4452     4478      +26     
- Misses      18601    18649      +48     
- Partials      763      774      +11

Impacted Files	Coverage Δ
x/gamm/types/query.pb.gw.go	`0.00% <0.00%> (ø)`
x/lockup/keeper/lock.go	`61.26% <14.28%> (-1.34%)`	⬇️
x/gamm/keeper/pool.go	`69.67% <52.08%> (-11.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8342388...0bd19df. Read the comment docs.

mconcat · 2021-12-17T06:35:13Z

R4R

ValarDragon · 2021-12-21T03:53:09Z

Notes from call earlier today/yesterday
* The flow is:
* DeletePool ->
* Force Unlock every LP share (ForceUnlock)
* Iterate through bank, and migrate every LP share to constituent assets
* Delete Pool data (via ClearPool)

ValarDragon · 2021-12-21T20:49:51Z

x/gamm/keeper/pool.go

+	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) {


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

change to do one iteration for all pools

wait for perf: x/bank create reverse prefix for denom<->address cosmos/cosmos-sdk#9611 to get merged into a release before we actually do the upgrade

iterate over addresses directly, and just get the balance of the relevant coin (rather than iterate over every addr,coin pair)

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 ?

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.

ValarDragon · 2021-12-21T20:56:37Z

x/gamm/keeper/pool.go

+			return
+		}
+
+		shareAmount := coin.Amount


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

ValarDragon · 2021-12-21T22:01:12Z

x/gamm/keeper/pool.go

+
+		// Refund assets
+		for _, asset := range poolAssets {
+			assetAmount := asset.Token.Amount.Mul(shareAmount).Quo(totalShares)


Suggested change

assetAmount := asset.Token.Amount.Mul(shareAmount).Quo(totalShares)

// lpShareEquivalentTokens = (amount in pool) * (your shares) / (total shares)

lpShareEquivalentTokens := asset.Token.Amount.Mul(shareAmount).Quo(totalShares)

ValarDragon · 2021-12-22T03:15:07Z

I intended pool destruction in order to make it able to resurrected in the future, but I think we don't need to store the actual time here. However, I agree that it does not really make sense to just pause the pool in this use case, as we completely destruct the pool and refund the whole assets. I will change it to state removal.

This still needs to be done right? (I'm still seeing the destruction time logic)

ValarDragon · 2021-12-22T03:16:26Z

x/lockup/keeper/lock.go

+func (k Keeper) GetLocksDenom(ctx sdk.Context, denom string) []types.PeriodLock {
+	unlockings := k.getLocksFromIterator(ctx, k.LockIteratorDenom(ctx, true, denom))
+	notUnlockings := k.getLocksFromIterator(ctx, k.LockIteratorDenom(ctx, false, denom))
+	return combineLocks(notUnlockings, unlockings)
+}


pretty surprised we don't already have a function for this lying around, pretty sure we use it in distribution

Hmmm... True... Did we / can we use GetLocksLongerThanDurationDenom with duration set to 0?

That worked!

ValarDragon · 2021-12-22T03:18:35Z

x/lockup/keeper/lock.go

+	// remove lock from store object
+	store := ctx.KVStore(k.storeKey)
+	store.Delete(lockStoreKey(lock.ID))
+
+	var keyprefix []byte
+	if lock.IsUnlocking() {
+		keyprefix = types.KeyPrefixUnlocking
+	} else {
+		keyprefix = types.KeyPrefixNotUnlocking
+	}
+
+	// delete lock refs from the unlocking queue
+	err = k.deleteLockRefs(ctx, keyprefix, lock)
+	if err != nil {
+		return err
+	}
+
+	// remove from accumulation store
+	for _, coin := range lock.Coins {
+		k.accumulationStore(ctx, coin.Denom).Decrease(accumulationKey(lock.Duration), coin.Amount)
+	}
+
+	k.hooks.OnTokenUnlocked(ctx, owner, lock.ID, lock.Coins, lock.Duration, lock.EndTime)
+	return nil


Can we reuse logic from the existing methods for a lock going from NotUnlocking -> Unlocking, and Unlocking -> finish unlocking?

Will do so!

Problem is that the existing method checks whether the time has been passed specified on the lock duration, and ForceUnlock need to ignore them.

Fixed by separating duration validation and actual unlocking logic

x/lockup/keeper/lock.go

mconcat · 2021-12-22T06:59:08Z

Few modification needed:

Remove pool destruction time, instead delete from state
Optimize refund logic performance
Check module account are iterated over

ValarDragon · 2021-12-22T07:13:21Z

We don't need to optimize performance for this PR, can just write an issue. (Performance gets ~basically solved if we just wait lol)

Co-authored-by: Dev Ojha <[email protected]>

…02-pool-cleanup

mconcat · 2022-01-06T18:16:41Z

Pools are now deleted when cleaned up
Reuse existing unlock methods
Module accounts are excluded
Cleanup process is batched now(single iteration over the account space)

x/gamm/keeper/pool.go

ValarDragon · 2022-01-06T20:38:15Z

x/gamm/keeper/pool.go

+	moduleAccounts := make(map[string]string)
+	for _, module := range excludedModules {
+		moduleAccounts[string(authtypes.NewModuleAddress(module))] = module
+	}


ValarDragon

Awesome, looks good to merge to me! Thanks for fixing everything and addressing comments

mconcat added 3 commits December 11, 2021 02:17

add pool cleanup

485f4b4

add pool destruction on cleanup

cc252f2

gofmt

f5b02a7

sunnya97 reviewed Dec 10, 2021

View reviewed changes

x/gamm/keeper/pool.go Outdated Show resolved Hide resolved

daniel-farina added this to the 2021 - December Milestone milestone Dec 13, 2021

mconcat and others added 3 commits December 14, 2021 15:15

Update x/gamm/keeper/pool.go

bc1859e

Co-authored-by: Sunny Aggarwal <[email protected]>

Merge branch 'main' of github.com:osmosis-labs/osmosis into mconcat/5…

1d272cd

…02-pool-cleanup

fix test

f52ab62

ValarDragon requested review from mattverse and ValarDragon December 14, 2021 06:51

mattverse reviewed Dec 15, 2021

View reviewed changes

mconcat added 2 commits December 17, 2021 15:27

add ForceUnlock

e1d9bcb

Merge branch 'mconcat/502-pool-cleanup' of github.com:osmosis-labs/os…

3b6af5c

…mosis into mconcat/502-pool-cleanup

ValarDragon reviewed Dec 21, 2021

View reviewed changes

ValarDragon reviewed Dec 22, 2021

View reviewed changes

x/lockup/keeper/lock.go Show resolved Hide resolved

move to batch processing cleanup, exclude module accounts

1436202

Update x/lockup/keeper/lock.go

57304a8

Co-authored-by: Dev Ojha <[email protected]>

daniel-farina removed this from the 2021-12 Milestone milestone Dec 24, 2021

daniel-farina mentioned this pull request Jan 4, 2022

x/GAMM: Create a function to refund all LP shares for a pool #502

Closed

ValarDragon assigned mconcat Jan 6, 2022

mconcat added 2 commits January 7, 2022 00:38

fix lock iteration and reuse unlock logic

c2e0c28

Merge branch 'main' of github.com:osmosis-labs/osmosis into mconcat/5…

0bd19df

…02-pool-cleanup

ValarDragon reviewed Jan 6, 2022

View reviewed changes

x/gamm/keeper/pool.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jan 6, 2022

View reviewed changes

x/gamm/keeper/pool.go Show resolved Hide resolved

ValarDragon reviewed Jan 6, 2022

View reviewed changes

x/gamm/keeper/pool.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jan 6, 2022

View reviewed changes

ValarDragon approved these changes Jan 6, 2022

View reviewed changes

Minor grammar fixes

3f4526f

ValarDragon merged commit ac37680 into main Jan 6, 2022

ValarDragon deleted the mconcat/502-pool-cleanup branch January 6, 2022 20:49

mattverse mentioned this pull request Jan 27, 2022

Fix test for 6.x branch to either remove or include pool cleanup #794

Closed

github-actions bot mentioned this pull request Apr 15, 2024

Broken Links Detected #8046

Closed

github-actions bot mentioned this pull request Nov 1, 2024

Broken Links Detected notional-labs/osmosis#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pool Cleanup #652

Pool Cleanup #652

mconcat commented Dec 10, 2021

ValarDragon commented Dec 14, 2021

mattverse Dec 15, 2021

mconcat Dec 16, 2021

mattverse Dec 20, 2021

mconcat Dec 21, 2021

ValarDragon Dec 21, 2021

mconcat Dec 22, 2021

mconcat Dec 22, 2021

mattverse commented Dec 15, 2021

mattverse commented Dec 15, 2021

mconcat commented Dec 16, 2021 •

edited

Loading

codecov-commenter commented Dec 17, 2021 •

edited

Loading

mconcat commented Dec 17, 2021

ValarDragon commented Dec 21, 2021

ValarDragon Dec 21, 2021 •

edited

Loading

ValarDragon Dec 21, 2021

mconcat Dec 22, 2021

ValarDragon Dec 21, 2021

ValarDragon Dec 21, 2021

ValarDragon commented Dec 22, 2021

ValarDragon Dec 22, 2021

mattverse Jan 5, 2022 •

edited

Loading

mconcat Jan 6, 2022

ValarDragon Dec 22, 2021

mconcat Dec 22, 2021

mconcat Jan 5, 2022

mconcat Jan 6, 2022

mconcat commented Dec 22, 2021

ValarDragon commented Dec 22, 2021

mconcat commented Jan 6, 2022

ValarDragon Jan 6, 2022

ValarDragon left a comment

	assetAmount := asset.Token.Amount.Mul(shareAmount).Quo(totalShares)
	// lpShareEquivalentTokens = (amount in pool) * (your shares) / (total shares)
	lpShareEquivalentTokens := asset.Token.Amount.Mul(shareAmount).Quo(totalShares)

Pool Cleanup #652

Pool Cleanup #652

Conversation

mconcat commented Dec 10, 2021

Description

ValarDragon commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse commented Dec 15, 2021

mattverse commented Dec 15, 2021

mconcat commented Dec 16, 2021 • edited Loading

codecov-commenter commented Dec 17, 2021 • edited Loading

Codecov Report

mconcat commented Dec 17, 2021

ValarDragon commented Dec 21, 2021

ValarDragon Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Dec 22, 2021

Choose a reason for hiding this comment

mattverse Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mconcat commented Dec 22, 2021

ValarDragon commented Dec 22, 2021

mconcat commented Jan 6, 2022

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

mconcat commented Dec 16, 2021 •

edited

Loading

codecov-commenter commented Dec 17, 2021 •

edited

Loading

ValarDragon Dec 21, 2021 •

edited

Loading

mattverse Jan 5, 2022 •

edited

Loading