Conversation
WalkthroughAdds liquidity provider eligibility tracking by enhancing Changes
Sequence DiagramsequenceDiagram
participant Client
participant place_split_limit_order as place_split_limit_order<br/>(Order Action)
participant ob_queries as ob_queries<br/>(Market Params)
participant ob_liquidity_providers as ob_liquidity_providers<br/>(LP Table)
participant get_lp_stats as get_lp_stats<br/>(Query View)
Client->>place_split_limit_order: Place split limit order<br/>(participant, split_amount, prices)
activate place_split_limit_order
place_split_limit_order->>ob_queries: Fetch market parameters<br/>(max_spread, min_order_size)
activate ob_queries
ob_queries-->>place_split_limit_order: Return parameters or NULL
deactivate ob_queries
alt Parameters Retrieved
place_split_limit_order->>place_split_limit_order: Compute price distances<br/>from 50¢ midpoint<br/>(YES & NO sides)
place_split_limit_order->>place_split_limit_order: Evaluate qualification:<br/>buy_distance ≤ max_spread AND<br/>sell_distance ≤ max_spread AND<br/>amount ≥ min_order_size
alt Qualified
place_split_limit_order->>ob_liquidity_providers: Upsert LP record<br/>(snapshot metadata)
activate ob_liquidity_providers
ob_liquidity_providers-->>place_split_limit_order: Record stored/updated
deactivate ob_liquidity_providers
rect rgba(76, 175, 80, 0.2)
note over place_split_limit_order: LP tracked for<br/>fee distribution
end
else Not Qualified
rect rgba(244, 67, 54, 0.2)
note over place_split_limit_order: LP not tracked
end
end
else Parameters Missing
rect rgba(244, 67, 54, 0.2)
note over place_split_limit_order: Error: Missing params
end
end
deactivate place_split_limit_order
Client->>get_lp_stats: Query LP statistics<br/>for market (query_id)
activate get_lp_stats
get_lp_stats->>ob_liquidity_providers: SELECT LP records<br/>joined with participants
activate ob_liquidity_providers
ob_liquidity_providers-->>get_lp_stats: Return LP details
deactivate ob_liquidity_providers
get_lp_stats-->>Client: Return sorted LP stats<br/>(by split_order_amount DESC)
deactivate get_lp_stats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Time Submission Status
|
![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: 0
🧹 Nitpick comments (7)
internal/migrations/034-lp-tracking.sql (1)
94-130: Verify the view action handles non-existent query_id gracefully.The
get_lp_statsaction returns an empty result set when no LPs exist for a market, which is correct behavior. However, consider whether it should also validate that the market exists (query_id is valid) and provide a more descriptive response if the market doesn't exist at all.Currently, querying a non-existent market returns the same empty result as a market with no LPs - this may be acceptable but could mask issues.
internal/migrations/032-order-book-actions.sql (3)
1452-1454: Fix duplicate section numbering.This section is labeled "SECTION 8: LP ELIGIBILITY TRACKING" but there's already a "SECTION 8: TRIGGER MATCHING ENGINE" at line 1438. This should be "SECTION 9" for consistency.
-- ========================================================================== - -- SECTION 8: LP ELIGIBILITY TRACKING (ISSUE 9A) + -- SECTION 9: LP ELIGIBILITY TRACKING (ISSUE 9A) -- ==========================================================================
1480-1490: Distance calculations are equivalent when prices sum to 100.Since
$false_price = 100 - $true_price, bothabs(50 - $true_price)andabs(50 - $false_price)yield the same value. For example:
true_price = 52, false_price = 48→abs(50-52) = 2, abs(50-48) = 2true_price = 60, false_price = 40→abs(50-60) = 10, abs(50-40) = 10The code is correct but could be simplified to check only one distance. The current implementation is fine for clarity and documentation purposes.
1466-1478: Consider combining market queries for efficiency.The market parameters (
max_spread,min_order_size) are fetched in a separate query (lines 1470-1473), whilesettledwas already queried fromob_queriesin Section 1.3 (lines 1323-1326). For minor efficiency gains, consider fetching all needed fields in a single query earlier in the action.This is a low-priority optimization since the database hit is minimal.
tests/streams/order_book/lp_tracking_test.go (3)
474-484: Consider adding nil checks for type assertions.The type assertions (e.g.,
row.Values[0].(int64)) will panic if any value isnil. While the database schema defines all columns asNOT NULL, adding defensive nil checks would make the test helper more robust:if row.Values[0] == nil { return fmt.Errorf("unexpected nil value for participant_id") }This is optional since the schema constraints should prevent nil values.
52-54: Consider extracting test state reset to a helper function.The pattern of resetting
balancePointCounterandlastBalancePointis repeated at the start of every test function. Consider extracting this to a helper:func resetTestState() { balancePointCounter = 100 lastBalancePoint = nil }Then call
resetTestState()at the start of each test. This reduces duplication and makes the intent clearer.Also applies to: 93-94, 125-127, 159-160, 198-200, 260-262, 312-314, 341-343, 382-384
447-456: Consider usingint32for price fields to match SQL INT type more precisely.The
LastOrderTruePriceandLastOrderFalsePricefields are defined asint16, but prices in the SQL schema areINT(typically 32-bit). While the value range (1-99) easily fits inint16, usingint32would be more consistent with the database type.This is a minor nit and the current implementation works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/migrations/032-order-book-actions.sql(1 hunks)internal/migrations/034-lp-tracking.sql(1 hunks)tests/streams/order_book/lp_tracking_test.go(1 hunks)
⏰ 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). (2)
- GitHub Check: acceptance-test
- GitHub Check: lint
🔇 Additional comments (3)
internal/migrations/034-lp-tracking.sql (1)
41-53: LGTM! Well-designed LP tracking schema.The table design correctly uses a composite primary key for one-record-per-LP-per-market semantics. The
ON DELETE CASCADEforquery_idensures cleanup when markets are deleted, while the absence of cascade onparticipant_idprevents orphan LP records if participants are somehow removed.internal/migrations/032-order-book-actions.sql (1)
1494-1520: LGTM! Correct UPSERT pattern for LP tracking.The UPSERT correctly:
- Inserts new LP records with initial values
- Accumulates
split_order_amounton conflict- Preserves
first_qualified_at(not updated on conflict)- Updates
last_qualified_atandlast_order_*_priceto latest values- Increments
total_qualified_orderscountertests/streams/order_book/lp_tracking_test.go (1)
21-42: LGTM! Comprehensive test coverage for LP tracking.The test suite covers all critical scenarios:
- Qualification within spread (Category A)
- Rejection when spread too wide or amount too small
- Boundary conditions (exact max_spread)
- Volume accumulation across orders (Category B)
- Per-market isolation
- Empty, single, and multiple LP stats queries (Category C)
1 participant
resolves: https://github.com/truflation/website/issues/2923
It is part of the bigger tasks. More PR will be needed for this issue
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.