Conversation
Time Submission Status
|
WalkthroughReplaces a stubbed matching routine with a complete order-book matching engine (private SQL actions): implements orchestrated Changes
Sequence DiagramsequenceDiagram
participant User as User
participant API as Order API
participant SQL as SQL Actions
participant Engine as match_orders
participant Direct as match_direct
participant Mint as match_mint
participant Burn as match_burn
participant Ledger as Holdings/Balances
User->>API: place_buy_order(query_id, outcome, price, amount)
API->>SQL: place_buy_order action (lock collateral, record order)
SQL->>Engine: match_orders(query_id, outcome, price)
Engine->>Direct: try match_direct(complement_price)
Direct->>Ledger: fetch FIFO buy/sell at prices
Direct->>Ledger: transfer shares / adjust orders
Direct->>Engine: return (continue?)
Engine->>Mint: try match_mint(complement_price)
Mint->>Ledger: mint YES/NO pairs, adjust collateral & holdings
Mint->>Engine: return (continue?)
Engine->>Burn: try match_burn(complement_price)
Burn->>Ledger: burn opposite pairs, unlock collateral, adjust/delete orders
Burn->>Engine: return (done)
SQL->>API: return final order/position state
API->>User: report matched/unmatched results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (1)
🔇 Additional comments (4)
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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/streams/order_book/matching_engine_test.go (2)
23-27: Consider using test-scoped state instead of package-level variables.Package-level mutable state (
balancePointCounter,lastBalancePoint) can cause issues if tests are ever run in parallel or if the test harness changes. While each test currently resetslastBalancePoint = nil, thebalancePointCounteris never reset and will continuously increment across test runs.Consider encapsulating this state in a struct passed to each test:
-var ( - balancePointCounter int64 = 100 - lastBalancePoint *int64 -) +type balanceTracker struct { + counter int64 + lastPoint *int64 +} + +func newBalanceTracker() *balanceTracker { + return &balanceTracker{counter: 100} +}
74-79: Consider adding edge case tests for boundary conditions.The current test suite is comprehensive for core matching logic. Consider adding tests for:
- Self-match scenario: Verify that a user cannot match with themselves (if this is prevented by the system).
- Price boundaries: Test behavior at
price = 1andprice = 99.- Same participant on both sides of mint/burn: Edge case where complementary orders belong to the same user.
internal/migrations/032-order-book-actions.sql (1)
361-407: Consider preventing self-matching in direct and mint matches.Currently,
match_directandmatch_mintdon't check if the buyer and seller are the same participant. While this doesn't cause economic harm (user trades with themselves), it wastes gas and could indicate a bug in client logic.For
match_burn, self-matching is a valid use case (user cashing out their complete position).-- No sell order, exit if NOT $sell_found { RETURN; } + -- Skip self-match (user cannot trade with themselves) + if $buy_participant_id = $sell_participant_id { + RETURN; + } + -- Defensive: Skip if either order has zero or negative amount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/032-order-book-actions.sql(2 hunks)tests/streams/order_book/matching_engine_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/order_book/matching_engine_test.go (4)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-85)tests/streams/utils/runner.go (1)
RunSchemaTest(37-116)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(75-88)tests/streams/order_book/buy_order_test.go (1)
Position(399-406)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (10)
tests/streams/order_book/matching_engine_test.go (5)
30-54: Helper function logic looks correct.The chained deposit mechanism properly maintains the linked-list ordering for ordered-sync, updating
lastBalancePointonly on success.
56-81: Good test organization with comprehensive scenario coverage.The test suite covers all three match types (direct, mint, burn) with both full and partial fill scenarios, plus multi-round matching and edge cases. Clear category comments improve readability.
88-146: Direct match full fill test is well-structured.The test correctly verifies that after a direct match of equal amounts, the buyer receives the expected holdings. The position-finding logic properly uses
&positions[i]to avoid loop variable capture issues.
528-537: Partial burn payout calculations are correct.
- User1 receives 36 TRUF (60 shares × $0.60)
- User2 net change is -76 TRUF (locked 100, received 24 from burn at $0.40)
The assertions correctly verify the expected collateral movements for partial burn matches.
639-709: Good edge case test for non-matching orders.The test correctly verifies that orders at different prices ($0.56 buy vs $0.65 sell) remain unmatched. The workaround to prevent self-burn (cancel NO sell after split limit) is appropriately documented.
internal/migrations/032-order-book-actions.sql (5)
478-587: Mint match implementation is correct.The collateral flow for mint matching is sound:
- Both buyers already have collateral locked (summing to $1.00 per pair)
- New shares are created and backed by existing locked collateral
- No additional collateral movement is needed
The FIFO ordering and partial fill logic are correctly implemented.
695-706: Burn match collateral unlocking is correct.The payout calculations properly distribute collateral to both sellers based on their sell prices, with the total exactly matching the $1.00 per share pair that was originally locked.
759-823: Match orchestration logic is sound.The orchestrator correctly:
- Validates all inputs
- Tries direct match first (most common case)
- Attempts mint match with complementary pricing
- Attempts burn match with complementary pricing
The order of operations is logical - direct matches are fastest and most common.
1139-1154: Holdings management logic is correct.The two-step approach (DELETE for full sell, UPDATE for partial sell) correctly handles the constraint that prevents zero-amount positions. The mutually exclusive WHERE clauses ensure exactly one statement executes.
422-445: Trade execution does not record transaction events.Unlike
create_marketwhich records events viarecord_transaction_event(8), the order matching functions (match_direct,match_mint,match_burn) have no corresponding transaction method_id defined. This means trades lack audit trail coverage for analytics, compliance, and trade history features.Consider adding a transaction method_id for order matching to maintain consistent event recording across all user-facing actions.
1 participant
resolves: https://github.com/truflation/website/issues/2920
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.