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: NoLock gauge type and external gauge creation wiring to CL #5459

Merged
merged 51 commits into from
Jun 10, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jun 8, 2023

Closes: #XXX

What is the purpose of the change

This PR prototypes concentrated liquidity external incentives wiring into the exiting gauges model.

Contrary to previous reviews, it did not work out of the box. As a result, modifications were necessary.

Core of the idea:

  • Create a gauge with NoLock LockQueryType enum
  • Add a pool_id field to MsgCreateGauge
  • If NoLock is set, pool id must also be set and the pool must be a CL pool
  • Modify distribution logic to check for NoLock. If that's set, validate that there is a CL pool associated with this gauge and create an incentive record for it
  • Otherwise, use the regular pre-existing distribution flow

Incentives Module Changes

Besides the perpetual and non-perpetual categorization, gauges can now be grouped across another dimension - ByDuration or NoLock.
This is set on the DistrTo.LockQueryType field of the MsgCreateGauge.

  • ByDuration when the gauge of this kind is created, it is meant to incentivize locks. When it is set,
    the PoolId field of the MsgCreateGauge must be zero.

  • NoLock when the gauge of this kind is created, it is meant to incentivize pools directly. When it is set,
    the PoolId field of the MsgCreateGauge must be non-zero. Additionally, it must be associated with a CL
    pool at launch. Moreover, the DistrTo.Denom field must be set to empty string in such a case.

Each of the ByDuration and NoLock gauges can be perpetual or non-perpetual and function according to the
conventional rules of the respective gauge type.

Additionally, for NoLock gauges, lockuptypes.Denom must be either an empty string, signifying that
this is an external gauge, or be equal to types.NoLockInternalGaugeDenom(poolId) (when created from the AfterPoolCreatedHook)
If the denom is empty, it will get overwritten to types.NoLockExternalGaugeDenom(poolId).
This denom formatting is useful for querying internal vs externa gauges associated with a pool since the denom prefix is
appended into the store prefix.

Concentrated Liquidity External Gauge Creation

To create a gauge dedicated to the concentrated liquidity pool, run a MsgCreateGauge message in the x/incentives module with the following parameters:

  • PoolId: The ID of the CL pool to create a gauge for.
  • DistrTo.LockQueryType must be set to locktypes.LockQueryType.NoLock
  • DistrTo.Denom must be an empty string.

The rest of the parameters can be set according to the desired configuration of the gauge. Please read the x/incentives module documentation for more information on how to configure gauges.

Note, that the created gauge will start emitting at the first epoch after the given StartTime. During the epoch, a x/concentrated-liquidity
module IncentiveRecord will be created for every denom in the gauge. This incentive record will be condifured to emit all given incentives
over the period of an epoch. If the gauge is non-perpetual (emits over several epochs), the distribution will be split evenly between the epochs.
and a new IncentiveRecord will be created for each denom every epoch with the emission rate and token set to finish emitting at the end of the epoch.

Testing and Verifying

Important Unit Tests to Verify

  • TestCreateGauge_NoLockGauges
  • TestDistribute_InternalIncentives_NoLock
  • TestDistribute_ExternalIncentives_NoLock

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

@p0mvn p0mvn changed the base branch from main to roman/cl-incentive June 8, 2023 01:33
Copy link
Contributor

@stackman27 stackman27 left a comment

Choose a reason for hiding this comment

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

thank you for the PR. looks solid so far

i'll try to re-review and run test by the eod

x/incentives/keeper/gauge.go Show resolved Hide resolved
@p0mvn p0mvn force-pushed the roman/cl-external-incentives branch from 4c65a87 to 6981481 Compare June 8, 2023 03:07
@github-actions github-actions bot removed T:build C:docs Improvements or additions to documentation labels Jun 8, 2023
@p0mvn p0mvn added V:state/breaking State machine breaking PR A:no-changelog labels Jun 8, 2023
@p0mvn p0mvn force-pushed the roman/cl-external-incentives branch from df80f1c to 68e42a6 Compare June 8, 2023 19:08
Comment on lines -301 to -306
denom := lockuptypes.NativeDenom(gauge.DistributeTo.Denom)
lockSum := lockuptypes.SumLocksByDenom(locks, denom)

if lockSum.IsZero() {
return nil, nil
}
Copy link
Member Author

@p0mvn p0mvn Jun 8, 2023

Choose a reason for hiding this comment

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

