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

[CL][bugfix] Update reward splitting logic to only use bonded balancer amounts #5239

Merged
merged 3 commits into from
May 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#5129](https://github.com/osmosis-labs/osmosis/pull/5129) Relax twap record validation in init genesis to allow one of the spot prices to be non-zero when twap error is observed.
* [#5165](https://github.com/osmosis-labs/osmosis/pull/5165) Improve error message when fully exiting from a pool.
* [#5187](https://github.com/osmosis-labs/osmosis/pull/5187) Expand `IncentivizedPools` query to include internally incentivized CL pools.
* [#5239](https://github.com/osmosis-labs/osmosis/pull/5239) Implement `GetTotalPoolShares` public keeper function for GAMM.

### API breaks

Expand Down
29 changes: 27 additions & 2 deletions x/concentrated-liquidity/incentives.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/math"
"github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/model"
"github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types"
gammtypes "github.com/osmosis-labs/osmosis/v15/x/gamm/types"
)

// createUptimeAccumulators creates accumulator objects in store for each supported uptime for the given poolId.
Expand Down Expand Up @@ -126,12 +127,36 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64)
return 0, sdk.ZeroDec(), nil
}

// Get Balancer pool liquidity
balancerPoolLiquidity, err := k.gammKeeper.GetTotalPoolLiquidity(ctx, canonicalBalancerPoolId)
// Get total balancer pool liquidity (denominated in pool coins)
totalBalancerPoolLiquidity, err := k.gammKeeper.GetTotalPoolLiquidity(ctx, canonicalBalancerPoolId)
if err != nil {
return 0, sdk.ZeroDec(), err
}

// Get total balancer shares for Balancer pool
totalBalancerPoolShares, err := k.gammKeeper.GetTotalPoolShares(ctx, canonicalBalancerPoolId)
if err != nil {
return 0, sdk.ZeroDec(), err
}

// Get total shares bonded on the longest lockup duration for Balancer pool
longestDuration, err := k.poolIncentivesKeeper.GetLongestLockableDuration(ctx)
if err != nil {
return 0, sdk.ZeroDec(), err
}
bondedShares := k.lockupKeeper.GetLockedDenom(ctx, gammtypes.GetPoolShareDenom(canonicalBalancerPoolId), longestDuration)

// Calculate portion of Balancer pool shares that are bonded
bondedShareRatio := bondedShares.ToDec().Quo(totalBalancerPoolShares.ToDec())

// Calculate rough number of assets in Balancer pool that are bonded
balancerPoolLiquidity := sdk.NewCoins()
for _, liquidityToken := range totalBalancerPoolLiquidity {
// Rounding behavior is not critical here, but for simplicity we do bankers multiplication then truncate.
bondedLiquidityAmount := liquidityToken.Amount.ToDec().Mul(bondedShareRatio).TruncateInt()
balancerPoolLiquidity = balancerPoolLiquidity.Add(sdk.NewCoin(liquidityToken.Denom, bondedLiquidityAmount))
}

// Validate Balancer pool liquidity. These properties should already be guaranteed by the caller,
// but we check them anyway as an additional guardrail in case migration link validation is ever
// relaxed in the future.
Expand Down
57 changes: 54 additions & 3 deletions x/concentrated-liquidity/incentives_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -909,7 +909,13 @@ func (s *KeeperTestSuite) TestUpdateUptimeAccumulatorsToNow() {
// If applicable, create and link a canonical balancer pool
balancerPoolId := uint64(0)
if tc.canonicalBalancerPoolAssets != nil {
// Create balancer pool and bond its shares
balancerPoolId = s.PrepareCustomBalancerPool(tc.canonicalBalancerPoolAssets, defaultBalancerPoolParams)
longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx)
s.Require().NoError(err)
_, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), gammtypes.InitPoolSharesSupply)), longestDuration)
s.Require().NoError(err)

