fix(streaming): handle nil taker order entities #3232

UnbornAztecKing · 2025-10-29T20:42:00Z

Changelist

Check for nil taker order entities at each level when filtering taker orders for streaming.

Test Plan

Added unit tests for check routine.
Deployment to internal networks and testnet.

Summary by CodeRabbit

Bug Fixes
- Improved null-safety in streaming update filtering to prevent edge-case failures when order or taker data is missing; call sites updated to use the safer filter.
Tests
- Added comprehensive tests for order filtering (covers nil, missing, matching and non-matching cases).
- Note: a duplicate test definition was introduced and should be cleaned up.

coderabbitai · 2025-10-29T20:42:30Z

Walkthrough

The pull request exports doFilterTakerOrderBySubaccount as DoFilterTakerOrderBySubaccount, adds nil-safety guards for takerOrder and nested Order, updates the call site in stream filtering, reorganizes imports, and adds unit tests for the exported function.

Changes

Cohort / File(s)	Summary
Function export & robustness `protocol/streaming/full_node_streaming_manager.go`	Renamed and exported `doFilterTakerOrderBySubaccount` → `DoFilterTakerOrderBySubaccount`; added nil checks for `takerOrder`, `takerOrder.TakerOrder`, and `order`; updated `doFilterStreamUpdateBySubaccount` to call the exported function; imports reordered.
Tests added / import reorg `protocol/streaming/full_node_streaming_manager_test.go`	Added `TestDoFilterTakerOrderBySubaccount` covering nil/missing and matching/non-matching scenarios; imports reorganized. Note: test function appears duplicated in the file.

Sequence Diagram(s)

sequenceDiagram
  participant StreamMgr as FullNodeStreamingManager
  participant FilterFn as DoFilterTakerOrderBySubaccount

  StreamMgr->>FilterFn: call DoFilterTakerOrderBySubaccount(takerOrder, subaccountIds)
  alt takerOrder is nil or takerOrder.TakerOrder is nil
    FilterFn-->>StreamMgr: return false
  else order is nil
    FilterFn-->>StreamMgr: return false
  else
    FilterFn-->>StreamMgr: return contains(order.OrderId.SubaccountId, subaccountIds)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Attention items:
- Remove or resolve duplicated TestDoFilterTakerOrderBySubaccount.
- Confirm nil-safety semantics match intended stream-filtering behavior.
- Verify all call sites were updated if there are other usages of the old unexported name.

Possibly related PRs

Full Node Streaming: filter all pairs when none are specified #2746 — Renames/exports the same taker-order-by-subaccount filter and adds tests; strongly related to this change.
Full Node Streaming Order Filtering by Subaccount impl and tests #2704 — Contains related edits to streaming manager subaccount filtering and call sites; touches similar logic.

Suggested reviewers

teddyding
matthewdowney
hwray

Poem

🐰 I hopped from private shade to light,

DoFilter now dances in plain sight.
Nil guards held fast, no tumble or fall,
Tests nibble crumbs and check them all.
A tidy hop — safe filters for all! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding nil safety checks for taker order entities in the streaming manager.
Description check	✅ Passed	The description includes both Changelist and Test Plan sections as required by the template, though the Author/Reviewer Checklist is not completed.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch handle-nil-taker-order

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2f8c2 and 3ebc692.

📒 Files selected for processing (1)

protocol/streaming/full_node_streaming_manager_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

protocol/streaming/full_node_streaming_manager_test.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)

GitHub Check: check-sample-pregenesis-up-to-date
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
GitHub Check: build
GitHub Check: test-coverage-upload
GitHub Check: liveness-test
GitHub Check: container-tests
GitHub Check: test-race
GitHub Check: unit-end-to-end-and-integration
GitHub Check: golangci-lint
GitHub Check: benchmark
GitHub Check: build-and-push-mainnet
GitHub Check: build-and-push-testnet
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Analyze (go)
GitHub Check: Summary

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55bdb1a and 2f2f8c2.

📒 Files selected for processing (2)

protocol/streaming/full_node_streaming_manager.go (3 hunks)
protocol/streaming/full_node_streaming_manager_test.go (2 hunks)

🧰 Additional context used

🧠 Learnings (9)

📓 Common learnings

Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#2779
File: protocol/x/clob/types/order.go:263-268
Timestamp: 2025-04-14T15:37:26.961Z
Learning: For TWAP orders in the dydxprotocol/v4-chain codebase, validation in message_place_order.go ensures that TwapParameters is not nil before the GetTotalLegsTWAPOrder method is called, making additional nil checks unnecessary.

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2704
File: indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts:281-287
Timestamp: 2025-01-27T14:49:32.366Z
Learning: The FilterOrdersBySubaccountId flag in StreamOrderbookUpdatesRequest is handled by passing req.GetFilterOrdersBySubaccountId() to the Subscribe method in protocol/x/clob/keeper/grpc_stream_orderbook.go.

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

