paginate liquidation daemon response #2118
Conversation
WalkthroughThe recent changes enhance the liquidation daemon's functionality by introducing new configuration options, particularly for pagination limits. Notably, a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LiquidationDaemon
participant Server
participant DaemonLiquidationInfo
Client->>LiquidationDaemon: Send liquidatable subaccount IDs
LiquidationDaemon->>Server: Process liquidation request
Server->>DaemonLiquidationInfo: Update liquidatable subaccounts with page limit
DaemonLiquidationInfo-->>Server: Confirm update
Server-->>LiquidationDaemon: Send response
LiquidationDaemon-->>Client: Return success
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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- protocol/daemons/flags/flags.go (6 hunks)
- protocol/daemons/flags/flags_test.go (3 hunks)
- protocol/daemons/liquidation/client/grpc_helper.go (2 hunks)
- protocol/daemons/liquidation/client/grpc_helper_test.go (4 hunks)
- protocol/daemons/liquidation/client/sub_task_runner.go (1 hunks)
- protocol/daemons/server/liquidation.go (1 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (3 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (11 hunks)
- protocol/x/clob/abci_test.go (1 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (1 hunks)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
Additional comments not posted (40)
protocol/daemons/server/liquidation.go (1)
45-50: LGTM! Streamlined update logic.The consolidation of multiple update calls into a single
Updatemethod enhances code readability and maintainability.Ensure that the
Updatemethod handles all necessary logic correctly.Run the following script to verify the
Updatemethod usage:Verification successful
The
Updatemethod handles parameters correctly. The method implementation correctly updates the necessary fields with proper locking for thread safety.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Update` method. # Test: Search for the `Update` method implementation and usage. Expect: Correct handling of parameters. ast-grep --lang go --pattern $'func (ls *DaemonLiquidationInfo) Update($_, $_, $_, $_) { $$$ }'Length of output: 1571
protocol/daemons/flags/flags_test.go (3)
29-29: LGTM! New flag integration.The
FlagLiquidationDaemonResponsePageLimitis correctly added to the command flags.
57-57: LGTM! New flag in options map.The flag is correctly set in the
optsMapfor testing.
88-88: LGTM! New flag assertion.The assertion for
FlagLiquidationDaemonResponsePageLimitensures the flag's value is correctly retrieved.Ensure that the flag is used correctly in the codebase.
Run the following script to verify the flag's usage:
Verification successful
Flag Usage Verified:
FlagLiquidationDaemonResponsePageLimitThe flag
FlagLiquidationDaemonResponsePageLimitis correctly retrieved, cast touint64, and used to set theResponsePageLimitin theresult.Liquidationconfiguration. This confirms its proper handling in the codebase.
- The flag is retrieved and used in
protocol/daemons/flags/flags.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `FlagLiquidationDaemonResponsePageLimit`. # Test: Search for the flag usage in the codebase. Expect: Correct handling in logic. rg --type go --word-regexp 'FlagLiquidationDaemonResponsePageLimit'Length of output: 706
Script:
#!/bin/bash # Description: Verify the handling of the retrieved option for `FlagLiquidationDaemonResponsePageLimit`. # Test: Search for the usage of the retrieved option in the codebase. rg --type go 'appOpts.Get\(FlagLiquidationDaemonResponsePageLimit\)'Length of output: 185
Script:
#!/bin/bash # Description: Get surrounding code for the usage of `FlagLiquidationDaemonResponsePageLimit` in `flags.go`. # Test: Extract the surrounding lines of the conditional block where the option is used. rg --type go -A 10 'appOpts.Get\(FlagLiquidationDaemonResponsePageLimit\)' protocol/daemons/flags/flags.goLength of output: 462
protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (4)
29-40: LGTM! Consolidated update method.The
Updatemethod effectively consolidates multiple updates, improving modularity.
70-77: Ensure block height logic is robust.The block height logic in
UpdateNegativeTncSubaccountIdsensures updates are only applied for valid block heights. Verify that panic conditions are handled appropriately.
85-119: Ensure block height logic is robust.The block height logic in
UpdateSubaccountsWithPositionsensures updates are only applied for valid block heights. Verify that panic conditions are handled appropriately.
54-61: Ensure block height logic is robust.The block height logic in
UpdateLiquidatableSubaccountIdsensures updates are only applied for valid block heights. Verify that panic conditions are handled appropriately.Run the following script to verify panic conditions:
Verification successful
Panic Condition in Block Height Logic Verified
The
UpdateLiquidatableSubaccountIdsmethod includes a panic condition that triggers when theblockHeightis less than the currentls.blockHeight. This ensures that updates are only applied for valid block heights, maintaining the robustness of the block height logic. The panic usage is correctly implemented.
- Method:
UpdateLiquidatableSubaccountIds- Panic condition:
panic("UpdateLiquidatableSubaccountIds: block height cannot be less than the current block height")Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify panic conditions in `UpdateLiquidatableSubaccountIds`. # Test: Search for panic usage in the method. Expect: Correct handling of block height conditions. ast-grep --lang go --pattern $'func (ls *DaemonLiquidationInfo) UpdateLiquidatableSubaccountIds($_, $_) { $$$ panic($_) $$$ }'Length of output: 133
Script:
#!/bin/bash # Search for the implementation of the UpdateLiquidatableSubaccountIds method to verify panic usage. ast-grep --lang go --pattern $'func (ls *DaemonLiquidationInfo) UpdateLiquidatableSubaccountIds($_, $_) { $$$ }'Length of output: 1587
protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (9)
28-28: LGTM! Test update for block height.The test for
UpdateLiquidatableSubaccountIdsnow includes block height, aligning with method changes.
42-42: LGTM! Test update for block height.The test for
UpdateNegativeTncSubaccountIdsnow includes block height, aligning with method changes.
63-63: LGTM! Test update for block height.The test for
UpdateSubaccountsWithPositionsnow includes block height, aligning with method changes.
81-97: LGTM! Multiple writes test with block height.The test for multiple writes to
UpdateLiquidatableSubaccountIdscorrectly incorporates block height.
108-124: LGTM! Multiple writes test with block height.The test for multiple writes to
UpdateNegativeTncSubaccountIdscorrectly incorporates block height.
Line range hint
143-184: LGTM! Multiple writes test with block height.The test for multiple writes to
UpdateSubaccountsWithPositionscorrectly incorporates block height.
199-205: LGTM! Empty update test with block height.The test for empty updates to
UpdateLiquidatableSubaccountIdscorrectly incorporates block height.
216-222: LGTM! Empty update test with block height.The test for empty updates to
UpdateNegativeTncSubaccountIdscorrectly incorporates block height.
240-250: LGTM! Empty update test with block height.The test for empty updates to
UpdateSubaccountsWithPositionscorrectly incorporates block height.protocol/daemons/liquidation/client/sub_task_runner.go (1)
86-86: LGTM! Enhanced task loop with response page limit.The addition of
liqFlags.ResponsePageLimitenhances the function's ability to manage response sizes efficiently.protocol/daemons/flags/flags.go (5)
29-29: New constantFlagLiquidationDaemonResponsePageLimitadded.This constant is introduced to define a limit on the number of items sent to the main application by the Liquidation Daemon. Ensure that this flag is used consistently across the codebase.
67-68: New fieldResponsePageLimitadded toLiquidationFlags.This field is used to store the pagination limit for responses sent by the Liquidation Daemon. Ensure that this field is properly initialized and used throughout the application.
112-112: Default value forResponsePageLimitset to 2,000.This default value is set in the
GetDefaultDaemonFlagsfunction. Ensure that this value is appropriate for the expected load and can be overridden if necessary.
191-195: CLI flag forResponsePageLimitadded.The command-line interface now includes a flag for setting the
ResponsePageLimit. Ensure that this is documented and that users are aware of how to configure this setting.
289-293: Handling ofResponsePageLimitinGetDaemonFlagValuesFromOptions.The function now retrieves the
ResponsePageLimitfrom application options. Ensure that this integration is tested to confirm that the value is correctly parsed and applied.protocol/daemons/liquidation/client/grpc_helper.go (7)
214-214: AddedpageLimitparameter toSendLiquidatableSubaccountIds.This parameter allows for pagination of requests, improving the handling of large datasets. Ensure that this parameter is consistently used in all related function calls.
245-252: Use ofGenerateLiquidateSubaccountsPaginatedRequests.The function generates paginated requests for liquidatable subaccount IDs. Ensure that the pagination logic correctly handles edge cases, such as when the number of IDs is less than the page limit.
255-262: Use ofGenerateNegativeTNCSubaccountsPaginatedRequests.This function appends paginated requests for negative TNC subaccount IDs. Verify that the logic correctly handles different sizes of input lists.
265-272: Use ofGenerateSubaccountOpenPositionPaginatedRequests.This function appends paginated requests for subaccount open position info. Ensure that the pagination logic is efficient and handles all potential input scenarios.
283-307: FunctionGenerateLiquidateSubaccountsPaginatedRequestsadded.This function creates paginated requests for liquidatable subaccounts. Ensure that the logic correctly handles empty input and that the pagination is efficient.
309-333: FunctionGenerateNegativeTNCSubaccountsPaginatedRequestsadded.This function creates paginated requests for negative TNC subaccounts. Ensure that the logic correctly handles empty input and that the pagination is efficient.
335-382: FunctionGenerateSubaccountOpenPositionPaginatedRequestsadded.This function creates paginated requests for subaccount open positions. Ensure that the logic correctly handles empty input and that the pagination is efficient.
protocol/daemons/liquidation/client/grpc_helper_test.go (7)
472-475: Test case forLiquidateSubaccountswithLiquidatableSubaccountIds.This test case ensures that requests with liquidatable subaccount IDs are handled correctly. Verify that the test covers all relevant scenarios and edge cases.
476-482: Test case forLiquidateSubaccountswithNegativeTncSubaccountIds.This test case ensures that requests with negative TNC subaccount IDs are handled correctly. Verify that the test covers all relevant scenarios and edge cases.
483-497: Test case forLiquidateSubaccountswithSubaccountOpenPositionInfo.This test case ensures that requests with subaccount open position info are handled correctly. Verify that the test covers all relevant scenarios and edge cases.
538-543: Test case for emptyLiquidatableSubaccountIds.This test case ensures that the function handles empty lists of liquidatable subaccount IDs correctly. Verify that the test covers all relevant scenarios and edge cases.
544-550: Test case for emptyNegativeTncSubaccountIds.This test case ensures that the function handles empty lists of negative TNC subaccount IDs correctly. Verify that the test covers all relevant scenarios and edge cases.
551-555: Test case for emptySubaccountOpenPositionInfo.This test case ensures that the function handles empty lists of subaccount open position info correctly. Verify that the test covers all relevant scenarios and edge cases.
565-566: Error propagation test case.This test case ensures that errors are correctly propagated when requests fail. Verify that the test covers all relevant scenarios and edge cases.
protocol/x/clob/abci_test.go (1)
1268-1271: UpdatedUpdateLiquidatableSubaccountIdsto include block height.The method now requires the block height as an additional parameter, providing more context. Ensure that this change is correctly integrated and that the block height is used appropriately.
protocol/x/clob/keeper/deleveraging_test.go (1)
870-871: UpdatedUpdateSubaccountsWithPositionsto include block height.The method now requires the block height as an additional parameter, providing more context. Ensure that this change is correctly integrated and that the block height is used appropriately.
protocol/x/clob/keeper/liquidations_test.go (1)
2136-2136: Verify the usage ofuint32(ctx.BlockHeight()).Ensure that the
UpdateSubaccountsWithPositionsmethod correctly handles the block height parameter. This change might affect how subaccount positions are updated based on the block height.Run the following script to verify the implementation of
UpdateSubaccountsWithPositionsand ensure it handles the block height correctly:Verification successful
Block height handling is correctly implemented in
UpdateSubaccountsWithPositions.The function properly resets or appends to the
subaccountsWithPositionsmap based on the block height, ensuring accurate updates to subaccount positions. No issues found with the current implementation.
- The map is reset if the block height increases.
- The map is appended to if the block height remains the same.
- A panic is triggered if the block height decreases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `UpdateSubaccountsWithPositions` to ensure it handles the block height. # Test: Search for the method implementation. Expect: Correct handling of block height. ast-grep --lang go --pattern $'func ($_ $_) UpdateSubaccountsWithPositions($_, $_) { $$$ }'Length of output: 5011
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/daemons/liquidation/client/sub_task_runner_test.go (11 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (3 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go
Additional comments not posted (5)
protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (3)
59-64: LGTM!The function correctly appends updates to the list of liquidatable subaccount IDs.
68-73: LGTM!The function correctly appends updates to the list of negative TNC subaccount IDs.
75-98: LGTM!The function effectively manages updates to the map of subaccounts with positions, ensuring no duplicate entries.
protocol/daemons/liquidation/client/sub_task_runner_test.go (2)
Line range hint
91-699: Refactoring improves readability and maintainability.The use of a helper function for setting up mock requests reduces code duplication and enhances test readability.
733-786: LGTM!The helper function is well-structured and effectively centralizes the setup of mock requests, improving test setup consistency.
| func (ls *DaemonLiquidationInfo) Update( | ||
| blockHeight uint32, | ||
| liquidatableSubaccountIds []satypes.SubaccountId, | ||
| negativeTncSubaccountIds []satypes.SubaccountId, | ||
| subaccountsWithPositions []clobtypes.SubaccountOpenPositionInfo, | ||
| ) { | ||
| ls.Lock() | ||
| defer ls.Unlock() | ||
| ls.blockHeight = blockHeight | ||
|
|
||
| if blockHeight > ls.blockHeight { | ||
| ls.liquidatableSubaccountIds = make([]satypes.SubaccountId, 0) | ||
| ls.negativeTncSubaccountIds = make([]satypes.SubaccountId, 0) | ||
| ls.subaccountsWithPositions = make(map[uint32]*clobtypes.SubaccountOpenPositionInfo) | ||
| } else if blockHeight < ls.blockHeight { | ||
| panic("UpdateLiquidatableSubaccountIds: block height cannot be less than the current block height") | ||
| } | ||
| ls.UpdateBlockHeight(blockHeight) | ||
|
|
||
| ls.UpdateLiquidatableSubaccountIds(liquidatableSubaccountIds, blockHeight) | ||
| ls.UpdateNegativeTncSubaccountIds(negativeTncSubaccountIds, blockHeight) | ||
| ls.UpdateSubaccountsWithPositions(subaccountsWithPositions, blockHeight) |
There was a problem hiding this comment.
Consider replacing panic with error handling.
Using panic for block height discrepancies might not be ideal in a production environment. Consider returning an error instead to handle this case more gracefully.
Refactor the function to return an error:
func (ls *DaemonLiquidationInfo) Update(
blockHeight uint32,
liquidatableSubaccountIds []satypes.SubaccountId,
negativeTncSubaccountIds []satypes.SubaccountId,
subaccountsWithPositions []clobtypes.SubaccountOpenPositionInfo,
) error {
ls.Lock()
defer ls.Unlock()
if blockHeight > ls.blockHeight {
ls.liquidatableSubaccountIds = make([]satypes.SubaccountId, 0)
ls.negativeTncSubaccountIds = make([]satypes.SubaccountId, 0)
ls.subaccountsWithPositions = make(map[uint32]*clobtypes.SubaccountOpenPositionInfo)
} else if blockHeight < ls.blockHeight {
return fmt.Errorf("block height cannot be less than the current block height")
}
ls.UpdateBlockHeight(blockHeight)
ls.UpdateLiquidatableSubaccountIds(liquidatableSubaccountIds, blockHeight)
ls.UpdateNegativeTncSubaccountIds(negativeTncSubaccountIds, blockHeight)
ls.UpdateSubaccountsWithPositions(subaccountsWithPositions, blockHeight)
return nil
}
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/daemons/server/types/liquidations/daemon_liquidation_info.go (4 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (2 hunks)
Additional comments not posted (6)
protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (4)
59-61: LGTM!The
UpdateBlockHeightmethod correctly updates the block height.
66-71: LGTM!The
UpdateLiquidatableSubaccountIdsmethod correctly appends new subaccount IDs.
73-80: LGTM!The
UpdateNegativeTncSubaccountIdsmethod correctly appends new subaccount IDs.
82-105: LGTM!The
UpdateSubaccountsWithPositionsmethod correctly updates the map of subaccounts with open positions.protocol/x/clob/e2e/withdrawal_gating_test.go (2)
415-415: Verify the staticBlockHeightvalue.The
BlockHeightis set to a static value of3. Ensure this aligns with the test's objectives.
517-518: LGTM!The dynamic calculation of
BlockHeightcorrectly simulates different scenarios.
| func (ls *DaemonLiquidationInfo) Update( | ||
| blockHeight uint32, | ||
| liquidatableSubaccountIds []satypes.SubaccountId, | ||
| negativeTncSubaccountIds []satypes.SubaccountId, | ||
| subaccountsWithPositions []clobtypes.SubaccountOpenPositionInfo, | ||
| ) { | ||
| ls.Lock() | ||
| defer ls.Unlock() | ||
| ls.blockHeight = blockHeight | ||
|
|
||
| if blockHeight > ls.blockHeight { | ||
| ls.liquidatableSubaccountIds = make([]satypes.SubaccountId, 0) | ||
| ls.negativeTncSubaccountIds = make([]satypes.SubaccountId, 0) | ||
| ls.subaccountsWithPositions = make(map[uint32]*clobtypes.SubaccountOpenPositionInfo) | ||
| } else if blockHeight < ls.blockHeight { | ||
| panic( | ||
| fmt.Sprintf( | ||
| "UpdateLiquidatableSubaccountIds: block height %d cannot be less than the current block height %d", | ||
| blockHeight, | ||
| ls.blockHeight, | ||
| ), | ||
| ) | ||
| } | ||
| ls.UpdateBlockHeight(blockHeight) | ||
|
|
||
| ls.UpdateLiquidatableSubaccountIds(liquidatableSubaccountIds, blockHeight) | ||
| ls.UpdateNegativeTncSubaccountIds(negativeTncSubaccountIds, blockHeight) | ||
| ls.UpdateSubaccountsWithPositions(subaccountsWithPositions, blockHeight) |
There was a problem hiding this comment.
Consider replacing panic with error handling.
Using panic for block height discrepancies might not be ideal in a production environment. Consider returning an error instead to handle this case more gracefully.
Refactor the function to return an error:
func (ls *DaemonLiquidationInfo) Update(
blockHeight uint32,
liquidatableSubaccountIds []satypes.SubaccountId,
negativeTncSubaccountIds []satypes.SubaccountId,
subaccountsWithPositions []clobtypes.SubaccountOpenPositionInfo,
) error {
ls.Lock()
defer ls.Unlock()
if blockHeight > ls.blockHeight {
ls.liquidatableSubaccountIds = make([]satypes.SubaccountId, 0)
ls.negativeTncSubaccountIds = make([]satypes.SubaccountId, 0)
ls.subaccountsWithPositions = make(map[uint32]*clobtypes.SubaccountOpenPositionInfo)
} else if blockHeight < ls.blockHeight {
return fmt.Errorf("block height cannot be less than the current block height")
}
ls.UpdateBlockHeight(blockHeight)
ls.UpdateLiquidatableSubaccountIds(liquidatableSubaccountIds, blockHeight)
ls.UpdateNegativeTncSubaccountIds(negativeTncSubaccountIds, blockHeight)
ls.UpdateSubaccountsWithPositions(subaccountsWithPositions, blockHeight)
return nil
}
|
@Mergifyio backport release/protocol/v5.2.x |
✅ Backports have been created
|
(cherry picked from commit 909fc3a)
Co-authored-by: jayy04 <[email protected]>
|
@Mergifyio backport release/protocol/v6.x |
✅ Backports have been created
|
(cherry picked from commit 909fc3a)
Co-authored-by: jayy04 <[email protected]>
Co-authored-by: jayy04 <[email protected]>
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
Bug Fixes
Refactor
Tests