perf: reduce supported uptimes #7651
Conversation
WalkthroughThe recent updates focus on optimizing performance by removing less utilized features and refining test scenarios. Notably, the changes eliminate one-week and two-week uptime accumulators, adjust time calculations in tests for incentives and uptime, and streamline data in genesis tests. These modifications aim to enhance efficiency and reduce unnecessary computational overhead. Changes
Assessment against linked issues
Poem
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: 7
x/concentrated-liquidity/types/constants.go (1)
- 46-47: The modification to the
SupportedUptimesarray, removing 1 Week and 2 Week durations, aligns with the PR's objective to enhance performance by eliminating underutilized accumulators. Ensure that other parts of the system that may have relied on these accumulators are also updated or verified to not be negatively impacted by this change.x/concentrated-liquidity/msg_server_test.go (1)
- 414-420: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [341-429]
Adjusting time calculations from two weeks to one day in test scenarios aligns with the removal of 1 Week and 2 Week accumulators. This ensures that the tests remain relevant to the updated system configuration. Consider reviewing the overall test coverage to ensure it remains comprehensive and covers a wide range of scenarios for robust system validation.
Verification successful
The verification process has confirmed that the concentrated liquidity module has comprehensive test coverage, including various scenarios related to position management, swaps, and incentives. This ensures that the adjustments to time calculations in test scenarios are well-supported by a robust set of tests, maintaining the integrity and relevance of the module's test suite.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases to ensure comprehensive coverage. rg 'func \(s \*KeeperTestSuite\) Test' --type goLength of output: 54056
x/concentrated-liquidity/keeper_test.go (2)
- 74-74: The change in the
ThreeUnorderedNonConsecutiveAuthorizedUptimesslice, from a week duration to a day, aligns well with the PR's objective to remove longer-duration accumulators. This should help in reducing unnecessary overhead as intended.- 71-77: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [5-5]
The use of
math/randis noted for generating randomness in tests. While this is standard practice in test environments to ensure predictability and reproducibility, it's important to usecrypto/randinstead for any security-sensitive operations requiring cryptographically secure randomness.x/concentrated-liquidity/genesis_test.go (1)
- 87-87: The reduction in the number of
testUptimeAccumRecordentries from five to four aligns with the PR's objective to remove the 1W and 2W uptime accumulators. This change is consistent with the removal of these accumulators and correctly reflects the updated system configuration in the test setup.CHANGELOG.md (1)
- 62-62: The entry for removing 1W and 2W supported uptimes for performance improvement is correctly documented. This aligns with the PR objectives and contributes to the system's optimization by eliminating underutilized functionalities.
x/incentives/keeper/distribute_test.go (1)
- 559-565: The test cases "non-perpetual, 1 coin, paid over 1 epoch, authorized 1h uptime" and "non-perpetual, 2 coins, paid over 3 epochs, authorized 1d uptime" are designed to verify the behavior of distributing incentives with authorized uptimes. However, it's crucial to ensure that the authorized uptimes are correctly set up in the system's parameters and that the gauge's distribute-to conditions match these authorized uptimes. This verification is essential to confirm that the system behaves as expected when distributing incentives based on authorized uptimes.
Additionally, the test cases "non-perpetual, 1 coin, paid over 1 epoch, unauthorized 1h uptime" and "non-perpetual, 2 coins, paid over 3 epochs, unauthorized 1d uptime" aim to test the behavior when the uptimes are not authorized. It's important to verify that the system correctly falls back to the default uptime in these scenarios and that the incentives are distributed accordingly.
Ensure that the system's parameters for authorized uptimes are correctly set up and that the gauge's distribute-to conditions accurately reflect these settings. This verification will help confirm that the system behaves as expected under both authorized and unauthorized uptime scenarios.
This reverts commit f4c546c.
This reverts commit f4c546c. Co-authored-by: Adam Tucker <[email protected]>
Closes: #7537
What is the purpose of the change
When calling updateAccumulatorsToNow, we update all 6 accumulators, despite not using the 1W and 2W accumulators. We should remove these for now, and if we decide we need these later we can implement them at that time (and maybe just enable at the per pool level, so we don't need to go through this unnecessary overhead).
Summary by CodeRabbit