fix: Implement retaining rewards for transfers #7785
Conversation
WalkthroughThis update primarily focuses on refining the handling of incentives during position transfers. It eliminates the collection of rewards at the time of transferring positions, ensuring that incentives remain with the positions and are not prematurely distributed. This change aligns with the goal of managing incentives more effectively and avoiding past issues related to their distribution. 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/concentrated-liquidity/msg_server_test.go (3 hunks)
- x/concentrated-liquidity/position.go (1 hunks)
- x/concentrated-liquidity/position_test.go (1 hunks)
Additional comments: 27
x/concentrated-liquidity/msg_server_test.go (1)
- 572-593: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [561-608]
The update to
expectedMessageEventsto always be 0 inTestTransferPositions_Eventsaligns with the PR's objectives to simplify the transfer process by not triggering additional events related to claiming rewards before transfers. It's crucial to ensure that this change does not inadvertently suppress any essential events that should be emitted during the transfer process. Please confirm that all necessary events, aside from those related to reward claims, are still being emitted as expected.x/concentrated-liquidity/position.go (22)
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
The package and import statements appear to be correctly organized and relevant to the functionality provided in this file. It's good practice to ensure that only necessary packages are imported to keep the code clean and maintainable.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [16-16]
The constant
MinNumPositionsis defined but not directly referenced in the provided code changes. Ensure that this constant is used appropriately elsewhere in the codebase or consider removing it if it's no longer needed.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [18-18]
The variable
emptyOptionsis defined for use withaccum.Options{}. It's a good practice to define such placeholders for default or empty configurations. Ensure that it's used appropriately where default options are needed.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [20-20]
The function
getOrInitPositionis well-documented and handles the retrieval or initialization of a position's liquidity. It correctly returns zero liquidity for non-existent positions, which is a safe default behavior.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [32-32]
The function
initOrUpdatePositionis crucial for managing position liquidity. It's important to ensure that the logic correctly handles both initialization and updates, especially in terms of liquidity adjustments and error handling. The warning about mutating the pool is appropriately placed, reminding developers to refetch the pool after calling this method.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [74-74]
The function
hasPositionprovides a straightforward way to check for the existence of a position by its ID. Usingstore.Has(positionIdKey)is an efficient approach to perform this check.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [76-76]
The function
HasAnyPositionForPoolis designed to check if any position exists for a given pool. The use ofosmoutils.HasAnyAtPrefixalong with a custom parsing function is a clever way to achieve this without needing to iterate over all positions manually.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [88-88]
The function
GetAllPositionIdsForPoolIdis well-structured and correctly parses position IDs from the store keys. The error handling and sorting of position IDs ensure that the function's output is reliable and consistent.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [120-120]
The function
GetPositionLiquidityandGetPositionare essential for retrieving position details. They correctly handle errors and return the expected results. It's important to ensure that these functions are used consistently throughout the codebase to fetch position information.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [142-142]
The function
GetUserPositionsandGetUserPositionsSerializedprovide mechanisms to retrieve user positions, with the latter supporting pagination. The separation of these concerns and the use of pagination demonstrate good API design principles.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [164-164]
The function
SetPositionis critical for creating or updating position information. The detailed comments and structured approach to setting various mappings in the store are commendable. Ensure that error handling is robust, especially for operations that modify the store.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [226-226]
The function
deletePositionhandles the deletion of position information comprehensively, including removing all related mappings. The error handling for missing mappings before deletion is a good practice to prevent inconsistencies.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [274-274]
The functions
CreateFullRangePosition,CreateFullRangePositionLocked, andCreateFullRangePositionUnlockingprovide various ways to create full-range positions, including handling locked positions. These functions are well-documented and appear to handle the creation process correctly. Pay special attention to error handling and the consistency of state changes.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [332-332]
The function
mintSharesAndLockis a key part of handling locked positions. It correctly mints shares and locks them for a specified duration. Ensure that the minting and locking processes are secure and that any potential errors are handled appropriately.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [370-370]
The function
CalculateUnderlyingAssetsFromPositionaccurately calculates the underlying assets for a given position. This function is crucial for determining the value of positions and should be used consistently where such calculations are needed.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [388-388]
The function
getNextPositionIdAndIncrementprovides a simple and effective way to manage position IDs. Ensuring atomicity in incrementing and retrieving the next position ID is crucial to prevent race conditions.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [396-396]
The functions
GetLockIdFromPositionIdandGetPositionIdToLockIdare essential for managing the mappings between positions and locks. The clear separation of concerns and error handling in these functions are good practices.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [414-414]
The function
setPositionIdToLockandRemovePositionIdForLockIdcorrectly manage the bidirectional mappings between positions and locks. This is crucial for maintaining the integrity of the system's state.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [432-432]
The functions
PositionHasActiveUnderlyingLockandpositionHasActiveUnderlyingLockAndUpdateare important for determining the status of underlying locks associated with positions. The distinction between non-mutative and mutative versions of the check is a good design choice.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [460-460]
The function
GetFullRangeLiquidityInPoolandupdateFullRangeLiquidityInPoolmanage the liquidity information for full-range positions within a pool. Ensuring accurate tracking of this liquidity is essential for the system's operation.
- 704-709: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [484-484]
The
transferPositionsfunction is central to the PR's objectives, handling the transfer of positions between users. It's important to ensure that this function correctly implements the new logic for handling rewards during transfers, as described in the PR objectives. The removal of the requirement to claim rewards before transferring positions should be reflected here.Verification successful
The analysis of the
transferPositionsfunction's implementation confirms that there is no explicit logic requiring rewards to be claimed before transferring positions. This aligns with the PR objectives of removing such a requirement, suggesting that the function correctly implements the new logic for handling rewards during transfers.* 704-709: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [536-536]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logic for handling rewards during transfers has been correctly implemented. # Specifically, check that rewards are no longer required to be claimed before transferring positions. ast-grep --lang go --pattern $'func (k Keeper) transferPositions($_, $_, $_, $_) error { $$$ // Check for logic related to claiming rewards before transfer $$$ }'Length of output: 169
Script:
#!/bin/bash # Search for the `transferPositions` function in the Go codebase to verify its existence and signature. ast-grep --lang go --pattern $'func (k Keeper) transferPositions($$$) $$$'Length of output: 4616
The function
UnderlyingPositionsValuecalculates the value of underlying assets for a set of positions. This function is useful for assessing the total value of positions and should be accurate and efficient in its calculations.x/concentrated-liquidity/position_test.go (4)
- 2447-2466: The assertions made after transferring positions and claiming rewards seem to validate the expected behavior correctly. However, it's crucial to ensure that the rewards are indeed transferred to the new owner and not accessible by the old owner post-transfer. Additionally, verifying the balance of the old owner before and after claiming rewards by the new owner is a good practice to confirm that no unintended transfers occur.
- 2470-2470: The logic for funding incentives and spread rewards appears to be correctly implemented, with careful consideration of the expected rewards based on uptime growth and the funding of both incentives and forfeited amounts to the pool. It's important to ensure that the calculations for expected rewards align with the system's rules and that the funded amounts are accurately reflected in the pool's state.
- 2447-2466: The
TestNegativeTickRange_SpreadFactortest case effectively covers the edge case of negative tick range accumulators and validates the correct handling of spread factor rewards. It's crucial to ensure that the system can handle these scenarios without errors and that rewards are accurately calculated and distributed, even in edge cases. The use of helper functions to estimate swap amounts and fund rewards aids in setting up the test scenarios accurately.- 2447-2466: The
TestMultipleRangestest case provides a comprehensive examination of handling multiple position ranges, including adjacent, non-adjacent, and overlapping ranges. It's important to verify that the system correctly handles these scenarios, especially in terms of liquidity calculations, rewards distribution, and position management. The test cases should ensure that positions within and outside the current tick range are accurately processed and that rewards are correctly allocated based on position ownership and range.
This reverts commit b1952b9.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 69-69: The addition to the changelog correctly documents the removal of reward claiming during position transfers as part of the pull request fix: Implement retaining rewards for transfers #7785. This change aligns with the PR's objective to simplify the transfer process and address the challenges associated with the manual reassignment of ownership over position IDs.
| postTransferNewOwnerFunds := s.App.BankKeeper.GetAllBalances(s.Ctx, newOwner) | ||
| s.Require().Equal(preTransferNewOwnerFunds, postTransferNewOwnerFunds) | ||
|
|
||
| // Test that claiming incentives/spread rewards with the new owner returns nothing |
There was a problem hiding this comment.
Seems like this diff is rendering weirdly. The changes here were:
- Remove previous assertion that claiming rewards post transfer yields nothing
- Claim rewards post transfer and ensure they go to new owner
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/position.go (2 hunks)
- x/concentrated-liquidity/position_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/concentrated-liquidity/position.go
- x/concentrated-liquidity/position_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- x/concentrated-liquidity/msg_server_test.go (3 hunks)
- x/concentrated-liquidity/position.go (2 hunks)
- x/concentrated-liquidity/position_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- x/concentrated-liquidity/msg_server_test.go
- x/concentrated-liquidity/position.go
- x/concentrated-liquidity/position_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/position_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/concentrated-liquidity/position_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/position.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/concentrated-liquidity/position.go
* implement retaining rewards for transfers * add changelog * clean up tests * driveby test cleanup * Revert "driveby test cleanup" This reverts commit b1952b9. * update transfer method spec * fix conflicts * lint (cherry picked from commit d28ed22) # Conflicts: # CHANGELOG.md # x/concentrated-liquidity/msg_server_test.go # x/concentrated-liquidity/position.go
* implement retaining rewards for transfers * add changelog * clean up tests * driveby test cleanup * Revert "driveby test cleanup" This reverts commit b1952b9. * update transfer method spec * fix conflicts * lint (cherry picked from commit d28ed22) # Conflicts: # CHANGELOG.md # x/concentrated-liquidity/msg_server_test.go # x/concentrated-liquidity/position.go Co-authored-by: Alpo <[email protected]> Co-authored-by: PaddyMc <[email protected]>
Closes: #7776
What is the purpose of the change
This PR removes reward claiming logic that previously executing prior to position transfers. Since the transfers are done through manual reassignment of ownership over position IDs, handling forfeited incentives become tricky/hacky since the positions do not actually move (see issue for further discussion).
Spread reward claiming is also removed for consistency. This also makes the transfer function run more efficiently, as these claims were not necessary for the correct functioning of the logic.
Testing and Verifying
Tests in
position_test.goare updated to ensure that claiming of old and new rewards (both incentives and spread rewards) function properly (i.e. go to the new owner and not the old).Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)Summary by CodeRabbit