test - fns su impl #2044
test - fns su impl #2044
Conversation
WalkthroughThe recent updates significantly enhance the dydxprotocol's streaming capabilities for managing subaccount updates. New modules and interfaces have been introduced, alongside modifications to existing structures, allowing for efficient real-time data handling. This restructuring promotes better integration with the Full Node Streaming Manager, ultimately refining the protocol's performance and scalability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FullNodeStreamingManager
participant SubaccountKeeper
participant StreamUpdate
Client->>FullNodeStreamingManager: Subscribe(subaccountIds)
FullNodeStreamingManager-->>Client: Acknowledge subscription
FullNodeStreamingManager->>SubaccountKeeper: SendSubaccountUpdates(subaccountUpdates)
SubaccountKeeper->>StreamUpdate: Process updates
StreamUpdate-->>FullNodeStreamingManager: Forward updates
FullNodeStreamingManager-->>Client: Stream updates
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/subaccounts/types/streaming.pb.gois excluded by!**/*.pb.go
Files selected for processing (17)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/streaming.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
- proto/dydxprotocol/clob/query.proto (3 hunks)
- proto/dydxprotocol/subaccounts/streaming.proto (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (14 hunks)
- protocol/streaming/noop_streaming_manager.go (3 hunks)
- protocol/streaming/types/interface.go (3 hunks)
- protocol/testutil/keeper/subaccounts.go (2 hunks)
- protocol/x/clob/keeper/grpc_stream_orderbook.go (1 hunks)
- protocol/x/subaccounts/keeper/keeper.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
- protocol/x/subaccounts/types/types.go (1 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
- indexer/packages/v4-protos/src/codegen/google/bundle.ts
Additional comments not posted (41)
protocol/x/clob/keeper/grpc_stream_orderbook.go (1)
13-13: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
StreamOrderbookUpdatesmatch the new signature.protocol/streaming/types/interface.go (2)
16-18: LGTM!The addition of the
subaccountIdsparameter to theSubscribemethod is consistent and correct.
39-43: LGTM!The new method
SendSubaccountUpdatesis well-defined and consistent with the interface's purpose.protocol/streaming/noop_streaming_manager.go (2)
Line range hint
24-28:
LGTM!The addition of the
subaccountIdsparameter to theSubscribemethod is consistent and correct.
47-52: LGTM!The new method
SendSubaccountUpdatesis consistent with the purpose of theNoopGrpcStreamingManagerclass.proto/dydxprotocol/subaccounts/streaming.proto (3)
10-23: LGTM!The
StreamSubaccountUpdatemessage is well-defined with clear comments explaining each field.
28-32: LGTM!The
SubaccountPerpetualPositionmessage is well-defined with clear comments explaining each field.
37-41: LGTM!The
SubaccountAssetPositionmessage is well-defined with clear comments explaining each field.protocol/x/subaccounts/types/types.go (1)
75-78: LGTM!The
SendSubaccountUpdatesmethod is a valuable addition to theSubaccountsKeeperinterface, enhancing its functionality to process subaccount updates.protocol/x/subaccounts/keeper/keeper.go (3)
5-5: LGTM!The new dependency
streamingtypesis correctly imported for streaming functionality.
25-25: LGTM!The
Keeperstruct modification to include thestreamingManagerfield is correctly implemented and enhances the functionality.
Line range hint
38-49:
LGTM!The
NewKeeperconstructor modification to accept thestreamingManagerparameter and initialize the new field is correctly implemented and enhances the functionality.protocol/testutil/keeper/subaccounts.go (1)
4-4: LGTM!The new import statement for the
streamingpackage is correct and necessary for the new functionality.protocol/lib/metrics/metric_keys.go (1)
73-73: LGTM!The new constant
GrpcSendSubaccountUpdatesLatencyenhances the metrics tracking capabilities and is correctly added.proto/dydxprotocol/clob/query.proto (2)
15-15: LGTM!The new import statement for
streaming.protofrom thesubaccountsdirectory is correct and necessary for the new functionality.
191-191: LGTM!The change to the field
subaccount_updatein theStreamUpdatemessage clarifies the origin of theStreamSubaccountUpdatemessage and is correctly implemented.indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/streaming.ts (11)
9-28: InterfaceStreamSubaccountUpdateis well-defined.The interface includes necessary fields for subaccount updates.
34-52: InterfaceStreamSubaccountUpdateSDKTypeis well-defined.The interface includes necessary fields for subaccount updates.
59-65: InterfaceSubaccountPerpetualPositionis well-defined.The interface includes necessary fields for subaccount perpetual positions.
71-77: InterfaceSubaccountPerpetualPositionSDKTypeis well-defined.The interface includes necessary fields for subaccount perpetual positions.
83-89: InterfaceSubaccountAssetPositionis well-defined.The interface includes necessary fields for subaccount asset positions.
103-109: FunctioncreateBaseStreamSubaccountUpdateis well-defined.The function initializes a base
StreamSubaccountUpdateobject with default values.
112-176: FunctionStreamSubaccountUpdateis well-defined.The methods handle the encoding, decoding, and partial creation of
StreamSubaccountUpdateobjects.
178-182: FunctioncreateBaseSubaccountPerpetualPositionis well-defined.The function initializes a base
SubaccountPerpetualPositionobject with default values.
185-231: FunctionSubaccountPerpetualPositionis well-defined.The methods handle the encoding, decoding, and partial creation of
SubaccountPerpetualPositionobjects.
233-237: FunctioncreateBaseSubaccountAssetPositionis well-defined.The function initializes a base
SubaccountAssetPositionobject with default values.
240-286: FunctionSubaccountAssetPositionis well-defined.The methods handle the encoding, decoding, and partial creation of
SubaccountAssetPositionobjects.protocol/x/subaccounts/keeper/subaccount.go (2)
740-742: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetFullNodeStreamingManagerare correctly implemented.Verification successful
Function
GetFullNodeStreamingManageris correctly implemented and used.The function
GetFullNodeStreamingManageris used in various files and contexts within the codebase, and all usages appear to be correctly implemented. The function is utilized for sending updates and checking if the streaming manager is enabled, which aligns with its intended purpose.
File:
protocol/x/subaccounts/keeper/subaccount.go
- Line 746:
k.GetFullNodeStreamingManager().SendSubaccountUpdates(...)File:
protocol/x/clob/keeper/order_state.go
- Line 92:
if k.GetFullNodeStreamingManager().Enabled() { ... }File:
protocol/x/clob/keeper/grpc_stream_orderbook.go
- Line 34:
err := k.GetFullNodeStreamingManager().Subscribe(...)File:
protocol/x/clob/keeper/process_operations.go
- Lines 107, 120, 133, 146, 159:
if streamingManager := k.GetFullNodeStreamingManager(); streamingManager.Enabled() { ... }File:
protocol/x/clob/keeper/keeper.go
- Lines 60, 72, 84:
k.GetFullNodeStreamingManager().SendOrderbookUpdates(...)The function calls are consistent with the expected usage, and no issues were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetFullNodeStreamingManager` are correctly implemented. # Test: Search for the function usage. Expect: Proper implementation of the function calls. rg --type go -A 5 $'GetFullNodeStreamingManager'Length of output: 5642
744-757: LGTM! But verify the streaming manager's method.The code changes are approved.
However, ensure that the
SendSubaccountUpdatesmethod in the streaming manager is correctly implemented.Verification successful
The
SendSubaccountUpdatesmethod in the streaming manager is correctly implemented.The method groups subaccount updates by their IDs, creates stream updates, and adds them to the cache, ensuring proper handling of subaccount updates.
protocol/streaming/full_node_streaming_manager.golines 434-461Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `SendSubaccountUpdates` method in the streaming manager is correctly implemented. # Test: Search for the method implementation. Expect: Proper implementation of the method. rg --type go -A 5 $'SendSubaccountUpdates'Length of output: 3939
Script:
#!/bin/bash # Extract the full implementation of the `SendSubaccountUpdates` method in `FullNodeStreamingManagerImpl`. rg --type go -A 50 $'func (sm *FullNodeStreamingManagerImpl) SendSubaccountUpdates('Length of output: 260
Script:
#!/bin/bash # Extract the full implementation of the `SendSubaccountUpdates` method in `FullNodeStreamingManagerImpl`. rg --type go -A 50 'func (sm \*FullNodeStreamingManagerImpl) SendSubaccountUpdates('Length of output: 263
Script:
#!/bin/bash # Extract the full implementation of the `SendSubaccountUpdates` method in `FullNodeStreamingManagerImpl` using ast-grep. ast-grep --lang go --pattern $'func (sm *FullNodeStreamingManagerImpl) SendSubaccountUpdates($_, $_, $_) {\n $$$\n}'Length of output: 2558
protocol/app/app.go (1)
1061-1061: LGTM! But verify the integration of the new parameter.The code changes are approved.
However, ensure that the
FullNodeStreamingManagerparameter is correctly integrated within theNewfunction.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4)
367-369: Verify the usage of the new entity in theClientFactoryexport.A new entity has been added to the
ClientFactoryexport. Ensure that this entity is correctly utilized in the codebase.
101-102: Verify the usage of the new import.The import statement for
subaccounts/subaccounthas been replaced withsubaccounts/streaming. Ensure that the new import is correctly utilized in the codebase.Verification successful
Verified the usage of the new import.
The import statement for
subaccounts/subaccounthas been replaced withsubaccounts/streaming. The new import is correctly utilized in the codebase.
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new import. # Test: Search for the usage of the new import. Expect: Occurrences of the new import in the codebase. rg --type typescript 'from "./subaccounts/streaming"'Length of output: 90
Script:
#!/bin/bash # Description: Verify the usage of the new import. # Test: Search for the usage of the new import. Expect: Occurrences of the new import in the codebase. rg --type ts 'from "./subaccounts/streaming"'Length of output: 158
Script:
#!/bin/bash # Description: Verify the usage of the new import variable `_105`. # Test: Search for the usage of `_105` in the `bundle.ts` file. Expect: Occurrences of `_105` in the file. rg '_105' indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.tsLength of output: 133
359-365: Verify the usage of the new entities in thevestexport.Several entities have been added to the
vestexport. Ensure that these entities are correctly utilized in the codebase.
349-354: Verify the removal of entities from thevaultexport.Several entities have been removed from the
vaultexport. Ensure that these entities are no longer needed in the codebase.protocol/streaming/full_node_streaming_manager.go (7)
56-57: Verify the usage of the new field inOrderbookSubscription.A new field has been added to the struct to support subaccount IDs. Ensure that this field is correctly utilized in the codebase.
Verification successful
Verify the usage of the new field in
OrderbookSubscription.The new field
subaccountIdshas been added to theOrderbookSubscriptionstruct and is used in multiple places in the codebase, includingprotocol/streaming/full_node_streaming_manager.go. Ensure that this field is correctly utilized in the codebase.
protocol/streaming/full_node_streaming_manager.go:
- Lines where
subaccountIdsis declared and used: 56-57, 59, 61-62, 64, 66, 68, 70, 72, 74, 76, 78, 80, 82, 84, 86Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new field in `OrderbookSubscription`. # Test: Search for the usage of the new field. Expect: Occurrences of the new field in the codebase. rg --type go 'subaccountIds'Length of output: 2119
33-40: Verify the usage of the new fields inFullNodeStreamingManagerImpl.New fields have been added to the struct to support streaming updates and subscriptions. Ensure that these fields are correctly utilized in the codebase.
Verification successful
The new fields in
FullNodeStreamingManagerImplare correctly utilized in the codebase.The fields
streamUpdateCache,streamUpdateSubscriptionCache,clobPairIdToSubscriptionIdMapping, andsubaccountIdToSubscriptionIdMappingare actively used in various parts of the code, ensuring their integration into the functionality.
streamUpdateCache: Initialized, appended to, and cleared.streamUpdateSubscriptionCache: Initialized, appended to, and cleared.clobPairIdToSubscriptionIdMapping: Initialized, appended to, checked, iterated over, and deleted.subaccountIdToSubscriptionIdMapping: Initialized, appended to, checked, iterated over, and deleted.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new fields in `FullNodeStreamingManagerImpl`. # Test: Search for the usage of the new fields. Expect: Occurrences of the new fields in the codebase. rg --type go 'streamUpdateSubscriptionCache' 'clobPairIdToSubscriptionIdMapping' 'subaccountIdToSubscriptionIdMapping'Length of output: 264
Script:
#!/bin/bash # Description: Verify the usage of the new fields in `FullNodeStreamingManagerImpl`. # Test: Search for the usage of the new fields. Expect: Occurrences of the new fields in the codebase. rg --type go 'streamUpdateCache' rg --type go 'streamUpdateSubscriptionCache' rg --type go 'clobPairIdToSubscriptionIdMapping' rg --type go 'subaccountIdToSubscriptionIdMapping'Length of output: 4949
Line range hint
535-574:
Verify the usage of the modified functionFlushStreamUpdatesWithLock.The function has been modified to support the new fields. Ensure that these modifications are correctly utilized in the codebase.
Verification successful
Verified the usage of the modified function
FlushStreamUpdatesWithLock.The function is correctly utilized in the codebase, with both calls ensuring the lock is acquired before invocation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the modified function `FlushStreamUpdatesWithLock`. # Test: Search for the usage of the modified function. Expect: Occurrences of the modified function in the codebase. rg --type go 'FlushStreamUpdatesWithLock'Length of output: 484
Script:
#!/bin/bash # Extract context around the usage of `FlushStreamUpdatesWithLock` within `protocol/streaming/full_node_streaming_manager.go`. rg --type go 'FlushStreamUpdatesWithLock' -A 5 -B 5Length of output: 2365
78-83: Verify the usage of the new parameter inNewFullNodeStreamingManager.The function parameters have been modified to include subaccount IDs. Ensure that this parameter is correctly utilized in the codebase.
434-461: Verify the usage of the new functionSendSubaccountUpdates.A new function has been added to handle subaccount updates. Ensure that this function is correctly utilized in the codebase.
Verification successful
The function
SendSubaccountUpdatesis correctly utilized in the codebase.
- The function is integrated within the
subaccount.gofile as part of theKeeperstruct.- It ensures that the streaming manager's
SendSubaccountUpdatesmethod is called appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new function `SendSubaccountUpdates`. # Test: Search for the usage of the new function. Expect: Occurrences of the new function in the codebase. rg --type go 'SendSubaccountUpdates'Length of output: 1024
Script:
#!/bin/bash # Description: Gather context around the usage of the function `SendSubaccountUpdates` in `subaccount.go`. # Get 10 lines before and after the match in `subaccount.go` to understand the context. rg --type go 'SendSubaccountUpdates' -A 10 -B 10 protocol/x/subaccounts/keeper/subaccount.goLength of output: 640
463-494: Verify the usage of the modified functionAddOrderbookUpdatesToCache.The function has been modified to support the new fields. Ensure that these modifications are correctly utilized in the codebase.
Verification successful
The usage of the modified function
AddOrderbookUpdatesToCacheis correctly implemented.The function calls are passing the correct arguments, and these arguments are properly defined and used in the surrounding code.
streamUpdates,clobPairIds, andupdatesororderbookFillsare correctly populated before being passed toAddOrderbookUpdatesToCache.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the modified function `AddOrderbookUpdatesToCache`. # Test: Search for the usage of the modified function. Expect: Occurrences of the modified function in the codebase. rg --type go 'AddOrderbookUpdatesToCache'Length of output: 428
Script:
#!/bin/bash # Extract lines around the call sites of AddOrderbookUpdatesToCache to verify correct usage rg --type go 'AddOrderbookUpdatesToCache' -A 5 -B 5Length of output: 2721
Script:
#!/bin/bash # Extract definitions and usages of streamUpdates, clobPairIds, updates, and orderbookFills around the call sites of AddOrderbookUpdatesToCache rg --type go 'streamUpdates|clobPairIds|updates|orderbookFills' -A 5 -B 5 protocol/streaming/full_node_streaming_manager.goLength of output: 11919
Line range hint
132-178:
Verify the usage of the new parameter inSubscribe.The function parameters have been modified to include subaccount IDs. Ensure that this parameter is correctly utilized in the codebase.
| @@ -128,6 +129,7 @@ func createSubaccountsKeeper( | |||
| btk, | |||
| rsk, | |||
| mockIndexerEventsManager, | |||
| streaming.NewNoopGrpcStreamingManager(), | |||
There was a problem hiding this comment.
Refactor suggestion: Pass the streaming manager as an argument.
To maintain consistency and flexibility, the NoopGrpcStreamingManager should be passed as an argument to the function.
- streaming.NewNoopGrpcStreamingManager(),
+ streamingManager,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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| streaming.NewNoopGrpcStreamingManager(), | |
| streamingManager, |
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/full_node_streaming_manager.go (15 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (4 hunks)
Additional comments not posted (16)
protocol/streaming/full_node_streaming_manager.go (13)
33-40: Verify initialization and usage of new fields.The new fields
streamUpdateSubscriptionCache,clobPairIdToSubscriptionIdMapping, andsubaccountIdToSubscriptionIdMappingshould be correctly initialized and used throughout the code.
56-57: Verify usage of the newsubaccountIdsfield.Ensure the new
subaccountIdsfield is correctly used in the subscription management logic.
78-83: Verify initialization of new fields.Ensure the new fields
streamUpdateSubscriptionCache,clobPairIdToSubscriptionIdMapping, andsubaccountIdToSubscriptionIdMappingare correctly initialized.
115-115: Verify metric reporting process.Ensure the metric reporting process correctly reflects the total number of updates in the
streamUpdateCache.
Line range hint
132-146:
Verify usage of the newsubaccountIdsparameter.Ensure the new
subaccountIdsparameter is correctly used in the subscription management logic.
150-171: Verify subscription management logic.Ensure the subscription management logic correctly updates the
clobPairIdToSubscriptionIdMappingandsubaccountIdToSubscriptionIdMappingmaps.
234-262: Verify updated subscription removal logic.Ensure the subscription removal logic correctly updates the
clobPairIdToSubscriptionIdMappingandsubaccountIdToSubscriptionIdMappingmaps.
365-386: Verify cache update logic.Ensure the cache update logic correctly aligns with the new list-based approach.
Line range hint
404-430:
Verify cache update logic.Ensure the cache update logic correctly aligns with the new list-based approach.
432-460: Verify integration of the newSendSubaccountUpdatesfunction.Ensure the new
SendSubaccountUpdatesfunction correctly integrates with the existing update handling framework.
Line range hint
463-494:
Verify cache management logic.Ensure the cache management logic correctly aligns with the new list-based approach.
496-516: Verify integration of the newAddSubaccountUpdatesToCachefunction.Ensure the new
AddSubaccountUpdatesToCachefunction correctly integrates with the existing cache management framework.
Line range hint
535-574:
Verify flush logic.Ensure the flush logic correctly aligns with the new list-based approach for collecting updates.
protocol/x/subaccounts/keeper/subaccount.go (3)
257-284: Verify processing logic and creation ofStreamSubaccountUpdateobject.Ensure the
GenerateStreamSubaccountUpdatefunction correctly processes asset and perpetual positions and creates theStreamSubaccountUpdateobject.
780-782: Verify retrieval logic.Ensure the
GetFullNodeStreamingManagerfunction correctly retrieves theFullNodeStreamingManager.
784-797: Verify integration and handling of empty updates.Ensure the
SendSubaccountUpdatesfunction correctly integrates with the gRPC streaming manager and handles empty updates.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (17 hunks)
- protocol/streaming/noop_streaming_manager.go (3 hunks)
- protocol/streaming/types/interface.go (4 hunks)
- protocol/x/clob/keeper/keeper.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/lib/metrics/metric_keys.go
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/types/interface.go
Additional comments not posted (8)
protocol/streaming/full_node_streaming_manager.go (8)
78-83: LGTM! Ensure proper initialization of new fields.The initialization of new fields in the
FullNodeStreamingManagerImplstruct looks good. Ensure that these fields are correctly utilized in other parts of the code.
115-118: LGTM! Ensure accurate metric reporting.The metric reporting logic looks good. Ensure that the metrics accurately reflect the state of the
streamUpdateCache.
234-262: LGTM! Ensure proper removal of subaccount ID mappings.The removal logic for subaccount ID mappings looks good. Ensure that the mappings are correctly updated in other parts of the code.
341-398: LGTM! Ensure proper handling of subaccount snapshots.The logic for sending subaccount snapshots looks good. Ensure that the snapshots are correctly handled in other parts of the code.
491-520: LGTM! Ensure proper handling of subaccount updates.The logic for sending subaccount updates looks good. Ensure that the updates are correctly handled in other parts of the code.
Line range hint
555-577: LGTM! Ensure proper caching of subaccount updates.The logic for caching subaccount updates looks good. Ensure that the updates are correctly handled in other parts of the code.
Line range hint
650-681: LGTM! Ensure proper initialization of subaccount snapshots.The logic for initializing subaccount snapshots looks good. Ensure that the snapshots are correctly handled in other parts of the code.
Line range hint
132-171: Ensure proper validation of subaccount IDs.The subscription logic looks good. Ensure that the subaccount IDs are properly validated before adding them to the subscription.
| assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions)) | ||
| for i, ap := range subaccount.AssetPositions { | ||
| assetPositions[i] = &satypes.SubaccountAssetPosition{ | ||
| AssetId: ap.AssetId, | ||
| Quantums: ap.Quantums.BigInt().Uint64(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the creation of asset positions.
The creation of assetPositions can be optimized by initializing the slice with the exact capacity and using a single append operation.
- assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions))
- for i, ap := range subaccount.AssetPositions {
- assetPositions[i] = &satypes.SubaccountAssetPosition{
- AssetId: ap.AssetId,
- Quantums: ap.Quantums.BigInt().Uint64(),
- }
- }
+ assetPositions := make([]*satypes.SubaccountAssetPosition, 0, len(subaccount.AssetPositions))
+ for _, ap := range subaccount.AssetPositions {
+ assetPositions = append(assetPositions, &satypes.SubaccountAssetPosition{
+ AssetId: ap.AssetId,
+ Quantums: ap.Quantums.BigInt().Uint64(),
+ })
+ }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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions)) | |
| for i, ap := range subaccount.AssetPositions { | |
| assetPositions[i] = &satypes.SubaccountAssetPosition{ | |
| AssetId: ap.AssetId, | |
| Quantums: ap.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| assetPositions := make([]*satypes.SubaccountAssetPosition, 0, len(subaccount.AssetPositions)) | |
| for _, ap := range subaccount.AssetPositions { | |
| assetPositions = append(assetPositions, &satypes.SubaccountAssetPosition{ | |
| AssetId: ap.AssetId, | |
| Quantums: ap.Quantums.BigInt().Uint64(), | |
| }) | |
| } |
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions)) | ||
| for i, pp := range subaccount.PerpetualPositions { | ||
| perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{ | ||
| PerpetualId: pp.PerpetualId, | ||
| Quantums: pp.Quantums.BigInt().Uint64(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the creation of perpetual positions.
The creation of perpetualPositions can be optimized by initializing the slice with the exact capacity and using a single append operation.
- perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions))
- for i, pp := range subaccount.PerpetualPositions {
- perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{
- PerpetualId: pp.PerpetualId,
- Quantums: pp.Quantums.BigInt().Uint64(),
- }
- }
+ perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, 0, len(subaccount.PerpetualPositions))
+ for _, pp := range subaccount.PerpetualPositions {
+ perpetualPositions = append(perpetualPositions, &satypes.SubaccountPerpetualPosition{
+ PerpetualId: pp.PerpetualId,
+ Quantums: pp.Quantums.BigInt().Uint64(),
+ })
+ }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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions)) | |
| for i, pp := range subaccount.PerpetualPositions { | |
| perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{ | |
| PerpetualId: pp.PerpetualId, | |
| Quantums: pp.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, 0, len(subaccount.PerpetualPositions)) | |
| for _, pp := range subaccount.PerpetualPositions { | |
| perpetualPositions = append(perpetualPositions, &satypes.SubaccountPerpetualPosition{ | |
| PerpetualId: pp.PerpetualId, | |
| Quantums: pp.Quantums.BigInt().Uint64(), | |
| }) | |
| } |
protocol/x/clob/keeper/keeper.go
Outdated
| func(subaccountId *satypes.SubaccountId) *satypes.StreamSubaccountUpdate { | ||
| subaccount := k.subaccountsKeeper.GetSubaccount( | ||
| ctx, | ||
| *subaccountId, | ||
| ) | ||
| assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions)) | ||
| for i, ap := range subaccount.AssetPositions { | ||
| assetPositions[i] = &satypes.SubaccountAssetPosition{ | ||
| AssetId: ap.AssetId, | ||
| Quantums: ap.Quantums.BigInt().Uint64(), | ||
| } | ||
| } | ||
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions)) | ||
| for i, pp := range subaccount.PerpetualPositions { | ||
| perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{ | ||
| PerpetualId: pp.PerpetualId, | ||
| Quantums: pp.Quantums.BigInt().Uint64(), | ||
| } | ||
| } | ||
|
|
||
| return &satypes.StreamSubaccountUpdate{ | ||
| SubaccountId: subaccountId, | ||
| UpdatedAssetPositions: assetPositions, | ||
| UpdatedPerpetualPositions: perpetualPositions, | ||
| Snapshot: false, | ||
| } | ||
| }, |
There was a problem hiding this comment.
Ensure proper error handling for subaccount retrieval.
The closure for subaccount updates does not handle potential errors from k.subaccountsKeeper.GetSubaccount. Ensure that the subaccount retrieval is successful before proceeding.
+ subaccount, err := k.subaccountsKeeper.GetSubaccount(
+ ctx,
+ *subaccountId,
+ )
+ if err != nil {
+ k.Logger(ctx).Error("Failed to retrieve subaccount", "subaccountId", subaccountId, "error", err)
+ return nil
+ }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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func(subaccountId *satypes.SubaccountId) *satypes.StreamSubaccountUpdate { | |
| subaccount := k.subaccountsKeeper.GetSubaccount( | |
| ctx, | |
| *subaccountId, | |
| ) | |
| assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions)) | |
| for i, ap := range subaccount.AssetPositions { | |
| assetPositions[i] = &satypes.SubaccountAssetPosition{ | |
| AssetId: ap.AssetId, | |
| Quantums: ap.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions)) | |
| for i, pp := range subaccount.PerpetualPositions { | |
| perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{ | |
| PerpetualId: pp.PerpetualId, | |
| Quantums: pp.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| return &satypes.StreamSubaccountUpdate{ | |
| SubaccountId: subaccountId, | |
| UpdatedAssetPositions: assetPositions, | |
| UpdatedPerpetualPositions: perpetualPositions, | |
| Snapshot: false, | |
| } | |
| }, | |
| func(subaccountId *satypes.SubaccountId) *satypes.StreamSubaccountUpdate { | |
| subaccount, err := k.subaccountsKeeper.GetSubaccount( | |
| ctx, | |
| *subaccountId, | |
| ) | |
| if err != nil { | |
| k.Logger(ctx).Error("Failed to retrieve subaccount", "subaccountId", subaccountId, "error", err) | |
| return nil | |
| } | |
| assetPositions := make([]*satypes.SubaccountAssetPosition, len(subaccount.AssetPositions)) | |
| for i, ap := range subaccount.AssetPositions { | |
| assetPositions[i] = &satypes.SubaccountAssetPosition{ | |
| AssetId: ap.AssetId, | |
| Quantums: ap.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| perpetualPositions := make([]*satypes.SubaccountPerpetualPosition, len(subaccount.PerpetualPositions)) | |
| for i, pp := range subaccount.PerpetualPositions { | |
| perpetualPositions[i] = &satypes.SubaccountPerpetualPosition{ | |
| PerpetualId: pp.PerpetualId, | |
| Quantums: pp.Quantums.BigInt().Uint64(), | |
| } | |
| } | |
| return &satypes.StreamSubaccountUpdate{ | |
| SubaccountId: subaccountId, | |
| UpdatedAssetPositions: assetPositions, | |
| UpdatedPerpetualPositions: perpetualPositions, | |
| Snapshot: false, | |
| } | |
| }, |
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)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (14 hunks)
- protocol/x/clob/keeper/keeper.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/keeper/keeper.go
- protocol/x/subaccounts/keeper/subaccount.go
Additional comments not posted (8)
.github/workflows/protocol-build-and-push.yml (1)
6-6: Verify the branch trigger addition.The branch
lw/sa2has been added to trigger the workflow. Ensure that this aligns with your intended workflow behavior and that the branch name is correct.protocol/streaming/full_node_streaming_manager.go (7)
81-86: LGTM! Initialization logic is correct.The initialization of
subaccountIdToSubscriptionIdMappingin the constructor is consistent with the other fields.
252-265: LGTM! Subscription removal logic is correct.The logic for handling
subaccountIdToSubscriptionIdMappingin theremoveSubscriptionmethod is consistent with the existing logic.
42-43: Verify the integration ofsubaccountIdToSubscriptionIdMapping.Ensure that the new field
subaccountIdToSubscriptionIdMappingis used consistently throughout the codebase and that it aligns with the intended functionality.Verification successful
Integration of
subaccountIdToSubscriptionIdMappingis consistent and correct.The field
subaccountIdToSubscriptionIdMappingis properly initialized and used throughout thefull_node_streaming_manager.gofile. It is consistently accessed and modified in a way that aligns with its intended functionality as a map from subaccount IDs to subscription IDs. No issues were found with its integration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `subaccountIdToSubscriptionIdMapping`. # Test: Search for all occurrences of `subaccountIdToSubscriptionIdMapping` to ensure consistent usage. rg 'subaccountIdToSubscriptionIdMapping'Length of output: 1541
59-60: Verify the integration ofsubaccountIds.Ensure that the new field
subaccountIdsis used consistently throughout the codebase and that it aligns with the intended functionality.Verification successful
Integration of
subaccountIdsappears consistent.The
subaccountIdsfield is used in various parts of the codebase, particularly within the streaming manager functionalities, and seems to be integrated consistently. No discrepancies were found in its usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `subaccountIds`. # Test: Search for all occurrences of `subaccountIds` to ensure consistent usage. rg 'subaccountIds'Length of output: 9747
Line range hint
135-174: Verify the subscription logic forsubaccountIds.Ensure that the logic for handling
subaccountIdsin theSubscribemethod is correct and aligns with the intended functionality.Verification successful
Subscription logic for
subaccountIdsis correct.The handling of
subaccountIdsin theSubscribemethod aligns with the intended functionality, ensuring that eachsubaccountIdis correctly mapped to its subscription ID. No issues were found with this logic.
- The
Subscribemethod correctly updates thesubaccountIdToSubscriptionIdMappingfor eachsubaccountId.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for handling `subaccountIds` in the `Subscribe` method. # Test: Search for references to `subaccountIds` in the `Subscribe` method to ensure correct handling. ast-grep --lang go --pattern $'func (sm *FullNodeStreamingManagerImpl) Subscribe($_, subaccountIds []*satypes.SubaccountId, $_) { $$$ }'Length of output: 139
Script:
#!/bin/bash # Search for the definition of the Subscribe method in the codebase. rg 'func \(sm \*FullNodeStreamingManagerImpl\) Subscribe' -A 20 # Search for references to subaccountIds in the file to verify its usage. rg 'subaccountIds' protocol/streaming/full_node_streaming_manager.go -A 5Length of output: 3921
494-530: Verify the logic inSendSubaccountUpdates.Ensure that the logic for sending subaccount updates is correct and aligns with the intended functionality.
Verification successful
Logic in
SendSubaccountUpdatesis CorrectThe
SendSubaccountUpdatesmethod correctly groups subaccount updates, logs them, and passes them to the caching mechanism. TheAddSubaccountUpdatesToCachefunction effectively manages the cache and handles overflow scenarios. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic in `SendSubaccountUpdates`. # Test: Search for references to `SendSubaccountUpdates` to ensure correct implementation and usage. ast-grep --lang go --pattern $'func (sm *FullNodeStreamingManagerImpl) SendSubaccountUpdates($_, $_, $_) { $$$ }'Length of output: 3112
Script:
#!/bin/bash # Search for the implementation of `AddSubaccountUpdatesToCache` to verify its functionality. ast-grep --lang go --pattern $'func (sm *FullNodeStreamingManagerImpl) AddSubaccountUpdatesToCache($_, $_, $_) { $$$ }'Length of output: 2854
344-401: Verify the logic inSendSubaccountSnapshot.Ensure that the logic for sending subaccount snapshots is correct and aligns with the intended functionality.
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)
- .github/workflows/protocol-build-and-push.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/protocol-build-and-push.yml
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/streaming/full_node_streaming_manager.go (14 hunks)
- protocol/streaming/noop_streaming_manager.go (3 hunks)
- protocol/streaming/types/interface.go (4 hunks)
- protocol/x/clob/keeper/keeper.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/types/interface.go
- protocol/x/clob/keeper/keeper.go
Additional comments not posted (8)
protocol/streaming/full_node_streaming_manager.go (8)
42-43: Enhancement in subscription management.The addition of
subaccountIdToSubscriptionIdMappingandsubaccountIdsfields enhances the granularity of subscription management by allowing tracking of subaccount IDs.Also applies to: 59-60
81-86: Proper initialization of new fields.The constructor now initializes
subaccountIdToSubscriptionIdMapping, ensuring that new fields are properly set up.
256-269: Consistency in subscription removal.The
removeSubscriptionmethod now updatessubaccountIdToSubscriptionIdMapping, ensuring consistency in subscription management.
348-405: New method for handling subaccount snapshots.The
SendSubaccountSnapshotmethod provides a dedicated function for handling subaccount snapshots, enhancing update management.
498-534: Efficient handling of subaccount updates.The
SendSubaccountUpdatesmethod efficiently groups and sends subaccount updates, contributing to enhanced update management.
569-607: Caching strategy for subaccount updates.The
AddSubaccountUpdatesToCachemethod efficiently manages caching of subaccount updates, aligning with the new subscription model.
Line range hint
671-702:
Enhanced initialization process for new streams.The
InitializeNewStreamsmethod now includes logic to send subaccount snapshots, supporting comprehensive update management.
Line range hint
135-185:
Enhanced subscription management inSubscribemethod.The
Subscribemethod now supports subaccount IDs, allowing for more detailed subscription management.Ensure that all calls to
Subscribeare updated to include the new parameter.Verification successful
Enhanced subscription management in
Subscribemethod verified.The
Subscribemethod is correctly implemented and used with the newsubaccountIdsparameter across the codebase. All relevant calls and method signatures include this parameter, ensuring consistent and accurate subscription management.
protocol/x/clob/keeper/grpc_stream_orderbook.go:Subscribeis called withreq.GetSubaccountIds().protocol/streaming/noop_streaming_manager.go: Method signature includessubaccountIds.protocol/streaming/types/interface.go: Method signature includessubaccountIds.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Subscribe` include the new parameter. # Test: Search for the function usage. Expect: All occurrences include subaccount IDs. rg --type go -A 5 $'Subscribe'Length of output: 3476
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
streamingmodule for enhanced management of subaccount updates.Bug Fixes
Documentation
Chores