Fix filterOrdersBySubaccountId param token and helper name #2694
Conversation
WalkthroughThis pull request introduces a new feature to filter orderbook updates by subaccount IDs across multiple components of the dYdX protocol. The changes span from protobuf definitions to the WebSocket server, adding a boolean flag Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebSocketServer
participant StreamingManager
participant OrderbookFilter
Client->>WebSocketServer: Connect with filterOrdersBySubaccountId=true
WebSocketServer->>StreamingManager: Subscribe(clobPairIds, subaccountIds, filterOrders=true)
StreamingManager->>OrderbookFilter: Filter updates by subaccount
OrderbookFilter-->>StreamingManager: Filtered updates
StreamingManager-->>WebSocketServer: Send filtered updates
WebSocketServer-->>Client: Stream only relevant updates
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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 using PR comments)
Other keywords and placeholders
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: 6
🧹 Nitpick comments (10)
protocol/streaming/full_node_streaming_manager.go (5)
5-5: Unnecessary Import of 'slices' PackageThe
slicespackage imported at line 5 is not used in the code.Consider removing this import to clean up the code.
15-16: Organize Imports for Better ReadabilityAt lines 15-16, the imports could be grouped logically to enhance readability.
Consider grouping related imports together, such as all
github.com/dydxprotocol/v4-chain/protocolimports.
97-130: Redundant Function Definitions for 'NewOrderbookSubscription'There are two functions for creating a new
OrderbookSubscription:
- Lines 97-114:
NewOrderbookSubscription- Lines 116-130:
(sm *FullNodeStreamingManagerImpl) NewOrderbookSubscriptionConsider refactoring to eliminate redundancy. You might combine them or clearly differentiate their purposes.
246-267: Inefficient Filtering Logic in 'FilterStreamUpdateBySubaccount'The
FilterStreamUpdateBySubaccountfunction may process updates unnecessarily, impacting performance.Optimize the filtering logic to skip updates that do not relate to subscribed subaccounts, improving efficiency especially with large datasets.
348-353: Potential Side Effects Due to Reassigning 'updates'In lines 348-353, reassigning
updatesafter filtering could lead to unintended side effects.Assign the filtered updates to a new variable to preserve the original data and enhance code clarity.
protocol/streaming/util/util.go (1)
25-44: Enhance Error Messages in 'GetOffChainUpdateV1SubaccountIdNumber'The error message returned when an
UpdateMessagetype is unrecognized is not very informative.Provide more detailed error messages to aid debugging.
return 0, fmt.Errorf( - "UpdateMessage type not in {OrderPlace, OrderRemove, OrderUpdate, OrderReplace}: %+v", + "Unrecognized UpdateMessage type '%T' in GetOffChainUpdateV1SubaccountIdNumber: %+v", updateMessage, )protocol/streaming/types/interface.go (1)
19-19: Consider using a more specific parameter name.The parameter name
filterOrdersis too generic. Consider renaming it tofilterOrdersBySubaccountIdto better reflect its specific purpose of filtering orders by subaccount ID.protocol/streaming/util/util_test.go (1)
113-113: Remove debug print statement.Remove the debug print statement that was likely used during development.
- fmt.Println("expected", id)protocol/streaming/ws/websocket_server.go (2)
183-183: Fix line length.The comment line exceeds the maximum length of 120 characters. Consider wrapping or shortening it.
-// parseFilterOrdersBySubaccountId is a helper function to parse the filterOrdersBySubaccountId flag from the query parameters. +// parseFilterOrdersBySubaccountId parses the filterOrdersBySubaccountId flag from query parameters.🧰 Tools
🪛 GitHub Actions: Protocol Lint
[warning] 183-183: Line length exceeds maximum limit of 120 characters (current length: 127 characters)
191-191: Enhance error message.The error message could be more helpful by indicating the expected format.
- return false, fmt.Errorf("invalid filterOrdersBySubaccountId: %s", token) + return false, fmt.Errorf("invalid filterOrdersBySubaccountId: %s, expected 'true' or 'false'", token)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts(6 hunks)proto/dydxprotocol/clob/query.proto(1 hunks)protocol/streaming/full_node_streaming_manager.go(6 hunks)protocol/streaming/full_node_streaming_manager_test.go(1 hunks)protocol/streaming/noop_streaming_manager.go(1 hunks)protocol/streaming/types/interface.go(1 hunks)protocol/streaming/util/util.go(1 hunks)protocol/streaming/util/util_test.go(1 hunks)protocol/streaming/ws/websocket_server.go(3 hunks)protocol/x/clob/keeper/grpc_stream_orderbook.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Protocol Lint
protocol/streaming/ws/websocket_server.go
[warning] 183-183: Line length exceeds maximum limit of 120 characters (current length: 127 characters)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test-sim-multi-seed-short
- GitHub Check: test-sim-after-import
- GitHub Check: test-sim-import-export
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: test-coverage-upload
- GitHub Check: container-tests
- GitHub Check: test / run_command
- GitHub Check: benchmark
- GitHub Check: Summary
🔇 Additional comments (10)
protocol/x/clob/keeper/grpc_stream_orderbook.go (1)
15-15: Missing Validation for New ParameterAt line 15,
req.GetFilterOrdersBySubaccountId()is added to theSubscribecall, but there's no validation for this new parameter.Ensure that
filterOrdersBySubaccountIdis properly validated before use to prevent misuse or unexpected behavior.protocol/streaming/noop_streaming_manager.go (1)
27-27: LGTM!The noop implementation correctly implements the updated interface.
protocol/streaming/ws/websocket_server.go (1)
100-108: LGTM!The parameter parsing follows the same pattern as other parameters and includes proper error handling.
protocol/streaming/full_node_streaming_manager_test.go (3)
257-261: LGTM: Well-structured test case definition.The
TestCasestruct is well-designed with clear fields for inputs (updates,subaccountIds) and expected outputs (filteredUpdates).
318-463: Comprehensive test coverage for filtering scenarios.The test cases cover a wide range of scenarios:
- Base cases (in/out of scope)
- Multiple subaccounts
- Empty updates/IDs
- Different update types (fills, taker orders, etc.)
466-476: LGTM: Clean test execution.The test execution is well-structured:
- Uses table-driven tests for better organization
- Proper use of subtests for isolation
- Clear assertions using
requirepackageindexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (3)
281-287: LGTM: Clear documentation for the new field.The TypeScript interface properly documents the new
filterOrdersBySubaccountIdfield with clear comments explaining its purpose and behavior.
1315-1316: LGTM: Proper encoding implementation.The encoding logic correctly handles the boolean field:
- Only writes the field if it's true (protobuf optimization)
- Uses the correct wire format (uint32(32) for field number 4)
Also applies to: 1341-1344
1388-1391: LGTM: Proper decoding implementation.The decoding logic correctly handles the boolean field using case 4 to match the protobuf field number.
proto/dydxprotocol/clob/query.proto (1)
189-193: LGTM: Clear protobuf field definition.The new field is well-defined:
- Uses field number 4 (maintains backward compatibility)
- Has clear documentation
- Uses appropriate type (bool)
| func doFilterStreamUpdateBySubaccount( | ||
| orderBookUpdate *clobtypes.StreamUpdate_OrderbookUpdate, | ||
| subaccountIdNumbers []uint32, | ||
| logger log.Logger, | ||
| ) bool { | ||
| for _, orderBookUpdate := range orderBookUpdate.OrderbookUpdate.Updates { | ||
| orderBookUpdateSubaccountIdNumber, err := streaming_util.GetOffChainUpdateV1SubaccountIdNumber(orderBookUpdate) | ||
| if err == nil { | ||
| if slices.Contains(subaccountIdNumbers, orderBookUpdateSubaccountIdNumber) { | ||
| return true | ||
| } | ||
| } else { | ||
| logger.Error(err.Error()) | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete Error Handling in 'doFilterStreamUpdateBySubaccount'
In the doFilterStreamUpdateBySubaccount function, errors from GetOffChainUpdateV1SubaccountIdNumber are only logged, not handled.
It's important to handle errors appropriately to prevent unexpected behavior. Consider returning the error or taking corrective action when an error occurs.
There was a problem hiding this comment.
The only error thrown from streaming_util.GetOffChainUpdateV1SubaccountIdNumber results from handling a new OffChainUpdateV1 message type.
It isn't clear Full Node Streaming should crash under this condition.
There was a problem hiding this comment.
@UnbornAztecKing You're absolutely right. Looking at the implementation, I can see that GetOffChainUpdateV1SubaccountIdNumber only returns an error when encountering an unknown OffChainUpdateV1.UpdateMessage type, which is explicitly expected behavior when new message types are added. The current approach of logging the error and continuing is the correct pattern as it:
- Maintains forward compatibility with new message types that may be added in the future
- Provides debug information through logs
- Gracefully handles unknown types by filtering them out (returning false)
- Preserves service stability instead of crashing
🔗 Analysis chain
@UnbornAztecKing I agree with your assessment. The current error handling approach is appropriate since:
- It maintains forward compatibility with new message types
- Crashing the streaming service would be too disruptive for what is just a filtering operation
- The error is logged for debugging purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the implementation of GetOffChainUpdateV1SubaccountIdNumber
ast-grep --pattern 'func GetOffChainUpdateV1SubaccountIdNumber($_, $_) ($_, $_) {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for error returns in GetOffChainUpdateV1SubaccountIdNumber
rg "return.*error" -A 5 -B 5 "GetOffChainUpdateV1SubaccountIdNumber"
Length of output: 146
Script:
#!/bin/bash
# Search for GetOffChainUpdateV1SubaccountIdNumber function using ripgrep
rg -A 10 -B 10 "func.*GetOffChainUpdateV1SubaccountIdNumber"
Length of output: 3181
Script:
#!/bin/bash
# Try finding the file first
fd "util.go"
Length of output: 580
Script:
#!/bin/bash
# Search for any usage of GetOffChainUpdateV1SubaccountIdNumber
rg "GetOffChainUpdateV1SubaccountIdNumber"
Length of output: 544
✏️ Learnings added
Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2694
File: protocol/streaming/full_node_streaming_manager.go:223-239
Timestamp: 2025-01-17T22:33:30.979Z
Learning: In Full Node Streaming's filtering operations, when handling OffChainUpdateV1 messages (e.g., in doFilterStreamUpdateBySubaccount), errors from unknown message types should be logged but not cause crashes. This pattern maintains forward compatibility when new message types are added to OffChainUpdateV1.UpdateMessage, as indicated by the comment "Error expected if OffChainUpdateV1.UpdateMessage message type is extended to more order events".
Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2694
File: protocol/streaming/full_node_streaming_manager.go:223-239
Timestamp: 2025-01-17T22:33:30.979Z
Learning: In Full Node Streaming's filtering operations (e.g., doFilterStreamUpdateBySubaccount), errors from unknown message types should be logged but not cause crashes, to maintain forward compatibility and service stability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| syncLocalUpdates, syncLocalClobPairIds := getStreamUpdatesFromOffchainUpdates( | ||
| streaming_util.GetOffchainUpdatesV1(orderBookUpdatesToSyncLocalOpsQueue), | ||
| lib.MustConvertIntegerToUint32(ctx.BlockHeight()), | ||
| ctx.ExecMode(), | ||
| ) | ||
| sm.cacheStreamUpdatesByClobPairWithLock(sycnLocalUpdates, syncLocalClobPairIds) | ||
| sm.cacheStreamUpdatesByClobPairWithLock(syncLocalUpdates, syncLocalClobPairIds) |
There was a problem hiding this comment.
Missing Error Handling in 'getStreamUpdatesFromOffchainUpdates'
At lines 1166-1171, the function getStreamUpdatesFromOffchainUpdates is called without checking for errors.
Include error handling to catch and manage potential errors from this function.
| if filterOrdersBySubAccountId && (len(subaccountIds) == 0) { | ||
| sm.logger.Error("filterOrdersBySubaccountId with no subaccountIds") | ||
| return types.ErrInvalidStreamingRequest | ||
| } |
There was a problem hiding this comment.
Inconsistent Parameter Naming
At lines 283-286, filterOrdersBySubAccountId is used instead of filterOrdersBySubaccountId.
Ensure consistent naming conventions for parameters to prevent confusion and potential bugs.
- if filterOrdersBySubAccountId && (len(subaccountIds) == 0) {
+ if filterOrdersBySubaccountId && (len(subaccountIds) == 0) {📝 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.
| if filterOrdersBySubAccountId && (len(subaccountIds) == 0) { | |
| sm.logger.Error("filterOrdersBySubaccountId with no subaccountIds") | |
| return types.ErrInvalidStreamingRequest | |
| } | |
| if filterOrdersBySubaccountId && (len(subaccountIds) == 0) { | |
| sm.logger.Error("filterOrdersBySubAccountId with no subaccountIds") | |
| return types.ErrInvalidStreamingRequest | |
| } |
| tests := map[string]struct { | ||
| update ocutypes.OffChainUpdateV1 | ||
| id uint32 | ||
| err error | ||
| }{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test cases.
The test suite should include negative test cases to verify error handling for invalid updates.
Add test cases like:
"InvalidUpdate": {
update: ocutypes.OffChainUpdateV1{
UpdateMessage: nil,
},
id: 0,
err: errors.New("invalid update: missing update message"),
},| require.Equal(t, err, testCase.err) | ||
| require.Equal(t, id, testCase.id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix assertion argument order.
The require.Equal arguments are in the wrong order. The first argument should be the expected value, and the second should be the actual value.
- require.Equal(t, err, testCase.err)
- require.Equal(t, id, testCase.id)
+ require.Equal(t, testCase.err, err)
+ require.Equal(t, testCase.id, id)📝 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.
| require.Equal(t, err, testCase.err) | |
| require.Equal(t, id, testCase.id) | |
| require.Equal(t, testCase.err, err) | |
| require.Equal(t, testCase.id, id) |
| Quantums: uint64(10 ^ 6), | ||
| Subticks: 1, |
There was a problem hiding this comment.
Fix incorrect power operator usage.
In Go, ^ is the bitwise XOR operator, not the power operator. Use math.Pow() or direct multiplication for powers.
- Quantums: uint64(10 ^ 6),
- Subticks: 1,
+ Quantums: uint64(1_000_000), // 10^6
+ Subticks: 1,📝 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.
| Quantums: uint64(10 ^ 6), | |
| Subticks: 1, | |
| Quantums: uint64(1_000_000), // 10^6 | |
| Subticks: 1, |
Changelist
Full Node Streaming provides an initial state and streaming updates for positions, orders, prices and fills.
The subscription API admits an optional sub account ID filter, which is only applied to the initial positions and position changes.
The user would like to apply the sub account ID filter to the order messages, in addition to position messages.
The change will add boolean flags to the Websocket and GRPC streaming API's:
filterOrdersBySubaccountIdboolean field for WS request (if not provided, default to False)filter_orders_by_subaccount_idboolean field forStreamOrderbookUpdatesRequestprotobuf (if not provided, default to False)For all endpoints, the previous behavior of not filtering orders for subaccounts is preserved by default.
If filtering orders is not specified, the code path remains the same for looping over stream updates.
If filtering orders is specified, each slice of updates received from the subscription
updatesChannelwill be filtered like:StreamUpdate_OrderbookUpdate, forward itStreamUpdate_OrderbookUpdate, forward it only if one of theOffChainUpdateV1messages inside are for a target subaccountTest Plan
Unit test
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
Summary by CodeRabbit
New Features
Improvements
Technical Updates
filterOrdersparameter.