chore: Buy orders match cross price with sell orders#1335
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces exact-price direct matching with price-crossing logic: match_direct/selects best bid and ask, requires buy_price ≥ sell_price, executes trades at sell_price, issues buyer refunds for price improvement, updates collateral/payments/tx_impact and ob_positions, and adds price-crossing and sweep tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Buyer
participant OrderBook
participant Seller
participant Ledger
Buyer->>OrderBook: place buy order (price = buy_price)
Seller->>OrderBook: place sell order (price = sell_price)
OrderBook->>OrderBook: select best_bid, best_ask
alt buy_price ≥ sell_price
OrderBook->>Ledger: execute trade at sell_price (transfer collateral)
OrderBook->>Buyer: refund (buy_price - sell_price)
OrderBook->>Ledger: record tx_impact and P&L
OrderBook->>OrderBook: update/delete ob_positions for buyer/seller
else
OrderBook->>Buyer: no match (orders remain)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/migrations/032-order-book-actions.sql (1)
455-512:⚠️ Potential issue | 🟠 MajorPick the best crossed pair with distinct participants here.
match_direct()now selects best ask and best bid independently, but unlikematch_mint()andmatch_burn()it never excludessell_participant_id = buy_participant_id. With price-crossing enabled, that lets one trader self-match their own crossed orders and can skip a real fill behind them. Example: A has ask 55 and bid 60, B has ask 56. This code clears A vs A first, and B never gets the 60 bid. Please choose the best overlapping buyer/seller pair with differentparticipant_ids in the selection itself; a later guard would still leave the crossed book stuck.
🧹 Nitpick comments (1)
internal/migrations/032-order-book-actions.sql (1)
446-447: Consider replacing this recursive whole-book sweep with bounded iteration.Now that
match_direct()ignores$priceand keeps recursing until no overlap remains, one placement can walk every crossed pair across all price levels for that outcome. On a deep book that makes a single order responsible for a large recursive action and raises the risk of hitting action-depth / gas limits and rolling the placement back. A bounded loop or batched matcher would be safer here.Also applies to: 455-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/032-order-book-actions.sql` around lines 446 - 447, The recursive whole-book sweep in match_direct() can traverse many price levels and risk deep recursion/gas limits; refactor it to a bounded iterative/batched approach by replacing the recursive call with a loop that processes a limited number of matched pairs per invocation (e.g., batch_size N) and returns a cursor/state when the batch limit is hit so the caller can resume; ensure you keep the same matching semantics (processing overlaps until none remain) by looping until either no overlap or batch_size reached, update any callers of match_direct() to detect the resume state and re-invoke the matcher (or implement a batched matcher wrapper), and adjust related procedures that relied on the old recursion to handle partial completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/streams/order_book/matching_engine_test.go`:
- Around line 876-932: Change the test to prove price-priority (not FIFO) by
placing the three sell orders in an order that is not sorted by price (e.g.
create seller2 at 50, seller3 at 52, then seller1 at 48 using
callPlaceSplitLimitOrder / callPlaceSellOrder), then place the buy via
callPlaceBuyOrder and after getPositions also check the buyer's balance delta
equals 30*48 + 40*50 + 30*52 (to assert correct refunds and best-price
execution); keep existing checks for buyerYesHoldings and seller3Remaining to
verify holdings and remaining quantity.
---
Nitpick comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 446-447: The recursive whole-book sweep in match_direct() can
traverse many price levels and risk deep recursion/gas limits; refactor it to a
bounded iterative/batched approach by replacing the recursive call with a loop
that processes a limited number of matched pairs per invocation (e.g.,
batch_size N) and returns a cursor/state when the batch limit is hit so the
caller can resume; ensure you keep the same matching semantics (processing
overlaps until none remain) by looping until either no overlap or batch_size
reached, update any callers of match_direct() to detect the resume state and
re-invoke the matcher (or implement a batched matcher wrapper), and adjust
related procedures that relied on the old recursion to handle partial
completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 856a9149-396c-459d-b025-a04474e63d65
📒 Files selected for processing (2)
internal/migrations/032-order-book-actions.sqltests/streams/order_book/matching_engine_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/streams/order_book/matching_engine_test.go (1)
665-667:⚠️ Potential issue | 🟡 MinorSection header is inconsistent with test registration.
This header still says "Category E: Edge Cases" but the test registration at line 86-87 now places
testNoMatchingOrdersunder "Category F: Edge Cases". The new price-crossing tests are now Category E.📝 Suggested fix
// ============================================================================= -// Category E: Edge Cases +// Category F: Edge Cases // =============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/matching_engine_test.go` around lines 665 - 667, The section header string is wrong: update the header that currently reads "Category E: Edge Cases" to match the test registration which places testNoMatchingOrders under "Category F: Edge Cases"; locate the header near the Category comments (the block containing "Category E: Edge Cases") and change it to "Category F: Edge Cases" so it aligns with the registration for testNoMatchingOrders and the new price-crossing tests remain in Category E.
🧹 Nitpick comments (1)
tests/streams/order_book/matching_engine_test.go (1)
823-835: LGTM on test logic; minor comment inconsistency.The price-crossing verification is correctly implemented. The calculation
50 × 51 × 10^16matches the expected cost at sell price. However, the comments reference "TRUF" while the test actually checks USDC balance viagetUSDCBalance.📝 Suggested comment fix
// Verify buyer got price improvement refund - // Buyer locked: 50 × 52 × 10^16 = 26 × 10^18 (26 TRUF) - // Seller received: 50 × 51 × 10^16 = 25.5 × 10^18 (25.5 TRUF) - // Buyer refund: 50 × 1 × 10^16 = 0.5 × 10^18 (0.5 TRUF) - // Net cost to buyer: 25.5 TRUF (not 26 TRUF) + // Buyer locked: 50 × 52 × 10^16 = 26 × 10^18 (26 USDC) + // Seller received: 50 × 51 × 10^16 = 25.5 × 10^18 (25.5 USDC) + // Buyer refund: 50 × 1 × 10^16 = 0.5 × 10^18 (0.5 USDC) + // Net cost to buyer: 25.5 USDC (not 26 USDC)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/matching_engine_test.go` around lines 823 - 835, Update the test comments to reflect the correct currency being asserted (USDC) rather than "TRUF": in the block around buyerBalanceAfter and getUSDCBalance, change occurrences of "TRUF" in the explanatory lines (the locked/received/refund/net cost comments) to "USDC" or a neutral unit (e.g., "units") so they match the actual balance check using getUSDCBalance and the expectedCost calculation (expectedCost, balanceDecrease, buyerBalanceBefore, buyerBalanceAfter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/streams/order_book/matching_engine_test.go`:
- Around line 665-667: The section header string is wrong: update the header
that currently reads "Category E: Edge Cases" to match the test registration
which places testNoMatchingOrders under "Category F: Edge Cases"; locate the
header near the Category comments (the block containing "Category E: Edge
Cases") and change it to "Category F: Edge Cases" so it aligns with the
registration for testNoMatchingOrders and the new price-crossing tests remain in
Category E.
---
Nitpick comments:
In `@tests/streams/order_book/matching_engine_test.go`:
- Around line 823-835: Update the test comments to reflect the correct currency
being asserted (USDC) rather than "TRUF": in the block around buyerBalanceAfter
and getUSDCBalance, change occurrences of "TRUF" in the explanatory lines (the
locked/received/refund/net cost comments) to "USDC" or a neutral unit (e.g.,
"units") so they match the actual balance check using getUSDCBalance and the
expectedCost calculation (expectedCost, balanceDecrease, buyerBalanceBefore,
buyerBalanceAfter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05dda70c-e849-4c7f-8c8b-5636727223ae
📒 Files selected for processing (1)
tests/streams/order_book/matching_engine_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/streams/order_book/matching_engine_test.go (1)
665-667: Consider reordering test functions to match category sequence.The Category F section (line 666) appears before the Category E section (line 744) in the source file, even though Category E logically precedes Category F. For consistency with the test registration order (lines 82-87), consider moving the Category E test implementations before Category F.
Also applies to: 744-746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/matching_engine_test.go` around lines 665 - 667, The Category F block appears before Category E in tests/streams/order_book/matching_engine_test.go; reorder the code so the "Category E" test implementations (the functions under the Category E header) are placed before the "Category F" block to match the test registration order; move or cut/paste the entire Category E section (including its test functions and header comment) so it precedes the Category F section, ensuring any related helper functions or shared variables remain in scope and imports/fixtures are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/streams/order_book/matching_engine_test.go`:
- Around line 665-667: The Category F block appears before Category E in
tests/streams/order_book/matching_engine_test.go; reorder the code so the
"Category E" test implementations (the functions under the Category E header)
are placed before the "Category F" block to match the test registration order;
move or cut/paste the entire Category E section (including its test functions
and header comment) so it precedes the Category F section, ensuring any related
helper functions or shared variables remain in scope and imports/fixtures are
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a0f4a52-6757-4429-b384-113f87b3f738
📒 Files selected for processing (1)
tests/streams/order_book/matching_engine_test.go
1 participant
resolves: https://github.com/truflation/website/issues/3465
Summary by CodeRabbit
New Features
Docs
Tests