Note: all of the removed logic here and below was moved to the else case of

if gauge.DistributeTo.LockQueryType == lockuptypes.NoLock {

ref: https://github.com/osmosis-labs/osmosis/pull/5459/files#r1223483274

Copy link
Contributor

@stackman27 stackman27 left a comment

Choose a reason for hiding this comment

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

This LGTM besides some minor comments

also localosmosis tested QueryExternalIncentiveGaugesRequest and it works as expected. Still testing QueryIncentivizedPoolsRequest

MacBook-Pro-26:osmosis sishirg27$ 
MacBook-Pro-26:osmosis sishirg27$ 
MacBook-Pro-26:osmosis sishirg27$ osmosisd q poolincentives external-incentivized-gauges
data:
- coins:
  - amount: "1000"
    denom: stake
  - amount: "10000"
    denom: uosmo
  distribute_to:
    denom: no-lock/e/1
    duration: 86400s
    lock_query_type: NoLock
    timestamp: "1970-01-01T00:00:00Z"
  distributed_coins: []
  filled_epochs: "0"
  id: "11"
  is_perpetual: true
  num_epochs_paid_over: "1"
  start_time: "1970-01-01T00:00:00Z"
- coins:
  - amount: "1200"
    denom: uion
  distribute_to:
    denom: no-lock/e/1
    duration: 86400s
    lock_query_type: NoLock
    timestamp: "1970-01-01T00:00:00Z"
  distributed_coins: []
  filled_epochs: "0"
  id: "12"
  is_perpetual: true
  num_epochs_paid_over: "1"
  start_time: "1970-01-01T00:00:00Z"

x/pool-incentives/keeper/keeper.go Outdated Show resolved Hide resolved
x/pool-incentives/keeper/keeper.go Show resolved Hide resolved

// SetPoolGaugeIdNoLock sets the link between pool id and gauge id for "NoLock" gauges.
// CONTRACT: the gauge of the given id must be "NoLock" gauge.
func (k Keeper) SetPoolGaugeIdNoLock(ctx sdk.Context, poolId uint64, gaugeId uint64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just as a point of caution, can we make sure that we cannot set one of LockableDuration to 0 for Lock case. For instance in this function SetLockableDurations ?

Copy link
Member Author

Choose a reason for hiding this comment

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

What specifically are you suggesting? I currently don't see anything that can be done where the added complexity would be justified by the security benefit but open to try specific paths if you see any

Copy link
Contributor

@stackman27 stackman27 Jun 9, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, not really concerned about that case because we explicitly hardcode that value to be incentivized epoch duration for "No Lock" gauges, changing which is gated by governance.

However, I will add the suggested check prior to merge

x/pool-incentives/keeper/keeper.go Outdated Show resolved Hide resolved
@p0mvn p0mvn requested review from stackman27 and czarcas7ic June 9, 2023 23:23
Copy link
Contributor

@stackman27 stackman27 left a comment

Choose a reason for hiding this comment

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

LGTM!!

Really great PR! There might be some weird edge case flow that might not be obvious so definitely want to do rigirious functional test on this

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Nice work!

Agreed with sish that we will likely need some functional tests to feel 100 percent with this, but feeling more comfortable with this now

x/incentives/keeper/distribute_test.go Outdated Show resolved Hide resolved
x/incentives/keeper/gauge.go Show resolved Hide resolved
x/incentives/types/msgs.go Outdated Show resolved Hide resolved
x/pool-incentives/keeper/keeper.go Show resolved Hide resolved
Base automatically changed from roman/cl-incentive to main June 10, 2023 17:07
@p0mvn p0mvn requested a review from a team as a code owner June 10, 2023 17:07
@p0mvn
Copy link
Member Author

p0mvn commented Jun 10, 2023

Thanks for the reviews.

Did some more localosmosis tests.

  • Created some balancer and CL pools, internal incentive distribution records for 1 balancer and 1 CL gauge
  • Created and external NoLock gauge.

Observed that the queries for internal and external gauges work as intended and that CL incentive records get created.

However, I did not validate the exact numbers which should be done in #5471

I also found a bug that is specific to CL incentive records and is outside of scope of this PR: #5492

Merging this PR to unblock #5471 and #5492

@p0mvn p0mvn merged commit 2fd0415 into main Jun 10, 2023
@p0mvn p0mvn deleted the roman/cl-external-incentives branch June 10, 2023 19:32
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants