fix(CL): tick iterator bugs and tests

[p0mvn]

Closes: #5538

What is the purpose of the change

There is a bug in zero for one swap with tick updates and crossing. The PR itself has 3 overall themes listed below. All 3 are related. As a result, I'm keeping them in the same PR. If desired, I'm happy to separate "CHANGE 3" into a separate PR.

Bug 1: If we perform a swap and cross a tick. The subsequent swap would cross the same tick again and mistakenly kick in liquidity, completely invalidating pool state.

The initial guess had mistakenly led to thinking that the reason is from inclusive next tick iterator search in the zero for one direction. However, further tests helped to understand and uncover that initialization is correct. The wrong part is about updating the next tick when crossing it in zero for one direction. To maintain our active range invariant of "lower tick <= current tick < upper tick", we must update the next tick by one more unit upon crossing it in zero for one direction.

The test added here reproduced the bug.

See these for the fix in the code:

• fix(CL): tick iterator bugs and tests #5526 (comment)
• fix(CL): tick iterator bugs and tests #5526 (comment)

Bug 2: Edge case with an incorrect update of current tick when stopping swap in the middle of the bucket but close to the edge.

With more tests, it was also discovered that we incorrectly update "current tick" when ending a swap in the middle of the bucket while being extremely close to the edge. This got fixed by a "TEMPORARY HACK" code in the swaps.go. See "BUG 2" section below for details. The current understanding that our "tick to price" fixed and sqrt monotonicity will help resolve this:

• Switch to monotonic sqrt #5554
• [CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

See this for the fix in the code:

• fix(CL): tick iterator bugs and tests #5526 (comment)

Change 3: UX Change: avoid tick spacing rounding in swaps

While this change does not fix any bug, it simplifies reasoning about tick ranges. Tick spacing rounding unnecessarily complicates the logic and breaks the invariant of "tick always mapping to the appropriate sqrt price". It is broken when we are stopped inside the bucket within tick spacing. For example, assume tick spacing of 100, initialized ticks at ticks 200 and 300, and current sqrt price floating somewhere in-between and directly mapping to tick 222. If we perform tick spacing rounding, we round the current tick down to 200. As a result, current tick does not directly map to the current sqrt price anymore and vice versa.

See this for the change in the code:

• fix(CL): tick iterator bugs and tests #5526 (comment)

Testing and Verifying

TestSwapOutGivenIn_Tick_Initialization_And_Crossing tests that ticks are initialized and updated correctly
across multiple swaps. In particular, this test does 2 swaps.
For every test case, the following invariants hold:

• first swap MAY cross a tick depending on test case configuration
• second swap MUST NOT cross a tick (only swap in-between ticks)
• both swaps are in the same direction

It creates 3 positions:

• (FR) full range
• narrow range where
  • (NR1) narrow range is 2 tick spacings around the current tick
  • (NR2) narrow range'is 5 tick spacings above current tick

Position setup:

	                                                                                                     cur tick
                                                                                                                    |
narrow range 1  (NR1)                                                                         //////////
narrow range 2  (NR2)                                                                //////////////////////
full range:     (FR)         //////////////////////////////////////////////////////////////////////////////////////////
	                           MinTick                                                                                                         MaxTick

Both directions are tested. Zero for one (left). One for zero (right).

For every test case, we set it up with 1 and 100 tick spacing.

This test helped to identify 2 high severity bugs:

BUG 1. Wrong "current tick" update when swapping in zero for one (left) direction.

The repro of the bug: if we perform a swap and cross a tick, the subsequent swap in the same direction
would cross the same tick again and mistakenly kick in liquidity, completely invalidating pool state.

Initial guess was that we should not search inclusive of the current tick when initializing the next tick iterator
in zero for one direction. Otherwise, it would be possible to initialize nextTick to the already crossed currenTick.
and cross it twice. Once during the first swap, and once during the second swap.
However, when initializing the tick for the second swap, it is correct to search inclusive of the current
tick. To understand this, consider another case where we swap right (zfo) and cross tick X, then when we swap
left (ofz). In such a case, we must cross tick X in the opposite direction when going left.
Therefore, the realization concluded that we need to special case the "update of the current tick after tick crossing
when swapping in zero for one direction". In such a case, we should kick current tick 1 unit to the left (outside of
the current range before first swap). This way, during second swap, we do not cross the already crossed tick again.
While with the sequence of 2 swaps (zfo, ofz), we do cross the same tick twice as expected.

This test is set up to reproduce all of the above.

It does the first swap that stops exactly after crossing the NR1's lower tick.
Next, it continues with the second swap that we do not expect to cross any ticks
and remain in the bucket between NR2 lower tick and NR1 lower tick.

Once the second swap is completed, we validate that the liquidity in the pool corresponds to
full range position only. If the tick were to be crossed twice, we would have mistakenly
subtracted liquidity net from the lower tick of the narrow position twice, making the
current liquidity be zero.

Prior to adding this test and implementing a fix, the system would have panicked with:
"negative coin amount: -7070961745605321174329" at the end of the second swap. This stems
from the invalid liquidity of zero that is incorrectly computed due to crossing the tick twice.

Additionally, it sets up a case of swapping in one for zero direction to swap directly at the next initialzed
tick. After the swap completes, we manually validate that next tick iterators return the correct values
in both directions (left and right).

BUG 2. Banker's rounding in sqrt price to tick conversion for zero for one swap makes current tick be off by 1,
leading to tick being crossed twice.

The consequences of this bug are similar to the first where we cross the tick twice. However, in
this case the error occurs at the end of the second swap, not the first. The reason is that
second swap takes in such a small amount as to barely move the sqrt price. Then, at the end it
converts the sqrt price to tick and rounds up. Rounding up, makes current tick be equal to the
tick we already crossed (be off by 1). As a result, inactive positions are treated as active
The solution is to avoid tick update if the swap state's tick is smaller than the tick computed
from the sqrt price. That is, if we already seen a further tick, we do not update it to an earlier one.

Next steps

In 2 separate PRs, I am planning to do the following:

• once PR is ACKed, document all tick findings in the README.md
• refactor and add tests for out given in
• add tests in both swap directions with 3 consecutive ticks initialized with tick spacing of 1

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]

Note: I only validated that the tick is in the desired bucket. I did not manually calculate and verify the translation between "current tick" and "current sqrt price", as I don't think the benefits of doing that outweigh the time costs.

[ValarDragon]

This SGTM

[ValarDragon]

State machine changes look great, nice job!

Still need to review tests

[AlpinYukseloglu]

At an initial review, the high level logic seems generally sound. Will need to do another pass before approving but great job finding and fixing this.

[AlpinYukseloglu]

Nice change.

[AlpinYukseloglu]

Are there any cases where none of these branches would be hit?

[p0mvn]

			if isZeroForOne {
				if newTick <= swapState.tick {
					swapState.tick = newTick
				} else {
					fmt.Println("11111")
				}
			} else {
				if newTick > swapState.tick {
					swapState.tick = newTick
				} else {
					fmt.Println("22222")
				}
			}

Prior to merging #5541 all branches were hit. Now, only "11111" is not being hit.

I'm going to make an issue to investigate this separately. Merging for now

[ValarDragon]

Reviewed all helpers

[ValarDragon]

Reviewed the tests once, I believe they make sense to me.

I would want to do one more pass on these tests prior to release, but I don't think it makes sense to block this PR on my detailed review of them. (The vectors and helpers all make sense to me!)

[AlpinYukseloglu]

Great work, LGTM!