fix: Implement redepositing of forfeited incentives #7746
Conversation
WalkthroughThe updates primarily focus on improving the management of forfeited incentives in the concentrated liquidity module. They introduce mechanisms to redirect forfeited incentives back to active liquidity providers instead of sending them to the community pool, enhancing the fairness and efficiency of incentive distribution. This is achieved through modifications in the handling of incentives within various functions and the introduction of new methods to accurately track and redeposit forfeited incentives. Changes
Assessment against linked issues
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 (
|
…labs/osmosis into alpo/redeposit-forfeits
![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 (11)
- CHANGELOG.md (1 hunks)
- x/concentrated-liquidity/export_test.go (3 hunks)
- x/concentrated-liquidity/incentives.go (7 hunks)
- x/concentrated-liquidity/incentives_test.go (10 hunks)
- x/concentrated-liquidity/invariant_test.go (1 hunks)
- x/concentrated-liquidity/lp.go (2 hunks)
- x/concentrated-liquidity/lp_test.go (3 hunks)
- x/concentrated-liquidity/msg_server.go (1 hunks)
- x/concentrated-liquidity/msg_server_test.go (6 hunks)
- x/concentrated-liquidity/position.go (1 hunks)
- x/concentrated-liquidity/position_test.go (2 hunks)
Additional comments: 25
x/concentrated-liquidity/msg_server.go (1)
- 151-151: The addition of
forfeitedIncentivesas an additional return value in theCollectIncentivesmethod aligns with the PR's objectives. Ensure that the handling of this new return value is integrated correctly in all downstream processing and that any necessary adjustments are made to accommodate this change.Verification successful
The search results indicate that
forfeitedIncentivesis processed in various parts of the codebase, including incentive calculations, message server logic, position-related logic, and client query wrappers. This widespread integration, along with its presence in test files, suggests that the addition offorfeitedIncentiveshas been correctly integrated downstream. However, without reviewing the specific implementations in each file, the correctness of the logic cannot be fully guaranteed. The presence in test files is a positive indicator of correct implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the forfeitedIncentives are processed correctly downstream. rg "forfeitedIncentives" --type goLength of output: 2651
x/concentrated-liquidity/invariant_test.go (1)
- 95-95: The adjustment in the test code to accommodate the additional return value from the
CollectIncentivesmethod is necessary and appears to be correctly implemented. Ensure that the test logic accurately reflects the handling of the new return value and that the tests are comprehensive enough to cover scenarios involving forfeited incentives.x/concentrated-liquidity/export_test.go (2)
- 261-261: The addition of an extra return value (
[]sdk.Coins) to theCollectIncentivesfunction is consistent with the PR's objectives. Ensure that this change is reflected in all relevant calls and that the handling of the new return value is correctly implemented in the context of redepositing forfeited incentives.- 273-273: Similar to the
CollectIncentivesfunction, the addition of an extra return value ([]sdk.Coins) to thePrepareClaimAllIncentivesForPositionfunction is necessary for the feature's implementation. Verify that this change is accurately integrated into the function's logic and that the new return value is used appropriately.x/concentrated-liquidity/msg_server_test.go (2)
- 360-360: The adjustments to the
expectedMessageEventsvalues in theTestCollectIncentives_Eventsfunction reflect the changes in how forfeited incentives are handled. Ensure that these changes are consistent with the updated logic for processing incentives and that the test cases accurately represent scenarios that will be encountered in production.Also applies to: 369-369, 378-378
- 575-575: The adjustments to the
expectedMessageEventsvalues in theTestTransferPositions_Eventsfunction are in line with the PR's objectives to handle forfeited incentives more effectively. It's important to verify that these test cases accurately simulate the expected real-world behavior, especially in scenarios involving the transfer of positions with claimable incentives and spread rewards.Also applies to: 590-590, 608-608
x/concentrated-liquidity/position.go (1)
- 711-712: The addition of a new parameter to the
collectIncentivesfunction call withintransferPositionsis a critical change. It's essential to ensure that this parameter is correctly handled withincollectIncentivesto achieve the intended functionality of redepositing forfeited incentives into pool uptime accumulators. Additionally, it would be beneficial to verify that this change does not introduce any security vulnerabilities or logic errors, especially in the context of incentive distribution and handling.x/concentrated-liquidity/lp.go (2)
- 271-271: The modification to the
collectIncentivesfunction call to return additional values, including forfeited incentives, is crucial for the new feature. However, ensure that the handling of these new return values is consistent across all calls to this function to prevent potential issues in other parts of the codebase.- 289-300: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [271-297]
Overall, the changes to the
WithdrawPositionfunction to handle forfeited incentives align with the PR objectives. It's important to thoroughly test these changes, especially in edge cases such as withdrawing positions with minimal liquidity or incentives. Ensure that the redepositing process does not negatively impact the user experience or the efficiency of the incentive distribution mechanism.x/concentrated-liquidity/incentives.go (2)
- 753-788: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [756-847]
The implementation of
prepareClaimAllIncentivesForPositionintroduces a mechanism to handle forfeited incentives by scaling them down before determining if they should be forfeited or collected. This approach ensures that incentives are accurately processed based on the age of the position relative to the uptime duration. However, there are a few areas that could be refined for clarity and efficiency:
The use of
scaledForfeitedIncentivesByUptimeto track forfeited incentives in scaled form is efficient for processing but may introduce complexity when understanding the code. Consider adding more detailed comments explaining the rationale behind scaling and the process of handling forfeited incentives.The loop through each uptime accumulator (lines 803-846) performs several operations, including scaling down incentives and determining if they should be forfeited or collected. This logic is crucial for the correct handling of incentives but could benefit from further optimization or simplification to enhance readability and maintainability.
The decision to forfeit or collect incentives based on the position's age relative to the uptime duration (lines 828-846) is a key part of the incentive distribution mechanism. Ensure that this logic is thoroughly tested to prevent any potential issues with incentive distribution.
Overall, the changes made in this function align with the PR objectives of redepositing forfeited incentives and improving the incentive distribution mechanism. However, refining the implementation for clarity and efficiency could further enhance the quality of the code.
Consider optimizing the logic within the loop through each uptime accumulator for clarity and efficiency. Additionally, ensure thorough testing of the incentive distribution mechanism to prevent potential issues.
- 849-894: The
redepositForfeitedIncentivesfunction introduces logic to handle the redepositing of forfeited incentives into uptime accumulators or sending them to the owner if there's no active liquidity. This implementation is a critical part of the PR objectives and appears to be correctly implemented based on the provided context. However, there are a few considerations:
The decision to redeposit forfeited incentives or send them to the owner based on active liquidity (lines 859-892) is a significant change to the incentive distribution mechanism. Ensure that this logic is thoroughly tested, especially in edge cases where active liquidity might fluctuate around the threshold.
The scaling down of forfeited incentives before adding them to the uptime accumulator (lines 873-884) is crucial for accurate incentive distribution. Ensure that the scaling factor used is consistent with other parts of the incentive handling logic to prevent discrepancies.
The use of
scaledForfeitedIncentivesByUptimeto efficiently handle forfeited incentives without recomputing expensive operations is a good approach. However, ensure that the documentation clearly explains this process to aid future maintainability.Overall, the changes made in this function align with the PR objectives and enhance the incentive distribution mechanism by ensuring that forfeited incentives are efficiently redeposited or handled appropriately.
The implementation of
redepositForfeitedIncentivesaligns with the PR objectives and introduces a significant improvement to the incentive distribution mechanism. Ensure thorough testing and clear documentation to support future maintainability.x/concentrated-liquidity/lp_test.go (2)
- 642-649: The logic for determining if the full incentives should be claimed based on the updated pool's liquidity being less than or equal to
sdk.OneDec()seems potentially fragile. This approach assumes that a very small amount of liquidity effectively means no positions are left in the pool, which might not always be a safe assumption, especially in edge cases or future modifications to the pool's behavior.Consider verifying the intended behavior more directly, such as checking if the position being withdrawn is indeed the last position in the pool.
- 654-661: The error message formatting using
fmt.Sprintffor asserting the expected and actual balances might not provide enough context in the case of a failure. It's generally helpful to include more information about what was being tested or the conditions under which the failure occurred to make debugging easier.Consider enhancing the error message to include more details about the test scenario, such as the specific coins being checked.
CHANGELOG.md (1)
- 70-70: The short summary added under version
v23.0.7-iavl-v1clearly and succinctly describes the significant change regarding the redepositing of forfeited incentives into the pool. This update is relevant and well-documented.x/concentrated-liquidity/incentives_test.go (11)
- 3703-3703: The test case
TestScaledUpTotalIncentiveAmountcorrectly tests the functionality of scaling up the total emitted incentive amount using a predefined scaling factor. It's good practice to test both successful scaling and scenarios where scaling results in an overflow error. This ensures that the system can handle extreme cases without unexpected behavior. The use ofcl.ScaleUpTotalEmittedAmountwith different parameters to simulate these scenarios is appropriate. Ensure that the error handling for overflow scenarios is robust and that the system gracefully handles such cases without causing disruptions.- 3703-3703: The test case
TestComputeTotalIncentivesToEmiteffectively tests the functionality of computing the total incentives to emit based on a given emission rate and time elapsed. Testing scenarios with different time durations and emission rates, including a scenario where the computation results in an overflow error, is crucial for ensuring the robustness of the incentive computation logic. The use ofcl.ComputeTotalIncentivesToEmitwith varying parameters to simulate these scenarios is well done. It's important to ensure that the system can handle overflow scenarios gracefully and that the error handling is robust to prevent any disruptions in incentive distribution.- 3703-3703: The test case
TestGetIncentiveScalingFactorForPooleffectively checks the functionality of retrieving the correct scaling factor for a pool based on its ID. Testing scenarios with pool IDs below, at, and above the migration threshold, as well as a scenario with a pool ID that has an overridden scaling factor, is crucial for ensuring that the system correctly applies scaling factors to incentives. The setup for each scenario and the assertions made are appropriate for capturing the expected behavior. It's important to ensure that the logic for determining the scaling factor is robust and accurately reflects the intended behavior for both migrated and non-migrated pools, as well as any exceptions for specific pool IDs.- 3703-3703: The test case
TestScaledUpTotalIncentiveAmountcorrectly tests the functionality of scaling up the total emitted incentive amount using a predefined scaling factor. It's good practice to test both successful scaling and scenarios where scaling results in an overflow error. This ensures that the system can handle extreme cases without unexpected behavior. The use ofcl.ScaleUpTotalEmittedAmountwith different parameters to simulate these scenarios is appropriate. Ensure that the error handling for overflow scenarios is robust and that the system gracefully handles such cases without causing disruptions.- 3703-3703: The test case
TestComputeTotalIncentivesToEmiteffectively tests the functionality of computing the total incentives to emit based on a given emission rate and time elapsed. Testing scenarios with different time durations and emission rates, including a scenario where the computation results in an overflow error, is crucial for ensuring the robustness of the incentive computation logic. The use ofcl.ComputeTotalIncentivesToEmitwith varying parameters to simulate these scenarios is well done. It's important to ensure that the system can handle overflow scenarios gracefully and that the error handling is robust to prevent any disruptions in incentive distribution.- 3703-3703: The test case
TestGetIncentiveScalingFactorForPooleffectively checks the functionality of retrieving the correct scaling factor for a pool based on its ID. Testing scenarios with pool IDs below, at, and above the migration threshold, as well as a scenario with a pool ID that has an overridden scaling factor, is crucial for ensuring that the system correctly applies scaling factors to incentives. The setup for each scenario and the assertions made are appropriate for capturing the expected behavior. It's important to ensure that the logic for determining the scaling factor is robust and accurately reflects the intended behavior for both migrated and non-migrated pools, as well as any exceptions for specific pool IDs.- 3703-3703: The test case
TestScaledUpTotalIncentiveAmountcorrectly tests the functionality of scaling up the total emitted incentive amount using a predefined scaling factor. It's good practice to test both successful scaling and scenarios where scaling results in an overflow error. This ensures that the system can handle extreme cases without unexpected behavior. The use ofcl.ScaleUpTotalEmittedAmountwith different parameters to simulate these scenarios is appropriate. Ensure that the error handling for overflow scenarios is robust and that the system gracefully handles such cases without causing disruptions.- 3703-3703: The test case
TestComputeTotalIncentivesToEmiteffectively tests the functionality of computing the total incentives to emit based on a given emission rate and time elapsed. Testing scenarios with different time durations and emission rates, including a scenario where the computation results in an overflow error, is crucial for ensuring the robustness of the incentive computation logic. The use ofcl.ComputeTotalIncentivesToEmitwith varying parameters to simulate these scenarios is well done. It's important to ensure that the system can handle overflow scenarios gracefully and that the error handling is robust to prevent any disruptions in incentive distribution.- 3703-3703: The test case
TestGetIncentiveScalingFactorForPooleffectively checks the functionality of retrieving the correct scaling factor for a pool based on its ID. Testing scenarios with pool IDs below, at, and above the migration threshold, as well as a scenario with a pool ID that has an overridden scaling factor, is crucial for ensuring that the system correctly applies scaling factors to incentives. The setup for each scenario and the assertions made are appropriate for capturing the expected behavior. It's important to ensure that the logic for determining the scaling factor is robust and accurately reflects the intended behavior for both migrated and non-migrated pools, as well as any exceptions for specific pool IDs.- 3703-3703: The test case
TestScaledUpTotalIncentiveAmountcorrectly tests the functionality of scaling up the total emitted incentive amount using a predefined scaling factor. It's good practice to test both successful scaling and scenarios where scaling results in an overflow error. This ensures that the system can handle extreme cases without unexpected behavior. The use ofcl.ScaleUpTotalEmittedAmountwith different parameters to simulate these scenarios is appropriate. Ensure that the error handling for overflow scenarios is robust and that the system gracefully handles such cases without causing disruptions.- 3703-3703: The test case
TestComputeTotalIncentivesToEmiteffectively tests the functionality of computing the total incentives to emit based on a given emission rate and time elapsed. Testing scenarios with different time durations and emission rates, including a scenario where the computation results in an overflow error, is crucial for ensuring the robustness of the incentive computation logic. The use ofcl.ComputeTotalIncentivesToEmitwith varying parameters to simulate these scenarios is well done. It's important to ensure that the system can handle overflow scenarios gracefully and that the error handling is robust to prevent any disruptions in incentive distribution.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/concentrated-liquidity/incentives.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/concentrated-liquidity/incentives.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- x/concentrated-liquidity/export_test.go (3 hunks)
- x/concentrated-liquidity/incentives.go (7 hunks)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
- x/concentrated-liquidity/lp_test.go (3 hunks)
- x/concentrated-liquidity/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/concentrated-liquidity/export_test.go
- x/concentrated-liquidity/incentives.go
- x/concentrated-liquidity/incentives_test.go
- x/concentrated-liquidity/lp_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/concentrated-liquidity/incentives_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
- x/concentrated-liquidity/msg_server_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- x/concentrated-liquidity/incentives_test.go
- x/concentrated-liquidity/msg_server_test.go
AlpinYukseloglu
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/concentrated-liquidity/incentives.go (7 hunks)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/concentrated-liquidity/incentives.go
- x/concentrated-liquidity/incentives_test.go
| // To avoid descaling and rescaling, we keep the forfeited incentives in scaled form. | ||
| // This is slightly unwieldy as it means we return a slice of scaled coins, but doing it this way | ||
| // allows us to efficiently handle all cases related to forfeited incentives. | ||
| scaledForfeitedIncentivesByUptime[uptimeIndex] = collectedIncentivesForUptimeScaled |
There was a problem hiding this comment.
Ah, at first I was trying to understand why the Coins array but this makes sense!
There was a problem hiding this comment.
Yeah also we'd need to track forfeits separately regardless to know which incentives were forfeited from which accumulators, so something of this form would be necessary regardless
| // If no active liquidity, give the forfeited incentives to the sender. | ||
| if activeLiquidity.LT(sdk.OneDec()) { | ||
| err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), sender, totalForefeitedIncentives) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
You can probably ignore this comment but, why not just send to the community pool? Just thinking in the event of unexpectedly being able to trigger this as a bug I would rather funds get sent to the community pool instead of the exploiter.
There was a problem hiding this comment.
This basically makes it so the logic is "do uptime incentives if possible, but fall back to status quo incentives if not". It feels like this is the most appropriate approach, especially since this logic will be affecting external incentives as well (who probably would prefer incentivizing active MMs over sending to community pool)
Also in the case where there's a bug in related logic, imo having the pool fall back to regular incentives seems more appropriate than bricking all incentives and sending them to community pool
Co-authored-by: Adam Tucker <adam@osmosis.team>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/concentrated-liquidity/incentives.go (7 hunks)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/concentrated-liquidity/incentives.go
- x/concentrated-liquidity/incentives_test.go
| @@ -708,7 +708,8 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender | |||
| if _, err := k.collectSpreadRewards(ctx, sender, positionId); err != nil { | |||
| return err | |||
| } | |||
| if _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil { | |||
|
|
|||
| if _, _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil { | |||
There was a problem hiding this comment.
Note: properly handling forfeits for transfers will be addressed in a (very light) follow-up PR. Tracked in #7776
…labs/osmosis into alpo/redeposit-forfeits
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/concentrated-liquidity/incentives.go (7 hunks)
- x/concentrated-liquidity/incentives_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/concentrated-liquidity/incentives.go
- x/concentrated-liquidity/incentives_test.go
|
thanks coderabbit, 9 reviews should be sufficient for airdrops |
|
coderabbitai absolutely unhinged |
* implement redepositing of forfeited incentives * fix broken tests to follow new logic * changelog * convert tracker for forfeited incentives from map to slice * clean up comments * move new lp logic into helper * remove prints * clean up helper logic * clean up test comments * tests for new helper * further test cleanup * simplify test helper logic * fix tests to work with new supported uptimes * add thorough godoc for redepositing logic * apply comment fixes from review Co-authored-by: Adam Tucker <adam@osmosis.team> * further clean up comments --------- Co-authored-by: Adam Tucker <adam@osmosis.team> (cherry picked from commit 446d894)
* implement redepositing of forfeited incentives * fix broken tests to follow new logic * changelog * convert tracker for forfeited incentives from map to slice * clean up comments * move new lp logic into helper * remove prints * clean up helper logic * clean up test comments * tests for new helper * further test cleanup * simplify test helper logic * fix tests to work with new supported uptimes * add thorough godoc for redepositing logic * apply comment fixes from review Co-authored-by: Adam Tucker <adam@osmosis.team> * further clean up comments --------- Co-authored-by: Adam Tucker <adam@osmosis.team> (cherry picked from commit 446d894) Co-authored-by: Alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
Closes: #7744
What is the purpose of the change
This PR implements redepositing of forfeited incentives into pool uptime accumulators. It replaces the previous approach, which sent forfeited funds to the community pool (see issue for discussion on why this is problematic).
Testing and Verifying
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)Summary by CodeRabbit
v23.0.7-iavl-v1.InvalidForfeitedIncentivesLengthErrorto handle errors related to mismatch in expected length of forfeited incentives.