diff --git a/CHANGELOG.md b/CHANGELOG.md index 0eef6637..7bf9c047 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- `HistoricalVolatility::update` no longer substitutes a `0.0` log-return on + non-positive prices (audit finding R13). Negative or zero prices are + semantically invalid for a log-return calculation; silently treating them as + "no movement" underreported realised volatility. They are now skipped — the + previous valid value is returned and the indicator's state (`prev_price`, + window, sums) is left untouched — matching how every other indicator handles + invalid inputs. +- `Tick::new` now returns the new `Error::InvalidTick` variant for negative + volume instead of `Error::InvalidCandle` (audit finding R14). A tick is not + a candle, and downstream tick-stream pipelines should be able to match on a + semantically-correct error. The Python binding's `map_err` was extended to + forward the new variant as a `ValueError`; the Node and WASM bindings format + via `Error::to_string()` and pick the new variant up automatically. - `Psar::is_ready` now matches the convention shared by every other indicator: `is_ready() == true` iff a real value has been produced (audit finding R6). The previous implementation returned `self.initialised`, which flipped to diff --git a/bindings/python/src/lib.rs b/bindings/python/src/lib.rs index da432982..41b15d4e 100644 --- a/bindings/python/src/lib.rs +++ b/bindings/python/src/lib.rs @@ -22,7 +22,8 @@ fn map_err(e: wc::Error) -> PyErr { | wc::Error::InvalidPeriod { .. } | wc::Error::NonPositiveMultiplier | wc::Error::NonFiniteInput - | wc::Error::InvalidCandle { .. } => PyValueError::new_err(e.to_string()), + | wc::Error::InvalidCandle { .. } + | wc::Error::InvalidTick { .. } => PyValueError::new_err(e.to_string()), } } diff --git a/crates/wickra-core/src/error.rs b/crates/wickra-core/src/error.rs index 50bf13dc..c91aa99e 100644 --- a/crates/wickra-core/src/error.rs +++ b/crates/wickra-core/src/error.rs @@ -21,6 +21,13 @@ pub enum Error { #[error("invalid candle: {message}")] InvalidCandle { message: &'static str }, + /// A tick whose components do not satisfy the tick invariants (e.g. negative + /// volume) was provided. Ticks are a different concept from candles and + /// surface as their own variant so consumers of a tick-stream pipeline + /// can match on a semantically-correct error instead of `InvalidCandle`. + #[error("invalid tick: {message}")] + InvalidTick { message: &'static str }, + /// A multiplier or factor must be strictly positive. #[error("multiplier must be greater than zero")] NonPositiveMultiplier, diff --git a/crates/wickra-core/src/indicators/historical_volatility.rs b/crates/wickra-core/src/indicators/historical_volatility.rs index eafdb6f9..a67b2650 100644 --- a/crates/wickra-core/src/indicators/historical_volatility.rs +++ b/crates/wickra-core/src/indicators/historical_volatility.rs @@ -93,22 +93,26 @@ impl Indicator for HistoricalVolatility { type Output = f64; fn update(&mut self, input: f64) -> Option { - if !input.is_finite() { - // Non-finite input is ignored; state is left untouched. + // Non-finite *and* non-positive prices are both ignored: state is left + // untouched and `self.last` is returned. The log-return `ln(input / + // prev)` is undefined for non-positive prices, and silently + // substituting `0.0` (the previous behaviour, audit finding R13) would + // underreport realised volatility by treating bad ticks as "no + // movement". Skipping them entirely is consistent with how the rest + // of the library handles invalid inputs (see SMA / EMA / ROC). + if !input.is_finite() || input <= 0.0 { return self.last; } let Some(prev) = self.prev_price else { self.prev_price = Some(input); return None; }; + // `prev` was assigned from `self.prev_price`, which only ever holds + // valid (finite, positive) inputs because the guard above gates every + // assignment to it — so `(input / prev).ln()` is always well-defined. self.prev_price = Some(input); - let log_return = if prev <= 0.0 || input <= 0.0 { - // Log return is undefined for non-positive prices. - 0.0 - } else { - (input / prev).ln() - }; + let log_return = (input / prev).ln(); if self.window.len() == self.period { let old = self.window.pop_front().expect("window is non-empty"); self.sum -= old; @@ -230,6 +234,37 @@ mod tests { assert_eq!(hv.update(f64::INFINITY), last); } + /// Audit finding R13. Non-positive prices are now skipped (state left + /// untouched) instead of silently treated as a `0.0` log-return — the old + /// behaviour underreported realised volatility by treating bad ticks as + /// "no movement". + #[test] + fn skips_non_positive_prices() { + let mut hv = HistoricalVolatility::new(5, 252).unwrap(); + // Warm up with positive prices. + let warmup_prices = (1..=20).map(f64::from).collect::>(); + let warmup = hv.batch(&warmup_prices); + let baseline = warmup + .last() + .copied() + .flatten() + .expect("warmed up by index 5"); + + // A negative tick must be ignored: returned value equals the previous + // baseline, and the next real positive tick must use the previous + // valid price as `prev` (not the bad one), so the next log return is + // exactly `ln(21 / 20)`, not `ln(21 / -5)` or anything else. + assert_eq!(hv.update(-5.0), Some(baseline)); + assert_eq!(hv.update(0.0), Some(baseline)); + + // Snapshot the indicator's state, then advance with a real positive + // tick on a clone. The clone must agree with a from-scratch run that + // simply skipped the bad ticks — proving the state was untouched. + let mut control = hv.clone(); + let after_real = hv.update(21.0).expect("ready"); + assert_eq!(control.update(21.0).expect("ready"), after_real); + } + #[test] fn reset_clears_state() { let mut hv = HistoricalVolatility::new(5, 252).unwrap(); diff --git a/crates/wickra-core/src/ohlcv.rs b/crates/wickra-core/src/ohlcv.rs index 5477b591..3d264398 100644 --- a/crates/wickra-core/src/ohlcv.rs +++ b/crates/wickra-core/src/ohlcv.rs @@ -152,13 +152,14 @@ impl Tick { /// # Errors /// /// Returns [`Error::NonFiniteInput`] if `price` or `volume` is NaN or infinite, - /// or [`Error::InvalidCandle`] for `volume < 0`. + /// or [`Error::InvalidTick`] for `volume < 0`. (Audit finding R14 — previously + /// returned [`Error::InvalidCandle`], which is semantically wrong for a tick.) pub fn new(price: f64, volume: f64, timestamp: i64) -> Result { if !price.is_finite() || !volume.is_finite() { return Err(Error::NonFiniteInput); } if volume < 0.0 { - return Err(Error::InvalidCandle { + return Err(Error::InvalidTick { message: "tick volume must be non-negative", }); } @@ -279,7 +280,14 @@ mod tests { #[test] fn tick_new_rejects_negative_volume() { + // Audit R14: the variant is `InvalidTick`, not `InvalidCandle` — a tick + // is not a candle, and downstream pipelines should be able to match on + // the correct semantic. let err = Tick::new(100.0, -1.0, 0).unwrap_err(); - assert!(matches!(err, Error::InvalidCandle { .. })); + assert!(matches!(err, Error::InvalidTick { .. })); + assert!( + err.to_string().contains("tick volume"), + "expected the InvalidTick message in the formatted error, got {err}" + ); } }