feat/test(CL Swaps): Swap fees for amount out given in #4097

p0mvn · 2023-01-25T05:11:38Z

Closes: #XXX

What is the purpose of the change

This PR introduces the implementation for swap fees when swapping for amount out given token in.

The test vectors are estimated using the Python sympy scripts. The documentation for running the scripts is added.

There are some internal methods that are untested:

It is best to test them after swap fees in the other direction are complete (ref: #4088). The reason is that we might refactor and remove these methods so it doesn't make sense to spend time adding detailed tests right now. The test coverage via script estimates is sufficient at this stage.

Lastly, there are some prints left in the codebase for development convenience. Removing them is tracked here: #4096

For context, the original implementation was prototyped here: #3893

Brief Changelog

implement swap fee for amount out given in
implement tests with swap fee for all existing edge case tests
- implement Python scripts to estimate the right values for them
refactor with better abstractions

Testing and Verifying

This change added 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

scripts/cl/common.py

x/concentrated-liquidity/swaps.go

p0mvn · 2023-01-25T05:38:23Z

scripts/cl/main.py

+
+    validate_confirmed_results(token_out_total, fee_growth_per_share_total, expected_token_out_total, expected_fee_growth_per_share_total)
+
+def main():


Note: For scripts, I suggest reviewing this file in-detail and comparing it against Go test cases. However, I don't think there is value in reviewing other internal script calculations so feel free to skip

mattverse

Left some minor comments, please take a look. Upon resolving / answered this LGTM.

(Note that I did not review the python code in detail as they were used for mathematic calculations)

x/concentrated-liquidity/fees.go

x/concentrated-liquidity/swaps.go

x/concentrated-liquidity/fees.go

mattverse · 2023-01-25T12:29:59Z

x/concentrated-liquidity/swaps.go

+func (ss *SwapState) updateFeeGrowthGlobal(feeChargeTotal sdk.Dec) {
+	if !ss.liquidity.IsZero() {
+		feeChargePerUnitOfLiquidity := feeChargeTotal.Quo(ss.liquidity)
+		// TODO: remove print.
+		fmt.Println("feeChargePerUnitOfLiquidity", feeChargePerUnitOfLiquidity)
+		ss.feeGrowthGlobal = ss.feeGrowthGlobal.Add(feeChargePerUnitOfLiquidity)
+	}


wdyt of including tests if the method is small enough in this PR?

This might change as we implement the swap in the other direction. I don't think it makes sense to test these right now given the high likelihood of these methods being changed / removed in the next PR.

Also, these are tracked here:

test(CL Fees): computeFeeChargePerStep #4087

test(CL Fees): updateFeeGrowthGlobal method #4086

x/concentrated-liquidity/swaps.go

Co-authored-by: Matt, Park <[email protected]>

scripts/cl/common.py

x/concentrated-liquidity/fees.go

x/concentrated-liquidity/swaps.go

czarcas7ic · 2023-01-26T01:28:26Z

x/concentrated-liquidity/swaps.go

+		// Charge fee
+		feeOnAmountRemainingIn := swapState.amountSpecifiedRemaining.Mul(swapFee)
+


This is probably right, but im not quite understanding the flow on first pass.

We charge a fee, then subtract that fee from the amount remaining on the swap step (this makes sense to me)

What I dont understand is why we run a seperate function computeFeeChargePerSwapStep to calculate the feeChargeTotal for the global accumulator. Why would it not just be taken from feeOnAmountRemainingIn?

(Once again, I am sure this is right, but on first pass its not clear why)

Let's define what swapState.amountSpecifiedRemaining is.

It is the amount of token in remaining over all swap steps.

After performing the current swap step, the following cases are possible:

we consumed all amount remaining

this is what we charge here:

osmosis/x/concentrated-liquidity/fees.go

Lines 292 to 295 in 3df2018

// Otherwise, the current tick had enough liquidity to fulfill the swap

// In that case, the fee is the difference between

// the amount needed to fulfill and the actual amount we ended up charging.

feeChargeTotal = amountSpecifiedRemaining.Sub(amountIn)

this is equivalent to the difference between the original amount remaining and the one actually consumed. The difference between them is the fee

we did not consume all amount remaining

then we charge this:

osmosis/x/concentrated-liquidity/fees.go

Line 290 in 3df2018

feeChargeTotal = amountIn.Mul(swapFee)

this is the fee charged on the amount actually consumed during a swap step

Price impact protection makes us exit before consuming all amount remaining

then we charge this (similar to option 2):

osmosis/x/concentrated-liquidity/fees.go

Line 290 in 3df2018

feeChargeTotal = amountIn.Mul(swapFee)

the swap fee on the amount in actually consumed before price impact protection got trigerred

Awesome! This is something I did not previously know clearly before! Would we be able to document this context somewhere?

This make sense to me, thanks! If this exact writeup could get documented somewhere that would be awesome

Docs added to architecture.md. The fee docs need to be refactored - quite outdated. I will come back to it after the swap in the other direction is done

Co-authored-by: Adam Tucker <[email protected]>

AlpinYukseloglu

Overall LGTM except one comment about chargeFee making the calc function mutative – happy to approve after that is addressed. Also left a few other minor comments

x/concentrated-liquidity/fees.go

AlpinYukseloglu · 2023-01-26T06:33:16Z

x/concentrated-liquidity/swaps.go

@@ -301,19 +318,25 @@ func (k Keeper) calcOutAmtGivenIn(ctx sdk.Context,
 			return writeCtx, sdk.Coin{}, sdk.Coin{}, sdk.Int{}, sdk.Dec{}, sdk.Dec{}, fmt.Errorf("could not convert next tick (%v) to nextSqrtPrice", nextTick)
 		}

+		// Charge fee
+		feeOnAmountRemainingIn := swapState.amountSpecifiedRemaining.Mul(swapFee)


We should specify in the comment that this is just a preliminary calculation for ComputeSwapStep and that the actual fee charging is done later by computeFeeChargePerSwapStep

Also nit: we should be rounding this up to ensure swapfee is never undercharged since Mul does bankers rounding

Done.

Rounding up at 10^-18 is done by doing MulTruncate and adding SmallestDec

AlpinYukseloglu · 2023-01-26T06:48:39Z

x/concentrated-liquidity/swaps.go

@@ -342,9 +365,13 @@ func (k Keeper) calcOutAmtGivenIn(ctx sdk.Context,
 		}
 	}

+	if err := k.chargeFee(ctx, poolId, sdk.NewDecCoinFromDec(tokenInMin.Denom, swapState.feeGrowthGlobal)); err != nil {


Doesn't this mutate state? I thought this calc method was supposed to be non-mutative

this doesn't since we are using cache ctx

That's correct, more context on how we actually make this be written to state:

osmosis/x/concentrated-liquidity/swaps.go

Lines 146 to 149 in 4d9b212

// N.B. making the call below ensures that any mutations done inside calcOutAmtGivenIn

// are written to store. If this call were skipped, calcOutAmtGivenIn would be non-mutative.

// An example of a store write done in calcOutAmtGivenIn is updating ticks as we cross them.

writeCtx()

AlpinYukseloglu · 2023-01-26T07:07:49Z

x/concentrated-liquidity/swaps.go

 	// coin amounts require int values
 	// round amountIn up to avoid under charging
-	amt0 := tokenAmountInAfterFee.Sub(swapState.amountSpecifiedRemaining).RoundInt()
+	amt0 := tokenAmountInSpecified.Sub(swapState.amountSpecifiedRemaining).RoundInt()


Just to clarify, these two are equivalent since we charge fees stepwise in amountSpecifiedRemaining instead of upfront in tokenAmountInAfterFee right?

Yes, this should be just a name change

mattverse

Thanks for replying to all the comments! LGTM 🌮

czarcas7ic

Approving since my only blocking question was answered, nicely done!

feat/test(CL Swaps): Swap fees for amount out given in

67661ec

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

p0mvn mentioned this pull request Jan 25, 2023

feat(CL): concentrated liquidity fees POC CU-86776j1g7 #3893

Closed

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

clean up

3b0a19e

p0mvn commented Jan 25, 2023

View reviewed changes

scripts/cl/common.py Outdated Show resolved Hide resolved

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

p0mvn added 3 commits January 25, 2023 00:27

Update scripts/cl/common.py

29d53be

reduce diff

265879a

clean up

8e7ba88

p0mvn commented Jan 25, 2023

View reviewed changes

p0mvn requested review from czarcas7ic, mattverse and AlpinYukseloglu January 25, 2023 05:42

p0mvn marked this pull request as ready for review January 25, 2023 05:56

mattverse reviewed Jan 25, 2023

View reviewed changes

p0mvn and others added 7 commits January 26, 2023 00:42

remove prints

b2db7d3

reduce diff

3b26bc9

clean up

360c30e

change name

34841a0

Update x/concentrated-liquidity/fees.go

cf99ea7

Co-authored-by: Matt, Park <[email protected]>

fix

a84d66d

clean up

3b8da5a

czarcas7ic reviewed Jan 26, 2023

View reviewed changes

Update scripts/cl/common.py

3df2018

Co-authored-by: Adam Tucker <[email protected]>

p0mvn requested review from czarcas7ic and mattverse January 26, 2023 06:57

AlpinYukseloglu reviewed Jan 26, 2023

View reviewed changes

comment and round up

bbf188c

p0mvn requested a review from AlpinYukseloglu January 26, 2023 08:32

mattverse approved these changes Jan 26, 2023

View reviewed changes

czarcas7ic approved these changes Jan 26, 2023

View reviewed changes

docs

3b29850

github-actions bot added the C:docs Improvements or additions to documentation label Jan 27, 2023

p0mvn merged commit 75da7ff into main Jan 27, 2023

p0mvn deleted the roman/fees-swapout-givenin branch January 27, 2023 04:05

p0mvn mentioned this pull request Jan 28, 2023

fix/test(CL Fees): swap out given in fee growth tests; clean up swap tests #4127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/test(CL Swaps): Swap fees for amount out given in #4097

feat/test(CL Swaps): Swap fees for amount out given in #4097

p0mvn commented Jan 25, 2023 •

edited

Loading

p0mvn Jan 25, 2023

mattverse left a comment

mattverse Jan 25, 2023

p0mvn Jan 25, 2023

p0mvn Jan 25, 2023 •

edited

Loading

czarcas7ic Jan 26, 2023

p0mvn Jan 26, 2023 •

edited

Loading

mattverse Jan 26, 2023

czarcas7ic Jan 26, 2023

p0mvn Jan 27, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Jan 26, 2023

AlpinYukseloglu Jan 26, 2023

p0mvn Jan 26, 2023

AlpinYukseloglu Jan 26, 2023

mattverse Jan 26, 2023

p0mvn Jan 26, 2023

AlpinYukseloglu Jan 26, 2023

p0mvn Jan 26, 2023

mattverse left a comment

czarcas7ic left a comment


		validate_confirmed_results(token_out_total, fee_growth_per_share_total, expected_token_out_total, expected_fee_growth_per_share_total)

		def main():

		// Charge fee
		feeOnAmountRemainingIn := swapState.amountSpecifiedRemaining.Mul(swapFee)

	// Otherwise, the current tick had enough liquidity to fulfill the swap
	// In that case, the fee is the difference between
	// the amount needed to fulfill and the actual amount we ended up charging.
	feeChargeTotal = amountSpecifiedRemaining.Sub(amountIn)

	// N.B. making the call below ensures that any mutations done inside calcOutAmtGivenIn
	// are written to store. If this call were skipped, calcOutAmtGivenIn would be non-mutative.
	// An example of a store write done in calcOutAmtGivenIn is updating ticks as we cross them.
	writeCtx()

feat/test(CL Swaps): Swap fees for amount out given in #4097

feat/test(CL Swaps): Swap fees for amount out given in #4097

Conversation

p0mvn commented Jan 25, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Jan 26, 2023 • edited Loading

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

AlpinYukseloglu 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

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 left a comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

p0mvn commented Jan 25, 2023 •

edited

Loading

p0mvn Jan 25, 2023 •

edited

Loading

p0mvn Jan 26, 2023 •

edited

Loading