improve zero output order pairs handling - #461
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
WalkthroughChangesThe round-processing flow partitions orders by sell-token balance, reports zero-output orders, returns total processed order counts, and exposes refreshed order metadata. Wallet operations now use timed scheduling and aggregated report attributes. Trade simulation uses a revised gas headroom factor, with tests updated for all changes. Order and round processing
Wallet operations and reporting
Trade simulation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RainSolver
participant OrderManager
participant Telemetry
CLI->>RainSolver: processNextRound()
RainSolver->>OrderManager: getNextRoundOrders()
OrderManager-->>RainSolver: nonZeroOutput, zeroOutput
RainSolver->>Telemetry: export zero-output report
RainSolver-->>CLI: totalLength and round results
CLI->>Telemetry: record round metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/order/index.ts (3)
472-497: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize vault-map lookup keys before partitioning.
addToTokenVaultsMap()stores orderbook, owner, and token keys in lowercase, but this lookup uses rawpair.orderbook,owner, and token addresses. A checksummed address misses the cached balance and falls back to stale pair data, so a zero-output pair can enternonZeroOutput.Use
.toLowerCase()for all lookup keys and add a regression test with mixed-case addresses.🤖 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 472 - 497, Normalize the orderbook, owner, sell-token, and buy-token keys to lowercase in the lookups within the consumingOrders balance-update loop, matching the keys written by addToTokenVaultsMap. Add a regression test covering mixed-case addresses and verify cached balances are used so zero-output pairs are partitioned correctly.
635-641: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompute distinct pairs across all orderbooks.
distinctPairsSetis recreated for each orderbook andtotalDistinctPairsCountis overwritten on every iteration. With multiple orderbooks, telemetry reports only the final orderbook’s count rather than the total distinct pair count.🤖 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 635 - 641, Update the orderbook aggregation flow around totalDistinctPairsCount and distinctPairsSet so distinct pair identities are accumulated across all orderbooks instead of resetting or overwriting the count per iteration. Ensure the metadata returned by the surrounding method reports the combined distinct-pair count while preserving the existing totalCount, totalOwnersCount, and totalPairsCount calculations.
201-202: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid recomputing metadata for every order during initial fetch.
fetch()callsaddOrder()for every order and then recalculates metadata once more at Line 132. SincegetCurrentMetadata()scans all owners, orders, and pairs, initial synchronization becomes quadratic as order volume grows. Defer the per-order refresh duringfetch()and retain the final refresh.🤖 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 201 - 202, Update addOrder() to skip the getCurrentMetadata() call when invoked during fetch()’s initial synchronization, while preserving metadata refreshes for other order additions. Keep fetch()’s existing final getCurrentMetadata() call so metadata is recalculated once after all orders are loaded.src/cli/index.ts (1)
354-375: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdvance maintenance timers only after successful operations.
Both timers are moved forward before the awaited work completes. If pending-worker removal, conversion, or any worker sweep fails, the error exits this method and retries are suppressed for one day or five days; a failed worker also prevents later workers from being swept.
Move each timer assignment after its corresponding operations succeed, and add failure-path coverage.
🤖 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/cli/index.ts` around lines 354 - 375, In the maintenance flow containing retryPendingRemoveWorkers, convertHoldingsToGas, and the worker sweep loop, move nextGasConversionTime and nextSweepTime updates until after all corresponding awaited operations complete successfully. Ensure failures leave the relevant timer unchanged so the work is retried, while preserving worker iteration behavior and adding coverage for failed removal, conversion, and worker-sweep operations.src/cli/index.test.ts (1)
763-764: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the new
totalLengthtelemetry contract.The mocked
processNextRound()result omitstotalLength, so this test permitsordersMetadata.roundProcessedOrderPairsCountto receiveundefined. Include a concretetotalLengthand assert that attribute explicitly.Proposed test update
(mockRainSolver.processNextRound as Mock).mockResolvedValue({ results: mockResults, reports: mockReports, checkpointReports: mockCheckpointReports, + totalLength: 4, }); +expect(mockRoundSpan.setAttribute).toHaveBeenCalledWith( + "ordersMetadata.roundProcessedOrderPairsCount", + 4, +);🤖 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/cli/index.test.ts` around lines 763 - 764, Update the mocked processNextRound() result in the relevant test to include a concrete totalLength value, then explicitly assert that ordersMetadata.roundProcessedOrderPairsCount is set to that value alongside the existing telemetry assertion.
🤖 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 97-111: Update the zero-output batching loop in
src/core/process/round.ts:97-111 to use a strict less-than condition so empty
batches never export order_zero_output spans. Update
src/core/process/round.test.ts:436-447 to remove the empty-report expectation
and add coverage confirming a non-empty zero-output batch is exported.
In `@src/wallet/index.test.ts`:
- Around line 361-371: The wallet report tests currently validate transfers and
swaps with substring checks, which can accept malformed or mismatched entries.
Update the relevant assertions in the report tests to parse the serialized
transfers/swaps data and structurally assert arrayContaining objects keyed by
token and type, covering skipped entries and native-gas cases while preserving
the expected transaction, status, and amount fields.
In `@src/wallet/index.ts`:
- Around line 350-370: Add an explicit native-gas discriminator to the
`remainingGas` record before it is pushed into `transfers`, using the
established identity value such as `type: "gas"`. Preserve the existing transfer
status, amount, and transaction fields and ensure the discriminator is present
for both success and failure paths.
- Around line 420-422: Update the skip-sweep branch in the wallet swap flow to
push the modified swap record into the swaps report before continuing,
preserving status = "skipped". Add a regression test covering a token in
skipSweep and assert that the exported swap report includes that skipped record.
---
Outside diff comments:
In `@src/cli/index.test.ts`:
- Around line 763-764: Update the mocked processNextRound() result in the
relevant test to include a concrete totalLength value, then explicitly assert
that ordersMetadata.roundProcessedOrderPairsCount is set to that value alongside
the existing telemetry assertion.
In `@src/cli/index.ts`:
- Around line 354-375: In the maintenance flow containing
retryPendingRemoveWorkers, convertHoldingsToGas, and the worker sweep loop, move
nextGasConversionTime and nextSweepTime updates until after all corresponding
awaited operations complete successfully. Ensure failures leave the relevant
timer unchanged so the work is retried, while preserving worker iteration
behavior and adding coverage for failed removal, conversion, and worker-sweep
operations.
In `@src/order/index.ts`:
- Around line 472-497: Normalize the orderbook, owner, sell-token, and buy-token
keys to lowercase in the lookups within the consumingOrders balance-update loop,
matching the keys written by addToTokenVaultsMap. Add a regression test covering
mixed-case addresses and verify cached balances are used so zero-output pairs
are partitioned correctly.
- Around line 635-641: Update the orderbook aggregation flow around
totalDistinctPairsCount and distinctPairsSet so distinct pair identities are
accumulated across all orderbooks instead of resetting or overwriting the count
per iteration. Ensure the metadata returned by the surrounding method reports
the combined distinct-pair count while preserving the existing totalCount,
totalOwnersCount, and totalPairsCount calculations.
- Around line 201-202: Update addOrder() to skip the getCurrentMetadata() call
when invoked during fetch()’s initial synchronization, while preserving metadata
refreshes for other order additions. Keep fetch()’s existing final
getCurrentMetadata() call so metadata is recalculated once after all orders are
loaded.
🪄 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 Plus
Run ID: 6115a4c6-bc44-47ec-b3cb-d7098fdcab1a
📒 Files selected for processing (10)
src/cli/index.test.tssrc/cli/index.tssrc/core/modes/simulator.test.tssrc/core/modes/simulator.tssrc/core/process/round.test.tssrc/core/process/round.tssrc/order/index.test.tssrc/order/index.tssrc/wallet/index.test.tssrc/wallet/index.ts
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref=""
for candidate in origin/master master; do
if git rev-parse --verify --quiet "$candidate" >/dev/null; then
base_ref="$candidate"
break
fi
done
if [ -z "$base_ref" ]; then
echo "Unable to locate master base ref." >&2
exit 1
fi
merge_base="$(git merge-base "$base_ref" HEAD)"
printf 'merge_base=%s\n' "$merge_base"
git diff --stat "$merge_base" HEAD
printf '\n--- numstat ---\n'
git diff --numstat "$merge_base" HEAD
printf '\n--- files ---\n'
git diff --name-only "$merge_base" HEADLength of output: 764 SIZE=M |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit