[ENG-1623] Do not return error when pnl/parentSubaccountNumber returns empty array#3291
[ENG-1623] Do not return error when pnl/parentSubaccountNumber returns empty array#3291davidli1997 merged 7 commits intomainfrom
Conversation
WalkthroughThis PR adds test coverage for empty PnL dataset scenarios and refactors the parent subaccount PnL endpoint to validate subaccount existence upfront while allowing empty results without error, replacing previous error-throwing behavior for missing PnL data. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (2)
.github/workflows/indexer-build-and-push-dev-staging.yml (1)
5-9: Avoid hard‑coding a personal feature branch in shared workflow triggersAdding
davidli/pnl_endpointis fine for local experimentation, but it’s usually better to:
- Either remove this entry before merge, or
- Replace it with a generic pattern (e.g.
feature/*) if you want workflows on many feature branches.Otherwise the workflow config accumulates stale, one-off branch names over time.
indexer/packages/postgres/__tests__/stores/pnl-table.test.ts (1)
857-919: Empty‑result coverage for hourly/daily aggregates is solidBoth tests correctly distinguish:
- No matches due to a restrictive
createdBeforeOrAtfilter, and- No PnL rows at all for valid subaccount IDs.
They verify that
findAllHourlyAggregate/findAllDailyAggregatereturnresults: []instead of erroring, which lines up with the new API behavior. If you want to extend coverage later, you could also asserttotal/offsetfor these empty cases, but the current assertions are sufficient for the regression you’re targeting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/indexer-build-and-push-dev-staging.yml(1 hunks)indexer/packages/postgres/__tests__/stores/pnl-table.test.ts(1 hunks)indexer/services/comlink/__tests__/controllers/api/v4/pnl-controller.test.ts(2 hunks)indexer/services/comlink/src/controllers/api/v4/pnl-controller.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
🧬 Code graph analysis (3)
indexer/services/comlink/src/controllers/api/v4/pnl-controller.ts (2)
indexer/packages/redis/__tests__/caches/constants.ts (1)
address(15-15)indexer/services/comlink/src/lib/errors.ts (1)
NotFoundError(15-20)
indexer/services/comlink/__tests__/controllers/api/v4/pnl-controller.test.ts (1)
indexer/services/comlink/__tests__/helpers/helpers.ts (2)
sendRequest(82-106)getQueryString(108-121)
indexer/packages/postgres/__tests__/stores/pnl-table.test.ts (1)
indexer/packages/postgres/__tests__/helpers/constants.ts (3)
defaultSubaccountId(156-159)defaultSubaccountId2(160-163)defaultPnl(1125-1132)
⏰ 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). (39)
- GitHub Check: lint
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: check-build-bazooka
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: test / run_command
- 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-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: (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: (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-ender / (ender) 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-vulcan / (vulcan) 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-and-push-testnet
- GitHub Check: run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (go)
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev4) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: Summary
🔇 Additional comments (4)
indexer/services/comlink/src/controllers/api/v4/pnl-controller.ts (1)
129-137: Parent subaccount existence check and 404 semantics look correctChecking
SubaccountTable.findByIdup front and throwingNotFoundErroronly when the parent subaccount row is missing cleanly separates:
- “Bad input” (non-existent parent subaccount → 404), from
- “No data yet” (existing parent with zero PnL → empty
pnlarray and 200).The error message matches the updated test expectation and the behavior is consistent with the single‑subaccount
/pnlendpoint.indexer/services/comlink/__tests__/controllers/api/v4/pnl-controller.test.ts (3)
979-985: Updated 404 expectation matches controller error contractThe new expected message:
'No subaccount found with address nonexistentaddress and parentSubaccountNumber 0'is consistent with the
NotFoundErrorthrown ingetPnlForParentSubaccount, so this test now accurately reflects the endpoint’s contract for non‑existent parents.
1226-1279: Good regression coverage for date‑filtered empty parent‑subaccount PnLThis test properly verifies that when all PnL records fall after
createdBeforeOrAt,/pnl/parentSubaccountNumber:
- Returns HTTP 200, and
- Returns
pnl: []for bothdaily=trueanddaily=false.That directly guards against the previous behavior where “no rows after filtering” was treated as an error.
1281-1311: Clear test for “no PnL yet” on an existing parent subaccountBy seeding subaccounts but not inserting any PnL rows, then asserting 200 +
pnl: []for both daily and hourly calls, this test ensures:
- Existing parent subaccounts no longer yield 404 purely due to missing PnL, and
- Behavior is consistent across aggregation modes.
The setup and assertions look correct and align tightly with the PR objective.
|
https://github.com/Mergifyio backport release/indexer/v9.x |
✅ Backports have been createdDetails
|
2 participants
Changelist
Fix
/v4/pnl/parentSubaccountNumberendpoint to return empty array instead of error when no PNL data matches filterspnlData.results.length === 0- now returns{"pnl":[]}instead of an errorTest Plan
Added controller tests:
Get /pnl/parentSubaccountNumber returns empty array when date filter excludes all data- verifies both daily and hourly endpoints return 200 with empty array when createdBeforeOrAt excludes all recordsGet /pnl/parentSubaccountNumber returns empty array when no PNL records exist- verifies empty array is returned when subaccount exists but has no PNL dataUpdated Get /pnl/parentSubaccountNumber with non-existent address returns 404- verifies 404 is still returned for non-existent addressesTested in staging,
before:
after:
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.