feat(CL): concentrated liquidity fees POC CU-86776j1g7 #3893
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
C:docs
mattverse
reviewed
Jan 5, 2023
x/concentrated-liquidity/fees.go
Outdated
Comment on lines
51
to
59
| feeAccumulator, err := k.getFeeAccumulator(ctx, poolId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := feeAccumulator.NewPositionCustom(owner.String(), zero, sdk.NewDecCoins(), nil); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
need to use `liquidityDelta instead of initializing them with zero values. Changed them in #
mattverse
reviewed
Jan 5, 2023
| @@ -38,8 +38,9 @@ func (s oneForZeroStrategy) ComputeSwapStep(sqrtPriceCurrent, nextSqrtPrice, liq | |||
| amountIn := math.CalcAmount1Delta(liquidity, nextSqrtPrice, sqrtPriceCurrent, false) | |||
| if amountRemaining.LT(amountIn) { | |||
| nextSqrtPrice = math.GetNextSqrtPriceFromAmount1RoundingDown(sqrtPriceCurrent, liquidity, amountRemaining) | |||
| amountIn = math.CalcAmount1Delta(liquidity, nextSqrtPrice, sqrtPriceCurrent, false) | |||
There was a problem hiding this comment.
why / where is this change coming from? Do you think we need extra tests for this?
There was a problem hiding this comment.
This is a drive-by change
It's logically equivalent to the old logic but avoids redundant recomputation in some cases.
It's unrelated to fees, I will submit this separately.
mattverse
reviewed
Jan 5, 2023
x/concentrated-liquidity/fees.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (k Keeper) getFeeGrowthOutside(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, lowerTick, upperTick int64) (sdk.DecCoins, error) { |
There was a problem hiding this comment.
owner not needed as argument. Removed in #
mattverse
reviewed
Jan 6, 2023
p0mvn
commented
Jan 11, 2023
x/concentrated-liquidity/fees.go
Outdated
Comment on lines
137
to
141
| poolFeeGrowth := poolFeeAccumulator.GetValue() | ||
|
|
||
| // calculate fee growth for upper tick and lower tick | ||
| feeGrowthAboveUpperTick := calculateFeeGrowth(upperTick, upperTickInfo.FeeGrowthOutside, currentTick, poolFeeGrowth, true) | ||
| feeGrowthBelowLowerTick := calculateFeeGrowth(lowerTick, lowerTickInfo.FeeGrowthOutside, currentTick, poolFeeGrowth, false) | ||
| feeGrowthGlobal := feeGlobalAccumulator.GetValue() | ||
|
|
||
| feeGrowthAboveUpperTick := calculateFeeGrowthAbove(upperTick, upperTickInfo.FeeGrowthOutside, currentTick, feeGrowthGlobal) | ||
| feeGrowthBelowLowerTick := calculateFeeGrowthBelow(lowerTick, lowerTickInfo.FeeGrowthOutside, currentTick, feeGrowthGlobal) |
|
Task linked: CU-86776j1g7 CL - Swap Fees |
Looks like click-up integration is connected via Sunny's account. We should consider switching to |
…protection interrupts trade early: eth -> usdc (1% fee)
github-actions
bot
removed
C:app-wiring
p0mvn
commented
Jan 24, 2023
p0mvn
commented
Jan 24, 2023
|
Everything that has been prototyped in this PR has been merged separately. The last item is here: #4097 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #XXX
What is the purpose of the change
This PR implements concentrated liquidity fees by utilizing our newly built
accummodule for claiming rewards.There are a few breaking changes to the
accumpackage made. For example, we need the ability to create, add to, and remove from a position with a custom accumulator update. In the context of concentrated liquidity, this custom accumulator update signifies the computation of fee growth outside the position. Without this change, we would be distributing rewards linearly across all positions.By providing a custom position accumulator update, we are able to propagate the fee growth within a range to it.
This PR will be split into multiple smaller PRs. Tests are to be added.
Additional Notes
I moved away from the idea of defining additional "Trackers" for ticks inside the package. Instead, tracked the per-tick accumulators directly in tick structs.
I think I was able to mitigate the problem that we were discussing earlier. The one about accum module distributing rewards linearly with the number of shares without accounting for whether a position range was ever activated.
It is mitigated by introducing an ability to provide custom updates to the position's accumulators. This way, we should be able to compute fee growth outside the range and, then, inject it into the accum module's position.
This custom "fee growth outside" position accumulator can then be used directly by the accum module's ClaimRewards function since it would be subtracted from pool's "fee growth global" to get position's "fee growth inside" and multiply it by the number of shares inside that position to get final result.