test/feat: incentive accumulator truncation #7395
Merged
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
p0mvn
added
the
V:state/compatible/backport
p0mvn
commented
Feb 1, 2024
Comment on lines
+251
to
+260
| ctx.EventManager().EmitEvent(sdk.NewEvent( | ||
| types.IncentiveTruncationPlaceholderName, | ||
| sdk.NewAttribute("pool_id", strconv.FormatUint(poolID, 10)), | ||
| sdk.NewAttribute("total_liq", liquidityInAccum.String()), | ||
| sdk.NewAttribute("per_unit_liq", incentivesPerLiquidity.String()), | ||
| sdk.NewAttribute("total_amt", totalEmittedAmount.String()), | ||
| )) | ||
|
|
||
| telemetry.IncrCounter(1, types.IncentiveTruncationPlaceholderName) | ||
| ctx.Logger().Error(types.IncentiveTruncationPlaceholderName, "pool_id", poolID, "total_liq", liquidityInAccum, "per_unit_liq", incentivesPerLiquidity, "total_amt", totalEmittedAmount) |
There was a problem hiding this comment.
We should create an alert for these and a Grafana dashboard cc: @niccoloraspa @alessandrolomanto
czarcas7ic
reviewed
Feb 1, 2024
| // Reset events | ||
| s.Ctx = s.Ctx.WithEventManager(sdk.NewEventManager()) | ||
|
|
||
| s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Minute * 51)) |
There was a problem hiding this comment.
Shouldn't we only need to add 1 minute here? Since we already went ahead 50 minutes above?
czarcas7ic
reviewed
Feb 1, 2024
| @@ -48,4 +48,6 @@ const ( | |||
| AttributeKeySpreadRewardGrowthOppositeDirectionOfLastTraversal = "spread_reward_growth" | |||
| AttributeKeyUptimeGrowthOppositeDirectionOfLastTraversal = "uptime_growth" | |||
| AttributeNewOwner = "new_owner" | |||
|
|
|||
| IncentiveTruncationPlaceholderName = "concentrated_liquidity_incentive_truncation" | |||
There was a problem hiding this comment.
Should we remove placeholder name?
czarcas7ic
approved these changes
Feb 1, 2024
mergify bot
pushed a commit
that referenced
this pull request
Feb 1, 2024
* test/feat: incentive accumulator truncation * updates (cherry picked from commit 8eaaae9)
p0mvn
added a commit
that referenced
this pull request
Feb 1, 2024
* test/feat: incentive accumulator truncation * updates (cherry picked from commit 8eaaae9) Co-authored-by: Roman <[email protected]>
Closed
Closed
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 shows that there is a chance of incentives being truncated due to large liquidity value.
We observed this in pool 1423 where both tokens have 18 decimal precision.
In such a case, the incentive record is updated correctly, not the global pool accumulator. The reason the pool accumulator is not updated is that it is a per unit of liquidity value (divided by liquidity). This division leads to truncation to zero.
It has been determined that no funds are at risk. The incentives are eventually distributed if either:
a) A long time without an update to the pool state occurs (at least 51 minutes with the current pool setup configuration)
b) Current tick liquidity becomes smaller
We confirmed that other pools are highly unlikely to be affected by setting a breakpoint with a debugger in-place of where the new event is emitted.
Going forward, we will have event, metric and log emitted if truncation happens.
Possible Solutions
To prevent this from happening again, we will evaluate the following approaches: