Skip to content

Commit

Permalink
[CL Message Audit]: MsgUnlockAndMigrateSharesToFullRangeConcentratedP…
Browse files Browse the repository at this point in the history 
…osition [2/2] (#5160)

* initial push

* update readme

* initial push audit changes

* use enum for migration type

* add back proto tag

* rename to validateGammLockForSuperfluidStaking

* remove unused errors

* Update x/superfluid/keeper/migrate.go

Co-authored-by: Matt, Park <[email protected]>

* add check for both bonded and unbonding

* update gomod

* allow max length 1 for linked synth locks

* feat: partial superfluid undelegate (#5162)

* partial superfluid undelegate

* further reduction of gas

* Update x/superfluid/keeper/migrate.go

Co-authored-by: Roman <[email protected]>

* reduce code duplication lock and lockNoSend

* lockNoSend as default

* add bug fix with expanded tests

* unit test for CreateLockNoSend

* Update x/superfluid/keeper/migrate.go

Co-authored-by: Matt, Park <[email protected]>

* fix merge

* partialSuperfluidUndelegate named return values

* expand test checks for migrateDelegated

* expanded bonded checks

* check new lock and exact amount

* assign vars directly

* update merge

* check newly created lock

* add extra logic branch for fail case

* split up partial superfluid undelegate func

* expand partial undelegate test case

* roman's review

* Update x/lockup/keeper/lock_test.go

* nit

* fix test

* expand CreateLockNoSend comment

* rename lockNoSend back to lock

* expand partial unlock comment

---------

Co-authored-by: Roman <[email protected]>
Co-authored-by: Matt, Park <[email protected]>

* remove unused error

* lint

---------

Co-authored-by: Matt, Park <[email protected]>
Co-authored-by: Roman <[email protected]>
  • Loading branch information
@czarcas7ic @mattverse
@p0mvn @pysel
3 people authored and pysel committed Jun 6, 2023
1 parent edfa5b6 commit b58c732
Show file tree
Hide file tree
Showing 13 changed files with 878 additions and 401 deletions.
2 changes: 1 addition & 1 deletion tests/cl-go-client/go.mod
View file
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-13c81d83ef0d
github.com/osmosis-labs/osmosis/v15 v15.0.0-20230511223858-61e374113afc

)

Expand Down
11 changes: 3 additions & 8 deletions x/concentrated-liquidity/position.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,17 +426,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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,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)
GetLockedDenom(ctx sdk.Context, denom string, duration time.Duration) sdk.Int
}
Expand Down
40 changes: 33 additions & 7 deletions x/lockup/keeper/lock.go
View file
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.
// 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) {
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
75 changes: 66 additions & 9 deletions x/lockup/keeper/lock_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,61 @@ func (s *KeeperTestSuite) TestCreateLock() {
s.Require().Equal(sdk.NewInt(30), balance.Amount)
}

func (s *KeeperTestSuite) TestCreateLockNoSend() {
s.SetupTest()

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

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

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

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

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

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

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

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

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

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

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

func (s *KeeperTestSuite) TestAddTokensToLock() {
initialLockCoin := sdk.NewInt64Coin("stake", 10)
addr1 := sdk.AccAddress([]byte("addr1---------------"))
Expand Down Expand Up @@ -717,17 +772,17 @@ func (s *KeeperTestSuite) TestLock() {
Coins: coins,
}

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

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

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

// Since lock method no longer sends the underlying coins, the account balance should be unchanged
balance := s.App.BankKeeper.GetBalance(s.Ctx, addr1, "stake")
s.Require().Equal(sdk.ZeroInt(), balance.Amount)
s.Require().Equal(sdk.NewInt(10).String(), balance.Amount.String())

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

func (s *KeeperTestSuite) AddTokensToLockForSynth() {
Expand Down Expand Up @@ -1272,6 +1328,7 @@ func (s *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 (s *KeeperTestSuite) TestPartialForceUnlock() {
}{
{
name: "unlock full amount",
coinsToForceUnlock: sdk.Coins{sdk.NewCoin(defaultDenomToLock, defaultAmountToLock)},
coinsToForceUnlock: coinsToLock,
expectedPass: true,
},
{
Expand All @@ -1302,9 +1359,9 @@ func (s *KeeperTestSuite) TestPartialForceUnlock() {
for _, tc := range testCases {
// set up test and create default lock
s.SetupTest()
coinsToLock := sdk.NewCoins(sdk.NewCoin("stake", defaultAmountToLock))

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

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

Expand Down
30 changes: 19 additions & 11 deletions x/superfluid/keeper/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import (

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

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

var (
Expand All @@ -22,24 +22,28 @@ 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, poolIdLeaving, poolIdEntering uint64, preMigrationLock *lockuptypes.PeriodLock, lockId uint64, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, concentratedPool cltypes.ConcentratedPoolExtension, remainingLockTime time.Duration, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId uint64, err error) {
return k.migrateSuperfluidBondedBalancerToConcentrated(ctx, sender, poolIdLeaving, poolIdEntering, preMigrationLock, lockId, sharesToMigrate, synthDenomBeforeMigration, concentratedPool, remainingLockTime, tokenOutMins)
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, poolIdLeaving, poolIdEntering uint64, preMigrationLock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, synthDenomBeforeMigration string, concentratedPool cltypes.ConcentratedPoolExtension, remainingLockTime time.Duration, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId uint64, err error) {
return k.migrateSuperfluidUnbondingBalancerToConcentrated(ctx, sender, poolIdLeaving, poolIdEntering, preMigrationLock, sharesToMigrate, synthDenomBeforeMigration, concentratedPool, remainingLockTime, 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, 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, poolIdLeaving, poolIdEntering uint64, preMigrationLock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, concentratedPool cltypes.ConcentratedPoolExtension, remainingLockTime time.Duration, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, joinTime time.Time, gammLockId, concentratedLockId uint64, err error) {
return k.migrateNonSuperfluidLockBalancerToConcentrated(ctx, sender, poolIdLeaving, poolIdEntering, preMigrationLock, sharesToMigrate, concentratedPool, remainingLockTime, 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, 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) (exitCoins sdk.Coins, err error) {
return k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, lock, 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, err error) {
return k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, lock, sharesToMigrate, tokenOutMins, remainingLockTime)
}

func (k Keeper) PrepareMigration(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin) (poolIdLeaving, poolIdEntering uint64, concentratedPool cltypes.ConcentratedPoolExtension, preMigrationLock *lockuptypes.PeriodLock, remainingLockTime time.Duration, synthLockBeforeMigration []lockuptypes.SyntheticLock, isSuperfluidBonded, isSuperfluidUnbonding bool, err error) {
return k.prepareMigration(ctx, sender, lockId, sharesToMigrate)
func (k Keeper) RouteMigration(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin) (synthLocksBeforeMigration []lockuptypes.SyntheticLock, migrationType MigrationType, err error) {
return k.routeMigration(ctx, sender, lockId, sharesToMigrate)
}

func (k Keeper) ValidateMigration(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin) (poolIdLeaving, poolIdEntering uint64, preMigrationLock *lockuptypes.PeriodLock, remainingLockTime time.Duration, err error) {
return k.validateMigration(ctx, sender, lockId, sharesToMigrate)
}

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) {
Expand All @@ -53,3 +57,7 @@ func (k Keeper) ValidateGammLockForSuperfluidStaking(ctx sdk.Context, sender sdk
func (k Keeper) GetExistingLockRemainingDuration(ctx sdk.Context, lock *lockuptypes.PeriodLock) (time.Duration, error) {
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

0 comments on commit b58c732

Please sign in to comment.