improve zero output order pairs handling#461
Conversation
WalkthroughChangesThe round-processing flow now partitions orders by sell-token balance, reports zero-output orders, returns total processed order counts, and exposes refreshed order metadata for CLI telemetry. Unit and end-to-end tests were updated for the new return shapes and reporting behavior. Order and metadata preparation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RainSolver
participant OrderManager
participant Telemetry
CLI->>RainSolver: processNextRound()
RainSolver->>OrderManager: getNextRoundOrders()
OrderManager-->>RainSolver: noneZeroOutput, zeroOutput
RainSolver->>Telemetry: export zero-output report
RainSolver-->>CLI: totalLength and round results
CLI->>Telemetry: record totalLength and order metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 🔧 ast-grep (0.44.1)test/e2e/e2e.test.jsast-grep timed out on this file 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/order/index.ts (1)
614-640: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccumulate
distinctPairsSetacross all orderbooks.By declaring
const distinctPairsSet = new Set<string>();inside the outerthis.ownersMap.forEachloop, the set is overwritten for each orderbook. As a result,totalDistinctPairsCountwill incorrectly reflect only the distinct pairs of the last orderbook processed.To correctly count distinct pairs globally, move the Set declaration outside the outer loop.
🐛 Proposed fix for the accumulation bug
- let totalDistinctPairsCount = 0; + const distinctPairsSet = new Set<string>(); this.ownersMap.forEach((ownersProfileMap) => { let obOwners = 0; let obOrders = 0; let obPairs = 0; - const distinctPairsSet = new Set<string>(); ownersProfileMap.forEach((ownerProfile) => { obOwners++; obOrders += ownerProfile.orders.size; ownerProfile.orders.forEach((orderProfile) => { obPairs += orderProfile.takeOrders.length; orderProfile.takeOrders.forEach((pair) => { distinctPairsSet.add(`${pair.buyToken}-${pair.sellToken}`); }); }); }); totalCount += obOrders; totalOwnersCount += obOwners; totalPairsCount += obPairs; - totalDistinctPairsCount = distinctPairsSet.size; }); this.metadata = { totalCount, totalOwnersCount, totalPairsCount, - totalDistinctPairsCount, + totalDistinctPairsCount: distinctPairsSet.size, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/order/index.ts` around lines 614 - 640, Move the distinctPairsSet declaration outside the outer this.ownersMap.forEach loop, then continue adding each order’s token pair to that shared set while processing all orderbooks. Assign totalDistinctPairsCount from the accumulated set size after the loop so it represents global distinct pairs rather than only the last orderbook.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/process/round.ts`:
- Around line 96-119: Update the zeroOutputReport construction in the round
processing flow to avoid storing the entire zeroOutputs array as one JSON span
attribute. Emit the zero-output order details through span events or bounded
batches, preserving pair, owner, orderHash, and orderbook while keeping each
OpenTelemetry attribute payload within exporter size limits.
In `@src/order/index.ts`:
- Around line 443-455: Rename the getNextRoundOrders result property
noneZeroOutput to nonZeroOutput, updating its return type, initialization, all
downstream callers, and test mocks consistently while preserving behavior.
---
Outside diff comments:
In `@src/order/index.ts`:
- Around line 614-640: Move the distinctPairsSet declaration outside the outer
this.ownersMap.forEach loop, then continue adding each order’s token pair to
that shared set while processing all orderbooks. Assign totalDistinctPairsCount
from the accumulated set size after the loop so it represents global distinct
pairs rather than only the last orderbook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 710ea9e2-6233-47dd-bba5-436686a1ce9b
📒 Files selected for processing (8)
src/cli/index.test.tssrc/cli/index.tssrc/core/index.tssrc/core/process/round.test.tssrc/core/process/round.tssrc/order/index.test.tssrc/order/index.tstest/e2e/e2e.test.js
| // report zero output order pairs | ||
| const zeroOutputReport = new PreAssembledSpan(`order_zero_output`, performance.now()); | ||
| zeroOutputReport.setAttr( | ||
| "details", | ||
| JSON.stringify( | ||
| zeroOutputs.map((p) => { | ||
| const pair = `${p.buyTokenSymbol}/${p.sellTokenSymbol}`; | ||
| const owner = p.takeOrder.struct.order.owner.toLowerCase(); | ||
| return { | ||
| pair, | ||
| owner, | ||
| orderHash: p.takeOrder.id, | ||
| orderbook: p.orderbook, | ||
| }; | ||
| }), | ||
| ), | ||
| ); | ||
| zeroOutputReport.end(); | ||
| this.logger?.exportPreAssembledSpan(zeroOutputReport, roundSpanCtx?.context); | ||
|
|
||
| return { | ||
| settlements, | ||
| checkpointReports, | ||
| totalLength, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Consider OpenTelemetry attribute size limits.
Serializing a potentially large array into a single JSON string for a span attribute can exceed the default length limits of OpenTelemetry exporters (often capped at 4096 bytes or similar). If a high volume of zero-output orders is expected, this attribute may be truncated or the span entirely rejected.
Consider logging this data as span events or breaking it into multiple batch attributes if high volumes are anticipated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/process/round.ts` around lines 96 - 119, Update the zeroOutputReport
construction in the round processing flow to avoid storing the entire
zeroOutputs array as one JSON span attribute. Emit the zero-output order details
through span events or bounded batches, preserving pair, owner, orderHash, and
orderbook while keeping each OpenTelemetry attribute payload within exporter
size limits.
| getNextRoundOrders(): { | ||
| noneZeroOutput: Pair[]; | ||
| zeroOutput: Pair[]; | ||
| } { | ||
| const result: { | ||
| noneZeroOutput: Pair[]; | ||
| zeroOutput: Pair[]; | ||
| } = { | ||
| noneZeroOutput: [], | ||
| zeroOutput: [], | ||
| }; | ||
| this.ownersMap.forEach((ownersProfileMap) => { | ||
| ownersProfileMap.forEach((ownerProfile) => { | ||
| ownersProfileMap.forEach((ownerProfile, owner) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename noneZeroOutput to nonZeroOutput.
"noneZero" is grammatically non-standard; "nonZero" is the idiomatic prefix. Please rename this property to nonZeroOutput here and across all downstream callers and test mocks to maintain a clean public API.
♻️ Proposed rename
getNextRoundOrders(): {
- noneZeroOutput: Pair[];
+ nonZeroOutput: Pair[];
zeroOutput: Pair[];
} {
const result: {
- noneZeroOutput: Pair[];
+ nonZeroOutput: Pair[];
zeroOutput: Pair[];
} = {
- noneZeroOutput: [],
+ nonZeroOutput: [],
zeroOutput: [],
};📝 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.
| getNextRoundOrders(): { | |
| noneZeroOutput: Pair[]; | |
| zeroOutput: Pair[]; | |
| } { | |
| const result: { | |
| noneZeroOutput: Pair[]; | |
| zeroOutput: Pair[]; | |
| } = { | |
| noneZeroOutput: [], | |
| zeroOutput: [], | |
| }; | |
| this.ownersMap.forEach((ownersProfileMap) => { | |
| ownersProfileMap.forEach((ownerProfile) => { | |
| ownersProfileMap.forEach((ownerProfile, owner) => { | |
| getNextRoundOrders(): { | |
| nonZeroOutput: Pair[]; | |
| zeroOutput: Pair[]; | |
| } { | |
| const result: { | |
| nonZeroOutput: Pair[]; | |
| zeroOutput: Pair[]; | |
| } = { | |
| nonZeroOutput: [], | |
| zeroOutput: [], | |
| }; | |
| this.ownersMap.forEach((ownersProfileMap) => { | |
| ownersProfileMap.forEach((ownerProfile, owner) => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/order/index.ts` around lines 443 - 455, Rename the getNextRoundOrders
result property noneZeroOutput to nonZeroOutput, updating its return type,
initialization, all downstream callers, and test mocks consistently while
preserving behavior.
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit