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

refactor/test(CL): LP methods with lower min spot price #6323

Merged
merged 14 commits into from
Sep 8, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Sep 5, 2023

Progress towards: #6060

What is the purpose of the change

Adds CreatePosition, WithdrawPosition and AddToPosition unit tests per vectors suggested in #6060.

Refactors validateTickRangeIsValid to allow the extended full range. As a result, makes this PR state-breaking.

Disables fuzz test suite in the extended range. To be re-enabled in #6066

The test suite is made with the goal of minimizing the changes necessary while validating that positions LPed in the extended range function as intended.

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

Comment on lines 226 to 234
// TODO: remove this limit upon completion of the refactor in:
// https://github.com/osmosis-labs/osmosis/issues/5726
// Due to an intermediary refactor step where we have
// full range positions created in the extended full range it
// sometimes tries to swap to the V2 MinInitializedTick that
// is not supported yet by the rest of the system.
if targetTick < types.MinInitializedTick {
return false, false
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: this is an acceptance criteria of #6066

Comment on lines 30 to 32
// TODO: swith DefaultMinCurrentTick to types.MinCurrentTickV2 upon
// completion of https://github.com/osmosis-labs/osmosis/issues/5726
DefaultMinCurrentTick = types.MinCurrentTick
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: this is added as a sub-task for #5726

Comment on lines -118 to +121
expectedSpreadRewardGrowthOutsideLower: oneEthCoins,
expectedSpreadRewardGrowthOutsideLower: cl.EmptyCoins,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: I think this is a test setup error where this should have been empty before

@p0mvn p0mvn marked this pull request as ready for review September 6, 2023 18:01
@p0mvn p0mvn changed the title refactor/test(CL): CreatePosition with lower min spot price refactor/test(CL): LP methods with lower min spot price Sep 6, 2023
@p0mvn
Copy link
Member Author

p0mvn commented Sep 6, 2023

Please do not merge until #6319 is in

@p0mvn p0mvn force-pushed the roman/tick-to-sqrt-breaking branch from cc7ab11 to 7db80ff Compare September 8, 2023 16:40
// Note that the min sqrt price must be computed from the min tick since that's
// the assumption made in LP logic.
lowerSqrtPrice, _ := math.TickToSqrtPrice(lpTest.lowerTick)
upperSqrtPrice, _ := math.TickToSqrtPrice(lpTest.upperTick)
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting the circular reliance on internal functions to compute expected values – in the past, bugs related to ticks/price precision tended to show up in these internal functions, so we should mentally flag that low-level edge cases might be missed here

Copy link
Member Author

Choose a reason for hiding this comment

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

Followed-up in DM. Will resolve

@@ -26,6 +26,10 @@ import (
)

var (
// TODO: switch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be too late for this but I wonder if it would have been beneficial to have a special TODO for precision stuff to make it easier to be sure we don't miss any of these later (e.g. TODO (precision): so we can search for that when #5726 is done)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea!

However, I made sure that every single one is tracked in the epic though so not worried about misses:
#5864

Base automatically changed from roman/tick-to-sqrt-breaking to main September 8, 2023 17:32
@p0mvn p0mvn merged commit 013cd96 into main Sep 8, 2023
@p0mvn p0mvn deleted the roman/min-spot-price-lp branch September 8, 2023 17:57
czarcas7ic added a commit that referenced this pull request Oct 12, 2023
czarcas7ic added a commit that referenced this pull request Oct 12, 2023
czarcas7ic added a commit that referenced this pull request Oct 13, 2023
* revert 6b3273f

* Revert "refactor/test(CL): LP methods with lower min spot price (#6323)"

This reverts commit 013cd96.

* Revert "test(CL): rewards logic to work on the extended price range (#6328)"

This reverts commit 3b5d993.

* adds check back to fuzzer

* Revert "test(CL): swap to new min tick and back (#6327)"

This reverts commit e2ef44f.

* remove unused imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants