Repository navigation
fix: publish one-sided executable price books (RAI-2930) - #196
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 6 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. WalkthroughMarket sampling now records executable bid and ask values independently of midpoint snapshots. The database stores and prunes book snapshots alongside price history. Non-historical price responses include available canonical-token books with a freshness source; historical responses omit them. Documentation describes the response fields and sampling behavior. Tests cover book aggregation, persistence, response conditions, and directional slippage references. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The one-sided executable book remains available when no midpoint exists. No identified issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.98.1)Clippy execution failed Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@rain-marvin review |
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a nullable executableBook (best bid and best ask, each optional) to the /v1 and /v2 price responses. Each side now refreshes on its own, so a market where only one side is funded (the SNES case in RAI-2930) still publishes a current trading price. The retained midpoint rules do not change. Books go into a new latest-only market_books table. They commit in the same transaction as the midpoint snapshots, are cleared when a completed sample no longer sees a side, are pruned with the same retention, and are left out of historical (at=) responses.
Overall read: the change is sound and well tested. The panel traced the sampler end to end: one-sided, empty and crossed books, a failed market that discards the whole sample, atomic persistence, and live or cached labeling. It also checked migration order and rollback safety. It found no blocker. Two minor points are below:
- the executable book still mixes in orders on legacy or unwrapped token variants, which the swap routes cannot take;
- the
observed_atupsert guard can silently freeze books after a backward clock correction.
One design trade-off to keep in mind: the book takes orders from any owner. When the maker's own side is empty, a single third-party order sets the directional reference. The docs and PR description already say this reference guards against worse execution, not against an unfair price, so this is not raised as a finding.
claude-opus-5-5 · high · 17 min
8ddd854 to
b9bf070
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@rain-marvin approve Both findings from your review are addressed in b9bf070:
All 520 local API tests, formatting, strict all-targets/all-features Clippy/static checks, and commit hooks pass. Please verify these fixes and approve if satisfied. Staging deployment was cancelled before deployment steps at Arda's request. |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR adds a nullable executableBook (best bid and best ask, each optional) to the /v1 and /v2 price responses, stored latest-only in market_books and committed atomically with the midpoint snapshots. The retained midpoint rules do not change.
Both earlier findings are resolved in b9bf070:
- Variant orders in the executable book (src/market_price.rs): resolved.
ObservedQuotenow carriessource_asset_address, set from the decoded order's actual asset token.aggregate_market_pricesbuilds a separate executable book only from observations whose source token is the canonical wrapped address, so a better legacy or underlying price can no longer becomebestAsk/bestBidand break the swap reference guard. Variants still feed the display midpoint. The crossed check now runs on each book independently, so a crossed display book no longer clears valid canonical liquidity. Four new tests cover a better variant price on both sides, variant-only liquidity (midpoint kept, executable sides null), independent crossing, and the decoded source token for canonical and variant orders. The docs say the same. - Silent
observed_atguard (src/db/market_price_history.rs): resolved. The monotonic guard stays, which my finding allowed, and the write now counts book rows that the guard skipped and logs a warning with produced, written and skipped counts after commit. A backward clock step is now visible in the logs instead of silent.
No thread was resolved by hand; both of my threads are still open and are addressed by the code above.
The diff since my last review adds no new defects that I found. CI is green on b9bf070 (all four Rust test shards, static checks and docs).
claude-opus-5-5 · high · 3 min
ueco-jb
left a comment
ueco-jb
left a comment
There was a problem hiding this comment.
executableBook takes only canonical-token orders. Failed, unfunded and dust quotes stay out, each completed sample clears sides it no longer sees, and books commit in the same transaction as the midpoints. Historical (at=) responses carry no book, and live means non-future and at most two sample intervals old. The units match the swap: bestAsk and 1/bestBid equal the candidate IO ratios the reference guard filters on. slippage.rs changes only in tests, and the retained midpoint behaviour is unchanged.
|
@coderabbitai approve |
|
@rain-marvin approve |
✅ Action performedComments resolved and changes approved. |
|
@rain-marvin approve |
1 similar comment
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR adds a nullable executableBook (best bid and best ask, each optional) to the /v1 and /v2 price responses. The book is built only from canonical wrapped-token orders, stored latest-only in market_books, and committed atomically with the midpoint snapshots. The only change since my last review (b9bf070) is a docs commit in docs/src/prices.md. The code is unchanged and still looks correct to me.
My earlier findings:
- Variant orders in the executable book (src/market_price.rs): resolved in b9bf070. Book sides come only from orders whose source token is the canonical wrapped address, and tests cover this.
- Silent
observed_atguard (src/db/market_price_history.rs): resolved in b9bf070. The guard stays, and skipped book rows are now counted and logged.
Threads resolved by hand:
- Crossed canonical executable book clears both sides (src/market_price.rs): dismissed, and the dismissal holds. The code is unchanged, and the docs state that crossed books publish both sides as null. This fails closed: a client loses its reference and does not get a loose one. A crossed canonical book is also an arbitrage that tends to clear. This is acceptable as documented.
- Unused
assets_per_sharecolumn inmarket_books: dismissed, and the dismissal holds. The column is still written and selected but never read into a response. It costs one text column and records which ratio applied to each book. This is a nit with no behavior impact. - Book references in unwrapped swap mode (docs/src/prices.md): addressed in b69a388. The docs now say to send references with
denomination: "wrapped", or to convert them withwrappedReference * inputAssetsPerShare / outputAssetsPerShare. I checked this formula againstnormalize_calldata_price_capinsrc/routes/swap/denomination.rs. The server converts unwrapped to wrapped with* outputAssetsPerShare / inputAssetsPerShare, so the documented conversion is its exact inverse. It also uses 1 for a token without a wrapper, asratio_for_tokendoes.
The new diff adds no defects that I found. CI is green on b69a388 (all four Rust test shards, static checks, docs).
claude-opus-5-5 · high · 45 s
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR adds a nullable executableBook (best bid and best ask, each optional) to the /v1 and /v2 price responses. The book is built only from canonical wrapped-token orders, stored latest-only in market_books, and written in the same transaction as the midpoint snapshots.
The head is still b69a388, the commit I already reviewed and approved, so there is no new diff to review. I checked the fixes again in the code at that commit.
My earlier findings:
- Variant orders in the executable book (
src/market_price.rs): resolved in b9bf070. Book sides come only from orders whose source token is the canonical wrapped address. Legacy and unwrapped orders still feed the display midpoint, and tests cover this. - Silent
observed_atguard (src/db/market_price_history.rs): resolved in b9bf070. The guard stays, but book rows that the upsert skips are now counted, and a warning is logged with the written and skipped counts.
Threads resolved by hand:
- Crossed canonical executable book clears both sides (
src/market_price.rs): dismissed, and the dismissal holds.docs/src/prices.mdsays that crossed books publish both sides as null, and the code logsignoring crossed canonical executable book. This fails closed: a client loses its reference and never gets a looser one. A crossed canonical book is also an arbitrage that tends to clear quickly. - Unused
assets_per_sharecolumn inmarket_books: dismissed, and the dismissal holds. No response reads it, but it costs one text column and records which ratio applied to each book. This is a nit with no effect on behavior. - Book references in unwrapped swap mode (
docs/src/prices.md): addressed in b69a388. The docs now say to send references withdenomination: "wrapped", or to convert them withwrappedReference * inputAssetsPerShare / outputAssetsPerShare. This is the exact inverse of whatnormalize_calldata_price_capdoes on the server.
I found no new defects. CI is green on b69a388 (all four Rust test shards, static checks, rust-checks and docs).
claude-opus-5-5 · high · 30 s
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR adds a nullable executableBook (best bid and best ask, each optional) to the /v1 and /v2 price responses. The book is built only from canonical wrapped-token orders, stored latest-only in market_books, and written in the same transaction as the midpoint snapshots.
The head is still b69a388, the commit I reviewed and approved before, so there is no new diff to check. I checked each fix again in the code at that commit.
My earlier findings:
- Variant orders in the executable book (
src/market_price.rs): resolved.aggregate_observationsfeedsexecutable_booksonly whensource_asset_address == asset_address, so legacy and unwrapped orders still feed the display midpoint but cannot setbestBidorbestAsk. - Silent
observed_atguard (src/db/market_price_history.rs): resolved. The guard stays, but the code now counts book rows that the upsert skips (rows_affected() == 0) and logs a warning with the written and skipped counts.
Threads resolved by hand:
- Crossed canonical executable book clears both sides (
src/market_price.rs): dismissed, and the dismissal holds.docs/src/prices.mdsays that crossed books publish both sides as null, and the code logsignoring crossed canonical executable book. This fails closed: a client loses its reference but never gets a looser one. - Unused
assets_per_sharecolumn inmarket_books: dismissed, and the dismissal holds. No response reads it, but it costs one text column and records which ratio applied to each book. This is a nit with no effect on behavior. - Book references in unwrapped swap mode (
docs/src/prices.md): addressed. The docs now say to send references withdenomination: "wrapped", or to convert them withwrappedReference * inputAssetsPerShare / outputAssetsPerShare.
I found no new defects. CI is green on b69a388 (all four Rust test shards, static checks, rust-checks and docs).
claude-opus-5-5 · high · 30 s
2 participants
The API now publishes fresh executable bid and ask observations independently when only one side has inventory. This fixes the API half of RAI-2930 while preserving retained midpoint prices and history; the companion website PR #307 uses the relevant side as its trading reference.
Live effect: Funded one-sided markets expose current trading prices · Risk: Medium (stored data and trade pricing) · Ships: Next API release
Decisions
executableBookalongside the existing display fields. Missing sides are cleared on each completed sample; current-book timestamps are separate from the retained midpoint. Aggregation.Risks
Proof
Rollout
Deploy the API to preview first and verify a fresh funded SNES bid with no ask. Then deploy the companion website and check quoting/calldata with unchanged 100 bps slippage. Roll back the website consumer first if needed; the additive API field/table can remain unused.