[CLOB-1043] replicate IsLiquidatable logic on daemon #873
Conversation
WalkthroughThe overall change streamlines the configuration of a liquidation daemon by consolidating two flags into one, affecting flag handling and related tests. It also involves significant refactoring of the liquidation client, including the removal of certain parameters and functions, and the introduction of new methods and tests to accommodate changes in the liquidation logic. Additionally, there are updates to perpetual contract calculations and the addition of new constants and test utilities. 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: 7
Configuration used: CodeRabbit UI
Files selected for processing (12)
- protocol/daemons/flags/flags.go (5 hunks)
- protocol/daemons/flags/flags_test.go (3 hunks)
- protocol/daemons/liquidation/client/client_test.go (2 hunks)
- protocol/daemons/liquidation/client/grpc_helper.go (5 hunks)
- protocol/daemons/liquidation/client/grpc_helper_test.go (10 hunks)
- protocol/daemons/liquidation/client/sub_task_runner.go (4 hunks)
- protocol/daemons/liquidation/client/sub_task_runner_test.go (1 hunks)
- protocol/lib/collections.go (1 hunks)
- protocol/lib/collections_test.go (1 hunks)
- protocol/testutil/constants/subaccounts.go (1 hunks)
- protocol/x/clob/keeper/liquidations.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (3 hunks)
Additional comments: 37
protocol/daemons/flags/flags.go (5)
20-26: The consolidation of the flags
LiquidationDaemonSubaccountPageLimitandLiquidationDaemonRequestChunkSizeintoFlagLiquidationDaemonQueryPageLimitis reflected correctly in the constants section. This change simplifies configuration and should be verified for correct implementation throughout the codebase.51-55: The
LiquidationFlagsstruct has been updated to include the newQueryPageLimitfield, which is consistent with the consolidation of the flags. Ensure that all references to the old flags are updated to use this new field.88-93: The default values for the
LiquidationFlagswithin thedefaultDaemonFlagssingleton have been set correctly, reflecting the newQueryPageLimitfield. This ensures that the daemon will have sensible defaults if no configuration is provided.157-162: The
AddDaemonFlagsToCmdfunction has been updated to include the new flagFlagLiquidationDaemonQueryPageLimit. This change allows for the new configuration to be set via command-line arguments, which is a necessary update following the consolidation of the flags.227-234: The
GetDaemonFlagValuesFromOptionsfunction has been updated to handle the newFlagLiquidationDaemonQueryPageLimit. This change is necessary to ensure that the daemon can retrieve the new configuration value from the application options.protocol/daemons/flags/flags_test.go (3)
25-31: The update to the test
TestAddDaemonFlagsToCmdreflects the consolidation of the flags correctly, ensuring that the new flagFlagLiquidationDaemonQueryPageLimitis included in the command's flags.52-58: The update to the test
TestGetDaemonFlagValuesFromOptions_Customalso correctly reflects the change fromFlagLiquidationDaemonSubaccountPageLimittoFlagLiquidationDaemonQueryPageLimit, ensuring that the new flag is being set and retrieved correctly.82-88: The assertion in
TestGetDaemonFlagValuesFromOptions_Customfor the newFlagLiquidationDaemonQueryPageLimitis correctly checking that the value set in the options map is equal to the value retrieved from theGetDaemonFlagValuesFromOptionsfunction, which confirms that the flag value is being passed through correctly.protocol/daemons/liquidation/client/client_test.go (3)
7-19: The removal of the
TestRunLiquidationDaemonTaskLoopfunction and its associated test cases is consistent with the PR's objective to refactor the liquidation daemon's logic. However, ensure that the logic tested by these cases is now covered by new tests, possibly insub_task_runner_test.goas mentioned in the PR overview.67-69: The addition of
FakeSubTaskRunnerand its methods appears to be a mock implementation for testing purposes. Ensure that this mock is utilized effectively in new test cases and that it aligns with the PR's objective to enhance testing around the liquidation daemon's logic.64-65: The
TestHealthCheck_Mixedfunction is testing the health check functionality of the liquidation daemon. Verify that the test cases cover all new scenarios introduced by the PR and that the health check logic is consistent with the daemon's updated behavior.protocol/daemons/liquidation/client/grpc_helper.go (4)
- 44-49: The
blockHeightparameter has been removed from theGetAllPerpetualsfunction. Ensure that all calls to this function have been updated to reflect this change and that the logic within the function does not require theblockHeightto function correctly.
Final review comment:
The removal of the
blockHeightparameter fromGetAllPerpetualshas been noted. However, the verification scripts did not yield any results, which prevents confirmation of whether all calls to this function have been appropriately updated or if the function is now unused. Further verification is required to ensure the integrity of the codebase after this change.
- 83-88: Similar to the
GetAllPerpetualsfunction, theblockHeightparameter has been removed from theGetAllLiquidityTiersfunction. Verify that all calls to this function have been updated accordingly and that the removal ofblockHeightdoes not negatively impact the function's logic.
The removal of the
blockHeightparameter fromGetAllLiquidityTiershas been noted. The verification scripts did not find any instances of the old or new function signatures, which suggests there may be no outdated calls. However, without any output, further verification could be warranted to ensure that all calls have been updated and that the logic is not negatively impacted.
- 122-127: The
blockHeightparameter has also been removed from theGetAllMarketPricesfunction. Confirm that all calls to this function have been modified to match the new signature and that the function's internal logic is not compromised by the absence ofblockHeight.
The removal of the
blockHeightparameter fromGetAllMarketPriceshas been noted. However, the verification scripts did not find any instances of the function's old or new signatures, which does not provide evidence of the function's usage in the codebase. It is recommended to verify that this change does not affect any other parts of the codebase where the function might be used.
- 204-206: The
SendLiquidatableSubaccountIdsfunction is present without any changes. Since the AI-generated summary mentioned the removal ofCheckCollateralizationForSubaccounts, ensure that this function or any other part of the code does not depend on the removed function, which could lead to broken functionality.
The removal of
CheckCollateralizationForSubaccountsdoes not appear to have left any direct references in the codebase, suggesting no immediate broken functionality. The changes are verified based on the current context.protocol/daemons/liquidation/client/grpc_helper_test.go (10)
92-98: The change from
df.Liquidation.SubaccountPageLimittodf.Liquidation.QueryPageLimitin the test setup forTestGetAllSubaccountsreflects the consolidation of flags mentioned in the PR overview. This change appears to be correctly implemented in the test setup.112-118: The same flag consolidation change is correctly applied in the paginated success case for
TestGetAllSubaccounts.128-134: The flag consolidation change is also correctly applied in the second request of the paginated success case for
TestGetAllSubaccounts.147-153: The flag consolidation change is correctly applied in the error propagation case for
TestGetAllSubaccounts.165-171: The test case
TestGetAllSubaccountscorrectly passes the consolidated flagdf.Liquidation.QueryPageLimitto theGetAllSubaccountsmethod.257-262: The test case
TestGetAllPerpetualsdoes not seem to be affected by the flag consolidation, as it uses a hardcoded limit for pagination. No action is required here.345-350: The test case
TestGetAllLiquidityTiersalso does not seem to be affected by the flag consolidation, as it uses a hardcoded limit for pagination. No action is required here.438-443: The test case
TestGetAllMarketPricesdoes not seem to be affected by the flag consolidation, as it uses a hardcoded limit for pagination. No action is required here.14-19: The imports in the
grpc_helper_test.gofile do not show any direct reference to the removedCheckCollateralizationForSubaccountsfunction. This suggests that the removal has been handled correctly, assuming that the function was not part of the imported packages.452-453: The test
TestSendLiquidatableSubaccountIdsis not directly related to the changes mentioned in the PR overview, but it is important to ensure that the test cases are consistent with the existing subaccounts and that the logic is sound. The test cases appear to be correctly set up and executed.protocol/daemons/liquidation/client/sub_task_runner.go (4)
5-17: The new imports are consistent with the added functionality in the file.
89-143: The new method
FetchApplicationStateAtBlockHeightis well-structured and uses theUniqueSliceToMaputility function to convert slices to maps, ensuring uniqueness of keys. This is a good use of utility functions to avoid code duplication and potential errors.50-77: The modifications to
RunLiquidationDaemonTaskLoopto incorporate the newFetchApplicationStateAtBlockHeightmethod and additional parameters are consistent with the PR's objective to replicate theIsLiquidatablelogic within the liquidation daemon.145-154: The
GetLiquidatableSubaccountIdsmethod has been updated to take additional parameters, which are used to determine if subaccounts are liquidatable. This change is necessary for the new liquidation logic.protocol/daemons/liquidation/client/sub_task_runner_test.go (1)
- 1-253: The test file
sub_task_runner_test.gohas been added with comprehensive test cases for theRunLiquidationDaemonTaskLoopfunction. The tests cover various scenarios, including liquidatable subaccounts, well-collateralized subaccounts, and subaccounts with no open positions. The mocks are correctly set up, and the test cases verify that the function behaves as expected under different conditions. The use oft.Runensures that each test case is isolated, and the assertions are appropriate for the expected outcomes. Overall, the test cases are well-structured and maintainable.protocol/lib/collections_test.go (1)
- 107-157: The test function
TestUniqueSliceToMapis well-structured and covers a variety of test cases, including empty input, basic input without duplicates, and input with duplicates that should cause a panic. The use of a struct with anIdfield for testing is appropriate, as it simulates a more complex scenario than using primitive types. The panic behavior is correctly tested withrequire.PanicsWithValue, ensuring that the function panics with the expected message when duplicates are encountered. Overall, the test function appears to be thorough and correctly implemented.protocol/testutil/constants/subaccounts.go (1)
- 331-345: The addition of the new subaccount
Dave_Num0_1BTC_Long_45000USD_Shortis consistent with the existing structure and naming conventions of the file. The data types and values used are in line with the other subaccount definitions.protocol/x/perpetuals/keeper/perpetual.go (5)
771-788: The logic for calculating net notional has been refactored into a standalone function
GetNetNotional. Ensure that all calls to the originalKeepermethod are updated to use the new standalone function and that the logic remains consistent with the previous implementation.774-788: The function
GetNetNotionalhas been made public. Confirm that this change in visibility is intentional and that the function should be accessible from outside thekeeperpackage.891-915: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [904-934]
The function
GetMarginRequirementshas been made public. Confirm that this change in visibility is intentional and that the function should be accessible from outside thekeeperpackage.
774-788: The standalone function
GetNetNotionaldoes not use a pointer receiver, unlike the originalKeepermethod. Confirm that this change is intentional and that there is no need for the function to modify the state of its receiver.891-915: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [904-934]
The standalone function
GetMarginRequirementsdoes not use a pointer receiver, unlike the originalKeepermethod. Confirm that this change is intentional and that there is no need for the function to modify the state of its receiver.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/liquidation/client/sub_task_runner.go (4 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/x/clob/keeper/liquidations.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/daemons/liquidation/client/sub_task_runner.go
Additional comments: 9
protocol/lib/metrics/constants.go (1)
- 293-299: The addition of the
FetchApplicationStateAtBlockHeightconstant is consistent with the PR's objective and the AI-generated summary. This new constant likely supports the new functionality for fetching application state at a specific block height, which is part of the refactoring of the liquidation logic.protocol/x/clob/keeper/liquidations.go (2)
366-378: The new
CanLiquidatefunction correctly encapsulates the logic to determine if a subaccount is liquidatable based on its net collateral and maintenance margin requirements. This function improves modularity by separating the liquidation check from theKeeperstructure, which can lead to easier testing and reuse.363-380: Verify that the integration of the new
CanLiquidatefunction is consistent throughout the codebase, and that any logic that previously depended onIsLiquidatablehas been updated accordingly.
The new
CanLiquidatefunction is being used insub_task_runner.goandliquidations.go, which aligns with the changes described in the PR. However, references to the oldIsLiquidatablemethod still exist in various files, includingquery.pb.go,liquidations_keeper.go,mem_clob_keeper.go,process_operations.go,liquidations.go,liquidations_test.go,grpc_query_are_subaccounts_liquidatable.go,memclob/keeper.go,ClobKeeper.go, andMemClobKeeper.go. These references need to be checked to ensure they have been updated to work with the newCanLiquidatefunction or are part of the intended remaining functionality.Final review comment:
Verify that the remaining references to `IsLiquidatable` are intentional and correctly updated to work with the new `CanLiquidate` function, or if they are part of the intended functionality that should remain unchanged.protocol/x/perpetuals/keeper/perpetual.go (6)
771-793: The refactor of
GetNetNotionalto use the new stateless functionGetNetNotionalInQuoteQuantumsis correct and maintains the original logic. The new function encapsulates the calculation logic, which can be reused elsewhere without depending on theKeeperstruct.894-922: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [897-941]
The refactor of
GetMarginRequirementsto use the new stateless functionGetMarginRequirementsInQuoteQuantumsis correct. The new function does not return an error, which is justified because the error conditions are checked before the function is called. This change simplifies the function signature and makes the function purely computational.
774-793: The implementation of
GetNetNotionalInQuoteQuantumsis correct. It encapsulates the logic for calculating the net notional in quote quantums, which is a stateless operation. This function can now be reused in different contexts without requiring aKeeperinstance.894-922: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [907-941]
The implementation of
GetMarginRequirementsInQuoteQuantumsis correct. It encapsulates the logic for calculating initial and maintenance margin requirements in quote quantums, which is a stateless operation. The function correctly handles the calculation based on the absolute value of the position size and the relevantLiquidityTierparameters.
- 768-785: Ensure that all references to the old
GetNetNotionalmethod are updated to use the newGetNetNotionalInQuoteQuantumsfunction, as the method signature has changed.
All references to the old
GetNetNotionalmethod appear to have been updated to the newGetNetNotionalInQuoteQuantumsfunction, as no occurrences of the old method signature were found in the codebase.
- 894-918: Ensure that all references to the old
GetMarginRequirementsmethod are updated to use the newGetMarginRequirementsInQuoteQuantumsfunction, as the method signature has changed.
Final review comment:
Ensure that all references to the old
GetMarginRequirementsmethod are updated to use the newGetMarginRequirementsInQuoteQuantumsfunction, as the method signature has changed. The verification script did not return any results, which is inconclusive. Please manually verify that all references have been updated accordingly.
| if !ok { | ||
| panic( | ||
| fmt.Sprintf( | ||
| "Perpetual not found for perpetual id %d", |
There was a problem hiding this comment.
Can we panic with an error instead of a fmt.Sprintf? I believe it would work better with the dd error tracking when we recover the panic, we will get better error tracking.
There was a problem hiding this comment.
actually I updated to return error instead. caller will logger.Error and propagate error to upstream
| // CanLiquidate returns true if the subaccount is able to be liquidated given the total net collateral | ||
| // and maintenance margin requirement of the subaccount. | ||
| // | ||
| // The subaccount is liquidatable if both of the following are true: |
There was a problem hiding this comment.
| // The subaccount is liquidatable if both of the following are true: | |
| // The given TNC, MMR pair is liquidatable if both of the following are true: |
There was a problem hiding this comment.
hmmm I think calling TNC/MMR pair liquidatable is slightly weird.
Going to leave this as is if that's okay but I renamed the function to CanLiquidatateSubaccount, hopefully this docstring is more appropriate now
| for _, subaccount := range subaccounts { | ||
| if len(subaccount.PerpetualPositions) > 0 { | ||
| subaccountsToCheck = append(subaccountsToCheck, *subaccount.Id) | ||
| // Skip subaccounts with no open positions. |
There was a problem hiding this comment.
For my understanding, why did we add this again?
There was a problem hiding this comment.
just to early return since MMR will be 0 and subaccount won't be liquidatable. no need to calculate TNC/MMR in this case.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/daemons/liquidation/client/sub_task_runner.go (4 hunks)
- protocol/x/clob/keeper/liquidations.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/liquidations.go
Additional comments: 10
protocol/daemons/liquidation/client/sub_task_runner.go (6)
5-17:
The new imports added are consistent with the new functionality introduced in thesub_task_runner.gofile, such as fetching application state and checking subaccount collateralization.89-150:
TheFetchApplicationStateAtBlockHeightmethod is a new addition that encapsulates the fetching of application state at a given block height. This method appears to be well-structured and modular, improving the maintainability of the code.50-77:
TheRunLiquidationDaemonTaskLoopmethod has been updated to use the newFetchApplicationStateAtBlockHeightmethod to gather necessary data and then process it to find liquidatable subaccount IDs. The error handling and data flow between these methods are correctly implemented.167-287: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [152-203]
The
GetLiquidatableSubaccountIdsmethod has been updated to accept new parameters that are used to determine if a subaccount is liquidatable. The method's logic and integration with the new parameters appear to be correct.
206-287:
TheCheckSubaccountCollateralizationmethod is a new addition that performs collateralization checks on subaccounts. The method's logic appears to be correct, and it uses the new parameters effectively to determine if a subaccount is liquidatable.47-77: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [47-86]
Telemetry is used effectively throughout the changes to measure the performance of operations, and error handling is consistent and appropriate, ensuring that errors are returned and handled by the caller.
protocol/x/perpetuals/keeper/perpetual.go (4)
771-795: The refactoring of
GetNetNotionalto use the new stateless functionGetNetNotionalInQuoteQuantumsis correct. The new function encapsulates the logic for calculating net notional in quote quantums, which simplifies the original function and makes the code more maintainable.896-924: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [899-943]
The refactoring of
GetMarginRequirementsto use the new stateless functionGetMarginRequirementsInQuoteQuantumsis correct. The new function encapsulates the logic for calculating initial and maintenance margin requirements in quote quantums, which simplifies the original function and makes the code more maintainable.
774-795: The new function
GetNetNotionalInQuoteQuantumscorrectly returns a*big.Inttype, which matches the return type of the originalGetNetNotionalfunction. This ensures consistency in the return types and allows for a seamless transition to the new function.896-924: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [909-943]
The new function
GetMarginRequirementsInQuoteQuantumscorrectly returns two*big.Inttypes for initial and maintenance margin requirements, which matches the return types of the originalGetMarginRequirementsfunction. This ensures consistency in the return types and allows for a seamless transition to the new function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/liquidation/client/sub_task_runner.go (4 hunks)
Additional comments: 6
protocol/daemons/liquidation/client/sub_task_runner.go (6)
5-17: The addition of new imports is consistent with the changes described in the PR objectives and AI-generated summary.
50-77: The implementation of fetching application state and identifying liquidatable subaccounts appears to be correct and aligns with the PR objectives. Ensure that the error handling is consistent throughout the new code.
89-150: The new
FetchApplicationStateAtBlockHeightmethod is well-structured and includes proper error handling. It also uses theUniqueSliceToMapfunction, which is consistent with the changes mentioned in the AI-generated summary.170-203: The changes to the
GetLiquidatableSubaccountIdsmethod, including the addition of new parameters and the logic to check for liquidatable subaccounts, are consistent with the PR objectives. The error handling and logging are also consistent with best practices.206-290: The new
CheckSubaccountCollateralizationmethod is implemented correctly, with checks for asset support and error handling for missing data. This method is crucial for the liquidation logic and aligns with the PR objectives.173-173: The discussion in the comment chain on line 173 regarding the early return for subaccounts with no open positions seems valid and is a good optimization to avoid unnecessary calculations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/daemons/liquidation/client/sub_task_runner.go (4 hunks)
- protocol/daemons/liquidation/client/sub_task_runner_test.go (1 hunks)
- protocol/testutil/constants/perpetuals.go (1 hunks)
- protocol/testutil/constants/subaccounts.go (4 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (5 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testutil/constants/perpetuals.go
Additional comments: 17
protocol/daemons/liquidation/client/sub_task_runner.go (4)
90-151: The
FetchApplicationStateAtBlockHeightmethod is well-structured and includes comprehensive error handling and resource management. The use ofdeferfor telemetry is appropriate, and the method's comments provide clear documentation of its purpose and functionality.207-301: The
CheckSubaccountCollateralizationmethod correctly uses big integers for financial calculations, which is crucial for precision in financial applications. The error handling is robust, with early returns in case of unsupported assets or missing data, and the method is well-documented.168-302: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [153-205]
The modifications to the
GetLiquidatableSubaccountIdsmethod align with the new collateralization check logic. The method efficiently constructs a list of liquidatable subaccount IDs and uses telemetry to monitor the number of subaccounts with open positions. The logic is consistent, and there are no apparent data races or performance issues.
- 48-78: The restructuring of the
RunLiquidationDaemonTaskLoopmethod to incorporate the newFetchApplicationStateAtBlockHeightandGetLiquidatableSubaccountIdsmethods is logical and necessary. The flow of data between methods is clear, and the overall logic is consistent with the PR's objectives.protocol/daemons/liquidation/client/sub_task_runner_test.go (1)
- 23-525: The test cases in
sub_task_runner_test.goare comprehensive and cover a variety of scenarios that the liquidation daemon might encounter. The use of constants for setting up mocks promotes maintainability, and the structure of the tests with clear setup and expectations is commendable. Ensure that the constants used in the test cases are defined correctly and that the mocks are being called with the expected arguments. Additionally, verify that theRunLiquidationDaemonTaskLoopfunction behaves as expected based on the flags provided and that both error handling and success paths are properly tested with the correct assertions.protocol/testutil/constants/subaccounts.go (2)
144-146: The addition of the
FundingIndexfield to thePerpetualPositionstruct is consistent with the PR's objective to refactor the liquidation logic. Ensure that all other instances ofPerpetualPositionin the codebase are updated accordingly if they require theFundingIndex.333-348: The addition of the new subaccount
Dave_Num0_1BTC_Long_45000USD_Shortis consistent with the PR's objectives. The struct is correctly initialized with theFundingIndexfield set todtypes.NewInt(0), which aligns with the changes made to thePerpetualPositionstruct.protocol/x/perpetuals/keeper/perpetual.go (4)
771-795: The refactoring of
GetNetNotionalto use the new utility functionGetNetNotionalInQuoteQuantumsis correct and improves modularity by separating the calculation logic into a stateless function. This change should make the code easier to test and maintain.896-924: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [899-943]
The refactoring of
GetMarginRequirementsto use the new utility functionGetMarginRequirementsInQuoteQuantumsis correct and improves modularity by separating the calculation logic into a stateless function. This change should make the code easier to test and maintain.
- 969-999: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [972-1006]
The refactoring of
GetSettlementPpmto use the new utility functionGetSettlementPpmWithPerpetualis correct and improves modularity by separating the calculation logic into a stateless function. This change should make the code easier to test and maintain.
- 969-999: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [771-1006]
The changes to the utility functions are consistent with the summary, which mentions the addition of new functions in the
perpetual.gofile. However, the summary also mentions other changes such as the consolidation of flags, removal of parameters, and movement of logic to standalone functions, which are not visible in the provided code. Ensure that these changes are reviewed in the appropriate parts of the codebase.protocol/x/subaccounts/keeper/subaccount.go (5)
20-21: The addition of new imports for
perpkeeperandperptypesis consistent with the PR's objective to enhance the liquidation logic, as these packages likely contain relevant functionality for perpetual contracts.395-407: The function
GetSettledSubaccountWithPerpetualsis correctly documented as a stateless utility function. It's crucial to ensure that it does not modify any external state and that its behavior is deterministic based on the inputs provided.416-422: The error handling within the loop that settles perpetual positions is robust, using
errorsmod.Wrapto provide context for the error. This is important for debugging and understanding the cause of errors when they occur.426-430: The call to
perpkeeper.GetSettlementPpmWithPerpetualencapsulates the logic for calculating net settlement and new funding index. It's important to verify that this function is well-tested and that its usage here is consistent with its intended purpose.431-433: The logic for recording non-zero funding payments is crucial for the correct functioning of the liquidation process. It's important to ensure that the funding payment calculation is accurate and that the events emitted reflect the actual state changes.
protocol/x/subaccounts/types/expected_keepers.go (1)
- 65-71: The addition of the
GetPerpetualmethod to thePerpetualsKeeperinterface will require all existing implementations of this interface to be updated to include this new method. Ensure that all concrete types that implementPerpetualsKeeperhave been updated accordingly to avoid compilation errors.
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(-100_000_000), // -1 BTC | ||
| FundingIndex: dtypes.NewInt(0), |
There was a problem hiding this comment.
The FundingIndex field is added to PerpetualPosition here, but it is missing in other instances within the same file (e.g., lines 330-332). This inconsistency could lead to runtime errors if the code assumes that FundingIndex is always present. All instances of PerpetualPosition should be reviewed and updated to include the FundingIndex where necessary.
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(100_000_000), // 1 BTC | ||
| FundingIndex: dtypes.NewInt(0), |
There was a problem hiding this comment.
The FundingIndex field is added to PerpetualPosition here, but it is missing in other instances within the same file (e.g., lines 330-332). This inconsistency could lead to runtime errors if the code assumes that FundingIndex is always present. All instances of PerpetualPosition should be reviewed and updated to include the FundingIndex where necessary.
| return GetNetNotionalInQuoteQuantums(perpetual, marketPrice, bigQuantums), nil | ||
| } | ||
|
|
||
| // GetNetNotionalInQuoteQuantums returns the net notional in quote quantums, which can be | ||
| // represented by the following equation: | ||
| // | ||
| // `quantums / 10^baseAtomicResolution * marketPrice * 10^marketExponent * 10^quoteAtomicResolution`. | ||
| // Note that longs are positive, and shorts are negative. | ||
| // | ||
| // Also note that this is a stateless function. | ||
| func GetNetNotionalInQuoteQuantums( | ||
| perpetual types.Perpetual, | ||
| marketPrice pricestypes.MarketPrice, | ||
| bigQuantums *big.Int, | ||
| ) ( | ||
| bigNetNotionalQuoteQuantums *big.Int, | ||
| ) { | ||
| bigQuoteQuantums := lib.BaseToQuoteQuantums( | ||
| bigQuantums, | ||
| perpetual.Params.AtomicResolution, | ||
| marketPrice.Price, | ||
| marketPrice.Exponent, | ||
| ) | ||
|
|
||
| return bigQuoteQuantums, nil | ||
| return bigQuoteQuantums |
There was a problem hiding this comment.
The new utility function GetNetNotionalInQuoteQuantums does not include error handling. Ensure that all input parameters are validated before calling this function to prevent any potential calculation errors.
| return nil, nil, err | ||
| } | ||
|
|
||
| bigInitialMarginQuoteQuantums, | ||
| bigMaintenanceMarginQuoteQuantums = GetMarginRequirementsInQuoteQuantums( | ||
| perpetual, | ||
| marketPrice, | ||
| liquidityTier, | ||
| bigQuantums, | ||
| ) | ||
| return bigInitialMarginQuoteQuantums, bigMaintenanceMarginQuoteQuantums, nil | ||
| } | ||
|
|
||
| // GetMarginRequirementsInQuoteQuantums returns initial and maintenance margin requirements | ||
| // in quote quantums, given the position size in base quantums. | ||
| // | ||
| // Note that this is a stateless function. | ||
| func GetMarginRequirementsInQuoteQuantums( | ||
| perpetual types.Perpetual, | ||
| marketPrice pricestypes.MarketPrice, | ||
| liquidityTier types.LiquidityTier, | ||
| bigQuantums *big.Int, | ||
| ) ( | ||
| bigInitialMarginQuoteQuantums *big.Int, | ||
| bigMaintenanceMarginQuoteQuantums *big.Int, | ||
| ) { | ||
| // Always consider the magnitude of the position regardless of whether it is long/short. | ||
| bigAbsQuantums := new(big.Int).Set(bigQuantums).Abs(bigQuantums) | ||
|
|
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [899-943]
The new utility function GetMarginRequirementsInQuoteQuantums does not include error handling. Ensure that all input parameters are validated before calling this function to prevent any potential calculation errors.
| return big.NewInt(0), big.NewInt(0), err | ||
| } | ||
|
|
||
| bigNetSettlementPpm, newFundingIndex = GetSettlementPpmWithPerpetual( | ||
| perpetual, | ||
| quantums, | ||
| index, | ||
| ) | ||
| return bigNetSettlementPpm, newFundingIndex, nil | ||
| } | ||
|
|
||
| // GetSettlementPpm returns the net settlement amount ppm (in quote quantums) given | ||
| // the perpetual and position size (in base quantums). | ||
| // | ||
| // Note that this function is a stateless utility function. | ||
| func GetSettlementPpmWithPerpetual( | ||
| perpetual types.Perpetual, | ||
| quantums *big.Int, | ||
| index *big.Int, | ||
| ) ( | ||
| bigNetSettlementPpm *big.Int, | ||
| newFundingIndex *big.Int, | ||
| ) { | ||
| indexDelta := new(big.Int).Sub(perpetual.FundingIndex.BigInt(), index) | ||
|
|
||
| // if indexDelta is zero, then net settlement is zero. | ||
| if indexDelta.Sign() == 0 { | ||
| return big.NewInt(0), perpetual.FundingIndex.BigInt(), nil | ||
| return big.NewInt(0), perpetual.FundingIndex.BigInt() | ||
| } | ||
|
|
||
| bigNetSettlementPpm = new(big.Int).Mul(indexDelta, quantums) |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [972-1006]
The new utility function GetSettlementPpmWithPerpetual does not include error handling. Ensure that all input parameters are validated before calling this function to prevent any potential calculation errors.
| ) { | ||
| // Fetch all relevant perpetuals. | ||
| perpetuals := make(map[uint32]perptypes.Perpetual) | ||
| for _, p := range subaccount.PerpetualPositions { | ||
| perpetual, err := k.perpetualsKeeper.GetPerpetual(ctx, p.PerpetualId) | ||
| if err != nil { | ||
| return types.Subaccount{}, nil, err | ||
| } | ||
| perpetuals[p.PerpetualId] = perpetual | ||
| } | ||
|
|
||
| return GetSettledSubaccountWithPerpetuals(subaccount, perpetuals) |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The search for the GetSettledSubaccountWithPerpetuals function within the protocol/x/subaccounts/keeper test files did not yield any results. This suggests that there are currently no unit tests covering this new function. Given the importance of this function in financial calculations and state management, it is recommended to add appropriate tests to ensure its correctness and robustness.
- The
GetSettledSubaccountWithPerpetualsfunction inprotocol/x/subaccounts/keeper/subaccount.golacks unit tests. It is advisable to implement tests that cover various scenarios and edge cases for this function.
Analysis chain
The new function GetSettledSubaccountWithPerpetuals is introduced and used within getSettledSubaccount. It's important to ensure that this new function is thoroughly tested, especially since it involves financial calculations and could impact the state of subaccounts.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new function is covered by tests.
rg 'GetSettledSubaccountWithPerpetuals' -- 'protocol/x/subaccounts/keeper/*_test.go'
Length of output: 164
Script:
#!/bin/bash
# Verify the existence of test files in the subaccounts keeper directory.
fd '_test.go' 'protocol/x/subaccounts/keeper'
Length of output: 261
Script:
#!/bin/bash
# Search for tests related to the GetSettledSubaccountWithPerpetuals function in the subaccounts keeper test files.
rg 'GetSettledSubaccountWithPerpetuals' 'protocol/x/subaccounts/keeper'
Length of output: 414
| ) ( | ||
| settledSubaccount types.Subaccount, | ||
| fundingPayments map[uint32]dtypes.SerializableInt, | ||
| err error, | ||
| ) { | ||
| totalNetSettlementPpm := big.NewInt(0) |
There was a problem hiding this comment.
The TODO comment about removing a function and using UpdateAssetPositions instead should be tracked in the project's issue tracker to ensure it is addressed in the future. This helps maintain the codebase's cleanliness and prevents technical debt from accumulating.
// TODO: Ensure that this TODO is tracked in the project's issue tracker.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/daemons/liquidation/client/grpc_helper.go (5 hunks)
- protocol/daemons/liquidation/client/grpc_helper_test.go (10 hunks)
- protocol/daemons/liquidation/client/sub_task_runner_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/daemons/liquidation/client/grpc_helper_test.go
Additional comments: 7
protocol/daemons/liquidation/client/grpc_helper.go (4)
- 44-49: The shell script results show that there are no calls to the removed
CheckCollateralizationForSubaccountsfunction and no remaining references to theclobtypespackage in thegrpc_helper.gofile. This suggests that the removal of theCheckCollateralizationForSubaccountsfunction and theclobtypesimport has been handled correctly, with no orphaned references left in the file.Based on the information gathered, it appears that the changes made to the
grpc_helper.gofile are consistent with the PR's objectives and do not leave any unresolved references or logic.Analysis chain
The
blockHeightparameter has been removed from theGetAllPerpetualsfunction. Verify that all calls to this function have been updated accordingly and that the logic within the function does not require theblockHeightfor correct operation.
The shell script results indicate that there are no calls to
GetAllPerpetualswith theblockHeightparameter and no remaining logic within theGetAllPerpetualsfunction that referencesblockHeight. This suggests that the removal of theblockHeightparameter has been handled correctly in the codebase, at least within the scope of theGetAllPerpetualsfunction.However, the script did not check for references to the removed
CheckCollateralizationForSubaccountsfunction or any remaining references to theclobtypespackage. To ensure that these changes have not left any orphaned references or logic, I will need to run additional scripts to check for these.* 83-88: The new shell script output shows three calls to the `GetAllLiquidityTiers` function within the codebase. All of these calls are using the correct number of parameters, which is consistent with the updated function signature that no longer includes the `blockHeight` parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `GetAllPerpetuals` that might still be passing the `blockHeight` parameter. ast-grep --lang go --pattern $'$_.$_.GetAllPerpetuals($_, $_, $_)' # Search for any remaining logic in `GetAllPerpetuals` that might reference `blockHeight`. ast-grep --lang go --pattern $'func (c *Client) GetAllPerpetuals($_, $_) { $$$ blockHeight $$$ }'Length of output: 166
Script:
#!/bin/bash # Search for any calls to the removed `CheckCollateralizationForSubaccounts` function. ast-grep --lang go --pattern $'$_.$_.CheckCollateralizationForSubaccounts($$$)' # Search for any remaining references to the `clobtypes` package in the file. rg --type=go 'clobtypes' protocol/daemons/liquidation/client/grpc_helper.goLength of output: 156
Given this information, it can be concluded that all calls to
GetAllLiquidityTiershave been updated accordingly, and there is no indication that theblockHeightparameter is being incorrectly used or that it is required for the function's operation within the current scope of the codebase.The verification is successful, and no issues are found with the changes made to the
GetAllLiquidityTiersfunction.Analysis chain
The
blockHeightparameter has been removed from theGetAllLiquidityTiersfunction. Verify that all calls to this function have been updated accordingly and that the logic within the function does not require theblockHeightfor correct operation.
The shell script execution did not return any output, which suggests that there are no instances where
GetAllLiquidityTiersis called with theblockHeightparameter, and there is no remaining logic within theGetAllLiquidityTiersfunction that referencesblockHeight. This indicates that the removal of theblockHeightparameter has been handled correctly in the codebase, at least within the scope of the search performed.However, to be thorough, I will perform one more check to ensure that there are no calls to
GetAllLiquidityTierswith any number of parameters, which will help confirm that the function is being called correctly throughout the codebase.* 122-127: The shell script execution did not return any results, which suggests that there are no calls to `GetAllMarketPrices` with the `blockHeight` parameter and no remaining logic within the `GetAllMarketPrices` function that references `blockHeight`. This indicates that the removal of the `blockHeight` parameter has been handled correctly in the codebase, at least within the scope of the provided context and the executed scripts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `GetAllLiquidityTiers` that might still be passing the `blockHeight` parameter. ast-grep --lang go --pattern $'$_.$_.GetAllLiquidityTiers($_, $_, $_)' # Search for any remaining logic in `GetAllLiquidityTiers` that might reference `blockHeight`. ast-grep --lang go --pattern $'func (c *Client) GetAllLiquidityTiers($_, $_) { $$$ blockHeight $$$ }'Length of output: 174
Script:
#!/bin/bash # Search for any calls to `GetAllLiquidityTiers` to ensure they are using the correct number of parameters. ast-grep --lang go --pattern $'$_.$_.GetAllLiquidityTiers($$$)'Length of output: 430
Analysis chain
The
blockHeightparameter has been removed from theGetAllMarketPricesfunction. Verify that all calls to this function have been updated accordingly and that the logic within the function does not require theblockHeightfor correct operation.* 12-17: The shell script executed a search for the string `clobtypes` within the file `protocol/daemons/liquidation/client/grpc_helper.go` and did not produce any output. This indicates that there are no remaining references to the `clobtypes` package in the file, confirming that the removal of the import is consistent with the current state of the code.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `GetAllMarketPrices` that might still be passing the `blockHeight` parameter. ast-grep --lang go --pattern $'$_.$_.GetAllMarketPrices($_, $_, $_)' # Search for any remaining logic in `GetAllMarketPrices` that might reference `blockHeight`. ast-grep --lang go --pattern $'func (c *Client) GetAllMarketPrices($_, $_) { $$$ blockHeight $$$ }'Length of output: 170
Analysis chain
The import of the
clobtypespackage has been removed. Verify that there are no remaining references to this package in the file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to `clobtypes` in the file. rg 'clobtypes' protocol/daemons/liquidation/client/grpc_helper.goLength of output: 65
protocol/daemons/liquidation/client/sub_task_runner_test.go (3)
23-526: The test cases within
TestRunLiquidationDaemonTaskLoopare comprehensive and cover a variety of scenarios, including different subaccount positions and the impact of funding payments on collateralization. This thorough testing approach is commendable for ensuring the robustness of the liquidation daemon's task loop logic.23-526: The use of constants and mocks in the test setup provides a clear and consistent testing environment, which is beneficial for readability and maintainability of the test code.
23-526: The use of the
requirepackage for assertions in the test cases is appropriate, as it ensures that the test will halt immediately upon a failed assertion, which is suitable for the test scenarios provided.
| // GetAllPerpetuals queries gRPC server and returns a list of perpetuals. | ||
| func (c *Client) GetAllPerpetuals( | ||
| ctx context.Context, | ||
| blockHeight uint32, | ||
| pageLimit uint64, | ||
| ) ( | ||
| perpetuals []perptypes.Perpetual, |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The results confirm that the metric constant CheckCollateralizationForSubaccounts is used in two places within the codebase:
- It is defined in
protocol/lib/metrics/constants.go. - It is used in
protocol/daemons/liquidation/client/sub_task_runner.gowithin adeferstatement for telemetry purposes.
The context provided around the usage in sub_task_runner.go indicates that the metric was used to measure the latency of the CheckCollateralizationForSubaccounts function. Since this function has been removed, the corresponding metric should also be removed or updated to reflect the new logic that replaces the removed function.
Given this information, the removal of the CheckCollateralizationForSubaccounts function does indeed require attention to the associated metric constant and its usage. The codebase should be updated to ensure that there are no orphaned references or incorrect telemetry measurements.
The review should therefore include the following points:
-
The metric constant
CheckCollateralizationForSubaccountsinprotocol/lib/metrics/constants.goshould be removed or repurposed if it is no longer relevant. -
The telemetry measurement in
protocol/daemons/liquidation/client/sub_task_runner.goshould be updated to remove the reference to the removed function or to measure the latency of the new logic that has replaced it. -
Remove or update the metric constant
CheckCollateralizationForSubaccountsinprotocol/lib/metrics/constants.go. -
Address the reference to
CheckCollateralizationForSubaccountsinprotocol/daemons/liquidation/client/sub_task_runner.goto ensure correct telemetry measurement.
Analysis chain
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [44-201]
The CheckCollateralizationForSubaccounts function has been removed. Verify that there are no orphaned references to this function in the codebase and that its removal does not affect any dependent logic.
The shell script results indicate that there are references to the CheckCollateralizationForSubaccounts function in two files: protocol/lib/metrics/constants.go and protocol/daemons/liquidation/client/sub_task_runner.go. To verify that the removal of this function does not affect any dependent logic, we need to check these files to understand the context in which the function is referenced.
Let's start by examining the references in protocol/lib/metrics/constants.go to see if it's just a metric constant that needs to be removed or updated. Then we'll check protocol/daemons/liquidation/client/sub_task_runner.go to see how the function was being used and ensure that its removal has been properly handled in the logic of the sub task runner.
The results from the shell scripts show that the CheckCollateralizationForSubaccounts function is referenced in two places:
-
In
protocol/lib/metrics/constants.go, it appears to be a string constant namedCheckCollateralizationForSubaccounts. This is likely used as a key or label for metrics and may need to be removed or updated if it is no longer relevant due to the removal of the function. -
In
protocol/daemons/liquidation/client/sub_task_runner.go, the constantmetrics.CheckCollateralizationForSubaccountsis used, which suggests that the function's removal should also prompt an update to the metrics collection logic in this file.
To ensure that the removal of the CheckCollateralizationForSubaccounts function is properly handled, we need to verify that:
- The metric constant in
protocol/lib/metrics/constants.gois either removed or no longer used. - The reference in
protocol/daemons/liquidation/client/sub_task_runner.gois addressed, meaning that the code has been updated to remove the metric logging or it has been replaced with an alternative logic.
Let's proceed with checking the usage of the metric constant in protocol/lib/metrics/constants.go and how the reference in protocol/daemons/liquidation/client/sub_task_runner.go is handled after the removal of the CheckCollateralizationForSubaccounts function.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to `CheckCollateralizationForSubaccounts` in the codebase.
rg 'CheckCollateralizationForSubaccounts' --files-with-matches
Length of output: 151
Script:
#!/bin/bash
# Check the reference in protocol/lib/metrics/constants.go
rg 'CheckCollateralizationForSubaccounts' protocol/lib/metrics/constants.go
# Check the reference in protocol/daemons/liquidation/client/sub_task_runner.go
rg 'CheckCollateralizationForSubaccounts' protocol/daemons/liquidation/client/sub_task_runner.go
Length of output: 302
Script:
#!/bin/bash
# Check if the metric constant is used elsewhere in the codebase
rg 'CheckCollateralizationForSubaccounts' --files-with-matches
# Examine the usage of the metric constant in sub_task_runner.go
rg 'CheckCollateralizationForSubaccounts' protocol/daemons/liquidation/client/sub_task_runner.go -A 5 -B 5
Length of output: 584
* [CLOB-1043] deprecate unused grpc query * fix lint
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/x/clob/types/query.pb.go
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (3 hunks)
- proto/dydxprotocol/clob/query.proto (3 hunks)
- protocol/mocks/QueryClient.go (1 hunks)
Additional comments: 12
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.rpc.Query.ts (4)
7-7: The
areSubaccountsLiquidatablemethod is not present in theQueryinterface as per the provided hunk, which aligns with the AI-generated summary stating its removal.30-35: The
areSubaccountsLiquidatablemethod is not present in the method bindings of theQueryClientImplclass, which confirms the removal as stated in the AI-generated summary.87-92: The
areSubaccountsLiquidatablemethod is not present in the extension functioncreateRpcQueryExtension, confirming its removal as per the AI-generated summary.4-4: The import statement in line 4 does not include
AreSubaccountsLiquidatableRequestandAreSubaccountsLiquidatableResponse, which aligns with the AI-generated summary stating their removal.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (3)
1-4: The removal of imports for
SubaccountIdandSubaccountIdSDKTypesuggests that these types are no longer used in this file. Verify that these types are not used elsewhere in the codebase where they might still be needed.1-4: If the logic for
IsLiquidatablehas been implemented elsewhere as part of this PR, ensure that it is properly integrated and tested. If it is part of a different PR, verify that the removal of the old logic does not affect the system's functionality before the new logic is merged.1-4: Ensure that the system's requirements for liquidation logic are still met after the removal of the
AreSubaccountsLiquidatablelogic. This may involve checking for new implementations or confirming that the logic is no longer necessary due to design changes.proto/dydxprotocol/clob/query.proto (4)
9-13: The removal of the import statement for "dydxprotocol/subaccounts/subaccount.proto" is not visible in the provided hunks. If this import is no longer needed due to the removal of dependencies on
subaccount.protowithin this file, then the change is justified.24-29: The removal of the RPC definition for "AreSubaccountsLiquidatable" is not visible in the provided hunks. Assuming that the logic for determining if an account is liquidatable has been replicated elsewhere as per the PR objectives, and this RPC is no longer required, the removal is justified.
28-29: The RPC definition for "MevNodeToNodeCalculation" appears to be correctly formatted without a semicolon after the returns clause, which aligns with the summary's mention of this adjustment.
9-14: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [9-29]
Please verify that the removal of the import statement and the RPC definition does not affect other services or integrations that may depend on them. This may require coordination with other teams to ensure that they are aware of these changes and have adjusted their code accordingly.
protocol/mocks/QueryClient.go (1)
- 213-215: The
BlockRateLimitConfigurationfunction remains unchanged, which is consistent with the PR's objectives. Ensure that the logic within this function and other functions in theQueryClientstruct are still correct and that their interactions with the removedAreSubaccountsLiquidatablefunction have been properly addressed.
| return r0, r1 | ||
| } | ||
|
|
||
| // AreSubaccountsLiquidatable provides a mock function with given fields: ctx, in, opts | ||
| func (_m *QueryClient) AreSubaccountsLiquidatable(ctx context.Context, in *clobtypes.AreSubaccountsLiquidatableRequest, opts ...grpc.CallOption) (*clobtypes.AreSubaccountsLiquidatableResponse, error) { | ||
| _va := make([]interface{}, len(opts)) | ||
| for _i := range opts { | ||
| _va[_i] = opts[_i] | ||
| } | ||
| var _ca []interface{} | ||
| _ca = append(_ca, ctx, in) | ||
| _ca = append(_ca, _va...) | ||
| ret := _m.Called(_ca...) | ||
|
|
||
| var r0 *clobtypes.AreSubaccountsLiquidatableResponse | ||
| if rf, ok := ret.Get(0).(func(context.Context, *clobtypes.AreSubaccountsLiquidatableRequest, ...grpc.CallOption) *clobtypes.AreSubaccountsLiquidatableResponse); ok { | ||
| r0 = rf(ctx, in, opts...) | ||
| } else { | ||
| if ret.Get(0) != nil { | ||
| r0 = ret.Get(0).(*clobtypes.AreSubaccountsLiquidatableResponse) | ||
| } | ||
| } | ||
|
|
||
| var r1 error | ||
| if rf, ok := ret.Get(1).(func(context.Context, *clobtypes.AreSubaccountsLiquidatableRequest, ...grpc.CallOption) error); ok { | ||
| r1 = rf(ctx, in, opts...) | ||
| } else { | ||
| r1 = ret.Error(1) | ||
| } | ||
|
|
||
| return r0, r1 | ||
| } | ||
|
|
||
| // BlockRateLimitConfiguration provides a mock function with given fields: ctx, in, opts | ||
| func (_m *QueryClient) BlockRateLimitConfiguration(ctx context.Context, in *clobtypes.QueryBlockRateLimitConfigurationRequest, opts ...grpc.CallOption) (*clobtypes.QueryBlockRateLimitConfigurationResponse, error) { | ||
| _va := make([]interface{}, len(opts)) |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The search results confirm that the AreSubaccountsLiquidatable function is referenced in the swagger.yaml file, which is part of the API documentation. The references to AreSubaccountsLiquidatableResponse and AreSubaccountsLiquidatableResponse.Result suggest that the documentation still includes information about the removed function's response structure.
Given this information, it is necessary to update the API documentation to reflect the removal of the AreSubaccountsLiquidatable function from the codebase. This ensures that the documentation remains consistent with the current state of the API.
The review should therefore include a recommendation to update the swagger.yaml file to remove or revise the sections that reference the removed function.
The reference to `AreSubaccountsLiquidatable` in the `swagger.yaml` file indicates that the API documentation still includes information about the removed function. Please update the documentation to reflect the changes in the codebase and ensure consistency.
- Update or remove the `AreSubaccountsLiquidatableResponse` and related documentation in the `swagger.yaml` file.
Analysis chain
The removal of the AreSubaccountsLiquidatable function from the QueryClient struct aligns with the PR's objective to replicate the IsLiquidatable logic elsewhere. Ensure that all references to this function are updated or removed in the codebase to prevent any broken dependencies.
The search results indicate that there is a reference to the AreSubaccountsLiquidatable function in the swagger.yaml file within the protocol/client/docs/swagger-ui directory. This could be part of the API documentation that needs to be updated due to the removal of the function.
To proceed, I will verify if the reference in the swagger.yaml file is indeed related to the removed function and if it requires an update.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the removed function to ensure they are updated or removed.
rg 'AreSubaccountsLiquidatable' --files-with-matches
Length of output: 97
Script:
#!/bin/bash
# Check the contents of the swagger.yaml file for the reference to AreSubaccountsLiquidatable to understand the context.
rg 'AreSubaccountsLiquidatable' protocol/client/docs/swagger-ui/swagger.yaml
Length of output: 276
| import { PageRequest, PageRequestSDKType, PageResponse, PageResponseSDKType } from "../../cosmos/base/query/v1beta1/pagination"; | ||
| import { SubaccountId, SubaccountIdSDKType } from "../subaccounts/subaccount"; | ||
| import { ValidatorMevMatches, ValidatorMevMatchesSDKType, MevNodeToNodeMetrics, MevNodeToNodeMetricsSDKType } from "./mev"; | ||
| import { ClobPair, ClobPairSDKType } from "./clob_pair"; | ||
| import { EquityTierLimitConfiguration, EquityTierLimitConfigurationSDKType } from "./equity_tier_limit_config"; |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The provided context from the swagger.yaml file confirms that there are still references to AreSubaccountsLiquidatableResponse and its Result type in the Swagger documentation. These references are part of the API documentation and should be updated or removed to reflect the changes made in the codebase where the corresponding interfaces and functions were removed.
Given this information, the review comment is valid, and the references in the Swagger documentation should be addressed to ensure consistency with the updated codebase.
The removal of interfaces and functions related to AreSubaccountsLiquidatable has not been fully propagated throughout the codebase. References to these entities still exist in the Swagger documentation (swagger.yaml). These should be updated or removed to reflect the changes in the codebase.
protocol/client/docs/swagger-ui/swagger.yaml: Contains outdated references toAreSubaccountsLiquidatableResponseandAreSubaccountsLiquidatableResponse.Result.
Analysis chain
Confirm that the removal of interfaces and functions related to AreSubaccountsLiquidatable has been fully propagated throughout the codebase, ensuring no dangling references or calls to these removed entities remain.
The search results indicate that there are still references to AreSubaccountsLiquidatableResponse and its Result in the swagger.yaml file within the protocol/client/docs/swagger-ui directory. These references should be checked and potentially removed or updated to reflect the changes made in the PR.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed interfaces and functions in the codebase.
rg 'AreSubaccountsLiquidatableRequest|AreSubaccountsLiquidatableResponse|AreSubaccountsLiquidatableResponse_Result|createBaseAreSubaccountsLiquidatableRequest'
Length of output: 496
Script:
#!/bin/bash
# Check the swagger.yaml file for the context of the remaining references to ensure they should be removed or updated.
rg -C 5 'AreSubaccountsLiquidatableResponse|AreSubaccountsLiquidatableResponse\.Result' protocol/client/docs/swagger-ui/swagger.yaml
Length of output: 1016
| function createBaseMevNodeToNodeCalculationRequest(): MevNodeToNodeCalculationRequest { | ||
| return { | ||
| blockProposerMatches: undefined, |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The search results indicate that the function createBaseMevNodeToNodeCalculationRequest is indeed referenced in the same file query.ts where it was defined. This suggests that the function was being used to create base requests for MevNodeToNodeCalculationRequest. However, since the function has been removed, these references will now lead to errors.
Given this information, it is clear that the removal of createBaseMevNodeToNodeCalculationRequest impacts the existing functionality, as there are still calls to this function in the codebase.
Here is the concise response:
The function createBaseMevNodeToNodeCalculationRequest has been removed, but there are still references to it in query.ts. This will cause errors since the function no longer exists. The removal of this function impacts existing functionality.
- Affected location:
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts
A fix would involve either restoring the function or updating the code to remove or replace these references.
Analysis chain
The function createBaseMevNodeToNodeCalculationRequest has been removed. Verify that this function is not used elsewhere in the codebase and that its removal does not impact any existing functionality.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to createBaseMevNodeToNodeCalculationRequest in the codebase.
rg 'createBaseMevNodeToNodeCalculationRequest'
Length of output: 462
* [CLOB-1046] populate negative tnc subaccounts in grpc request * [CLOB-1047] populate subaccounts with open positions in grpc request (#892) * [CLOB-1047] populate subaccounts with open positions in grpc request * comments
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (10)
- protocol/daemons/liquidation/client/grpc_helper.go (7 hunks)
- protocol/daemons/liquidation/client/grpc_helper_test.go (9 hunks)
- protocol/daemons/liquidation/client/sub_task_runner.go (3 hunks)
- protocol/daemons/liquidation/client/sub_task_runner_test.go (1 hunks)
- protocol/daemons/server/liquidation.go (1 hunks)
- protocol/daemons/server/liquidation_test.go (3 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (3 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/testutil/constants/subaccounts.go (5 hunks)
Files skipped from review due to trivial changes (1)
- protocol/daemons/server/liquidation_test.go
Additional comments: 31
protocol/daemons/liquidation/client/grpc_helper.go (7)
13-13: The addition of the
libimport statement is noted and appears to be used later in the file. Ensure that this new dependency is properly managed and that the package is used as intended.46-51: The
blockHeightparameter has been removed from theGetAllPerpetualsfunction signature. This change aligns with the PR's objective to refactor the liquidation daemon's logic. Ensure that all calls to this function have been updated accordingly across the codebase.85-90: The
blockHeightparameter has been removed from theGetAllLiquidityTiersfunction signature. As with the previous function, ensure that all calls to this function have been updated to reflect the new signature.124-129: The
blockHeightparameter has been removed from theGetAllMarketPricesfunction signature. Verify that all calls to this function have been updated to match the new signature without theblockHeightparameter.Verification successful
The
blockHeightparameter has been successfully removed from theGetAllMarketPricesfunction signature, and the calls to this function have been updated to match the new signature without theblockHeightparameter, as confirmed by the search results in thesub_task_runner.gofile.* 206-213: The `SendLiquidatableSubaccountIds` function has been updated to include new parameters: `blockHeight`, `liquidatableSubaccountIds`, `negativeTncSubaccountIds`, and `openPositionInfoMap`. This change likely reflects the new logic for handling liquidation checks. Ensure that the new parameters are being used correctly and that all calls to this function have been updated to include the new arguments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to GetAllMarketPrices with the old signature including blockHeight. ast-grep --lang go --pattern $'GetAllMarketPrices($_, $_, $_)'Length of output: 63
Script:
#!/bin/bash # Search for calls to GetAllMarketPrices with the new signature excluding blockHeight. ast-grep --lang go --pattern $'GetAllMarketPrices($_, $_)'Length of output: 59
Script:
#!/bin/bash # Search for any instance of "GetAllMarketPrices" to ensure we're not missing calls due to pattern matching issues. rg 'GetAllMarketPrices' --goLength of output: 457
Script:
#!/bin/bash # Correct the script to use the proper flag for searching within Go files. rg 'GetAllMarketPrices' --type goLength of output: 2011
- 257-259: A new function
newContextWithQueryBlockHeighthas been added. This function likely serves to attach the block height to the context for subsequent gRPC calls. Ensure that this function is being used wherever the block height needs to be passed to the gRPC context.Verification successful
The shell script output indicates that the
newContextWithQueryBlockHeightfunction is used in thesub_task_runner.gofile at line 127. This confirms that the new function is being utilized within the codebase, and since we only have evidence of its usage in one place, we can infer that it's being used correctly in that instance.However, without further context, we cannot definitively say whether it should be used in more places. The verification process does not allow us to assume that it should be used elsewhere without evidence. Therefore, the verification is successful based on the information available.
Final response:
The
newContextWithQueryBlockHeightfunction is being used insub_task_runner.go. There is no evidence of misuse or additional required usage within the provided context. The function's implementation and usage have been verified as correct based on the available information.* 210-213: The `SendLiquidatableSubaccountIds` function now includes additional parameters for `blockHeight`, `liquidatableSubaccountIds`, `negativeTncSubaccountIds`, and `openPositionInfoMap`. This change suggests a more complex logic for determining liquidatable subaccounts. Verify that the new parameters are being used correctly and that their introduction does not introduce any potential data races, especially considering the use of a map (`openPositionInfoMap`).Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of newContextWithQueryBlockHeight to ensure it's being used correctly. ast-grep --lang go --pattern $'newContextWithQueryBlockHeight($_, $_)'Length of output: 194
protocol/daemons/liquidation/client/grpc_helper_test.go (6)
93-99: The update to use
df.Liquidation.QueryPageLimitinstead of a hardcoded value is consistent with the PR's objective to consolidate flags. This change should ensure that the tests are using the same configuration as the daemon itself.113-119: The same flag consolidation is applied here for paginated requests, which maintains consistency across the test cases.
129-135: Again, the use of
df.Liquidation.QueryPageLimitfor paginated requests in subsequent calls is consistent with the PR's objective.148-154: The error propagation test case correctly uses the new flag, ensuring that the test remains valid even after the consolidation of flags.
166-172: The test case for
GetAllSubaccountscorrectly passes the new consolidated flag as a parameter, aligning with the changes in the daemon's configuration.453-558: The test case
TestSendLiquidatableSubaccountIdsseems to be correctly setting up mocks and calling theSendLiquidatableSubaccountIdsmethod with the expected parameters. However, it is important to ensure that theBlockHeightand other parameters passed to the method reflect the actual state and requirements of the daemon after the changes made in the PR. If the daemon's logic has changed significantly, these test cases may need to be updated to match the new logic.protocol/daemons/liquidation/client/sub_task_runner.go (8)
5-21: The addition of new imports suggests that the file now relies on additional modules to implement the updated logic. Ensure that these imports are used within the file and that there are no unused imports, which could lead to unnecessary dependencies.
52-69: The new method
FetchApplicationStateAtBlockHeightis introduced to fetch application state at a given block height. Ensure that the method is used consistently and that the error handling is robust, as it seems to be a critical part of the liquidation process.71-82: The method
GetLiquidatableSubaccountIdshas been updated with new parameters and logic. It is crucial to ensure that the method's logic correctly identifies liquidatable subaccounts and that the error handling is comprehensive.84-94: The method
GetSubaccountOpenPositionInfois added to build a map of perpetual id to subaccounts with open positions. Verify that the data structure used for the map is appropriate and that the method's logic is efficient and accurate.184-212: The logic within
GetLiquidatableSubaccountIdsnow includes a check for subaccounts with no open positions, which is an optimization to skip unnecessary processing. Ensure that this early return does not miss any edge cases where a subaccount might still be liquidatable despite having no open positions.215-272: The method
GetSubaccountOpenPositionInfocreates a map of perpetual id to open position info. Ensure that the logic correctly handles the case where multiple subaccounts may have open positions in the same perpetual, and that the data structure is updated correctly.274-371: The method
CheckSubaccountCollateralizationperforms collateralization checks using the provided market prices, perpetuals, and liquidity tiers. It is important to ensure that the logic for calculating net collateral and maintenance margin is correct and that the method accurately determines if a subaccount is liquidatable.187-190: The existing comment chain indicates a discussion about an early return in the case of subaccounts with no open positions. The response from
jayy04explains the reasoning behind the addition. It is important to ensure that this logic is still valid in the context of the updated code and that the early return does not introduce any unintended side effects.protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (2)
- 83-83: The method signature for
UpdateSubaccountsWithPositionshas been changed to accept a slice instead of a map. Verify that all calls to this function have been updated accordingly to pass a slice and that the logic that depends on this method has been adjusted to handle the new data structure.Verification successful
The initial script was intended to find calls to the
UpdateSubaccountsWithPositionsmethod to ensure that they have been updated to pass a slice instead of a map. However, the script did not produce any output, which means that either there are no calls to this method in the codebase, or the pattern used in the script did not match the way the method is called.To ensure that we have covered all possibilities, I will write a new script that searches for the method name in a more general way without specifying the structure of the call. This should help us find any instance of the method being called, regardless of how the arguments are passed.
The new script has successfully identified several calls to the
UpdateSubaccountsWithPositionsmethod within the codebase. The output shows that this method is called inliquidation.goand multiple times indaemon_liquidation_info_test.go.The call in
liquidation.goseems to passreq.SubaccountOpenPositionInfo, which suggests that the expected slice is being passed to the method after the change in its signature. The calls indaemon_liquidation_info_test.goare in test files, which also appear to have been updated accordingly, as they are simply passing a variableinput,input2, orinput3to the method.Given this information, it seems that the necessary updates to the method calls have been made to accommodate the change from a map to a slice in the method signature. However, without seeing the actual contents of the variables being passed to the method, we cannot be 100% certain that they are slices. To verify this, we would need to inspect the definitions of
req.SubaccountOpenPositionInfo,input,input2, andinput3.To confirm that the variables being passed are indeed slices, I will write a script to check the definitions of these variables.
The output from the script provides the definitions of the variables being passed to the
UpdateSubaccountsWithPositionsmethod. It shows thatinput,input2, andinput3are indeed slices ofclobtypes.SubaccountOpenPositionInfo, which is consistent with the updated method signature that now expects a slice instead of a map.For example, we can see the following pattern in the test file
daemon_liquidation_info_test.go:info := clobtypes.SubaccountOpenPositionInfo{...} input := []clobtypes.SubaccountOpenPositionInfo{info} ls.UpdateSubaccountsWithPositions(input)This pattern confirms that the test cases have been updated to create a slice of
SubaccountOpenPositionInfoand pass it to theUpdateSubaccountsWithPositionsmethod.Additionally, in
liquidation.go, the methodUpdateSubaccountsWithPositionsis called withreq.SubaccountOpenPositionInfo. Without the full context, we cannot be certain of the type ofreq.SubaccountOpenPositionInfo, but the naming convention suggests that it is likely to be a slice ofSubaccountOpenPositionInfoas required by the updated method signature.Based on the information provided, it appears that the calls to
UpdateSubaccountsWithPositionshave been correctly updated to pass a slice, and the logic that depends on this method has been adjusted to handle the new data structure. Therefore, the changes to the method signature and its usage in the codebase are consistent with the PR's objective.* 87-96: The logic for updating `subaccountsWithPositions` now correctly clones the `SubaccountOpenPositionInfo` for each entry in the slice. Ensure that the rest of the codebase that interacts with `DaemonLiquidationInfo.subaccountsWithPositions` is updated to work with the new map structure that is keyed by `PerpetualId`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to UpdateSubaccountsWithPositions and ensure they pass a slice. ast-grep --lang go --pattern $'$_.$("UpdateSubaccountsWithPositions")($$$)'Length of output: 76
Script:
#!/bin/bash # Search for the string "UpdateSubaccountsWithPositions" to find any usage in the codebase. rg "UpdateSubaccountsWithPositions"Length of output: 1186
Script:
#!/bin/bash # Check the definitions of the variables being passed to UpdateSubaccountsWithPositions. rg -A 3 "SubaccountOpenPositionInfo|input|input2|input3" protocol/daemons/server/liquidation.go protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.goLength of output: 6741
Verification successful
The search results indicate that all references to
subaccountsWithPositionsare contained within thedaemon_liquidation_info.gofile. The changes made to theUpdateSubaccountsWithPositionsmethod and the waysubaccountsWithPositionsis used within the file are consistent with each other. There are no indications of external dependencies that would be affected by these changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to subaccountsWithPositions to ensure they expect a map keyed by PerpetualId. ast-grep --lang go --pattern $'$_.$("subaccountsWithPositions")[$$$]'Length of output: 70
Script:
#!/bin/bash # Search for the string "subaccountsWithPositions" throughout the codebase to find any references. rg 'subaccountsWithPositions'Length of output: 999
protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (3)
52-70: The test
TestSubaccountsWithOpenPositions_Multiple_Readshas been updated to useinfoandinputinstead ofexpected. Ensure that the test still validates the correct behavior after the changes, and the naming of variables is clear and consistent with the rest of the codebase.123-171: In
TestSubaccountsWithOpenPositions_Multiple_Writes, the logic has been updated to useinfo,info2,info3, and their correspondinginputvalues. It's important to ensure that eachinfovariable represents a unique state and that the test cases are independent of each other to avoid shared state issues. Also, verify that the test assertions are correctly checking the intended behavior.209-227: The test
TestSubaccountsWithOpenPosition_Empty_Updatehas been updated to useinfoandinputinstead ofexpected. Ensure that the test correctly verifies the behavior when an empty update is performed and that the assertions are accurately reflecting the expected outcomes.protocol/lib/metrics/constants.go (1)
- 293-306: The addition of new constants
FetchApplicationStateAtBlockHeight,GetSubaccountOpenPositionInfo, andNegativeTncSubaccountIdsin theconstants.gofile is consistent with the PR's objective to enhance the liquidation daemon's functionality. Ensure that these constants are documented and used consistently across the codebase.Verification successful
The previous command used to verify the usage of the new constants was incorrect as it specified the file type as Python. I will correct this and run a new search within Go files to check for the usage of the new constants and their documentation.
The search results confirm that the new constants
FetchApplicationStateAtBlockHeight,GetSubaccountOpenPositionInfo, andNegativeTncSubaccountIdsare being used throughout the codebase, particularly within the liquidation daemon and associated test files. This aligns with the PR's objectives to enhance the liquidation daemon's functionality. While the search results do not provide direct evidence of documentation for these constants, their usage in the codebase is a positive indication of proper integration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new constants are used in the codebase and check for documentation. rg --type python 'FetchApplicationStateAtBlockHeight|GetSubaccountOpenPositionInfo|NegativeTncSubaccountIds'Length of output: 141
Script:
#!/bin/bash # Verify that the new constants are used in the codebase and check for documentation. rg 'FetchApplicationStateAtBlockHeight|GetSubaccountOpenPositionInfo|NegativeTncSubaccountIds'Length of output: 8602
protocol/testutil/constants/subaccounts.go (4)
- 175-177: The addition of the
FundingIndexfield here is consistent with the previous hunk, but as noted earlier, it is important to ensure that this field is added to allPerpetualPositioninstances throughout the file to maintain consistency.
- 333-345: This hunk shows the addition of the
FundingIndexfield to aPerpetualPositioninstance. However, as previously mentioned, it is crucial to ensure that this field is consistently present in all instances ofPerpetualPositionto avoid potential errors.
- 359-361: Again, the
FundingIndexfield is added here, but the same consistency check applies. All instances ofPerpetualPositionin the file should be verified to include theFundingIndexfield.
- 406-408: The
FundingIndexfield is added to thePerpetualPositionstruct in this instance. It is important to ensure that this field is consistently added to all instances ofPerpetualPositionwithin the file to avoid any inconsistencies.
| s.daemonLiquidationInfo.UpdateBlockHeight(req.BlockHeight) | ||
| s.daemonLiquidationInfo.UpdateLiquidatableSubaccountIds(req.LiquidatableSubaccountIds) | ||
| s.daemonLiquidationInfo.UpdateNegativeTncSubaccountIds(req.NegativeTncSubaccountIds) | ||
| s.daemonLiquidationInfo.UpdateSubaccountsWithPositions(req.SubaccountOpenPositionInfo) |
There was a problem hiding this comment.
The new method calls to daemonLiquidationInfo do not include error handling. If any of these methods can fail, the errors should be caught and handled appropriately to prevent the server from entering an inconsistent state or failing silently.
| daemon.PerpetualsQueryClient = queryClientMock | ||
| actual, err := daemon.GetAllPerpetuals( | ||
| grpc.Ctx, | ||
| uint32(50), | ||
| tc.limit, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
The test case for GetAllPerpetuals does not seem to reflect the changes related to flag consolidation. It uses a hardcoded limit value instead of the new QueryPageLimit flag. This should be updated to use the new flag for consistency and to ensure that the test reflects the current state of the daemon's configuration.
- limit: 1_000,
+ limit: df.Liquidation.QueryPageLimit,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.
| daemon.PerpetualsQueryClient = queryClientMock | |
| actual, err := daemon.GetAllPerpetuals( | |
| grpc.Ctx, | |
| uint32(50), | |
| tc.limit, | |
| ) | |
| if err != nil { | |
| daemon.PerpetualsQueryClient = queryClientMock | |
| actual, err := daemon.GetAllPerpetuals( | |
| grpc.Ctx, | |
| df.Liquidation.QueryPageLimit, | |
| ) | |
| if err != nil { |
| }, | ||
| }, | ||
| }, | ||
| } | ||
| response3 := &api.LiquidateSubaccountsResponse{} | ||
| mck.On("LiquidateSubaccounts", ctx, req).Return(response3, nil) | ||
| }, | ||
| }, | ||
| "Can get negative tnc subaccount with long position": { | ||
| setupMocks: func(ctx context.Context, mck *mocks.QueryClient) { | ||
| // Block height. | ||
| res := &blocktimetypes.QueryPreviousBlockInfoResponse{ | ||
| Info: &blocktimetypes.BlockInfo{ | ||
| Height: uint32(50), | ||
| Timestamp: constants.TimeTen, | ||
| }, | ||
| } | ||
| mck.On("PreviousBlockInfo", mock.Anything, mock.Anything).Return(res, nil) | ||
|
|
||
| // Subaccount. | ||
| res2 := &satypes.QuerySubaccountAllResponse{ | ||
| Subaccount: []satypes.Subaccount{ | ||
| // Dave has TNC of -$1. | ||
| constants.Dave_Num0_1BTC_Long_50001USD_Short, | ||
| }, | ||
| } | ||
| mck.On("SubaccountAll", mock.Anything, mock.Anything).Return(res2, nil) | ||
|
|
||
| // Market prices. | ||
| res3 := &pricestypes.QueryAllMarketPricesResponse{ | ||
| MarketPrices: constants.TestMarketPrices, | ||
| } | ||
| mck.On("AllMarketPrices", mock.Anything, mock.Anything).Return(res3, nil) | ||
|
|
||
| // Perpetuals. | ||
| res4 := &perptypes.QueryAllPerpetualsResponse{ | ||
| Perpetual: []perptypes.Perpetual{ | ||
| constants.BtcUsd_20PercentInitial_10PercentMaintenance, | ||
| }, | ||
| } | ||
| mck.On("AllPerpetuals", mock.Anything, mock.Anything).Return(res4, nil) | ||
|
|
||
| // Liquidity tiers. | ||
| res5 := &perptypes.QueryAllLiquidityTiersResponse{ | ||
| LiquidityTiers: constants.LiquidityTiers, | ||
| } | ||
| mck.On("AllLiquidityTiers", mock.Anything, mock.Anything).Return(res5, nil) | ||
|
|
||
| // Sends liquidatable subaccount ids to the server. | ||
| req := &api.LiquidateSubaccountsRequest{ | ||
| BlockHeight: uint32(50), | ||
| LiquidatableSubaccountIds: []satypes.SubaccountId{ | ||
| constants.Dave_Num0, | ||
| }, | ||
| NegativeTncSubaccountIds: []satypes.SubaccountId{ | ||
| constants.Dave_Num0, | ||
| }, | ||
| SubaccountOpenPositionInfo: []clobtypes.SubaccountOpenPositionInfo{ | ||
| { | ||
| PerpetualId: 0, | ||
| SubaccountsWithLongPosition: []satypes.SubaccountId{ | ||
| constants.Dave_Num0, | ||
| }, | ||
| SubaccountsWithShortPosition: []satypes.SubaccountId{}, | ||
| }, | ||
| }, | ||
| } | ||
| response3 := &api.LiquidateSubaccountsResponse{} | ||
| mck.On("LiquidateSubaccounts", ctx, req).Return(response3, nil) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| queryClientMock := &mocks.QueryClient{} | ||
| tc.setupMocks(grpc.Ctx, queryClientMock) | ||
| s := client.SubTaskRunnerImpl{} | ||
|
|
||
| c := client.NewClient(log.NewNopLogger()) | ||
| c.SubaccountQueryClient = queryClientMock | ||
| c.ClobQueryClient = queryClientMock | ||
| c.LiquidationServiceClient = queryClientMock | ||
| c.PerpetualsQueryClient = queryClientMock | ||
| c.PricesQueryClient = queryClientMock | ||
| c.BlocktimeQueryClient = queryClientMock | ||
|
|
||
| err := s.RunLiquidationDaemonTaskLoop( | ||
| grpc.Ctx, | ||
| c, | ||
| flags.GetDefaultDaemonFlags().Liquidation, | ||
| ) | ||
| if tc.expectedError != nil { | ||
| require.EqualError(t, err, tc.expectedError.Error()) | ||
| } else { | ||
| require.NoError(t, err) | ||
| queryClientMock.AssertExpectations(t) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test cases defined in TestRunLiquidationDaemonTaskLoop are missing assertions to verify the expected outcomes of the RunLiquidationDaemonTaskLoop method. Each test case should assert that the correct subaccounts are identified as liquidatable or well-collateralized based on the mock responses provided. This is crucial to ensure that the IsLiquidatable logic is being replicated correctly within the daemon.
For example, after calling RunLiquidationDaemonTaskLoop, there should be assertions to check if the LiquidateSubaccounts method was called with the expected arguments, reflecting the correct subaccounts to be liquidated. This would typically involve checking the LiquidatableSubaccountIds and NegativeTncSubaccountIds fields of the request object passed to the mock.
Here is an example of how an assertion could be added for one of the test cases:
// After calling RunLiquidationDaemonTaskLoop
queryClientMock.AssertCalled(t, "LiquidateSubaccounts", grpc.Ctx, mock.MatchedBy(func(req *api.LiquidateSubaccountsRequest) bool {
// Replace with the correct logic to match the expected request
return len(req.LiquidatableSubaccountIds) == 1 && req.LiquidatableSubaccountIds[0] == constants.Carl_Num0
}))Please add the necessary assertions to each test case to ensure that the expected behavior is being tested.
| daemon.PerpetualsQueryClient = queryClientMock | ||
| actual, err := daemon.GetAllLiquidityTiers( | ||
| grpc.Ctx, | ||
| uint32(50), | ||
| tc.limit, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
The test case for GetAllLiquidityTiers has the same issue as the GetAllPerpetuals test case. It should use the new QueryPageLimit flag instead of a hardcoded limit.
- limit: 5,
+ limit: df.Liquidation.QueryPageLimit,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.
| daemon.PerpetualsQueryClient = queryClientMock | |
| actual, err := daemon.GetAllLiquidityTiers( | |
| grpc.Ctx, | |
| uint32(50), | |
| tc.limit, | |
| ) | |
| if err != nil { | |
| daemon.PerpetualsQueryClient = queryClientMock | |
| actual, err := daemon.GetAllLiquidityTiers( | |
| grpc.Ctx, | |
| df.Liquidation.QueryPageLimit, | |
| ) | |
| if err != nil { |
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(-100_000_000), // -1 BTC | ||
| FundingIndex: dtypes.NewInt(0), |
There was a problem hiding this comment.
The FundingIndex field is added to the PerpetualPosition struct in this instance, but it is not present in all instances of PerpetualPosition within the file. This inconsistency could lead to runtime errors if the code assumes that FundingIndex is always present. All instances of PerpetualPosition should be reviewed and updated to include the FundingIndex where necessary.
| daemon.PricesQueryClient = queryClientMock | ||
| actual, err := daemon.GetAllMarketPrices( | ||
| grpc.Ctx, | ||
| uint32(50), | ||
| tc.limit, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
The test case for GetAllMarketPrices also does not reflect the flag consolidation changes. It should be updated to use the QueryPageLimit flag.
- limit: 2,
+ limit: df.Liquidation.QueryPageLimit,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.
| daemon.PricesQueryClient = queryClientMock | |
| actual, err := daemon.GetAllMarketPrices( | |
| grpc.Ctx, | |
| uint32(50), | |
| tc.limit, | |
| ) | |
| if err != nil { | |
| daemon.PricesQueryClient = queryClientMock | |
| actual, err := daemon.GetAllMarketPrices( | |
| grpc.Ctx, | |
| df.Liquidation.QueryPageLimit, | |
| ) | |
| if err != nil { |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/flags/flags.go (5 hunks)
Additional comments: 5
protocol/daemons/flags/flags.go (5)
23-25: The renaming of
FlagLiquidationDaemonSubaccountPageLimittoFlagLiquidationDaemonQueryPageLimitand the removal ofFlagLiquidationDaemonRequestChunkSizeare correctly reflected in the code.54-55: The
LiquidationFlagsstruct correctly reflects the removal ofSubaccountPageLimitandRequestChunkSizefields and the addition of theQueryPageLimitfield.91-93: The default values for the liquidation daemon flags, including the new
QueryPageLimit, are correctly set in theGetDefaultDaemonFlagsfunction.160-162: The
AddDaemonFlagsToCmdfunction correctly adds the newFlagLiquidationDaemonQueryPageLimitflag to the cobra command with the appropriate description.230-232: The
GetDaemonFlagValuesFromOptionsfunction correctly retrieves the value of the newFlagLiquidationDaemonQueryPageLimitflag from theAppOptionsstruct.
Changelist
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.