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

[Uptime Incentives]: Incorporate gov param in distribution logic #7419

Merged
merged 12 commits into from
Feb 7, 2024
Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#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
* [#7357](https://github.com/osmosis-labs/osmosis/pull/7357) Fix: Ensure rate limits are not applied to packets that aren't ics20s
* [#7417](https://github.com/osmosis-labs/osmosis/pull/7417) Update CL gauges to use gauge duration as uptime, falling back to default if unauthorized or invalid
* [#7419](https://github.com/osmosis-labs/osmosis/pull/7419) Use new module param for internal incentive uptimes

### Bug Fixes

Expand Down
36 changes: 34 additions & 2 deletions x/incentives/keeper/distribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,37 @@ func (k Keeper) distributeInternal(
// Get distribution epoch duration. This is used to calculate the emission rate.
currentEpoch := k.GetEpochInfo(ctx)

// If internal gauge, use InternalUptime param as the gauge's uptime.
// Otherwise, use the gauge's duration.
gaugeUptime := gauge.DistributeTo.Duration
if gauge.DistributeTo.Denom == types.NoLockInternalGaugeDenom(pool.GetId()) {
gaugeUptime = k.GetParams(ctx).InternalUptime
}

// Validate that the gauge's corresponding uptime is authorized.
authorizedUptimes := k.clk.GetParams(ctx).AuthorizedUptimes
isUptimeAuthorized := false
for _, authorizedUptime := range authorizedUptimes {
if gaugeUptime == authorizedUptime {
isUptimeAuthorized = true
}
}

// If the gauge's uptime is not authorized, we fall back to a default instead of erroring.
//
// This is for two reasons:
// 1. To allow uptimes to be unauthorized without entirely freezing existing gauges
// 2. To avoid having to do a state migration on existing gauges at time of adding
// this change, since prior to this, CL gauges were not required to associate with
// an uptime that was authorized.
//
// Note that this fallback happens even if the gauge is an internal gauge. This is because
// we want to be able to unauthorize internal gauges as well in the event that there is an
// issue found with a previously authorized uptime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A secondary reason is that allowing "supported but unauthorized uptimes" to be valid for internal gauges would require hackily bypassing CL incentive record checks, which currently validate against authorized uptimes. I started with trying this and ended up walking back my impl to this version.

if !isUptimeAuthorized {
gaugeUptime = types.DefaultConcentratedUptime
}

// For every coin in the gauge, calculate the remaining reward per epoch
// and create a concentrated liquidity incentive record for it that
// is supposed to distribute over that epoch.
Expand All @@ -625,8 +656,9 @@ func (k Keeper) distributeInternal(
// Gauge start time should be checked whenever moving between active
// and inactive gauges. By the time we get here, the gauge should be active.
ctx.BlockTime(),
// Only default uptime is supported at launch.
types.DefaultConcentratedUptime,
// The uptime for each distribution is determined by the gauge's duration field.
// If it is unauthorized, we fall back to a default above.
gaugeUptime,
)

ctx.Logger().Info(fmt.Sprintf("distributeInternal CL for pool id %d finished", pool.GetId()))
Expand Down
233 changes: 148 additions & 85 deletions x/incentives/keeper/distribute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,45 +251,84 @@ func (s *KeeperTestSuite) TestDistribute_InternalIncentives_NoLock() {
tokensToAddToGauge sdk.Coins
gaugeStartTime time.Time
gaugeCoins sdk.Coins
internalUptime time.Duration

// expected
expectErr bool
// Note: internal gauge distributions should never error, so we don't expect any errors here.
expectedDistributions sdk.Coins
expectedUptime time.Duration
}{
"valid case: one poolId and gaugeId": {
Copy link
Contributor Author

@AlpinYukseloglu AlpinYukseloglu Feb 6, 2024

Choose a reason for hiding this comment

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

Note: diff is a bit messy due to driveby test case renaming/removal of expectErr: falses. Happy to undo either of these if anyone has strong qualms.

numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
"one poolId and gaugeId": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: types.DefaultConcentratedUptime,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fiveKRewardCoins),
expectErr: false,
},
"valid case: gauge with multiple coins": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins, fiveKRewardCoinsUosmo),
"gauge with multiple coins": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins, fiveKRewardCoinsUosmo),
internalUptime: types.DefaultConcentratedUptime,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fiveKRewardCoins, fiveKRewardCoinsUosmo),
expectErr: false,
},
"valid case: multiple gaugeId and poolId": {
numPools: 3,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
"multiple gaugeId and poolId": {
numPools: 3,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: types.DefaultConcentratedUptime,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fifteenKRewardCoins),
expectErr: false,
},
"valid case: one poolId and gaugeId, five 5000 coins": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
"one poolId and gaugeId, five 5000 coins": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: types.DefaultConcentratedUptime,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fiveKRewardCoins),
expectErr: false,
},
"valid case: attempt to createIncentiveRecord with start time < currentBlockTime - gets set to block time in incentive record": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime.Add(-5 * time.Hour),
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
"attempt to createIncentiveRecord with start time < currentBlockTime - gets set to block time in incentive record": {
numPools: 1,
gaugeStartTime: defaultGaugeStartTime.Add(-5 * time.Hour),
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: types.DefaultConcentratedUptime,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fiveKRewardCoins),
expectErr: false,
},
// We expect incentive record to fall back to the default uptime, since the internal uptime is unauthorized.
"unauthorized internal uptime": {
numPools: 3,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: time.Hour * 24 * 7,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fifteenKRewardCoins),
},
"invalid internal uptime": {
numPools: 3,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),
internalUptime: time.Hour * 4,

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fifteenKRewardCoins),
},
"nil internal uptime": {
numPools: 3,
gaugeStartTime: defaultGaugeStartTime,
gaugeCoins: sdk.NewCoins(fiveKRewardCoins),

expectedUptime: types.DefaultConcentratedUptime,
expectedDistributions: sdk.NewCoins(fifteenKRewardCoins),
},
}