s.App.GAMMKeeper.OverwriteMigrationRecords(s.Ctx,
gammtypes.MigrationRecords{
BalancerToConcentratedPoolLinks: []gammtypes.BalancerToConcentratedPoolLink{
Expand Down Expand Up @@ -3793,6 +3799,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
tests := map[string]struct {
existingConcentratedLiquidity sdk.Coins
balancerPoolAssets []balancer.PoolAsset
portionOfSharesBonded sdk.Dec

noCanonicalBalancerPool bool
noBalancerPoolWithID bool
Expand All @@ -3806,6 +3813,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
// 100 existing shares and 100 shares added from balancer
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: defaultBalancerAssets,
portionOfSharesBonded: sdk.NewDec(1),
},
"same spot price, different total share amount": {
// 100 existing shares and 200 shares added from balancer
Expand All @@ -3814,6 +3822,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(200))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(200))},
},
portionOfSharesBonded: sdk.NewDec(1),
},
"different spot price between balancer and CL pools (excess asset0)": {
// 100 existing shares and 100 shares added from balancer. We expect only the even portion of
Expand All @@ -3823,6 +3832,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(150))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))},
},
portionOfSharesBonded: sdk.NewDec(1),
},
"different spot price between balancer and CL pools (excess asset1)": {
// 100 existing shares and 100 shares added from balancer. We expect only the even portion of
Expand All @@ -3832,11 +3842,32 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(150))},
},
portionOfSharesBonded: sdk.NewDec(1),
},
"same spot price, different total share amount, only half bonded": {
// 100 existing shares and 200 shares added from balancer
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: []balancer.PoolAsset{
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(200))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(200))},
},
portionOfSharesBonded: sdk.MustNewDecFromStr("0.5"),
},
"different spot price between balancer and CL pools (excess asset1), only partially bonded": {
// 100 existing shares and 100 shares added from balancer. We expect only the even portion of
// the Balancer pool to be joined, with the remaining 50bar not qualifying.
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: []balancer.PoolAsset{
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(150))},
},
portionOfSharesBonded: sdk.MustNewDecFromStr("0.1"),
},
"no canonical balancer pool": {
// 100 existing shares and 100 shares added from balancer
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: defaultBalancerAssets,
portionOfSharesBonded: sdk.NewDec(1),

// Note that we expect this to fail quietly, as most CL pools will not have linked Balancer pools
noCanonicalBalancerPool: true,
Expand All @@ -3848,6 +3879,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
// 100 existing shares and 100 shares added from balancer
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: defaultBalancerAssets,
portionOfSharesBonded: sdk.NewDec(1),
noBalancerPoolWithID: true,

expectedError: gammtypes.PoolDoesNotExistError{PoolId: invalidPoolId},
Expand All @@ -3859,6 +3891,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))},
},
portionOfSharesBonded: sdk.NewDec(1),
invalidBalancerPoolLiquidity: true,

expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("invalid", sdk.NewInt(100)), sdk.NewCoin("bar", sdk.NewInt(100)))},
Expand All @@ -3870,6 +3903,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid", sdk.NewInt(100))},
},
portionOfSharesBonded: sdk.NewDec(1),
invalidBalancerPoolLiquidity: true,

expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)), sdk.NewCoin("invalid", sdk.NewInt(100)))},
Expand All @@ -3881,6 +3915,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid1", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid2", sdk.NewInt(100))},
},
portionOfSharesBonded: sdk.NewDec(1),
invalidBalancerPoolLiquidity: true,

expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("invalid1", sdk.NewInt(100)), sdk.NewCoin("invalid2", sdk.NewInt(100)))},
Expand All @@ -3893,6 +3928,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))},
{Weight: sdk.NewInt(1), Token: sdk.NewCoin("baz", sdk.NewInt(100))},
},
portionOfSharesBonded: sdk.NewDec(1),
invalidBalancerPoolLiquidity: true,

expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)), sdk.NewCoin("bar", sdk.NewInt(100)), sdk.NewCoin("baz", sdk.NewInt(100)))},
Expand All @@ -3901,6 +3937,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
// 100 existing shares and 100 shares added from balancer
existingConcentratedLiquidity: defaultConcentratedAssets,
balancerPoolAssets: defaultBalancerAssets,
portionOfSharesBonded: sdk.NewDec(1),
invalidConcentratedPoolID: true,

expectedError: types.PoolNotFoundError{PoolId: invalidPoolId + 1},
Expand All @@ -3918,6 +3955,14 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {

// If a canonical balancer pool exists, we create it and link it with the CL pool
balancerPoolId := s.PrepareCustomBalancerPool(tc.balancerPoolAssets, defaultBalancerPoolParams)

// Bond the appropriate portion of total Balancer shares as defined by the current test case
longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx)
s.Require().NoError(err)
bondedShares := gammtypes.InitPoolSharesSupply.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt()
_, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), bondedShares)), longestDuration)
s.Require().NoError(err)

if tc.noBalancerPoolWithID {
balancerPoolId = invalidPoolId
} else if tc.noCanonicalBalancerPool {
Expand All @@ -3937,7 +3982,9 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
// Calculate balancer share amount for full range
updatedClPool, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, clPool.GetId())
s.Require().NoError(err)
qualifyingSharesPreDiscount := math.GetLiquidityFromAmounts(updatedClPool.GetCurrentSqrtPrice(), types.MinSqrtPrice, types.MaxSqrtPrice, tc.balancerPoolAssets[1].Token.Amount, tc.balancerPoolAssets[0].Token.Amount)
asset0BalancerAmount := tc.balancerPoolAssets[0].Token.Amount.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt()
asset1BalancerAmount := tc.balancerPoolAssets[1].Token.Amount.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt()
qualifyingSharesPreDiscount := math.GetLiquidityFromAmounts(updatedClPool.GetCurrentSqrtPrice(), types.MinSqrtPrice, types.MaxSqrtPrice, asset1BalancerAmount, asset0BalancerAmount)
qualifyingShares := (sdk.OneDec().Sub(types.DefaultBalancerSharesDiscount)).Mul(qualifyingSharesPreDiscount)

clearOutQualifyingShares := tc.noBalancerPoolWithID || tc.invalidBalancerPoolLiquidity || tc.invalidConcentratedPoolID || tc.invalidBalancerPoolID || tc.noCanonicalBalancerPool
Expand Down Expand Up @@ -4124,8 +4171,12 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() {
// Note that the second return value here is the position ID, not an error.
initialLiquidity, _ := s.SetupPosition(clPoolId, s.TestAccs[0], tc.existingConcentratedLiquidity, DefaultMinTick, DefaultMaxTick, s.Ctx.BlockTime())

// Create balancer pool to be linked with CL pool in happy path cases
// Create and bond shares for balancer pool to be linked with CL pool in happy path cases
balancerPoolId := s.PrepareCustomBalancerPool(tc.balancerPoolAssets, defaultBalancerPoolParams)
longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx)
s.Require().NoError(err)
_, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), gammtypes.InitPoolSharesSupply)), longestDuration)
s.Require().NoError(err)

// Invalidate pool IDs if needed for error cases
if tc.balancerPoolDoesNotExist {
Expand Down Expand Up @@ -4165,7 +4216,7 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() {
s.FundAcc(clPool.GetIncentivesAddress(), normalizedEmissions)
}
}
err := addToUptimeAccums(s.Ctx, clPool.GetId(), s.App.ConcentratedLiquidityKeeper, tc.uptimeGrowth)
err = addToUptimeAccums(s.Ctx, clPool.GetId(), s.App.ConcentratedLiquidityKeeper, tc.uptimeGrowth)
s.Require().NoError(err)

// --- System under test ---
Expand Down
2 changes: 2 additions & 0 deletions x/concentrated-liquidity/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type PoolManagerKeeper interface {
type GAMMKeeper interface {
GetTotalPoolLiquidity(ctx sdk.Context, poolId uint64) (sdk.Coins, error)
GetLinkedBalancerPoolID(ctx sdk.Context, poolIdEntering uint64) (uint64, error)
GetTotalPoolShares(ctx sdk.Context, poolId uint64) (sdk.Int, error)
}

type PoolIncentivesKeeper interface {
Expand All @@ -58,6 +59,7 @@ type LockupKeeper interface {
ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error
CreateLock(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
}

// CommunityPoolKeeper defines the contract needed to be fulfilled for distribution keeper.
Expand Down
10 changes: 10 additions & 0 deletions x/gamm/keeper/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,16 @@ func (k Keeper) GetTotalPoolLiquidity(ctx sdk.Context, poolId uint64) (sdk.Coins
return pool.GetTotalPoolLiquidity(ctx), nil
}

// GetTotalPoolShares returns the total number of pool shares for the given pool.
func (k Keeper) GetTotalPoolShares(ctx sdk.Context, poolId uint64) (sdk.Int, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: implementing this helper was necessary to avoid import cycles that came up when interacting with lower levels of GAMM abstractions

pool, err := k.GetCFMMPool(ctx, poolId)
if err != nil {
return sdk.Int{}, err
}

return pool.GetTotalShares(), nil
}

// setStableSwapScalingFactors sets the stable swap scaling factors.
// errors if the pool does not exist, the sender is not the scaling factor controller, or due to other
// internal errors.
Expand Down
60 changes: 60 additions & 0 deletions x/gamm/keeper/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -517,3 +517,63 @@ func (s *KeeperTestSuite) TestSetStableSwapScalingFactors() {
})
}
}

func (suite *KeeperTestSuite) TestGetTotalPoolShares() {
tests := map[string]struct {
sharesJoined sdk.Int
poolNotCreated bool

expectedError error
}{
"happy path: default balancer pool": {
sharesJoined: sdk.ZeroInt(),
},
"Multiple LPs with shares exist": {
sharesJoined: types.OneShare,
},
"error: pool does not exist": {
sharesJoined: sdk.ZeroInt(),
poolNotCreated: true,
expectedError: types.PoolDoesNotExistError{PoolId: uint64(0)},
},
}

for name, tc := range tests {
suite.Run(name, func() {
suite.SetupTest()
gammKeeper := suite.App.GAMMKeeper
testAccount := suite.TestAccs[0]

// --- Setup ---

// Mint some assets to the accounts.
balancerPoolId := uint64(0)
if !tc.poolNotCreated {
balancerPoolId = suite.PrepareBalancerPool()
}

sharesJoined := sdk.ZeroInt()
if !tc.sharesJoined.Equal(sdk.ZeroInt()) {
suite.FundAcc(testAccount, defaultAcctFunds)
_, sharesActualJoined, err := gammKeeper.JoinPoolNoSwap(suite.Ctx, testAccount, balancerPoolId, tc.sharesJoined, sdk.Coins{})
suite.Require().NoError(err)
sharesJoined = sharesActualJoined
}

// --- System under test ---

totalShares, err := gammKeeper.GetTotalPoolShares(suite.Ctx, balancerPoolId)

// --- Assertions ---

if tc.expectedError != nil {
suite.Require().Error(err)
suite.Require().ErrorContains(err, tc.expectedError.Error())
return
}

suite.Require().NoError(err)
suite.Require().Equal(types.InitPoolSharesSupply.Add(sharesJoined), totalShares)
})
}
}