feat: read a stream's high and low across a quiet range - #1421
Conversation
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe high and low value actions now use the last value before the requested range as an anchor. Composed-stream branches also initialize their first result correctly. New schema tests cover quiet ranges, ties, frozen views, and primitive/composed consistency. ChangesHigh and low attestation actions
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR updates existing high/low reads so quiet ranges and composed streams return the correct datapoint without broadening access. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The migration carries forward the last published value for quiet ranges and fixes composed-stream behavior. The added tests cover quiet ranges, primitive/composed consistency, anchors, ties, and pre-first-record cases required by issue Full details: Docstring CoverageExplanation Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
@holdex pr submit-time 4h |
resolves: #1419
get_high_value_primitiveandget_low_value_primitiveselected only rows inside[from, to]. A stream that published nothing during the window returned zero rows, even though the value it published last stood for the whole window and every other read of that stream returns it. Both actions are on the attestation allowlist, andcall_dispatchencodes an empty row set rather than raising, so a quiet range produced a signed, fee-charged attestation carrying no datapoint.Adds the
anchor_recordpattern fromget_record_primitiveto both, unioned into the comparison rather than replacing it, so an in-range row still wins when it is genuinely higher or lower. Edited in place in045— migrations are not append-only, and045has no.prod.sqlvariant (it touches no bridge namespace, andgenerate_prod_migrations.pydoes not list it).The composed branch was broken too
Writing the "primitive and composed agree" test turned up a second defect. Both composed branches returned nothing for every composed stream, not just quiet ranges:
The interpreter does not implement three-valued logic. On the first row
$max_value IS NULLis TRUE and$row.value > $max_valueis NULL, andTRUE OR NULLevaluates to NULL, which reads as not-taken. The first assignment never happened, so$max_valuestayed NULL and the loop discarded every rowget_recordhanded it. Split intoIS NULL/ELSEIFin the same commit.This is the third live instance of that pattern in the repo.
maa_do_withdraw(049-maa-funding.sql:117) still carries it; not touched here.Ties go to the anchor, deliberately
ORDER BY value DESC, event_time ASCmeans the earliest row wins a tie, and the anchor is always the earliest. So a range whose extreme equals the carried-forward value reports anevent_timebeforefrom.That is what the composed branch already does — it walks
get_recordwith a strict>and keeps the first maximum, andget_recordleads with its own anchor. The two branches have to agree. It is also the more honest answer once duplicate pruning lands, since the tying in-range row will usually not exist.Tests
New
tests/streams/high_low_value_test.go, six cases:(event_time, value)— the case that caught the second defectevent_timefrozen_atthat predates the stream still returns nothinggo test -tags kwiltest ./tests/streams/ -run TestHighLowValuepasses.kwil-cli utils parseclean.Note for review
This changes an attestable result. Neither action has ever been attested on mainnet or testnet — mainnet holds 1,011 attestations and testnet 86,966, all of them
price_above_threshold,price_below_threshold,value_in_rangeorindex_change_in_range— so no signed result anyone holds moves. The action name is resolved inside block execution, so every validator needs the new binary before the first call that depends on it.Summary by CodeRabbit
Bug Fixes
Tests