Expand All @@ -301,6 +340,9 @@ func (s *KeeperTestSuite) TestDistribute_InternalIncentives_NoLock() {
// We fix blocktime to ensure tests are deterministic
s.Ctx = s.Ctx.WithBlockTime(defaultGaugeStartTime)

// Set internal uptime module param as defined in test case
s.App.IncentivesKeeper.SetParam(s.Ctx, types.KeyInternalUptime, tc.internalUptime)

var gauges []types.Gauge

// prepare the minting account
Expand Down Expand Up @@ -334,73 +376,51 @@ func (s *KeeperTestSuite) TestDistribute_InternalIncentives_NoLock() {
gauges = append(gauges, *gauge)
}

// Distribute tokens from the gauge
// System under test: Distribute tokens from the gauge
totalDistributedCoins, err := s.App.IncentivesKeeper.Distribute(s.Ctx, gauges)
if tc.expectErr {
s.Require().Error(err)

// module account amount must stay the same
balance := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(types.ModuleName))
s.Require().Equal(coinsToMint, balance)

for i, gauge := range gauges {
for j := range gauge.Coins {
incentiveId := i*len(gauge.Coins) + j + 1

// get poolId from GaugeId
poolId, err := s.App.PoolIncentivesKeeper.GetPoolIdFromGaugeId(s.Ctx, gauge.GetId(), currentEpoch.Duration)
s.Require().NoError(err)

// check that incentive record wasn't created
_, err = s.App.ConcentratedLiquidityKeeper.GetIncentiveRecord(s.Ctx, poolId, currentEpoch.Duration, uint64(incentiveId))
s.Require().Error(err)
}
}
} else {
Copy link
Contributor Author

@AlpinYukseloglu AlpinYukseloglu Feb 6, 2024

Choose a reason for hiding this comment

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

After a couple cleanup attempts, it seems a messed up diff is unavoidable here. The core changes are:

  1. Removed error branch, as the logic covered by these cases should never be erroring (also there were no error cases)
  2. Moved the passing logic out of the else block, which caused the indentation-related diff below
  3. Updated uptime checks to be against tc.expectedUptime instead of a static default value

Note: i made the changes for 1 in a separate commit in case we want to revert. I find this version of the tests to be much less verbose & more readable, but on the fence on this.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine to me imo

s.Require().NoError(err)
s.Require().NoError(err)

// check that gauge is not empty
s.Require().NotEqual(len(gauges), 0)
// check that gauge is not empty
s.Require().NotEqual(len(gauges), 0)

// check if module amount got deducted correctly
balance := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(types.ModuleName))
for _, coin := range balance {
actualbalanceAfterDistribution := coinsToMint.AmountOf(coin.Denom).Sub(coin.Amount)
s.Require().Equal(tc.expectedDistributions.AmountOf(coin.Denom).Add(osmomath.ZeroInt()), actualbalanceAfterDistribution.Add(osmomath.ZeroInt()))
}
// check if module amount got deducted correctly
balance := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(types.ModuleName))
for _, coin := range balance {
actualbalanceAfterDistribution := coinsToMint.AmountOf(coin.Denom).Sub(coin.Amount)
s.Require().Equal(tc.expectedDistributions.AmountOf(coin.Denom).Add(osmomath.ZeroInt()), actualbalanceAfterDistribution.Add(osmomath.ZeroInt()))
}

for i, gauge := range gauges {
for j, coin := range gauge.Coins {
incentiveId := i*len(gauge.Coins) + j + 1
for i, gauge := range gauges {
for j, coin := range gauge.Coins {
incentiveId := i*len(gauge.Coins) + j + 1

gaugeId := gauge.GetId()
gaugeId := gauge.GetId()

// get poolId from GaugeId
poolId, err := s.App.PoolIncentivesKeeper.GetPoolIdFromGaugeId(s.Ctx, gaugeId, currentEpoch.Duration)
s.Require().NoError(err)
// get poolId from GaugeId
poolId, err := s.App.PoolIncentivesKeeper.GetPoolIdFromGaugeId(s.Ctx, gaugeId, currentEpoch.Duration)
s.Require().NoError(err)

// GetIncentiveRecord to see if pools received incentives properly
incentiveRecord, err := s.App.ConcentratedLiquidityKeeper.GetIncentiveRecord(s.Ctx, poolId, types.DefaultConcentratedUptime, uint64(incentiveId))
s.Require().NoError(err)
// GetIncentiveRecord to see if pools received incentives properly
incentiveRecord, err := s.App.ConcentratedLiquidityKeeper.GetIncentiveRecord(s.Ctx, poolId, tc.expectedUptime, uint64(incentiveId))
s.Require().NoError(err)

expectedEmissionRate := osmomath.NewDecFromInt(coin.Amount).QuoTruncate(osmomath.NewDec(int64(currentEpoch.Duration.Seconds())))
expectedEmissionRate := osmomath.NewDecFromInt(coin.Amount).QuoTruncate(osmomath.NewDec(int64(currentEpoch.Duration.Seconds())))

// Check that gauge distribution state is updated.
s.ValidateDistributedGauge(gaugeId, 1, tc.gaugeCoins)
// Check that gauge distribution state is updated.
s.ValidateDistributedGauge(gaugeId, 1, tc.gaugeCoins)

// check every parameter in incentiveRecord so that it matches what we created
incentiveRecordBody := incentiveRecord.GetIncentiveRecordBody()
s.Require().Equal(poolId, incentiveRecord.PoolId)
s.Require().Equal(expectedEmissionRate, incentiveRecordBody.EmissionRate)
s.Require().Equal(s.Ctx.BlockTime().UTC().String(), incentiveRecordBody.StartTime.UTC().String())
s.Require().Equal(types.DefaultConcentratedUptime, incentiveRecord.MinUptime)
s.Require().Equal(coin.Amount, incentiveRecordBody.RemainingCoin.Amount.TruncateInt())
s.Require().Equal(coin.Denom, incentiveRecordBody.RemainingCoin.Denom)
}
// check every parameter in incentiveRecord so that it matches what we created
incentiveRecordBody := incentiveRecord.GetIncentiveRecordBody()
s.Require().Equal(poolId, incentiveRecord.PoolId)
s.Require().Equal(expectedEmissionRate, incentiveRecordBody.EmissionRate)
s.Require().Equal(s.Ctx.BlockTime().UTC().String(), incentiveRecordBody.StartTime.UTC().String())
s.Require().Equal(tc.expectedUptime, incentiveRecord.MinUptime)
s.Require().Equal(coin.Amount, incentiveRecordBody.RemainingCoin.Amount.TruncateInt())
s.Require().Equal(coin.Denom, incentiveRecordBody.RemainingCoin.Denom)
}
// check the totalAmount of tokens distributed, for both lock gauges and CL pool gauges
s.Require().Equal(tc.expectedDistributions, totalDistributedCoins)
}
// check the totalAmount of tokens distributed, for both lock gauges and CL pool gauges
s.Require().Equal(tc.expectedDistributions, totalDistributedCoins)
})
}
}
Expand Down Expand Up @@ -436,6 +456,7 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
startTime time.Time
numEpochsPaidOver uint64
poolId uint64
authorizedUptime bool

