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

[CL][Incentives Query][Bug]: Implement support for CL gauges to incentivized pools query #5187

Merged
merged 4 commits into from
May 19, 2023
Merged

[CL][Incentives Query][Bug]: Implement support for CL gauges to incentivized pools query #5187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cf272d
implement and test query support for CL gauges
AlpinYukseloglu May 15, 2023
6be66e2
update changelog
AlpinYukseloglu May 19, 2023
80fcad3
Merge branch 'main' into alpo/fix-incentive-query
AlpinYukseloglu May 19, 2023
c292794
fix conflicts
AlpinYukseloglu 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#2741](https://github.com/osmosis-labs/osmosis/pull/2741) Prevent updating the twap record if `ctx.BlockTime <= record.Time` or `ctx.BlockHeight <= record.Height`. Exception, can update the record created when creating the pool in the same block.
* [#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.

### API breaks

Expand Down
17 changes: 17 additions & 0 deletions x/pool-incentives/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@ func (q Querier) IncentivizedPools(ctx context.Context, _ *types.QueryIncentiviz
lockableDurations := q.Keeper.GetLockableDurations(sdkCtx)
distrInfo := q.Keeper.GetDistrInfo(sdkCtx)

// Add epoch duration to lockable durations if not already present.
// This is to ensure that concentrated gauges (which run on epoch time) are
// always included in the query, even if the epoch duration changes in the future.
epochDuration := q.incentivesKeeper.GetEpochInfo(sdkCtx).Duration
epochAlreadyLockable := false
for _, lockableDuration := range lockableDurations {
if lockableDuration == epochDuration {
epochAlreadyLockable = true
break
}
}

// Ensure that we only add epoch duration if it does not already exist as a lockable duration.
if !epochAlreadyLockable {
lockableDurations = append(lockableDurations, epochDuration)
}
Comment on lines +90 to +105
Copy link
Member

Choose a reason for hiding this comment

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

On mainnet, epoch duration is 1 day, and we also have 1-day uptime. How are we going to differentiate between internal per-epoch gauges and 1 day uptime gauges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to differentiate, right? My understanding is that the goal of the query is to get all the incentivized pools, so as long as the 1-day period is included in the list then all CL pools will be part of the query response.

This section simply ensures that if the epoch duration already exists as a lockup duration (e.g. as on mainnet), then it does not add it as a duplicate.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, from @jonator

Yeah we need to make sure it only refers to pools and gauges that are internally incentivized via mint incentives

For external incentives, we handle those in the opposite direction: by querying the gauges and linking those to pools

(Which, a separate topic, causes a lot of performance issues)

Like in hindsight, we could have reduced complexity by having the node return all gauges for a pool, internal and external

Comment on lines +93 to +105
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
epochDuration := q.incentivesKeeper.GetEpochInfo(sdkCtx).Duration
epochAlreadyLockable := false
for _, lockableDuration := range lockableDurations {
if lockableDuration == epochDuration {
epochAlreadyLockable = true
break
}
}
// Ensure that we only add epoch duration if it does not already exist as a lockable duration.
if !epochAlreadyLockable {
lockableDurations = append(lockableDurations, epochDuration)
}
epochDuration := q.incentivesKeeper.GetEpochInfo(sdkCtx).Duration
for _, lockableDuration := range lockableDurations {
if lockableDuration == epochDuration {
lockableDurations = append(lockableDurations, epochDuration)
break
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change seems to add epoch time to lockable durations only if it already exists in there - we want the exact opposite no?


// While there are exceptions, typically the number of incentivizedPools
// equals to the number of incentivized gauges / number of lockable durations.
incentivizedPools := make([]types.IncentivizedPool, 0, len(distrInfo.Records)/len(lockableDurations))
Expand Down
25 changes: 25 additions & 0 deletions x/pool-incentives/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ func (s *KeeperTestSuite) TestIncentivizedPools() {
desc string
poolCreated bool
weights []sdk.Int
clPoolWithGauge bool
clGaugeWeight sdk.Int
perpetual bool
nonPerpetual bool
expectedRecordLength int
Expand Down Expand Up @@ -189,6 +191,14 @@ func (s *KeeperTestSuite) TestIncentivizedPools() {
nonPerpetual: true,
expectedRecordLength: 0,
},
{
desc: "Concentrated case",
poolCreated: true,
clPoolWithGauge: true,
clGaugeWeight: sdk.NewInt(400),
weights: []sdk.Int{sdk.NewInt(100), sdk.NewInt(200), sdk.NewInt(300)},
expectedRecordLength: 4,
},
} {
tc := tc
s.Run(tc.desc, func() {
Expand All @@ -206,6 +216,21 @@ func (s *KeeperTestSuite) TestIncentivizedPools() {
s.Require().Equal(3, len(lockableDurations))

var distRecords []types.DistrRecord

// If appropriate, create a concentrated pool with a gauge.
// Recall that concentrated pool gauges are created on epoch duration, which
// is set in default genesis to be 168 hours (1 week). Since this is not included
// in the set of lockable durations, creating this gauge serves as a way to ensure
// CL gauges are captured by the IncentivizedPools query.
if tc.clPoolWithGauge {
clPool := s.PrepareConcentratedPool()
epochDuration := s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration

clGaugeId, err := keeper.GetPoolGaugeId(s.Ctx, clPool.GetId(), epochDuration)
s.Require().NoError(err)
distRecords = append(distRecords, types.DistrRecord{GaugeId: clGaugeId, Weight: tc.clGaugeWeight})
}

for i := 0; i < len(lockableDurations); i++ {
gaugeId, err := keeper.GetPoolGaugeId(s.Ctx, poolId, lockableDurations[i])
s.Require().NoError(err)
Expand Down