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

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

Merged
merged 8 commits into from
Jan 30, 2024
Merged

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
317ae1c
add uptime validation logic and fix pool gauge link
AlpinYukseloglu Jan 28, 2024
ff27caf
add tests and clean up implementation
AlpinYukseloglu Jan 28, 2024
a12082b
add changelog and clean up prints/diff
AlpinYukseloglu Jan 28, 2024
815245e
update changelog entry to be clearer
AlpinYukseloglu Jan 28, 2024
e036a7b
clean up comments and add additional tests
AlpinYukseloglu Jan 28, 2024
39ba614
fix broken genesis tests
AlpinYukseloglu Jan 28, 2024
00b845b
extract default NoLock duration to variable and add further comments
AlpinYukseloglu Jan 28, 2024
34bba93
switch naming from concentrated to NoLock and move duration setup int…
AlpinYukseloglu Jan 29, 2024
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 @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### State Breaking

* [#7181](https://github.com/osmosis-labs/osmosis/pull/7181) Improve errors for out of gas
* [#7376](https://github.com/osmosis-labs/osmosis/pull/7376) Add uptime validation logic for `NoLock` (CL) gauges and switch CL gauge to pool ID links to be duration-based

### Bug Fixes

Expand Down
2 changes: 2 additions & 0 deletions x/incentives/keeper/distribute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ var (
}

defaultZeroWeightGaugeRecord = types.InternalGaugeRecord{GaugeId: 1, CurrentWeight: osmomath.ZeroInt(), CumulativeWeight: osmomath.ZeroInt()}
defaultNoLockDuration = time.Nanosecond
)

type GroupCreationFields struct {
Expand Down Expand Up @@ -1139,6 +1140,7 @@ func (s *KeeperTestSuite) CreateNoLockExternalGauges(clPoolId uint64, externalGa
clPoolExternalGaugeId, err := s.App.IncentivesKeeper.CreateGauge(s.Ctx, numEpochsPaidOver == 1, gaugeCreator, externalGaugeCoins,
lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
},
s.Ctx.BlockTime(),
numEpochsPaidOver,
Expand Down
43 changes: 38 additions & 5 deletions x/incentives/keeper/gauge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,42 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr
return 0, types.ErrZeroNumEpochsPaidOver
}

// If the gauge has no lock, then we assume it is a concentrated pool and ensure
// the gauge "lock" duration is an authorized uptime.
isConcentratedPoolGauge := distrTo.LockQueryType == lockuptypes.NoLock
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved

// If the gauge has an internal gauge denom, it is an internal gauge
// and should be run through different validation logic (see below).
//
// Two important reminders/assumptions:
// 1. `NoLock` gauges are required to have empty denoms in `ValidateBasic`, so this
// check cannot be controlled by user input.
// 2. The safety of this leans on the special-casing of internal gauge logic during
// distributions, which should be using the internal incentive duration gov param instead of the duration value.
isInternalConcentratedPoolGauge := distrTo.Denom == types.NoLockInternalGaugeDenom(poolId)
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved
isExternalConcentratedPoolGauge := isConcentratedPoolGauge && !isInternalConcentratedPoolGauge

// Ensure that this gauge's duration is one of the allowed durations on chain
durations := k.GetLockableDurations(ctx)
if distrTo.LockQueryType == lockuptypes.ByDuration {
// Concentrated pool gauges check against authorized uptimes (if external) or
// epoch duration (if internal).
//
// All other gauges check against the default set of lockable durations.
var durations []time.Duration
if isExternalConcentratedPoolGauge {
durations = k.clk.GetParams(ctx).AuthorizedUptimes
} else if isInternalConcentratedPoolGauge {
// Internal CL gauges use epoch time as their duration. This is a legacy
// property that does not affect the uptime on created records, which is
// determined by the gov param for internal incentive uptimes.
durations = []time.Duration{k.GetEpochInfo(ctx).Duration}
} else {
// This branch is applicable to CFMM pool types such as balancer and stableswap.
durations = k.GetLockableDurations(ctx)
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

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

This would make the conditional branching stricter and eliminate the likelihood of invalid field setup causing issues.

nit: the else branch is also not covered by a test at the moment and might benefit from it

Suggested change
if isExternalConcentratedPoolGauge {
durations = k.clk.GetParams(ctx).AuthorizedUptimes
} else if isInternalConcentratedPoolGauge {
// Internal CL gauges use epoch time as their duration. This is a legacy
// property that does not affect the uptime on created records, which is
// determined by the gov param for internal incentive uptimes.
durations = []time.Duration{k.GetEpochInfo(ctx).Duration}
} else {
// This branch is applicable to CFMM pool types such as balancer and stableswap.
durations = k.GetLockableDurations(ctx)
}
if isExternalConcentratedPoolGauge {
durations = k.clk.GetParams(ctx).AuthorizedUptimes
} else if isInternalConcentratedPoolGauge {
// Internal CL gauges use epoch time as their duration. This is a legacy
// property that does not affect the uptime on created records, which is
// determined by the gov param for internal incentive uptimes.
durations = []time.Duration{k.GetEpochInfo(ctx).Duration}
} else if !isConcentratedPoolGauge {
// This branch is applicable to CFMM pool types such as balancer and stableswap.
durations = k.GetLockableDurations(ctx)
} else {
return 0, fmt.Errorf("invalid gauge type: %s", distrTo.LockQueryType)
}

Copy link
Contributor Author

@AlpinYukseloglu AlpinYukseloglu Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see what you're getting at here, but this flow is not intended to serve as a check or filter where we return an error. For example, your implementation would error if CreateGauge is called for a Group gauge, which is not what we want here.

It does seem as though this logic is somewhat unclear though. I moved this check into the block where the duration checks happen, as that probably makes it clearer (i.e. if NoLock gauge || ByDuration gauge, run this logic implies that we are covering everything we need to with the logic branches in the original implementation). Hopefully this makes it a bit clearer.

Please let me know if I misunderstood your suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

34bba93


// We check durations if the gauge is a regular duration based gauge or if it is a
// CL gauge. Note that this excludes time-based gauges and group gauges.
if distrTo.LockQueryType == lockuptypes.ByDuration || isConcentratedPoolGauge {
durationOk := false
for _, duration := range durations {
if duration == distrTo.Duration {
Expand All @@ -146,7 +179,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr

// For no lock gauges, a pool id must be set.
// A pool with such id must exist and be a concentrated pool.
if distrTo.LockQueryType == lockuptypes.NoLock {
if isConcentratedPoolGauge {
if poolId == 0 {
return 0, fmt.Errorf("'no lock' type gauges must have a pool id")
}
Expand All @@ -155,7 +188,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr
// and get overwritten with the external prefix + pool id
// for internal query purposes.
distrToDenom := distrTo.Denom
if distrToDenom != types.NoLockInternalGaugeDenom(poolId) {
if !isInternalConcentratedPoolGauge {
// If denom is set, then fails.
if distrToDenom != "" {
return 0, fmt.Errorf("'no lock' type external gauges must have an empty denom set, was %s", distrToDenom)
Expand All @@ -177,7 +210,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr
// That being said, internal gauges have an additional linking
// by duration where duration is the incentives epoch duration.
// The internal incentive linking is set in x/pool-incentives CreateConcentratedLiquidityPoolGauge.
k.pik.SetPoolGaugeIdNoLock(ctx, poolId, nextGaugeId)
k.pik.SetPoolGaugeIdNoLock(ctx, poolId, nextGaugeId, distrTo.Duration)
} else {
// For all other gauges, pool id must be 0.
if poolId != 0 {
Expand Down
54 changes: 46 additions & 8 deletions x/incentives/keeper/gauge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is external
Denom: "",
Denom: "",
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add non-failing cases for the three branches that has been added to nolock createGauge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The non-failing case should be covered by existing tests, but will re review to make sure I'm not missing anything

Copy link
Member

Choose a reason for hiding this comment

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

Sg! On first site of existing tests, test cases did not seem obvious in which case touches which branch. Please feel free to merge once you have re-checked!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right – since 1 nanosecond is an authorized uptime by default, the test cases with that duration should be covering the non-failing branches

Duration: time.Nanosecond,
},
poolId: concentratedPoolId,

Expand All @@ -520,7 +521,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is internal
Denom: types.NoLockInternalGaugeDenom(concentratedPoolId),
Denom: types.NoLockInternalGaugeDenom(concentratedPoolId),
Duration: s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration,
},
poolId: concentratedPoolId,

Expand All @@ -533,7 +535,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this is invalid for NoLock gauges
Denom: "uosmo",
Denom: "uosmo",
Duration: time.Nanosecond,
},
poolId: concentratedPoolId,

Expand All @@ -543,6 +546,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
name: "fail to create no lock gauge with balancer pool",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
},
poolId: balancerPoolId,

Expand All @@ -552,6 +556,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
name: "fail to create no lock gauge with non-existent pool",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
},
poolId: invalidPool,

Expand All @@ -561,9 +566,46 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
name: "fail to create no lock gauge with zero pool id",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
},
poolId: zeroPoolId,

expectErr: true,
},
{
name: "fail to create external no lock gauge due to unauthorized uptime",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is external
Denom: "",
// 1h is a supported uptime that is not authorized
Duration: time.Hour,
},
poolId: concentratedPoolId,
expectErr: true,
},
{
name: "fail to create external no lock gauge due to entirely invalid uptime",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is external
Denom: "",
// 2ns is an uptime that isn't supported at all (i.e. can't even be authorized)
Duration: 2 * time.Nanosecond,
},
poolId: concentratedPoolId,
expectErr: true,
},
{
name: "fail to create an internal gauge with an unexpected duration",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is internal
Denom: types.NoLockInternalGaugeDenom(concentratedPoolId),
Duration: time.Nanosecond,
},
poolId: concentratedPoolId,

expectErr: true,
},
}
Expand All @@ -589,10 +631,6 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {

s.Require().Equal(tc.expectedGaugeId, gaugeId)

// Assert that pool id and gauge id link meant for internally incentivized gauges is unset.
_, err := s.App.PoolIncentivesKeeper.GetPoolGaugeId(s.Ctx, tc.poolId, tc.distrTo.Duration)
s.Require().Error(err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highlighting this for review @p0mvn. The key-related change in this PR means that the "general linking" between gauges and pools is now based on duration, which means that this check is no longer necessary.

It also technically means that we can skip the "internal gauge link" component of the internal gauge setup, but that logic is not well abstracted so it would require meaningful additional work to save on the duplicate write that would be happening here.

// Confirm that the general pool id to gauge id link is set.
gaugeIds, err := s.App.PoolIncentivesKeeper.GetNoLockGaugeIdsFromPool(s.Ctx, tc.poolId)
s.Require().NoError(err)
Expand Down Expand Up @@ -774,7 +812,7 @@ func (s *KeeperTestSuite) createGaugeNoRestrictions(isPerpetual bool, coins sdk.
}

if poolID != 0 {
s.App.PoolIncentivesKeeper.SetPoolGaugeIdNoLock(s.Ctx, poolID, nextGaugeID)
s.App.PoolIncentivesKeeper.SetPoolGaugeIdNoLock(s.Ctx, poolID, nextGaugeID, distrTo.Duration)
}

err := s.App.IncentivesKeeper.SetGauge(s.Ctx, &gauge)
Expand Down
1 change: 1 addition & 0 deletions x/incentives/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var (

distrToNoLock = lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
}

distrToNoLockPool1 = lockuptypes.QueryCondition{
Expand Down
3 changes: 2 additions & 1 deletion x/incentives/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type TxFeesKeeper interface {
type ConcentratedLiquidityKeeper interface {
CreateIncentive(ctx sdk.Context, poolId uint64, sender sdk.AccAddress, incentiveCoin sdk.Coin, emissionRate osmomath.Dec, startTime time.Time, minUptime time.Duration) (cltypes.IncentiveRecord, error)
GetConcentratedPoolById(ctx sdk.Context, poolId uint64) (cltypes.ConcentratedPoolExtension, error)
GetParams(ctx sdk.Context) (params cltypes.Params)
}

type AccountKeeper interface {
Expand All @@ -59,7 +60,7 @@ type AccountKeeper interface {
type PoolIncentiveKeeper interface {
GetPoolIdFromGaugeId(ctx sdk.Context, gaugeId uint64, lockableDuration time.Duration) (uint64, error)
GetInternalGaugeIDForPool(ctx sdk.Context, poolID uint64) (uint64, error)
SetPoolGaugeIdNoLock(ctx sdk.Context, poolId uint64, gaugeId uint64)
SetPoolGaugeIdNoLock(ctx sdk.Context, poolId uint64, gaugeId uint64, uptime time.Duration)
GetLongestLockableDuration(ctx sdk.Context) (time.Duration, error)
}

Expand Down
2 changes: 1 addition & 1 deletion x/pool-incentives/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
}
if genState.ConcentratedPoolToNoLockGauges != nil {
for _, record := range genState.ConcentratedPoolToNoLockGauges.PoolToGauge {
k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId)
k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId, record.Duration)
Copy link
Member

Choose a reason for hiding this comment

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

Are we planning for any migration PR after this? 👀

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 shouldn't need to - please see #7375 (if you see any problems with the reasoning in that issue please flag them!)

}
}
}
Expand Down
1 change: 1 addition & 0 deletions x/pool-incentives/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ func (s *KeeperTestSuite) TestImportExportGenesis_ExternalNoLock() {
// Create external non-perpetual gauge
externalGaugeID, err := s.App.IncentivesKeeper.CreateGauge(s.Ctx, false, s.TestAccs[0], defaultCoins.Add(defaultCoins...), lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
}, s.Ctx.BlockTime(), 2, clPool.GetId())
s.Require().NoError(err)

Expand Down
3 changes: 3 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 @@ -16,6 +16,8 @@ import (
var (
isPerpetual = true
notPerpetual = false

defaultNoLockDuration = time.Nanosecond
)

func (s *KeeperTestSuite) TestGaugeIds() {
Expand Down Expand Up @@ -443,6 +445,7 @@ func (s *KeeperTestSuite) TestExternalIncentiveGauges_NoLock() {
defaultNoLockGaugeConfig = gaugeConfig{
distributeTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
Duration: defaultNoLockDuration,
},
poolId: concentratedPoolId,
}
Expand Down
7 changes: 2 additions & 5 deletions x/pool-incentives/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func (k Keeper) SetPoolGaugeIdInternalIncentive(ctx sdk.Context, poolId uint64,

// 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) {
func (k Keeper) SetPoolGaugeIdNoLock(ctx sdk.Context, poolId uint64, gaugeId uint64, incentivizedUptime time.Duration) {
store := ctx.KVStore(k.storeKey)
// maps pool id and gauge id to gauge id.
// Note: this could be pool id and gauge id to empty byte array,
Expand All @@ -170,10 +170,7 @@ func (k Keeper) SetPoolGaugeIdNoLock(ctx sdk.Context, poolId uint64, gaugeId uin
store.Set(key, sdk.Uint64ToBigEndian(gaugeId))

// Note: this index is used for general linking.
// We supply zero for incentivized duration as "NoLock" gauges are not
// associated with any lockable duration. Instead, they incentivize
// pools directly.
key = types.GetPoolIdFromGaugeIdStoreKey(gaugeId, 0)
key = types.GetPoolIdFromGaugeIdStoreKey(gaugeId, incentivizedUptime)
store.Set(key, sdk.Uint64ToBigEndian(poolId))
}

Expand Down