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

fix: check denom routes at time of gauge creation #8136

Merged
merged 13 commits into from
Apr 26, 2024
6 changes: 3 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### State Breaking

* [#7005](https://github.com/osmosis-labs/osmosis/pull/7005) Adding deactivated smart account module.
* [#8106](https://github.com/osmosis-labs/osmosis/pull/8106) Enable ProtoRev distro on epoch
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
* [#8053](https://github.com/osmosis-labs/osmosis/pull/8053) Reset validator signing info missed blocks counter
* [#8106](https://github.com/osmosis-labs/osmosis/pull/8106) Enable ProtoRev distro on epoch.
* [#8030](https://github.com/osmosis-labs/osmosis/pull/8030) Delete legacy behavior where lockups could not unbond at very small block heights on a testnet.
* [#8073](https://github.com/osmosis-labs/osmosis/pull/8073) Speedup CL spread factor calculations, but mildly changes rounding behavior in the final decimal place.
* [#8125](https://github.com/osmosis-labs/osmosis/pull/8125) When using smart accounts, fees are deducted directly after the feePayer is authenticated. Regardless of the authentication of other signers.
* [#8125](https://github.com/osmosis-labs/osmosis/pull/8125) When using smart accounts, fees are deducted directly after the feePayer is authenticated. Regardless of the authentication of other signers
* [#8136](https://github.com/osmosis-labs/osmosis/pull/8136) Don't allow gauge creation/addition with rewards that have no protorev route (i.e. no way to determine if rewards meet minimum epoch value distribution requirements)
* [#8144](https://github.com/osmosis-labs/osmosis/pull/8144) IBC wasm clients can now make stargate queries and support abort.

### State Compatible
Expand Down
13 changes: 13 additions & 0 deletions x/incentives/keeper/distribute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/osmosis-labs/osmosis/osmomath"
"github.com/osmosis-labs/osmosis/osmoutils/coinutil"
appParams "github.com/osmosis-labs/osmosis/v24/app/params"
appparams "github.com/osmosis-labs/osmosis/v24/app/params"
"github.com/osmosis-labs/osmosis/v24/x/incentives/types"
incentivetypes "github.com/osmosis-labs/osmosis/v24/x/incentives/types"
lockuptypes "github.com/osmosis-labs/osmosis/v24/x/lockup/types"
Expand Down Expand Up @@ -1196,6 +1197,12 @@ func (s *KeeperTestSuite) TestFunctionalInternalExternalCLGauge() {
halfOfExternalGaugeCoins = sdk.NewCoins(sdk.NewCoin("eth", osmomath.NewInt(defaultExternalGaugeValue/numEpochsPaidOverGaugeTwo)), sdk.NewCoin("usdc", osmomath.NewInt(defaultExternalGaugeValue/numEpochsPaidOverGaugeTwo))) // distributed at each epoch for non-perp gauge with numEpoch = 2
)

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
for _, coin := range append(externalGaugeCoins, internalGaugeCoins...) {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}

s.FundAcc(s.TestAccs[1], requiredBalances)
s.FundAcc(s.TestAccs[2], requiredBalances)
s.FundModuleAcc(incentivetypes.ModuleName, requiredBalances)
Expand Down Expand Up @@ -1346,6 +1353,12 @@ func (s *KeeperTestSuite) TestFunctionalInternalExternalCLGauge() {
}

func (s *KeeperTestSuite) CreateNoLockExternalGauges(clPoolId uint64, externalGaugeCoins sdk.Coins, gaugeCreator sdk.AccAddress, numEpochsPaidOver uint64) uint64 {
// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
for _, coin := range externalGaugeCoins {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}

// Create 1 external no-lock gauge perpetual over 1 epochs MsgCreateGauge
clPoolExternalGaugeId, err := s.App.IncentivesKeeper.CreateGauge(s.Ctx, numEpochsPaidOver == 1, gaugeCreator, externalGaugeCoins,
lockuptypes.QueryCondition{
Expand Down
4 changes: 4 additions & 0 deletions x/incentives/keeper/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,7 @@ func (k Keeper) GetNoLockGaugeUptime(ctx sdk.Context, gauge types.Gauge, poolId
func (k Keeper) SkipSpamGaugeDistribute(ctx sdk.Context, locks []*lockuptypes.PeriodLock, gauge types.Gauge, totalDistrCoins sdk.Coins, remainCoins sdk.Coins) (bool, sdk.Coins, error) {
return k.skipSpamGaugeDistribute(ctx, locks, gauge, totalDistrCoins, remainCoins)
}

func (k Keeper) CheckIfDenomsAreDistributable(ctx sdk.Context, coins sdk.Coins) error {
return k.checkIfDenomsAreDistributable(ctx, coins)
}
30 changes: 30 additions & 0 deletions x/incentives/keeper/gauge.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/osmosis-labs/osmosis/osmomath"
appparams "github.com/osmosis-labs/osmosis/v24/app/params"
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this file are the only actual changes. All other changes are just fixing tests to register routes so they don't fail at this step (which is good / expected behavior).

"github.com/osmosis-labs/osmosis/v24/x/incentives/types"
lockuptypes "github.com/osmosis-labs/osmosis/v24/x/lockup/types"
poolmanagertypes "github.com/osmosis-labs/osmosis/v24/x/poolmanager/types"
Expand Down Expand Up @@ -127,6 +128,15 @@ func (k Keeper) CreateGauge(ctx sdk.Context, isPerpetual bool, owner sdk.AccAddr
return 0, types.ErrZeroNumEpochsPaidOver
}

// Check that the coins being sent to the gauge exist as a skip hot route
// This is used to determine the underlying value of the rewards per user at epoch,
// since we don't distribute tokens values under a certain threshold.
// If the denom doesn't exist in the skip hot route, we would never distribute rewards
// from this gauge.
if err := k.checkIfDenomsAreDistributable(ctx, coins); err != nil {
return 0, err
}

// 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
Expand Down Expand Up @@ -413,6 +423,10 @@ func (k Keeper) AddToGaugeRewards(ctx sdk.Context, owner sdk.AccAddress, coins s
// Notes: does not do token transfers since it is used internally for token transferring value within the
// incentives module or by higher level functions that do transfer.
func (k Keeper) addToGaugeRewards(ctx sdk.Context, coins sdk.Coins, gaugeID uint64) error {
if err := k.checkIfDenomsAreDistributable(ctx, coins); err != nil {
return err
}

gauge, err := k.GetGaugeByID(ctx, gaugeID)
if err != nil {
return err
Expand Down Expand Up @@ -457,3 +471,19 @@ func (k Keeper) chargeFeeIfSufficientFeeDenomBalance(ctx sdk.Context, address sd
}
return nil
}

// checkIfDenomsAreDistributable checks if the denoms in the provided coins are registered in protorev.
// It iterates over the coins and for each coin, it tries to get the pool for the denom pair with "uosmo".
// If the pool does not exist, it returns an error indicating that the denom does not exist as a protorev hot route.
// If all denoms are valid, it returns nil.
func (k Keeper) checkIfDenomsAreDistributable(ctx sdk.Context, coins sdk.Coins) error {
for _, coin := range coins {
if coin.Denom != appparams.BaseCoinUnit {
_, err := k.prk.GetPoolForDenomPairNoOrder(ctx, coin.Denom, appparams.BaseCoinUnit)
if err != nil {
return types.NoRouteForDenomError{Denom: coin.Denom}
}
}
}
return nil
}
123 changes: 122 additions & 1 deletion x/incentives/keeper/gauge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/osmosis-labs/osmosis/osmomath"
"github.com/osmosis-labs/osmosis/osmoutils"
appparams "github.com/osmosis-labs/osmosis/v24/app/params"
incentiveskeeper "github.com/osmosis-labs/osmosis/v24/x/incentives/keeper"
"github.com/osmosis-labs/osmosis/v24/x/incentives/types"
lockuptypes "github.com/osmosis-labs/osmosis/v24/x/lockup/types"
Expand Down Expand Up @@ -54,6 +55,12 @@ var (
func (s *KeeperTestSuite) TestInvalidDurationGaugeCreationValidation() {
s.SetupTest()

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
for _, coin := range defaultLiquidTokens {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}

addrs := s.SetupManyLocks(1, defaultLiquidTokens, defaultLPTokens, defaultLockDuration)
distrTo := lockuptypes.QueryCondition{
LockQueryType: lockuptypes.ByDuration,
Expand All @@ -72,6 +79,12 @@ func (s *KeeperTestSuite) TestInvalidDurationGaugeCreationValidation() {
func (s *KeeperTestSuite) TestNonExistentDenomGaugeCreation() {
s.SetupTest()

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
for _, coin := range defaultLiquidTokens {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}

addrNoSupply := sdk.AccAddress([]byte("Gauge_Creation_Addr_"))
addrs := s.SetupManyLocks(1, defaultLiquidTokens, defaultLPTokens, defaultLockDuration)
distrTo := lockuptypes.QueryCondition{
Expand Down Expand Up @@ -397,7 +410,8 @@ func (s *KeeperTestSuite) TestAddToGaugeRewards() {
gaugeId uint64
minimumGasConsumed uint64

expectErr bool
skipSettingRoute bool
expectErr bool
}{
{
name: "valid case: valid gauge",
Expand Down Expand Up @@ -441,11 +455,33 @@ func (s *KeeperTestSuite) TestAddToGaugeRewards() {

expectErr: true,
},
{
name: "invalid case: valid gauge, but errors due to no protorev route",
owner: s.TestAccs[0],
coinsToAdd: sdk.NewCoins(
sdk.NewCoin("uosmo", osmomath.NewInt(100000)),
sdk.NewCoin("atom", osmomath.NewInt(99999)),
),
gaugeId: 1,
minimumGasConsumed: uint64(2 * types.BaseGasFeeForAddRewardToGauge),

skipSettingRoute: true,
expectErr: true,
},
}

for _, tc := range testCases {
s.Run(tc.name, func() {
s.SetupTest()

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
if !tc.skipSettingRoute {
for _, coin := range tc.coinsToAdd {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}
}

_, _, existingGaugeCoins, _ := s.SetupNewGauge(true, defaultCoins)

s.FundAcc(tc.owner, tc.coinsToAdd)
Expand Down Expand Up @@ -500,6 +536,7 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {

expectedGaugeId uint64
expectedDenomSet string
skipSettingRoute bool
expectErr bool
}{
{
Expand All @@ -516,6 +553,21 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
expectedDenomSet: types.NoLockExternalGaugeDenom(concentratedPoolId),
expectErr: false,
},
{
name: "create valid no lock gauge with CL pool, but errors due to no protorev route",
distrTo: lockuptypes.QueryCondition{
LockQueryType: lockuptypes.NoLock,
// Note: this assumes the gauge is external
Denom: "",
Duration: time.Nanosecond,
},
poolId: concentratedPoolId,

expectedGaugeId: defaultExpectedGaugeId,
expectedDenomSet: types.NoLockExternalGaugeDenom(concentratedPoolId),
skipSettingRoute: true,
expectErr: true,
},
{
name: "create valid no lock gauge with CL pool (denom set to no lock internal prefix)",
distrTo: lockuptypes.QueryCondition{
Expand Down Expand Up @@ -615,6 +667,14 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
s.Run(tc.name, func() {
s.SetupTest()

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
if !tc.skipSettingRoute {
for _, coin := range defaultGaugeCreationCoins {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}
}

s.PrepareBalancerPool()
s.PrepareConcentratedPool()

Expand Down Expand Up @@ -667,6 +727,7 @@ func (s *KeeperTestSuite) TestCreateGauge_Group() {

expectedGaugeId uint64
expectedDenomSet string
skipSettingRoute bool
expectErr error
}{
{
Expand Down Expand Up @@ -699,13 +760,31 @@ func (s *KeeperTestSuite) TestCreateGauge_Group() {

expectErr: types.ErrZeroNumEpochsPaidOver,
},
{
name: "create valid non-perpetual group gauge, but errors due to no protorev route",
distrTo: incentiveskeeper.ByGroupQueryCondition,
poolId: zeroPoolId,
isPerpetual: false,
numEpochsPaidOver: types.PerpetualNumEpochsPaidOver + 1,

skipSettingRoute: true,
expectErr: types.NoRouteForDenomError{Denom: "atom"},
},
}

for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
s.SetupTest()

// Since this test creates or adds to a gauge, we need to ensure a route exists in protorev hot routes.
// The pool doesn't need to actually exist for this test, so we can just ensure the denom pair has some entry.
if !tc.skipSettingRoute {
for _, coin := range defaultGaugeCreationCoins {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}
}

s.PrepareBalancerPool()
s.PrepareConcentratedPool()

Expand Down Expand Up @@ -834,3 +913,45 @@ func (s *KeeperTestSuite) validateNoGaugeIDInSlice(slice []types.Gauge, gaugeID
// No gauge matched ID.
s.Require().Empty(gaugeMatch)
}

func (s *KeeperTestSuite) TestCheckIfDenomsAreDistributable() {
s.SetupTest()

coinWithRouteA := sdk.NewCoin("denom1", sdk.NewInt(100))
coinWithRouteB := sdk.NewCoin("denom2", sdk.NewInt(100))
coinWithoutRouteC := sdk.NewCoin("denom3", sdk.NewInt(100))

for _, coin := range []sdk.Coin{coinWithRouteA, coinWithRouteB} {
s.App.ProtoRevKeeper.SetPoolForDenomPair(s.Ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}

testCases := []struct {
name string
coins sdk.Coins
expectedErr error
}{
{
name: "valid case: all denoms are distributable",
coins: sdk.NewCoins(coinWithRouteA, coinWithRouteB),
},
{
name: "invalid case: one denom is not distributable",
coins: sdk.NewCoins(coinWithRouteA, coinWithoutRouteC),
expectedErr: types.NoRouteForDenomError{Denom: coinWithoutRouteC.Denom},
},
}

for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
// System under test
err := s.App.IncentivesKeeper.CheckIfDenomsAreDistributable(s.Ctx, tc.coins)

if tc.expectedErr != nil {
s.Require().Error(err)
} else {
s.Require().NoError(err)
}
})
}
}
4 changes: 4 additions & 0 deletions x/incentives/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/osmosis-labs/osmosis/osmomath"
osmoapp "github.com/osmosis-labs/osmosis/v24/app"
appparams "github.com/osmosis-labs/osmosis/v24/app/params"

"github.com/osmosis-labs/osmosis/v24/x/concentrated-liquidity/model"
"github.com/osmosis-labs/osmosis/v24/x/incentives/types"
Expand Down Expand Up @@ -230,6 +231,9 @@ func TestIncentivesInitGenesis(t *testing.T) {
}

func createAllGaugeTypes(t *testing.T, app *osmoapp.OsmosisApp, ctx sdk.Context, addr sdk.AccAddress, coins sdk.Coins, startTime time.Time) {
for _, coin := range coins {
app.ProtoRevKeeper.SetPoolForDenomPair(ctx, appparams.BaseCoinUnit, coin.Denom, 9999)
}
// create a byDuration gauge
_, err := app.IncentivesKeeper.CreateGauge(ctx, true, addr, coins, distrToByDuration, startTime, 1, 0)
require.NoError(t, err)
Expand Down
Loading