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: genesis bug in pool incentives linking NoLock gauges and PoolIDs #6644

Merged
merged 7 commits into from
Oct 8, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Oct 7, 2023

Closes: #XXX

What is the purpose of the change

Caught this bug while testing incentive distributions from the mainnet state exported edge net.

Observed the following in the logs:

4:32PM INF distributeInternal CL for pool id 1133 finished
4:32PM INF distributeInternal, CreateIncentiveRecord NoLock gauge gaugeId=34523 height=11732289 module=incentives poolId=1134 remainCoinPerEpoch={"amount":"41314029279","denom":"uosmo"}
4:32PM INF distributeInternal CL for pool id 1134 finished
4:32PM INF distributeInternal, CreateIncentiveRecord NoLock gauge gaugeId=34524 height=11732289 module=incentives poolId=1135 remainCoinPerEpoch={"amount":"108157658047","denom":"uosmo"}
4:32PM INF distributeInternal CL for pool id 1135 finished
4:32PM INF distributeInternal, CreateIncentiveRecord NoLock gauge gaugeId=34525 height=11732289 module=incentives poolId=1136 remainCoinPerEpoch={"amount":"14323634734","denom":"uosmo"}
4:32PM INF distributeInternal CL for pool id 1136 finished
4:32PM ERR no pool associated with gauge id (34562) and duration (0)
4:32PM ERR error in epoch hook no pool associated with gauge id (34562) and duration (0)
  • Noted that the gauge with this ID is an external NoLock gauge on mainnet

After investigations, realized that pool incentive store indexes are not exported correctly. Currently, we only export enough pool ID and gauge links for the internal gauges.

However, all NoLock gauges have additional store indexes to connect to a pool. These are particularly necessary for the external NoLockGauges

Solution

The solution was to:

  • continue exporting internal gauge IDs and their respective pool IDs in AnyPoolToInternalGauges genesis field
  • Introduce a new genesis entry called ConcentratedPoolToNoLockGauges that focuses on connecting CL pool IDs and their respective gauges where gauges can be both external and internal

Testing and Verifying

  • Added TestImportExportGenesis_ExternalNoLock that validates NoLock pool incentives store export and reimport in more detail

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v17.x backport patches to v17.x branch A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch labels Oct 7, 2023
@p0mvn p0mvn changed the title fix: genesis bug in pool incentives fix: genesis bug in pool incentives linking NoLock gauges and PoolIDs Oct 7, 2023
@p0mvn p0mvn added the A:backport/v16.x backport patches to v16.x branch label Oct 7, 2023
@p0mvn p0mvn marked this pull request as ready for review October 7, 2023 12:12
@p0mvn p0mvn added the A:backport/v20.x backport patches to v20.x branch label Oct 7, 2023
// 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

Comment on lines +58 to +63
message AnyPoolToInternalGauges {
repeated PoolToGauge pool_to_gauge = 2 [ (gogoproto.nullable) = false ];
}

message ConcentratedPoolToNoLockGauges {
repeated PoolToGauge pool_to_gauge = 1 [ (gogoproto.nullable) = false ];
Copy link
Member

@czarcas7ic czarcas7ic Oct 8, 2023

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.,

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Good find / change

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

This reverts commit a0ed7a0.
@czarcas7ic czarcas7ic merged commit ab93bb2 into main Oct 8, 2023
1 check passed
@czarcas7ic czarcas7ic deleted the roman/pool-incentives-genesis-bug branch October 8, 2023 02:15
mergify bot pushed a commit that referenced this pull request Oct 8, 2023
…#6644)

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go
mergify bot pushed a commit that referenced this pull request Oct 8, 2023
…#6644)

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go
mergify bot pushed a commit that referenced this pull request Oct 8, 2023
…#6644)

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go
mergify bot pushed a commit that referenced this pull request Oct 8, 2023
…#6644)

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
czarcas7ic added a commit that referenced this pull request Oct 8, 2023
… (backport #6644) (#6649)

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

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

* add default coins

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
czarcas7ic added a commit that referenced this pull request Oct 8, 2023
… (backport #6644) (#6646)

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

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go

* fix backport

* regen protos

* attempt to fix e2e

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
czarcas7ic added a commit that referenced this pull request Oct 8, 2023
… (backport #6644) (#6647)

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

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go

* Generated protofile changes

* backport fixes

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
czarcas7ic added a commit that referenced this pull request Oct 8, 2023
… (backport #6644) (#6648)

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

* fix: genesis bug in pool incentives

* changelog

* lint

* add clarifying comment

* merge main

* Revert "merge main"

This reverts commit a0ed7a0.

---------

Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit ab93bb2)

# Conflicts:
#	CHANGELOG.md
#	x/pool-incentives/keeper/genesis_test.go
#	x/pool-incentives/types/genesis.pb.go
#	x/pool-incentives/types/incentives.pb.go

* Generated protofile changes

* Update CHANGELOG.md

* Update genesis_test.go

* sdk math and unused import

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v16.x backport patches to v16.x branch A:backport/v17.x backport patches to v17.x branch A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch A:backport/v20.x backport patches to v20.x branch C:x/pool-incentives V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants