[CT-950] safety heap methods #1821
Conversation
WalkthroughThis update enhances the decentralized finance protocol by managing a safety heap for subaccounts. The changes introduce new methods for inserting, removing, and maintaining subaccounts within the heap, ensuring proper order and integrity. Additionally, tests have been added to verify the functionality of these heap operations. Changes
Tip AI model upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/subaccounts/keeper/safety_heap.go (1 hunks)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Additional comments not posted (8)
protocol/x/subaccounts/keeper/safety_heap_test.go (1)
17-68: Comprehensive Testing of Safety Heap Insertion and RemovalThe test function
TestSafetyHeapInsertRemovalis well-structured and tests the heap operations thoroughly by:
- Creating a large set of subaccounts with varying asset positions.
- Randomly shuffling these subaccounts to simulate different scenarios.
- Inserting them into the heap and verifying the order after each insertion.
- Removing them from the heap and verifying the heap's integrity at each step.
Suggestions:
- Consider edge cases where subaccounts might have identical asset positions to ensure the heap handles ties correctly.
- It might be beneficial to break down this test into smaller functions for better readability and maintainability.
protocol/x/subaccounts/keeper/safety_heap.go (7)
9-20: Review ofRemoveSubaccountFromSafetyHeapFunctionThis function correctly identifies the subaccount's index in the heap and removes it using the
MustRemoveElementAtIndexfunction. The separation of concerns is maintained, and the function is concise.Suggestions:
- Ensure that the function handles cases where the subaccount is not found in the heap, possibly by verifying if the index retrieval was successful before attempting removal.
22-32: Review ofAddSubaccountToSafetyHeapFunctionThe function is straightforward and effectively adds a subaccount to the heap. It uses the
Insertmethod, which encapsulates the logic for adjusting the heap after insertion.Suggestions:
- Consider adding logging or metrics collection to monitor the performance and usage of heap operations, which could be crucial for debugging and performance tuning in a production environment.
36-52: Optimization Suggestion forInsertFunctionThe function performs essential heap operations and maintains the heap property by calling
HeapifyUp. The implementation is correct.Suggestions:
- To improve performance, consider optimizing the
SetSubaccountAtIndexandSetSafetyHeapLengthoperations if they involve significant computation or IO.
78-94: Review ofHeapifyUpFunctionThis function correctly restores the heap property by moving an element up the heap until it is in the correct position. The recursive approach is appropriate for this operation.
Suggestions:
- Ensure that the recursive depth does not become a performance issue on very large heaps. Consider an iterative approach if needed.
96-121: Review ofHeapifyDownFunctionThe function effectively handles both left and right children during the heapify down process, ensuring that the heap property is maintained.
Suggestions:
- Add more detailed comments explaining the logic, especially how it decides between left and right children, to improve code readability and maintainability.
123-138: Review ofSwapFunctionThe
Swapfunction is implemented correctly, handling the no-op case and swapping elements efficiently.Suggestions:
- Ensure that all uses of this function check for the validity of the indices before calling, to prevent potential errors.
140-174: Complex Logic inLessFunctionThe
Lessfunction uses financial risk calculations to determine the order of elements in the heap. This is a critical part of maintaining the heap's integrity.Suggestions:
- Given the complexity and importance of this function, consider adding unit tests specifically targeting various scenarios and edge cases in the risk comparison logic.
| // MustRemoveElementAtIndex removes the element at the given index | ||
| // from the safety heap. | ||
| func (k Keeper) MustRemoveElementAtIndex( | ||
| ctx sdk.Context, | ||
| store prefix.Store, | ||
| index uint32, | ||
| ) { | ||
| length := k.GetSafetyHeapLength(store) | ||
| if index >= length { | ||
| panic(types.ErrSafetyHeapSubaccountNotFoundAtIndex) | ||
| } | ||
|
|
||
| // Swap the element with the last element. | ||
| k.Swap(store, index, length-1) | ||
|
|
||
| // Remove the last element. | ||
| k.DeleteSubaccountAtIndex(store, length-1) | ||
| k.SetSafetyHeapLength(store, length-1) | ||
|
|
||
| // Heapify down the element at the given index | ||
| // to restore the heap property. | ||
| k.HeapifyDown(ctx, store, index) | ||
| } |
There was a problem hiding this comment.
Error Handling in MustRemoveElementAtIndex
The function includes a panic for an out-of-bounds index, which is a harsh failure mode.
Suggestions:
- Replace
panicwith a more graceful error handling mechanism, such as returning an error to the caller. This change would make the system more robust and maintainable, especially in production environments where a panic could lead to service disruption.
- panic(types.ErrSafetyHeapSubaccountNotFoundAtIndex)
+ return fmt.Errorf("subaccount not found at index: %d", index)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/subaccounts/keeper/safety_heap.go (1 hunks)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap.go
Additional comments not posted (1)
protocol/x/subaccounts/keeper/safety_heap_test.go (1)
186-199: LGTM!The helper function correctly verifies the heap properties by recursively checking that each node is less than its children.
| func TestSafetyHeapInsertRemoveMin(t *testing.T) { | ||
| perpetualId := uint32(0) | ||
| side := satypes.Long | ||
| totalSubaccounts := 1000 | ||
|
|
||
| // Create 1000 subaccounts with balances ranging from -500 to 500. | ||
| // The subaccounts should be sorted by balance. | ||
| allSubaccounts := make([]satypes.Subaccount, 0) | ||
| for i := 0; i < totalSubaccounts; i++ { | ||
| subaccount := satypes.Subaccount{ | ||
| Id: &satypes.SubaccountId{ | ||
| Owner: types.MustBech32ifyAddressBytes( | ||
| config.Bech32PrefixAccAddr, | ||
| constants.AliceAccAddress, | ||
| ), | ||
| Number: uint32(i), | ||
| }, | ||
| AssetPositions: testutil.CreateUsdcAssetPositions( | ||
| // Create asset positions with balances ranging from -500 to 500. | ||
| big.NewInt(int64(i - totalSubaccounts/2)), | ||
| ), | ||
| } | ||
|
|
||
| // Handle special case. | ||
| if i-totalSubaccounts/2 == 0 { | ||
| subaccount.AssetPositions = nil | ||
| } | ||
|
|
||
| allSubaccounts = append(allSubaccounts, subaccount) | ||
| } | ||
|
|
||
| for iter := 0; iter < 100; iter++ { | ||
| // Setup keeper state and test parameters. | ||
| ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false) | ||
|
|
||
| // Shuffle the subaccounts so that insertion order is random. | ||
| slices.Shuffle(allSubaccounts) | ||
|
|
||
| store := subaccountsKeeper.GetSafetyHeapStore(ctx, perpetualId, side) | ||
| for i, subaccount := range allSubaccounts { | ||
| subaccountsKeeper.SetSubaccount(ctx, subaccount) | ||
| subaccountsKeeper.AddSubaccountToSafetyHeap( | ||
| ctx, | ||
| *subaccount.Id, | ||
| perpetualId, | ||
| side, | ||
| ) | ||
|
|
||
| require.Equal( | ||
| t, | ||
| uint32(i+1), | ||
| subaccountsKeeper.GetSafetyHeapLength(store), | ||
| ) | ||
| } | ||
|
|
||
| // Make sure subaccounts are sorted correctly. | ||
| for i := 0; i < totalSubaccounts; i++ { | ||
| // Get the subaccount with the lowest safety score. | ||
| // In this case, the subaccount with the lowest USDC balance. | ||
| subaccountId := subaccountsKeeper.MustGetSubaccountAtIndex(store, uint32(0)) | ||
| subaccount := subaccountsKeeper.GetSubaccount(ctx, subaccountId) | ||
|
|
||
| // Subaccounts should be sorted by asset position balance. | ||
| require.Equal(t, uint32(i), subaccountId.Number) | ||
| require.Equal( | ||
| t, | ||
| big.NewInt(int64(i-totalSubaccounts/2)), | ||
| subaccount.GetUsdcPosition(), | ||
| ) | ||
|
|
||
| // Remove the subaccount from the heap. | ||
| subaccountsKeeper.RemoveSubaccountFromSafetyHeap( | ||
| ctx, | ||
| subaccountId, | ||
| perpetualId, | ||
| side, | ||
| ) | ||
| require.Equal( | ||
| t, | ||
| uint32(totalSubaccounts-i-1), | ||
| subaccountsKeeper.GetSafetyHeapLength(store), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM! Consider verifying heap properties after each removal.
The test logic is comprehensive and ensures that subaccounts are inserted and removed correctly. However, it would be beneficial to verify the heap properties after each removal to ensure the heap remains valid throughout the process.
+ // Verify that the heap property is maintained after each removal.
+ verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0)Committable suggestion was skipped due to low confidence.
| func TestSafetyHeapInsertRemoveIndex(t *testing.T) { | ||
| perpetualId := uint32(0) | ||
| side := satypes.Long | ||
| totalSubaccounts := 100 | ||
|
|
||
| // Create 1000 subaccounts with balances ranging from -500 to 500. | ||
| // The subaccounts should be sorted by balance. | ||
| allSubaccounts := make([]satypes.Subaccount, 0) | ||
| for i := 0; i < totalSubaccounts; i++ { | ||
| subaccount := satypes.Subaccount{ | ||
| Id: &satypes.SubaccountId{ | ||
| Owner: types.MustBech32ifyAddressBytes( | ||
| config.Bech32PrefixAccAddr, | ||
| constants.AliceAccAddress, | ||
| ), | ||
| Number: uint32(i), | ||
| }, | ||
| AssetPositions: testutil.CreateUsdcAssetPositions( | ||
| // Create asset positions with balances ranging from -500 to 500. | ||
| big.NewInt(int64(i - totalSubaccounts/2)), | ||
| ), | ||
| } | ||
|
|
||
| // Handle special case. | ||
| if i-totalSubaccounts/2 == 0 { | ||
| subaccount.AssetPositions = nil | ||
| } | ||
|
|
||
| allSubaccounts = append(allSubaccounts, subaccount) | ||
| } | ||
|
|
||
| for iter := 0; iter < 100; iter++ { | ||
| // Setup keeper state and test parameters. | ||
| ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false) | ||
|
|
||
| // Shuffle the subaccounts so that insertion order is random. | ||
| slices.Shuffle(allSubaccounts) | ||
|
|
||
| store := subaccountsKeeper.GetSafetyHeapStore(ctx, perpetualId, side) | ||
| for i, subaccount := range allSubaccounts { | ||
| subaccountsKeeper.SetSubaccount(ctx, subaccount) | ||
| subaccountsKeeper.AddSubaccountToSafetyHeap( | ||
| ctx, | ||
| *subaccount.Id, | ||
| perpetualId, | ||
| side, | ||
| ) | ||
|
|
||
| require.Equal( | ||
| t, | ||
| uint32(i+1), | ||
| subaccountsKeeper.GetSafetyHeapLength(store), | ||
| ) | ||
| } | ||
|
|
||
| for i := totalSubaccounts; i > 0; i-- { | ||
| // Remove a random subaccount from the heap. | ||
| index := rand.Intn(i) | ||
|
|
||
| subaccountId := subaccountsKeeper.MustGetSubaccountAtIndex(store, uint32(index)) | ||
| subaccountsKeeper.RemoveSubaccountFromSafetyHeap( | ||
| ctx, | ||
| subaccountId, | ||
| perpetualId, | ||
| side, | ||
| ) | ||
|
|
||
| require.Equal( | ||
| t, | ||
| uint32(i-1), | ||
| subaccountsKeeper.GetSafetyHeapLength(store), | ||
| ) | ||
|
|
||
| // Verify that the heap property is restored. | ||
| verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM! Consider verifying heap properties after each removal.
The test logic is comprehensive and ensures that subaccounts are inserted and removed correctly at random indices. However, it would be beneficial to verify the heap properties after each removal to ensure the heap remains valid throughout the process.
+ // Verify that the heap property is maintained after each removal.
+ verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSafetyHeapInsertRemoveIndex(t *testing.T) { | |
| perpetualId := uint32(0) | |
| side := satypes.Long | |
| totalSubaccounts := 100 | |
| // Create 1000 subaccounts with balances ranging from -500 to 500. | |
| // The subaccounts should be sorted by balance. | |
| allSubaccounts := make([]satypes.Subaccount, 0) | |
| for i := 0; i < totalSubaccounts; i++ { | |
| subaccount := satypes.Subaccount{ | |
| Id: &satypes.SubaccountId{ | |
| Owner: types.MustBech32ifyAddressBytes( | |
| config.Bech32PrefixAccAddr, | |
| constants.AliceAccAddress, | |
| ), | |
| Number: uint32(i), | |
| }, | |
| AssetPositions: testutil.CreateUsdcAssetPositions( | |
| // Create asset positions with balances ranging from -500 to 500. | |
| big.NewInt(int64(i - totalSubaccounts/2)), | |
| ), | |
| } | |
| // Handle special case. | |
| if i-totalSubaccounts/2 == 0 { | |
| subaccount.AssetPositions = nil | |
| } | |
| allSubaccounts = append(allSubaccounts, subaccount) | |
| } | |
| for iter := 0; iter < 100; iter++ { | |
| // Setup keeper state and test parameters. | |
| ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false) | |
| // Shuffle the subaccounts so that insertion order is random. | |
| slices.Shuffle(allSubaccounts) | |
| store := subaccountsKeeper.GetSafetyHeapStore(ctx, perpetualId, side) | |
| for i, subaccount := range allSubaccounts { | |
| subaccountsKeeper.SetSubaccount(ctx, subaccount) | |
| subaccountsKeeper.AddSubaccountToSafetyHeap( | |
| ctx, | |
| *subaccount.Id, | |
| perpetualId, | |
| side, | |
| ) | |
| require.Equal( | |
| t, | |
| uint32(i+1), | |
| subaccountsKeeper.GetSafetyHeapLength(store), | |
| ) | |
| } | |
| for i := totalSubaccounts; i > 0; i-- { | |
| // Remove a random subaccount from the heap. | |
| index := rand.Intn(i) | |
| subaccountId := subaccountsKeeper.MustGetSubaccountAtIndex(store, uint32(index)) | |
| subaccountsKeeper.RemoveSubaccountFromSafetyHeap( | |
| ctx, | |
| subaccountId, | |
| perpetualId, | |
| side, | |
| ) | |
| require.Equal( | |
| t, | |
| uint32(i-1), | |
| subaccountsKeeper.GetSafetyHeapLength(store), | |
| ) | |
| // Verify that the heap property is restored. | |
| verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0) | |
| } | |
| } | |
| } | |
| func TestSafetyHeapInsertRemoveIndex(t *testing.T) { | |
| perpetualId := uint32(0) | |
| side := satypes.Long | |
| totalSubaccounts := 100 | |
| // Create 1000 subaccounts with balances ranging from -500 to 500. | |
| // The subaccounts should be sorted by balance. | |
| allSubaccounts := make([]satypes.Subaccount, 0) | |
| for i := 0; i < totalSubaccounts; i++ { | |
| subaccount := satypes.Subaccount{ | |
| Id: &satypes.SubaccountId{ | |
| Owner: types.MustBech32ifyAddressBytes( | |
| config.Bech32PrefixAccAddr, | |
| constants.AliceAccAddress, | |
| ), | |
| Number: uint32(i), | |
| }, | |
| AssetPositions: testutil.CreateUsdcAssetPositions( | |
| // Create asset positions with balances ranging from -500 to 500. | |
| big.NewInt(int64(i - totalSubaccounts/2)), | |
| ), | |
| } | |
| // Handle special case. | |
| if i-totalSubaccounts/2 == 0 { | |
| subaccount.AssetPositions = nil | |
| } | |
| allSubaccounts = append(allSubaccounts, subaccount) | |
| } | |
| for iter := 0; iter < 100; iter++ { | |
| // Setup keeper state and test parameters. | |
| ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false) | |
| // Shuffle the subaccounts so that insertion order is random. | |
| slices.Shuffle(allSubaccounts) | |
| store := subaccountsKeeper.GetSafetyHeapStore(ctx, perpetualId, side) | |
| for i, subaccount := range allSubaccounts { | |
| subaccountsKeeper.SetSubaccount(ctx, subaccount) | |
| subaccountsKeeper.AddSubaccountToSafetyHeap( | |
| ctx, | |
| *subaccount.Id, | |
| perpetualId, | |
| side, | |
| ) | |
| require.Equal( | |
| t, | |
| uint32(i+1), | |
| subaccountsKeeper.GetSafetyHeapLength(store), | |
| ) | |
| } | |
| for i := totalSubaccounts; i > 0; i-- { | |
| // Remove a random subaccount from the heap. | |
| index := rand.Intn(i) | |
| subaccountId := subaccountsKeeper.MustGetSubaccountAtIndex(store, uint32(index)) | |
| subaccountsKeeper.RemoveSubaccountFromSafetyHeap( | |
| ctx, | |
| subaccountId, | |
| perpetualId, | |
| side, | |
| ) | |
| require.Equal( | |
| t, | |
| uint32(i-1), | |
| subaccountsKeeper.GetSafetyHeapLength(store), | |
| ) | |
| // Verify that the heap property is restored. | |
| verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0) | |
| // Verify that the heap property is maintained after each removal. | |
| verifyHeapProperties(t, subaccountsKeeper, ctx, store, 0) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/safety_heap_test.go
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Tests