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

feat: partial superfluid undelegate #5162

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
61e3741
partial superfluid undelegate
czarcas7ic May 11, 2023
ac83d9f
further reduction of gas
czarcas7ic May 12, 2023
b72ea48
Update x/superfluid/keeper/migrate.go
czarcas7ic May 12, 2023
ec3672d
Merge branch 'adam/MsgUnlockAndMigrateSharesToFullRangeConcentratedPo…
czarcas7ic May 15, 2023
749b82d
reduce code duplication lock and lockNoSend
czarcas7ic May 15, 2023
d783cd5
lockNoSend as default
czarcas7ic May 15, 2023
c7fd2bb
add bug fix with expanded tests
czarcas7ic May 15, 2023
eb8b67d
unit test for CreateLockNoSend
czarcas7ic May 15, 2023
2a619ad
Update x/superfluid/keeper/migrate.go
czarcas7ic May 15, 2023
03b1074
fix merge
czarcas7ic May 15, 2023
0d1e2c5
partialSuperfluidUndelegate named return values
czarcas7ic May 16, 2023
d71ad69
expand test checks for migrateDelegated
czarcas7ic May 16, 2023
a7cd212
expanded bonded checks
czarcas7ic May 16, 2023
c20dded
check new lock and exact amount
czarcas7ic May 16, 2023
91911c8
assign vars directly
czarcas7ic May 16, 2023
efb468c
Merge branch 'adam/MsgUnlockAndMigrateSharesToFullRangeConcentratedPo…
czarcas7ic May 16, 2023
2c96b6d
update merge
czarcas7ic May 16, 2023
1aa1f05
check newly created lock
czarcas7ic May 16, 2023
ff187c6
add extra logic branch for fail case
czarcas7ic May 16, 2023
9a91185
split up partial superfluid undelegate func
czarcas7ic May 16, 2023
83e6725
expand partial undelegate test case
czarcas7ic May 16, 2023
ead576b
roman's review
p0mvn May 19, 2023
ad73380
Update x/lockup/keeper/lock_test.go
p0mvn May 19, 2023
d596854
nit
p0mvn May 19, 2023
38ae310
Merge branch 'adam/partial-superfluid-unstake' of github.com:osmosis-…
p0mvn May 19, 2023
5ecec51
Merge branch 'adam/MsgUnlockAndMigrateSharesToFullRangeConcentratedPo…
czarcas7ic May 19, 2023
dfe204b
fix test
czarcas7ic May 19, 2023
c59c387
expand CreateLockNoSend comment
czarcas7ic May 19, 2023
d1c7b6f
rename lockNoSend back to lock
czarcas7ic May 19, 2023
315e8a0
expand partial unlock comment
czarcas7ic May 19, 2023
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
2 changes: 1 addition & 1 deletion tests/cl-go-client/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ go 1.20
require (
github.com/cosmos/cosmos-sdk v0.47.2
github.com/ignite/cli v0.23.0
github.com/osmosis-labs/osmosis/v15 v15.0.0-20230502194055-e465f0b40c14
github.com/osmosis-labs/osmosis/v15 v15.0.0-20230511223858-61e374113afc

)

Expand Down
11 changes: 3 additions & 8 deletions x/concentrated-liquidity/position.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,17 +385,12 @@ func (k Keeper) mintSharesAndLock(ctx sdk.Context, concentratedPoolId, positionI
if err != nil {
return 0, sdk.Coins{}, err
}
err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, lockuptypes.ModuleName, owner, underlyingLiquidityTokenized)
if err != nil {
return 0, sdk.Coins{}, err
}

// Lock the position for the specified duration.
// We don't need to send the coins from the owner to the lockup module account because the coins were minted directly to the module account above.
// Note, the end blocker for the lockup module contains an exception for this CL denom. When a lock with a denom of cl/pool/{poolId} is mature,
// it does not send the coins to the owner account but instead burns them.
// This is implemented in such a way to use well-tested pre-existing methods rather than
// completely re-implementing concentrated liquidity superfluid infrastructure that has a risk of introducing bugs with new logic and methods.
concentratedLock, err := k.lockupKeeper.CreateLock(ctx, owner, underlyingLiquidityTokenized, remainingLockDuration)
// it does not send the coins to the owner account and instead burns them. This is strictly to use well tested pre-existing methods rather than potentially introducing bugs with new logic and methods.
concentratedLock, err := k.lockupKeeper.CreateLockNoSend(ctx, owner, underlyingLiquidityTokenized, remainingLockDuration)
if err != nil {
return 0, sdk.Coins{}, err
}
Expand Down
1 change: 1 addition & 0 deletions x/concentrated-liquidity/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type LockupKeeper interface {
BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error)
ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error
CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (lockuptypes.PeriodLock, error)
CreateLockNoSend(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (lockuptypes.PeriodLock, error)
SlashTokensFromLockByID(ctx sdk.Context, lockID uint64, coins sdk.Coins) (*lockuptypes.PeriodLock, error)
}

Expand Down
40 changes: 33 additions & 7 deletions x/lockup/keeper/lock.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac
}

lock.Coins = lock.Coins.Add(tokensToAdd)

// Send the tokens we are about to add to lock to the lockup module account.
if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, sdk.NewCoins(tokensToAdd)); err != nil {
return nil, err
}

err = k.lock(ctx, *lock, sdk.NewCoins(tokensToAdd))
if err != nil {
return nil, err
Expand All @@ -116,10 +122,31 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac
// Returns an error in the following conditions:
// - account does not have enough balance
func (k Keeper) CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (types.PeriodLock, error) {
// Send the coins we are about to lock to the lockup module account.
if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, coins); err != nil {
return types.PeriodLock{}, err
}

// Run the createLock logic without the send since we sent the coins above.
lock, err := k.CreateLockNoSend(ctx, owner, coins, duration)
if err != nil {
return types.PeriodLock{}, err
}
return lock, nil
}

// CreateLockNoSend behaves the same as CreateLock, but does not send the coins to the lockup module account.
Copy link
Member

@p0mvn p0mvn May 19, 2023

Choose a reason for hiding this comment

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

Can you elaborate in the comment why this is needed? That is why we have CreateLockNoSend and CreateLock. I would also copy this context into CreateLock godoc

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment expanded here c59c387

