[CT-1103] FNS subaccount WS support #2088
Conversation
WalkthroughThe recent modifications simplify the GitHub Actions workflow by consolidating multiple job definitions and focusing solely on building Docker images for AWS ECR. Additionally, the WebSocket server's handler has been enhanced to dynamically handle subaccount identifiers from incoming requests, improving error management and overall application usability. These updates lead to a more maintainable and user-friendly system capable of efficiently handling multiple subaccounts. Changes
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 (
|
dydxwill
changed the title
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/protocol-build-and-push.yml (2 hunks)
- protocol/streaming/ws/websocket_server.go (3 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/protocol-build-and-push.yml
Additional comments not posted (3)
protocol/streaming/ws/websocket_server.go (3)
103-130: Well-implementedparseSubaccountIdsfunction.The function effectively parses and validates
subaccountIds, handling errors gracefully. The use ofstrconv.Atoiensures numeric validation.
Line range hint
131-149:
Consistent implementation inparseClobPairIds.The function maintains consistency with
parseSubaccountIdsin parsing and error handling. This uniformity aids in maintainability.
69-78: LGTM! Verify the usage ofparseSubaccountIds.The addition of
parseSubaccountIdsenhances the flexibility of theHandlermethod. The error handling is robust and logs errors appropriately.Ensure that the usage of
parseSubaccountIdsis consistent and correctly integrated with other parts of the codebase.Verification successful
Usage of
parseSubaccountIdsis limited to a single file.The
parseSubaccountIdsfunction is defined and used exclusively withinprotocol/streaming/ws/websocket_server.go. There are no other references or usages of this function elsewhere in the codebase, ensuring its integration is consistent and localized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `parseSubaccountIds` in the codebase. # Test: Search for the function usage. Expect: Consistent integration across the codebase. rg --type go -A 5 $'parseSubaccountIds'Length of output: 1005
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
|
|
||
| return subaccountIds, nil | ||
| } | ||
|
|
||
| // parseClobPairIds is a helper function to parse the clobPairIds from the query parameters. | ||
| func parseClobPairIds(r *http.Request) ([]uint32, error) { | ||
| clobPairIdsParam := r.URL.Query().Get("clobPairIds") | ||
| if clobPairIdsParam == "" { |
There was a problem hiding this comment.
think we can remove this special case now that empty clobpair ids is valid, i think rest of code should result in nil, nil as well
| // parseSubaccountIds is a helper function to parse the subaccountIds from the query parameters. | ||
| func parseSubaccountIds(r *http.Request) ([]*satypes.SubaccountId, error) { | ||
| subaccountIdsParam := r.URL.Query().Get("subaccountIds") | ||
| if subaccountIdsParam == "" { |
There was a problem hiding this comment.
ditto here, i think split on empty string probably returns empty array
| } | ||
|
|
||
| idStrs := strings.Split(subaccountIdsParam, ",") | ||
| subaccountIds := make([]*satypes.SubaccountId, len(idStrs)) |
There was a problem hiding this comment.
pref zero-initialize to reduce error situations where an empty array initialized with len X has length X and we might check len(subaccounts) to determine success of parsing subaccount ids
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 (1 hunks)
- protocol/streaming/ws/websocket_server.go (3 hunks)
Additional comments not posted (3)
protocol/streaming/ws/websocket_server.go (2)
69-78: Verify the error handling forsubaccountIds.The added parsing logic for
subaccountIdsis robust, with appropriate error handling and logging. Ensure that the error messages are clear and informative for debugging purposes.
103-126: Consider edge cases inparseSubaccountIds.The function correctly parses
subaccountIdsand handles errors. However, consider edge cases such as empty strings or unexpected formats. Ensure that the function is resilient to malformed inputs.protocol/streaming/full_node_streaming_manager.go (1)
151-151: Ensure proper validation logic inSubscribe.The additional validation ensures that both
clobPairIdsandsubaccountIdsare non-empty. Verify that this change aligns with the intended functionality and does not disrupt existing workflows.
Changelist
[Describe or list the changes made in this PR]
Test Plan
websocat "ws://xxx:9091/ws?clobPairIds=1,2&subaccountIds=dydx1nzuttarf5k2j0nug5yzhr6p74t9avehn9hlh8m/0"
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
subaccountIdsfrom incoming requests, improving flexibility and usability.subaccountIds, ensuring smooth operation and user feedback.clobPairIdsandsubaccountIdsare provided for valid subscription requests.