Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds transaction-level P&L instrumentation and audit tables to the order-book, threads query IDs and per-payout outcomes through settlement/unlock/fee distribution, and updates actions and tests to record and materialize per-TX net impacts without changing public action signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrderBook as "OrderBook Actions (SQL)"
participant PNL as "P&L Ledger (ob_tx_payouts / ob_net_impacts)"
participant Bridge as "Bridge / Unlock"
rect rgba(200,200,255,0.5)
Client->>OrderBook: place/match/split/cancel/change order
OrderBook->>PNL: ob_record_tx_payout / ob_record_tx_impact (accumulate per-TX)
end
rect rgba(200,255,200,0.5)
OrderBook->>OrderBook: ob_cleanup_tx_payouts(query_id)
OrderBook->>PNL: ob_record_net_impact(...) (persist per-TX nets)
end
rect rgba(255,200,200,0.5)
OrderBook->>Bridge: ob_batch_unlock_collateral(query_id,...,outcomes)
Bridge-->>OrderBook: unlocked amounts
OrderBook->>PNL: record unlock-related net impacts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
tests/streams/order_book/pnl_impact_test.go (1)
99-111: Strengthen assertions for matched counterparty impacts.This test only validates the initiating side after matching. Add assertions for the passive/matched participant to prevent silent regressions in per-participant P&L accounting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/pnl_impact_test.go` around lines 99 - 111, The test currently only checks initiating sides; extend the assertions after calling getNetImpacts(ctx, platform, int(marketID)) to validate the matched/passive participant's impact as well by adding checks against the appropriate impacts slice entries (e.g., the counterparty index in impacts returned by getNetImpacts). Specifically, assert the passive participant's SharesChange and CollateralChange match expected values (mirror/opposite of initiator where applicable) using require.Equal on impacts[<counterpartyIndex>].SharesChange and impacts[<counterpartyIndex>].CollateralChange so per-participant P&L accounting cannot regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 535-537: The payout handling currently only materializes the
caller into ob_net_impacts and misses cleaning up counterparty payout rows
(causing stale ob_tx_payouts), and the buy flow never cleans up; modify the flow
that calls ob_record_tx_payout to (1) collect all participant IDs involved in
the transaction (caller, counterparty, any matched participants), (2) call or
extend ob_record_tx_payout for each participant so every touched participant is
materialized into ob_net_impacts before the cleanup step, and (3) ensure the
cleanup logic that deletes from ob_tx_payouts runs in both sell and buy flows;
update any helper/cleanup routines to iterate over the full participant set
rather than only the caller to prevent dangling ob_tx_payouts rows.
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 78-83: The code currently hardcodes outcome=TRUE when calling
ob_record_net_impact, which will mislabel NO-side and mixed refund cases;
replace the literal TRUE with the actual settlement outcome boolean from the
surrounding settlement context (e.g., the variable that represents whether this
position was on the YES side or the resolved outcome) and ensure
ob_record_net_impact($query_id, $pid, <actual_outcome_boolean>, 0::INT8,
$amount, FALSE) uses that computed value so NO-side or refund scenarios are
recorded correctly; reference ob_record_net_impact, $pid, $query_id, and $amount
to locate and update the call.
In `@tests/streams/order_book/pnl_impact_test.go`:
- Around line 239-247: callProcessSettlement currently returns only the
transport/runtime error from platform.Engine.Call and ignores action-level
failures contained in res.Error; update the helper (the code invoking
platform.Engine.Call in callProcessSettlement) to capture the response (res)
and, after checking the returned err, also check if res.Error is non-nil and
return that error (or wrap it) so action-level failures propagate to the
caller—mirror the pattern used in
callPlaceSplitLimitOrder/callPlaceSellOrder/callExecuteAction by returning
res.Error when present.
---
Nitpick comments:
In `@tests/streams/order_book/pnl_impact_test.go`:
- Around line 99-111: The test currently only checks initiating sides; extend
the assertions after calling getNetImpacts(ctx, platform, int(marketID)) to
validate the matched/passive participant's impact as well by adding checks
against the appropriate impacts slice entries (e.g., the counterparty index in
impacts returned by getNetImpacts). Specifically, assert the passive
participant's SharesChange and CollateralChange match expected values
(mirror/opposite of initiator where applicable) using require.Equal on
impacts[<counterpartyIndex>].SharesChange and
impacts[<counterpartyIndex>].CollateralChange so per-participant P&L accounting
cannot regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e75290e7-4041-48d7-a462-c9c7209a5f30
📒 Files selected for processing (8)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqlinternal/migrations/043-order-book-pnl.sqltests/streams/order_book/market_creation_test.gotests/streams/order_book/matching_engine_test.gotests/streams/order_book/pnl_impact_test.gotests/streams/order_book/queries_test.gotests/streams/order_book/test_helpers_orderbook.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/migrations/033-order-book-settlement.sql (1)
282-283:⚠️ Potential issue | 🟠 MajorLP fee impacts are still pinned to YES.
Line 283 still records every LP reward with
outcome = TRUE, so NO-resolved markets will write those payouts onto the wrong side. Thread the resolved outcome intodistribute_feesand use it here instead of hardcoding YES.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/033-order-book-settlement.sql` around lines 282 - 283, The LP reward call currently hardcodes outcome = TRUE in ob_batch_unlock_collateral, causing NO-resolved markets to record payouts on the wrong side; instead, propagate the resolved outcome into distribute_fees (add a parameter like resolved_outcome) and pass that variable into ob_batch_unlock_collateral (replace the literal TRUE with the resolved outcome variable) so fees are recorded for the actual resolved side; update function signatures and all call sites of distribute_fees and ob_batch_unlock_collateral accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 1198-1203: The cleanup hook ob_cleanup_tx_payouts($query_id) is
missing after code paths that call match_orders, so matched edits never
materialize into ob_net_impacts and committed ob_tx_payouts accumulate; add
calls to ob_cleanup_tx_payouts($query_id) immediately after each match_orders
invocation (specifically in the change_bid and change_ask functions and the
other edit paths noted) so the transaction payout rows are processed into
ob_net_impacts and cleaned up; search for match_orders within change_bid,
change_ask and the other edit handlers and insert
ob_cleanup_tx_payouts($query_id) in those branches right after match_orders
completes.
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 40-45: The helper currently collapses outcomes into a single
batch-level $outcome which misattributes refunds; update the helper used by
process_settlement so it preserves outcome at the aggregation level and creates
unlock batches keyed by (wallet, outcome) instead of per-wallet only: add an
outcome dimension to the helper's aggregation (keep $outcome per entry or accept
amounts grouped by outcome), change the unlock batch materialization to include
and stamp each batch with the corresponding outcome (i.e., produce one unlock
record per wallet+outcome), and adjust any calls from process_settlement to pass
through the per-outcome values so refunds and winner redemptions are recorded
under the correct outcome (refer to process_settlement, $outcome, and the
unlock/aggregation logic where batches are created).
---
Duplicate comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 282-283: The LP reward call currently hardcodes outcome = TRUE in
ob_batch_unlock_collateral, causing NO-resolved markets to record payouts on the
wrong side; instead, propagate the resolved outcome into distribute_fees (add a
parameter like resolved_outcome) and pass that variable into
ob_batch_unlock_collateral (replace the literal TRUE with the resolved outcome
variable) so fees are recorded for the actual resolved side; update function
signatures and all call sites of distribute_fees and ob_batch_unlock_collateral
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36b86880-a386-4d3e-9a56-eb0063bad542
📒 Files selected for processing (4)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqlinternal/migrations/043-order-book-pnl.sqltests/streams/order_book/pnl_impact_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
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)
2050-2073:⚠️ Potential issue | 🔴 CriticalLock transactions lack impact recording.
The
else if $collateral_delta > $zeroat line 2069 is unreachable—it's nested inside theelse if $collateral_delta < $zeroblock. When collateral_delta is positive, the first lock operation executes without recording the corresponding impact. The impact recording for unlocks (negative delta) exists, but the symmetric recording for locks is missing.Fix
if $collateral_delta > $zero { -- New order needs MORE collateral -- Lock additional amount (will ERROR if insufficient balance) if $bridge = 'hoodi_tt2' { hoodi_tt2.lock($collateral_delta); } else if $bridge = 'sepolia_bridge' { sepolia_bridge.lock($collateral_delta); } else if $bridge = 'ethereum_bridge' { ethereum_bridge.lock($collateral_delta); } + ob_record_tx_impact($participant_id, $outcome, 0::INT8, $collateral_delta, TRUE); } else if $collateral_delta < $zero { -- New order needs LESS collateral -- Unlock excess amount $unlock_amount NUMERIC(78, 0) := $zero - $collateral_delta; -- Make positive ob_unlock_collateral($bridge, `@caller`, $unlock_amount); -- Record initial impact (refund) ob_record_tx_impact($participant_id, $outcome, 0::INT8, $unlock_amount, FALSE); - } else if $collateral_delta > $zero { - -- Record initial impact (lock) - ob_record_tx_impact($participant_id, $outcome, 0::INT8, $collateral_delta, TRUE); - } - -- If $collateral_delta = 0, no collateral adjustment needed + } + -- If $collateral_delta = 0, no collateral adjustment needed🤖 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 2050 - 2073, The positive-collateral branch currently performs the lock (via hoodi_tt2.lock / sepolia_bridge.lock / ethereum_bridge.lock) but never calls ob_record_tx_impact because the impact-recording else-if is mistakenly nested inside the negative-collateral branch and therefore unreachable; move or add a call to ob_record_tx_impact($participant_id, $outcome, 0::INT8, $collateral_delta, TRUE) immediately after the lock logic in the if $collateral_delta > $zero branch, and remove the stray unreachable "else if $collateral_delta > $zero" block so that locks and unlocks both record impact symmetrically while leaving ob_unlock_collateral and the negative-collateral impact recording unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 1591-1597: The code currently records the entire collateral lock
on the TRUE leg via ob_record_tx_impact, which makes ob_cleanup_tx_payouts treat
the NO leg as having zero cost; instead compute and record a per-outcome
collateral share before recording the mints: compute collateral_for_outcome =
$collateral_needed * amount_for_outcome / (sum_of_amounts_for_both_legs) (or use
$collateral_needed/2 for equal splits), then call
ob_record_tx_impact($participant_id, TRUE, 0::INT8, collateral_for_true, TRUE)
and ob_record_tx_impact($participant_id, FALSE, 0::INT8, collateral_for_false,
TRUE) prior to the ob_record_tx_impact mints; keep casts/types consistent and
use the same identifiers ($collateral_needed, $amount) and functions
(ob_record_tx_impact, ob_cleanup_tx_payouts) referenced in the diff.
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 327-339: The action trigger_fee_distribution currently trusts the
passed-in $winning_outcome; instead query the market truth from
ob_queries.winning_outcome using the provided $query_id and use that value when
calling distribute_fees. Update trigger_fee_distribution so it either drops the
$winning_outcome parameter or ignores it, add a local variable (e.g., $winning
BOOL) populated by SELECT winning_outcome FROM ob_queries WHERE query_id =
$query_id LIMIT 1, and then pass that $winning value into
distribute_fees($query_id, $fees, $winning). Ensure the role check and fee
parsing remain unchanged.
- Around line 296-304: The three ARRAY_AGG calls in the CTE named aggregated are
non-deterministic and must use the same ORDER BY to preserve positional
alignment when later UNNESTing; update each ARRAY_AGG(wallet),
ARRAY_AGG(amount::NUMERIC(78, 0)), and ARRAY_AGG(outcome) to include an
identical deterministic ORDER BY clause based on a stable key available in
calculated_payouts (for example wallet, or a payout_id/timestamp/composite like
(wallet, outcome) if available) so wallets, amounts and outcomes remain
correctly zipped.
---
Outside diff comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 2050-2073: The positive-collateral branch currently performs the
lock (via hoodi_tt2.lock / sepolia_bridge.lock / ethereum_bridge.lock) but never
calls ob_record_tx_impact because the impact-recording else-if is mistakenly
nested inside the negative-collateral branch and therefore unreachable; move or
add a call to ob_record_tx_impact($participant_id, $outcome, 0::INT8,
$collateral_delta, TRUE) immediately after the lock logic in the if
$collateral_delta > $zero branch, and remove the stray unreachable "else if
$collateral_delta > $zero" block so that locks and unlocks both record impact
symmetrically while leaving ob_unlock_collateral and the negative-collateral
impact recording unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec30c999-9008-4da5-ae31-2b3e4466d942
📒 Files selected for processing (3)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqltests/streams/order_book/pnl_impact_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/streams/order_book/pnl_impact_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/migrations/033-order-book-settlement.sql (1)
337-350:⚠️ Potential issue | 🟠 MajorRequire a settled market before triggering fee distribution.
An existing-but-unsettled market sets
$found = TRUEwhile leaving$winningunset, so this path can calldistribute_feesbefore settlement has produced a valid outcome. Please gate the lookup onsettled = TRUE(or explicitly rejectNULLoutcomes) before any funds are distributed.Suggested fix
- for $row in SELECT winning_outcome FROM ob_queries WHERE id = $query_id { + for $row in SELECT winning_outcome + FROM ob_queries + WHERE id = $query_id + AND settled = TRUE { $winning := $row.winning_outcome; $found := TRUE; } - if NOT $found { - ERROR('Market not found: ' || $query_id::TEXT); + if NOT $found OR $winning IS NULL { + ERROR('Market must be settled before fee distribution'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/033-order-book-settlement.sql` around lines 337 - 350, The current loop sets $found = TRUE for an existing ob_queries row even if the market isn't settled, allowing distribute_fees($query_id, $fees, $winning) to be called with NULL/invalid $winning; update the lookup to only consider settled markets (e.g., SELECT winning_outcome FROM ob_queries WHERE id = $query_id AND settled = TRUE) or explicitly check that $winning IS NOT NULL after the select and raise an ERROR if not settled, and only call distribute_fees when a valid $winning value is present; references: ob_queries, $query_id, $winning, $found, distribute_fees.
🧹 Nitpick comments (1)
tests/streams/order_book/pnl_impact_test.go (1)
227-245: The “validator reward” assertion is ambiguous.Line 185 makes
userAthe data provider, and this check accepts any0.025TRUF impact as the validator row. That means the test can still pass on the DP reward even if validator-impact recording regresses. Please assert the recipient/participant for this row, or rename the expectation to an infra-reward check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/pnl_impact_test.go` around lines 227 - 245, The validator reward assertion is ambiguous because the loop that finds validatorReward by CollateralChange == "25000000000000000" can match the data provider's 0.025 TRUF; update the test in tests/streams/order_book/pnl_impact_test.go to either (A) additionally assert the recipient/participant on the found NetImpact (e.g., check validatorReward.Participant or validatorReward.Recipient equals the expected validator ID like userA or validatorID) so the impact is validated as coming from the validator, or (B) rename validatorReward to infraReward and change the assertion to reflect infra-reward semantics; locate the variables impacts, settlementPayout, validatorReward and the NetImpact struct usage to add the extra participant/recipient check or rename the variable and adjust the message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 70-74: The new distribute_fees action now requires a
$winning_outcome parameter, so update all callers to pass the outcome (or
replace calls with trigger_fee_distribution where appropriate); locate
invocations of distribute_fees (e.g., in the fee distribution test helper and
any helpers that call distribute_fees) and either thread the boolean outcome
through their call chain to provide the new $winning_outcome argument or change
the call site to trigger_fee_distribution if that better matches the intent,
ensuring signatures match and tests/helpers compile and run without arity
errors.
---
Duplicate comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 337-350: The current loop sets $found = TRUE for an existing
ob_queries row even if the market isn't settled, allowing
distribute_fees($query_id, $fees, $winning) to be called with NULL/invalid
$winning; update the lookup to only consider settled markets (e.g., SELECT
winning_outcome FROM ob_queries WHERE id = $query_id AND settled = TRUE) or
explicitly check that $winning IS NOT NULL after the select and raise an ERROR
if not settled, and only call distribute_fees when a valid $winning value is
present; references: ob_queries, $query_id, $winning, $found, distribute_fees.
---
Nitpick comments:
In `@tests/streams/order_book/pnl_impact_test.go`:
- Around line 227-245: The validator reward assertion is ambiguous because the
loop that finds validatorReward by CollateralChange == "25000000000000000" can
match the data provider's 0.025 TRUF; update the test in
tests/streams/order_book/pnl_impact_test.go to either (A) additionally assert
the recipient/participant on the found NetImpact (e.g., check
validatorReward.Participant or validatorReward.Recipient equals the expected
validator ID like userA or validatorID) so the impact is validated as coming
from the validator, or (B) rename validatorReward to infraReward and change the
assertion to reflect infra-reward semantics; locate the variables impacts,
settlementPayout, validatorReward and the NetImpact struct usage to add the
extra participant/recipient check or rename the variable and adjust the message
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85b5a547-1196-4710-8e08-3c2a8616d7f9
📒 Files selected for processing (3)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqltests/streams/order_book/pnl_impact_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/pnl_impact_test.go`:
- Around line 208-249: The test undercounts impacts and only finds one 0.025
TRUF reward: update the assertion for impacts length from require.Len(t,
impacts, 4) to require.Len(t, impacts, 5) and modify the search loop (iterating
over impacts) to capture two separate reward impacts (e.g., add a dpReward
variable in addition to validatorReward and settlementPayout) instead of
matching rewards solely by amount; for each imp where imp.SharesChange==0 &&
!imp.IsNegative && imp.ParticipantID==userAPID, assign the impact to
settlementPayout if imp.CollateralChange == toWei("9.8"), otherwise if
imp.CollateralChange == "25000000000000000" fill dpReward if nil else fill
validatorReward, then assert both dpReward and validatorReward are non-nil and
have Outcome==true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f485e20-b7fe-42ea-99dc-068aecf31003
📒 Files selected for processing (3)
tests/streams/order_book/fee_distribution_audit_test.gotests/streams/order_book/fee_distribution_test.gotests/streams/order_book/pnl_impact_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/truflation/website/issues/3410
Summary by CodeRabbit
New Features
Tests
Chores