feat: record order events for wallet history#1343
Merged
Merged
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds comprehensive order event recording to the order-book system. It creates a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 3
🧹 Nitpick comments (1)
tests/streams/order_book/order_events_test.go (1)
37-43: Please add coverage for the remaining new event emitters.This suite never exercises
settle_market()/process_settlement(),match_mint(),match_burn(),change_bid(), orchange_ask(). Those are all new emitters in this PR, and the settlement path in particular is easy to regress without a dedicated test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/order_events_test.go` around lines 37 - 43, The test suite is missing coverage for the new event emitters; add tests that exercise settle_market()/process_settlement(), match_mint(), match_burn(), change_bid(), and change_ask() by creating new TestFunc entries (e.g. testOrderEventSettleMarket(t), testOrderEventMatchMint(t), testOrderEventMatchBurn(t), testOrderEventChangeBid(t), testOrderEventChangeAsk(t)) following the existing test patterns used for buy/sell/split/cancel/direct-fill: invoke the API/engine actions that trigger each emitter, wait for the emitted event, and assert the expected event payloads so the settlement and match paths are covered. Ensure the new test functions are appended to the FunctionTests slice alongside testOrderEventBuyPlaced/testOrderEventSellPlaced etc., and reuse existing helpers for setup/teardown and event assertions.
🤖 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 294-301: The settled-event logic redeems positions for price >= 0
using a hardcoded 98¢ multiplier but still persists abs(raw_price) into
ob_order_events.price, causing winning holdings to be recorded as 0 or stale
limit prices; update the SELECT that builds settled events so that for the price
>= 0 branch you compute and persist the actual settlement/redemption price (the
post-fee per-share redemption used to compute pay) into the price column (and
use its absolute value where code expects abs(raw_price)), and ensure the
out/fee/pay expressions reference the same computed settlement_price
variable/expression (rather than raw_price) so ob_order_events.price reflects
the true settlement price for winning outcomes.
- Around line 290-301: The settlement-event SELECT currently only emits
winners/open buys; include losing/resting positions too by removing any
winner-only filter and ensuring the CASE expressions set pay, fee, and out
correctly for losers: keep the existing CASEs on price (use price<0 path to
produce pay as 0 or appropriate losing payout calculation), set fee to 0 for
losing positions, and set out to outcome (not $winning_outcome) for losers;
update the SELECT that produces pay, out, fee (fields named pay, out, fee) while
still returning participant_id, wallet_address, raw_amount and raw_price so
wallets for the losing side receive terminal settlement events. Ensure
references to price, amount, $winning_outcome, participant_id and wallet_address
are preserved when expanding the row set.
In `@internal/migrations/044-order-book-events.sql`:
- Around line 15-30: The ob_order_events table's foreign keys currently use ON
DELETE CASCADE which causes events to be removed when ob_queries or
ob_participants rows are deleted; change the FK constraints on query_id and
participant_id in ob_order_events to remove ON DELETE CASCADE (leave plain
FOREIGN KEY (...) REFERENCES ... or use ON DELETE NO ACTION/RESTRICT) so parent
cleanup cannot cascade-delete unsynced history (keep the ob_order_events table,
its columns, and the FK names but drop the cascading behavior relied upon by
trim_order_events()).
---
Nitpick comments:
In `@tests/streams/order_book/order_events_test.go`:
- Around line 37-43: The test suite is missing coverage for the new event
emitters; add tests that exercise settle_market()/process_settlement(),
match_mint(), match_burn(), change_bid(), and change_ask() by creating new
TestFunc entries (e.g. testOrderEventSettleMarket(t),
testOrderEventMatchMint(t), testOrderEventMatchBurn(t),
testOrderEventChangeBid(t), testOrderEventChangeAsk(t)) following the existing
test patterns used for buy/sell/split/cancel/direct-fill: invoke the API/engine
actions that trigger each emitter, wait for the emitted event, and assert the
expected event payloads so the settlement and match paths are covered. Ensure
the new test functions are appended to the FunctionTests slice alongside
testOrderEventBuyPlaced/testOrderEventSellPlaced etc., and reuse existing
helpers for setup/teardown and event assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56eda294-8c9e-4ebb-8a32-ab61504715ad
📒 Files selected for processing (4)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqlinternal/migrations/044-order-book-events.sqltests/streams/order_book/order_events_test.go
MicBun
force-pushed
the
walletHistorical
branch
from
977b8b3 to
ccbc5e0
Compare
MicBun
force-pushed
the
walletHistorical
branch
from
ccbc5e0 to
1f64117
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/tn_digest/internal/engine_ops.go`:
- Around line 520-543: parseTrimResultFromTxLog currently ignores fmt.Sscanf's
return and silently returns zeroed TrimOrderEventsTxResult on parse failure;
change parseTrimResultFromTxLog to return (*TrimOrderEventsTxResult, error),
validate fmt.Sscanf by checking the scanned count and error (expecting 3
fields), and return a descriptive error if parsing fails (include the offending
line or full log). Update the caller that does result :=
parseTrimResultFromTxLog(txResult.Log) (and the subsequent e.logger.Info/return)
to handle the error path: log the parse failure and return the error instead of
treating a zeroed result as success (e.g., in BroadcastTrimOrderEvents or the
function invoking parseTrimResultFromTxLog).
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 285-309: The loop over ob_positions that emits settlement events
is non-deterministic because the SELECT used by the for $evt_row has no ORDER
BY; modify the SELECT in the for loop that reads from ob_positions (the query
feeding $evt_row in the settlement block) to include a deterministic ORDER BY
(e.g., ORDER BY participant_id, price, amount, outcome) so
ob_record_order_event($query_id, $evt_pid, 'settled', ...) is called in a
stable, repeatable sequence.
In `@internal/migrations/044-order-book-events.sql`:
- Around line 15-16: The ob_order_events.id must be monotonic across trims:
change the column from INT to BIGINT and back it with a dedicated sequence (use
a BIGSERIAL-like sequence owned by ob_order_events.id) so inserts use
nextval(...) instead of recomputing the next id from table contents; remove or
replace the logic that recomputes the next id from current rows (the code
referenced around the recompute block) and ensure the sequence's last_value is
advanced if you seed events so the sequence never hands out reused small values
after a trim.
In `@tests/streams/order_book/order_events_test.go`:
- Around line 693-706: The test collects settlement events from the slice
settled and currently only asserts foundWinnerAt98; update the assertions to
also assert that foundLoserAt0 is true so losing NO-position settlements are
verified. Specifically, in the test that declares vars foundWinnerAt98 and
foundLoserAt0 and iterates over settled, add a require.True(t, foundLoserAt0,
"should have losing settlement event with price=0") (or similar message)
alongside the existing require for foundWinnerAt98 so both winning (price==98)
and losing (price==0) settlement paths are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d100e10c-c5fd-47e8-9595-bd7990e6deac
📒 Files selected for processing (6)
extensions/tn_digest/internal/engine_ops.goextensions/tn_digest/scheduler/constants.goextensions/tn_digest/scheduler/scheduler.gointernal/migrations/033-order-book-settlement.sqlinternal/migrations/044-order-book-events.sqltests/streams/order_book/order_events_test.go
✅ Files skipped from review due to trivial changes (1)
- extensions/tn_digest/scheduler/constants.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/migrations/044-order-book-events.sql (1)
16-16:⚠️ Potential issue | 🔴 CriticalMake
ob_order_events.idmonotonic across trims.
trim_order_events()can eventually empty this table, and Lines 50-63 then restart ids from1viaMAX(id)+1. Any indexer cursor keyed by last-seenidwill miss fresh events after the first full trim.INTalso leaves limited headroom for a long-lived event stream. Please backidwith a monotonic bigint sequence/identity instead of deriving it from current contents.Also applies to: 50-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/044-order-book-events.sql` at line 16, The ob_order_events table currently defines id as INT and the code in trim_order_events() (and the insert path using MAX(id)+1) derives new ids from table contents; change id to a monotonic BIGINT identity/sequence instead: alter the table to use BIGINT for id and set a DEFAULT nextval(...) from a dedicated sequence (or use GENERATED ALWAYS AS IDENTITY), update any INSERTs to rely on the sequence (remove MAX(id)+1 usage), and ensure trim_order_events() resets or advances the sequence safely after trimming (set the sequence's nextval to greater than the current max id if needed) so ids remain monotonic across trims.tests/streams/order_book/order_events_test.go (1)
693-706:⚠️ Potential issue | 🟡 MinorAssert the other settlement branches this fixture already creates.
This setup leaves both a losing position (
price = 0) and an unmatched YES bid (price = 30) at settlement, but the test only enforces the winner-at-98 path. A regression in either non-winning branch would still pass.🛠 Proposed fix
// Check that winning holdings have price=98 (redemption price) var foundWinnerAt98 bool var foundLoserAt0 bool + var foundOpenBuyAt30 bool for _, evt := range settled { if evt.Outcome == true && evt.Price == 98 { foundWinnerAt98 = true } - if evt.Price == 0 { + if evt.Outcome == false && evt.Price == 0 { foundLoserAt0 = true } + if evt.Outcome == true && evt.Price == 30 { + foundOpenBuyAt30 = true + } } require.True(t, foundWinnerAt98, "should have winning settlement event with price=98") - // Losers may or may not exist depending on position state after matching - _ = foundLoserAt0 + require.True(t, foundLoserAt0, "should have losing settlement event with price=0") + require.True(t, foundOpenBuyAt30, "should have unmatched bid settlement event with price=30")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/order_events_test.go` around lines 693 - 706, The test currently only asserts a winning settlement event at price 98; update it to also assert the other expected settlement branches by scanning the settled events for a loser with Price == 0 and for an unmatched YES bid with Price == 30 (add a new boolean like foundUnmatchedYes30), set them true when encountered, and add require.True checks for both foundLoserAt0 and foundUnmatchedYes30 so the test fails if either the losing settlement or the unmatched YES bid branch is missing; use the existing loop over settled and the same pattern used for foundWinnerAt98 to locate these events.internal/migrations/033-order-book-settlement.sql (1)
285-289:⚠️ Potential issue | 🔴 CriticalAdd a deterministic ordering to the settlement-event scan.
ob_record_order_event()assigns ids in loop order. Without anORDER BYhere, the sameob_positionsset can produce differentsettledid sequences across executions, which is risky for consensus and for downstream sync that readsob_order_eventssequentially.🛠 Proposed fix
for $evt_row in SELECT pos.participant_id, pos.price, pos.amount, pos.outcome FROM ob_positions pos WHERE pos.query_id = $query_id + ORDER BY pos.participant_id ASC, pos.outcome ASC, pos.price ASC {🤖 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 285 - 289, The SELECT over ob_positions used to drive the settlement loop is unordered, causing non-deterministic id assignment by ob_record_order_event(); fix by adding a deterministic ORDER BY to that query (e.g., ORDER BY participant_id, price, amount, outcome or by a stable primary key if present) so the loop processes rows in a stable sequence and produced settled ids are reproducible across runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 285-289: The SELECT over ob_positions used to drive the settlement
loop is unordered, causing non-deterministic id assignment by
ob_record_order_event(); fix by adding a deterministic ORDER BY to that query
(e.g., ORDER BY participant_id, price, amount, outcome or by a stable primary
key if present) so the loop processes rows in a stable sequence and produced
settled ids are reproducible across runs.
In `@internal/migrations/044-order-book-events.sql`:
- Line 16: The ob_order_events table currently defines id as INT and the code in
trim_order_events() (and the insert path using MAX(id)+1) derives new ids from
table contents; change id to a monotonic BIGINT identity/sequence instead: alter
the table to use BIGINT for id and set a DEFAULT nextval(...) from a dedicated
sequence (or use GENERATED ALWAYS AS IDENTITY), update any INSERTs to rely on
the sequence (remove MAX(id)+1 usage), and ensure trim_order_events() resets or
advances the sequence safely after trimming (set the sequence's nextval to
greater than the current max id if needed) so ids remain monotonic across trims.
In `@tests/streams/order_book/order_events_test.go`:
- Around line 693-706: The test currently only asserts a winning settlement
event at price 98; update it to also assert the other expected settlement
branches by scanning the settled events for a loser with Price == 0 and for an
unmatched YES bid with Price == 30 (add a new boolean like foundUnmatchedYes30),
set them true when encountered, and add require.True checks for both
foundLoserAt0 and foundUnmatchedYes30 so the test fails if either the losing
settlement or the unmatched YES bid branch is missing; use the existing loop
over settled and the same pattern used for foundWinnerAt98 to locate these
events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d5c496e-348c-44d8-8b4c-66a48367c040
📒 Files selected for processing (4)
internal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqlinternal/migrations/044-order-book-events.sqltests/streams/order_book/order_events_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/migrations/032-order-book-actions.sql
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/3547
Summary by CodeRabbit
Release Notes
New Features
Tests