// expected
expectErr bool
Expand All @@ -451,6 +472,7 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
startTime: oneHourAfterDefault,
numEpochsPaidOver: 1,
poolId: defaultCLPool,
authorizedUptime: false,
expectErr: false,

expectedDistributions: sdk.NewCoins(fiveKRewardCoins),
Expand Down Expand Up @@ -508,6 +530,18 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
return tc
}

withAuthorizedUptime := func(tc test, duration time.Duration) test {
tc.distrTo.Duration = duration
tc.authorizedUptime = true
return tc
}

withUnauthorizedUptime := func(tc test, duration time.Duration) test {
tc.distrTo.Duration = duration
tc.authorizedUptime = false
return tc
}

withError := func(tc test) test {
tc.expectErr = true
return tc
Expand All @@ -520,8 +554,24 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
"perpetual, 2 coins, paid over 1 epoch": withIsPerpetual(withGaugeCoins(defaultTest, defaultBothCoins), true),
"non-perpetual, 1 coin, paid over 2 epochs": withNumEpochs(defaultTest, 2),
"non-perpetual, 2 coins, paid over 3 epochs": withNumEpochs(withGaugeCoins(defaultTest, defaultBothCoins), 3),
"error: balancer pool id": withError(withPoolId(defaultTest, defaultBalancerPool)),
"error: inactive gauge": withError(withNumEpochs(defaultTest, 0)),

// We expect incentives with the set uptime to be created
"non-perpetual, 1 coin, paid over 1 epoch, authorized 1d uptime": withAuthorizedUptime(defaultTest, time.Hour*24),
"non-perpetual, 2 coins, paid over 3 epochs, authorized 7d uptime": withAuthorizedUptime(withNumEpochs(withGaugeCoins(defaultTest, defaultBothCoins), 3), time.Hour*24*7),
"perpetual, 2 coins, authorized 1h uptime": withAuthorizedUptime(withIsPerpetual(withGaugeCoins(defaultTest, defaultBothCoins), true), time.Hour),

// We expect incentives to fall back to default uptime of 1ns
"non-perpetual, 1 coin, paid over 1 epoch, unauthorized 1d uptime": withUnauthorizedUptime(defaultTest, time.Hour*24),
"non-perpetual, 2 coins, paid over 3 epochs, unauthorized 7d uptime": withUnauthorizedUptime(withNumEpochs(withGaugeCoins(defaultTest, defaultBothCoins), 3), time.Hour*24*7),
"perpetual, 2 coins, unauthorized 1h uptime": withUnauthorizedUptime(withIsPerpetual(withGaugeCoins(defaultTest, defaultBothCoins), true), time.Hour),

// 3h is not a valid uptime, so we expect this to fall back to 1ns
"non-perpetual, 1 coin, paid over 1 epoch, unauthorized and invalid uptime": withUnauthorizedUptime(defaultTest, time.Hour*3),
"non-perpetual, 2 coins, paid over 3 epochs, unauthorized and invalid uptime": withUnauthorizedUptime(withNumEpochs(withGaugeCoins(defaultTest, defaultBothCoins), 3), time.Hour*3),
"perpetual, 2 coins, unauthorized and invalid uptime": withUnauthorizedUptime(withIsPerpetual(withGaugeCoins(defaultTest, defaultBothCoins), true), time.Hour*3),

"error: balancer pool id": withError(withPoolId(defaultTest, defaultBalancerPool)),
"error: inactive gauge": withError(withNumEpochs(defaultTest, 0)),
}

