feat: validate market collateral and token parity#1282
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a SQL migration that defines Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
MicBun
changed the title
Time Submission Status
|
![coderabbitai[bot]](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 (3)
internal/migrations/037-order-book-validation.sql (1)
125-131: Note: Collateral validation has multi-market limitations.The exact equality check
vault_balance = expected_collateralcorrectly validates single-market scenarios, but will returnFALSEwhen multiple markets exist sincevault_balanceis the total across all markets. The testtestMultipleMarketsIsolationdocuments this behavior.Consider adding a comment here noting this limitation, or returning an additional field indicating the validation scope.
tests/streams/order_book/validate_market_collateral_test.go (2)
168-223: Test name is misleading.The test is named
testValidMarketAfterSettlementand the comment says "tests validation after settlement (positions deleted)", but the test doesn't actually perform settlement. It only validates a market with existing positions. Consider renaming totestValidMarketWithPositionsBeforeSettlementor adding a TODO to implement actual settlement testing.
369-379: Type assertions may panic on unexpected data.The type assertions (e.g.,
row.Values[0].(bool),row.Values[4].(*kwilTypes.Decimal)) will panic if the SQL function returns unexpected types. While this is acceptable in tests, consider adding error context for debugging failed type assertions.func(row *common.Row) error { - validBinaries = row.Values[0].(bool) - validCollateral = row.Values[1].(bool) - totalTrue = row.Values[2].(int64) - totalFalse = row.Values[3].(int64) - vaultBalance = row.Values[4].(*kwilTypes.Decimal).String() - expectedCollateral = row.Values[5].(*kwilTypes.Decimal).String() - openBuysValue = row.Values[6].(int64) + var ok bool + validBinaries, ok = row.Values[0].(bool) + require.True(t, ok, "validBinaries type assertion failed: got %T", row.Values[0]) + validCollateral, ok = row.Values[1].(bool) + require.True(t, ok, "validCollateral type assertion failed: got %T", row.Values[1]) + totalTrue, ok = row.Values[2].(int64) + require.True(t, ok, "totalTrue type assertion failed: got %T", row.Values[2]) + totalFalse, ok = row.Values[3].(int64) + require.True(t, ok, "totalFalse type assertion failed: got %T", row.Values[3]) + dec, ok := row.Values[4].(*kwilTypes.Decimal) + require.True(t, ok, "vaultBalance type assertion failed: got %T", row.Values[4]) + vaultBalance = dec.String() + dec, ok = row.Values[5].(*kwilTypes.Decimal) + require.True(t, ok, "expectedCollateral type assertion failed: got %T", row.Values[5]) + expectedCollateral = dec.String() + openBuysValue, ok = row.Values[6].(int64) + require.True(t, ok, "openBuysValue type assertion failed: got %T", row.Values[6]) rowCount++ return nil })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/037-order-book-validation.sql(1 hunks)tests/streams/order_book/validate_market_collateral_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/order_book/validate_market_collateral_test.go (3)
tests/streams/utils/runner.go (1)
RunSchemaTest(37-116)internal/migrations/migration.go (1)
GetSeedScriptStatements(60-109)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(75-88)
⏰ 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: acceptance-test
- GitHub Check: lint
🔇 Additional comments (12)
internal/migrations/037-order-book-validation.sql (5)
1-17: Clear and well-documented migration header.The documentation clearly explains the purpose, validation checks, dependencies, and return values. This is helpful for understanding the function's role in the system.
57-79: LGTM!The share counting logic correctly aggregates holdings (price=0) and open sell orders (price>0) for both TRUE and FALSE outcomes with proper null handling via COALESCE.
81-93: LGTM!The open buy collateral calculation correctly sums
|price| * amountto get total cents, with the wei conversion handled in step 4.
95-100: LGTM!The wei conversion arithmetic is correct: share collateral uses 10^18 wei per share, and buy collateral properly converts cents to wei by multiplying by 10^18 then dividing by 100.
102-115: LGTM!Good defensive programming: the error handling correctly distinguishes between an unavailable bridge (no rows) and a legitimately empty vault (zero balance), providing a clear error message for debugging.
tests/streams/order_book/validate_market_collateral_test.go (7)
19-36: LGTM!The test structure follows the established patterns from the codebase, using
RunSchemaTestwith proper seed statements and test options.
38-73: LGTM!Good coverage of the base case - empty market validation with expected zero values for all metrics.
75-116: LGTM!Test correctly validates balanced split limit order scenario with proper assertions on share counts and expected collateral.
118-166: LGTM!Test correctly validates order matching scenario. After User2 buys 50 NO shares from User1's sell order, the total shares remain balanced at 100 TRUE and 100 FALSE.
225-269: LGTM!Test correctly validates open buy order scenario with proper assertions on zero shares and 6000 cents ($60) escrowed collateral.
271-348: Well-documented multi-market isolation test.This test effectively demonstrates and documents the limitation of per-market collateral validation when multiple markets exist. The comments at lines 325-326 and 340-341 clearly explain the expected behavior.
385-402: LGTM!The helper function correctly creates markets with consistent test parameters. The hardcoded query hash is acceptable since each test function runs in isolation with state resets.
1 participant
resolves: https://github.com/truflation/website/issues/2924
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.