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

feat: mitigate inverse relationship problems in swaps; more tests; clean up spot price #5002

Merged
merged 11 commits into from
Apr 25, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 25, 2023

Closes: #XXX

What is the purpose of the change

This PR is focused on truncations in the right direction as we square "square root price" in swaps.

While working on inverse relationship tests, I realized that some of the fractional residues due to intermediary math calculations are inevitable. Ref: #4998 (comment)

In swaps, what we want to guarantee is that we always round in the direction of the pool. As a result, here we introduce a method defined on the swap strategy that rounds in the desired direction based on swap strategy.

There are 2 drive-by changes here:

  • Refactoring CalculateSpotPrice keeper method to use pool's spot price implementation instead of re-implementing on its own
  • Adding more inverse relationship tick to price test vectors

Brief Changelog

  • add math.SquareRoundUp and math.SquareRoundDown
  • add SquareSqrtPrice defined on each of the swap strategies and utilizing the methods above
  • add tick to price inverse relationship tests
  • refactor CalculateSpotPrice

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

x/concentrated-liquidity/math/math.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/math/tick_test.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/swapstrategy/one_for_zero.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/swapstrategy/swap_strategy.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/swapstrategy/zero_for_one.go Outdated Show resolved Hide resolved
p0mvn and others added 5 commits April 25, 2023 11:27
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
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.

Nice work, I really like the abstractions you set up here

Base automatically changed from roman/power-fn-fix to main April 25, 2023 20:11
@p0mvn p0mvn added no-changelog V:state/compatible/no_backport State machine compatible PR, depends on prior breaks labels Apr 25, 2023
@p0mvn p0mvn merged commit e871bed into main Apr 25, 2023
@p0mvn p0mvn deleted the roman/fix-inverse-relationship-2 branch April 25, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants