[CLOB-1050] update daemon liquidation info to include negative tnc su… #879
Conversation
…baccounts and open positions
WalkthroughThe updates across various files reflect a significant refactor in the handling of liquidation-related data within a trading protocol. The core change is the introduction of a new data structure, 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 (
|
| blockHeight uint32 // block height of the last update | ||
| liquidatableSubaccountIds []satypes.SubaccountId // liquidatable subaccount ids | ||
| negativeTncSubaccountIds []satypes.SubaccountId // negative total net collateral subaccount ids | ||
| subaccountsWithPositions map[uint32]clobtypes.SubaccountOpenPositionInfo |
There was a problem hiding this comment.
@jakob-dydx I thought map[uint32]map[bool]map[subaccountId]struct{} was kind of messy so I created a struct called SubaccountOpenPositionInfo. let me know what you think
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/x/clob/types/liquidations.pb.go
Files selected for processing (11)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/liquidations.ts (2 hunks)
- proto/dydxprotocol/clob/liquidations.proto (1 hunks)
- protocol/app/app.go (2 hunks)
- protocol/daemons/server/liquidation.go (3 hunks)
- protocol/daemons/server/liquidation_test.go (2 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (1 hunks)
- protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (1 hunks)
- protocol/x/clob/abci.go (2 hunks)
- protocol/x/clob/abci_test.go (3 hunks)
- protocol/x/clob/module.go (3 hunks)
- protocol/x/clob/module_test.go (1 hunks)
Additional comments: 37
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/liquidations.ts (2)
100-109: The
SubaccountOpenPositionInfoSDKTypeinterface uses snake_case for its properties, which aligns with the conventions for protobuf-generated SDK types. This change appears to be consistent with the PR's objective to update data structures for SDK usage.242-305: The creation functions and encode/decode methods for
SubaccountOpenPositionInfohave been added. It is important to ensure that these methods are thoroughly tested, especially since they handle the serialization and deserialization of the new data structure. This is crucial for data integrity and compatibility with the rest of the system that will consume these serialized forms.proto/dydxprotocol/clob/liquidations.proto (1)
- 36-46: The addition of the
SubaccountOpenPositionInfomessage type inliquidations.protois consistent with the PR objectives to update liquidation information. This new message type will hold information about open positions for a perpetual, including the ids of subaccounts with long and short positions. Ensure that all services and functions that will consume this new message type are updated accordingly to handle the new structure.
The addition of the
SubaccountOpenPositionInfomessage type inliquidations.protois confirmed. The search for the renamedSubaccountOpenPositionInfointerface in TypeScript files indicates that the renaming has taken place and corresponding encoding and decoding functions have been added. Ensure that all services and functions that will consume this new message type are updated accordingly to handle the new structure.protocol/app/app.go (3)
586-587: The introduction of
daemonLiquidationInfoand its usage in theServerinstance is consistent with the PR's objectives to update liquidation information handling. Ensure that theServer's methods that previously usedliquidatableSubaccountIdsare correctly updated to work withdaemonLiquidationInfo.871-871: The
daemonLiquidationInfois being passed to theClobKeeperduring its construction. Verify that theClobKeeper's implementation is correctly updated to use this new field instead ofliquidatableSubaccountIds, and that all related logic is consistent with the new data structure.
The verification process using shell scripts did not yield any output, indicating that the
ClobKeeper's methods usingdaemonLiquidationInfocould not be confirmed. This lack of evidence prevents a definitive conclusion regarding the update of theClobKeeperimplementation to use the new fielddaemonLiquidationInfoinstead ofliquidatableSubaccountIds.Given the absence of output from the verification script, it is not possible to confirm that the
ClobKeeper's implementation has been correctly updated to usedaemonLiquidationInfo. Therefore, further verification is required to ensure that theClobKeeperand related logic are consistent with the new data structure.
- 583-587: Given the structural changes to the liquidation information handling, it is crucial to manually verify against the PR checklist for state-breaking changes and modifications in critical functions. This is to ensure that the changes are compliant with the project's standards for such updates.
protocol/daemons/server/liquidation.go (4)
3-8: Check if the import section is correctly organized and if there are any unnecessary imports or missing ones that are required by the code.
13-16: The addition of the
daemonLiquidationInfofield to theLiquidationServerstruct aligns with the PR objectives to update the daemon liquidation information.18-26: The
WithDaemonLiquidationInfomethod has been correctly implemented to update thedaemonLiquidationInfofield in theLiquidationServerstruct.42-48: The
LiquidateSubaccountsmethod correctly uses thedaemonLiquidationInfofield to update liquidatable subaccount IDs, which is in line with the PR objectives.protocol/daemons/server/liquidation_test.go (2)
18-31: The update from
liquidatableSubaccountIdstodaemonLiquidationInfoinTestLiquidateSubaccounts_Empty_Updateis consistent with the PR's objectives and the AI-generated summaries. The test now correctly initializes thedaemonLiquidationInfoand uses its methods to assert the state of liquidatable subaccount IDs.15-47: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [37-58]
The changes in
TestLiquidateSubaccounts_Multiple_Subaccount_Idsalso reflect the renaming and reorganization of the liquidation-related data structure. The test is updated to usedaemonLiquidationInfoand its methods to assert the correct behavior when multiple subaccount IDs are involved in liquidation.protocol/daemons/server/types/liquidations/daemon_liquidation_info.go (4)
10-18: The introduction of the
DaemonLiquidationInfostruct with a mutex for thread safety is a good practice for managing concurrent access to liquidation-related information.81-117: The deep copying in
UpdateSubaccountsWithPositionsandGetSubaccountsWithPositionsmethods is a good practice to prevent race conditions in a concurrent environment.29-98: The locking pattern used in
UpdateBlockHeight,UpdateLiquidatableSubaccountIds,UpdateNegativeTncSubaccountIds, andUpdateSubaccountsWithPositionsmethods is consistent and correctly implemented for thread safety.36-116: The locking pattern used in
GetBlockHeight,GetLiquidatableSubaccountIds,GetNegativeTncSubaccountIds, andGetSubaccountsWithPositionsmethods is consistent and correctly implemented for thread safety.protocol/daemons/server/types/liquidations/daemon_liquidation_info_test.go (10)
13-18: The test
TestNewDaemonLiquidationInfocorrectly checks for the initialization state ofDaemonLiquidationInfo. It ensures that the newly created object has empty slices/maps for its fields, which is a good practice to confirm that the constructor-like function initializes the struct as expected.20-32: The test
TestLiquidatableSubaccountIds_Multiple_Readsverifies that theUpdateLiquidatableSubaccountIdsmethod correctly updates the internal state and that theGetLiquidatableSubaccountIdsmethod returns the updated state consistently across multiple reads. This is a good test to ensure the read methods are idempotent and thread-safe.34-46: The test
TestNegativeTncSubaccounts_Multiple_Readsis similar to the previous test but for theNegativeTncSubaccountIdsfield. It ensures that updates to the field are reflected correctly and that subsequent reads return the same data. This consistency is crucial for the reliability of the daemon's state management.48-67: The test
TestSubaccountsWithOpenPositions_Multiple_Readschecks the functionality for updating and retrieving the map of subaccounts with open positions. It's important to ensure that the complex data structure like a map is handled correctly, and this test appears to do so by checking for equality of the expected and actual maps.69-90: The test
TestLiquidatableSubaccountIds_Multiple_Writesverifies that multiple updates to theLiquidatableSubaccountIdsfield overwrite the previous state as expected. This is a good test to ensure that the update method works correctly and does not append or merge but replaces the state.92-113: The test
TestNegativeTncSubaccounts_Multiple_Writesperforms the same checks as the previous test but for theNegativeTncSubaccountIdsfield. It's good to see that the tests are thorough and cover multiple fields of the struct.115-160: The test
TestSubaccountsWithOpenPositions_Multiple_Writesensures that updates to the map of subaccounts with open positions are handled correctly. The test checks that the update method replaces the entire map and does not perform a merge, which is important for the correctness of the daemon's state.162-175: The test
TestLiquidatableSubaccountIds_Empty_Updatechecks the behavior of theUpdateLiquidatableSubaccountIdsmethod when an empty slice is passed. It's important to confirm that the method can handle empty updates correctly, as this might be a common case in the daemon's operation.177-190: The test
TestNegativeTnc_Empty_Updateis similar to the previous test but for theNegativeTncSubaccountIdsfield. It confirms that the update method correctly handles empty updates, which is a necessary check for robustness.192-213: The test
TestSubaccountsWithOpenPosition_Empty_Updatechecks the update behavior for the map of subaccounts with open positions when an empty map is passed. This test is crucial to ensure that the system can correctly handle the case where all positions are closed, and the map should be emptied.protocol/x/clob/abci.go (2)
117-123: The parameter change from
liquidatableSubaccountIdstodaemonLiquidationInfoin thePrepareCheckStatefunction is consistent with the PR objectives to update liquidation information. Ensure that all references to this parameter within the function have been updated accordingly.200-200: The method call
daemonLiquidationInfo.GetLiquidatableSubaccountIds()replaces the previousGetSubaccountIds()method. This renaming seems to better describe the method's functionality. Confirm that theGetLiquidatableSubaccountIdsmethod is correctly implemented in theDaemonLiquidationInfotype and that it returns the expected data.
The attempt to verify the implementation of
GetLiquidatableSubaccountIdsin theDaemonLiquidationInfotype usingast-grepresulted in an error due to a pattern parsing issue. The error message indicates that the pattern syntax was incorrect, which prevented the command from producing any output. Therefore, we cannot confirm the implementation of the method based on the provided script.Given this, we need to correct the syntax and re-run the script to verify the implementation of the method.
#!/bin/bash # Correct the pattern syntax and verify the implementation of GetLiquidatableSubaccountIds in the DaemonLiquidationInfo type. ast-grep --lang go --pattern $'type DaemonLiquidationInfo struct { $$$ func (DaemonLiquidationInfo) GetLiquidatableSubaccountIds() []string { $$$ } $$$ }'protocol/x/clob/abci_test.go (4)
1102-1102: The addition of
liquidationtypes.NewDaemonLiquidationInfo()as a parameter to thePrepareCheckStatefunction call in theTestPrepareCheckState_WithProcessProposerMatchesEventsWithBadBlockHeighttest is consistent with the PR's objective of updating the daemon liquidation information. Ensure that theNewDaemonLiquidationInfofunction initializes theDaemonLiquidationInfostruct correctly and that it is used appropriately within thePrepareCheckStatefunction.1129-1129: Similarly, the addition of
liquidationtypes.NewDaemonLiquidationInfo()as a parameter to thePrepareCheckStatefunction call in theTestCommitBlocker_WithProcessProposerMatchesEventsWithBadBlockHeighttest aligns with the PR's objective. Verify that theDaemonLiquidationInfostruct is being used as intended within thePrepareCheckStatefunction.1478-1479: The creation of a
DaemonLiquidationInfoobject and the update of liquidatable subaccount IDs in theTestPrepareCheckStatetest are in line with the PR's objective to include details about negative TNC subaccounts and open positions. This change is also consistent with the AI-generated summary, which mentions the addition of theDaemonLiquidationInfostruct and methods in the newtypespackage.1482-1482: The call to
PrepareCheckStatewith the newliquidatableSubaccountIdsparameter in theTestPrepareCheckStatetest is appropriate and reflects the changes made in the codebase as per the PR's objective. This ensures that the updated liquidation information is being considered in the state preparation.protocol/x/clob/module.go (4)
104-111: The addition of the
daemonLiquidationInfofield to theAppModulestruct is consistent with the PR objectives and the AI-generated summaries. This change is crucial for integrating the updated liquidation information into the module's functionality.117-131: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [114-128]
The
NewAppModulefunction correctly initializes thedaemonLiquidationInfofield in theAppModulestruct. This ensures that the new liquidation information is properly set up when creating a new instance ofAppModule.
194-198: The update to the
Commitmethod to passdaemonLiquidationInfotoPrepareCheckStateis consistent with the changes made to theAppModulestruct and theNewAppModulefunction. This ensures that the new liquidation information is utilized during the commit phase.117-131: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [104-199]
Verify that all usages of the
AppModulestruct and its methods throughout the codebase have been updated to accommodate the newdaemonLiquidationInfofield and the changes to theNewAppModuleandCommitfunctions.
Final review comment:
Verify that all usages of the
AppModulestruct and its methods throughout the codebase have been updated to accommodate the newdaemonLiquidationInfofield and the changes to theNewAppModuleandCommitfunctions. The extensive list of files provided by the scripts suggests that tests and various modules have been updated, but direct evidence of the changes in the main implementation files is not visible. Further verification is needed to ensure full compatibility.protocol/x/clob/module_test.go (1)
- 103-107: The addition of
liquidations_types.NewDaemonLiquidationInfo()in thecreateAppModuleWithKeeperfunction aligns with the PR objectives to update the daemon liquidation information. Ensure that theNewDaemonLiquidationInfofunction is properly implemented and that theDaemonLiquidationInfostruct and its methods are thread-safe as intended.
The function
NewDaemonLiquidationInfois present in the codebase as expected. However, the thread safety of its implementation needs to be verified. Further analysis of theDaemonLiquidationInfostruct and associated methods is required to ensure they are implemented with thread safety in mind.
protocol/daemons/server/types/liquidations/daemon_liquidation_info.go
Outdated
Show resolved
Hide resolved
| @@ -197,7 +197,7 @@ func PrepareCheckState( | |||
| } | |||
|
|
|||
| // 6. Get all potentially liquidatable subaccount IDs and attempt to liquidate them. | |||
| subaccountIds := liquidatableSubaccountIds.GetSubaccountIds() | |||
| subaccountIds := daemonLiquidationInfo.GetLiquidatableSubaccountIds() | |||
| if err := keeper.LiquidateSubaccountsAgainstOrderbook(ctx, subaccountIds); err != nil { | |||
| panic(err) | |||
There was a problem hiding this comment.
The use of panic for error handling in the PrepareCheckState function is consistent with the existing codebase's pattern for handling critical errors in ABCI functions. However, consider whether there are more graceful recovery strategies or error logging mechanisms that could be employed here in the future.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/x/clob/types/liquidations.pb.go
Files selected for processing (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/liquidations.ts (2 hunks)
- proto/dydxprotocol/clob/liquidations.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/dydxprotocol/clob/liquidations.proto
Additional comments: 4
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/liquidations.ts (4)
91-96: The addition of the
SubaccountOpenPositionInfointerface with perpetualId, subaccountsWithLongPosition, and subaccountsWithShortPosition properties aligns with the PR's objective to enhance liquidation information.102-106: The
SubaccountOpenPositionInfoSDKTypeinterface correctly uses snake_case for its properties, adhering to protobuf naming conventions.250-303: The implementation of encode, decode, and fromPartial functions for
SubaccountOpenPositionInfois necessary for handling serialization and partial object creation, which is crucial for the new data structure's integration into the system.237-305: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [91-303]
Ensure that the new interfaces and functions are correctly used throughout the codebase, and that any code that interacts with these new structures is updated accordingly.
The search results indicate that the new interfaces
SubaccountOpenPositionInfoandSubaccountOpenPositionInfoSDKTypeare being used in various Go files (liquidations.pb.go,daemon_liquidation_info.go,daemon_liquidation_info_test.go) and the corresponding protobuf file (liquidations.proto). Additionally, the TypeScript files in theindexerpackage also reflect these changes. This suggests that the necessary updates to accommodate the new data structures have been made across the codebase.Given this context, it appears that the integration of the new interfaces and functions has been handled correctly. However, without running the code or seeing the full implementation, it's not possible to guarantee that there are no issues. The changes should still be verified through testing to ensure that they work as expected within the system.
Final review comment:
The usage of
SubaccountOpenPositionInfoandSubaccountOpenPositionInfoSDKTypeacross the codebase has been identified in Go and TypeScript files, as well as in protobuf definitions, indicating that the necessary updates have been made to integrate the new structures. Ensure thorough testing is conducted to validate the changes.
| blockHeight uint32 // block height of the last update | ||
| liquidatableSubaccountIds []satypes.SubaccountId // liquidatable subaccount ids | ||
| negativeTncSubaccountIds []satypes.SubaccountId // negative total net collateral subaccount ids | ||
| subaccountsWithPositions map[uint32]clobtypes.SubaccountOpenPositionInfo |
There was a problem hiding this comment.
Suggestion: less error prone to have values of subaccountsWithPositions be a pointer? Cause default value of clobtypes.SubaccountOpenPositionInfo is technically valid (perp ID of 0, no open positions)
| // in the next block. Methods are goroutine safe. | ||
| type DaemonLiquidationInfo struct { | ||
| sync.Mutex // lock | ||
| blockHeight uint32 // block height of the last update |
There was a problem hiding this comment.
Are all of the liquidatable / negative TNC subaccounts read from the same block height (which is the same as this blockHeight field)?
There was a problem hiding this comment.
yes - I will be updating the liquidation daemon to make height based queries so everything is read from the same height
…baccounts and open positions
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.