From 317ae1c931515893bfee7377cdef838ec6192b94 Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 10:44:58 -0800 Subject: [PATCH 1/8] add uptime validation logic and fix pool gauge link --- x/incentives/keeper/distribute.go | 3 +- x/incentives/keeper/distribute_test.go | 1 + x/incentives/keeper/gauge.go | 34 ++++++++++++++++++--- x/incentives/keeper/gauge_test.go | 13 ++++++-- x/incentives/keeper/genesis_test.go | 1 + x/incentives/keeper/keeper_test.go | 2 ++ x/incentives/types/expected_keepers.go | 3 +- x/pool-incentives/keeper/genesis.go | 2 +- x/pool-incentives/keeper/grpc_query_test.go | 1 + x/pool-incentives/keeper/keeper.go | 7 ++--- 10 files changed, 52 insertions(+), 15 deletions(-) diff --git a/x/incentives/keeper/distribute.go b/x/incentives/keeper/distribute.go index 22240ff2c46..1dfe3e4d917 100644 --- a/x/incentives/keeper/distribute.go +++ b/x/incentives/keeper/distribute.go @@ -589,7 +589,6 @@ func (k Keeper) distributeInternal( if gauge.DistributeTo.LockQueryType == lockuptypes.NoLock { ctx.Logger().Debug("distributeInternal NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "height", ctx.BlockHeight()) pool, err := k.GetPoolFromGaugeId(ctx, gauge.Id, gauge.DistributeTo.Duration) - if err != nil { return nil, err } @@ -732,6 +731,7 @@ func guaranteedNonzeroCoinAmountOf(coins sdk.Coins, denom string) osmomath.Int { // Also adds the coins that were just distributed to the gauge's distributed coins field. func (k Keeper) updateGaugePostDistribute(ctx sdk.Context, gauge types.Gauge, newlyDistributedCoins sdk.Coins) error { gauge.FilledEpochs += 1 + fmt.Println(fmt.Sprintf("Filled epoch on gauge %d. New filled epochs: %d", gauge.Id, gauge.FilledEpochs)) gauge.DistributedCoins = gauge.DistributedCoins.Add(newlyDistributedCoins...) if err := k.setGauge(ctx, &gauge); err != nil { return err @@ -802,6 +802,7 @@ func (k Keeper) Distribute(ctx sdk.Context, gauges []types.Gauge) (sdk.Coins, er scratchSlice := make([]*lockuptypes.PeriodLock, 0, 10000) for _, gauge := range gauges { + fmt.Println("Distributing from gauge: ", gauge.Id) var gaugeDistributedCoins sdk.Coins filteredLocks := k.getDistributeToBaseLocks(ctx, gauge, locksByDenomCache, &scratchSlice) // send based on synthetic lockup coins if it's distributing to synthetic lockups diff --git a/x/incentives/keeper/distribute_test.go b/x/incentives/keeper/distribute_test.go index 0c4eeb81ab9..961061322e8 100644 --- a/x/incentives/keeper/distribute_test.go +++ b/x/incentives/keeper/distribute_test.go @@ -1139,6 +1139,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: time.Nanosecond, }, s.Ctx.BlockTime(), numEpochsPaidOver, diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 872b0572811..1215068af34 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -127,9 +127,34 @@ 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 + + // If the gauge is being created by the pool incentives module, it is for an internal + // gauge and should not be blocked on creation here. + // + // Two important reminders: + // 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) + 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 (excluding internal) check against authorized uptimes, + // while all other gauges check against the default set of lockable durations. + var durations []time.Duration + if isExternalConcentratedPoolGauge { + durations = k.clk.GetParams(ctx).AuthorizedUptimes + fmt.Println("AUTHORIZED UPTIMES: ", durations) + } else { + durations = k.GetLockableDurations(ctx) + } + + // We check durations if the gauge is a regular duration based gauge, or if it is an external CL gauge. + if distrTo.LockQueryType == lockuptypes.ByDuration || isExternalConcentratedPoolGauge { durationOk := false for _, duration := range durations { if duration == distrTo.Duration { @@ -146,7 +171,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") } @@ -177,7 +202,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 { @@ -218,6 +243,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr return 0, err } k.SetLastGaugeID(ctx, gauge.Id) + fmt.Println("Created gauge with ID (is internal CL): ", nextGaugeId, isInternalConcentratedPoolGauge) combinedKeys := combineKeys(types.KeyPrefixUpcomingGauges, getTimeKey(gauge.StartTime)) diff --git a/x/incentives/keeper/gauge_test.go b/x/incentives/keeper/gauge_test.go index 5cc545b74a9..e4ba15e4ce5 100644 --- a/x/incentives/keeper/gauge_test.go +++ b/x/incentives/keeper/gauge_test.go @@ -507,7 +507,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, // Note: this assumes the gauge is external - Denom: "", + Denom: "", + Duration: time.Nanosecond, }, poolId: concentratedPoolId, @@ -520,6 +521,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, // Note: this assumes the gauge is internal + // We intentionally do not set a gauge duration as it should make no + // difference for internal gauges. Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), }, poolId: concentratedPoolId, @@ -533,7 +536,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, @@ -543,6 +547,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { name: "fail to create no lock gauge with balancer pool", distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, + Duration: time.Nanosecond, }, poolId: balancerPoolId, @@ -552,6 +557,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { name: "fail to create no lock gauge with non-existent pool", distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, + Duration: time.Nanosecond, }, poolId: invalidPool, @@ -561,6 +567,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { name: "fail to create no lock gauge with zero pool id", distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, + Duration: time.Nanosecond, }, poolId: zeroPoolId, @@ -774,7 +781,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) diff --git a/x/incentives/keeper/genesis_test.go b/x/incentives/keeper/genesis_test.go index 1809e0c0d8a..def45832977 100644 --- a/x/incentives/keeper/genesis_test.go +++ b/x/incentives/keeper/genesis_test.go @@ -27,6 +27,7 @@ var ( distrToNoLock = lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, + Duration: time.Nanosecond, } distrToNoLockPool1 = lockuptypes.QueryCondition{ diff --git a/x/incentives/keeper/keeper_test.go b/x/incentives/keeper/keeper_test.go index d2321d0fad9..7b159e1c197 100644 --- a/x/incentives/keeper/keeper_test.go +++ b/x/incentives/keeper/keeper_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + "fmt" "testing" "time" @@ -64,6 +65,7 @@ func (s *KeeperTestSuite) ValidateDistributedGauge(gaugeID uint64, expectedFille // Check that filled epcohs is not updated gauge, err := s.App.IncentivesKeeper.GetGaugeByID(s.Ctx, gaugeID) s.Require().NoError(err) + fmt.Println("Shown filled epocs in validation for gauge ID (ID, num filled): ", gaugeID, gauge.FilledEpochs) s.Require().Equal(expectedFilledEpoch, gauge.FilledEpochs) // Check that distributed coins is not updated diff --git a/x/incentives/types/expected_keepers.go b/x/incentives/types/expected_keepers.go index 05d8e44ddf3..16487b87ed4 100644 --- a/x/incentives/types/expected_keepers.go +++ b/x/incentives/types/expected_keepers.go @@ -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 { @@ -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) } diff --git a/x/pool-incentives/keeper/genesis.go b/x/pool-incentives/keeper/genesis.go index 82925b82976..43bfdd73d56 100644 --- a/x/pool-incentives/keeper/genesis.go +++ b/x/pool-incentives/keeper/genesis.go @@ -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) } } } diff --git a/x/pool-incentives/keeper/grpc_query_test.go b/x/pool-incentives/keeper/grpc_query_test.go index 0e956fe200c..3da4c3f28a7 100644 --- a/x/pool-incentives/keeper/grpc_query_test.go +++ b/x/pool-incentives/keeper/grpc_query_test.go @@ -443,6 +443,7 @@ func (s *KeeperTestSuite) TestExternalIncentiveGauges_NoLock() { defaultNoLockGaugeConfig = gaugeConfig{ distributeTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, + Duration: time.Nanosecond, }, poolId: concentratedPoolId, } diff --git a/x/pool-incentives/keeper/keeper.go b/x/pool-incentives/keeper/keeper.go index fd2edbfec45..395fa96c421 100644 --- a/x/pool-incentives/keeper/keeper.go +++ b/x/pool-incentives/keeper/keeper.go @@ -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, @@ -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)) } From ff27cafe7fe0f205f2ae01bb3d7f9e1fae5d7e6e Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 12:24:26 -0800 Subject: [PATCH 2/8] add tests and clean up implementation --- x/incentives/keeper/gauge.go | 10 +++++++--- x/incentives/keeper/gauge_test.go | 32 ++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 1215068af34..02f9b160bf3 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -148,13 +148,17 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr var durations []time.Duration if isExternalConcentratedPoolGauge { durations = k.clk.GetParams(ctx).AuthorizedUptimes - fmt.Println("AUTHORIZED UPTIMES: ", durations) + } 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 { durations = k.GetLockableDurations(ctx) } // We check durations if the gauge is a regular duration based gauge, or if it is an external CL gauge. - if distrTo.LockQueryType == lockuptypes.ByDuration || isExternalConcentratedPoolGauge { + if distrTo.LockQueryType == lockuptypes.ByDuration || isConcentratedPoolGauge { durationOk := false for _, duration := range durations { if duration == distrTo.Duration { @@ -180,7 +184,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) diff --git a/x/incentives/keeper/gauge_test.go b/x/incentives/keeper/gauge_test.go index e4ba15e4ce5..2da32a75421 100644 --- a/x/incentives/keeper/gauge_test.go +++ b/x/incentives/keeper/gauge_test.go @@ -523,7 +523,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { // Note: this assumes the gauge is internal // We intentionally do not set a gauge duration as it should make no // difference for internal gauges. - Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), + Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), + Duration: s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration, }, poolId: concentratedPoolId, @@ -571,6 +572,31 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { }, 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: "", + 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 + // We intentionally do not set a gauge duration as it should make no + // difference for internal gauges. + Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), + Duration: time.Nanosecond, + }, + poolId: concentratedPoolId, + expectErr: true, }, } @@ -596,10 +622,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) - // 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) From a12082b48f086bc752cca3cc5e24581a19613aa6 Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 12:58:35 -0800 Subject: [PATCH 3/8] add changelog and clean up prints/diff --- CHANGELOG.md | 1 + x/incentives/keeper/distribute.go | 3 +-- x/incentives/keeper/gauge.go | 1 - x/incentives/keeper/keeper_test.go | 2 -- 4 files changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19fbd5b858b..8570f69c103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 duration-based CL gauge to pool ID links ### Bug Fixes diff --git a/x/incentives/keeper/distribute.go b/x/incentives/keeper/distribute.go index 1dfe3e4d917..22240ff2c46 100644 --- a/x/incentives/keeper/distribute.go +++ b/x/incentives/keeper/distribute.go @@ -589,6 +589,7 @@ func (k Keeper) distributeInternal( if gauge.DistributeTo.LockQueryType == lockuptypes.NoLock { ctx.Logger().Debug("distributeInternal NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "height", ctx.BlockHeight()) pool, err := k.GetPoolFromGaugeId(ctx, gauge.Id, gauge.DistributeTo.Duration) + if err != nil { return nil, err } @@ -731,7 +732,6 @@ func guaranteedNonzeroCoinAmountOf(coins sdk.Coins, denom string) osmomath.Int { // Also adds the coins that were just distributed to the gauge's distributed coins field. func (k Keeper) updateGaugePostDistribute(ctx sdk.Context, gauge types.Gauge, newlyDistributedCoins sdk.Coins) error { gauge.FilledEpochs += 1 - fmt.Println(fmt.Sprintf("Filled epoch on gauge %d. New filled epochs: %d", gauge.Id, gauge.FilledEpochs)) gauge.DistributedCoins = gauge.DistributedCoins.Add(newlyDistributedCoins...) if err := k.setGauge(ctx, &gauge); err != nil { return err @@ -802,7 +802,6 @@ func (k Keeper) Distribute(ctx sdk.Context, gauges []types.Gauge) (sdk.Coins, er scratchSlice := make([]*lockuptypes.PeriodLock, 0, 10000) for _, gauge := range gauges { - fmt.Println("Distributing from gauge: ", gauge.Id) var gaugeDistributedCoins sdk.Coins filteredLocks := k.getDistributeToBaseLocks(ctx, gauge, locksByDenomCache, &scratchSlice) // send based on synthetic lockup coins if it's distributing to synthetic lockups diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 02f9b160bf3..18f6174e863 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -247,7 +247,6 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr return 0, err } k.SetLastGaugeID(ctx, gauge.Id) - fmt.Println("Created gauge with ID (is internal CL): ", nextGaugeId, isInternalConcentratedPoolGauge) combinedKeys := combineKeys(types.KeyPrefixUpcomingGauges, getTimeKey(gauge.StartTime)) diff --git a/x/incentives/keeper/keeper_test.go b/x/incentives/keeper/keeper_test.go index 7b159e1c197..d2321d0fad9 100644 --- a/x/incentives/keeper/keeper_test.go +++ b/x/incentives/keeper/keeper_test.go @@ -1,7 +1,6 @@ package keeper_test import ( - "fmt" "testing" "time" @@ -65,7 +64,6 @@ func (s *KeeperTestSuite) ValidateDistributedGauge(gaugeID uint64, expectedFille // Check that filled epcohs is not updated gauge, err := s.App.IncentivesKeeper.GetGaugeByID(s.Ctx, gaugeID) s.Require().NoError(err) - fmt.Println("Shown filled epocs in validation for gauge ID (ID, num filled): ", gaugeID, gauge.FilledEpochs) s.Require().Equal(expectedFilledEpoch, gauge.FilledEpochs) // Check that distributed coins is not updated From 815245e2b0ae5f05dac5a781c7afbd7828c338a2 Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 13:00:15 -0800 Subject: [PATCH 4/8] update changelog entry to be clearer --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8570f69c103..e6728bc81f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +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 duration-based CL gauge to pool ID links +* [#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 From e036a7b9c672c78803446c034c32d7b61032996f Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 13:16:23 -0800 Subject: [PATCH 5/8] clean up comments and add additional tests --- x/incentives/keeper/gauge.go | 17 ++++++++++------- x/incentives/keeper/gauge_test.go | 19 ++++++++++++++----- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 18f6174e863..ba65f31193d 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -131,20 +131,22 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr // the gauge "lock" duration is an authorized uptime. isConcentratedPoolGauge := distrTo.LockQueryType == lockuptypes.NoLock - // If the gauge is being created by the pool incentives module, it is for an internal - // gauge and should not be blocked on creation here. + // If the gauge has an internal gauge denpom, it is an internal gauge + // and should be run through different validation logic (see below). // - // Two important reminders: + // Two important reminders/assumptions: // 1. `NoLock` gauges are required to have empty denoms in `ValidateBasic`, so this - // check cannot be controlled by user input + // 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) isExternalConcentratedPoolGauge := isConcentratedPoolGauge && !isInternalConcentratedPoolGauge // Ensure that this gauge's duration is one of the allowed durations on chain - // Concentrated pool gauges (excluding internal) check against authorized uptimes, - // while all other gauges check against the default set of lockable durations. + // 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 @@ -157,7 +159,8 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr durations = k.GetLockableDurations(ctx) } - // We check durations if the gauge is a regular duration based gauge, or if it is an external CL gauge. + // 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 { diff --git a/x/incentives/keeper/gauge_test.go b/x/incentives/keeper/gauge_test.go index 2da32a75421..d4ee60dfc76 100644 --- a/x/incentives/keeper/gauge_test.go +++ b/x/incentives/keeper/gauge_test.go @@ -521,8 +521,6 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, // Note: this assumes the gauge is internal - // We intentionally do not set a gauge duration as it should make no - // difference for internal gauges. Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), Duration: s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration, }, @@ -579,7 +577,20 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, // Note: this assumes the gauge is external - Denom: "", + 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, @@ -590,8 +601,6 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, // Note: this assumes the gauge is internal - // We intentionally do not set a gauge duration as it should make no - // difference for internal gauges. Denom: types.NoLockInternalGaugeDenom(concentratedPoolId), Duration: time.Nanosecond, }, From 39ba6149bf4b2f02178c8357f83a54c7392d35a6 Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 13:18:05 -0800 Subject: [PATCH 6/8] fix broken genesis tests --- x/pool-incentives/keeper/genesis_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/pool-incentives/keeper/genesis_test.go b/x/pool-incentives/keeper/genesis_test.go index 29e33786465..ecbf0602618 100644 --- a/x/pool-incentives/keeper/genesis_test.go +++ b/x/pool-incentives/keeper/genesis_test.go @@ -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: time.Nanosecond, }, s.Ctx.BlockTime(), 2, clPool.GetId()) s.Require().NoError(err) From 00b845b3320834dfd410f8d06d5e5df1678ce346 Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 14:32:43 -0800 Subject: [PATCH 7/8] extract default NoLock duration to variable and add further comments --- x/incentives/keeper/distribute_test.go | 3 ++- x/incentives/keeper/gauge.go | 3 ++- x/incentives/keeper/gauge_test.go | 6 +++--- x/incentives/keeper/genesis_test.go | 2 +- x/pool-incentives/keeper/genesis_test.go | 2 +- x/pool-incentives/keeper/grpc_query_test.go | 4 +++- 6 files changed, 12 insertions(+), 8 deletions(-) diff --git a/x/incentives/keeper/distribute_test.go b/x/incentives/keeper/distribute_test.go index 961061322e8..80df51a630e 100644 --- a/x/incentives/keeper/distribute_test.go +++ b/x/incentives/keeper/distribute_test.go @@ -74,6 +74,7 @@ var ( } defaultZeroWeightGaugeRecord = types.InternalGaugeRecord{GaugeId: 1, CurrentWeight: osmomath.ZeroInt(), CumulativeWeight: osmomath.ZeroInt()} + defaultNoLockDuration = time.Nanosecond ) type GroupCreationFields struct { @@ -1139,7 +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: time.Nanosecond, + Duration: defaultNoLockDuration, }, s.Ctx.BlockTime(), numEpochsPaidOver, diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index ba65f31193d..986deb2d8d1 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -131,7 +131,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr // the gauge "lock" duration is an authorized uptime. isConcentratedPoolGauge := distrTo.LockQueryType == lockuptypes.NoLock - // If the gauge has an internal gauge denpom, it is an internal gauge + // 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: @@ -156,6 +156,7 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr // 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) } diff --git a/x/incentives/keeper/gauge_test.go b/x/incentives/keeper/gauge_test.go index d4ee60dfc76..1ec234fc5b7 100644 --- a/x/incentives/keeper/gauge_test.go +++ b/x/incentives/keeper/gauge_test.go @@ -546,7 +546,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { name: "fail to create no lock gauge with balancer pool", distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, - Duration: time.Nanosecond, + Duration: defaultNoLockDuration, }, poolId: balancerPoolId, @@ -556,7 +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: time.Nanosecond, + Duration: defaultNoLockDuration, }, poolId: invalidPool, @@ -566,7 +566,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() { name: "fail to create no lock gauge with zero pool id", distrTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, - Duration: time.Nanosecond, + Duration: defaultNoLockDuration, }, poolId: zeroPoolId, diff --git a/x/incentives/keeper/genesis_test.go b/x/incentives/keeper/genesis_test.go index def45832977..ea642ab049b 100644 --- a/x/incentives/keeper/genesis_test.go +++ b/x/incentives/keeper/genesis_test.go @@ -27,7 +27,7 @@ var ( distrToNoLock = lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, - Duration: time.Nanosecond, + Duration: defaultNoLockDuration, } distrToNoLockPool1 = lockuptypes.QueryCondition{ diff --git a/x/pool-incentives/keeper/genesis_test.go b/x/pool-incentives/keeper/genesis_test.go index ecbf0602618..9ad88e4d9cc 100644 --- a/x/pool-incentives/keeper/genesis_test.go +++ b/x/pool-incentives/keeper/genesis_test.go @@ -151,7 +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: time.Nanosecond, + Duration: defaultNoLockDuration, }, s.Ctx.BlockTime(), 2, clPool.GetId()) s.Require().NoError(err) diff --git a/x/pool-incentives/keeper/grpc_query_test.go b/x/pool-incentives/keeper/grpc_query_test.go index 3da4c3f28a7..86415c6f737 100644 --- a/x/pool-incentives/keeper/grpc_query_test.go +++ b/x/pool-incentives/keeper/grpc_query_test.go @@ -16,6 +16,8 @@ import ( var ( isPerpetual = true notPerpetual = false + + defaultNoLockDuration = time.Nanosecond ) func (s *KeeperTestSuite) TestGaugeIds() { @@ -443,7 +445,7 @@ func (s *KeeperTestSuite) TestExternalIncentiveGauges_NoLock() { defaultNoLockGaugeConfig = gaugeConfig{ distributeTo: lockuptypes.QueryCondition{ LockQueryType: lockuptypes.NoLock, - Duration: time.Nanosecond, + Duration: defaultNoLockDuration, }, poolId: concentratedPoolId, } From 34bba93dc0d82eb528c4a9c123a27abc6d912dbb Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 28 Jan 2024 23:18:07 -0800 Subject: [PATCH 8/8] switch naming from concentrated to NoLock and move duration setup into appropriate logic branch --- x/incentives/keeper/gauge.go | 52 ++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/x/incentives/keeper/gauge.go b/x/incentives/keeper/gauge.go index 986deb2d8d1..893c2fa5708 100644 --- a/x/incentives/keeper/gauge.go +++ b/x/incentives/keeper/gauge.go @@ -127,9 +127,9 @@ 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 + // If the gauge has no lock, then we currently assume it is a concentrated pool + // and ensure the gauge "lock" duration is an authorized uptime. + isNoLockGauge := distrTo.LockQueryType == lockuptypes.NoLock // If the gauge has an internal gauge denom, it is an internal gauge // and should be run through different validation logic (see below). @@ -139,30 +139,30 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr // 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) - isExternalConcentratedPoolGauge := isConcentratedPoolGauge && !isInternalConcentratedPoolGauge - - // Ensure that this gauge's duration is one of the allowed durations on chain - // 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) - } + isInternalNoLockGauge := isNoLockGauge && distrTo.Denom == types.NoLockInternalGaugeDenom(poolId) + isExternalNoLockGauge := isNoLockGauge && !isInternalNoLockGauge // 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 { + if isNoLockGauge || distrTo.LockQueryType == lockuptypes.ByDuration { + // Ensure that this gauge's duration is one of the allowed durations on chain + // 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 isExternalNoLockGauge { + durations = k.clk.GetParams(ctx).AuthorizedUptimes + } else if isInternalNoLockGauge { + // 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) + } + durationOk := false for _, duration := range durations { if duration == distrTo.Duration { @@ -179,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 isConcentratedPoolGauge { + if isNoLockGauge { if poolId == 0 { return 0, fmt.Errorf("'no lock' type gauges must have a pool id") } @@ -188,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 !isInternalConcentratedPoolGauge { + if !isInternalNoLockGauge { // 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)