Revert "perf: reduce supported uptimes (#7651)" #7766
Conversation
This reverts commit f4c546c.
czarcas7ic
added
V:state/breaking
czarcas7ic
requested review from
mattverse,
p0mvn and
AlpinYukseloglu
as code owners
WalkthroughThe recent updates focus on enhancing efficiency and extending functionality within the concentrated liquidity and incentives modules. Key improvements include the removal of redundant code, expansion of supported uptimes, and adjustments in time units for better accuracy in incentive calculations. These changes aim to refine the system's performance and adaptability to different time-based scenarios, ensuring more precise and efficient operations. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- x/concentrated-liquidity/genesis_test.go (6 hunks)
- x/concentrated-liquidity/incentives_test.go (10 hunks)
- x/concentrated-liquidity/keeper_test.go (1 hunks)
- x/concentrated-liquidity/msg_server_test.go (4 hunks)
- x/concentrated-liquidity/types/constants.go (1 hunks)
- x/incentives/keeper/distribute_test.go (1 hunks)
Additional comments: 9
x/concentrated-liquidity/types/constants.go (1)
- 46-47: The change to expand the
SupportedUptimesarray to include durations like 1 week and 2 weeks aligns with the PR's objective to revert a previous commit. This increases the system's flexibility but consider adding a comment or documentation to explain the rationale behind these specific uptimes, especially if there are performance implications.x/concentrated-liquidity/msg_server_test.go (3)
- 341-341: Changing the duration from one day to two weeks in the
TestCollectIncentives_Eventstest case aligns with the PR's objective to revert previous changes and expand supported uptimes. This adjustment is crucial for accurately testing the system's behavior with the reintroduced longer uptimes.- 417-417: Adjusting the block time manipulation from one hour to one day in the
TestCollectIncentives_Eventstest case is appropriate for testing incentive claiming scenarios with the reintroduced longer uptimes. This change ensures that the tests accurately reflect the system's behavior with the expanded range of supported uptimes.- 429-429: The calculation of incentives to be sent to the community pool based on a two-week duration in the
TestCollectIncentives_Eventstest case is consistent with the PR's objective to revert previous changes and test the system's behavior with longer supported uptimes. This adjustment is necessary for accurately simulating incentive distribution over the expanded range of uptimes.x/concentrated-liquidity/keeper_test.go (1)
- 74-74: The change to
ThreeUnorderedNonConsecutiveAuthorizedUptimesfromtime.Hour * 24totime.Hour * 24 * 7correctly aligns with the PR's objective to reintroduce longer uptime durations. Ensure to verify the impact of this change on related tests and system behavior to confirm it integrates smoothly without unintended side effects.x/concentrated-liquidity/genesis_test.go (1)
- 87-94: The addition of extra
accumRecordentries totestUptimeAccumRecordreflects the increase in supported uptimes from 4 to 5. This change aligns with the PR's objective to revert a previous performance enhancement by reintroducing longer uptime durations. Ensure that the added uptime durations are correctly represented in all relevant test cases and that the logic for handling these uptimes is properly tested.CHANGELOG.md (1)
- 62-62: The removal of the duplicate CL accumulator update logic is mentioned. Ensure that this change aligns with the project's current requirements and does not affect any dependent functionalities.
x/incentives/keeper/distribute_test.go (2)
- 627-628: The test cases "non-perpetual, 1 coin, paid over 1 epoch, authorized 1d uptime" and "non-perpetual, 2 coins, paid over 3 epochs, authorized 7d uptime" are designed to verify the behavior of distributing incentives for gauges with authorized uptimes. It's crucial to ensure that the authorized uptimes are correctly recognized and applied during the distribution process. This validation is essential for maintaining the integrity of the incentives distribution based on the configured uptimes.
- 632-633: The test cases "non-perpetual, 1 coin, paid over 1 epoch, unauthorized 1d uptime" and "non-perpetual, 2 coins, paid over 3 epochs, unauthorized 7d uptime" aim to verify the fallback mechanism to the default uptime when the specified uptime is not authorized. This behavior is critical to ensure that the system gracefully handles unauthorized uptimes by reverting to a predefined default, thereby preserving the consistency and predictability of the incentives distribution process.
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(1), osmomath.NewInt(100), osmomath.NewInt(50)), | ||
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(1), osmomath.NewInt(100), osmomath.NewInt(50)), |
There was a problem hiding this comment.
The addition of two identical accumRecordWithDefinedValues calls with the same parameters in the TestInitGenesis test case seems redundant. Given the context of increasing supported uptimes, it's likely that only one additional record was intended to be added. Please verify if this duplication is intentional or if one of these lines should be removed or modified to reflect a different test scenario.
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(1), osmomath.NewInt(100), osmomath.NewInt(50)), | ||
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(1), osmomath.NewInt(100), osmomath.NewInt(50)), |
There was a problem hiding this comment.
Similar to the previous comment, the addition of two identical accumRecordWithDefinedValues calls in the TestInitGenesis test case appears to be redundant. This duplication might be an oversight. Please confirm the intention behind these additions and consider removing or altering one of them if they were not meant to be identical.
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(9), osmomath.NewInt(50), osmomath.NewInt(25)), | ||
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(9), osmomath.NewInt(50), osmomath.NewInt(25)), |
There was a problem hiding this comment.
In the TestInitGenesis test case, the addition of two identical accumRecordWithDefinedValues calls for the second pool scenario also seems redundant. This pattern of duplication is consistent with previous observations. Please review these additions to ensure they accurately represent the intended test scenarios and adjust as necessary.
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(9), osmomath.NewInt(50), osmomath.NewInt(25)), | ||
| accumRecordWithDefinedValues(accumRecord, osmomath.NewDec(9), osmomath.NewInt(50), osmomath.NewInt(25)), |
There was a problem hiding this comment.
The duplication of accumRecordWithDefinedValues calls in the TestExportGenesis test case for the second pool scenario mirrors the issues identified in the TestInitGenesis test case. This repetition may not be intentional and could potentially obscure the test's purpose. Please review and correct if necessary.
czarcas7ic
added
A:backport/v24.x
This reverts commit f4c546c.
Closes: #XXX
What is the purpose of the change
Decided to revert and come back to another time, we have external incentives using the 2w duration ATM.
Summary by CodeRabbit