📚 Learning: 2025-01-27T14:49:32.366Z

Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2704
File: indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts:281-287
Timestamp: 2025-01-27T14:49:32.366Z
Learning: The FilterOrdersBySubaccountId flag in StreamOrderbookUpdatesRequest is handled by passing req.GetFilterOrdersBySubaccountId() to the Subscribe method in protocol/x/clob/keeper/grpc_stream_orderbook.go.

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2024-11-15T16:00:11.304Z

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2024-11-15T15:59:28.095Z

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2024-11-15T16:17:29.092Z

Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2025-05-24T00:45:00.519Z

Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#2837
File: proto/dydxprotocol/clob/order.proto:4-4
Timestamp: 2025-05-24T00:45:00.519Z
Learning: In the dydxprotocol/v4-chain repository, cosmos_proto dependencies are managed through buf.yaml configuration rather than being physically present in the repository. The import "cosmos_proto/cosmos.proto" is a standard pattern used across 28+ proto files for cosmos address annotations like (cosmos_proto.scalar) = "cosmos.AddressString".

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2025-01-25T19:55:32.807Z

Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2704
File: protocol/streaming/full_node_streaming_manager.go:205-218
Timestamp: 2025-01-25T19:55:32.807Z
Learning: In the streaming manager's subaccount filtering logic, `slices.Contains` is used instead of a map-based set as it's an optimization for small sets of target subaccountIds, where the overhead of creating and maintaining a hash map would outweigh the benefits.

Applied to files:

protocol/streaming/full_node_streaming_manager_test.go
protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2025-01-17T22:33:31.219Z

Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2694
File: protocol/streaming/full_node_streaming_manager.go:223-239
Timestamp: 2025-01-17T22:33:31.219Z
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".

Applied to files:

protocol/streaming/full_node_streaming_manager.go

📚 Learning: 2025-01-17T22:33:31.219Z

Learnt from: UnbornAztecKing
PR: dydxprotocol/v4-chain#2694
File: protocol/streaming/full_node_streaming_manager.go:223-239
Timestamp: 2025-01-17T22:33:31.219Z
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.

Applied to files:

protocol/streaming/full_node_streaming_manager.go

🧬 Code graph analysis (2)

protocol/streaming/full_node_streaming_manager_test.go (2)

protocol/x/clob/types/query.pb.go (8)

StreamUpdate_TakerOrder (1196-1198)

StreamUpdate_TakerOrder (1208-1208)

StreamTakerOrder (1409-1421)

StreamTakerOrder (1425-1425)

StreamTakerOrder (1426-1428)

StreamTakerOrder (1501-1506)

StreamTakerOrder_Order (1462-1464)

StreamTakerOrder_Order (1469-1469)

protocol/streaming/full_node_streaming_manager.go (1)

DoFilterTakerOrderBySubaccount (220-232)

protocol/streaming/full_node_streaming_manager.go (1)

protocol/x/clob/types/query.pb.go (2)

StreamUpdate_TakerOrder (1196-1198)

StreamUpdate_TakerOrder (1208-1208)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)

GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
GitHub Check: test-coverage-upload
GitHub Check: unit-end-to-end-and-integration
GitHub Check: test-race
GitHub Check: liveness-test
GitHub Check: check-sample-pregenesis-up-to-date
GitHub Check: build
GitHub Check: build-and-push-testnet
GitHub Check: golangci-lint
GitHub Check: container-tests
GitHub Check: benchmark
GitHub Check: build-and-push-mainnet
GitHub Check: Analyze (go)
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Summary

🔇 Additional comments (2)

protocol/streaming/full_node_streaming_manager.go (2)

220-232: LGTM! Nil-safety checks are correctly implemented.

The function now properly handles nil values at all three levels:

Line 224: guards against nil takerOrder

Line 224: guards against nil takerOrder.TakerOrder

Lines 228-230: guards against nil return from GetOrder()

This defensive approach prevents potential panics when processing taker order updates.

284-284: LGTM! Call site correctly updated.

The call to the newly exported function is correct and maintains the same behavior.

protocol/streaming/full_node_streaming_manager_test.go

fix(streaming): handle nil taker order entities

2f2f8c2

UnbornAztecKing requested a review from a team as a code owner October 29, 2025 20:42

UnbornAztecKing temporarily deployed to testnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet October 29, 2025 20:42 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet October 29, 2025 20:42 — with GitHub Actions Inactive

github-actions bot added the protocol label Oct 29, 2025

coderabbitai bot reviewed Oct 29, 2025

View reviewed changes

protocol/streaming/full_node_streaming_manager_test.go Show resolved Hide resolved

add test case

3ebc692

UnbornAztecKing temporarily deployed to testnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to testnet November 5, 2025 17:57 — with GitHub Actions Inactive

UnbornAztecKing temporarily deployed to mainnet November 5, 2025 17:57 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(streaming): handle nil taker order entities #3232

fix(streaming): handle nil taker order entities #3232

UnbornAztecKing commented Oct 29, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 29, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

fix(streaming): handle nil taker order entities #3232

Are you sure you want to change the base?

fix(streaming): handle nil taker order entities #3232

Conversation

UnbornAztecKing commented Oct 29, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelist

Test Plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

UnbornAztecKing commented Oct 29, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 29, 2025 •

edited

Loading