Refactor architecture and integrate Smith-Wilson extrapolation - #120
Merged
Conversation
|
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 (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughRemoves the convex-traits crate, adds convex-ports and convex-core IDs, rewires engine/adapters/tests to new ports/ID locations, adds UFR-convergence extrapolation and BuiltCurve inner caching, updates monotone-convex math, and refactors analytics to use bond cash flows and compounding-aware z-spreads. ChangesPort and type migration
Curve infrastructure with UFR convergence and cached internals
Analytics calculation updates around bond cash flows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/convex-math/src/extrapolation/smith_wilson.rs (1)
190-216:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
last_derivativeis ignored, so this still fails CI and drops the LLP slope information.The current implementation does two bad things at once: it hard-fails
cargo doc/clippybecauselast_derivativeis unused, and it discards the boundary slope thatDiscreteCurvenow threads through for the extrapolation handoff. That means the post-LLP curve is generally not tangent to the in-range curve even though the API suggests it should be. At minimum this needs to stop passing an unused parameter; the real fix is to make the extrapolated shape honor the boundary derivative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-math/src/extrapolation/smith_wilson.rs` around lines 190 - 216, The extrapolate method currently ignores the last_derivative parameter and thus fails linting and loses slope continuity; update extrapolate to incorporate last_derivative so the extrapolated zero-rate is tangent at last_t: keep existing alpha, tau, convergence and ufr_implied calculations, but add a local linear tangent target value = last_value + last_derivative * tau (or adjust for log/zero-rate semantics if needed) and blend between that tangent target and the ufr_implied value (using the same convergence factor) so at tau->0 you recover last_value and slope, and as tau grows you converge to UFR; this both uses last_derivative (removing the unused-variable error) and ensures the extrapolated curve matches the boundary slope from DiscreteCurve.Source: Pipeline failures
crates/convex-engine/src/pricing_router.rs (1)
1656-1675:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis helper no longer matches the flat-extrapolation assertion later in the test module.
Calling
rebuild_inner()switches this test curve to MonotoneConvex + Smith-Wilson behavior, so extrapolation past 30Y will converge away from the last pillar instead of staying flat.test_built_curve_interpolation()still expects the old flat result on Line 2364, so this helper change leaves that test out of sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-engine/src/pricing_router.rs` around lines 1656 - 1675, The helper constructs a BuiltCurve with explicit points but then calls rebuild_inner(), which converts it to a MonotoneConvex/Smith-Wilson representation and changes extrapolation behavior; revert this by removing the call to BuiltCurve::rebuild_inner() (leave built.inner as None) so the test_built_curve_interpolation() continues to see flat extrapolation past the last 30Y pillar, or alternatively update the test expectations to match the new monotone/Smith-Wilson extrapolation if that behavior is intended; locate the helper that creates BuiltCurve (the block that sets curve_id, reference_date, points, built_at, inputs_hash, inner) and remove the built.rebuild_inner() invocation.crates/convex-server/tests/websocket_integration_tests.rs (1)
70-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the fixed sleeps from the websocket tests.
These 50–100ms waits make the suite timing-dependent; on a slower CI runner the server may not be ready yet or the broadcast may arrive after the assertion window. Replace them with a readiness poll or a deterministic event from the server.
Also applies to: 469-470, 557-558, 659-660, 761-762
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-server/tests/websocket_integration_tests.rs` around lines 70 - 80, The fixed tokio::time::sleep calls (e.g., after TcpListener::bind / before connecting to the test server) make the tests timing-dependent; remove those sleeps and replace them with an explicit readiness check—either have the spawned server send a ready signal via a oneshot channel (create a tokio::sync::oneshot::channel and send after the axum server is ready) or poll-connect in a loop (attempt TcpStream::connect to listener.local_addr() with a small sleep/retry until success or timeout). Update the instances around TcpListener::bind, tokio::spawn/axum::serve and the other similar locations (lines referenced) to use the readiness signal/poll instead of tokio::time::sleep.
🧹 Nitpick comments (1)
crates/convex-server/tests/websocket_integration_tests.rs (1)
26-47: Ports API types still match the struct-literal setup (MarketDataProvider/ReferenceDataProvider/EngineConfig).
MarketDataProviderandReferenceDataProviderare still concretepub structs (not traits) withArc<dyn ...>fields, andEngineConfigremains apub structwithimpl Default, so this test’s struct-literal construction matches the ports API shape.
- Replace fixed
sleep-based timing in the websocket integration tests with a readiness/polling condition to reduce intermittent CI flakiness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-server/tests/websocket_integration_tests.rs` around lines 26 - 47, The tests use fixed sleep delays (e.g., tokio::time::sleep or std::thread::sleep) in the websocket integration tests which causes flaky CI; replace those sleeps with a readiness/polling loop that waits for a concrete condition (socket connected, handshake completed, or specific message received) with a bounded timeout. Locate the websocket test code and replace calls to sleep with a loop that polls the actual readiness predicate (checking the connection state, reading from the test socket, or awaiting a specific server response) using tokio::time::interval or tokio::time::timeout and fail the test if the timeout elapses; reference the existing test helpers and the create_test_engine function to access the engine/socket and ensure the readiness check ties to an observable state rather than elapsed time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/convex-analytics/src/spreads/asw/par_par.rs`:
- Around line 197-210: calculate_annuity() currently uses a hard-coded
year_fraction (1 / coupon_frequency) while calculate() and implied_price()
accumulate annuity using per-cash-flow accrual-aware tau values, causing
mismatch on stub/irregular schedules; refactor by extracting the per-cash-flow
tau computation into a shared helper (e.g., compute_tau_for_cashflow or
tau_for(cf, payments_per_year)) and replace the hard-coded year_fraction in
calculate_annuity() with calls to that helper so calculate(), implied_price(),
and annuity() all use the same tau logic when iterating cash_flows and computing
annuity.
- Around line 153-164: The computed first-period accrual fraction `tau` uses
cashflow accrual_start/accrual_end directly (see the `tau` match using
`cf.accrual_start`/`cf.accrual_end` and `payments_per_year`), causing the first
post-settlement term to be measured from the coupon period start instead of from
`settlement`; clamp the accrual window start to `settlement` when computing
`days` for the first accrual window so the first ASW period equals the traded
stub (i.e., replace `start` with `max(start, settlement)` or otherwise take `let
start = if start < settlement { settlement } else { start }` before
`start.days_between(&end)`), and apply the same change in both `par_par.rs` and
`proceeds.rs`.
In `@crates/convex-analytics/src/spreads/zspread.rs`:
- Around line 96-100: The docs currently claim Z-spread pricing uses exp(-Z × t)
but the builder's with_compounding method (and its compounding field of type
convex_core::types::Compounding) supports simple and periodic compounding too;
update the public documentation for the Z-spread type and the with_compounding()
method to list the exact discount adjustments for each Compounding variant
(e.g., continuous: exp(-Z * t), simple: 1 / (1 + Z * t), periodic (n
periods/year): (1 + Z/n)^(-n*t) or equivalent), so callers understand which
formula is applied for each compounding mode.
- Around line 171-179: The pricing paths (price_with_spread and
calculate_from_cash_flows) currently use hard-coded continuous discounting while
calculate() respects self.compounding; extract the compounding match into a
shared helper (e.g., a method or private fn like spread_df(&self, z: f64, dt:
f64) -> f64) that implements the match on convex_core::types::Compounding
(Continuous, Simple, others using periods_per_year()) and then replace the
inline matches in calculate(), price_with_spread(), and
calculate_from_cash_flows() to call this helper so all pricing and DV01 helpers
honor self.compounding consistently.
In `@crates/convex-core/src/types/currency.rs`:
- Around line 3-6: Remove the unused import USCalendar to fix the -D warnings
compilation failure: in crates/convex-core/src/types/currency.rs delete the
USCalendar entry from the use list so only actually used calendars (e.g.,
Calendar, JapanCalendar, SIFMACalendar, Target2Calendar, UKCalendar,
WeekendCalendar) remain; verify default_calendar (which uses SIFMACalendar for
USD) and any other references still compile cleanly after removing USCalendar.
In `@crates/convex-curves/src/curves/discrete.rs`:
- Around line 289-298: The Smith-Wilson branch in
ExtrapolationMethod::SmithWilson is blindly passing DiscreteCurve values
(last_value, last_derivative from interpolator.derivative at max_tenor) as if
they are continuously compounded zero rates; restrict or convert: either
validate that the DiscreteCurve is a zero-rate curve and return Err or panic
early when used on non-zero-rate representations, or convert the terminal
value/derivative to zero-rate form before calling sw.extrapolate and convert the
extrapolated result back to the curve's representation. To convert
discount-factor-like values use r = -ln(df)/T and dr/dT = -(d(df)/dT)/(df*T) (or
compute derivative from last_derivative accordingly) at llp (max_tenor), call
sw.extrapolate with r and dr/dT, then map the returned zero-rate back to
discount factor via df = exp(-r*T); implement this check/convert around the code
using DiscreteCurve, ExtrapolationMethod::SmithWilson, last_value,
last_derivative, interpolator.derivative, sw.extrapolate and max_tenor.
In `@crates/convex-engine/src/cache.rs`:
- Around line 9-10: Run rustfmt by executing cargo fmt --all to fix import
formatting and ordering issues in the file that imports InstrumentId and
RawQuote (the use statements referencing convex_core::ids::InstrumentId and
crate::ports::market_data::RawQuote); after formatting, commit the changes and
re-run CI to ensure the import formatting check passes.
In `@crates/convex-engine/src/calc_graph.rs`:
- Around line 39-41: Import ordering is not rustfmt-compliant causing CI
failures; run rustfmt (cargo fmt) on calc_graph.rs to reorder the use statements
so they follow rustfmt's canonical ordering (group and alphabetize imports),
ensuring the lines importing Date, crate::ports::config::{NodeConfig,
UpdateFrequency}, and convex_core::ids::* are reordered/formatted accordingly;
commit the formatted file to unblock the pipeline.
In `@crates/convex-engine/src/context.rs`:
- Around line 7-8: Imports in context.rs are failing CI formatting checks; run
the Rust formatter to reorder and format imports. Execute `cargo fmt --all` (or
run your IDE's Rustfmt) to apply the required import formatting so lines like
`use convex_core::ids::{CurveId, InstrumentId};` and `use
crate::ports::market_data::RawQuote;` match project style and pass CI.
In `@crates/convex-engine/src/curve_builder.rs`:
- Around line 40-43: interpolate_rate currently trusts self.inner if present but
rebuild_inner only replaces self.inner on success, so stale Arc<DiscreteCurve>
can persist after points change; modify rebuild_inner to set self.inner = None
at the start of the rebuild attempt (clear the cached Arc<DiscreteCurve> before
validating/constructing a new one) and change rebuild_inner's signature to
return a Result indicating success or failure so callers (and interpolate_rate)
can react to rebuild errors rather than silently using the old curve; update
caller sites of rebuild_inner and any logic in interpolate_rate to handle the
Result and avoid using self.inner when rebuild failed.
In `@crates/convex-math/src/interpolation/monotone_convex.rs`:
- Around line 208-214: forward_rate() clamps negative forward rates to zero but
interpolate() currently integrates the unclamped quadratic (using variables
f_discrete, f_lo, f_hi and x), allowing the integrated curve z(t) to be built
from negative area; update interpolate() (and the second occurrence around the
other block) to compute the quadratic value (e.g., f_raw = f_discrete + ... )
and then apply the same non-negative clamp (f = f_raw.max(0.0)) before returning
or using it so that interpolate() and forward_rate() describe the same
non-negative curve, preserving
InterpolationMethod::guarantees_positive_forwards(). Ensure both occurrences use
the clamped value.
---
Outside diff comments:
In `@crates/convex-engine/src/pricing_router.rs`:
- Around line 1656-1675: The helper constructs a BuiltCurve with explicit points
but then calls rebuild_inner(), which converts it to a
MonotoneConvex/Smith-Wilson representation and changes extrapolation behavior;
revert this by removing the call to BuiltCurve::rebuild_inner() (leave
built.inner as None) so the test_built_curve_interpolation() continues to see
flat extrapolation past the last 30Y pillar, or alternatively update the test
expectations to match the new monotone/Smith-Wilson extrapolation if that
behavior is intended; locate the helper that creates BuiltCurve (the block that
sets curve_id, reference_date, points, built_at, inputs_hash, inner) and remove
the built.rebuild_inner() invocation.
In `@crates/convex-math/src/extrapolation/smith_wilson.rs`:
- Around line 190-216: The extrapolate method currently ignores the
last_derivative parameter and thus fails linting and loses slope continuity;
update extrapolate to incorporate last_derivative so the extrapolated zero-rate
is tangent at last_t: keep existing alpha, tau, convergence and ufr_implied
calculations, but add a local linear tangent target value = last_value +
last_derivative * tau (or adjust for log/zero-rate semantics if needed) and
blend between that tangent target and the ufr_implied value (using the same
convergence factor) so at tau->0 you recover last_value and slope, and as tau
grows you converge to UFR; this both uses last_derivative (removing the
unused-variable error) and ensures the extrapolated curve matches the boundary
slope from DiscreteCurve.
In `@crates/convex-server/tests/websocket_integration_tests.rs`:
- Around line 70-80: The fixed tokio::time::sleep calls (e.g., after
TcpListener::bind / before connecting to the test server) make the tests
timing-dependent; remove those sleeps and replace them with an explicit
readiness check—either have the spawned server send a ready signal via a oneshot
channel (create a tokio::sync::oneshot::channel and send after the axum server
is ready) or poll-connect in a loop (attempt TcpStream::connect to
listener.local_addr() with a small sleep/retry until success or timeout). Update
the instances around TcpListener::bind, tokio::spawn/axum::serve and the other
similar locations (lines referenced) to use the readiness signal/poll instead of
tokio::time::sleep.
---
Nitpick comments:
In `@crates/convex-server/tests/websocket_integration_tests.rs`:
- Around line 26-47: The tests use fixed sleep delays (e.g., tokio::time::sleep
or std::thread::sleep) in the websocket integration tests which causes flaky CI;
replace those sleeps with a readiness/polling loop that waits for a concrete
condition (socket connected, handshake completed, or specific message received)
with a bounded timeout. Locate the websocket test code and replace calls to
sleep with a loop that polls the actual readiness predicate (checking the
connection state, reading from the test socket, or awaiting a specific server
response) using tokio::time::interval or tokio::time::timeout and fail the test
if the timeout elapses; reference the existing test helpers and the
create_test_engine function to access the engine/socket and ensure the readiness
check ties to an observable state rather than elapsed time.
🪄 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: 809b2bd6-b5f1-4ba2-9ee4-138d6ecb8f3b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
Cargo.tomlcrates/convex-analytics/src/spreads/asw/par_par.rscrates/convex-analytics/src/spreads/asw/proceeds.rscrates/convex-analytics/src/spreads/zspread.rscrates/convex-core/src/ids.rscrates/convex-core/src/lib.rscrates/convex-core/src/types/currency.rscrates/convex-curves/src/curves/discrete.rscrates/convex-curves/src/lib.rscrates/convex-engine/Cargo.tomlcrates/convex-engine/benches/pricing_benchmarks.rscrates/convex-engine/src/builder.rscrates/convex-engine/src/cache.rscrates/convex-engine/src/calc_graph.rscrates/convex-engine/src/context.rscrates/convex-engine/src/curve_builder.rscrates/convex-engine/src/error.rscrates/convex-engine/src/etf_pricing.rscrates/convex-engine/src/lib.rscrates/convex-engine/src/market_data_listener.rscrates/convex-engine/src/portfolio_analytics.rscrates/convex-engine/src/ports/config.rscrates/convex-engine/src/ports/error.rscrates/convex-engine/src/ports/market_data.rscrates/convex-engine/src/ports/mock.rscrates/convex-engine/src/ports/mod.rscrates/convex-engine/src/ports/output.rscrates/convex-engine/src/ports/reference_data.rscrates/convex-engine/src/ports/storage.rscrates/convex-engine/src/pricing_router.rscrates/convex-engine/src/reactive.rscrates/convex-engine/src/scheduler.rscrates/convex-ext-file/Cargo.tomlcrates/convex-ext-file/src/lib.rscrates/convex-ext-file/src/market_data.rscrates/convex-ext-file/src/reference_data.rscrates/convex-ext-redb/Cargo.tomlcrates/convex-ext-redb/src/lib.rscrates/convex-ffi/Cargo.tomlcrates/convex-math/src/extrapolation/smith_wilson.rscrates/convex-math/src/interpolation/monotone_convex.rscrates/convex-mcp/Cargo.tomlcrates/convex-server/Cargo.tomlcrates/convex-server/src/handlers.rscrates/convex-server/src/main.rscrates/convex-server/src/websocket.rscrates/convex-server/tests/api_integration_tests.rscrates/convex-server/tests/websocket_integration_tests.rscrates/convex-traits/Cargo.tomlcrates/convex-traits/src/lib.rs
💤 Files with no reviewable changes (2)
- crates/convex-traits/src/lib.rs
- crates/convex-traits/Cargo.toml
Comment on lines
+208
to
214
| let f_discrete = self.discrete_forwards[i]; | ||
|
|
||
| let f = f_discrete | ||
| + (f_lo - f_discrete) * (1.0 - 4.0 * x + 3.0 * x * x) | ||
| + (f_hi - f_discrete) * (-2.0 * x + 3.0 * x * x); | ||
|
|
||
| Ok(f.max(0.0)) // Ensure non-negative |
There was a problem hiding this comment.
forward_rate() and interpolate() can now describe different curves.
forward_rate() clamps negative values to zero, but interpolate() integrates the unclamped quadratic. Once this new unconstrained shape dips below zero, callers can observe non-negative forwards while z(t) is still built from negative forward area. That breaks the positive-forward contract exposed by InterpolationMethod::guarantees_positive_forwards() and makes downstream differentiation inconsistent.
Also applies to: 294-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/convex-math/src/interpolation/monotone_convex.rs` around lines 208 -
214, forward_rate() clamps negative forward rates to zero but interpolate()
currently integrates the unclamped quadratic (using variables f_discrete, f_lo,
f_hi and x), allowing the integrated curve z(t) to be built from negative area;
update interpolate() (and the second occurrence around the other block) to
compute the quadratic value (e.g., f_raw = f_discrete + ... ) and then apply the
same non-negative clamp (f = f_raw.max(0.0)) before returning or using it so
that interpolate() and forward_rate() describe the same non-negative curve,
preserving InterpolationMethod::guarantees_positive_forwards(). Ensure both
occurrences use the clamped value.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 issues found across 51 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/convex-curves/src/curves/discrete.rs">
<violation number="1" location="crates/convex-curves/src/curves/discrete.rs:294">
P1: The Smith-Wilson branch passes `last_value` directly to the extrapolator assuming it is a continuously compounded zero rate. However, `DiscreteCurve` can also represent `DiscountFactor` or other value types. For non-zero-rate curves, this produces numerically incorrect extrapolations. Either validate the `ValueType` up front (rejecting non-zero-rate) or convert through a zero-rate representation before calling Smith-Wilson.</violation>
<violation number="2" location="crates/convex-curves/src/curves/discrete.rs:296">
P2: Passing unvalidated `alpha` to `SmithWilson::new`, which panics if alpha ≤ 0. Since `ExtrapolationMethod::SmithWilson(ufr, alpha)` has no construction-time validation, a zero or negative alpha will cause a runtime panic on extrapolation rather than an error result.</violation>
</file>
<file name="crates/convex-analytics/src/spreads/zspread.rs">
<violation number="1" location="crates/convex-analytics/src/spreads/zspread.rs:171">
P1: Compounding is applied only in `calculate`, while repricing/risk paths still assume continuous discounting, causing inconsistent Z-spread and DV01 behavior when non-continuous compounding is selected.</violation>
</file>
<file name="crates/convex-analytics/src/spreads/asw/par_par.rs">
<violation number="1" location="crates/convex-analytics/src/spreads/asw/par_par.rs:150">
P2: `calculate_annuity()` hard-codes `year_fraction = 1/payments_per_year` for every period, but `annuity_and_mismatch_pct()` now uses accrual-aware `tau` values. On stub or irregular schedules the public `annuity()` result will disagree with the annuity used in `calculate()` and `implied_price()`. Extract the tau computation into a shared helper and use it in both paths.</violation>
</file>
<file name="crates/convex-core/src/types/currency.rs">
<violation number="1" location="crates/convex-core/src/types/currency.rs:214">
P2: Several G10 currencies (CHF, CAD, AUD, NZD, SEK, NOK) fall back to `WeekendCalendar`, which only excludes weekends — no national holidays. In a pricing context this can produce incorrect business-day rolling, settlement dates, and curve interpolation for these currencies. Consider adding specific calendars or using `JointCalendar`/`CustomCalendarBuilder` for at least the G10 currencies.</violation>
</file>
<file name="crates/convex-math/src/interpolation/monotone_convex.rs">
<violation number="1" location="crates/convex-math/src/interpolation/monotone_convex.rs:300">
P1: The new quadratic forward interpolation can become negative without monotonicity limiting, and `interpolate()` integrates those negative values while `forward_rate()` clamps them, creating inconsistent curve behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Ok(z_final) | ||
|
|
||
| // Exact integral of the Hagan-West quadratic forward rate | ||
| let integral_x = (x - 2.0 * x * x + x * x * x) * f_lo |
There was a problem hiding this comment.
P1: The new quadratic forward interpolation can become negative without monotonicity limiting, and interpolate() integrates those negative values while forward_rate() clamps them, creating inconsistent curve behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-math/src/interpolation/monotone_convex.rs, line 300:
<comment>The new quadratic forward interpolation can become negative without monotonicity limiting, and `interpolate()` integrates those negative values while `forward_rate()` clamps them, creating inconsistent curve behavior.</comment>
<file context>
@@ -312,25 +288,22 @@ impl Interpolator for MonotoneConvex {
- Ok(z_final)
+
+ // Exact integral of the Hagan-West quadratic forward rate
+ let integral_x = (x - 2.0 * x * x + x * x * x) * f_lo
+ + (-x * x + x * x * x) * f_hi
+ + (3.0 * x * x - 2.0 * x * x * x) * f_discrete;
</file context>
default_calendar() had zero call sites and was the only reason currency.rs pulled in the calendar types (one of which, USCalendar, was already unused). Remove the speculative method and its imports; it can be reintroduced where a calendar is actually selected if needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ot the engine The previous refactor folded the hexagonal port traits into convex-engine (crate::ports). That inverted the dependency direction: adapters (convex-ext-file, convex-ext-redb) had to depend on the whole engine just to implement a trait, and convex-ffi / convex-mcp were given an engine dependency they never used. It also created a dev-dependency cycle (convex-engine ->dev-> convex-ext-file -> convex-engine), which pulled two copies of the engine into the test graph and forced a 246-line duplicate set of empty mocks (ports/mock.rs) because the engine's own tests could not use the adapter empties. Extract a thin convex-ports crate (trait definitions only, no runtime deps) that both the engine and the adapters depend down on: - New crate convex-ports holding market_data, reference_data, storage, output, config, error (moved verbatim; crate::ports:: -> crate:: internally). - convex-engine re-exports it as `pub use convex_ports as ports`, so existing convex_engine::ports::* paths (server, benches) keep working unchanged. - convex-ext-file / convex-ext-redb now depend on convex-ports, not convex-engine. - convex-ffi / convex-mcp drop the unused convex-engine dependency. - Delete the duplicate ports/mock.rs and point the reactive test back at the convex-ext-file empties (now possible: the cycle is gone). Drop the unused mockall dev-dependency and the now-unused async-trait dependency in the engine. cargo check --workspace --all-targets passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both par-par and proceeds ASW reconstructed the period year fraction with a copy-pasted heuristic: actual-days/365 when the period length differed from 365/freq by more than a magic 15 days, else the nominal 1/freq. That ignored the instrument's own day count and mispriced stubs and non-Act/365 conventions. Replace it with a single shared `coupon_year_fraction` helper that uses the bond's day-count convention over the cash flow's accrual boundaries, falling back to the nominal 1/freq when accrual dates are absent. Also drop the dead `frequency_to_months` helpers (left unused after the cash-flow refactor) and the unused `tau` / `maturity` bindings in par_par. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ence
The `SmithWilson` extrapolator was not Smith-Wilson: it was an ad-hoc
exponential blend of zero rates that ignored its `last_derivative` argument
(an unused-variable warning), so it was not even continuous in the forward at
the last liquid point, while its docs claimed EIOPA / Solvency II regulatory
compliance and C-infinity continuity. True Smith-Wilson fits a Wilson-kernel
curve to all input instruments and cannot be expressed as a pointwise tail
extrapolator, so the name was actively misleading.
Rename it to `UfrConvergence` and implement it honestly: the instantaneous
forward decays exponentially from its observed value at the LLP towards the UFR,
and the zero rate is the exact integral of that forward. This now uses
`last_derivative` (continuous in level and forward at the LLP) and is documented
as a heuristic, explicitly not EIOPA Smith-Wilson. Drops the dead Wilson-kernel
and convergence-weight helpers and the unused EIOPA preset constructors.
Renames the `ExtrapolationMethod::SmithWilson` variants (convex-math and
convex-curves) to `UfrConvergence { ufr, alpha }` accordingly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… drop
rebuild_inner() baked EUR insurance parameters
(UfrConvergence { ufr: 0.042, alpha: 0.1 }) into every curve regardless of
currency, and deduplicated pillars by dropping any tenor not strictly greater
than the previous one -- silently discarding unsorted or out-of-order input
points.
- Add an `extrapolation` field to BuiltCurve and a configurable default on
CurveBuilder (`with_extrapolation`, defaulting to flat-forward instead of a
hard-coded UFR). Bumped-curve clones used for KR01/CS01 sensitivities now
preserve the original curve's extrapolation automatically.
- rebuild_inner now sorts pillars by tenor and collapses only exact-duplicate
tenors (keeping the latest rate); distinct points are never dropped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ctor) The architecture refactor was applied with mechanical scripts that left large swaths of non-rustfmt-compliant code (one-line if/return bodies, trailing whitespace, hand-wrapped argument lists), which would fail the CI `cargo fmt --all -- --check` gate. Reformat the affected files; only files the branch had made non-compliant are touched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- discrete.rs: the new UFR-convergence match arm had a `use` after statements (clippy::items_after_statements under convex-curves' `#![warn(pedantic)]`); call the trait method via its fully-qualified path instead. - convex-ports lib docs: `[`module`]: text` list items parse as markdown link reference definitions (clippy "link reference defined in list item"); use an em dash separator. Pre-existing `result_large_err` warnings in convex-engine's market_data_listener are unchanged from main and left out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 49 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/convex-curves/src/curves/discrete.rs">
<violation number="1" location="crates/convex-curves/src/curves/discrete.rs:296">
P2: Passing unvalidated `alpha` to `SmithWilson::new`, which panics if alpha ≤ 0. Since `ExtrapolationMethod::SmithWilson(ufr, alpha)` has no construction-time validation, a zero or negative alpha will cause a runtime panic on extrapolation rather than an error result.</violation>
</file>
<file name="crates/convex-core/src/types/currency.rs">
<violation number="1" location="crates/convex-core/src/types/currency.rs:214">
P2: Several G10 currencies (CHF, CAD, AUD, NZD, SEK, NOK) fall back to `WeekendCalendar`, which only excludes weekends — no national holidays. In a pricing context this can produce incorrect business-day rolling, settlement dates, and curve interpolation for these currencies. Consider adding specific calendars or using `JointCalendar`/`CustomCalendarBuilder` for at least the G10 currencies.</violation>
</file>
<file name="crates/convex-math/src/interpolation/monotone_convex.rs">
<violation number="1" location="crates/convex-math/src/interpolation/monotone_convex.rs:300">
P1: The new quadratic forward interpolation can become negative without monotonicity limiting, and `interpolate()` integrates those negative values while `forward_rate()` clamps them, creating inconsistent curve behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`MarketDataPublisher`'s seven `publish_*` methods returned `Result<(), broadcast::error::SendError<MarketDataUpdate>>`. The error carries the un-delivered update (~152 bytes), tripping clippy's `result_large_err` and bloating the success path of every publish. Box the error behind a `PublishResult` alias and route all seven methods through a single private `publish()` helper (which also removes the duplicated `send(...).map(|_| ())` boilerplate). This was pre-existing debt on main, not introduced by the curve refactor, but it was the last thing keeping `cargo clippy --workspace -- -D warnings` from passing on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/convex-math/src/extrapolation/mod.rs (1)
60-76: Clarify the relationship between the twoExtrapolationMethodenums
convex_math::extrapolation::ExtrapolationMethod={ None, #[default] Flat, Linear, UfrConvergence{..} }, whileconvex_curves::ExtrapolationMethod={ #[default] None, Flat, Linear, FlatForward, UfrConvergence{..} }.- In
convex_curvesdiscrete curves,UfrConvergencedelegates intoconvex_math::extrapolation, butNone/Flat/Linear/FlatForwardare handled directly (withFlatForwardcurrently effectively falling back to flat behavior).- Add/expand docs (or a conversion layer) making it explicit that these enums are not interchangeable and that
Defaultdiffers (convex_math→Flat,convex_curves→None), even thoughCurveBuildersets its owndefault_extrapolation(e.g.,Flatfor rate/credit curves).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-math/src/extrapolation/mod.rs` around lines 60 - 76, Document and/or add an explicit conversion layer clarifying that the two enums convex_math::extrapolation::ExtrapolationMethod and convex_curves::ExtrapolationMethod are distinct and not interchangeable: state that convex_math::extrapolation::ExtrapolationMethod defaults to Flat while convex_curves::ExtrapolationMethod defaults to None, enumerate how each variant maps (e.g., convex_curves::UfrConvergence -> convex_math::UfrConvergence, while None/Flat/Linear/FlatForward are handled locally with FlatForward currently falling back to Flat), and either add conversion functions (e.g., try_from_convex_curves_to_math and from_math_to_curves) or expand the docs in the enum definitions and CurveBuilder/default_extrapolation to explicitly describe expected behavior and defaulting rules so callers are not surprised.crates/convex-analytics/src/spreads/asw/proceeds.rs (1)
279-290: 💤 Low valueConsider adding test coverage for accrual-aware year fraction calculation.
The mock sets
accrual_start: Noneandaccrual_end: Nonefor all generated cash flows, which meanscoupon_year_fractionwill always use the nominal fallback (1 / payments_per_year). This exercises the fallback path but not the day-count-aware calculation introduced inmod.rs.Consider adding a test case where cash flows include accrual boundaries to verify the day-count logic works correctly for stub periods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-analytics/src/spreads/asw/proceeds.rs` around lines 279 - 290, The generated mock cash flows in proceeds.rs set accrual_start and accrual_end to None, so tests only cover the nominal fallback in coupon_year_fraction; add a new unit test that constructs BondCashFlow entries with explicit accrual_start and accrual_end spanning a stub/short period (e.g., first or last coupon) and use those in the same payment_dates -> BondCashFlow mapping used in the proceeds tests to exercise the day-count-aware code in mod.rs; specifically, create flows with non-None accrual_start/accrual_end and assert coupon_year_fraction (or the public function that consumes it) returns the expected fractional year according to the day count convention implemented in mod.rs.crates/convex-analytics/src/spreads/asw/mod.rs (1)
27-42: Day-count parsing accepts"ACT/ACT"; nominal fallback in ASW tests is due to missing accrual dates
DayCountConvention::from_strincrates/convex-core/src/daycounts/mod.rsaccepts"ACT/ACT"(whitespace/case tolerant) and maps it toActActIsda, so parsing won’t fail silently.- In the ASW test mocks in
crates/convex-analytics/src/spreads/asw/proceeds.rs/par_par.rs, theBondCashFlowliterals setaccrual_start: Noneandaccrual_end: None, socoupon_year_fractionalways falls back tonominalregardless ofday_count.- If you want coverage for the accrual-aware year-fraction path, add tests where
BondCashFlowprovidesSome(accrual_start)andSome(accrual_end).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/convex-analytics/src/spreads/asw/mod.rs` around lines 27 - 42, The ASW tests are always hitting the nominal fallback because coupon_year_fraction sees BondCashFlow.accrual_start and accrual_end as None; update the test fixtures in crates/convex-analytics/src/spreads/asw/proceeds.rs and par_par.rs to provide Some(accrual_start) and Some(accrual_end) dates for at least one BondCashFlow so coupon_year_fraction("ACT/ACT", cf, ...) exercises the accrual-aware branch (function coupon_year_fraction) and verifies year_fraction is computed using DayCountConvention parsing rather than using the nominal fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/convex-curves/src/curves/discrete.rs`:
- Around line 289-305: The UFR-convergence branch in
DiscreteCurve.with_extrapolation() passes last_value and last_derivative into
convex_math::extrapolation::UfrConvergence assuming continuously compounded
zero-rate semantics; update with_extrapolation() so that when
ExtrapolationMethod::UfrConvergence is selected it either (A) validates that
self.value_type is ValueType::ZeroRate with Compounding::Continuous and returns
an Err if not, or (B) converts the tail basis to continuous zero rates before
computing last_value = z(LLP) and last_derivative = z'(LLP) and then converts
results back to the curve’s original ValueType; change the UfrConvergence match
arm (the block constructing ext and calling Extrapolator::extrapolate) to use
the validated/converted zero-rate values (referencing DiscreteCurve,
with_extrapolation, ExtrapolationMethod::UfrConvergence, ValueType::ZeroRate,
and Compounding::Continuous).
---
Nitpick comments:
In `@crates/convex-analytics/src/spreads/asw/mod.rs`:
- Around line 27-42: The ASW tests are always hitting the nominal fallback
because coupon_year_fraction sees BondCashFlow.accrual_start and accrual_end as
None; update the test fixtures in
crates/convex-analytics/src/spreads/asw/proceeds.rs and par_par.rs to provide
Some(accrual_start) and Some(accrual_end) dates for at least one BondCashFlow so
coupon_year_fraction("ACT/ACT", cf, ...) exercises the accrual-aware branch
(function coupon_year_fraction) and verifies year_fraction is computed using
DayCountConvention parsing rather than using the nominal fallback.
In `@crates/convex-analytics/src/spreads/asw/proceeds.rs`:
- Around line 279-290: The generated mock cash flows in proceeds.rs set
accrual_start and accrual_end to None, so tests only cover the nominal fallback
in coupon_year_fraction; add a new unit test that constructs BondCashFlow
entries with explicit accrual_start and accrual_end spanning a stub/short period
(e.g., first or last coupon) and use those in the same payment_dates ->
BondCashFlow mapping used in the proceeds tests to exercise the day-count-aware
code in mod.rs; specifically, create flows with non-None
accrual_start/accrual_end and assert coupon_year_fraction (or the public
function that consumes it) returns the expected fractional year according to the
day count convention implemented in mod.rs.
In `@crates/convex-math/src/extrapolation/mod.rs`:
- Around line 60-76: Document and/or add an explicit conversion layer clarifying
that the two enums convex_math::extrapolation::ExtrapolationMethod and
convex_curves::ExtrapolationMethod are distinct and not interchangeable: state
that convex_math::extrapolation::ExtrapolationMethod defaults to Flat while
convex_curves::ExtrapolationMethod defaults to None, enumerate how each variant
maps (e.g., convex_curves::UfrConvergence -> convex_math::UfrConvergence, while
None/Flat/Linear/FlatForward are handled locally with FlatForward currently
falling back to Flat), and either add conversion functions (e.g.,
try_from_convex_curves_to_math and from_math_to_curves) or expand the docs in
the enum definitions and CurveBuilder/default_extrapolation to explicitly
describe expected behavior and defaulting rules so callers are not surprised.
🪄 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: f90f7151-438f-4a5e-a216-3f485db05c13
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
Cargo.tomlcrates/convex-analytics/src/spreads/asw/mod.rscrates/convex-analytics/src/spreads/asw/par_par.rscrates/convex-analytics/src/spreads/asw/proceeds.rscrates/convex-analytics/src/spreads/zspread.rscrates/convex-curves/src/curves/discrete.rscrates/convex-curves/src/lib.rscrates/convex-engine/Cargo.tomlcrates/convex-engine/benches/pricing_benchmarks.rscrates/convex-engine/src/cache.rscrates/convex-engine/src/calc_graph.rscrates/convex-engine/src/context.rscrates/convex-engine/src/curve_builder.rscrates/convex-engine/src/etf_pricing.rscrates/convex-engine/src/lib.rscrates/convex-engine/src/market_data_listener.rscrates/convex-engine/src/portfolio_analytics.rscrates/convex-engine/src/pricing_router.rscrates/convex-engine/src/reactive.rscrates/convex-ext-file/Cargo.tomlcrates/convex-ext-file/src/lib.rscrates/convex-ext-file/src/market_data.rscrates/convex-ext-file/src/reference_data.rscrates/convex-ext-redb/Cargo.tomlcrates/convex-ext-redb/src/lib.rscrates/convex-math/src/extrapolation/linear.rscrates/convex-math/src/extrapolation/mod.rscrates/convex-math/src/extrapolation/smith_wilson.rscrates/convex-math/src/extrapolation/ufr_convergence.rscrates/convex-math/src/interpolation/monotone_convex.rscrates/convex-math/src/lib.rscrates/convex-ports/Cargo.tomlcrates/convex-ports/src/config.rscrates/convex-ports/src/error.rscrates/convex-ports/src/lib.rscrates/convex-ports/src/market_data.rscrates/convex-ports/src/output.rscrates/convex-ports/src/reference_data.rscrates/convex-ports/src/storage.rscrates/convex-server/src/handlers.rscrates/convex-server/src/main.rscrates/convex-server/tests/api_integration_tests.rscrates/convex-server/tests/websocket_integration_tests.rs
💤 Files with no reviewable changes (1)
- crates/convex-math/src/extrapolation/smith_wilson.rs
✅ Files skipped from review due to trivial changes (8)
- crates/convex-ports/src/market_data.rs
- crates/convex-engine/src/calc_graph.rs
- crates/convex-ports/src/output.rs
- crates/convex-server/tests/websocket_integration_tests.rs
- crates/convex-math/src/extrapolation/linear.rs
- crates/convex-ext-file/src/lib.rs
- crates/convex-ext-file/src/reference_data.rs
- crates/convex-engine/src/context.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/convex-engine/src/etf_pricing.rs
- crates/convex-ext-redb/src/lib.rs
- crates/convex-ext-file/src/market_data.rs
- crates/convex-engine/src/cache.rs
- crates/convex-server/src/main.rs
- Cargo.toml
- crates/convex-server/tests/api_integration_tests.rs
- crates/convex-engine/benches/pricing_benchmarks.rs
- crates/convex-server/src/handlers.rs
- crates/convex-engine/src/portfolio_analytics.rs
- crates/convex-engine/src/curve_builder.rs
- crates/convex-analytics/src/spreads/asw/par_par.rs
- crates/convex-engine/src/reactive.rs
- crates/convex-analytics/src/spreads/zspread.rs
- crates/convex-math/src/interpolation/monotone_convex.rs
The refactor that added sort/dedup to rebuild_inner also changed it to unconditionally assign `self.inner` (build result `.ok()`), whereas the original only assigned on success. As a result a failed reconstruction now wipes a working `inner` to `None`, silently dropping the curve to the linear `interpolate_rate` fallback. This is reachable on bumped curves during KRD/PV01 (pricing_router clones the curve, mutates rates, then rebuilds), corrupting the sensitivity. Build only when >= 2 distinct pillars remain; on a build error keep the previously-built curve and log a warning instead of discarding it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
coupon_year_fraction re-parsed the bond's day-count string and rebuilt the boxed day counter on every cash flow in the coupon loop. Add a `day_counter` helper that parses once into a reusable `Box<dyn DayCount>`; the year-fraction helper now takes the pre-built `&dyn DayCount`, so par_par and proceeds parse a single time before the loop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The crate doc claimed "ONLY trait definitions and their plain-data types, with no runtime dependencies", but convex-ports depends on tokio (for the sync::broadcast receiver types) and ships concrete helpers (receiver wrappers, bond-filter matching, config constructors). Reword to state accurately what it contains and that its only non-core dependency is tokio's sync feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ate() calculate() discounted the spread using self.compounding, but price_with_spread, spread_dv01 (via price_with_spread), and calculate_from_cash_flows hard-coded continuous discounting. With a non-continuous calculator (e.g. the z_spread() helper derives SemiAnnual from the coupon frequency) the solve and the pricing paths used different conventions, so price_with_spread/calculate did not round-trip and DV01 was computed under the wrong convention. Extract the compounding match into a private spread_df(z, dt) helper and route calculate(), price_with_spread(), and calculate_from_cash_flows() through it. Behavior is unchanged for the continuous default; add a semi-annual round-trip test to guard the consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ro curves The UFR-convergence tail reads the last value/derivative as a continuously compounded zero rate (instantaneous forward f_LLP = z + t*z'), which only holds when the curve stores continuously-compounded zero rates. with_extrapolation() applied it regardless of ValueType, so selecting it on e.g. a discount-factor curve produced silently wrong rates. Validate at construction and return an error otherwise (the sole production caller already uses continuous zero rates). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
convex_math::extrapolation::ExtrapolationMethod was never constructed or matched anywhere -- all curve extrapolation selection goes through the separate convex_curves::ExtrapolationMethod enum. The dead duplicate only invited confusion about which enum is authoritative; delete it and its prelude re-export rather than document a mapping between a live and a dead type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ASW mock fixtures emit cash flows with accrual_start/end = None, so the existing tests only exercised the nominal 1/frequency fallback. Add a focused unit test that builds a cash flow with explicit accrual boundaries and asserts coupon_year_fraction uses the parsed day-count convention (ACT/360 = days/360), not the nominal fraction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
This PR removes the convex-traits crate by consolidating ports into convex-engine/src/ports, and updates BuiltCurve to incorporate Smith-Wilson extrapolation and Monotone Convex interpolation natively within the pricing router.
Summary by CodeRabbit
New Features
Improvements