[CL]: Fix low level tick rounding to mitigate fund drain bug #5510
Conversation
| @@ -411,14 +411,14 @@ var ( | |||
| spreadFactor: sdk.ZeroDec(), | |||
| // params | |||
| // liquidity: 1517882343.751510418088349649 | |||
| // sqrtPriceNext: 70.668238976219012613 which is 4994 https://www.wolframalpha.com/input?i=70.710678118654752440+%2B+42000000+%2F+1517882343.751510418088349649 | |||
There was a problem hiding this comment.
Note to reviewer: this test vector seemed to have been incorrectly set up (previous wolfram link went to an unrelated equation)
| tickIndex := CalculatePriceToTick(price) | ||
| tickIndex, err := CalculatePriceToTick(price) | ||
| if err != nil { | ||
| return 0, err |
There was a problem hiding this comment.
Note to reviewer: returning 0 here isn't ideal, but it seems to be what we do in all other tick logic upon error. If there's a better value to return here that's more clearly invalid (e.g. an sdk.Int{} equivalent) please lmk
| // If price is lower than the price in the tick below, our error assumption is violated so we panic. | ||
| if price.LT(tickBelowPrice) { | ||
| panic(fmt.Sprintf("price %s is outside of error bounds for tick %d", price, tickIndex)) | ||
| } | ||
|
|
||
| // If price falls in the bucket below `tickIndex`, return that bucket index. | ||
| // Otherwise, return the current tick index. | ||
| if price.GTE(tickBelowPrice) && price.LT(curPrice) { | ||
| return tickIndex - 1, nil | ||
| } |
There was a problem hiding this comment.
I think we should make a new SqrtPriceToTick function, that does the squaring internally, then does this comparison on the derived sqrt prices. As the problem is our price definition doesn't match due to errors.
(I don't think this on its own will save us from anything aside from the erroneous bucket definition due to not truncating)
|
Closing in favor of #5541 |
Closes: #5516
What is the purpose of the change
This PR implements the discussed fix to the vector outlined in #5493. It currently breaks tick invariant tests, which are skipped in this PR until we converge on how to handle tick <> sqrt price conversions in #5519.
This PR also renames
PriceToTickRoundDowntoPriceToTickRoundDownSpacing, as the original naming caused some confusion around how this bug could be possible in the first place.Testing and Verifying
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)