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/test/docs: negative interval accumulation with incentive rewards #5872

Merged
merged 12 commits into from
Aug 1, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jul 18, 2023

Progress towards: #5854

What is the purpose of the change

Please review #5869 first for better context and easier review experience.

This PR fixes a negative interval accumulation edge case bug by simply replacing Sub with SafeSub where applicable. It introduces new coming helpers in osmoutils to achieve that.

The tests are extended to cover claiming rewards below, inside, and above the position after it is initialized.

Note, I noticed another edge case around negative values while working on this. Added docs to explain it. It is covered by current test

Testing and Verifying

The global invariants are asserted, and the tests ensure that the appropriate amount is claimed from all positions in all cases.

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 the V:state/breaking State machine breaking PR label Jul 18, 2023
@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/concentrated-liquidity labels Jul 18, 2023
@p0mvn p0mvn marked this pull request as ready for review July 18, 2023 17:20
@p0mvn p0mvn requested a review from czarcas7ic as a code owner July 18, 2023 17:20
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.

Needs second ACK due to change with incentives.go, but LGTM

Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

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

Core changes LGTM! Did slightly more cursory review of tests but they generally look good (will try to do another pass later to make sure the changes are covered but shouldn't be blocking)

Base automatically changed from roman/fix-neg-range-repro to main July 31, 2023 17:17
@ValarDragon
Copy link
Member

I think theres some merge conflict issues with the other PR, PR LGTM assuming these are resolved

@czarcas7ic czarcas7ic merged commit 97f4901 into main Aug 1, 2023
@czarcas7ic czarcas7ic deleted the roman/fix-neg-range-repro-incentives branch August 1, 2023 23:05
@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
C:docs Improvements or additions to documentation C:x/concentrated-liquidity V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants