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

Jump to bottom

fix: genesis bug in pool incentives linking NoLock gauges and PoolIDs #6644

Merged
merged 7 commits into from
Oct 8, 2023
Merged

fix: genesis bug in pool incentives linking NoLock gauges and PoolIDs #6644

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1a1c939
fix: genesis bug in pool incentives
p0mvn Oct 7, 2023
7b01e24
changelog
p0mvn Oct 7, 2023
486c299
lint
p0mvn Oct 7, 2023
1184ff2
add clarifying comment
czarcas7ic Oct 8, 2023
a0ed7a0
merge main
czarcas7ic Oct 8, 2023
e6cb9b6
Revert "merge main"
czarcas7ic Oct 8, 2023
aea8009
Merge branch 'main' into roman/pool-incentives-genesis-bug
czarcas7ic Oct 8, 2023
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#6627](https://github.com/osmosis-labs/osmosis/pull/6627) Limit pow iterations in osmomath.
* [#6586](https://github.com/osmosis-labs/osmosis/pull/6586) add auth.moduleaccounts to the stargate whitelist

### Bug Fixes
* [#6644](https://github.com/osmosis-labs/osmosis/pull/6644) fix: genesis bug in pool incentives linking NoLock gauges and PoolIDs

## v19.2.0

### Misc Improvements
Expand Down
5 changes: 3 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ github.com/go-playground/locales v0.13.0/go.mod h1:taPMhCMXrRLJO55olJkUXHZBHCxTM
github.com/go-playground/locales v0.14.1 h1:EWaQ/wswjilfKLTECiXz7Rh+3BjFhfDFKv/oXslEjJA=
github.com/go-playground/universal-translator v0.17.0/go.mod h1:UkSxE5sNxxRwHyU+Scu5vgOQjsIJAF8j9muTVoKLVtA=
github.com/go-playground/universal-translator v0.18.1 h1:Bcnm0ZwsGyWbCzImXv+pAJnYK9S473LQFuzCbDbfSFY=
github.com/go-playground/universal-translator v0.18.1/go.mod h1:xekY+UJKNuX9WP91TpwSH2VMlDf28Uj24BCp08ZFTUY=
github.com/go-playground/validator/v10 v10.2.0/go.mod h1:uOYAAleCW8F/7oMFd6aG0GOhaH6EGOAJShg8Id5JGkI=
github.com/go-playground/validator/v10 v10.14.0 h1:vgvQWe3XCz3gIeFDm/HnTIbj6UGmg/+t63MyGU2n5js=
github.com/go-sourcemap/sourcemap v2.1.3+incompatible/go.mod h1:F8jJfvm2KbVjc5NqelyYJmf/v5J0dwNLS2mL4sNA1Jg=
Expand Down Expand Up @@ -797,6 +798,7 @@ github.com/ldez/tagliatelle v0.5.0/go.mod h1:rj1HmWiL1MiKQuOONhd09iySTEkUuE/8+5j
github.com/leanovate/gopter v0.2.9/go.mod h1:U2L/78B+KVFIx2VmW6onHJQzXtFb+p5y3y2Sh+Jxxv8=
github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII=
github.com/leodido/go-urn v1.2.4 h1:XlAE/cm/ms7TE/VMVoduSpNBoyc2dOxHs5MZSwAN63Q=
github.com/leodido/go-urn v1.2.4/go.mod h1:7ZrI8mTSeBSHl/UaRyKQW1qZeMgak41ANeCNaVckg+4=
github.com/leonklingele/grouper v1.1.1 h1:suWXRU57D4/Enn6pXR0QVqqWWrnJ9Osrz+5rjt8ivzU=
github.com/leonklingele/grouper v1.1.1/go.mod h1:uk3I3uDfi9B6PeUjsCKi6ndcf63Uy7snXgR4yDYQVDY=
github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
Expand Down Expand Up @@ -962,8 +964,6 @@ github.com/osmosis-labs/cosmos-sdk v0.45.0-rc1.0.20230922030206-734f99fba785 h1:
github.com/osmosis-labs/cosmos-sdk v0.45.0-rc1.0.20230922030206-734f99fba785/go.mod h1:toI9Pf+e5C4TuWAFpXfkxnkpr1RVFMK2qr7QMdkFrY8=
github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3 h1:YlmchqTmlwdWSmrRmXKR+PcU96ntOd8u10vTaTZdcNY=
github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3/go.mod h1:lV6KnqXYD/ayTe7310MHtM3I2q8Z6bBfMAi+bhwPYtI=
github.com/osmosis-labs/osmosis/osmomath v0.0.7-0.20230924192433-36cf2950dca4 h1:venI3u6DjxKcQbjiCO3V8s0ww/jx+cAZRkpwLHadms8=
github.com/osmosis-labs/osmosis/osmomath v0.0.7-0.20230924192433-36cf2950dca4/go.mod h1:IlCTpM2uoi8SUAigc9r9kAaoz7K5H9O84u7CwaTLDdY=
github.com/osmosis-labs/osmosis/osmomath v0.0.7-0.20231003135651-7418f98f1a04 h1:C8LtPGkhJxfJNj7Xtok1If5yAV53O6XG3USaOJSgeg4=
github.com/osmosis-labs/osmosis/osmomath v0.0.7-0.20231003135651-7418f98f1a04/go.mod h1:jtOM+8RJMOn5e8YIaodzvO0b8kvBcHDgtCVCmWrx6wU=
github.com/osmosis-labs/osmosis/osmoutils v0.0.7-0.20230929193736-aae32321cac7 h1:E5rwhxKEt6XOIfLkoLNiqCCFNCymJjiwMYIP+0ABMKM=
Expand Down Expand Up @@ -1238,6 +1238,7 @@ github.com/twitchyliquid64/golang-asm v0.15.1 h1:SU5vSMR7hnwNxj24w34ZyCi/FmDZTkS
github.com/twitchyliquid64/golang-asm v0.15.1/go.mod h1:a1lVb/DtPvCB8fslRZhAngC2+aY1QWCk3Cedj/Gdt08=
github.com/tyler-smith/go-bip39 v1.0.1-0.20181017060643-dbb3b84ba2ef/go.mod h1:sJ5fKU0s6JVwZjjcUEX2zFOnvq0ASQ2K9Zr6cf67kNs=
github.com/tyler-smith/go-bip39 v1.0.2/go.mod h1:sJ5fKU0s6JVwZjjcUEX2zFOnvq0ASQ2K9Zr6cf67kNs=
github.com/ugorji/go v1.1.7 h1:/68gy2h+1mWMrwZFeD1kQialdSzAb432dtpeJ42ovdo=
github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY=
Expand Down
14 changes: 12 additions & 2 deletions proto/osmosis/pool-incentives/v1beta1/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,18 @@ message GenesisState {
(gogoproto.nullable) = true,
(gogoproto.moretags) = "yaml:\"distr_info\""
];
PoolToGauges pool_to_gauges = 4 [
// any_pool_to_internal_gauges defines the gauges for any pool to internal
// pool. For every pool type (e.g. LP, Concentrated, etc), there is one such
// link
AnyPoolToInternalGauges any_pool_to_internal_gauges = 4 [
(gogoproto.nullable) = true,
(gogoproto.moretags) = "yaml:\"pool_to_gauges\""
(gogoproto.moretags) = "yaml:\"internal_pool_to_gauges\""
];
// concentrated_pool_to_no_lock_gauges defines the no lock gauges for
// concentrated pool. This only exists between concentrated pool and no lock
// gauges. Both external and internal gauges are included.
ConcentratedPoolToNoLockGauges concentrated_pool_to_no_lock_gauges = 5 [
(gogoproto.nullable) = true,
(gogoproto.moretags) = "yaml:\"concentrated_pool_to_no_lock_gauges\""
];
}
6 changes: 5 additions & 1 deletion proto/osmosis/pool-incentives/v1beta1/incentives.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ message PoolToGauge {
];
}

message PoolToGauges {
message AnyPoolToInternalGauges {
repeated PoolToGauge pool_to_gauge = 2 [ (gogoproto.nullable) = false ];
}

message ConcentratedPoolToNoLockGauges {
repeated PoolToGauge pool_to_gauge = 1 [ (gogoproto.nullable) = false ];
Comment on lines +58 to +63
Copy link
Member

@czarcas7ic czarcas7ic Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not quite sure why these need their own message types when the underlying is the same, and the name can be changed in the actual message, but will not block on this and will not change this myself in the event this was intentional and its more of a nit.,

}
51 changes: 35 additions & 16 deletions x/pool-incentives/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,28 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
} else {
k.SetDistrInfo(ctx, *genState.DistrInfo)
}
if genState.PoolToGauges != nil {
for _, record := range genState.PoolToGauges.PoolToGauge {
if record.Duration == 0 {
k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId)
} else {
if err := k.SetPoolGaugeIdInternalIncentive(ctx, record.PoolId, record.Duration, record.GaugeId); err != nil {
panic(err)
}
if genState.AnyPoolToInternalGauges != nil {
for _, record := range genState.AnyPoolToInternalGauges.PoolToGauge {
if err := k.SetPoolGaugeIdInternalIncentive(ctx, record.PoolId, record.Duration, record.GaugeId); err != nil {
panic(err)
}
}
}
if genState.ConcentratedPoolToNoLockGauges != nil {
for _, record := range genState.ConcentratedPoolToNoLockGauges.PoolToGauge {
k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId)
}
}
}

func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
distrInfo := k.GetDistrInfo(ctx)
lastPoolId := k.poolmanagerKeeper.GetNextPoolId(ctx)
lockableDurations := k.GetLockableDurations(ctx)
var poolToGauges types.PoolToGauges
var (
anyPoolToInternalGauges types.AnyPoolToInternalGauges
concentratedPoolToNoLockGauges types.ConcentratedPoolToNoLockGauges
)
for poolId := 1; poolId < int(lastPoolId); poolId++ {
pool, err := k.poolmanagerKeeper.GetPool(ctx, uint64(poolId))
if err != nil {
Expand All @@ -53,8 +57,9 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {

isCLPool := pool.GetType() == poolmanagertypes.Concentrated
if isCLPool {
// This creates a link for the internal pool gauge.
// Every CL pool has one such gauge.
incParams := k.incentivesKeeper.GetEpochInfo(ctx)

gaugeID, err := k.GetPoolGaugeId(ctx, uint64(poolId), incParams.Duration)
if err != nil {
panic(err)
Expand All @@ -63,7 +68,20 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
poolToGauge.Duration = incParams.Duration
poolToGauge.GaugeId = gaugeID
poolToGauge.PoolId = uint64(poolId)
poolToGauges.PoolToGauge = append(poolToGauges.PoolToGauge, poolToGauge)
anyPoolToInternalGauges.PoolToGauge = append(anyPoolToInternalGauges.PoolToGauge, poolToGauge)

// All concentrated pools need an additional link for the no-lock gauge.
gaugeIDs, err := k.GetNoLockGaugeIdsFromPool(ctx, uint64(poolId))
if err != nil {
panic(err)
}
for _, gaugeID := range gaugeIDs {
poolToGauge := types.PoolToGauge{
GaugeId: gaugeID,
PoolId: uint64(poolId),
}
concentratedPoolToNoLockGauges.PoolToGauge = append(concentratedPoolToNoLockGauges.PoolToGauge, poolToGauge)
}
} else {
for _, duration := range lockableDurations {
gaugeID, err := k.GetPoolGaugeId(ctx, uint64(poolId), duration)
Expand All @@ -74,15 +92,16 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
poolToGauge.Duration = duration
poolToGauge.GaugeId = gaugeID
poolToGauge.PoolId = uint64(poolId)
poolToGauges.PoolToGauge = append(poolToGauges.PoolToGauge, poolToGauge)
anyPoolToInternalGauges.PoolToGauge = append(anyPoolToInternalGauges.PoolToGauge, poolToGauge)
}
}
}

return &types.GenesisState{
Params: k.GetParams(ctx),
LockableDurations: k.GetLockableDurations(ctx),
DistrInfo: &distrInfo,
PoolToGauges: &poolToGauges,
Params: k.GetParams(ctx),
LockableDurations: k.GetLockableDurations(ctx),
DistrInfo: &distrInfo,
AnyPoolToInternalGauges: &anyPoolToInternalGauges,
ConcentratedPoolToNoLockGauges: &concentratedPoolToNoLockGauges,
}
}
81 changes: 64 additions & 17 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 @@ -9,10 +9,9 @@ import (
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

"github.com/osmosis-labs/osmosis/osmomath"
pool_incentives "github.com/osmosis-labs/osmosis/v19/x/pool-incentives"

simapp "github.com/osmosis-labs/osmosis/v19/app"

lockuptypes "github.com/osmosis-labs/osmosis/v19/x/lockup/types"
pool_incentives "github.com/osmosis-labs/osmosis/v19/x/pool-incentives"
"github.com/osmosis-labs/osmosis/v19/x/pool-incentives/types"
)

Expand All @@ -36,7 +35,7 @@ var (
},
},
},
PoolToGauges: &types.PoolToGauges{
AnyPoolToInternalGauges: &types.AnyPoolToInternalGauges{
PoolToGauge: []types.PoolToGauge{
{
PoolId: 1,
Expand All @@ -48,18 +47,14 @@ var (
GaugeId: 2,
Duration: time.Second,
},
// This duplication with zero duration
// can happen with "NoLock" gauges
// where the link containing the duration
// is used to signify that the gauge is internal
// while the link without the duration is used
// for general purpose. This redundancy is
// made for convinience of plugging in the
// later added "NoLock" gauge into the existing
// logic without having to change majority of the queries.
},
},
ConcentratedPoolToNoLockGauges: &types.ConcentratedPoolToNoLockGauges{
PoolToGauge: []types.PoolToGauge{
{
PoolId: 2,
GaugeId: 2,
PoolId: 3,
GaugeId: 3,
Duration: 0,
},
},
},
Expand Down Expand Up @@ -124,7 +119,7 @@ func (s *KeeperTestSuite) TestExportGenesis() {
s.App.PoolIncentivesKeeper.SetLockableDurations(ctx, durations)
savedDurations := s.App.PoolIncentivesKeeper.GetLockableDurations(ctx)
s.Equal(savedDurations, durations)
var expectedPoolToGauges types.PoolToGauges
var expectedPoolToGauges types.AnyPoolToInternalGauges
var gauge uint64
for _, duration := range durations {
gauge++
Expand All @@ -139,5 +134,57 @@ func (s *KeeperTestSuite) TestExportGenesis() {
s.Equal(genesisExported.Params, genesis.Params)
s.Equal(genesisExported.LockableDurations, durations)
s.Equal(genesisExported.DistrInfo, genesis.DistrInfo)
s.Equal(genesisExported.PoolToGauges, &expectedPoolToGauges)
s.Equal(genesisExported.AnyPoolToInternalGauges, &expectedPoolToGauges)
}

// This test validates that all store indexes are set correctly
// for NoLock gauges after exporting and then reimporting genesis.
func (s *KeeperTestSuite) TestImportExportGenesis_ExternalNoLock() {
s.SetupTest()

// Prepare concentrated pool
clPool := s.PrepareConcentratedPool()

// Fund account to create gauge
s.FundAcc(s.TestAccs[0], defaultCoins.Add(defaultCoins...))

// 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,
}, s.Ctx.BlockTime(), 2, clPool.GetId())
s.Require().NoError(err)

// We expect internal gauge to be created first
internalGaugeID := externalGaugeID - 1

// Export genesis
export := s.App.PoolIncentivesKeeper.ExportGenesis(s.Ctx)

// Validate that only one link for internal gauges is created
s.Require().Equal(1, len(export.AnyPoolToInternalGauges.PoolToGauge))

// Validate that 2 links, one for external and one for internal gauge, are created
Copy link
Member

Choose a reason for hiding this comment

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

Modified the comment to this as the last comment threw me off

s.Require().Equal(2, len(export.ConcentratedPoolToNoLockGauges.PoolToGauge))

// Reset state
s.SetupTest()

// Import genesis
s.App.PoolIncentivesKeeper.InitGenesis(s.Ctx, export)

// Get the general link between external gauge ID and pool
poolID, err := s.App.PoolIncentivesKeeper.GetPoolIdFromGaugeId(s.Ctx, externalGaugeID, 0)
s.Require().NoError(err)
s.Require().Equal(clPool.GetId(), poolID)

// Get the general link between internal gauge ID and pool
poolID, err = s.App.PoolIncentivesKeeper.GetPoolIdFromGaugeId(s.Ctx, internalGaugeID, 0)
s.Require().NoError(err)
s.Require().Equal(clPool.GetId(), poolID)

// Get the internal gauge
incentivesEpochDuration := s.App.IncentivesKeeper.GetEpochInfo(s.Ctx).Duration
internalGaugeIDAfterImport, err := s.App.PoolIncentivesKeeper.GetPoolGaugeId(s.Ctx, poolID, incentivesEpochDuration)
s.Require().NoError(err)
s.Require().Equal(internalGaugeID, internalGaugeIDAfterImport)
}
Loading