FFI redesign: mark-driven JSON RPC + slimmer Excel wrapper#80
Merged
Conversation
Replace the legacy ~70-symbol Rust FFI with a 13-symbol JSON-RPC surface
built around the trader Mark and the typed DTOs in convex-analytics::dto.
The Excel add-in is rewritten on top: cell UDFs collapse from per-input
shape (CX.PRICE, CX.PRICE.ZSPREAD, CX.YIELD, CX.YIELD.TRUE, ...) to per
question (CX.PRICE, CX.RISK, CX.SPREAD, CX.CASHFLOWS, CX.CURVE.QUERY).
Adding a new bond shape, spread family, or pricing convention is now a
serde enum variant plus a dispatch arm — no new C symbol, no new
P/Invoke, no new UDF.
Rust:
* convex-core: Mark::from_str — textual mark grammar (clean/dirty,
32nds, yield+frequency, spread+benchmark, explicit type) + tests.
* convex-analytics: new `dto` module covering BondSpec / CurveSpec /
PricingRequest / RiskRequest / SpreadRequest / CashflowRequest /
CurveQueryRequest and their responses, all serde-tagged.
* convex-ffi: rewritten lib.rs / build.rs / dispatch.rs / schemas.rs.
13 C symbols total — bond/curve construction (handles), 5 stateless
JSON-RPC analytics, plus schema/mark introspection and string-free
boilerplate. Dispatcher wires every spread family (Z / G / I / ASW
par + proceeds / OAS / DM) plus FRN, zero-coupon, sinking-fund and
callable bond shapes through the same routing. KRD via curve bumping
with implied Z-spread held fixed. Identifier handling now satisfies
the FixedRateBond invariant for free-form names (regression test
added).
* smoke.rs integration tests cover every shape and family (20 tests).
Excel:
* Drop System.Text.Json (transitive-DLL pain on net472 + ExcelDnaPack)
in favour of Newtonsoft.Json — single DLL, no transitive deps,
packs cleanly into the .xll.
* New core: Cx.cs (P/Invoke + envelope-aware RPC), CxParse.cs (cell
coercion), CxSettings.cs (persisted defaults), Functions.cs (UDFs),
helpers/{BondSpecs,CurveSpecs,SheetHelpers,IconAtlas} so the cell
UDFs and ribbon forms share a single source of truth for JSON
construction.
* Ribbon rebuilt with eight forms: BondBuilder, CurveBuilder,
PricingTicket, SpreadTicket, CurveViewer, ScenarioForm,
ObjectBrowser, SchemaBrowser, Settings.
* Demo workbook is now a derived artifact regenerated by
excel/build_demo.py (openpyxl) — updates whenever the cell API
changes.
Documentation:
* CLAUDE.md, excel/README.md, excel/SMOKE_TEST.md, convex-ffi/README.md
rewritten to reflect the current surface.
P0 correctness: - Z-spread anchored at settlement (not curve ref date) via forward DF = DF(cf)/DF(settle); fixes silent multi-bp drift in Z, KRD, risk metrics. - KRD denominator switched from clean to dirty (Bloomberg convention); numerator already cancels accrued. - FRN discount margin honors in-progress (already-fixed) coupon from cf.amount; future periods use simple forward over the accrual interval and the bond's day-count year fraction instead of an instantaneous forward + 365-day year. - OAS solver: Brent over a single shared HW1F tree (was bisection rebuilding the tree every iteration). Callability layers driven off (time, date) pairs to avoid t*365 round-trip drift on dates one day off a coupon. Curve extrapolation errors propagate (no more silent unwrap_or(0.05)). - ICMA day count: added DayCount::period_year_fraction and routed FRN period_coupon through it; ICMA bare year_fraction keeps the QL-style ISDA fallback so yield solvers without period info still work. P1/P2 perf: - OAS default tree_steps 100 -> 200. - oas_duration reuses the tree across the 3 prices. - option_value re-anchored at settlement. - FloatingRateNote schedule cached via OnceCell. - Added ForwardCurve::simple_forward_period and discount_curve() accessors. - Factored zspread cashflow projection into forward_cashflows(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The grammar used <number>, <int>, <FREQ>, <BENCH> placeholders that rustdoc was parsing as HTML tags / intra-doc links. Fence as ```text``` so rustdoc treats it as plain code. Also picks up the cargo-fmt whitespace tidy in the same file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure rustfmt output; no behavior change. Fixes the CI fmt check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pass build::bond_from_json / curve_from_json / schemas::lookup directly to with_str/with_str_owned. Fixes clippy warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Callable bonds trading at a premium can need OAS well below -5% (the model price asymptotes toward the call price). Step the low bound down in 5% increments to a -50% floor; if Brent still can't bracket, propagate honestly (price truly unreachable for any OAS under this curve and vol). Fixes engine test test_price_callable_with_oas which exercises a 5%-coupon callable trading at the first call price. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughRefactors the FFI from pointer/struct APIs to a JSON-over-ABI RPC model: adds JSON DTOs and schemas, JSON-based constructors and stateless analytics RPCs, registry/object-kind changes, removes legacy C ABI and Excel wrapper/RTD UI layers, and replaces them with a thin JSON P/Invoke layer and new Excel UDFs. Changes
Sequence Diagram(s)sequenceDiagram
participant Excel as Excel UDF
participant UDF as Functions.cs
participant Cx as Cx.cs (P/Invoke)
participant FFI as convex_ffi (native)
participant Dispatch as dispatch.rs
participant Analytics as convex-analytics
Excel->>UDF: Invoke UDF (e.g., CxPrice)
UDF->>UDF: Build JSON request (DTO)
UDF->>Cx: Call P/Invoke (Price with JSON ptr)
Cx->>FFI: convex_price(JSON ptr) (C ABI)
FFI->>Dispatch: Pass JSON to dispatch handler
Dispatch->>Analytics: Run pricing/risk/spread logic
Analytics-->>Dispatch: Return DTO result
Dispatch->>FFI: Serialize Envelope<T> JSON string
FFI-->>Cx: Return JSON pointer
Cx-->>UDF: Convert and free JSON, return data
UDF-->>Excel: Render scalar/grid result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 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 35 minutes and 50 seconds.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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
crates/convex-ffi/src/schemas.rs-79-84 (1)
79-84:⚠️ Potential issue | 🟠 MajorModel handles as
#CX#<u64>strings, not integers.Every handle-bearing property in this file is still declared as
integer, which conflicts with the JSON-wire handle format used by the FFI layer. That will make schema-driven clients generate the wrong payload shape. As per coding guidelines,Handle registry format must use the pattern#CX#<u64> where handles start at 100 for visual contrast.Also applies to: 106-111, 134-143, 167-168, 186-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/schemas.rs` around lines 79 - 84, The schema fields that represent FFI handles (e.g., properties "bond", "curve", "forward_curve" and any other handle-bearing properties referenced later) are incorrectly typed as integer; update each handle-bearing property to be a string with a regex pattern enforcing the "#CX#<u64>" wire format (for example pattern "^#CX#\\d+$") and add a description noting handles start at 100 for visual contrast; also update any relevant definitions (the referenced handle definitions in the blocks around the other occurrences) to the same string+pattern shape so generated clients produce the correct payload shape.crates/convex-ffi/src/schemas.rs-129-146 (1)
129-146:⚠️ Potential issue | 🟠 MajorUse the canonical spread-type names here.
The enum values (
ZSpread,AssetSwapPar,DiscountMargin, …) don't match the wire values required elsewhere in this repo, so schema introspection will advertise a different contract than the dispatcher accepts. As per coding guidelines,Spread types must be one of: Z, G, I, OAS, DM, ASW, ASW_PROC, CREDIT.Suggested fix
- "spread_type": {"enum": ["ZSpread","GSpread","ISpread","AssetSwapPar","AssetSwapProceeds","OAS","Credit","DiscountMargin"]}, + "spread_type": {"enum": ["Z","G","I","ASW","ASW_PROC","OAS","DM","CREDIT"]},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/schemas.rs` around lines 129 - 146, The SPREAD_REQUEST schema's "spread_type" enum uses non-canonical names (e.g., "ZSpread","AssetSwapPar","DiscountMargin") which mismatch the wire values; update the enum in the SPREAD_REQUEST const to use the canonical spread-type names exactly: "Z","G","I","OAS","DM","ASW","ASW_PROC","CREDIT" so schema introspection matches the dispatcher contract (leave other properties and the required array intact).crates/convex-ffi/src/schemas.rs-74-113 (1)
74-113:⚠️ Potential issue | 🟠 MajorResolve the dangling schema refs and keep
RiskRequestaligned with the DTO.
PricingRequestandRiskRequestpoint at#/definitions/Markand#/definitions/Frequency, butlookup()returns standalone schema strings, so those anchors never resolve here.RiskRequestalso dropsforward_curve, which exists incrates/convex-analytics/src/dto.rs:347-360. Either inline the shared shapes or emit a root schema with a definitions block.Suggested fix for the missing field
"properties": { "bond": {"type": "integer"}, "settlement": {"type": "string", "format": "date"}, "mark": {"$ref": "#/definitions/Mark"}, "curve": {"type": ["integer","null"]}, "quote_frequency": {"$ref": "#/definitions/Frequency"}, + "forward_curve": {"type": ["integer","null"], "description": "FRN projection curve (default: discount curve)"} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/schemas.rs` around lines 74 - 113, The JSON schemas PRICING_REQUEST and RISK_REQUEST reference definitions ("#/definitions/Mark" and "#/definitions/Frequency") that never resolve because lookup() returns standalone schema strings; update these constants so the referenced types are available at the root (either by inlining the Mark and Frequency object schemas directly into PRICING_REQUEST and RISK_REQUEST, or by emitting a single root schema string that includes a "definitions" block containing Mark and Frequency and then references them), and also add the missing forward_curve property to RISK_REQUEST (matching the forward_curve definition used in PRICING_REQUEST / the DTO) so RiskRequest stays aligned with the DTO.crates/convex-analytics/src/spreads/discount_margin.rs-113-126 (1)
113-126:⚠️ Potential issue | 🟠 MajorUse the discount curve reference date for discounting tenors.
Line 113/Line 124derivet_settleandt_cffromself.forward_curve.reference_date(), butdiscount_factor(...)is called onself.discount_curve. If the two curves have different reference dates, PV is wrong.Suggested fix
- let ref_date = self.forward_curve.reference_date(); + let fwd_ref_date = self.forward_curve.reference_date(); + let disc_ref_date = self.discount_curve.reference_date(); @@ - let t_settle = ref_date.days_between(&settlement) as f64 / 365.0; + let t_settle = disc_ref_date.days_between(&settlement) as f64 / 365.0; @@ - let t_cf = ref_date.days_between(&cf.date) as f64 / 365.0; + let t_cf = disc_ref_date.days_between(&cf.date) as f64 / 365.0; @@ - let t_start = ref_date.days_between(&start) as f64 / 365.0; - let t_end = ref_date.days_between(&end) as f64 / 365.0; + let t_start = fwd_ref_date.days_between(&start) as f64 / 365.0; + let t_end = fwd_ref_date.days_between(&end) as f64 / 365.0;Also applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-analytics/src/spreads/discount_margin.rs` around lines 113 - 126, The PV code currently computes t_settle and t_cf using ref_date (from the forward curve) but then calls self.discount_curve.discount_factor(...), which is wrong if the curves have different reference dates; change the reference used when computing tenors for discounting to the discount curve's reference date (use self.discount_curve.reference_date() when computing t_settle and t_cf) and apply the same fix to the analogous computations around the t_cf/dt block later (the other occurrence at lines ~133-135); keep calls to self.discount_curve.discount_factor(...) but feed it tenors computed from the discount curve reference date.excel/Convex.Excel/CxSettings.cs-35-44 (1)
35-44:⚠️ Potential issue | 🟠 MajorPrevent shared mutable state from escaping
CxSettings.
Currentreturns the cachedSnapshotinstance directly, andSavecaches the same caller-owned instance. That allows unsynchronized mutation of global settings outside the lock and can create surprising in-memory state drift.Suggested hardening
public sealed class Snapshot { public string DefaultFrequency { get; set; } = "SemiAnnual"; public string DefaultDayCount { get; set; } = "Thirty360US"; public string DefaultSpreadType { get; set; } = "Z"; public string DefaultCurrency { get; set; } = "USD"; + + public Snapshot Clone() => new() + { + DefaultFrequency = DefaultFrequency, + DefaultDayCount = DefaultDayCount, + DefaultSpreadType = DefaultSpreadType, + DefaultCurrency = DefaultCurrency, + }; } @@ public static Snapshot Current { get { lock (_lock) { - if (_cached != null) return _cached; + if (_cached != null) return _cached.Clone(); _cached = Load() ?? new Snapshot(); - return _cached; + return _cached.Clone(); } } } @@ public static void Save(Snapshot snap) { lock (_lock) { + var copy = snap.Clone(); var node = new JObject { - ["DefaultFrequency"] = snap.DefaultFrequency, - ["DefaultDayCount"] = snap.DefaultDayCount, - ["DefaultSpreadType"] = snap.DefaultSpreadType, - ["DefaultCurrency"] = snap.DefaultCurrency, + ["DefaultFrequency"] = copy.DefaultFrequency, + ["DefaultDayCount"] = copy.DefaultDayCount, + ["DefaultSpreadType"] = copy.DefaultSpreadType, + ["DefaultCurrency"] = copy.DefaultCurrency, }; File.WriteAllText(Path, node.ToString(Formatting.None)); - _cached = snap; + _cached = copy; } }Also applies to: 66-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@excel/Convex.Excel/CxSettings.cs` around lines 35 - 44, CxSettings.Current exposes the cached Snapshot instance (_cached) directly and Save stores caller-owned Snapshot into the cache, allowing unsynchronized external mutation; to fix, ensure CxSettings returns a defensive copy and caches a copy instead of the original: in the Current getter, after Load() or when returning _cached, return a clone/new Snapshot constructed from _cached (or call Snapshot.Clone/Copy ctor), and in Save accept the incoming Snapshot but store a clone into _cached under the _lock; update or add a Snapshot.Clone/Copy constructor if needed and use Load(), Save(Snapshot) and _cached/_lock to locate where to apply the copy semantics.crates/convex-analytics/src/spreads/zspread.rs-42-46 (1)
42-46:⚠️ Potential issue | 🟠 MajorGuard against non-positive cashflow discount factors.
forward_cashflowschecksdf_settle, butdf_cfcan still be<= 0. That can generate invalid forward DFs and unstable pricing.Suggested fix
let df_cf = curve .discount_factor(t_cf) .map_err(|e| AnalyticsError::InvalidInput(format!("curve DF at cf: {e}")))?; + if df_cf <= 0.0 { + return Err(AnalyticsError::InvalidInput( + "curve DF at cf is non-positive".to_string(), + )); + } out.push((dt, df_cf / df_settle, cf.amount.to_f64().unwrap_or(0.0)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-analytics/src/spreads/zspread.rs` around lines 42 - 46, forward_cashflows can produce non-positive cashflow discount factors (df_cf), which will lead to invalid forward DFs; after computing df_cf in the loop (the value returned by curve.discount_factor for t_cf) add a guard that checks if df_cf <= 0 and return an AnalyticsError::InvalidInput with a clear message (similar style to the existing df_settle check) instead of pushing the tuple; update the code surrounding the df_cf computation (the block that currently does map_err(...)? and then out.push((dt, df_cf / df_settle, ...))) so that you validate df_cf, handle the error consistently, and only push when df_cf > 0.crates/convex-analytics/src/spreads/discount_margin.rs-126-130 (1)
126-130:⚠️ Potential issue | 🟠 MajorValidate
df_cfis strictly positive before using it.
df_settleis guarded, butdf_cfis not. A non-positivedf_cfwill flow intoadjusted_dfand produce invalid PV math.Suggested fix
- let Ok(df_cf) = self.discount_curve.discount_factor(t_cf) else { - return 0.0; - }; + let df_cf = match self.discount_curve.discount_factor(t_cf) { + Ok(v) if v > 0.0 => v, + _ => return 0.0, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-analytics/src/spreads/discount_margin.rs` around lines 126 - 130, The code uses df_cf without verifying it’s positive; update the block around self.discount_curve.discount_factor(t_cf) (where df_cf and df_settle are used to compute adjusted_df) to ensure df_cf is strictly > 0 before proceeding—if the call returns non-positive (<= 0) then short-circuit the same way df_settle is guarded (e.g., return 0.0 or propagate an error) so adjusted_df = (df_cf / df_settle) * (-dm * dt).exp() never receives an invalid df_cf value.crates/convex-curves/src/curves/mod.rs-121-132 (1)
121-132:⚠️ Potential issue | 🟠 MajorDo not coerce invalid forward inputs to
0.0.Returning
Ok(0.0)for invalid span / non-positive discount factors hides curve errors and can silently distort FRN coupon projection downstream.Suggested fix
pub fn simple_forward_period(&self, t_start: f64, t_end: f64) -> CurveResult<f64> { let span = t_end - t_start; if span <= 0.0 { - return Ok(0.0); + return Err(CurveError::invalid_value("t_end must be greater than t_start")); } let df_start = self.discount_curve.discount_factor(t_start.max(0.0))?; let df_end = self.discount_curve.discount_factor(t_end)?; - if df_end <= 0.0 { - return Ok(0.0); + if df_start <= 0.0 || df_end <= 0.0 { + return Err(CurveError::invalid_value( + "discount factors must be strictly positive", + )); } Ok((df_start / df_end - 1.0) / span) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-curves/src/curves/mod.rs` around lines 121 - 132, The function simple_forward_period currently masks invalid inputs by returning Ok(0.0) when span <= 0.0 or when df_end <= 0.0; change these branches to return a failing CurveResult error instead (do not coerce to 0.0). Specifically, in simple_forward_period replace the two early returns that yield Ok(0.0) with Err(...) using the project's CurveError/CcurveResult error constructor (e.g., CurveError::InvalidInput or a suitable existing variant) and include a clear message referencing invalid span or non-positive discount factor; keep the discount_curve.discount_factor(...) calls and propagate their errors via the existing ? operator. This ensures invalid span or non-positive df_end are surfaced rather than silently producing 0.0.crates/convex-ffi/src/build.rs-252-264 (1)
252-264:⚠️ Potential issue | 🟠 MajorPreserve
compoundingandday_countfromZeroCouponSpec.
build_zero_coupononly carriesmaturity,currency,face_value, andissueinto the registered bond. Any non-defaultcompounding/day_countin the JSON spec is ignored, so zero-coupon pricing and risk can run under different conventions than the caller requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/build.rs` around lines 252 - 264, build_zero_coupon currently ignores ZeroCouponSpec.compounding and .day_count when constructing the ZeroCouponBond; update build_zero_coupon to propagate these fields into the bond before registering it (e.g., call the appropriate setters on ZeroCouponBond such as .with_compounding(spec.compounding) and .with_day_count(spec.day_count) or the actual method names provided by ZeroCouponBond) so the registered bond preserves the spec's compounding and day count conventions; ensure these calls occur after ZeroCouponBond::new(...) and before registry::register(...).crates/convex-ffi/src/dispatch.rs-716-727 (1)
716-727:⚠️ Potential issue | 🟠 Major
convex_cashflowscurrently rejects FRN and zero-coupon handles.Routing this RPC through
with_fixed_bond!meansFloatingRateandZeroCouponnever reach their owncash_flows(settlement)implementation. That breaksCX.CASHFLOWSfor two supported bond kinds in the new JSON surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/dispatch.rs` around lines 716 - 727, The current use of the with_fixed_bond! macro around req.bond prevents FloatingRate and ZeroCoupon variants from reaching their own cash_flows(settlement) implementations; update the logic so you call the bond.cash_flows(req.settlement) on the actual bond variant (either by removing/avoiding with_fixed_bond! and calling req.bond.cash_flows(...) directly, or by pattern-matching on req.bond variants and invoking each variant's cash_flows), then map to CashflowEntry (using cashflow_kind_tag(cf.flow_type) and amount.try_into().unwrap_or(0.0)) and return Ok(CashflowResponse { flows }) so FRN and ZeroCoupon are handled correctly.crates/convex-ffi/src/dispatch.rs-740-759 (1)
740-759:⚠️ Potential issue | 🟠 MajorPreserve query failures instead of returning
invalid_handle.The closure converts every curve-method error into
None, and the outer match turns that into"invalid_handle". For an existing curve, bad tenors or forward ranges will now surface as “handle not found”, which is the wrong envelope and loses the real failure reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/dispatch.rs` around lines 740 - 759, The closure passed to registry::with_object::<RateCurve<DiscreteCurve>, _, _>(req.curve, |c| ...) swallows all method errors by calling .ok(), causing bad tenors or forward ranges to be reported as an "invalid handle"; instead make the closure return Result<_, DispatchError> (or a Result<Option<_>, DispatchError> as appropriate), map method errors (from zero_rate_at_tenor, discount_factor_at_tenor, forward_rate_at_tenors) into DispatchError::handle with clear context (e.g., "curve query failed: <method> error") rather than .ok(), and update the outer match to propagate that Err through to the caller so CurveQueryResponse is only returned on a genuine success while real query failures are preserved.excel/Convex.Excel/CxParse.cs-28-38 (1)
28-38:⚠️ Potential issue | 🟠 MajorValidate numeric handles before casting.
(ulong)dwill silently truncate101.9to101and can turn bad negative numbers into unrelated integers. At this boundary that is dangerous: a malformed Excel input can resolve to the wrong native object instead of producing a clear validation error.🛡️ Fail fast on malformed numeric handles
case double d: - return (ulong)d; + if (double.IsNaN(d) || double.IsInfinity(d) || d < 0 || d % 1 != 0) + throw new ConvexException($"{fieldName} {d}: not a recognized handle"); + return (ulong)d;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@excel/Convex.Excel/CxParse.cs` around lines 28 - 38, The double case currently casts a double to ulong directly which silently truncates fractional values and mishandles negatives; update the switch case that matches "case double d:" in CxParse.cs to validate that d is an exact non-negative integer within UInt64 range before returning (use fieldName in the thrown message), and if validation fails throw a ConvexException like the string branch; keep the existing string handling (HandlePrefix) and the default Convert.ToUInt64 behavior unchanged.excel/Convex.Excel/Cx.cs-101-105 (1)
101-105:⚠️ Potential issue | 🟠 Major
convex_list_objects()is not envelope-wrapped.
crates/convex-ffi/src/lib.rsreturns a raw JSON array here, not{ "ok": ..., "result": ... }. This implementation will therefore throw on every successful call, so the object-browser path is effectively dead.🧭 Minimal fix
public static List<ObjectEntry> ListObjects() { var raw = ConsumeString(convex_list_objects()); - var env = JToken.Parse(raw) ?? throw new ConvexException("empty list response"); - if ((string?)env["ok"] != "true") - throw new ConvexException((string?)env["error"]?["message"] ?? "list error"); - var arr = env["result"] as JArray ?? new JArray(); + var arr = JToken.Parse(raw) as JArray + ?? throw new ConvexException("list response was not a JSON array"); var list = new List<ObjectEntry>(arr.Count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@excel/Convex.Excel/Cx.cs` around lines 101 - 105, The call to convex_list_objects() returns a raw JSON array, but the current code expects an envelope object and throws; update the parsing in the Cx.cs path that calls ConsumeString(convex_list_objects()) so it handles both envelope-wrapped responses and raw arrays: parse raw into a JToken, if it's a JArray use that as arr, otherwise if it's an object validate "ok" and extract "result" into arr; keep throwing ConvexException only on an explicit error envelope, and use symbols from the diff (convex_list_objects(), ConsumeString, ConvexException, env, arr) to locate and modify the logic accordingly.crates/convex-ffi/src/registry.rs-66-70 (1)
66-70:⚠️ Potential issue | 🟠 MajorName and object updates need to be atomic.
register(),release(), andclear_all()mutatenamesandobjectsunder separate locks, so concurrent readers can observe dangling name→handle mappings. In that windowlookup(name)can hand back a handle that was already removed fromobjects, which turns into intermittent invalid-handle errors. These two maps should be updated under one shared lock/state.Also applies to: 84-106, 145-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/registry.rs` around lines 66 - 70, The name→handle and handle→object maps in struct Inner are being mutated under separate locks (objects: RwLock<HashMap<Handle, Entry>> and names: RwLock<HashMap<String, Handle>>), causing races where lookup(name) can return a handle already removed from objects; fix by making updates to both maps atomic: replace the two independent RwLocks with a single lock/guard that protects both maps together (e.g., a single Mutex or one RwLock wrapping a struct containing both HashMaps) and update register(), release(), clear_all(), and any other code paths (including lookup()) to acquire that single lock when reading or writing both maps so name↔handle consistency is preserved.excel/Convex.Excel/Functions.cs-251-273 (1)
251-273:⚠️ Potential issue | 🟠 Major
CX.SPREADcannot issue a valid G-spread request.The wire DTO requires
params.govt_curvefor G-spread, but this UDF only accepts one curve handle and never sends that field. SoCX.SPREAD(..., "G", ...)is either going to fail or use the wrong benchmark. The signature needs an additional government-curve input before G-spread should be advertised here.crates/convex-ffi/src/lib.rs-181-186 (1)
181-186:⚠️ Potential issue | 🟠 MajorKeep wire error codes inside the documented enum.
convex_schema()and the shared envelope helpers currently emit custom codes like"schema"and"serialize". That breaks the FFI contract clients are supposed to switch on. These failures should be mapped onto the existing wire codes instead.Possible fix
- Err(msg) => err_envelope("schema", &msg), + Err(msg) => err_envelope("invalid_input", &msg), @@ - Err(e) => err_envelope("serialize", &e.to_string()), + Err(e) => err_envelope("analytics", &e.to_string()), @@ - Err(e) => err_envelope("serialize", &e.to_string()), + Err(e) => err_envelope("analytics", &e.to_string()),As per coding guidelines, "FFI request/response envelopes must follow the format
{\"ok\":\"true\",\"result\":...}on success and{\"ok\":\"false\",\"error\":{\"code\",\"message\",\"field?\"}}on failure with error codes:invalid_input,invalid_handle,analytics".Also applies to: 199-203, 257-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/lib.rs` around lines 181 - 186, The FFI error envelopes emitted by convex_schema (and the other helpers referenced) use custom codes like "schema" and "serialize" which violate the documented wire enum; update error reporting so err_envelope (and any helper that builds {"ok":"false","error":...}) only emits the allowed codes ("invalid_input", "invalid_handle", "analytics"). Concretely, change calls in convex_schema and the other sites to map their internal failure reasons into one of the documented codes, and refactor err_envelope (or provide a thin mapper) to accept that enum/value instead of arbitrary strings so all error objects conform to {"ok":"false","error":{"code","message","field?"}}; ensure serialize-related failures map to an appropriate documented code and that no caller emits "schema" or "serialize" directly.crates/convex-analytics/src/dto.rs-127-147 (1)
127-147:⚠️ Potential issue | 🟠 MajorUse integer types for basis-point fields.
spread_bps/z_spread_bpsare exposed asDecimal/f64, which lets fractional bps leak onto the JSON contract. That diverges from the repo’s stated bps convention and makes round-tripping across Excel ⇄ JSON ⇄ Rust ambiguous. These DTO fields should be integer bps on the wire, with any fractional math kept internal.As per coding guidelines, "Spread basis points must be expressed as integer basis points (75 = 75 bp) in all domain types and DTOs".
Also applies to: 339-341, 413-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-analytics/src/dto.rs` around lines 127 - 147, The spread basis-point fields are using floating types (Decimal/f64) which allows fractional bps on the wire; change those DTO fields to integer basis points and update serialization accordingly: replace spread_bps (in FloatingRateSpec) and all z_spread_bps occurrences with an integer type (e.g., i64) in the DTOs, adjust related serde defaults/attributes so they serialize as integers, and update any deserialization/conversion code that previously accepted Decimal/f64 to convert between internal Decimal math and the new integer-bps DTO (preserve internal fractional calculations but ensure JSON/DTO contract is integer bps); update imports/type annotations for the affected structs and any unit tests or helpers referencing spread_bps/z_spread_bps.excel/Convex.Excel/Functions.cs-531-543 (1)
531-543:⚠️ Potential issue | 🟠 MajorReturn cashflow dates as
DateTime, not text.
CX.CASHFLOWScurrently emits ISO date strings into the grid, so Excel treats the first column as text. That breaks normal date sorting/formatting/formula behavior. Parse the wire value back toDateTimebefore returning it to Excel.Possible fix
for (int i = 0; i < arr.Count; i++) { var item = arr[i] as JObject; - g[i + 1, 0] = (string?)item?["date"] ?? ""; + var iso = (string?)item?["date"]; + g[i + 1, 0] = + iso is not null && + DateTime.TryParse(iso, CultureInfo.InvariantCulture, DateTimeStyles.None, out var dt) + ? dt + : (object)(iso ?? ""); g[i + 1, 1] = (double?)item?["amount"] ?? 0.0; g[i + 1, 2] = (string?)item?["kind"] ?? ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@excel/Convex.Excel/Functions.cs` around lines 531 - 543, The CashflowsToGrid function currently puts ISO date strings into the output grid so Excel treats them as text; change the assignment for g[i + 1, 0] to parse the wire value into a DateTime (e.g., use DateTime.TryParse on item?["date"] string and fall back to either DateTime.MinValue or null/empty cell) and assign the DateTime instance into the grid instead of the string; keep existing null/empty handling for missing dates and leave numeric and kind columns unchanged so Excel receives real DateTime objects for proper sorting/formatting.
🟡 Minor comments (2)
crates/convex-ffi/src/schemas.rs-115-127 (1)
115-127:⚠️ Potential issue | 🟡 MinorTighten the
key_ratesitem schema.
RiskResponse.key_ratesallows{}today because the item object has no required fields, even though the DTO always emits bothtenorandduration. That makes the contract looser than the Rust type.Suggested fix
- "key_rates": {"type": "array", "items": {"type":"object","properties":{"tenor":{"type":"number"},"duration":{"type":"number"}}}} + "key_rates": {"type": "array", "items": {"type":"object","required":["tenor","duration"],"properties":{"tenor":{"type":"number"},"duration":{"type":"number"}}}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/src/schemas.rs` around lines 115 - 127, The JSON schema in the RISK_RESPONSE constant currently allows empty objects for elements of RiskResponse.key_rates because the item object has no required fields; update the key_rates item definition inside the RISK_RESPONSE string to include "required": ["tenor","duration"] (ensuring the existing "tenor" and "duration" properties remain typed as "number") so the schema matches the Rust DTO contract.crates/convex-core/src/types/mark.rs-237-240 (1)
237-240:⚠️ Potential issue | 🟡 MinorTighten benchmark token validation in spread parsing.
Grammar says benchmark is a non-whitespace identifier, but this accepts values like
"USD SOFR"and defers failure downstream. Reject whitespace here for deterministic parse errors.As per coding guidelines, Mark parsing must support textual forms with explicit benchmark/type semantics.Suggested fix
let benchmark = tail[1..].trim().to_string(); - if benchmark.is_empty() { + if benchmark.is_empty() || benchmark.chars().any(char::is_whitespace) { return Err(MarkParseError("missing benchmark after '@'".into())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-core/src/types/mark.rs` around lines 237 - 240, The current spread parsing accepts benchmark strings with internal whitespace (e.g., "USD SOFR"); update the validation after extracting let benchmark = tail[1..].trim().to_string() to reject any benchmark containing whitespace so parsing fails deterministically. Specifically, in the same scope where benchmark is assigned, add a check like if benchmark.chars().any(|c| c.is_whitespace()) { return Err(MarkParseError("invalid benchmark token".into())); } (referencing the benchmark variable and MarkParseError used in this block) so only non-whitespace identifier tokens are accepted.
🧹 Nitpick comments (1)
crates/convex-ffi/tests/smoke.rs (1)
209-235: Add smoke coverage forconvex_curve_query.This file exercises the other public JSON RPCs, but not the fifth analytics endpoint. One happy-path
convex_curve_querycase plus one bad-range/error-envelope case would lock down the newCX.CURVE.QUERYsurface and catch contract regressions early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/convex-ffi/tests/smoke.rs` around lines 209 - 235, Add smoke tests for convex_curve_query: use the existing helpers build_curve(spec: Value) and flat_curve(rate: f64) to create a curve handle, call convex_ffi::convex_curve_query with a valid date/tenor payload and assert a successful handle/result and expected values, and add a second test that calls convex_ffi::convex_curve_query with an out-of-range or malformed query to assert it returns convex_ffi::INVALID_HANDLE and that convex_ffi::convex_last_error() yields a non-null descriptive error string; ensure both tests clean up any handles per existing test conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/convex-ffi/README.md`:
- Around line 201-205: The ctypes example incorrectly sets
lib.convex_price.restype = ctypes.c_char_p which causes ctypes to copy the
Rust-owned string into a Python bytes and lose the original pointer; instead set
functions that return owned FFI strings (e.g., convex_price and any other
Rust-returning string like convex_bond_from_json if applicable) to return
ctypes.c_void_p and set lib.convex_string_free.argtypes = [ctypes.c_void_p];
after calling the function, manually convert the returned void pointer to
bytes/str in Python (e.g., via ctypes.cast/ctypes.string_at and decoding) and
then call convex_string_free with that same void pointer to free the original
Rust allocation. Ensure both return-type changes and convex_string_free.argtypes
use ctypes.c_void_p for all owned FFI strings mentioned.
In `@crates/convex-ffi/src/dispatch.rs`:
- Around line 607-643: The option_value is effectively zero because bullet_dirty
is computed by re-pricing the bullet through with_callable using
ZSpreadCalculator (bullet_dirty via ZSpreadCalculator::new(...).calculate(...)
and price_with_spread) which matches the same dirty input used to solve the
callable OAS, so bullet_dirty - p0 collapses; fix by computing the bullet PV at
the implied Z-spread outside the callable path (i.e., compute z via
ZSpreadCalculator::new(&typed_curve).calculate(...) on the base bond BB and then
price the non-callable bullet using BB::price or the price_with_spread path that
does not route through CallableBond, then set option_value = bullet_pv_at_z -
p0; ensure you still compute eff_dur/eff_cvx via OASCalculator::price_with_oas
with parallel ±1bp bumps holding OAS constant (calc.price_with_oas, curve_up,
curve_dn) and return that bullet PV in SpreadResponse::option_value instead of
the current bullet_dirty variable derived through with_callable.
In `@crates/convex-ffi/src/registry.rs`:
- Around line 108-126: The dispatch layer still calls the removed
registry::with_object_kind(...) but registry.rs now exposes kind_of(handle:
Handle) and with_object<T...>, so fix by either restoring a thin helper named
with_object_kind(handle: Handle) that returns Option<ObjectKind> delegating to
kind_of(handle), or update crates/convex-ffi/src/dispatch.rs to call
registry::kind_of(handle) (or use with_object when a typed reference is needed)
wherever with_object_kind is referenced; search for with_object_kind in
dispatch.rs and replace those calls with the appropriate kind_of(handle) or
with_object usage to make the API consistent.
---
Major comments:
In `@crates/convex-analytics/src/dto.rs`:
- Around line 127-147: The spread basis-point fields are using floating types
(Decimal/f64) which allows fractional bps on the wire; change those DTO fields
to integer basis points and update serialization accordingly: replace spread_bps
(in FloatingRateSpec) and all z_spread_bps occurrences with an integer type
(e.g., i64) in the DTOs, adjust related serde defaults/attributes so they
serialize as integers, and update any deserialization/conversion code that
previously accepted Decimal/f64 to convert between internal Decimal math and the
new integer-bps DTO (preserve internal fractional calculations but ensure
JSON/DTO contract is integer bps); update imports/type annotations for the
affected structs and any unit tests or helpers referencing
spread_bps/z_spread_bps.
In `@crates/convex-analytics/src/spreads/discount_margin.rs`:
- Around line 113-126: The PV code currently computes t_settle and t_cf using
ref_date (from the forward curve) but then calls
self.discount_curve.discount_factor(...), which is wrong if the curves have
different reference dates; change the reference used when computing tenors for
discounting to the discount curve's reference date (use
self.discount_curve.reference_date() when computing t_settle and t_cf) and apply
the same fix to the analogous computations around the t_cf/dt block later (the
other occurrence at lines ~133-135); keep calls to
self.discount_curve.discount_factor(...) but feed it tenors computed from the
discount curve reference date.
- Around line 126-130: The code uses df_cf without verifying it’s positive;
update the block around self.discount_curve.discount_factor(t_cf) (where df_cf
and df_settle are used to compute adjusted_df) to ensure df_cf is strictly > 0
before proceeding—if the call returns non-positive (<= 0) then short-circuit the
same way df_settle is guarded (e.g., return 0.0 or propagate an error) so
adjusted_df = (df_cf / df_settle) * (-dm * dt).exp() never receives an invalid
df_cf value.
In `@crates/convex-analytics/src/spreads/zspread.rs`:
- Around line 42-46: forward_cashflows can produce non-positive cashflow
discount factors (df_cf), which will lead to invalid forward DFs; after
computing df_cf in the loop (the value returned by curve.discount_factor for
t_cf) add a guard that checks if df_cf <= 0 and return an
AnalyticsError::InvalidInput with a clear message (similar style to the existing
df_settle check) instead of pushing the tuple; update the code surrounding the
df_cf computation (the block that currently does map_err(...)? and then
out.push((dt, df_cf / df_settle, ...))) so that you validate df_cf, handle the
error consistently, and only push when df_cf > 0.
In `@crates/convex-curves/src/curves/mod.rs`:
- Around line 121-132: The function simple_forward_period currently masks
invalid inputs by returning Ok(0.0) when span <= 0.0 or when df_end <= 0.0;
change these branches to return a failing CurveResult error instead (do not
coerce to 0.0). Specifically, in simple_forward_period replace the two early
returns that yield Ok(0.0) with Err(...) using the project's
CurveError/CcurveResult error constructor (e.g., CurveError::InvalidInput or a
suitable existing variant) and include a clear message referencing invalid span
or non-positive discount factor; keep the discount_curve.discount_factor(...)
calls and propagate their errors via the existing ? operator. This ensures
invalid span or non-positive df_end are surfaced rather than silently producing
0.0.
In `@crates/convex-ffi/src/build.rs`:
- Around line 252-264: build_zero_coupon currently ignores
ZeroCouponSpec.compounding and .day_count when constructing the ZeroCouponBond;
update build_zero_coupon to propagate these fields into the bond before
registering it (e.g., call the appropriate setters on ZeroCouponBond such as
.with_compounding(spec.compounding) and .with_day_count(spec.day_count) or the
actual method names provided by ZeroCouponBond) so the registered bond preserves
the spec's compounding and day count conventions; ensure these calls occur after
ZeroCouponBond::new(...) and before registry::register(...).
In `@crates/convex-ffi/src/dispatch.rs`:
- Around line 716-727: The current use of the with_fixed_bond! macro around
req.bond prevents FloatingRate and ZeroCoupon variants from reaching their own
cash_flows(settlement) implementations; update the logic so you call the
bond.cash_flows(req.settlement) on the actual bond variant (either by
removing/avoiding with_fixed_bond! and calling req.bond.cash_flows(...)
directly, or by pattern-matching on req.bond variants and invoking each
variant's cash_flows), then map to CashflowEntry (using
cashflow_kind_tag(cf.flow_type) and amount.try_into().unwrap_or(0.0)) and return
Ok(CashflowResponse { flows }) so FRN and ZeroCoupon are handled correctly.
- Around line 740-759: The closure passed to
registry::with_object::<RateCurve<DiscreteCurve>, _, _>(req.curve, |c| ...)
swallows all method errors by calling .ok(), causing bad tenors or forward
ranges to be reported as an "invalid handle"; instead make the closure return
Result<_, DispatchError> (or a Result<Option<_>, DispatchError> as appropriate),
map method errors (from zero_rate_at_tenor, discount_factor_at_tenor,
forward_rate_at_tenors) into DispatchError::handle with clear context (e.g.,
"curve query failed: <method> error") rather than .ok(), and update the outer
match to propagate that Err through to the caller so CurveQueryResponse is only
returned on a genuine success while real query failures are preserved.
In `@crates/convex-ffi/src/lib.rs`:
- Around line 181-186: The FFI error envelopes emitted by convex_schema (and the
other helpers referenced) use custom codes like "schema" and "serialize" which
violate the documented wire enum; update error reporting so err_envelope (and
any helper that builds {"ok":"false","error":...}) only emits the allowed codes
("invalid_input", "invalid_handle", "analytics"). Concretely, change calls in
convex_schema and the other sites to map their internal failure reasons into one
of the documented codes, and refactor err_envelope (or provide a thin mapper) to
accept that enum/value instead of arbitrary strings so all error objects conform
to {"ok":"false","error":{"code","message","field?"}}; ensure serialize-related
failures map to an appropriate documented code and that no caller emits "schema"
or "serialize" directly.
In `@crates/convex-ffi/src/registry.rs`:
- Around line 66-70: The name→handle and handle→object maps in struct Inner are
being mutated under separate locks (objects: RwLock<HashMap<Handle, Entry>> and
names: RwLock<HashMap<String, Handle>>), causing races where lookup(name) can
return a handle already removed from objects; fix by making updates to both maps
atomic: replace the two independent RwLocks with a single lock/guard that
protects both maps together (e.g., a single Mutex or one RwLock wrapping a
struct containing both HashMaps) and update register(), release(), clear_all(),
and any other code paths (including lookup()) to acquire that single lock when
reading or writing both maps so name↔handle consistency is preserved.
In `@crates/convex-ffi/src/schemas.rs`:
- Around line 79-84: The schema fields that represent FFI handles (e.g.,
properties "bond", "curve", "forward_curve" and any other handle-bearing
properties referenced later) are incorrectly typed as integer; update each
handle-bearing property to be a string with a regex pattern enforcing the
"#CX#<u64>" wire format (for example pattern "^#CX#\\d+$") and add a description
noting handles start at 100 for visual contrast; also update any relevant
definitions (the referenced handle definitions in the blocks around the other
occurrences) to the same string+pattern shape so generated clients produce the
correct payload shape.
- Around line 129-146: The SPREAD_REQUEST schema's "spread_type" enum uses
non-canonical names (e.g., "ZSpread","AssetSwapPar","DiscountMargin") which
mismatch the wire values; update the enum in the SPREAD_REQUEST const to use the
canonical spread-type names exactly:
"Z","G","I","OAS","DM","ASW","ASW_PROC","CREDIT" so schema introspection matches
the dispatcher contract (leave other properties and the required array intact).
- Around line 74-113: The JSON schemas PRICING_REQUEST and RISK_REQUEST
reference definitions ("#/definitions/Mark" and "#/definitions/Frequency") that
never resolve because lookup() returns standalone schema strings; update these
constants so the referenced types are available at the root (either by inlining
the Mark and Frequency object schemas directly into PRICING_REQUEST and
RISK_REQUEST, or by emitting a single root schema string that includes a
"definitions" block containing Mark and Frequency and then references them), and
also add the missing forward_curve property to RISK_REQUEST (matching the
forward_curve definition used in PRICING_REQUEST / the DTO) so RiskRequest stays
aligned with the DTO.
In `@excel/Convex.Excel/Cx.cs`:
- Around line 101-105: The call to convex_list_objects() returns a raw JSON
array, but the current code expects an envelope object and throws; update the
parsing in the Cx.cs path that calls ConsumeString(convex_list_objects()) so it
handles both envelope-wrapped responses and raw arrays: parse raw into a JToken,
if it's a JArray use that as arr, otherwise if it's an object validate "ok" and
extract "result" into arr; keep throwing ConvexException only on an explicit
error envelope, and use symbols from the diff (convex_list_objects(),
ConsumeString, ConvexException, env, arr) to locate and modify the logic
accordingly.
In `@excel/Convex.Excel/CxParse.cs`:
- Around line 28-38: The double case currently casts a double to ulong directly
which silently truncates fractional values and mishandles negatives; update the
switch case that matches "case double d:" in CxParse.cs to validate that d is an
exact non-negative integer within UInt64 range before returning (use fieldName
in the thrown message), and if validation fails throw a ConvexException like the
string branch; keep the existing string handling (HandlePrefix) and the default
Convert.ToUInt64 behavior unchanged.
In `@excel/Convex.Excel/CxSettings.cs`:
- Around line 35-44: CxSettings.Current exposes the cached Snapshot instance
(_cached) directly and Save stores caller-owned Snapshot into the cache,
allowing unsynchronized external mutation; to fix, ensure CxSettings returns a
defensive copy and caches a copy instead of the original: in the Current getter,
after Load() or when returning _cached, return a clone/new Snapshot constructed
from _cached (or call Snapshot.Clone/Copy ctor), and in Save accept the incoming
Snapshot but store a clone into _cached under the _lock; update or add a
Snapshot.Clone/Copy constructor if needed and use Load(), Save(Snapshot) and
_cached/_lock to locate where to apply the copy semantics.
In `@excel/Convex.Excel/Functions.cs`:
- Around line 531-543: The CashflowsToGrid function currently puts ISO date
strings into the output grid so Excel treats them as text; change the assignment
for g[i + 1, 0] to parse the wire value into a DateTime (e.g., use
DateTime.TryParse on item?["date"] string and fall back to either
DateTime.MinValue or null/empty cell) and assign the DateTime instance into the
grid instead of the string; keep existing null/empty handling for missing dates
and leave numeric and kind columns unchanged so Excel receives real DateTime
objects for proper sorting/formatting.
---
Minor comments:
In `@crates/convex-core/src/types/mark.rs`:
- Around line 237-240: The current spread parsing accepts benchmark strings with
internal whitespace (e.g., "USD SOFR"); update the validation after extracting
let benchmark = tail[1..].trim().to_string() to reject any benchmark containing
whitespace so parsing fails deterministically. Specifically, in the same scope
where benchmark is assigned, add a check like if benchmark.chars().any(|c|
c.is_whitespace()) { return Err(MarkParseError("invalid benchmark
token".into())); } (referencing the benchmark variable and MarkParseError used
in this block) so only non-whitespace identifier tokens are accepted.
In `@crates/convex-ffi/src/schemas.rs`:
- Around line 115-127: The JSON schema in the RISK_RESPONSE constant currently
allows empty objects for elements of RiskResponse.key_rates because the item
object has no required fields; update the key_rates item definition inside the
RISK_RESPONSE string to include "required": ["tenor","duration"] (ensuring the
existing "tenor" and "duration" properties remain typed as "number") so the
schema matches the Rust DTO contract.
---
Nitpick comments:
In `@crates/convex-ffi/tests/smoke.rs`:
- Around line 209-235: Add smoke tests for convex_curve_query: use the existing
helpers build_curve(spec: Value) and flat_curve(rate: f64) to create a curve
handle, call convex_ffi::convex_curve_query with a valid date/tenor payload and
assert a successful handle/result and expected values, and add a second test
that calls convex_ffi::convex_curve_query with an out-of-range or malformed
query to assert it returns convex_ffi::INVALID_HANDLE and that
convex_ffi::convex_last_error() yields a non-null descriptive error string;
ensure both tests clean up any handles per existing test conventions.
🪄 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: c728132d-bae8-4c36-a408-bd9f00fe7e72
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexcel/ConvexDemo.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (69)
CLAUDE.mdcrates/convex-analytics/Cargo.tomlcrates/convex-analytics/src/dto.rscrates/convex-analytics/src/lib.rscrates/convex-analytics/src/spreads/discount_margin.rscrates/convex-analytics/src/spreads/oas.rscrates/convex-analytics/src/spreads/zspread.rscrates/convex-bonds/src/instruments/floating_rate.rscrates/convex-core/src/daycounts/actact.rscrates/convex-core/src/daycounts/mod.rscrates/convex-core/src/types/mark.rscrates/convex-curves/src/curves/mod.rscrates/convex-ffi/Cargo.tomlcrates/convex-ffi/README.mdcrates/convex-ffi/src/bonds.rscrates/convex-ffi/src/build.rscrates/convex-ffi/src/curves.rscrates/convex-ffi/src/dispatch.rscrates/convex-ffi/src/error.rscrates/convex-ffi/src/lib.rscrates/convex-ffi/src/pricing.rscrates/convex-ffi/src/registry.rscrates/convex-ffi/src/schemas.rscrates/convex-ffi/src/spreads.rscrates/convex-ffi/tests/smoke.rsexcel/Convex.Excel/BondAnalyzerForm.csexcel/Convex.Excel/BondFunctions.csexcel/Convex.Excel/BootstrapForm.csexcel/Convex.Excel/Convex.Excel.csprojexcel/Convex.Excel/Convex.Excel.dnaexcel/Convex.Excel/ConvexWrapper.csexcel/Convex.Excel/CurveFunctions.csexcel/Convex.Excel/CurveViewerForm.csexcel/Convex.Excel/Cx.csexcel/Convex.Excel/CxParse.csexcel/Convex.Excel/CxSettings.csexcel/Convex.Excel/ExcelErrorHelper.csexcel/Convex.Excel/Functions.csexcel/Convex.Excel/HandleHelper.csexcel/Convex.Excel/HelpForm.csexcel/Convex.Excel/NativeLoader.csexcel/Convex.Excel/NativeMethods.csexcel/Convex.Excel/NewBondForm.csexcel/Convex.Excel/NewCurveForm.csexcel/Convex.Excel/ObjectBrowserForm.csexcel/Convex.Excel/PricingFunctions.csexcel/Convex.Excel/RibbonController.csexcel/Convex.Excel/Rtd/ConvexRtdServer.csexcel/Convex.Excel/Rtd/RtdFunctions.csexcel/Convex.Excel/Rtd/RtdSettings.csexcel/Convex.Excel/Rtd/RtdSettingsForm.csexcel/Convex.Excel/Rtd/TopicManager.csexcel/Convex.Excel/SpreadFunctions.csexcel/Convex.Excel/UtilityFunctions.csexcel/Convex.Excel/forms/BondBuilderForm.csexcel/Convex.Excel/forms/CurveBuilderForm.csexcel/Convex.Excel/forms/CurveViewerForm.csexcel/Convex.Excel/forms/ObjectBrowserForm.csexcel/Convex.Excel/forms/PricingTicketForm.csexcel/Convex.Excel/forms/ScenarioForm.csexcel/Convex.Excel/forms/SchemaBrowserForm.csexcel/Convex.Excel/forms/SettingsForm.csexcel/Convex.Excel/forms/SpreadTicketForm.csexcel/Convex.Excel/helpers/BondSpecs.csexcel/Convex.Excel/helpers/IconAtlas.csexcel/Convex.Excel/helpers/SheetHelpers.csexcel/README.mdexcel/SMOKE_TEST.mdexcel/build_demo.py
💤 Files with no reviewable changes (21)
- excel/Convex.Excel/CurveViewerForm.cs
- excel/Convex.Excel/BondAnalyzerForm.cs
- excel/Convex.Excel/Rtd/RtdFunctions.cs
- excel/Convex.Excel/ExcelErrorHelper.cs
- excel/Convex.Excel/NewBondForm.cs
- excel/Convex.Excel/NewCurveForm.cs
- excel/Convex.Excel/ObjectBrowserForm.cs
- excel/Convex.Excel/HelpForm.cs
- excel/Convex.Excel/BootstrapForm.cs
- excel/Convex.Excel/PricingFunctions.cs
- excel/Convex.Excel/Rtd/RtdSettings.cs
- excel/Convex.Excel/Rtd/ConvexRtdServer.cs
- excel/Convex.Excel/NativeMethods.cs
- excel/Convex.Excel/BondFunctions.cs
- crates/convex-ffi/src/pricing.rs
- excel/Convex.Excel/ConvexWrapper.cs
- crates/convex-ffi/src/curves.rs
- excel/Convex.Excel/CurveFunctions.cs
- excel/Convex.Excel/HandleHelper.cs
- crates/convex-ffi/src/spreads.rs
- crates/convex-ffi/src/bonds.rs
Accuracy: - discount_margin: tenor measurements now use the discount curve's reference date for discount-curve queries and the forward curve's for projection queries (was mixing them — wrong DFs whenever the two curves had different ref dates). - zspread: forward_cashflows guards against non-positive DF at each cashflow tenor, mirroring the existing settle-tenor check. - ForwardCurve::simple_forward_period now returns CurveError::InvalidValue for invalid span / non-positive DF instead of silently yielding 0. - dispatch::spread_oas: option_value now uses OASCalculator::option_value (bullet PV at OAS minus callable PV) — the prior bullet-at-Z minus callable-at-OAS path collapsed to ~0 by Z and OAS solver round-trips. Coverage: - build_zero_coupon: route through the builder so spec.compounding and spec.day_count actually reach the bond. - cashflows_inner: dispatch FRN and ZeroCoupon variants to their own Bond::cash_flows impls (with_fixed_bond! used to refuse them). - curve_query_inner: propagate query errors as Analytics, distinct from invalid_handle. Concurrency / docs: - registry: collapse separate object/name RwLocks into one Tables guard so register/release update both maps atomically. - README ctypes example: c_void_p for owned-string returns (c_char_p auto-copies the bytes and loses the original pointer). - CxParse.AsHandle(double): reject fractional / negative / out-of-range doubles instead of silently truncating via (ulong)d. Findings ignored after verification: - with_object_kind callers — already removed; no references remain. - Schemas typing handles as integer — correct, the wire is u64. - Cx.cs ListObjects — already parses the envelope; FFI never returns a raw array. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Excel was treating the date cells as text — no sort, no date format. Parse the wire ISO string into DateTime; fall back to "" on missing or unparseable values. Verified the spread-bps DTO finding: keeping Decimal/f64. Bloomberg, TradeWeb, MarketAxess all quote spreads with fractional bps (ASW at 0.1bp, OAS / credit routinely fractional, FRN new issues like SOFR+50.5). Truncating to i64 would lose precision quants depend on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/convex-analytics/src/spreads/discount_margin.rs`:
- Around line 136-153: The bug is that the match arm treats a period with
accrual_start equal to settlement as already fixed (using cf.amount) because it
tests start > settlement; change the guard so periods that start on or after
settlement are projected instead of treated as fixed (i.e., use start >=
settlement in the pattern guard), keeping the rest of the projection logic that
calls self.forward_curve.simple_forward_period and frn.effective_rate (refer to
cf.accrual_start, cf.accrual_end, settlement,
forward_curve.simple_forward_period, frn.effective_rate) so coupons with reset
on settlement are correctly projected.
In `@crates/convex-ffi/README.md`:
- Around line 19-23: The table row for "Rust rlib" is missing cells for macOS
and Windows which breaks the table; update the "Rust rlib" row in README.md (the
table under the Artifact header) to include placeholder cells for the Linux,
macOS, and Windows columns (e.g., keep the Linux entry `libconvex_ffi.rlib (for
integration tests)` and add "-" or "n/a" for the macOS and Windows cells) so the
Markdown table remains rectangular.
In `@crates/convex-ffi/src/dispatch.rs`:
- Around line 231-244: The DM spread is being passed as a raw basis-point number
to price_with_dm (Mark::Spread branch uses value.as_decimal() → dm_decimal) but
price_with_dm expects a decimal (bps → decimal: 1 bp = 0.0001). Convert the
spread from bps to decimal before calling
DiscountMarginCalculator::price_with_dm (e.g., dm_decimal =
dec_to_f64(value.as_decimal()) * 0.0001 or divide by 10_000.0) so price_with_dm
receives the correct decimal-format DM.
In `@excel/Convex.Excel/CxParse.cs`:
- Around line 124-138: AsSpreadType currently hardcodes an allowlist and throws
ConvexException for unknown spread families; instead, keep only legacy shortcut
mappings and let any other input pass through so canonical Rust enum names work
without editing C#; update AsSpreadType to map known shortcuts (e.g. "Z", "G",
"I", legacy variants) to their canonical names and for the default case return
the trimmed/original input (e.g. s.Trim()) rather than throwing ConvexException
so new SpreadType names from Rust are accepted automatically.
In `@excel/Convex.Excel/Functions.cs`:
- Around line 251-273: The CxSpread wrapper only populates params.volatility and
thus blocks G-spread and DM families; update CxSpread to accept and populate
params.govt_curve, params.forward_curve and params.current_index from the Excel
inputs (use CxParse.AsHandle for curve handles and AsString/AsDouble/IsBlank
helpers as used for volatility) and attach them under req["params"] alongside
volatility; additionally enforce dispatcher requirements: when spreadType == "G"
require params.govt_curve be provided (return a clear error via Safe if
missing), and for DM (spreadType matching DM family) prefer params.forward_curve
if supplied and, if params.current_index is supplied, set parameters to trigger
the closed-form simple margin path instead of the iterative DM solver. Ensure
field parsing remains unchanged and reuse existing symbols CxSpread, req,
CxParse.AsHandle, AsString, AsDouble, IsBlank.
- Around line 269-270: The code is incorrectly scaling OAS volatility twice:
when setting req["params"] for volatility inside the conditional that checks
!IsBlank(volatility) (the block that uses AsDouble(volatility, 0.01) / 100.0),
remove the extra division by 100. Keep the call AsDouble(volatility, 0.01) which
already treats volatility as a decimal (0.01 = 1%) and assign that value
directly to ["volatility"] in req["params"] so CX.SPREAD uses the intended
volatility.
🪄 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: ef12d5aa-e0a3-4e75-9fb9-8d04c8e902d5
📒 Files selected for processing (9)
crates/convex-analytics/src/spreads/discount_margin.rscrates/convex-analytics/src/spreads/zspread.rscrates/convex-curves/src/curves/mod.rscrates/convex-ffi/README.mdcrates/convex-ffi/src/build.rscrates/convex-ffi/src/dispatch.rscrates/convex-ffi/src/registry.rsexcel/Convex.Excel/CxParse.csexcel/Convex.Excel/Functions.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/convex-ffi/src/build.rs
Comment on lines
+19
to
+23
| | Artifact | Linux | macOS | Windows | | ||
| |---|---|---|---| | ||
| | Shared library | `libconvex_ffi.so` | `libconvex_ffi.dylib` | `convex_ffi.dll` | | ||
| | Static archive | `libconvex_ffi.a` | `libconvex_ffi.a` | `convex_ffi.lib` | | ||
| | Rust rlib | `libconvex_ffi.rlib` (for integration tests) | |
There was a problem hiding this comment.
Fix the artifact table row width.
The Rust rlib row only has one platform cell, so the table renders incorrectly and fails markdown linting. Add placeholders for the unsupported columns to keep the matrix rectangular.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Table column count
Expected: 4; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/convex-ffi/README.md` around lines 19 - 23, The table row for "Rust
rlib" is missing cells for macOS and Windows which breaks the table; update the
"Rust rlib" row in README.md (the table under the Artifact header) to include
placeholder cells for the Linux, macOS, and Windows columns (e.g., keep the
Linux entry `libconvex_ffi.rlib (for integration tests)` and add "-" or "n/a"
for the macOS and Windows cells) so the Markdown table remains rectangular.
Comment on lines
+231
to
+244
| Mark::Spread { value, .. } if value.spread_type() == SpreadType::DiscountMargin => { | ||
| let discount_handle = req.curve.ok_or_else(|| { | ||
| DispatchError::input_field("curve", "FRN DM mark requires a discount curve") | ||
| })?; | ||
| let discount = required_curve(discount_handle)?; | ||
| let forward_curve = build_forward_curve(req.forward_curve.unwrap_or(discount_handle))?; | ||
| let dm_decimal = dec_to_f64(value.as_decimal()); | ||
| let dirty = with_frn(req.bond, |frn| { | ||
| DiscountMarginCalculator::new(&forward_curve, &*discount).price_with_dm( | ||
| frn, | ||
| dm_decimal, | ||
| req.settlement, | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Convert DM marks from bps before calling price_with_dm.
Line 237 forwards value.as_decimal() directly, but spreads are represented in basis points while price_with_dm expects a decimal spread. A 75 DM mark is currently priced as 75.0 instead of 0.0075.
Suggested fix
- let dm_decimal = dec_to_f64(value.as_decimal());
+ let dm_decimal = dec_to_f64(value.as_decimal()) / 10_000.0;As per coding guidelines, "Numeric conventions: coupon rates as decimals (0.05 = 5%), spreads in basis points (75 = 75 bp), OAS volatility as decimal (0.01 = 1%), prices per 100 face value".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/convex-ffi/src/dispatch.rs` around lines 231 - 244, The DM spread is
being passed as a raw basis-point number to price_with_dm (Mark::Spread branch
uses value.as_decimal() → dm_decimal) but price_with_dm expects a decimal (bps →
decimal: 1 bp = 0.0001). Convert the spread from bps to decimal before calling
DiscountMarginCalculator::price_with_dm (e.g., dm_decimal =
dec_to_f64(value.as_decimal()) * 0.0001 or divide by 10_000.0) so price_with_dm
receives the correct decimal-format DM.
Comment on lines
+124
to
+138
| // Spread type (canonical names match `SpreadType` Rust enum). | ||
| public static string AsSpreadType(string s) | ||
| { | ||
| return s.Trim().ToUpperInvariant() switch | ||
| { | ||
| "Z" or "ZSPREAD" or "Z-SPREAD" => "ZSpread", | ||
| "G" or "GSPREAD" or "G-SPREAD" => "GSpread", | ||
| "I" or "ISPREAD" or "I-SPREAD" => "ISpread", | ||
| "OAS" => "OAS", | ||
| "DM" or "DISCOUNT_MARGIN" or "DISCOUNTMARGIN" => "DiscountMargin", | ||
| "ASW" or "ASW_PAR" or "ASW-PAR" => "AssetSwapPar", | ||
| "ASW_PROC" or "ASW_PROCEEDS" => "AssetSwapProceeds", | ||
| "CREDIT" => "Credit", | ||
| _ => throw new ConvexException($"unknown spread type {s}"), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don’t hardcode the spread-family allowlist in Excel.
AsSpreadType has to be edited every time Rust adds a new SpreadType, so new families will stay inaccessible from Excel until this file changes. Prefer aliasing the legacy shortcuts you want to support, but let unknown values pass through unchanged so canonical Rust enum names light up automatically.
Based on learnings, "The C FFI surface, P/Invoke layer, and Excel UDFs do not require changes when adding new bond shapes, spread families, curve types, or analytics fields".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@excel/Convex.Excel/CxParse.cs` around lines 124 - 138, AsSpreadType currently
hardcodes an allowlist and throws ConvexException for unknown spread families;
instead, keep only legacy shortcut mappings and let any other input pass through
so canonical Rust enum names work without editing C#; update AsSpreadType to map
known shortcuts (e.g. "Z", "G", "I", legacy variants) to their canonical names
and for the default case return the trimmed/original input (e.g. s.Trim())
rather than throwing ConvexException so new SpreadType names from Rust are
accepted automatically.
Comment on lines
+251
to
+273
| public static object CxSpread( | ||
| object bondRef, | ||
| object curveRef, | ||
| DateTime settlement, | ||
| [ExcelArgument("Mark — see CX.PRICE for grammar")] object mark, | ||
| [ExcelArgument("Spread: Z (default) | G | I | OAS | DM | ASW | ASW_PROC | CREDIT")] object spreadType, | ||
| [ExcelArgument("Optional volatility for OAS, default 1%")] object volatility, | ||
| [ExcelArgument("Field: bps (default) | grid")] object field) => | ||
| Safe(() => | ||
| { | ||
| var req = new JObject | ||
| { | ||
| ["bond"] = CxParse.AsHandle(bondRef, "bond"), | ||
| ["curve"] = CxParse.AsHandle(curveRef, "curve"), | ||
| ["settlement"] = CxParse.AsIsoDate(settlement), | ||
| ["mark"] = CxParse.AsMark(mark), | ||
| ["spread_type"] = CxParse.AsSpreadType(AsString(spreadType, "Z")), | ||
| }; | ||
| if (!IsBlank(volatility)) | ||
| req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) / 100.0 }; | ||
|
|
||
| var result = Cx.Spread(req); | ||
| return SelectSpreadField(result, AsString(field, "bps").ToLowerInvariant()); |
There was a problem hiding this comment.
Expose the spread-family parameters that the dispatcher already requires.
CX.SPREAD can only send volatility. That means Excel cannot supply params.govt_curve for G-spread, or params.forward_curve / params.current_index for DM, so those families are either blocked or forced onto the fallback path from this UI surface.
As per coding guidelines, "DM (Discount Margin) calculations must use params.forward_curve (defaults to discount curve when omitted); when params.current_index is passed, use closed-form simple margin instead of iterative DM solver" and "G-spread calculations must require an explicit params.govt_curve handle (a synthesised benchmark from discount curve is dishonest); dispatcher must refuse without one".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@excel/Convex.Excel/Functions.cs` around lines 251 - 273, The CxSpread wrapper
only populates params.volatility and thus blocks G-spread and DM families;
update CxSpread to accept and populate params.govt_curve, params.forward_curve
and params.current_index from the Excel inputs (use CxParse.AsHandle for curve
handles and AsString/AsDouble/IsBlank helpers as used for volatility) and attach
them under req["params"] alongside volatility; additionally enforce dispatcher
requirements: when spreadType == "G" require params.govt_curve be provided
(return a clear error via Safe if missing), and for DM (spreadType matching DM
family) prefer params.forward_curve if supplied and, if params.current_index is
supplied, set parameters to trigger the closed-form simple margin path instead
of the iterative DM solver. Ensure field parsing remains unchanged and reuse
existing symbols CxSpread, req, CxParse.AsHandle, AsString, AsDouble, IsBlank.
Comment on lines
+269
to
+270
| if (!IsBlank(volatility)) | ||
| req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) / 100.0 }; |
There was a problem hiding this comment.
Stop scaling OAS volatility twice.
The wire DTO already expects volatility as a decimal (0.01 = 1%). Dividing by 100 here turns a valid 0.01 input into 0.0001, so CX.SPREAD(..., "OAS", ...) will price with 100x less volatility than requested.
Suggested fix
- if (!IsBlank(volatility))
- req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) / 100.0 };
+ if (!IsBlank(volatility))
+ req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) };As per coding guidelines, "Numeric conventions: coupon rates as decimals (0.05 = 5%), spreads in basis points (75 = 75 bp), OAS volatility as decimal (0.01 = 1%), prices per 100 face value".
📝 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 (!IsBlank(volatility)) | |
| req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) / 100.0 }; | |
| if (!IsBlank(volatility)) | |
| req["params"] = new JObject { ["volatility"] = AsDouble(volatility, 0.01) }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@excel/Convex.Excel/Functions.cs` around lines 269 - 270, The code is
incorrectly scaling OAS volatility twice: when setting req["params"] for
volatility inside the conditional that checks !IsBlank(volatility) (the block
that uses AsDouble(volatility, 0.01) / 100.0), remove the extra division by 100.
Keep the call AsDouble(volatility, 0.01) which already treats volatility as a
decimal (0.01 = 1%) and assign that value directly to ["volatility"] in
req["params"] so CX.SPREAD uses the intended volatility.
A period whose accrual_start == settlement has accrued zero time and is conceptually a future period. Trusting cf.amount there only works if the caller updated frn.current_rate to the freshly-set rate. The forward-curve projection works regardless and matches Bloomberg DM. Verified the dispatch.rs DM bps→decimal finding is incorrect: Spread::as_decimal() already returns value_bps / 10_000, so dec_to_f64(value.as_decimal()) is already decimal — adding * 0.0001 would double-divide. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/convex-analytics/src/spreads/discount_margin.rs`:
- Around line 118-121: The code currently clamps negative settlement tenor to
zero using t_settle.max(0.0) before calling self.discount_curve.discount_factor,
which can silently misprice; change the call to use the real tenor (pass
t_settle directly) and let discount_factor return an error for negative tenors
(or explicitly check if t_settle < 0.0 and return an error/early failure)
instead of forcing DF(settlement)=DF(0). Update the df_settle binding that calls
self.discount_curve.discount_factor to use t_settle (and optionally add an
explicit negative-tenor branch) so misdated settlements fail fast rather than
being clamped.
🪄 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: 2f5a73cf-1270-4896-ab36-ae7fd07f8f0c
📒 Files selected for processing (1)
crates/convex-analytics/src/spreads/discount_margin.rs
Comment on lines
+118
to
+121
| let t_settle = disc_ref.days_between(&settlement) as f64 / 365.0; | ||
| let df_settle = match self.discount_curve.discount_factor(t_settle.max(0.0)) { | ||
| Ok(v) if v > 0.0 => v, | ||
| _ => return 0.0, |
There was a problem hiding this comment.
Avoid clamping negative settlement tenor to zero.
Line 119 uses t_settle.max(0.0), which can silently misprice when settlement is before the discount curve reference date by forcing DF(settlement) to DF(0). Prefer querying with the real tenor (or failing fast), not clamping.
Suggested fix
let t_settle = disc_ref.days_between(&settlement) as f64 / 365.0;
-let df_settle = match self.discount_curve.discount_factor(t_settle.max(0.0)) {
+let df_settle = match self.discount_curve.discount_factor(t_settle) {
Ok(v) if v > 0.0 => v,
_ => return 0.0,
};📝 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
| let t_settle = disc_ref.days_between(&settlement) as f64 / 365.0; | |
| let df_settle = match self.discount_curve.discount_factor(t_settle.max(0.0)) { | |
| Ok(v) if v > 0.0 => v, | |
| _ => return 0.0, | |
| let t_settle = disc_ref.days_between(&settlement) as f64 / 365.0; | |
| let df_settle = match self.discount_curve.discount_factor(t_settle) { | |
| Ok(v) if v > 0.0 => v, | |
| _ => return 0.0, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/convex-analytics/src/spreads/discount_margin.rs` around lines 118 -
121, The code currently clamps negative settlement tenor to zero using
t_settle.max(0.0) before calling self.discount_curve.discount_factor, which can
silently misprice; change the call to use the real tenor (pass t_settle
directly) and let discount_factor return an error for negative tenors (or
explicitly check if t_settle < 0.0 and return an error/early failure) instead of
forcing DF(settlement)=DF(0). Update the df_settle binding that calls
self.discount_curve.discount_factor to use t_settle (and optionally add an
explicit negative-tenor branch) so misdated settlements fail fast rather than
being clamped.
Cuts multi-paragraph WHY blocks added during the recent review sweep down to one short line where they earn their keep, drops the ones that just restate the code. Also: cargo test in CI now passes --test-threads=1 since the FFI registry is global state (smoke tests already require this locally). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
This was referenced May 9, 2026
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.
Summary
Replaces the legacy ~70-symbol Rust FFI with a 13-symbol JSON-RPC surface
built around the trader
Markand the typed DTOs inconvex_analytics::dto. The Excel add-in is rewritten on top: cell UDFscollapse from per-input-shape (
CX.PRICE,CX.PRICE.ZSPREAD,CX.YIELD,CX.YIELD.TRUE, ...) to per-question (CX.PRICE,CX.RISK,CX.SPREAD,CX.CASHFLOWS,CX.CURVE.QUERY).After this, adding a new bond shape, spread family, or pricing convention
is a serde enum variant plus a dispatch arm — no new C symbol, no new
P/Invoke, no new UDF.
What changed
Rust
convex-core—Mark::from_str(textual mark grammar: clean/dirty,32nds, yield+frequency, spread+benchmark with optional explicit type)
with full unit tests.
convex-analytics— newdtomodule coveringBondSpec,CurveSpec,PricingRequest,RiskRequest,SpreadRequest,CashflowRequest,CurveQueryRequestand their responses, allserde-tagged.
convex-ffi—lib.rs/build.rs/dispatch.rs/schemas.rsrewritten. Five stateful symbols (build bond/curve from JSON, describe,
release, list/count/clear), five stateless analytics RPCs
(
convex_price/convex_risk/convex_spread/convex_cashflows/
convex_curve_query), three utility (convex_schema,convex_mark_parse,convex_string_free+ version/last-error). TypedDispatchErrorso the JSON envelope is structural rather than stringprefix.
params.govt_curve), I,ASW (par + proceeds), OAS (curve-bumped effective dur/cvx, real
bullet-vs-callable option value), DM (with
params.forward_curveandparams.current_indexfor simple-margin). Credit routes through Z.zero-coupon (closed-form), sinking-fund, all from
BondSpecJSON.key tenor, reprice. One entry per requested tenor.
FixedRateBondBuilder'sidentifiersinvariant is now satisfied for free-form names (regression test in
smoke.rs::build_fixed_rate_bond_with_free_form_name).FRN/zero/sinking-fund pricing and risk, KRD, cashflow tag stability,
schema introspection, mark parsing, and the error envelope.
Excel add-in
System.Text.Json(transitive-DLL pain onnet472 + ExcelDnaPack) in favour of
Newtonsoft.Json— single DLL, notransitive deps, packs cleanly into the
.xll.Cx.cs(P/Invoke + envelope-aware RPC),CxParse.cs(cell coercion),
CxSettings.cs(persisted defaults),Functions.cs(UDF surface),
helpers/{BondSpecs,CurveSpecs,SheetHelpers,IconAtlas}so cell UDFs and ribbon forms share a single source of truth for
request construction.
BondBuilderForm,CurveBuilderForm,PricingTicketForm,SpreadTicketForm,CurveViewerForm,ScenarioForm,ObjectBrowserForm,SchemaBrowserForm,SettingsForm— every one routes through the same JSON RPC the cellUDFs use.
excel/ConvexDemo.xlsx) is now a derived artifactregenerated by
excel/build_demo.py(openpyxl). Six sheets — README,Bonds, Curves, Spreads, Scenarios, Schemas — using the current API
exclusively.
Documentation
CLAUDE.md,excel/README.md,excel/SMOKE_TEST.md,crates/convex-ffi/README.mdrewritten to reflect the currentsurface (textual marks,
CX.*cell UDFs, typed DTOs, errorenvelope, conventions).
Diff stats
−7,838LOC, with broader functional coverage.Test plan
cargo test -p convex-core --lib types::mark(mark parser tests)cargo test -p convex-analytics --lib dto(DTO round-trips)cargo test -p convex-ffi --test smoke --release -- --test-threads=1(20 end-to-end integration tests)
cd excel/Convex.Excel && dotnet build --configuration Release(clean — 0 warnings, 0 errors)
python excel/build_demo.py(regeneratesConvexDemo.xlsx)excel/ConvexDemo.xlsxwith the add-in loaded; every formulaevaluates without
#NAME?. Walk the manual checklist inexcel/SMOKE_TEST.md.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation