Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughTwo SQL migrations were updated: settlement fee accumulator assignments were moved to immediately follow unlock operations; LP reward sampling logic was reworked to explicitly initialize and transform bid/ask values and adjust midpoint/liquidity checks. Tests were updated to compute LP expectations from sampled rewards rather than fixed percentages. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
🧹 Nitpick comments (1)
internal/migrations/034-order-book-rewards.sql (1)
117-153: Add a regression test for complementary-side midpoint cases.This rewrite changes how
outcome = FALSEorders are projected onto the YES price scale before the two-sided-liquidity gate and midpoint calculation. The current reward tests cover empty/incomplete books and a happy path, but they do not pin cases where the best bid or ask comes only from NO-side orders. A targeted test here would make future refactors much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/034-order-book-rewards.sql` around lines 117 - 153, Add a regression test that covers complementary-side midpoint cases where the best bid or ask comes only from NO-side orders (price projection onto YES scale) to exercise the two-sided-liquidity gate and midpoint calculation in the logic that reads ob_positions by query_id and outcome; specifically, create tests that insert NO-side buys (price < 0) and NO-side sells (price > 0) and verify they correctly update $best_bid and $best_ask and produce the expected $x_mid, as well as tests where only NO-side provides the best bid/ask to ensure the code path that returns early when $best_bid = 0 OR $best_ask = 100 is not incorrectly triggered. Ensure test names reference the midpoint behavior (e.g., "complementary_side_midpoint_no_side_only") and assert the computed midpoint equals the manual projection ((best_ask + best_bid) / 2) after the NO->YES projections are applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/migrations/034-order-book-rewards.sql`:
- Around line 117-153: Add a regression test that covers complementary-side
midpoint cases where the best bid or ask comes only from NO-side orders (price
projection onto YES scale) to exercise the two-sided-liquidity gate and midpoint
calculation in the logic that reads ob_positions by query_id and outcome;
specifically, create tests that insert NO-side buys (price < 0) and NO-side
sells (price > 0) and verify they correctly update $best_bid and $best_ask and
produce the expected $x_mid, as well as tests where only NO-side provides the
best bid/ask to ensure the code path that returns early when $best_bid = 0 OR
$best_ask = 100 is not incorrectly triggered. Ensure test names reference the
midpoint behavior (e.g., "complementary_side_midpoint_no_side_only") and assert
the computed midpoint equals the manual projection ((best_ask + best_bid) / 2)
after the NO->YES projections are applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11e95c4b-114f-44cc-a24f-40ae2b4e9709
📒 Files selected for processing (2)
internal/migrations/033-order-book-settlement.sqlinternal/migrations/034-order-book-rewards.sql
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/streams/order_book/fee_distribution_test.go (2)
198-207: Potential precision loss when converting float64 percentage to int64.The conversion
int64(p1Reward*100)truncates toward zero. Ifp1Rewardis stored as64.33but represented in float64 as64.32999999..., then64.32999*100 = 6432.999...truncates to6432instead of6433, causing an off-by-one error in expected values.🔧 Suggested fix using math.Round
+import "math" + if p1Reward, ok := rewards[1]; ok { // Convert float64 percentage to big.Int with precision // reward_percent is already 0-100 - p1Wei := new(big.Int).Mul(lpShareTotal, big.NewInt(int64(p1Reward*100))) + p1Wei := new(big.Int).Mul(lpShareTotal, big.NewInt(int64(math.Round(p1Reward*100)))) expectedLP1 = new(big.Int).Div(p1Wei, big.NewInt(10000)) } if p2Reward, ok := rewards[2]; ok { - p2Wei := new(big.Int).Mul(lpShareTotal, big.NewInt(int64(p2Reward*100))) + p2Wei := new(big.Int).Mul(lpShareTotal, big.NewInt(int64(math.Round(p2Reward*100)))) expectedLP2 = new(big.Int).Div(p2Wei, big.NewInt(10000)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_test.go` around lines 198 - 207, The test is truncating float percentages when computing expectedLP1/expectedLP2 (conversion int64(p1Reward*100) and int64(p2Reward*100)), which can cause off-by-one errors; update the conversion to round the scaled float before casting (e.g., use math.Round(p1Reward*100) and math.Round(p2Reward*100)) so the calculations that use lpShareTotal produce correct big.Int results for expectedLP1 and expectedLP2.
388-401: Direct map access and float64 precision concerns.Two issues:
Direct map access: Unlike the 1-block test which uses
if _, ok := rewards[1]; ok, this code directly accessesrewards1000[1]etc. If a participant ID doesn't exist, Go returns0.0silently, which could mask test setup issues or lead to false positives.Precision loss: Same truncation issue with
int64(totalP1Reward*100)— usemath.Roundfor safety.🔧 Suggested fix for safety and precision
// Calculate average LP share across all blocks +// Verify expected participant IDs exist +require.Contains(t, rewards1000, 1, "Missing participant 1 at block 1000") +require.Contains(t, rewards1000, 2, "Missing participant 2 at block 1000") +require.Contains(t, rewards2000, 1, "Missing participant 1 at block 2000") +require.Contains(t, rewards2000, 2, "Missing participant 2 at block 2000") +require.Contains(t, rewards3000, 1, "Missing participant 1 at block 3000") +require.Contains(t, rewards3000, 2, "Missing participant 2 at block 3000") + totalP1Reward := rewards1000[1] + rewards2000[1] + rewards3000[1] totalP2Reward := rewards1000[2] + rewards2000[2] + rewards3000[2] // User1 LP: (lpShareTotal * totalP1Reward) / (100 * 3) // User2 LP: (lpShareTotal * totalP2Reward) / (100 * 3) expectedLP1 := new(big.Int).Div( - new(big.Int).Mul(lpShareTotal, big.NewInt(int64(totalP1Reward*100))), + new(big.Int).Mul(lpShareTotal, big.NewInt(int64(math.Round(totalP1Reward*100)))), big.NewInt(30000), ) expectedLP2 := new(big.Int).Div( - new(big.Int).Mul(lpShareTotal, big.NewInt(int64(totalP2Reward*100))), + new(big.Int).Mul(lpShareTotal, big.NewInt(int64(math.Round(totalP2Reward*100)))), big.NewInt(30000), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_test.go` around lines 388 - 401, The test directly indexes rewards1000/rewards2000/rewards3000 (used to compute totalP1Reward/totalP2Reward) and converts float64 to int64 via int64(totalP1Reward*100) which can silently mask missing keys and cause precision truncation; update the code that computes totalP1Reward/totalP2Reward to first check key presence (e.g., using the comma-ok pattern) for each participant ID in rewards1000/rewards2000/rewards3000 and fail the test or handle missing entries explicitly, and when converting the scaled float to integer for expectedLP1/expectedLP2 use math.Round on the product (e.g., math.Round(totalP1Reward*100)) before casting to int64 to avoid truncation; keep references to lpShareTotal, expectedLP1 and expectedLP2 to replace the current int64(...) conversions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/streams/order_book/fee_distribution_test.go`:
- Around line 198-207: The test is truncating float percentages when computing
expectedLP1/expectedLP2 (conversion int64(p1Reward*100) and
int64(p2Reward*100)), which can cause off-by-one errors; update the conversion
to round the scaled float before casting (e.g., use math.Round(p1Reward*100) and
math.Round(p2Reward*100)) so the calculations that use lpShareTotal produce
correct big.Int results for expectedLP1 and expectedLP2.
- Around line 388-401: The test directly indexes
rewards1000/rewards2000/rewards3000 (used to compute
totalP1Reward/totalP2Reward) and converts float64 to int64 via
int64(totalP1Reward*100) which can silently mask missing keys and cause
precision truncation; update the code that computes totalP1Reward/totalP2Reward
to first check key presence (e.g., using the comma-ok pattern) for each
participant ID in rewards1000/rewards2000/rewards3000 and fail the test or
handle missing entries explicitly, and when converting the scaled float to
integer for expectedLP1/expectedLP2 use math.Round on the product (e.g.,
math.Round(totalP1Reward*100)) before casting to int64 to avoid truncation; keep
references to lpShareTotal, expectedLP1 and expectedLP2 to replace the current
int64(...) conversions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca1638ee-2beb-4353-b637-c069d521e76e
📒 Files selected for processing (1)
tests/streams/order_book/fee_distribution_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/3419
Summary by CodeRabbit
Bug Fixes
Chores
Tests