Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CL Incentives] Add check to only sync uptime accums when necessary #4422

Closed
Tracked by #3991
AlpinYukseloglu opened this issue Feb 25, 2023 · 4 comments
Closed
Tracked by #3991
Assignees
Labels
F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Milestone

Comments

@AlpinYukseloglu
Copy link
Contributor

Background

We currently sync uptime accumulators before any operation that uses their values (e.g. tick init, tick crossing etc.). Since a single function is technically capable of triggering many of these updates, we are likely taking on redundant computation in the average case.

Suggested Design

  • Add a check for LastLiquidityUpdate < ctx.Blocktime() before all uses of updateUptimeAccumulatorsToNow

Acceptance Criteria

  • New and existing tests should pass
@AlpinYukseloglu AlpinYukseloglu added the F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board label Feb 25, 2023
@p0mvn
Copy link
Member

p0mvn commented Feb 26, 2023

What if instead of having this function be executed in locations such as getTickInfo and crossTick, we move up this call higher in the execution flow? E.g. to updatePosition function or the swap-related functions.

That way, the accumulators will already be updated when logic that depends on the update such as getInitialUptimeGrowthOutsidesForTick is executed. At the same time, we avoid executing the same logic multiple times without the additional complexity surrounding each call to updateUptimeAccumulatorsToNow with last liquidity update checks.

Not sure if this is right but just another idea to explore - if updating these trackers is necessary for each block, maybe we have it in "begin block"?

@p0mvn
Copy link
Member

p0mvn commented Feb 26, 2023

ref: #4399 (comment)

@AlpinYukseloglu
Copy link
Contributor Author

What if instead of having this function be executed in locations such as getTickInfo and crossTick, we move up this call higher in the execution flow? E.g. to updatePosition function or the swap-related functions.

Perhaps a simpler approach would be to rename getTickInfo to getOrInitTickInfo, much like what we have in other parts of the CL package.

Not sure if this is right but just another idea to explore - if updating these trackers is necessary for each block, maybe we have it in "begin block"?

We don't necessarily need to update every tracker for every tick each block. In fact, if a pool isn't touched for 1,000 blocks, the incentive accumulators should remain unchanged – this is by design! I think the approach suggested in the issue achieves the minimum number of calls possible all things considered.

@p0mvn p0mvn added this to the CL 9 milestone Feb 28, 2023
@AlpinYukseloglu AlpinYukseloglu self-assigned this Mar 2, 2023
@AlpinYukseloglu
Copy link
Contributor Author

This was addressed in an earlier PR (we already skip accumulator updates if no time has elapsed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Projects
Archived in project
Development

No branches or pull requests

2 participants