Open
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughAdds guaranteed LP sampling during settlement, reworks sample_lp_rewards with midpoint/spread and two-sided LEAST scoring, reorders settlement processing to sample before position deletion, and updates tests/helpers to exercise and log the new sampling behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Distribute as distribute_fees / process_settlement
participant Sampler as sample_lp_rewards
participant OB as ob_positions (orderbook)
participant Rewards as rewards_insertion
Distribute->>Sampler: sample_lp_rewards(query_id, `@height`)
Sampler->>OB: Query YES buy orders
Sampler->>OB: Query YES sell orders
alt Missing side or already sampled or market settled
Sampler-->>Distribute: NOTICE + early return
else Compute midpoint & spread
Sampler->>Sampler: compute x_mid, x_spread_base, map spread bucket
alt Spread ineligible
Sampler-->>Distribute: NOTICE + return
else Eligible spread
Sampler->>OB: Join paired positions (p1.price = 100 + p2.price)
Sampler->>Sampler: compute TRUE_sum and FALSE_sum per participant
Sampler->>Sampler: participant_score = LEAST(TRUE_sum, FALSE_sum)
Sampler->>Sampler: accumulate global_total_score
alt global_total_score <= 0
Sampler-->>Distribute: NOTICE + abort
else
Sampler->>Rewards: Insert normalized reward_percent per participant
Rewards-->>Distribute: rewards recorded
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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/fee_distribution_audit_test.go (1)
140-141: These block-count assertions still skip the real settlement flow.Both cases call
distribute_fees()directly, so the extra sample is taken whileob_positionsstill exist. The production path goes throughprocess_settlement(), which clears the book before fee distribution, so these assertions won't catch a regression in settlement-time sampling. Please add at least one end-to-end case that settles viasettle_market.Also applies to: 252-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_audit_test.go` around lines 140 - 141, The tests call distribute_fees() directly which skips the real settlement flow and leaves ob_positions populated, so the block-count assertions (e.g., the require.Equal checking blockCount == 2) miss regressions in settlement-time sampling; add at least one end-to-end test that invokes settle_market (or the public entry that triggers process_settlement()) so the book is cleared before fee distribution, then assert the expected blockCount and sampling behavior after a real settlement; specifically update the test(s) referencing distribute_fees(), process_settlement(), ob_positions and the blockCount assertions to include a path that calls settle_market and verifies the final sample is taken post-clearance.
🤖 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 126-128: The final LP sampling call sample_lp_rewards($query_id,
`@height`) must run before ob_positions is deleted during settlement; move the
sample_lp_rewards call earlier in process_settlement() (i.e., execute it before
the code that clears/deletes ob_positions) so the function reads the live
ob_positions rather than an emptied book and produces the intended LP snapshot
prior to calling distribute_fees().
In `@internal/migrations/034-order-book-rewards.sql`:
- Around line 223-227: Remove the unnecessary equality filter "AND p1.amount =
p2.amount" from the complementary-leg JOIN conditions in both the global-score
and per-participant queries (the JOINs referencing p1.query_id = p2.query_id,
p1.participant_id = p2.participant_id, p1.outcome != p2.outcome, p1.amount =
p2.amount, p1.price = 100 + p2.price) so asymmetric legs are not dropped; i.e.,
delete the p1.amount = p2.amount predicate in both places and ensure the score
computation still uses LEAST(p1.amount, p2.amount) to clamp matched size, and
add a regression test case that covers unequal sizes (e.g., a YES sell 300 + NO
buy 100) to verify the smaller leg is scored.
In `@tests/streams/order_book/test_helpers_orderbook.go`:
- Around line 193-215: The helper callSettleMarket currently passes two args and
uses time.Now(), but the settle_market action accepts only query_id and requires
the block timestamp to be the market's settlement timestamp; change
callSettleMarket(ctx, platform, caller, queryID, winningOutcome) to accept a
settlementTimestamp (int64) instead of winningOutcome, set
common.BlockContext.Timestamp to that settlementTimestamp, and call
platform.Engine.Call with a single argument []any{int64(queryID)} (remove
winningOutcome). Update references to callSettleMarket accordingly.
---
Nitpick comments:
In `@tests/streams/order_book/fee_distribution_audit_test.go`:
- Around line 140-141: The tests call distribute_fees() directly which skips the
real settlement flow and leaves ob_positions populated, so the block-count
assertions (e.g., the require.Equal checking blockCount == 2) miss regressions
in settlement-time sampling; add at least one end-to-end test that invokes
settle_market (or the public entry that triggers process_settlement()) so the
book is cleared before fee distribution, then assert the expected blockCount and
sampling behavior after a real settlement; specifically update the test(s)
referencing distribute_fees(), process_settlement(), ob_positions and the
blockCount assertions to include a path that calls settle_market and verifies
the final sample is taken post-clearance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0243cc31-8438-4fc4-8b53-7f2242f56052
📒 Files selected for processing (6)
internal/migrations/033-order-book-settlement.sqlinternal/migrations/034-order-book-rewards.sqltests/streams/order_book/fee_distribution_audit_test.gotests/streams/order_book/fee_distribution_test.gotests/streams/order_book/rewards_test.gotests/streams/order_book/test_helpers_orderbook.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fee_distribution_test.go`:
- Around line 568-570: The test currently computes lpShare but only logs it;
update the test to assert the escrow balance equals this retained LP share after
calling distribute_fees. Specifically, after distribute_fees invoke the existing
method that reads the vault/escrow balance (e.g., the test's escrow balance
getter or state query used elsewhere in this test suite) and add an assertion
that escrowBalance.Cmp(lpShare) == 0 (or equivalent assertion helper) so the
test fails if the 75% remainder was moved or burned; refer to the variables
lpShare, totalFees, infraShare and the distribute_fees call to locate where to
insert the assertion.
In `@tests/streams/order_book/test_helpers_orderbook.go`:
- Around line 198-200: The helper currently hardcodes BlockContext.Height = 1
which causes every settlement sample to be keyed to block 1; change the helper
signature to accept a settlement block height parameter and set
BlockContext.Height to that parameter instead of 1, then update callers (e.g.,
where settle_market and tests that invoke this helper) to pass the appropriate
settlement block height so process_settlement()/sample_lp_rewards($query_id,
`@height`) record the correct block for the final LP sample; adjust any test
fixtures that assumed height 1 accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79d4f96f-f629-4da1-98da-eb83a181d7aa
📒 Files selected for processing (4)
internal/migrations/033-order-book-settlement.sqltests/streams/order_book/fee_distribution_audit_test.gotests/streams/order_book/fee_distribution_test.gotests/streams/order_book/test_helpers_orderbook.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/migrations/033-order-book-settlement.sql
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 `@internal/migrations/032-order-book-actions.sql`:
- Around line 2552-2559: The migration currently calls
process_settlement($query_id, $winning_outcome) while ob_queries.settled is
still false, allowing non-idempotent side-effects in process_settlement and its
callees (e.g. sample_lp_rewards()) to run multiple times on retry; to fix,
persist a durable idempotency guard (e.g. insert/update a settlement_journal or
set ob_queries.settlement_state='processing') immediately before any
side-effects and before invoking process_settlement (change in the block that
calls process_settlement in 032-order-book-actions.sql), and update
process_settlement() and its downstream routines (the non-idempotent code paths
referenced in 033-order-book-settlement.sql lines ~12–265 and
sample_lp_rewards()) to check that persistent marker and return early if it
already exists so retries cannot double-apply payouts/fees; after successful
completion, atomically flip the marker to 'applied' and then set
ob_queries.settled = true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf987e82-0e1c-4a99-8bf3-1c6e981af1b2
📒 Files selected for processing (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/3441
Summary by CodeRabbit
New Features
Bug Fixes
Tests