feat: sign broker-mark prices instead of IEX bid#8
Merged
Conversation
The deployed oracle hits the Alpaca Market Data API's
/v2/stocks/{sym}/quotes/latest with no `feed` param, which silently
defaults to the free-tier IEX-only feed. IEX's book is thin, frozen
outside its session, and one-venue — so the bid we sign drifts well
off real NBBO. Two examples observed today (pre-market):
SPYM: oracle 81.16 vs broker mark 83.86 (-3.2%)
PPLT: oracle 170.23 vs broker mark 176.35 (-3.5%)
MSTR: oracle 150.94 vs broker mark 158.43 (-4.7%, IEX ask=0)
st0x.liquidity-monitor already routes around this — it reads each
position's `current_price` from the Broker API rather than paying
for the SIP Market Data subscription. We mirror that here. The
broker mark is Alpaca's internal real-time NBBO-driven valuation
of the issuer's holdings, exposed at no extra cost on top of the
broker account.
Migration:
- AlpacaClient now hits broker-api.alpaca.markets/v1/trading/
accounts/{id}/positions with HTTP Basic auth.
- QuoteData carries a single `price: f64` (the broker mark) plus
`t: DateTime<Utc>` set to fetch time. The broker exposes no
per-mark timestamp, and an oracle that polls every poll_interval
bounds how stale that fetch time is in practice.
- /context/v1 signs the same mark for both directions; sells still
invert via Rain Float in build_context.
- New required env: ALPACA_BROKER_ACCOUNT_ID. ALPACA_API_KEY_ID /
_SECRET_KEY are reused as Broker API creds (Basic auth).
Verification (see examples/probe_local.rs): a locally-run server
points at the live broker account and returns prices that match
the broker's reported `current_price` exactly (0.0000% drift)
across all 11 registered symbols.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR switches price sourcing from Alpaca Market Data (per-symbol bid/ask) to Alpaca Broker positions (account marks), changes auth to HTTP Basic (base64 api_key:api_secret), refactors QuoteData to a single price field, and updates polling to fetch all account positions once per cycle. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Example/App
participant Cache as QuoteCache
participant Alpaca as AlpacaClient
participant BrokerAPI as Alpaca Broker API
participant Oracle as Oracle Build
rect rgba(100, 150, 200, 0.5)
Note over Client,Oracle: Polling / Cache Update Flow
Cache->>Cache: poll_once()
Cache->>Alpaca: fetch_marks()
Alpaca->>BrokerAPI: GET /v2/accounts/{account_id}/positions (Basic Auth)
BrokerAPI-->>Alpaca: List[{symbol, current_price, ...}]
Alpaca->>Alpaca: positions_to_marks() -> HashMap<symbol,{price,t}>
Alpaca-->>Cache: marks map
loop For Each Configured Symbol
Cache->>Cache: Update if symbol present in marks
end
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Oracle: Request Handling Flow
Client->>Cache: snapshot_many(symbols)
Cache-->>Client: HashMap<symbol,{price,t}>
Client->>Oracle: build_response_from_quote(price)
Oracle->>Oracle: Validate price > 0.0
Oracle-->>Client: OracleResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/probe_local.rs`:
- Around line 92-100: The code currently uses panic-prone indexing and unwraps
when parsing probe responses (e.g., buy_resp[0], sell_resp[0], and
context[1]/context[2] plus .format().unwrap()); change these to safe accesses
and propagate clear errors: use .get(0) on buy_resp/sell_resp and
.get(1)/.get(2) on their context arrays, convert the found B256 values with
Float::from(...).format()? and return a Result (or use ? with a custom error)
when any get() is None or format() fails, producing descriptive messages like
"missing buy response element" or "missing context[1] in sell response" to
replace the current unwraps and indexing.
In `@src/alpaca.rs`:
- Around line 147-177: The SAMPLE_POSITIONS_JSON constant contains real,
sensitive broker/account values; replace its contents with synthetic,
non-identifying test data while preserving the exact JSON structure and field
names used for deserialization (e.g., "asset_id", "symbol", "qty",
"current_price", "avg_entry_price", "market_value", "cost_basis",
"unrealized_pl", etc.). Ensure numeric values remain as strings where currently
represented (e.g., "qty": "123.45") so deserialization types don't change, keep
both position objects (one fully-populated and one minimal) to exercise optional
fields, and update the SAMPLE_POSITIONS_JSON constant accordingly in alpaca.rs
(const SAMPLE_POSITIONS_JSON) so tests validate shape not real data.
In `@src/lib.rs`:
- Around line 224-228: The guard currently only rejects non-positive marks but
lets non-finite values pass; update the condition in the same block that returns
AppError::BadRequest to also check for non-finite prices by using
quote.price.is_finite(), e.g. change the if that uses quote.price <= 0.0 to
something like if !quote.price.is_finite() || quote.price <= 0.0 { ... } so
malformed upstream marks are rejected at the boundary (referencing quote.price,
pair.symbol and the AppError::BadRequest return).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30d7bf0f-b220-4a6d-98cc-ef576f7c67c8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.env.exampleCargo.tomlexamples/probe_local.rssrc/alpaca.rssrc/cache.rssrc/lib.rssrc/main.rssrc/oracle.rstests/integration.rs
Comment on lines
+92
to
+100
| let buy_price = Float::from(alloy::primitives::B256::from(buy_resp[0].context[1])) | ||
| .format() | ||
| .unwrap(); | ||
| let sell_price = Float::from(alloy::primitives::B256::from(sell_resp[0].context[1])) | ||
| .format() | ||
| .unwrap(); | ||
| let publish = Float::from(alloy::primitives::B256::from(buy_resp[0].context[2])) | ||
| .format() | ||
| .unwrap(); |
There was a problem hiding this comment.
Avoid panic-prone indexing in probe response parsing.
Direct [0] and context indexing can panic on unexpected responses; returning a clear error makes this smoke probe much easier to trust during incidents.
Suggested fix
- let buy_price = Float::from(alloy::primitives::B256::from(buy_resp[0].context[1]))
+ let buy0 = buy_resp
+ .first()
+ .ok_or_else(|| anyhow::anyhow!("empty oracle response for buy request"))?;
+ let sell0 = sell_resp
+ .first()
+ .ok_or_else(|| anyhow::anyhow!("empty oracle response for sell request"))?;
+
+ let buy_price = Float::from(alloy::primitives::B256::from(
+ buy0.context
+ .get(1)
+ .cloned()
+ .ok_or_else(|| anyhow::anyhow!("missing price field in buy context"))?,
+ ))
.format()
.unwrap();
- let sell_price = Float::from(alloy::primitives::B256::from(sell_resp[0].context[1]))
+ let sell_price = Float::from(alloy::primitives::B256::from(
+ sell0.context
+ .get(1)
+ .cloned()
+ .ok_or_else(|| anyhow::anyhow!("missing price field in sell context"))?,
+ ))
.format()
.unwrap();
- let publish = Float::from(alloy::primitives::B256::from(buy_resp[0].context[2]))
+ let publish = Float::from(alloy::primitives::B256::from(
+ buy0.context
+ .get(2)
+ .cloned()
+ .ok_or_else(|| anyhow::anyhow!("missing publish_time field in buy context"))?,
+ ))
.format()
.unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/probe_local.rs` around lines 92 - 100, The code currently uses
panic-prone indexing and unwraps when parsing probe responses (e.g.,
buy_resp[0], sell_resp[0], and context[1]/context[2] plus .format().unwrap());
change these to safe accesses and propagate clear errors: use .get(0) on
buy_resp/sell_resp and .get(1)/.get(2) on their context arrays, convert the
found B256 values with Float::from(...).format()? and return a Result (or use ?
with a custom error) when any get() is None or format() fails, producing
descriptive messages like "missing buy response element" or "missing context[1]
in sell response" to replace the current unwraps and indexing.
Comment on lines
+147
to
+177
| /// Real positions response shape captured from the live Broker API | ||
| /// for the issuer's account. Exercises the fields we care about | ||
| /// plus extras we ignore. | ||
| const SAMPLE_POSITIONS_JSON: &str = r#"[ | ||
| { | ||
| "asset_id": "de29752c-29ea-479c-8abe-5fca106af9e6", | ||
| "symbol": "SPYM", | ||
| "exchange": "ARCA", | ||
| "asset_class": "us_equity", | ||
| "asset_marginable": true, | ||
| "qty": "741.476711632", | ||
| "qty_available": "741.476711632", | ||
| "avg_entry_price": "80.878548", | ||
| "side": "long", | ||
| "market_value": "62180.014594", | ||
| "cost_basis": "59969.559813", | ||
| "unrealized_pl": "2210.454781", | ||
| "unrealized_plpc": "0.03686", | ||
| "unrealized_intraday_pl": "73.925228", | ||
| "unrealized_intraday_plpc": "0.00119", | ||
| "current_price": "83.8597", | ||
| "lastday_price": "83.76", | ||
| "change_today": "0.00119" | ||
| }, | ||
| { | ||
| "asset_id": "0000", | ||
| "symbol": "COIN", | ||
| "qty": "10", | ||
| "current_price": "182.00" | ||
| } | ||
| ]"#; |
There was a problem hiding this comment.
Replace the live broker fixture with synthetic data.
This test constant is documented as captured from the issuer's live broker account and includes actual quantity, market value, cost basis, and P/L fields. That's confidential financial data and isn't needed to validate deserialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/alpaca.rs` around lines 147 - 177, The SAMPLE_POSITIONS_JSON constant
contains real, sensitive broker/account values; replace its contents with
synthetic, non-identifying test data while preserving the exact JSON structure
and field names used for deserialization (e.g., "asset_id", "symbol", "qty",
"current_price", "avg_entry_price", "market_value", "cost_basis",
"unrealized_pl", etc.). Ensure numeric values remain as strings where currently
represented (e.g., "qty": "123.45") so deserialization types don't change, keep
both position objects (one fully-populated and one minimal) to exercise optional
fields, and update the SAMPLE_POSITIONS_JSON constant accordingly in alpaca.rs
(const SAMPLE_POSITIONS_JSON) so tests validate shape not real data.
Comment on lines
+224
to
228
| if quote.price <= 0.0 { | ||
| return Err(AppError::BadRequest(format!( | ||
| "Zero or negative price for {} (bid={}, ask={}). Market may be closed or data is bad.", | ||
| pair.symbol, quote.bid_price, quote.ask_price | ||
| "Zero or negative broker mark for {} (price={}). Market may be closed or data is bad.", | ||
| pair.symbol, quote.price | ||
| ))); |
There was a problem hiding this comment.
Reject non-finite marks at the guard boundary.
This check should also reject non-finite values so malformed upstream marks don’t propagate into a later internal-error path.
Suggested fix
- if quote.price <= 0.0 {
+ if !quote.price.is_finite() || quote.price <= 0.0 {
return Err(AppError::BadRequest(format!(
"Zero or negative broker mark for {} (price={}). Market may be closed or data is bad.",
pair.symbol, quote.price
)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if quote.price <= 0.0 { | |
| return Err(AppError::BadRequest(format!( | |
| "Zero or negative price for {} (bid={}, ask={}). Market may be closed or data is bad.", | |
| pair.symbol, quote.bid_price, quote.ask_price | |
| "Zero or negative broker mark for {} (price={}). Market may be closed or data is bad.", | |
| pair.symbol, quote.price | |
| ))); | |
| if !quote.price.is_finite() || quote.price <= 0.0 { | |
| return Err(AppError::BadRequest(format!( | |
| "Zero or negative broker mark for {} (price={}). Market may be closed or data is bad.", | |
| pair.symbol, quote.price | |
| ))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 224 - 228, The guard currently only rejects
non-positive marks but lets non-finite values pass; update the condition in the
same block that returns AppError::BadRequest to also check for non-finite prices
by using quote.price.is_finite(), e.g. change the if that uses quote.price <=
0.0 to something like if !quote.price.is_finite() || quote.price <= 0.0 { ... }
so malformed upstream marks are rejected at the boundary (referencing
quote.price, pair.symbol and the AppError::BadRequest return).
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.
Caution
DO NOT DEPLOY until reviewed. This PR changes what price the oracle signs in production. A regression here means we sign wrong prices for live trading orders. See verification below; please replicate locally before approving.
Why
The deployed oracle calls Alpaca's Market Data API with no
feedparam, which silently defaults to the free-tier IEX-only feed. IEX has thin volume per name and freezes outside its session, so the bid we sign drifts well off the real consolidated NBBO. Live examples observed pre-market today:Concretely: a real user querying SPYM today saw 81.16 vs a market close of 83.x, hence this fix.
st0x.liquidity-monitoralready routes around the same problem — it reads each position'scurrent_pricefrom the Broker API rather than paying for the SIP Market Data subscription tier (seeliquidity-monitor/src/routes/refresh.rs:57-58: "This avoids needing a separate Market Data API subscription for SIP quotes"). This PR mirrors that approach in the oracle server.What changes
src/alpaca.rs:AlpacaClientnow hitsbroker-api.alpaca.markets/v1/trading/accounts/{id}/positionswith HTTP Basic auth. Drops the per-symbollatest_quote()call in favour of one positions fetch per poll cycle that returns marks for every held symbol.QuoteData:bid_price/ask_pricecollapse to a singleprice: f64(the broker mark).t: DateTime<Utc>is now fetch time rather than an exchange-stamped quote time, because the broker positions endpoint exposes no per-mark timestamp. Polling cadence (poll_interval_secs, currently 10) bounds how stale that is in practice.src/lib.rs:build_response_from_quotesigns the mark directly. Both buy and sell directions use the same number; sells still invert via Rain Float inbuild_context.src/oracle.rs: docstring forbuild_contextupdated to reflect the broker-mark semantics.src/main.rs/.env.example: new required env varALPACA_BROKER_ACCOUNT_ID— the issuer's brokerage account ID. Startup fails loud if anyconfig.tomlsymbol has no current position in that account.No on-the-wire changes for clients:
/context/v1still returns the v1 schema ([version, price, publish_time]) and is binary-compatible with deployed strategies. The only thing that changes is which price gets signed.Verification (please replicate before approving)
examples/probe_local.rsis a small smoke client that hits a locally-running server and cross-checks the signed price against the broker's reportedcurrent_pricefor the same symbol.Output for every one of the 11 registered symbols, run today:
All 11 symbols matched broker mark with 0.0000% drift. The probe asserts
<1%drift and exits non-zero if it ever exceeds that.Test plan
cargo test— 25 unit + 7 integration tests passcargo clippy -- -D warningscleancargo fmt --checkcleanALPACA_BROKER_ACCOUNT_IDwill be set in fly secrets before mergeSummary by CodeRabbit
New Features
Configuration