feat: view prediction market data by stream#1320
Merged
Merged
Conversation
|
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 denormalized discovery fields (data_provider, stream_id, action_id, query_args) to order-book schema, persists them from ABI-decoded query_components in create_market, surfaces them via get_market_info, adds a paginated get_markets_by_stream view, updates migrations, and adds a test validating discovery behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CreateMarket as create_market Action
participant DB as ob_queries Table
participant GetByStream as get_markets_by_stream View
Client->>CreateMarket: Call create_market(query_components ...)
activate CreateMarket
CreateMarket->>CreateMarket: ABI-decode query_components -> data_provider, stream_id, action_id, query_args
CreateMarket->>CreateMarket: Validate non-null decoded fields
CreateMarket->>DB: INSERT new ob_queries row with denormalized fields
deactivate CreateMarket
Client->>GetByStream: Call get_markets_by_stream(stream_id, limit, offset)
activate GetByStream
GetByStream->>DB: SELECT ... FROM ob_queries WHERE stream_id = ? ORDER BY created_at DESC, id DESC LIMIT ... OFFSET ...
DB-->>GetByStream: Market summary rows (with denormalized fields)
deactivate GetByStream
GetByStream-->>Client: Return paginated market summaries
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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 (3)
tests/streams/order_book/discovery_test.go (2)
125-138: UnusedisCheckparameter inengCtxhelper.The
isCheckparameter is accepted but never referenced in the function body. Either remove it or wire it into the context if it serves a purpose (e.g., setting a read-only/check mode flag on the engine context).Proposed fix (remove unused param)
-func engCtx(ctx context.Context, platform *kwilTesting.Platform, caller string, height int64, isCheck bool) *common.EngineContext { +func engCtx(ctx context.Context, platform *kwilTesting.Platform, caller string, height int64) *common.EngineContext {And update the call site on line 67:
- engineCtx := engCtx(ctx, platform, userAddr.Address(), 1, false) + engineCtx := engCtx(ctx, platform, userAddr.Address(), 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/discovery_test.go` around lines 125 - 138, The helper function engCtx currently accepts an unused parameter isCheck; remove isCheck from engCtx's signature and from its callers, and update any tests that call engCtx to pass the existing arguments (ctx, platform, caller, height) only; keep the returned EngineContext, TxContext, and BlockContext construction unchanged (symbols: engCtx, EngineContext, TxContext, BlockContext, coreauth.EthPersonalSignAuth, platform.Txid()).
74-81: Type assertions on row values will panic onnilcolumns.Lines 76–79 use bare type assertions (
row.Values[0].([]byte), etc.) without comma-ok checks. If the decode logic fails to populate a column (returns SQL NULL), the Go value will beniland the assertion to[]byte/stringwill panic with an unhelpful message instead of a clear test failure.This is test code, so it's low severity, but using
requireassertions would give better diagnostics on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/discovery_test.go` around lines 74 - 81, The test currently does bare type assertions on row.Values (in the platform.Engine.Execute callback) which will panic if a column is NULL; update the callback to first require that each row.Values[i] is not nil (use require.NotNil(t, row.Values[0], ...)) and then perform safe comma-ok type assertions (e.g., v, ok := row.Values[0].([]byte); require.True(t, ok, "expected []byte for data_provider")) before assigning to dbProvider, dbStreamID, dbActionID and dbQueryArgs so failures produce clear test errors instead of panics.internal/migrations/044-get-markets-by-stream.sql (1)
16-38: Missing pagination — unbounded result set.The
list_marketsaction (in 032) caps results at 100 rows withLIMIT/OFFSET. This view has no such guard. A popular stream could return an unbounded number of rows.Consider adding pagination parameters consistent with
list_markets:Suggested addition
-CREATE OR REPLACE ACTION get_markets_by_stream($stream_id BYTEA) +CREATE OR REPLACE ACTION get_markets_by_stream( + $stream_id BYTEA, + $limit_val INT, + $offset_val INT +) PUBLIC VIEW RETURNS TABLE ( ... ) { if $stream_id IS NULL { ERROR('stream_id is required'); } + $effective_limit INT := 100; + $effective_offset INT := 0; + if $limit_val IS NOT NULL AND $limit_val > 0 AND $limit_val <= 100 { + $effective_limit := $limit_val; + } + if $offset_val IS NOT NULL AND $offset_val >= 0 { + $effective_offset := $offset_val; + } + RETURN SELECT id, hash, data_provider, action_id, settle_time, settled, winning_outcome, max_spread, min_order_size, created_at FROM ob_queries WHERE stream_id = $stream_id - ORDER BY created_at DESC; + ORDER BY created_at DESC + LIMIT $effective_limit OFFSET $effective_offset; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/044-get-markets-by-stream.sql` around lines 16 - 38, get_markets_by_stream currently returns an unbounded result set; add pagination parameters and enforcement similar to list_markets: add two parameters (e.g., $limit INT, $offset INT) alongside $stream_id, validate/normalize them (default $limit to 100 if NULL, enforce a max cap like 100), keep the existing NULL check for $stream_id, and update the RETURN SELECT in get_markets_by_stream to append ORDER BY created_at DESC LIMIT $limit OFFSET $offset so results are bounded and pageable.
🤖 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/044-get-markets-by-stream.sql`:
- Around line 16-38: get_markets_by_stream currently returns an unbounded result
set; add pagination parameters and enforcement similar to list_markets: add two
parameters (e.g., $limit INT, $offset INT) alongside $stream_id,
validate/normalize them (default $limit to 100 if NULL, enforce a max cap like
100), keep the existing NULL check for $stream_id, and update the RETURN SELECT
in get_markets_by_stream to append ORDER BY created_at DESC LIMIT $limit OFFSET
$offset so results are bounded and pageable.
In `@tests/streams/order_book/discovery_test.go`:
- Around line 125-138: The helper function engCtx currently accepts an unused
parameter isCheck; remove isCheck from engCtx's signature and from its callers,
and update any tests that call engCtx to pass the existing arguments (ctx,
platform, caller, height) only; keep the returned EngineContext, TxContext, and
BlockContext construction unchanged (symbols: engCtx, EngineContext, TxContext,
BlockContext, coreauth.EthPersonalSignAuth, platform.Txid()).
- Around line 74-81: The test currently does bare type assertions on row.Values
(in the platform.Engine.Execute callback) which will panic if a column is NULL;
update the callback to first require that each row.Values[i] is not nil (use
require.NotNil(t, row.Values[0], ...)) and then perform safe comma-ok type
assertions (e.g., v, ok := row.Values[0].([]byte); require.True(t, ok, "expected
[]byte for data_provider")) before assigning to dbProvider, dbStreamID,
dbActionID and dbQueryArgs so failures produce clear test errors instead of
panics.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/migrations/032-order-book-actions.sqlinternal/migrations/044-get-markets-by-stream.sqlscripts/migrate.shtests/streams/order_book/discovery_test.go
MicBun
force-pushed
the
getMarketByStream
branch
from
55d5815 to
afe1c6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/migrations/044-get-markets-by-stream.sql (1)
39-55: Pagination logic is duplicated verbatim fromlist_markets.The
effective_limit/effective_offsetcomputation block is identical tolist_markets(migration 032, lines 366–374). If the cap or default ever needs updating, both actions must be changed in sync.If Kuneiform supports private helper actions that return scalars, this pattern could be extracted. Otherwise, at minimum, a cross-reference comment would reduce the maintenance risk:
+ -- NOTE: Keep pagination logic in sync with list_markets (032-order-book-actions.sql) $effective_limit INT := 100;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/044-get-markets-by-stream.sql` around lines 39 - 55, The pagination cap/default computation using $effective_limit and $effective_offset is duplicated from list_markets (migration 032); either extract that logic into a reusable helper action (e.g., compute_effective_limit_offset) and call it from this action to return the effective_limit and effective_offset, or if Kuneiform cannot expose such a helper, replace the duplicated block with a clear cross-reference comment pointing to list_markets (migration 032) and the canonical implementation so future changes stay in sync; update the code to call the helper action or add the comment and remove duplication accordingly, referencing $effective_limit, $effective_offset and list_markets in your changes.
🤖 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 117-119: Extend the NULL-check in the block that handles
tn_utils.decode_query_components to include all four decoded fields
($data_provider, $stream_id, $action_id_str, $query_args); currently only
$data_provider and $stream_id are validated, so update the conditional to also
test $action_id_str IS NOT NULL and $query_args IS NOT NULL, and raise the same
ERROR('Failed to decode query_components: invalid ABI data or missing
components') when any are NULL to prevent persisting a NULL action_id into
ob_queries.action_id (which later affects get_markets_by_stream).
---
Nitpick comments:
In `@internal/migrations/044-get-markets-by-stream.sql`:
- Around line 39-55: The pagination cap/default computation using
$effective_limit and $effective_offset is duplicated from list_markets
(migration 032); either extract that logic into a reusable helper action (e.g.,
compute_effective_limit_offset) and call it from this action to return the
effective_limit and effective_offset, or if Kuneiform cannot expose such a
helper, replace the duplicated block with a clear cross-reference comment
pointing to list_markets (migration 032) and the canonical implementation so
future changes stay in sync; update the code to call the helper action or add
the comment and remove duplication accordingly, referencing $effective_limit,
$effective_offset and list_markets in your changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/migrations/032-order-book-actions.sqlinternal/migrations/044-get-markets-by-stream.sqltests/streams/order_book/discovery_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/streams/order_book/discovery_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/migrations/032-order-book-actions.sql (1)
104-119: NULL validation correctly extended to all four decoded fields.The guard at Line 117 now checks all of
$data_provider,$stream_id,$action_id_str, and$query_argsbefore persisting, which prevents a silent NULLaction_idfrom reachingob_queriesin the event of a precompile regression. This directly addresses the previous review concern.🤖 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 104 - 119, The validation is good but the declarations/assignments are incorrect: remove the leading $ from variable names (use data_provider, stream_id, action_id_str, query_args) and assign them from tn_utils.decode_query_components using a proper FOR ... IN ... LOOP or SELECT INTO pattern (e.g., FOR row IN SELECT * FROM tn_utils.decode_query_components($query_components) LOOP data_provider := row.data_provider; stream_id := row.stream_id; action_id_str := row.action_id; query_args := row.args; END LOOP;) so the subsequent NULL check references the actual variables (data_provider, stream_id, action_id_str, query_args) before persisting.
🧹 Nitpick comments (1)
internal/migrations/032-order-book-actions.sql (1)
264-294:get_market_infoomits the four newly-populated fields — consider exposing them.
get_market_infois the canonical single-market detail endpoint and its return signature still does not includedata_provider,stream_id,action_id, orquery_args, even though these are now reliably denormalized on every market. Callers who need stream-identity information are forced to useget_markets_by_stream(migration 044) or re-decodequery_componentsthemselves.This is not a blocking defect — the data is still reachable — but it creates an asymmetry in the API surface.
♻️ Suggested extension to
get_market_infoCREATE OR REPLACE ACTION get_market_info($query_id INT) PUBLIC VIEW RETURNS ( hash BYTEA, query_components BYTEA, + data_provider BYTEA, + stream_id BYTEA, + action_id TEXT, + query_args BYTEA, bridge TEXT, settle_time INT8, settled BOOLEAN, winning_outcome BOOLEAN, settled_at INT8, max_spread INT, min_order_size INT8, created_at INT8, creator BYTEA ) { ... for $market in - SELECT hash, query_components, bridge, settle_time, settled, winning_outcome, settled_at, - max_spread, min_order_size, created_at, creator + SELECT hash, query_components, data_provider, stream_id, action_id, query_args, + bridge, settle_time, settled, winning_outcome, settled_at, + max_spread, min_order_size, created_at, creator FROM ob_queries WHERE id = $query_id { - RETURN $market.hash, $market.query_components, $market.bridge, ...; + RETURN $market.hash, $market.query_components, + $market.data_provider, $market.stream_id, $market.action_id, $market.query_args, + $market.bridge, ...; }🤖 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 264 - 294, get_market_info currently returns a subset of ob_queries fields and omits the newly-denormalized stream identity columns; update the action signature and implementation to include data_provider, stream_id, action_id, and query_args by adding those columns to the RETURNS clause, selecting them from ob_queries in the FOR ... SELECT, and returning them in the RETURN statement (retain existing fields/order or append the four new fields consistently) so callers can get stream identity directly from get_market_info.
🤖 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 207-210: The migration ordering is wrong: the create_market action
in migration 032 inserts into ob_queries using columns data_provider, stream_id,
action_id, and query_args that are added in migration 043, so ensure those ALTER
TABLE statements run before the action is created/executed—either renumber
migration 043 so it runs before 032 or move the ALTER TABLE statements (adding
data_provider, stream_id, action_id, query_args to ob_queries) into an earlier
migration (030 or 031); update the migration sequence so create_market (032) and
its INSERTs into ob_queries reference columns that already exist.
---
Duplicate comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 104-119: The validation is good but the declarations/assignments
are incorrect: remove the leading $ from variable names (use data_provider,
stream_id, action_id_str, query_args) and assign them from
tn_utils.decode_query_components using a proper FOR ... IN ... LOOP or SELECT
INTO pattern (e.g., FOR row IN SELECT * FROM
tn_utils.decode_query_components($query_components) LOOP data_provider :=
row.data_provider; stream_id := row.stream_id; action_id_str := row.action_id;
query_args := row.args; END LOOP;) so the subsequent NULL check references the
actual variables (data_provider, stream_id, action_id_str, query_args) before
persisting.
---
Nitpick comments:
In `@internal/migrations/032-order-book-actions.sql`:
- Around line 264-294: get_market_info currently returns a subset of ob_queries
fields and omits the newly-denormalized stream identity columns; update the
action signature and implementation to include data_provider, stream_id,
action_id, and query_args by adding those columns to the RETURNS clause,
selecting them from ob_queries in the FOR ... SELECT, and returning them in the
RETURN statement (retain existing fields/order or append the four new fields
consistently) so callers can get stream identity directly from get_market_info.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/032-order-book-actions.sqlinternal/migrations/044-get-markets-by-stream.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/migrations/044-get-markets-by-stream.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/migrations/030-order-book-schema.sql (1)
103-106: Consider a composite index to support stream discovery sorting.
get_markets_by_streamorders bycreated_at, so a composite index on(stream_id, created_at)can avoid extra sorting as data grows.📈 Index suggestion
CREATE INDEX IF NOT EXISTS idx_ob_queries_stream_id ON ob_queries(stream_id); +CREATE INDEX IF NOT EXISTS idx_ob_queries_stream_created_at ON ob_queries(stream_id, created_at); CREATE INDEX IF NOT EXISTS idx_ob_queries_provider_stream ON ob_queries(data_provider, stream_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/030-order-book-schema.sql` around lines 103 - 106, The current single-column index idx_ob_queries_stream_id on ob_queries(stream_id) won't support ORDER BY created_at used by get_markets_by_stream; add a composite index on (stream_id, created_at) (e.g., create or replace idx_ob_queries_stream_id_created_at) so queries filtering by stream_id and ordering by created_at can use an index-only scan and avoid sorting, and consider removing or keeping the old single-column idx_ob_queries_stream_id only if still needed by other queries; update migration SQL around the ob_queries index creation (referencing idx_ob_queries_stream_id and the created_at column) to add the composite index.
🤖 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/043-get-markets-by-stream.sql`:
- Around line 52-57: The ORDER BY in the RETURN SELECT from ob_queries uses only
created_at which can produce non-deterministic pagination; update the ORDER BY
clause to add a deterministic unique tiebreaker using the primary key (id) —
e.g., ORDER BY created_at DESC, id DESC — so pagination (LIMIT/OFFSET) is stable
when created_at ties occur; ensure the added id sort uses the same direction as
created_at to preserve intended result ordering.
---
Nitpick comments:
In `@internal/migrations/030-order-book-schema.sql`:
- Around line 103-106: The current single-column index idx_ob_queries_stream_id
on ob_queries(stream_id) won't support ORDER BY created_at used by
get_markets_by_stream; add a composite index on (stream_id, created_at) (e.g.,
create or replace idx_ob_queries_stream_id_created_at) so queries filtering by
stream_id and ordering by created_at can use an index-only scan and avoid
sorting, and consider removing or keeping the old single-column
idx_ob_queries_stream_id only if still needed by other queries; update migration
SQL around the ob_queries index creation (referencing idx_ob_queries_stream_id
and the created_at column) to add the composite index.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/migrations/030-order-book-schema.sqlinternal/migrations/032-order-book-actions.sqlinternal/migrations/043-denormalize-ob-queries.sqlinternal/migrations/043-get-markets-by-stream.sqltests/streams/order_book/discovery_test.go
💤 Files with no reviewable changes (1)
- internal/migrations/043-denormalize-ob-queries.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/migrations/032-order-book-actions.sql
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 `@internal/migrations/030-order-book-schema.sql`:
- Around line 24-27: The nullable discovery columns (data_provider, stream_id,
action_id, query_args) allow partially-populated rows that break downstream
create_market (migration 032) and make markets invisible to
get_markets_by_stream; add a CHECK constraint on the table that enforces
atomicity of the denormalized set (e.g., require either all four columns are
NULL or all four are NOT NULL, or at minimum enforce stream_id IS NOT NULL =>
data_provider IS NOT NULL and stream_id IS NOT NULL => action_id IS NOT NULL and
stream_id IS NOT NULL => query_args IS NOT NULL) and include the constraint name
so it’s applied by migration 030 and prevents silent partial population.
- Around line 103-107: Migration 030 was modified retroactively to add columns
and indexes but existing deployments that already ran 030 will skip it; create a
follow-up safety migration that retroactively adds the missing columns and
indexes using idempotent statements: run ALTER TABLE ob_queries ADD COLUMN IF
NOT EXISTS for data_provider, stream_id, action_id, and query_args, and run
CREATE INDEX IF NOT EXISTS for idx_ob_queries_stream_id_created,
idx_ob_queries_provider_stream, and idx_ob_queries_action_id so existing systems
get the new columns/indexes without failing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/030-order-book-schema.sqlinternal/migrations/043-get-markets-by-stream.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/migrations/043-get-markets-by-stream.sql
Contributor
Author
|
@holdex pr submit-time 4h |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
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/3382
Summary by CodeRabbit
New Features
Chores
Tests