Optimize OffsetSubaccountPerpetualPosition subaccount iteration #906
Conversation
WalkthroughThe codebase has undergone a refactoring that centralizes the handling of liquidation information within the 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 X ? TipsChat with CodeRabbit Bot (
|
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (16)
- protocol/app/ante_whitebox_test.go (3 hunks)
- protocol/app/app.go (1 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1 hunks)
- protocol/testutil/clob/open_positions.go (1 hunks)
- protocol/testutil/keeper/clob.go (2 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/abci_test.go (4 hunks)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go (2 hunks)
- protocol/x/clob/keeper/deleveraging.go (1 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (2 hunks)
- protocol/x/clob/keeper/keeper.go (4 hunks)
- protocol/x/clob/module.go (4 hunks)
- protocol/x/clob/module_test.go (2 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (2 hunks)
Files skipped from review due to trivial changes (3)
- protocol/x/clob/module_test.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/keeper/subaccount_test.go
Additional comments: 22
protocol/testutil/clob/open_positions.go (1)
- 8-42: The function
GetOpenPositionsFromSubaccountsis well-structured and follows best practices for iterating over slices and appending to slices in Go. The use of a map to track positions and then converting it to a slice before returning is efficient for avoiding duplicate entries and ensuring uniquePerpetualIdvalues.protocol/x/clob/types/expected_keepers.go (1)
- 42-47: The removal of the
ForEachSubaccountRandomStartmethod from theSubaccountsKeeperinterface aligns with the PR's objective to change the iteration logic for subaccounts. The remaining methodGetRandomSubaccountis unchanged, which is consistent with the provided summary.protocol/app/ante_whitebox_test.go (1)
- 77-77: The addition of
liquidationtypes.NewDaemonLiquidationInfo()in thenewTestHandlerOptionsfunction is consistent with the changes made across the codebase to move daemon liquidation information to the keeper level.protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1)
- 119-136: The new method
GetSubaccountsWithOpenPositionsForPerpetualadded to theDaemonLiquidationInfotype is correctly implemented. It locks the shared resource to prevent race conditions and provides a clear way to retrieve subaccount IDs with open positions based on the perpetual ID and position type.protocol/x/clob/module.go (1)
- 115-127: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [106-124]
The restructuring of the
AppModulestruct to includesubaccountsKeeperinstead ofdaemonLiquidationInfoand the corresponding changes in theNewAppModulefunction are in line with the PR's objective to refactor the subaccount iteration logic and move daemon liquidation information to the keeper level.protocol/x/clob/keeper/keeper.go (1)
- 61-61: The declaration of
DaemonLiquidationInfowithin theKeeperstruct and its initialization in theNewKeeperfunction are consistent with the architectural shift of moving liquidation responsibilities to the keeper level.protocol/x/clob/abci.go (1)
- 198-201: The logic to retrieve potentially liquidatable subaccount IDs and attempt to liquidate them using the
DaemonLiquidationInfoinstance is correctly placed within thePrepareCheckStatefunction, which is part of the ABCI lifecycle where such operations should occur.protocol/testutil/keeper/clob.go (1)
- 221-221: The addition of
liquidationtypes.NewDaemonLiquidationInfo()in thecreateClobKeeperfunction is consistent with the changes made across the codebase to move daemon liquidation information to the keeper level.protocol/x/clob/keeper/deleveraging.go (1)
- 286-292: The use of
panicwithin theOffsetSubaccountPerpetualPositionfunction when encountering anErrInvalidPerpetualPositionSizeDeltaerror is a critical behavior. Panicking in a blockchain context can be dangerous as it can halt the entire node. It is essential to ensure that this is the desired behavior and that there are no alternative error handling strategies that could be employed here. If this panic is indeed necessary, it should be clearly documented as to why this is the case.protocol/x/clob/e2e/liquidation_deleveraging_test.go (2)
13-13: The addition of
clobtestpackage import seems to be necessary for the new test logic introduced. Ensure that this new dependency is properly managed and does not introduce any versioning conflicts.1127-1128: The
LiquidateSubaccountsRequestnow includesSubaccountOpenPositionInfo, which is populated usingclobtest.GetOpenPositionsFromSubaccounts(tc.subaccounts). This change aligns with the PR's objective to update the test setup to reflect the new subaccount iteration logic. Ensure that theGetOpenPositionsFromSubaccountsfunction is thoroughly tested to prevent incorrect test behavior.protocol/x/clob/keeper/deleveraging_test.go (2)
18-18: The addition of the
clobtestimport is appropriate for the new test utilities being used.720-721: The introduction of
positionsand the call toUpdateSubaccountsWithPositionsalign with the PR's objective to update subaccounts with positions. Ensure that theclobtest.GetOpenPositionsFromSubaccountsfunction is thoroughly tested to prevent incorrect position updates.protocol/x/clob/abci_test.go (5)
25-30: The import removal of "liquidationtypes" and the changes in the test functions to use "ks.ClobKeeper.DaemonLiquidationInfo" directly are consistent with the PR's objective to move daemon liquidation information to the keeper level.
1098-1103: The test
TestPrepareCheckState_WithProcessProposerMatchesEventsWithBadBlockHeightis designed to ensure that the system panics if the block height does not match the stored events. This is a good test for ensuring data consistency and correct behavior in the face of incorrect block height data.1124-1129: The test
TestCommitBlocker_WithProcessProposerMatchesEventsWithBadBlockHeightappears to be a duplicate ofTestPrepareCheckState_WithProcessProposerMatchesEventsWithBadBlockHeightas it tests the same panic condition for block height mismatch. It's important to ensure that this is intentional and that the test is not redundant.1475-1481: The test
TestBeginBlocker_Successcorrectly initializes the next block's process proposer matches events, which is a critical part of the block lifecycle in the clob module. This test ensures that the state is correctly overwritten, which is essential for the integrity of the block processing.1475-1481: The test
TestPrepareCheckStateis comprehensive and tests the clob module's state preparation logic with various scenarios, including edge cases like replaying operations. This ensures that the module behaves correctly under different conditions and that the memclob state is consistent with expectations.protocol/app/app.go (4)
882-882: The addition of
daemonLiquidationInfoas an argument to theNewfunction aligns with the PR's objective to move daemon liquidation information to the keeper level. Ensure that all instances where theNewfunction is called have been updated to pass this new argument.882-892: The
daemonLiquidationInfois correctly passed to theclobmodulekeeper.NewKeeper. This change centralizes the liquidation logic within the keeper, which is consistent with the PR's objectives. Confirm that theclobmodulekeeperis properly utilizingdaemonLiquidationInfofor its operations.879-892: The initialization order within the
Newfunction appears to be correct. However, it's crucial to ensure that the newly addeddaemonLiquidationInfodoes not introduce any dependency issues, especially since it's now being used in the initialization of theClobKeeper.879-892: Review the integration of the new
daemonLiquidationInfoparameter throughout theNewfunction to ensure it does not disrupt existing module initializations and that it is correctly utilized in all relevant places.
| subaccountsWithOpenPositions := k.DaemonLiquidationInfo.GetSubaccountsWithOpenPositionsForPerpetual( | ||
| perpetualId, | ||
| deltaQuantumsTotal.Sign() == 1, | ||
| ) | ||
| for _, subaccountId := range subaccountsWithOpenPositions { | ||
| // Iterate at most `MaxDeleveragingSubaccountsToIterate` subaccounts. | ||
| if numSubaccountsIterated >= k.Flags.MaxDeleveragingSubaccountsToIterate { | ||
| break | ||
| } | ||
|
|
||
| if deltaQuantumsRemaining.Sign() == 0 { | ||
| break | ||
| } | ||
|
|
||
| numSubaccountsIterated++ | ||
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, subaccountId) | ||
| offsettingPosition, _ := offsettingSubaccount.GetPerpetualPositionForId(perpetualId) | ||
| bigOffsettingPositionQuantums := offsettingPosition.GetBigQuantums() | ||
|
|
||
| // Skip subaccounts that do not have a position in the opposite direction as the liquidated subaccount. | ||
| if deltaQuantumsRemaining.Sign() != bigOffsettingPositionQuantums.Sign() { | ||
| numSubaccountsWithNoOpenPositionOnOppositeSide++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
The refactoring of the iteration logic in OffsetSubaccountPerpetualPosition to use DaemonLiquidationInfo for obtaining subaccount IDs is a significant change. Ensure that the new logic correctly handles the iteration and early return conditions, especially the check for the sign of deltaQuantumsRemaining and bigOffsettingPositionQuantums to determine if the subaccount has a position in the opposite direction as required.
Additionally, the presence of the TODO comment on line 238 suggests that there is a decision pending on how to determine the max amount to offset per offsetting subaccount. This should be resolved or at least tracked in an issue management system to ensure it is not forgotten.
| // Fetch delta quote quantums. Calculated at bankruptcy price for standard | ||
| // deleveraging and at oracle price for final settlement deleveraging. | ||
| deltaQuoteQuantums, err := k.getDeleveragingQuoteQuantumsDelta( | ||
| ctx, | ||
| perpetualId, | ||
| liquidatedSubaccountId, | ||
| deltaQuantums, | ||
| ) | ||
| if err != nil { | ||
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | ||
| k.Logger(ctx).Error( | ||
| "Encountered error when getting quote quantums for deleveraging", | ||
| "error", err, | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "perpetualId", perpetualId, | ||
| "deltaQuantums", deltaQuantums, | ||
| "liquidatedSubaccount", liquidatedSubaccount, | ||
| "offsettingSubaccount", offsettingSubaccount, | ||
| ) | ||
| if err != nil { | ||
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | ||
| k.Logger(ctx).Error( | ||
| "Encountered error when getting quote quantums for deleveraging", | ||
| "error", err, | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "perpetualId", perpetualId, | ||
| "deltaQuantums", deltaQuantums, | ||
| "liquidatedSubaccount", liquidatedSubaccount, | ||
| "offsettingSubaccount", offsettingSubaccount, | ||
| ) | ||
| return false | ||
| } | ||
|
|
||
| // Try to process the deleveraging operation for both subaccounts. | ||
| if err := k.ProcessDeleveraging( | ||
| ctx, | ||
| liquidatedSubaccountId, | ||
| *offsettingSubaccount.Id, | ||
| perpetualId, | ||
| continue |
There was a problem hiding this comment.
The error handling within the iteration loop of OffsetSubaccountPerpetualPosition has been updated. It is important to ensure that the error from getDeleveragingQuoteQuantumsDelta is handled correctly. The current implementation logs the error and continues the loop, which is a reasonable approach if the error is not critical and does not affect the overall operation. However, if the error indicates a more serious issue that could compromise the integrity of the deleveraging process, it may be more appropriate to halt the operation and return the error.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/deleveraging.go (1 hunks)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/deleveraging.go
| deltaQuantums, | ||
| // Find subaccounts with open positions on the opposite side of the liquidated subaccount. | ||
| isDeleveragingLong := deltaQuantumsTotal.Sign() == -1 | ||
| subaccountsWithOpenPositions := k.DaemonLiquidationInfo.GetSubaccountsWithOpenPositionsForPerpetual( |
There was a problem hiding this comment.
By doing this we are losing the property that we are start deleveraging with at a random subaccount as the offsetting account. Is this a concern?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/app/app.go (1 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/deleveraging.go (1 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (2 hunks)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/app.go
- protocol/x/clob/abci.go
- protocol/x/clob/keeper/deleveraging_test.go
- protocol/x/clob/keeper/liquidations_test.go
Additional comments: 1
protocol/x/clob/keeper/deleveraging.go (1)
- 236-351: The refactoring of
OffsetSubaccountPerpetualPositionto useDaemonLiquidationInfo.GetSubaccountsWithOpenPositionsForPerpetualfor iterating subaccounts is a significant change. Ensure that the new logic correctly handles the iteration and that the concerns raised in the previous comment about losing the random start property are addressed or mitigated.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (8 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/deleveraging.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/abci.go
- protocol/x/clob/keeper/deleveraging.go
Additional comments: 3
protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1)
- 115-132: The method
GetSubaccountsWithOpenPositionsOnSideis similar toGetSubaccountsWithOpenPositionsbut filters based on theisLongparameter. The code is clean and follows best practices for locking and defer unlocking. The logic is straightforward and there are no apparent issues with the implementation.protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (2)
65-71: The test
TestSubaccountsWithOpenPositions_Multiple_Readscorrectly checks the expected results against the output ofGetSubaccountsWithOpenPositions. The test ensures that multiple reads return consistent results, which is good for validating the immutability of the read operation.211-211: The test
TestSubaccountsWithOpenPosition_Empty_Updatecorrectly checks that the update with an empty slice results in an empty slice being retrieved. This is a good test for ensuring that the update method handles empty inputs correctly.
| // GetSubaccountsWithOpenPositions returns the list of subaccount ids with open positions for a perpetual. | ||
| func (ls *DaemonLiquidationInfo) GetSubaccountsWithOpenPositions( | ||
| perpetualId uint32, | ||
| ) []satypes.SubaccountId { | ||
| ls.Lock() | ||
| defer ls.Unlock() | ||
|
|
||
| result := make(map[uint32]*clobtypes.SubaccountOpenPositionInfo) | ||
| for perpetualId, info := range ls.subaccountsWithPositions { | ||
| clone := &clobtypes.SubaccountOpenPositionInfo{ | ||
| PerpetualId: perpetualId, | ||
| SubaccountsWithLongPosition: make([]satypes.SubaccountId, len(info.SubaccountsWithLongPosition)), | ||
| SubaccountsWithShortPosition: make([]satypes.SubaccountId, len(info.SubaccountsWithShortPosition)), | ||
| result := make([]satypes.SubaccountId, 0) | ||
| if info, ok := ls.subaccountsWithPositions[perpetualId]; ok { | ||
| result = append(result, info.SubaccountsWithLongPosition...) | ||
| result = append(result, info.SubaccountsWithShortPosition...) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The implementation of GetSubaccountsWithOpenPositions correctly locks the shared resource, initializes a slice to store results, checks for the existence of the perpetual ID, and appends both long and short positions. However, there is no check for the validity of the perpetualId. Adding a validity check could prevent potential issues with invalid IDs.
func (ls *DaemonLiquidationInfo) GetSubaccountsWithOpenPositions(
perpetualId uint32,
) []satypes.SubaccountId {
+ // Add validation for perpetualId if applicable
ls.Lock()
defer ls.Unlock()
...
}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.
| // GetSubaccountsWithOpenPositions returns the list of subaccount ids with open positions for a perpetual. | |
| func (ls *DaemonLiquidationInfo) GetSubaccountsWithOpenPositions( | |
| perpetualId uint32, | |
| ) []satypes.SubaccountId { | |
| ls.Lock() | |
| defer ls.Unlock() | |
| result := make(map[uint32]*clobtypes.SubaccountOpenPositionInfo) | |
| for perpetualId, info := range ls.subaccountsWithPositions { | |
| clone := &clobtypes.SubaccountOpenPositionInfo{ | |
| PerpetualId: perpetualId, | |
| SubaccountsWithLongPosition: make([]satypes.SubaccountId, len(info.SubaccountsWithLongPosition)), | |
| SubaccountsWithShortPosition: make([]satypes.SubaccountId, len(info.SubaccountsWithShortPosition)), | |
| result := make([]satypes.SubaccountId, 0) | |
| if info, ok := ls.subaccountsWithPositions[perpetualId]; ok { | |
| result = append(result, info.SubaccountsWithLongPosition...) | |
| result = append(result, info.SubaccountsWithShortPosition...) | |
| } | |
| return result | |
| } | |
| // GetSubaccountsWithOpenPositions returns the list of subaccount ids with open positions for a perpetual. | |
| func (ls *DaemonLiquidationInfo) GetSubaccountsWithOpenPositions( | |
| perpetualId uint32, | |
| ) []satypes.SubaccountId { | |
| // Add validation for perpetualId if applicable | |
| ls.Lock() | |
| defer ls.Unlock() | |
| result := make([]satypes.SubaccountId, 0) | |
| if info, ok := ls.subaccountsWithPositions[perpetualId]; ok { | |
| result = append(result, info.SubaccountsWithLongPosition...) | |
| result = append(result, info.SubaccountsWithShortPosition...) | |
| } | |
| return result | |
| } |
| @@ -14,7 +14,7 @@ func TestNewDaemonLiquidationInfo(t *testing.T) { | |||
| ls := liquidationstypes.NewDaemonLiquidationInfo() | |||
| require.Empty(t, ls.GetLiquidatableSubaccountIds()) | |||
| require.Empty(t, ls.GetNegativeTncSubaccountIds()) | |||
| require.Empty(t, ls.GetSubaccountsWithPositions()) | |||
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(0)) | |||
There was a problem hiding this comment.
The test TestNewDaemonLiquidationInfo has been updated to use the new method GetSubaccountsWithOpenPositions with a hardcoded 0 as the perpetual ID. This is a good change, but it would be beneficial to test with a non-zero perpetual ID as well to ensure the method works with different inputs.
func TestNewDaemonLiquidationInfo(t *testing.T) {
...
+ require.Empty(t, ls.GetSubaccountsWithOpenPositions(1)) // Test with a non-zero perpetual ID
}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.
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(0)) | |
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(1)) |
| @@ -118,7 +119,7 @@ func TestNegativeTncSubaccounts_Multiple_Writes(t *testing.T) { | |||
|
|
|||
| func TestSubaccountsWithOpenPositions_Multiple_Writes(t *testing.T) { | |||
| ls := liquidationstypes.NewDaemonLiquidationInfo() | |||
| require.Empty(t, ls.GetSubaccountsWithPositions()) | |||
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(0)) | |||
There was a problem hiding this comment.
In TestSubaccountsWithOpenPositions_Multiple_Writes, the initial require.Empty check is good for ensuring the state before the test begins. It would be beneficial to include tests for different perpetual IDs to ensure that the update and retrieval methods work correctly across different IDs.
func TestSubaccountsWithOpenPositions_Multiple_Writes(t *testing.T) {
...
+ // Additional tests for different perpetual IDs should be included
}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.
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(0)) | |
| require.Empty(t, ls.GetSubaccountsWithOpenPositions(0)) | |
| // Additional tests for different perpetual IDs should be included |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/deleveraging.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/deleveraging.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go (5 hunks)
- protocol/x/clob/keeper/deleveraging.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go
Additional comments: 4
protocol/x/clob/keeper/deleveraging.go (4)
236-241: The new logic introduced to find subaccounts with open positions on the opposite side is a significant change. Ensure that the
GetSubaccountsWithOpenPositionsOnSidemethod is thoroughly tested, especially for edge cases where subaccounts may not have positions on the expected side.256-258: The use of
pseudoRandto determine the starting index for iteration introduces a deterministic randomness based on the current context. Verify that theGetPseudoRandmethod generates a sufficiently random value and that this randomness is appropriate for the use case.574-577: The method
GetSubaccountsWithOpenPositionsis used to retrieve subaccounts with positions in final settlement markets. Ensure that this method is optimized for performance since it could potentially be a costly operation depending on the number of subaccounts and positions.233-369: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [368-371]
The metrics and telemetry calls are used to monitor the performance and behavior of the deleveraging process. Verify that the labels and metrics used are consistent with the rest of the application and that they provide meaningful insights for monitoring.
| liquidatedSubaccountId, | ||
| *offsettingSubaccount.Id, | ||
| perpetualId, | ||
| deltaBaseQuantums, | ||
| deltaQuoteQuantums, | ||
| ); err == nil { | ||
| // Update the remaining liquidatable quantums. | ||
| deltaQuantumsRemaining = new(big.Int).Sub( | ||
| deltaQuantumsRemaining, | ||
| deltaBaseQuantums, | ||
| ) | ||
| fills = append(fills, types.MatchPerpetualDeleveraging_Fill{ | ||
| OffsettingSubaccountId: *offsettingSubaccount.Id, | ||
| FillAmount: new(big.Int).Abs(deltaBaseQuantums).Uint64(), | ||
| }) | ||
|
|
||
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | ||
| // if the branched state is discarded, so batching is not necessary. | ||
| k.GetIndexerEventManager().AddTxnEvent( | ||
| ctx, | ||
| indexerevents.SubtypeDeleveraging, | ||
| indexerevents.DeleveragingEventVersion, | ||
| indexer_manager.GetBytes( | ||
| indexerevents.NewDeleveragingEvent( | ||
| liquidatedSubaccountId, | ||
| *offsettingSubaccount.Id, | ||
| perpetualId, | ||
| satypes.BaseQuantums(new(big.Int).Abs(deltaBaseQuantums).Uint64()), | ||
| satypes.BaseQuantums(deltaQuoteQuantums.Uint64()), | ||
| deltaBaseQuantums.Sign() > 0, | ||
| isFinalSettlement, | ||
| ), | ||
| indexerevents.SubtypeDeleveraging, | ||
| indexerevents.DeleveragingEventVersion, | ||
| indexer_manager.GetBytes( | ||
| indexerevents.NewDeleveragingEvent( | ||
| liquidatedSubaccountId, | ||
| *offsettingSubaccount.Id, | ||
| perpetualId, | ||
| satypes.BaseQuantums(new(big.Int).Abs(deltaBaseQuantums).Uint64()), | ||
| satypes.BaseQuantums(deltaQuoteQuantums.Uint64()), | ||
| deltaBaseQuantums.Sign() > 0, | ||
| isFinalSettlement, | ||
| ), | ||
| ) | ||
| } else if errors.Is(err, types.ErrInvalidPerpetualPositionSizeDelta) { | ||
| panic( | ||
| fmt.Sprintf( | ||
| "Invalid perpetual position size delta when processing deleveraging. error: %v", | ||
| err, | ||
| ), | ||
| ) | ||
| } else { | ||
| // If an error is returned, it's likely because the subaccounts' bankruptcy prices do not overlap. | ||
| // TODO(CLOB-75): Support deleveraging subaccounts with non overlapping bankruptcy prices. | ||
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | ||
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, *offsettingSubaccount.Id) | ||
| k.Logger(ctx).Debug( | ||
| "Encountered error when processing deleveraging", | ||
| "error", err, | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "checkTx", ctx.IsCheckTx(), | ||
| "perpetualId", perpetualId, | ||
| "deltaQuantums", deltaBaseQuantums, | ||
| "liquidatedSubaccount", liquidatedSubaccount, | ||
| "offsettingSubaccount", offsettingSubaccount, | ||
| ) | ||
| numSubaccountsWithNonOverlappingBankruptcyPrices++ | ||
| } | ||
|
|
||
| return deltaQuantumsRemaining.Sign() == 0 | ||
| }, | ||
| k.GetPseudoRand(ctx), | ||
| ) | ||
| ), | ||
| ) | ||
| } else if errors.Is(err, types.ErrInvalidPerpetualPositionSizeDelta) { | ||
| panic( | ||
| fmt.Sprintf( | ||
| "Invalid perpetual position size delta when processing deleveraging. error: %v", | ||
| err, | ||
| ), | ||
| ) | ||
| } else { | ||
| // If an error is returned, it's likely because the subaccounts' bankruptcy prices do not overlap. | ||
| // TODO(CLOB-75): Support deleveraging subaccounts with non overlapping bankruptcy prices. | ||
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | ||
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, *offsettingSubaccount.Id) | ||
| k.Logger(ctx).Debug( | ||
| "Encountered error when processing deleveraging", | ||
| "error", err, | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "checkTx", ctx.IsCheckTx(), | ||
| "perpetualId", perpetualId, | ||
| "deltaBaseQuantums", deltaBaseQuantums, | ||
| "liquidatedSubaccount", liquidatedSubaccount, | ||
| "offsettingSubaccount", offsettingSubaccount, | ||
| ) | ||
| numSubaccountsWithNonOverlappingBankruptcyPrices++ | ||
| } |
There was a problem hiding this comment.
The loop iterates over a subset of subaccounts and processes deleveraging. It's important to ensure that the error handling within the loop is robust and that the panic condition at lines 342-348 is justified. Panics should be avoided in production code unless absolutely necessary. Consider replacing the panic with a more graceful error handling mechanism.
- panic(
- fmt.Sprintf(
- "Invalid perpetual position size delta when processing deleveraging. error: %v",
- err,
- ),
- )
+ // Handle the error more gracefully instead of panicking
+ k.Logger(ctx).Error(
+ "Invalid perpetual position size delta when processing deleveraging",
+ "error", err,
+ )
+ return errCommittable 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.
| for i := 0; i < numSubaccountsToIterate && deltaQuantumsRemaining.Sign() != 0; i++ { | |
| index := (i + indexOffset) % numSubaccounts | |
| subaccountId := subaccountsWithOpenPositions[index] | |
| numSubaccountsIterated++ | |
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, subaccountId) | |
| offsettingPosition, _ := offsettingSubaccount.GetPerpetualPositionForId(perpetualId) | |
| bigOffsettingPositionQuantums := offsettingPosition.GetBigQuantums() | |
| // Skip subaccounts that do not have a position in the opposite direction as the liquidated subaccount. | |
| if deltaQuantumsRemaining.Sign() != bigOffsettingPositionQuantums.Sign() { | |
| numSubaccountsWithNoOpenPositionOnOppositeSide++ | |
| continue | |
| } | |
| // TODO(DEC-1495): Determine max amount to offset per offsetting subaccount. | |
| var deltaBaseQuantums *big.Int | |
| if deltaQuantumsRemaining.CmpAbs(bigOffsettingPositionQuantums) > 0 { | |
| deltaBaseQuantums = new(big.Int).Set(bigOffsettingPositionQuantums) | |
| } else { | |
| deltaBaseQuantums = new(big.Int).Set(deltaQuantumsRemaining) | |
| } | |
| // Fetch delta quote quantums. Calculated at bankruptcy price for standard | |
| // deleveraging and at oracle price for final settlement deleveraging. | |
| deltaQuoteQuantums, err := k.getDeleveragingQuoteQuantumsDelta( | |
| ctx, | |
| perpetualId, | |
| liquidatedSubaccountId, | |
| deltaBaseQuantums, | |
| isFinalSettlement, | |
| ) | |
| if err != nil { | |
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | |
| k.Logger(ctx).Error( | |
| "Encountered error when getting quote quantums for deleveraging", | |
| "error", err, | |
| "blockHeight", ctx.BlockHeight(), | |
| "perpetualId", perpetualId, | |
| "deltaBaseQuantums", deltaBaseQuantums, | |
| "liquidatedSubaccount", liquidatedSubaccount, | |
| "offsettingSubaccount", offsettingSubaccount, | |
| ) | |
| if err != nil { | |
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | |
| k.Logger(ctx).Error( | |
| "Encountered error when getting quote quantums for deleveraging", | |
| "error", err, | |
| "blockHeight", ctx.BlockHeight(), | |
| "perpetualId", perpetualId, | |
| "deltaBaseQuantums", deltaBaseQuantums, | |
| "liquidatedSubaccount", liquidatedSubaccount, | |
| "offsettingSubaccount", offsettingSubaccount, | |
| "isFinalSettlement", isFinalSettlement, | |
| ) | |
| return false | |
| } | |
| // Try to process the deleveraging operation for both subaccounts. | |
| if err := k.ProcessDeleveraging( | |
| continue | |
| } | |
| // Try to process the deleveraging operation for both subaccounts. | |
| if err := k.ProcessDeleveraging( | |
| ctx, | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| deltaBaseQuantums, | |
| deltaQuoteQuantums, | |
| ); err == nil { | |
| // Update the remaining liquidatable quantums. | |
| deltaQuantumsRemaining.Sub(deltaQuantumsRemaining, deltaBaseQuantums) | |
| fills = append(fills, types.MatchPerpetualDeleveraging_Fill{ | |
| OffsettingSubaccountId: *offsettingSubaccount.Id, | |
| FillAmount: new(big.Int).Abs(deltaBaseQuantums).Uint64(), | |
| }) | |
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |
| // if the branched state is discarded, so batching is not necessary. | |
| k.GetIndexerEventManager().AddTxnEvent( | |
| ctx, | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| deltaBaseQuantums, | |
| deltaQuoteQuantums, | |
| ); err == nil { | |
| // Update the remaining liquidatable quantums. | |
| deltaQuantumsRemaining = new(big.Int).Sub( | |
| deltaQuantumsRemaining, | |
| deltaBaseQuantums, | |
| ) | |
| fills = append(fills, types.MatchPerpetualDeleveraging_Fill{ | |
| OffsettingSubaccountId: *offsettingSubaccount.Id, | |
| FillAmount: new(big.Int).Abs(deltaBaseQuantums).Uint64(), | |
| }) | |
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |
| // if the branched state is discarded, so batching is not necessary. | |
| k.GetIndexerEventManager().AddTxnEvent( | |
| ctx, | |
| indexerevents.SubtypeDeleveraging, | |
| indexerevents.DeleveragingEventVersion, | |
| indexer_manager.GetBytes( | |
| indexerevents.NewDeleveragingEvent( | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| satypes.BaseQuantums(new(big.Int).Abs(deltaBaseQuantums).Uint64()), | |
| satypes.BaseQuantums(deltaQuoteQuantums.Uint64()), | |
| deltaBaseQuantums.Sign() > 0, | |
| isFinalSettlement, | |
| ), | |
| indexerevents.SubtypeDeleveraging, | |
| indexerevents.DeleveragingEventVersion, | |
| indexer_manager.GetBytes( | |
| indexerevents.NewDeleveragingEvent( | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| satypes.BaseQuantums(new(big.Int).Abs(deltaBaseQuantums).Uint64()), | |
| satypes.BaseQuantums(deltaQuoteQuantums.Uint64()), | |
| deltaBaseQuantums.Sign() > 0, | |
| isFinalSettlement, | |
| ), | |
| ) | |
| } else if errors.Is(err, types.ErrInvalidPerpetualPositionSizeDelta) { | |
| panic( | |
| fmt.Sprintf( | |
| "Invalid perpetual position size delta when processing deleveraging. error: %v", | |
| err, | |
| ), | |
| ) | |
| } else { | |
| // If an error is returned, it's likely because the subaccounts' bankruptcy prices do not overlap. | |
| // TODO(CLOB-75): Support deleveraging subaccounts with non overlapping bankruptcy prices. | |
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | |
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, *offsettingSubaccount.Id) | |
| k.Logger(ctx).Debug( | |
| "Encountered error when processing deleveraging", | |
| "error", err, | |
| "blockHeight", ctx.BlockHeight(), | |
| "checkTx", ctx.IsCheckTx(), | |
| "perpetualId", perpetualId, | |
| "deltaQuantums", deltaBaseQuantums, | |
| "liquidatedSubaccount", liquidatedSubaccount, | |
| "offsettingSubaccount", offsettingSubaccount, | |
| ) | |
| numSubaccountsWithNonOverlappingBankruptcyPrices++ | |
| } | |
| return deltaQuantumsRemaining.Sign() == 0 | |
| }, | |
| k.GetPseudoRand(ctx), | |
| ) | |
| ), | |
| ) | |
| } else if errors.Is(err, types.ErrInvalidPerpetualPositionSizeDelta) { | |
| panic( | |
| fmt.Sprintf( | |
| "Invalid perpetual position size delta when processing deleveraging. error: %v", | |
| err, | |
| ), | |
| ) | |
| } else { | |
| // If an error is returned, it's likely because the subaccounts' bankruptcy prices do not overlap. | |
| // TODO(CLOB-75): Support deleveraging subaccounts with non overlapping bankruptcy prices. | |
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | |
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, *offsettingSubaccount.Id) | |
| k.Logger(ctx).Debug( | |
| "Encountered error when processing deleveraging", | |
| "error", err, | |
| "blockHeight", ctx.BlockHeight(), | |
| "checkTx", ctx.IsCheckTx(), | |
| "perpetualId", perpetualId, | |
| "deltaBaseQuantums", deltaBaseQuantums, | |
| "liquidatedSubaccount", liquidatedSubaccount, | |
| "offsettingSubaccount", offsettingSubaccount, | |
| ) | |
| numSubaccountsWithNonOverlappingBankruptcyPrices++ | |
| } | |
| // Try to process the deleveraging operation for both subaccounts. | |
| if err := k.ProcessDeleveraging( | |
| ctx, | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| deltaBaseQuantums, | |
| deltaQuoteQuantums, | |
| ); err == nil { | |
| // Update the remaining liquidatable quantums. | |
| deltaQuantumsRemaining.Sub(deltaQuantumsRemaining, deltaBaseQuantums) | |
| fills = append(fills, types.MatchPerpetualDeleveraging_Fill{ | |
| OffsettingSubaccountId: *offsettingSubaccount.Id, | |
| FillAmount: new(big.Int).Abs(deltaBaseQuantums).Uint64(), | |
| }) | |
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |
| // if the branched state is discarded, so batching is not necessary. | |
| k.GetIndexerEventManager().AddTxnEvent( | |
| ctx, | |
| indexerevents.SubtypeDeleveraging, | |
| indexerevents.DeleveragingEventVersion, | |
| indexer_manager.GetBytes( | |
| indexerevents.NewDeleveragingEvent( | |
| liquidatedSubaccountId, | |
| *offsettingSubaccount.Id, | |
| perpetualId, | |
| satypes.BaseQuantums(new(big.Int).Abs(deltaBaseQuantums).Uint64()), | |
| satypes.BaseQuantums(deltaQuoteQuantums.Uint64()), | |
| deltaBaseQuantums.Sign() > 0, | |
| isFinalSettlement, | |
| ), | |
| ), | |
| ) | |
| } else if errors.Is(err, types.ErrInvalidPerpetualPositionSizeDelta) { | |
| // Handle the error more gracefully instead of panicking | |
| k.Logger(ctx).Error( | |
| "Invalid perpetual position size delta when processing deleveraging", | |
| "error", err, | |
| ) | |
| return err | |
| } else { | |
| // If an error is returned, it's likely because the subaccounts' bankruptcy prices do not overlap. | |
| // TODO(CLOB-75): Support deleveraging subaccounts with non overlapping bankruptcy prices. | |
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | |
| offsettingSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, *offsettingSubaccount.Id) | |
| k.Logger(ctx).Debug( | |
| "Encountered error when processing deleveraging", | |
| "error", err, | |
| "blockHeight", ctx.BlockHeight(), | |
| "checkTx", ctx.IsCheckTx(), | |
| "perpetualId", perpetualId, | |
| "deltaBaseQuantums", deltaBaseQuantums, | |
| "liquidatedSubaccount", liquidatedSubaccount, | |
| "offsettingSubaccount", offsettingSubaccount, | |
| ) | |
| numSubaccountsWithNonOverlappingBankruptcyPrices++ | |
| } |
| "Failed to find subaccounts with open positions on opposite side of liquidated subaccount", | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "perpetualId", perpetualId, | ||
| "deltaQuantumsTotal", deltaQuantumsTotal, | ||
| "liquidatedSubaccount", liquidatedSubaccount, | ||
| ) |
There was a problem hiding this comment.
Suggestion: is it worth noting in the error log that the daemon might have an invalid historical view of the subaccounts with open positions?
Changelist
OffsetSubaccountPerpetualPositionto iterate over subaccounts on a specific side instead of pseudo randomlyTest 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.