for name, tc := range tests {
Expand All @@ -540,6 +590,13 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
// can function properly.
s.Ctx = s.Ctx.WithBlockTime(oneHourAfterDefault)

// If applicable, authorize gauge's uptime in CL module
if tc.authorizedUptime {
clParams := s.App.ConcentratedLiquidityKeeper.GetParams(s.Ctx)
clParams.AuthorizedUptimes = append(clParams.AuthorizedUptimes, tc.distrTo.Duration)
s.App.ConcentratedLiquidityKeeper.SetParams(s.Ctx, clParams)
}

// Create gauge and get it from state
externalGauge := s.createGaugeNoRestrictions(tc.isPerpertual, tc.gaugeCoins, tc.distrTo, tc.startTime, tc.numEpochsPaidOver, defaultCLPool)

Expand Down Expand Up @@ -569,9 +626,15 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
incentivesEpochDuration := s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration
incentivesEpochDurationSeconds := osmomath.NewDec(incentivesEpochDuration.Milliseconds()).QuoInt(osmomath.NewInt(1000))

// If uptime is not authorized, we expect to fall back to default
expectedUptime := types.DefaultConcentratedUptime
if tc.authorizedUptime {
expectedUptime = tc.distrTo.Duration
}

// Check that incentive records were created
for i, coin := range tc.expectedDistributions {
incentiveRecords, err := s.App.ConcentratedLiquidityKeeper.GetIncentiveRecord(s.Ctx, tc.poolId, time.Nanosecond, uint64(i+1))
incentiveRecords, err := s.App.ConcentratedLiquidityKeeper.GetIncentiveRecord(s.Ctx, tc.poolId, expectedUptime, uint64(i+1))
s.Require().NoError(err)

expectedEmissionRatePerEpoch := coin.Amount.ToLegacyDec().QuoTruncate(incentivesEpochDurationSeconds)
Expand All @@ -580,7 +643,7 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
s.Require().Equal(coin.Denom, incentiveRecords.IncentiveRecordBody.RemainingCoin.Denom)
s.Require().Equal(tc.expectedRemainingAmountIncentiveRecord[i], incentiveRecords.IncentiveRecordBody.RemainingCoin.Amount)
s.Require().Equal(expectedEmissionRatePerEpoch, incentiveRecords.IncentiveRecordBody.EmissionRate)
s.Require().Equal(time.Nanosecond, incentiveRecords.MinUptime)
s.Require().Equal(expectedUptime, incentiveRecords.MinUptime)
}

// Check that the gauge's distribution state was updated
Expand Down