Conversation
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User
participant Engine
participant place_sell_order as place_sell_order Action
participant DB
participant Matcher as Matching Engine
User->>Engine: Call place_sell_order(query_id, outcome, price, amount)
rect rgb(200, 220, 240)
note over place_sell_order: Validation Phase
Engine->>place_sell_order: Invoke with signed context
place_sell_order->>place_sell_order: Validate 0x-prefixed caller
place_sell_order->>place_sell_order: Check params (price ∈ [1,99], amount > 0)
end
rect rgb(220, 240, 200)
note over place_sell_order: Market & Holdings Verification
place_sell_order->>DB: Query market existence & settlement
DB-->>place_sell_order: Market data
place_sell_order->>DB: Query participant & holdings at price=0
DB-->>place_sell_order: Holdings data
place_sell_order->>place_sell_order: Verify sufficient holdings
end
rect rgb(240, 220, 200)
note over place_sell_order: State Transition
place_sell_order->>DB: Remove/update holdings (price=0)
place_sell_order->>DB: Upsert sell order (positive price)
DB-->>place_sell_order: Confirmed
end
place_sell_order->>Matcher: Trigger match_orders
Matcher-->>place_sell_order: Matching initiated
place_sell_order-->>Engine: Success
Engine-->>User: Sell order placed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 (2)
internal/migrations/032-order-book-actions.sql (1)
657-663: Consider adding defensive verification that holdings were actually reduced.While the sequential transaction model in Kwil ensures the earlier holdings check remains valid, adding a check that the UPDATE affected exactly 1 row would provide an extra layer of safety against unexpected data inconsistencies.
Example approach:
-- After the UPDATE, verify it succeeded $rows_updated INT; -- Note: Kuneiform may not support ROW_COUNT, check documentationBased on learnings, Kwil processes transactions sequentially within a block, so race conditions are not possible. However, defensive programming is still valuable for detecting unexpected states.
tests/streams/order_book/sell_order_test.go (1)
573-636: Consider simplifyinggiveSharesto avoid collateral locking issues.The current implementation creates a buy order (which locks collateral) and then converts it to holdings. However, this locked collateral is never released, which could cause balance issues when multiple tests use
giveShares.A simpler and more efficient approach would be to directly INSERT holdings using admin privileges:
func giveShares(ctx context.Context, platform *kwilTesting.Platform, signer *util.EthereumAddress, marketID int, outcome bool, amount int64) error { callerBytes := signer.Bytes() // Step 1: Get or create participant tx := &common.TxContext{ Ctx: ctx, BlockContext: &common.BlockContext{Height: 1, Timestamp: time.Now().Unix()}, Signer: platform.Deployer, Caller: "0x0000000000000000000000000000000000000000", TxID: platform.Txid(), } engineCtx := &common.EngineContext{TxContext: tx, OverrideAuthz: true} var participantID int64 err := platform.Engine.Execute(engineCtx, platform.DB, `INSERT INTO ob_participants (id, wallet_address) SELECT COALESCE(MAX(id), 0) + 1, $addr FROM ob_participants ON CONFLICT (wallet_address) DO NOTHING`, map[string]any{"$addr": callerBytes}, nil) if err != nil { return fmt.Errorf("failed to create participant: %w", err) } // Get participant ID err = platform.Engine.Execute(engineCtx, platform.DB, `SELECT id FROM ob_participants WHERE wallet_address = $addr`, map[string]any{"$addr": callerBytes}, func(row *common.Row) error { participantID = row.Values[0].(int64) return nil }) if err != nil { return fmt.Errorf("failed to get participant ID: %w", err) } // Step 2: Directly insert holdings (price = 0) return platform.Engine.Execute(engineCtx, platform.DB, `INSERT INTO ob_positions (query_id, participant_id, outcome, price, amount, last_updated) VALUES ($qid, $pid, $outcome, 0, $amount, $timestamp) ON CONFLICT (query_id, participant_id, outcome, price) DO UPDATE SET amount = ob_positions.amount + EXCLUDED.amount`, map[string]any{ "$qid": marketID, "$pid": participantID, "$outcome": outcome, "$amount": amount, "$timestamp": time.Now().Unix(), }, nil) }This approach:
- Avoids locking collateral unnecessarily
- Is simpler and more direct
- Prevents potential balance exhaustion in test suites
- More clearly expresses intent (test setup, not real trading flow)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/032-order-book-actions.sql(1 hunks)tests/streams/order_book/sell_order_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/order_book/sell_order_test.go (1)
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). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (8)
internal/migrations/032-order-book-actions.sql (5)
557-608: LGTM! Validation logic is thorough and consistent.The validation section correctly checks:
- Caller address format (0x-prefixed Ethereum address)
- Parameter constraints (price 1-99, amount positive and within limits)
- Market existence and settlement status
The validation order is efficient (cheap checks before expensive DB lookups).
610-626: LGTM! Correctly prevents auto-creation of participants for sell orders.Unlike buy orders, sell orders require an existing participant with holdings. The explicit check and clear error message ensure users understand they must own shares before selling.
628-651: LGTM! Holdings verification is correct and user-friendly.The logic correctly:
- Queries for holdings at price = 0 (per the price convention)
- Distinguishes between no holdings and insufficient holdings
- Provides informative error messages with actual vs. required amounts
674-686: LGTM! UPSERT logic correctly accumulates sell orders at the same price.The implementation:
- Uses positive price for sell orders (consistent with the price convention)
- Accumulates amounts when multiple orders are placed at the same price
- Updates timestamp to maintain FIFO ordering within price levels
688-696: LGTM! Matching engine stub is properly integrated.The function correctly triggers the matching engine after placing the sell order. The stub implementation is documented and aligns with the overall order book architecture.
tests/streams/order_book/sell_order_test.go (3)
22-46: LGTM! Test suite structure is well-organized with clear documentation.The test suite:
- Properly uses the
kwiltestbuild tag- Clearly marks tests blocked by Issue 6 (matching engine) with TODO comments
- Focuses on validation paths that can be tested without the matching engine
- Uses appropriate test utilities and schema setup
48-537: LGTM! Test functions provide comprehensive coverage of validation paths.The test suite intelligently:
- Separates tests that can run now (validation/error paths) from those blocked by Issue 6
- Tests all validation constraints (price range, amount limits, market state)
- Includes edge cases (no shares, wrong outcome, settled market)
- Uses clear assertions with helpful error messages
The blocked tests (success paths, UPSERT behavior) are well-structured and ready to be enabled when the matching engine is implemented.
541-571: LGTM! Helper function follows standard Kwil testing patterns.The
callPlaceSellOrderhelper:
- Correctly constructs a signed transaction context
- Uses appropriate authenticator (EthPersonalSignAuth)
- Properly handles both engine errors and result errors
- Follows the same pattern as buy order tests
1 participant
resolves: https://github.com/truflation/website/issues/2917
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.