[CT-1050] DeliverTx state change reset for subaccount updates #2063
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
protocol/streaming/full_node_streaming_manager.go (2)
42-43: Add a comment for the new mapping.The
subaccountIdToSubscriptionIdMappingis a significant addition. Consider adding a comment to explain its purpose and usage.+ // map from subaccount id to subscription ids. subaccountIdToSubscriptionIdMapping map[satypes.SubaccountId][]uint32
59-60: Document thesubaccountIdsfield.The
subaccountIdsfield inOrderbookSubscriptionis a new addition. Adding a comment would improve code readability and maintainability.+ // Subaccount ids to subscribe to. subaccountIds []satypes.SubaccountId
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/streaming/full_node_streaming_manager.go (15 hunks)
Additional comments not posted (7)
protocol/streaming/full_node_streaming_manager.go (7)
81-86: Ensure proper initialization of new fields.The initialization of
subaccountIdToSubscriptionIdMappinglooks correct. Ensure that all new fields are properly initialized and used throughout the codebase.
256-269: Ensure consistency in subscription removal.The logic for removing subscriptions from
subaccountIdToSubscriptionIdMappingmirrors that forclobPairIdToSubscriptionIdMapping. Ensure consistency and correctness in both implementations.
498-526: Review theSendSubaccountUpdatesmethod.The
SendSubaccountUpdatesmethod is a new addition. Ensure that it correctly groups and sends subaccount updates.
588-619: Review theAddSubaccountUpdatesToCachemethod.The
AddSubaccountUpdatesToCachemethod is a new addition. Ensure that it correctly adds subaccount updates to the cache and handles buffer overflows appropriately.
Line range hint
683-714:
Verify snapshot initialization for subaccounts.The
InitializeNewStreamsmethod now includes subaccount snapshot initialization. Ensure that this logic is correctly integrated and does not introduce race conditions.Verification successful
Verify snapshot initialization for subaccounts in
InitializeNewStreams.The
InitializeNewStreamsmethod does indeed include logic for initializing subaccount snapshots. The method uses async.Onceinstance (subscription.initialize.Do) to ensure that snapshots are sent only once per subscription. This approach should prevent race conditions by ensuring that the initialization logic runs only once per subscription.
- Location:
protocol/streaming/full_node_streaming_manager.go, lines 681-718The integration appears correct, and the use of
sync.Oncehelps mitigate race conditions. However, manual verification or additional testing may be necessary to ensure that this logic functions correctly in all scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of subaccount snapshots in InitializeNewStreams. # Test: Check for correct initialization and integration. Expect: Proper handling of subaccount snapshots. rg --type go -A 5 $'func (sm *FullNodeStreamingManagerImpl) InitializeNewStreams'Length of output: 82
Script:
#!/bin/bash # Description: Search for any function definitions in FullNodeStreamingManagerImpl that might relate to stream initialization. # Test: Look for functions in FullNodeStreamingManagerImpl that might handle stream initialization. ast-grep --lang go --pattern $'func (sm *FullNodeStreamingManagerImpl) $_($$$) { $$$ }'Length of output: 39856
348-405: Review theSendSubaccountSnapshotmethod.The
SendSubaccountSnapshotmethod is a new addition. Ensure that it correctly handles subaccount snapshots and integrates well with the existing streaming logic.Verification successful
The
SendSubaccountSnapshotmethod is correctly implemented and integrated.The method is defined and used within the
protocol/streaming/full_node_streaming_manager.gofile, indicating proper integration with the existing streaming logic. Additionally, its performance is monitored through a metric key, ensuring its operation is tracked. No issues were found with its implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of the SendSubaccountSnapshot method. # Test: Check for correct implementation and integration. Expect: Proper handling of subaccount snapshots. rg --type go -A 5 $'func (sm *FullNodeStreamingManagerImpl) SendSubaccountSnapshot'Length of output: 84
Script:
#!/bin/bash # Description: Search for the SendSubaccountSnapshot method across the codebase # and check for any references to understand its integration. # Search for the definition of the SendSubaccountSnapshot method. ast-grep --lang go --pattern 'func (sm *FullNodeStreamingManagerImpl) SendSubaccountSnapshot($_, $_, $_, $_) { $$$ }' # Search for references or calls to the SendSubaccountSnapshot method to check its usage. rg --type go 'SendSubaccountSnapshot'Length of output: 4931
Line range hint
135-185:
Verify subaccount ID handling inSubscribemethod.The
Subscribemethod now includes handling for subaccount IDs. Ensure that this logic correctly integrates with existing subscription processes and that all edge cases are considered.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/streaming/noop_streaming_manager.go (1 hunks)
- protocol/streaming/types/interface.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/streaming/types/interface.go
Additional comments not posted (3)
protocol/streaming/noop_streaming_manager.go (3)
47-52: Confirm the purpose of theSendSubaccountUpdatesmethod.The
SendSubaccountUpdatesmethod is currently empty. As this class is a no-operation implementation, confirm if leaving it unimplemented is intentional.
Line range hint
13-18:
TheSubscribemethod changes are appropriate.The update to handle multiple subaccounts aligns with the intended functionality. Returning
types.ErrNotImplementedis suitable for a no-operation implementation.
Line range hint
38-45:
TheInitializeNewStreamsmethod changes are appropriate.The addition of the parameter for subaccount snapshots is consistent with the PR's objectives and appears to be correctly integrated.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
Additional comments not posted (2)
protocol/x/subaccounts/keeper/subaccount.go (2)
135-161: LGTM! TheGetStreamSubaccountUpdatefunction is well-implemented.The method effectively retrieves subaccount data and formats it for streaming. The use of
BigInt().Uint64()for quantums conversion is appropriate. The logic is clear and aligns with the intended functionality.
Line range hint
303-310:
LGTM! TheSendSubaccountUpdatesfunction is effectively implemented.The function appropriately checks for the presence of updates before sending them to the gRPC streaming manager. This ensures efficient use of resources and aligns with best practices.
| @@ -75,6 +91,21 @@ func (k Keeper) ProcessProposerOperations( | |||
| allUpdates.Append(orderbookUpdate) | |||
| } | |||
| k.SendOrderbookUpdates(ctx, allUpdates) | |||
|
|
|||
| // send subaccount snapshots | |||
| subaccountIdsFromProposed := fetchSubaccountIdsInvolvedInOpQueue( | |||
There was a problem hiding this comment.
super meganit, up to you: combine the two loops where we iterate through the opqueue?
There was a problem hiding this comment.
only sending local for now
Co-authored-by: Jonathan Fung <[email protected]>
| SubaccountId: &id, | ||
| UpdatedAssetPositions: assetPositions, | ||
| UpdatedPerpetualPositions: perpetualPositions, | ||
| Snapshot: true, |
There was a problem hiding this comment.
I don't think this is a snapshot
There was a problem hiding this comment.
Why isn't it? We are getting the subaccount from state which should have all asset/perp positions?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
Additional comments not posted (3)
protocol/x/clob/keeper/process_operations.go (3)
41-54: LGTM: Efficient retrieval of subaccount IDs.The function efficiently collects subaccount IDs involved in matches using a map to avoid duplicates. The use of
lib.MergeMapsis appropriate here.
76-79: Clarify subaccount snapshot criteria.The comment provides a clear definition of impacted subaccounts. Ensure this aligns with the actual implementation logic.
98-111: Consider combining loops for efficiency.The loops for processing order IDs and subaccount IDs could potentially be combined to improve efficiency, as suggested in previous reviews.
Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Jonathan Fung <[email protected]>
Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Jonathan Fung <[email protected]>
Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Jonathan Fung <[email protected]>
Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Jonathan Fung <[email protected]>
Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Jonathan Fung <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores