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

Add sqrt price to tick function #5522

Closed
wants to merge 1 commit into from
Closed

Add sqrt price to tick function #5522

wants to merge 1 commit into from

Conversation

ValarDragon
Copy link
Member

This is an alternative to #5510 , adding this sqrt price to tick function and using comparisons off the sqrt prices, to handle errors in price -> sqrt -> price cycle.

This needs test vectors to be clear that this is doing what we need everywhere.

@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/concentrated-liquidity labels Jun 14, 2023
@ValarDragon ValarDragon added V:state/breaking State machine breaking PR A:no-changelog labels Jun 14, 2023
@AlpinYukseloglu AlpinYukseloglu mentioned this pull request Jun 15, 2023
Merged
6 tasks
mergify bot pushed a commit that referenced this pull request Jun 19, 2023
@AlpinYukseloglu
[CL]: Fix tick rounding bug and implement direct conversion from tick…
2853245 
… <> sqrt price (#5541)

Closes: #5516

> **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 
> The rest of the changes are related to function renames/test refactors.

## What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

## Testing and Verifying

The new function is tested in `math/tick_test.go`

The original attack vector is also converted into an edge case test in `position_test.go`

## 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
mattverse pushed a commit that referenced this pull request Jun 20, 2023
@AlpinYukseloglu @mattverse
[CL]: Fix tick rounding bug and implement direct conversion from tick…
d56b619 
… <> sqrt price (#5541)

Closes: #5516

> **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 
> The rest of the changes are related to function renames/test refactors.

## What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

## Testing and Verifying

The new function is tested in `math/tick_test.go`

The original attack vector is also converted into an edge case test in `position_test.go`

## 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
ValarDragon added a commit that referenced this pull request Jun 20, 2023
@mattverse @ValarDragon
@AlpinYukseloglu @p0mvn @czarcas7ic
[CL] Changes from audit results(1/2) (#5568)
0cea7aa 
* Some optimizations

* Update x/concentrated-liquidity/math/math.go

Co-authored-by: Dev Ojha <[email protected]>

* [CL]: Fix incorrect bound check/chain halt vector (#5557)

* repro panic trigger and fix bound check

* fix comments

* docs: precision issues around price; short and long term solution (#5552)

Closes: #XXX

## What is the purpose of the change

Related to: #5550

Documenting latest decisions around tick and price conversions

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## 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

* Make go tests only run if relevant (#5569)

* CL: migration functional test (#5560)

Closes: #XXX

## What is the purpose of the change

The following PR introduces a functional test that:
- Creates every type of balancer position that can be created
  - Bonded superfluid
  - Unbonded superfluid (locked)
  - Unbonded superfluid (unlocking)
  - Vanilla lock (locked)
  - Vanilla lock (unlocking)
  - No lock
- It then migrates each position, asserting invariants after each position is migrated
- Finally, an overall invariant is run after all positions have been migrated, asserting that all funds are accounted for in some way

The next PR subsequent to this will be adding randomization to the inputs in order to make the test non deterministic. 

## Testing and Verifying

Functional test above added

## 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

* [CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price (#5541)

Closes: #5516

> **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 
> The rest of the changes are related to function renames/test refactors.

## What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

## Testing and Verifying

The new function is tested in `math/tick_test.go`

The original attack vector is also converted into an edge case test in `position_test.go`

## 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

* Still fixing merge conlfict

* Revert lp.go change as commented

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: alpo <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic will be requested when the pull request is marked ready for review czarcas7ic is a code owner

@p0mvn p0mvn Awaiting requested review from p0mvn p0mvn will be requested when the pull request is marked ready for review p0mvn is a code owner

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu will be requested when the pull request is marked ready for review AlpinYukseloglu is a code owner

Assignees
No one assigned
Labels
A:no-changelog C:docs Improvements or additions to documentation C:x/concentrated-liquidity V:state/breaking State machine breaking PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ValarDragon