fix: add indirect incentivized pools to incentivized pools query #5589

czarcas7ic · 2023-06-20T17:51:31Z

Closes: #XXX

What is the purpose of the change

When we link a concentrated pool to a balancer pool, the concentrated pool's gauge ID completely replaces the balancer pool's gauge ID in the incentivized gauges list. This is because all incentives for that denom pair is sent to the CL pool, the balancer pool determines it's share of the rewards via pseudo full range creation and taking into account a discount factor, and the CL pool sends these funds to the balancer pool's gauge for distribution to it's LPs.

The pool is still incentivized, but in an indirect way. We must therefore include these gauges still in the incentivized pools query, despite not being incentivized directly via inflation.

Testing and Verifying

Incentivized pools query tested via gotest

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

czarcas7ic · 2023-06-20T20:15:38Z

proto/osmosis/pool-incentives/v1beta1/genesis.proto

+
+// MigrationRecords contains all the links between balancer and concentrated
+// pools.
+//
+// This is copied over from the gamm proto file in order to circumnavigate
+// the circular dependency between the two modules.
+message MigrationRecords {
+  repeated BalancerToConcentratedPoolLink balancer_to_concentrated_pool_links =
+      1 [ (gogoproto.nullable) = false ];
+}
+
+// BalancerToConcentratedPoolLink defines a single link between a single
+// balancer pool and a single concentrated liquidity pool. This link is used to
+// allow a balancer pool to migrate to a single canonical full range
+// concentrated liquidity pool position
+// A balancer pool can be linked to a maximum of one cl pool, and a cl pool can
+// be linked to a maximum of one balancer pool.
+//
+// This is copied over from the gamm proto file in order to circumnavigate
+// the circular dependency between the two modules.
+message BalancerToConcentratedPoolLink {
+  option (gogoproto.equal) = true;
+  uint64 balancer_pool_id = 1;
+  uint64 cl_pool_id = 2;
+}


This was sadly the cleanest way knew to get passed the circular dependency issue. The only module that pool-incentives had access to that could access gamm's store was superfluid. So I basically called GetAllMigrationInfo in pool-incentives via superfluid and parsed the result into an identical struct. Please lmk if there is a better way to achieve this without a major refactor.

this might sound a bit unorthodox but how about we create a separate file that store shared interfaces between modules

for instance;

gamm

shared_keepers.go (includes PoolIncentiveKeeper)

pool-incentives

shared_keepers.go (includes GammKeeper)

I agree with this! I think it makes it simpler to reason about when we come back around and resolve this with the solution Roman recommended. Added here e70ac41

p0mvn · 2023-06-20T20:56:18Z

x/pool-incentives/keeper/grpc_query.go

+		// If the linked concentrated liquidity pool is in the list of incentivized pools,
+		// note this and add its balancer counterpart to the list of incentivized pools.
+		for _, incentivizedPool := range incentivizedPools {
+			if incentivizedPool.PoolId == record.ClPoolId {
+				linkedClPoolIsIncentivized = true
+				duration = incentivizedPool.LockableDuration
+				continue
+			}
+		}


Consider creating a map[uint64]struct{} of incentivized pool IDs in the first loop above. This would allow checking the map here in O(1) instead of repeatedly iterating all incentivized pools for every link

Good call, added here bba673d

p0mvn · 2023-06-20T20:58:32Z

x/pool-incentives/keeper/grpc_query.go

+		for _, incentivizedPool := range incentivizedPools {
+			if incentivizedPool.PoolId == record.ClPoolId {
+				linkedClPoolIsIncentivized = true
+				duration = incentivizedPool.LockableDuration


Is it correct to use this for duration?

AFAIR, we update the largest lockable duration gauge for the balancer pool while using incentivized epoch duration for CL pool which might not necessarily match

Its just a holder value, but we can set this as the longest duration just in case, because I agree that is "default"

Done here 18ee857

stackman27 · 2023-06-20T20:29:23Z

x/pool-incentives/keeper/grpc_query.go

+		gaugeId, err := q.Keeper.GetPoolGaugeId(sdkCtx, record.BalancerPoolId, duration)
+		if err == nil {
+			incentivizedPool := types.IncentivizedPool{
+				PoolId:           record.BalancerPoolId,


do we never expect PoolId to be a CL pool Id?

In every case the balancer pool will be the pool that is getting indirectly incentivized. This means that there is never a case in which a CL pool does not appear in the incentivized pools list that should be there. That is why we only retrieve the balancer pools at the end, since they are the ones that are getting replaced and funded via the CL gauge

stackman27 · 2023-06-20T21:17:00Z

proto/osmosis/pool-incentives/v1beta1/genesis.proto

+
+// MigrationRecords contains all the links between balancer and concentrated
+// pools.
+//
+// This is copied over from the gamm proto file in order to circumnavigate
+// the circular dependency between the two modules.
+message MigrationRecords {
+  repeated BalancerToConcentratedPoolLink balancer_to_concentrated_pool_links =
+      1 [ (gogoproto.nullable) = false ];
+}
+
+// BalancerToConcentratedPoolLink defines a single link between a single
+// balancer pool and a single concentrated liquidity pool. This link is used to
+// allow a balancer pool to migrate to a single canonical full range
+// concentrated liquidity pool position
+// A balancer pool can be linked to a maximum of one cl pool, and a cl pool can
+// be linked to a maximum of one balancer pool.
+//
+// This is copied over from the gamm proto file in order to circumnavigate
+// the circular dependency between the two modules.
+message BalancerToConcentratedPoolLink {
+  option (gogoproto.equal) = true;
+  uint64 balancer_pool_id = 1;
+  uint64 cl_pool_id = 2;
+}


this might sound a bit unorthodox but how about we create a separate file that store shared interfaces between modules

for instance;

gamm

shared_keepers.go (includes PoolIncentiveKeeper)

pool-incentives

shared_keepers.go (includes GammKeeper)

p0mvn

LGTM once minor suggestions are addressed

p0mvn · 2023-06-21T15:56:44Z

proto/osmosis/gamm/v1beta1/shared.proto

+// MigrationRecords contains all the links between balancer and concentrated
+// pools
+message MigrationRecords {
+  repeated BalancerToConcentratedPoolLink balancer_to_concentrated_pool_links =
+      1 [ (gogoproto.nullable) = false ];
+}
+
+// BalancerToConcentratedPoolLink defines a single link between a single
+// balancer pool and a single concentrated liquidity pool. This link is used to
+// allow a balancer pool to migrate to a single canonical full range
+// concentrated liquidity pool position
+// A balancer pool can be linked to a maximum of one cl pool, and a cl pool can
+// be linked to a maximum of one balancer pool.
+message BalancerToConcentratedPoolLink {
+  option (gogoproto.equal) = true;
+  uint64 balancer_pool_id = 1;
+  uint64 cl_pool_id = 2;
+}


Can we make an issue to track unifying this?

I know we discussed this, and I'm not intending to block on this but it really feels like less overall overhead if we just unified it in this PR. This should be faster to do than writing up a good issue

Discussed offline after leaving this comment and made a PR which ended up being quick: #5596

Happy to deal with #5596 separately

x/pool-incentives/keeper/grpc_query.go

p0mvn · 2023-06-21T16:03:27Z

x/pool-incentives/keeper/grpc_query.go

+			for _, pool := range incentivizedPools {
+				if pool.PoolId == record.ClPoolId {
+					duration = pool.LockableDuration
+					break
+				}
+			}


Let's put the duration into the incentivizedPoolIDs map from above. That way, we don't need to iterate over incentivizedPools repeatedly

add indirect incentivized pools to inc pool query

5431164

czarcas7ic added the V:state/breaking State machine breaking PR label Jun 20, 2023

github-actions bot added C:x/pool-incentives C:x/superfluid labels Jun 20, 2023

czarcas7ic added 2 commits June 20, 2023 12:53

add changelog entry

ceccc51

type conversion

81ce026

github-actions bot added the C:app-wiring Changes to the app folder label Jun 20, 2023

czarcas7ic added 2 commits June 20, 2023 14:55

fix behavior

ea3788a

fix behavior

28f7ee9

czarcas7ic changed the title ~~fix: add indirect incentivized pools to inc pool query~~ fix: add indirect incentivized pools to incentivized pools query Jun 20, 2023

czarcas7ic commented Jun 20, 2023

View reviewed changes

czarcas7ic marked this pull request as ready for review June 20, 2023 20:17

czarcas7ic requested review from mattverse, stackman27 and p0mvn as code owners June 20, 2023 20:17

p0mvn reviewed Jun 20, 2023

View reviewed changes

stackman27 reviewed Jun 20, 2023

View reviewed changes

czarcas7ic added 3 commits June 20, 2023 16:26

use map to prevent unnecessary second iteration

bba673d

nil pointer error

42a2e17

implement shared proto file

e70ac41

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jun 21, 2023

use longest duration as holder value

18ee857

p0mvn approved these changes Jun 21, 2023

View reviewed changes

czarcas7ic added 2 commits June 21, 2023 12:18

use longest lockable duration

1ebe6dc

add duration into incentivizedPoolIDs map

b7e3225

p0mvn merged commit 53ca015 into main Jun 22, 2023

p0mvn deleted the adam/direct-indirect-incentivized-pools branch June 22, 2023 01:54

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

Broken Links Detected #7319

Closed

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

Broken Links Detected #7903

Closed

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

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add indirect incentivized pools to incentivized pools query #5589

fix: add indirect incentivized pools to incentivized pools query #5589

czarcas7ic commented Jun 20, 2023 •

edited

Loading

czarcas7ic Jun 20, 2023

stackman27 Jun 20, 2023

czarcas7ic Jun 21, 2023

p0mvn Jun 20, 2023

czarcas7ic Jun 20, 2023

p0mvn Jun 20, 2023

czarcas7ic Jun 21, 2023 •

edited

Loading

stackman27 Jun 20, 2023

czarcas7ic Jun 20, 2023

stackman27 Jun 20, 2023

p0mvn left a comment

p0mvn Jun 21, 2023

p0mvn Jun 21, 2023

p0mvn Jun 21, 2023

czarcas7ic Jun 21, 2023

fix: add indirect incentivized pools to incentivized pools query #5589

fix: add indirect incentivized pools to incentivized pools query #5589

Conversation

czarcas7ic commented Jun 20, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

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

czarcas7ic Jun 21, 2023 • 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

p0mvn left a 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

czarcas7ic commented Jun 20, 2023 •

edited

Loading

czarcas7ic Jun 21, 2023 •

edited

Loading