[CL] Change `calcOutAmtGivenIn` and `calcInAmtGivenOut` to be non-mutative #3910

mattverse · 2023-01-03T11:03:11Z

Closes: #3889

What is the purpose of the change

This PR changes the two calc methods, namely calcOutAmtGivenIn and calcInAmtGivenOut to be non mutative methods, as previously they have been mutating ticks of the pool when upon crossing tick during iteration process of the calculation.

Also added existing checks to existing test cases that ensures that pool has not been changed after performing calculation method.

Testing and Verifying

Changed existing test to cover changes made within this PR

p0mvn · 2023-01-03T15:59:21Z

x/concentrated-liquidity/swaps.go

@@ -210,6 +210,7 @@ func (k Keeper) calcOutAmtGivenIn(ctx sdk.Context,
 	priceLimit sdk.Dec,
 	poolId uint64,
 ) (tokenIn, tokenOut sdk.Coin, updatedTick sdk.Int, updatedLiquidity, updatedSqrtPrice sdk.Dec, err error) {
+	ctx, _ = ctx.CacheContext()


We should return the second return of CacheContext from the caller (calcOutAmtGivenIn) itself. The second return is a callback to write the cache context.

The reason for this requirement is that if calcOutAmtGivenIn is called from swapOtAmtGivenIn, we actually do want to write the cache context to storage.

I also noticed that part in the original issue, but did not quite understand.

What's the reason we want to use write cache context here? Do we not store in state anyways using the results from calling the calc method anyways within the swap method?(https://github.com/osmosis-labs/osmosis/blob/main/x/concentrated-liquidity/swaps.go#L167)

I am not sure if this is what you are saying Roman, but shouldnt we pass this cache ctx as a return value and then write in the swap function that calls this? If that is what you are saying, I agree. Without doing this, despite us calling applySwap, the tick boundaries that got passed wont be updated.

Yes, that's what I meant

Gotcha, this makes sense

mattverse · 2023-01-10T12:35:59Z

@p0mvn @czarcas7ic Would it be possible to give a quick conceptACK on the direction we're going that I put in da1c910 before I make the changes further on?

p0mvn

This is what I meant in an earlier discussion

p0mvn · 2023-01-11T06:50:58Z

x/concentrated-liquidity/swaps.go

@@ -209,10 +209,11 @@ func (k Keeper) calcOutAmtGivenIn(ctx sdk.Context,
 	swapFee sdk.Dec,
 	priceLimit sdk.Dec,
 	poolId uint64,
-) (tokenIn, tokenOut sdk.Coin, updatedTick sdk.Int, updatedLiquidity, updatedSqrtPrice sdk.Dec, err error) {
+) (cacheCtx sdk.Context, tokenIn, tokenOut sdk.Coin, updatedTick sdk.Int, updatedLiquidity, updatedSqrtPrice sdk.Dec, err error) {
+	ctx, _ = ctx.CacheContext()


Suggested change

ctx, _ = ctx.CacheContext()

ctx, writeCtx := ctx.CacheContext()

p0mvn · 2023-01-11T06:51:13Z

x/concentrated-liquidity/swaps.go

@@ -335,7 +336,7 @@ func (k Keeper) calcOutAmtGivenIn(ctx sdk.Context,
 	tokenIn = sdk.NewCoin(tokenInMin.Denom, amt0)
 	tokenOut = sdk.NewCoin(tokenOutDenom, amt1)

-	return tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil
+	return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil


Suggested change

return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

Suggested change

return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

return writeCtx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

p0mvn · 2023-01-11T06:55:50Z

x/concentrated-liquidity/swaps.go

@@ -178,12 +178,12 @@ func (k Keeper) CalcOutAmtGivenIn(
 	tokenIn sdk.Coin,
 	tokenOutDenom string,
 	swapFee sdk.Dec,
-) (tokenOut sdk.Coin, err error) {
-	_, tokenOut, _, _, _, err = k.calcOutAmtGivenIn(ctx, tokenIn, tokenOutDenom, swapFee, sdk.ZeroDec(), poolI.GetId())
+) (newCtx sdk.Context, tokenOut sdk.Coin, err error) {


Suggested change

) (newCtx sdk.Context, tokenOut sdk.Coin, err error) {

) (writeCache func(), tokenOut sdk.Coin, err error) {

This reverts commit da1c910.

mattverse · 2023-01-11T12:36:25Z

@p0mvn Thanks for the clarification. I'm not knowledgable of writeCtx() function that much, but AFAIU, writeCtx works in pair with the cached context returned by ctx.CacheContext(). That is, AFAIU, only returning and calling the writeCtx function would not have effect on the actual context being used. See 1738558#diff-1a552fcc0a3d36556402345e840b658a7daccb101d7f73bca9ee2e3057ade2e6R584-R593 for example. This was why I originally tried the approach of returning the context itself. Is there something I'm misunderstanding about how writeCtx works?

p0mvn · 2023-01-11T12:47:13Z

@p0mvn Thanks for the clarification. I'm not knowledgable of writeCtx() function that much, but AFAIU, writeCtx works in pair with the cached context returned by ctx.CacheContext(). That is, AFAIU, only returning and calling the writeCtx function would not have effect on the actual context being used. See 1738558#diff-1a552fcc0a3d36556402345e840b658a7daccb101d7f73bca9ee2e3057ade2e6R584-R593 for example. This was why I originally tried the approach of returning the context itself. Is there something I'm misunderstanding about how writeCtx works?

What are you trying to test in the linked example? It seems that you're trying to assert that the pool has been updated.

However, it would not be because we write pool data to store in applySwap function:

osmosis/x/concentrated-liquidity/swaps.go

Line 146 in 794e3b1

    
           err = k.applySwap(ctx, tokenIn, tokenOut, poolId, newLiquidity, newCurrentTick, newSqrtPrice)

In fact, currently, we do not write anything to store inside calcOutAmtGivenIn and calcInAmtGivenOut. However, we should be writing ticks: #3890

This trick this writeCtx function is to make sure that we control when ticks are written to store once #3890 is implemented

mattverse · 2023-01-12T06:45:08Z

@p0mvn Thanks for the clarification. I'm not knowledgable of writeCtx() function that much, but AFAIU, writeCtx works in pair with the cached context returned by ctx.CacheContext(). That is, AFAIU, only returning and calling the writeCtx function would not have effect on the actual context being used. See 1738558#diff-1a552fcc0a3d36556402345e840b658a7daccb101d7f73bca9ee2e3057ade2e6R584-R593 for example. This was why I originally tried the approach of returning the context itself. Is there something I'm misunderstanding about how writeCtx works?

What are you trying to test in the linked example? It seems that you're trying to assert that the pool has been updated.

However, it would not be because we write pool data to store in applySwap function:

osmosis/x/concentrated-liquidity/swaps.go

Line 146 in 794e3b1

err = k.applySwap(ctx, tokenIn, tokenOut, poolId, newLiquidity, newCurrentTick, newSqrtPrice)

In fact, currently, we do not write anything to store inside calcOutAmtGivenIn and calcInAmtGivenOut. However, we should be writing ticks: #3890

This trick this writeCtx function is to make sure that we control when ticks are written to store once #3890 is implemented

I see, then do you have suggestion upon testing on state change by calling the writeCtx function as linked in my original question? If not, would pass on the testings for this specific case until PR #3890 is implemented

p0mvn · 2023-01-12T06:53:14Z

@p0mvn Thanks for the clarification. I'm not knowledgable of writeCtx() function that much, but AFAIU, writeCtx works in pair with the cached context returned by ctx.CacheContext(). That is, AFAIU, only returning and calling the writeCtx function would not have effect on the actual context being used. See 1738558#diff-1a552fcc0a3d36556402345e840b658a7daccb101d7f73bca9ee2e3057ade2e6R584-R593 for example. This was why I originally tried the approach of returning the context itself. Is there something I'm misunderstanding about how writeCtx works?

What are you trying to test in the linked example? It seems that you're trying to assert that the pool has been updated.
However, it would not be because we write pool data to store in applySwap function:

osmosis/x/concentrated-liquidity/swaps.go

Line 146 in 794e3b1

err = k.applySwap(ctx, tokenIn, tokenOut, poolId, newLiquidity, newCurrentTick, newSqrtPrice)

In fact, currently, we do not write anything to store inside calcOutAmtGivenIn and calcInAmtGivenOut. However, we should be writing ticks: #3890
This trick this writeCtx function is to make sure that we control when ticks are written to store once #3890 is implemented

I see, then do you have suggestion upon testing on state change by calling the writeCtx function as linked in my original question? If not, would pass on the testings for this specific case until PR #3890 is implemented

I think it's fine to pass testing until #3890 is implemented. Could you please make sure that we track adding these tests in #3890?

mattverse · 2023-01-12T07:39:28Z

@p0mvn made #3992 to track this. Also this PR is r4r, please feel free to give reviews for this PR

x/concentrated-liquidity/swaps_test.go

p0mvn

LGTM

x/concentrated-liquidity/swaps.go

p0mvn · 2023-01-12T07:55:03Z

x/concentrated-liquidity/swaps.go

@@ -203,16 +205,18 @@ func (k Keeper) CalcInAmtGivenOut(
 // calcOutAmtGivenIn calculates tokens to be swapped out given the provided amount and fee deducted. It also returns
 // what the updated tick, liquidity, and currentSqrtPrice for the pool would be after this swap.
 // Note this method is non-mutative, so the values returned by CalcOutAmtGivenIn do not get stored
+// Instead, we return writeCtx function so that the caller of this method can decide to write the cached ctx to store or not.


Do you mind adding spec for calcInAmtGivenOut as a drive by change in this PR please?

Just noticed it's missing

Co-authored-by: Roman <roman@osmosis.team>

p0mvn · 2023-01-12T12:13:44Z

Going to merge this as discussed offline.

We can iterate on this further in #3890 and #3992. Thanks for making the issue

Use CacheCtx for Calc method

65fd4ce

github-actions bot added the C:x/concentrated-liquidity label Jan 3, 2023

mattverse added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Jan 3, 2023

mattverse requested review from czarcas7ic and p0mvn January 3, 2023 11:06

p0mvn reviewed Jan 3, 2023

View reviewed changes

WIP: used cachedctx

da1c910

p0mvn reviewed Jan 11, 2023

View reviewed changes

mattverse added 2 commits January 11, 2023 20:09

Revert "WIP: used cachedctx"

34e4276

This reverts commit da1c910.

WIP:Write

1738558

mattverse added 2 commits January 12, 2023 15:53

Return WriteCtx for all calc methods

12c567c

Merge branch 'main' into mattverse/cl-calc-non-mutative

3328ea9

mattverse mentioned this pull request Jan 12, 2023

(CL test): Add test for ensuring writeCtx from calc methods correctly overwrites Context #3992

Closed

mattverse requested a review from p0mvn January 12, 2023 07:39

mattverse commented Jan 12, 2023

View reviewed changes

x/concentrated-liquidity/swaps_test.go Outdated Show resolved Hide resolved

Update x/concentrated-liquidity/swaps_test.go

62674a2

p0mvn approved these changes Jan 12, 2023

View reviewed changes

mattverse and others added 4 commits January 12, 2023 20:04

Update x/concentrated-liquidity/swaps.go

6d028e2

Co-authored-by: Roman <roman@osmosis.team>

Update x/concentrated-liquidity/swaps.go

94a8311

Co-authored-by: Roman <roman@osmosis.team>

Add simple spec for calcInAmtGivenOut

7b6ffb6

Fix lint

4aa2968

p0mvn approved these changes Jan 12, 2023

View reviewed changes

Merge branch 'main' into mattverse/cl-calc-non-mutative

2ea55b6

p0mvn merged commit fa47daa into main Jan 12, 2023

p0mvn deleted the mattverse/cl-calc-non-mutative branch January 12, 2023 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL] Change `calcOutAmtGivenIn` and `calcInAmtGivenOut` to be non-mutative #3910

[CL] Change `calcOutAmtGivenIn` and `calcInAmtGivenOut` to be non-mutative #3910

mattverse commented Jan 3, 2023

p0mvn Jan 3, 2023

mattverse Jan 4, 2023

czarcas7ic Jan 5, 2023

p0mvn Jan 9, 2023

mattverse Jan 10, 2023

mattverse commented Jan 10, 2023

p0mvn left a comment

p0mvn Jan 11, 2023

p0mvn Jan 11, 2023

p0mvn Jan 11, 2023

mattverse commented Jan 11, 2023

p0mvn commented Jan 11, 2023

mattverse commented Jan 12, 2023

p0mvn commented Jan 12, 2023

mattverse commented Jan 12, 2023

p0mvn left a comment

p0mvn Jan 12, 2023

p0mvn commented Jan 12, 2023

	ctx, _ = ctx.CacheContext()
	ctx, writeCtx := ctx.CacheContext()

	return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil
	return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

	return ctx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil
	return writeCtx, tokenIn, tokenOut, swapState.tick, swapState.liquidity, swapState.sqrtPrice, nil

	) (newCtx sdk.Context, tokenOut sdk.Coin, err error) {
	) (writeCache func(), tokenOut sdk.Coin, err error) {

[CL] Change calcOutAmtGivenIn and calcInAmtGivenOut to be non-mutative #3910

[CL] Change calcOutAmtGivenIn and calcInAmtGivenOut to be non-mutative #3910

Conversation

mattverse commented Jan 3, 2023

What is the purpose of the change

Testing and Verifying

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse commented Jan 10, 2023

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse commented Jan 11, 2023

p0mvn commented Jan 11, 2023

mattverse commented Jan 12, 2023

p0mvn commented Jan 12, 2023

mattverse commented Jan 12, 2023

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Jan 12, 2023

[CL] Change `calcOutAmtGivenIn` and `calcInAmtGivenOut` to be non-mutative #3910

[CL] Change `calcOutAmtGivenIn` and `calcInAmtGivenOut` to be non-mutative #3910