// This method is used in the concentrated liquidity module since we mint coins directly to the lockup module account.
// We do not want to mint the coins to send to the user just to send them back to the lockup module account for two reasons:
// - it is gas inefficient
// - users should not be able to have cl shares in their account, so this is an extra safety measure
func (k Keeper) CreateLockNoSend(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (types.PeriodLock, error) {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
ID := k.GetLastLockID(ctx) + 1
// unlock time is initially set without a value, gets set as unlock start time + duration
// when unlocking starts.
lock := types.NewPeriodLock(ID, owner, duration, time.Time{}, coins)

// lock the coins without sending them to the lockup module account
err := k.lock(ctx, lock, lock.Coins)
if err != nil {
return lock, err
Expand All @@ -139,14 +166,13 @@ func (k Keeper) CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coin
// This is only called by either of the two possible entry points to lock tokens.
// 1. CreateLock
// 2. AddTokensToLockByID
// WARNING: this method does not send the underlying coins to the lockup module account.
// This must be done by the caller.
func (k Keeper) lock(ctx sdk.Context, lock types.PeriodLock, tokensToLock sdk.Coins) error {
owner, err := sdk.AccAddressFromBech32(lock.Owner)
if err != nil {
return err
}
if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, tokensToLock); err != nil {
return err
}

// store lock object into the store
err = k.setLock(ctx, lock)
Expand Down Expand Up @@ -218,7 +244,7 @@ func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Co
// Otherwise, split the lock into two locks, and fully unlock the newly created lock.
// (By virtue, the newly created lock we split into should have the unlock amount)
if len(coins) != 0 && !coins.IsEqual(lock.Coins) {
splitLock, err := k.splitLock(ctx, lock, coins, false)
splitLock, err := k.SplitLock(ctx, lock, coins, false)
if err != nil {
return 0, err
}
Expand Down Expand Up @@ -336,7 +362,7 @@ func (k Keeper) PartialForceUnlock(ctx sdk.Context, lock types.PeriodLock, coins
// split lock to support partial force unlock.
// (By virtue, the newly created lock we split into should have the unlock amount)
if len(coins) != 0 && !coins.IsEqual(lock.Coins) {
splitLock, err := k.splitLock(ctx, lock, coins, true)
splitLock, err := k.SplitLock(ctx, lock, coins, true)
if err != nil {
return err
}
Expand Down Expand Up @@ -750,9 +776,9 @@ func (k Keeper) deleteLock(ctx sdk.Context, id uint64) {
store.Delete(lockStoreKey(id))
}

// splitLock splits a lock with the given amount, and stores split new lock to the state.
// SplitLock splits a lock with the given amount, and stores split new lock to the state.
// Returns the new lock after modifying the state of the old lock.
func (k Keeper) splitLock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins, forceUnlock bool) (types.PeriodLock, error) {
func (k Keeper) SplitLock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins, forceUnlock bool) (types.PeriodLock, error) {
if !forceUnlock && lock.IsUnlocking() {
return types.PeriodLock{}, fmt.Errorf("cannot split unlocking lock")
}
Expand Down
77 changes: 67 additions & 10 deletions x/lockup/keeper/lock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,61 @@ func (suite *KeeperTestSuite) TestCreateLock() {
suite.Require().Equal(sdk.NewInt(30), balance.Amount)
}

func (suite *KeeperTestSuite) TestCreateLockNoSend() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason you made these non table driven?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the pre-existing TestCreateLock was not table driven, and I wanted to use the exact logic flow to ensure this method behaves just like CreateLock, just without the send

suite.SetupTest()

addr1 := sdk.AccAddress([]byte("addr1---------------"))
coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)}

// test locking without balance
lock, err := suite.App.LockupKeeper.CreateLockNoSend(suite.Ctx, addr1, coins, time.Second)
suite.Require().NoError(err)

// check new lock
suite.Require().Equal(coins, lock.Coins)
suite.Require().Equal(time.Second, lock.Duration)
suite.Require().Equal(time.Time{}, lock.EndTime)
suite.Require().Equal(uint64(1), lock.ID)

lockID := suite.App.LockupKeeper.GetLastLockID(suite.Ctx)
suite.Require().Equal(uint64(1), lockID)

// check accumulation store
accum := suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{
LockQueryType: types.ByDuration,
Denom: "stake",
Duration: time.Second,
})
suite.Require().Equal(accum.String(), "10")

// create new lock (this time with a balance)
originalLockBalance := int64(20)
coins = sdk.Coins{sdk.NewInt64Coin("stake", originalLockBalance)}
suite.FundAcc(addr1, coins)

lock, err = suite.App.LockupKeeper.CreateLockNoSend(suite.Ctx, addr1, coins, time.Second)
suite.Require().NoError(err)

lockID = suite.App.LockupKeeper.GetLastLockID(suite.Ctx)
suite.Require().Equal(uint64(2), lockID)

// check accumulation store
accum = suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{
LockQueryType: types.ByDuration,
Denom: "stake",
Duration: time.Second,
})
suite.Require().Equal(accum.String(), "30")

// check that send did not occur and balances are unchanged
balance := suite.App.BankKeeper.GetBalance(suite.Ctx, addr1, "stake")
suite.Require().Equal(sdk.NewInt(originalLockBalance).String(), balance.Amount.String())

acc := suite.App.AccountKeeper.GetModuleAccount(suite.Ctx, types.ModuleName)
balance = suite.App.BankKeeper.GetBalance(suite.Ctx, acc.GetAddress(), "stake")
suite.Require().Equal(sdk.ZeroInt().String(), balance.Amount.String())
}

func (suite *KeeperTestSuite) TestAddTokensToLock() {
initialLockCoin := sdk.NewInt64Coin("stake", 10)
addr1 := sdk.AccAddress([]byte("addr1---------------"))
Expand Down Expand Up @@ -703,7 +758,7 @@ func (suite *KeeperTestSuite) TestHasLock() {
}
}

func (suite *KeeperTestSuite) TestLock() {
func (suite *KeeperTestSuite) TestLockNoSend() {
suite.SetupTest()

addr1 := sdk.AccAddress([]byte("addr1---------------"))
Expand All @@ -717,17 +772,17 @@ func (suite *KeeperTestSuite) TestLock() {
Coins: coins,
}

// test locking without balance
// test locking without balance (should work since we don't send the underlying balance)
err := suite.App.LockupKeeper.Lock(suite.Ctx, lock, coins)
suite.Require().Error(err)
suite.Require().NoError(err)

// check accumulation store
accum := suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{
LockQueryType: types.ByDuration,
Denom: "stake",
Duration: time.Second,
})
suite.Require().Equal(accum.String(), "0")
suite.Require().Equal(accum.String(), "10")

suite.FundAcc(addr1, coins)
err = suite.App.LockupKeeper.Lock(suite.Ctx, lock, coins)
Expand All @@ -739,14 +794,15 @@ func (suite *KeeperTestSuite) TestLock() {
Denom: "stake",
Duration: time.Second,
})
suite.Require().Equal(accum.String(), "10")
suite.Require().Equal(accum.String(), "20")

// Since lockNoSend does not send the underlying coins, the account balance should be unchanged
balance := suite.App.BankKeeper.GetBalance(suite.Ctx, addr1, "stake")
suite.Require().Equal(sdk.ZeroInt(), balance.Amount)
suite.Require().Equal(sdk.NewInt(10).String(), balance.Amount.String())

acc := suite.App.AccountKeeper.GetModuleAccount(suite.Ctx, types.ModuleName)
balance = suite.App.BankKeeper.GetBalance(suite.Ctx, acc.GetAddress(), "stake")
suite.Require().Equal(sdk.NewInt(10), balance.Amount)
suite.Require().Equal(sdk.NewInt(0).String(), balance.Amount.String())
}

func (suite *KeeperTestSuite) AddTokensToLockForSynth() {
Expand Down Expand Up @@ -1272,6 +1328,7 @@ func (suite *KeeperTestSuite) TestPartialForceUnlock() {

defaultDenomToLock := "stake"
defaultAmountToLock := sdk.NewInt(10000000)
coinsToLock := sdk.NewCoins(sdk.NewCoin("stake", defaultAmountToLock))

testCases := []struct {
name string
Expand All @@ -1280,7 +1337,7 @@ func (suite *KeeperTestSuite) TestPartialForceUnlock() {
}{
{
name: "unlock full amount",
coinsToForceUnlock: sdk.Coins{sdk.NewCoin(defaultDenomToLock, defaultAmountToLock)},
coinsToForceUnlock: coinsToLock,
expectedPass: true,
},
{
Expand All @@ -1302,9 +1359,9 @@ func (suite *KeeperTestSuite) TestPartialForceUnlock() {
for _, tc := range testCases {
// set up test and create default lock
suite.SetupTest()
coinsToLock := sdk.NewCoins(sdk.NewCoin("stake", defaultAmountToLock))

suite.FundAcc(addr1, sdk.NewCoins(coinsToLock...))
// balanceBeforeLock := suite.App.BankKeeper.GetAllBalances(suite.Ctx, addr1)

lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coinsToLock, time.Minute)
suite.Require().NoError(err)

Expand Down
21 changes: 17 additions & 4 deletions x/superfluid/keeper/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

lockuptypes "github.com/osmosis-labs/osmosis/v15/x/lockup/types"
"github.com/osmosis-labs/osmosis/v15/x/superfluid/types"
)

var (
Expand All @@ -21,19 +22,19 @@ func (k Keeper) PrepareConcentratedLockForSlash(ctx sdk.Context, lock *lockuptyp
return k.prepareConcentratedLockForSlash(ctx, lock, slashAmt)
}

func (k Keeper) MigrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
func (k Keeper) MigrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
return k.migrateSuperfluidBondedBalancerToConcentrated(ctx, sender, lockId, sharesToMigrate, synthDenomBeforeMigration, tokenOutMins)
}

func (k Keeper) MigrateSuperfluidUnbondingBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
func (k Keeper) MigrateSuperfluidUnbondingBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
return k.migrateSuperfluidUnbondingBalancerToConcentrated(ctx, sender, lockId, sharesToMigrate, synthDenomBeforeMigration, tokenOutMins)
}

func (k Keeper) MigrateNonSuperfluidLockBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
func (k Keeper) MigrateNonSuperfluidLockBalancerToConcentrated(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, concentratedLockId, poolIdLeaving, poolIdEntering uint64, err error) {
return k.migrateNonSuperfluidLockBalancerToConcentrated(ctx, sender, lockId, sharesToMigrate, tokenOutMins)
}

func (k Keeper) ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins, remainingLockTime time.Duration) (exitCoins sdk.Coins, remainingSharesLock lockuptypes.PeriodLock, err error) {
func (k Keeper) ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins, remainingLockTime time.Duration) (exitCoins sdk.Coins, err error) {
return k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, lock, sharesToMigrate, tokenOutMins, remainingLockTime)
}

Expand All @@ -48,3 +49,15 @@ func (k Keeper) ValidateMigration(ctx sdk.Context, sender sdk.AccAddress, lockId
func (k Keeper) AddToConcentratedLiquiditySuperfluidPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added sdk.Int) (uint64, sdk.Int, sdk.Int, sdk.Dec, uint64, error) {
return k.addToConcentratedLiquiditySuperfluidPosition(ctx, owner, positionId, amount0Added, amount1Added)
}

func (k Keeper) ValidateGammLockForSuperfluidStaking(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, lockId uint64) (*lockuptypes.PeriodLock, error) {
return k.validateGammLockForSuperfluidStaking(ctx, sender, poolId, lockId)
}

func (k Keeper) GetExistingLockRemainingDuration(ctx sdk.Context, lock *lockuptypes.PeriodLock) time.Duration {
return k.getExistingLockRemainingDuration(ctx, lock)
}

func (k Keeper) PartialSuperfluidUndelegateToConcentratedPosition(ctx sdk.Context, sender string, lockID uint64, amountToUndelegate sdk.Coin) (intermediaryAcc types.SuperfluidIntermediaryAccount, newlock *lockuptypes.PeriodLock, err error) {
return k.partialSuperfluidUndelegateToConcentratedPosition(ctx, sender, lockID, amountToUndelegate)
}
Loading