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]: Use gauge duration as uptime with appropriate validation and fallback #7417

Merged
merged 5 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ 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

### Bug Fixes

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

// Validate that the gauge's corresponding uptime is authorized.
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved
authorizedUptimes := k.clk.GetParams(ctx).AuthorizedUptimes
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pass authorized uptimes as a func input param instead of retrieving it for every single gauge, this could be expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I agree that this might be expensive, but unfortunately distributeInternal is used for all gauges, not just CL gauges, so authorized uptimes as a func input would be super odd. Happy to continue discussion offline

Copy link
Member

@ValarDragon ValarDragon Feb 6, 2024

Choose a reason for hiding this comment

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

This is only in the noLock gauge (not superfluid) and can be altered state compatibly so i'm fine w/ filing this into a GH issue and doing later!

gaugeUptime := gauge.DistributeTo.Duration
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.
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 +646,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
51 changes: 47 additions & 4 deletions x/incentives/keeper/distribute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ func (s *KeeperTestSuite) TestDistribute_ExternalIncentives_NoLock() {
startTime time.Time
numEpochsPaidOver uint64
poolId uint64
authorizedUptime bool

// expected
expectErr bool
Expand All @@ -451,6 +452,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 +510,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 +534,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)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: diff seems to be rendering poorly – these are moved to the end of the block


// We expect incentives with the set uptime to be created
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of this test format due to poor readability for more complex test cases, but opted to conform to it since refactoring would be higher overhead to discuss/implement/review

"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 +570,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 +606,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
}
Comment on lines +609 to +613
Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest hardcoding the expected uptime in the test case struct as dynamically selecting the expected value in the test case logic is prone to test configuration bugs, especially, as the test case grows


// 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 +623,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