JVM bindings, hedge-advisor FFI, and Excel concurrency/handle fixes#112
Merged
Conversation
- Add convex_risk_profile / convex_hedge / convex_compare wrapping the risk/hedging module (compute_position_risk, the strategy builders, compare_hedges, narrate). RiskProfile round-trips back into the hedge and compare calls, so no server-side session is needed. - registry: clone the object's Arc out under the read lock and run the caller's closure after releasing it. Holding the lock across analytics nested a second read lock (e.g. KRD re-fetching the curve) and could deadlock under concurrent access. - registry: add an optional registry_key to BondIdentifier and the curve specs; when set it is the eviction slot (preferred over cusip/isin/name), letting a host scope objects per owning cell instead of per security. - Generate the C ABI header via cbindgen (build.rs + cbindgen.toml). - Smoke tests for the advisor flow and per-key eviction.
- New single-module Maven project under bindings/java (io.github.sujitn:convex-java, JDK 22+). A typed API — builders, BigDecimal/LocalDate, immutable result records, AutoCloseable handles — over a hidden FFM + Jackson layer; the JSON-RPC boundary is not exposed. - Covers fixed-rate / callable / floating-rate / zero-coupon / sinking-fund bonds, curves, pricing, risk, spreads, cashflows, make-whole, and the hedge advisor. - NativeLoader extracts the platform cdylib bundled in the jar. - CI: java.yml (build cdylib + mvn verify on JDK 22) and java-release.yml (per-platform native matrix + signed deploy to Maven Central).
- Build bonds/curves with registry_key = calling cell (xlfCaller) so two cells referencing the same CUSIP no longer evict each other's handle. Previously, because Excel's calc order across cells is nondeterministic, the losing cell would intermittently show stale or #ERROR values on recalc. - Mark the stateless analytics UDFs (PRICE/RISK/SPREAD/CASHFLOWS/MW/ CURVE.QUERY) IsThreadSafe for multi-threaded recalc; builders stay on the main thread (they mutate the registry and use xlfCaller).
📝 WalkthroughWalkthroughAdds Java FFM bindings and typed analytics APIs, new Rust DTOs/dispatch for risk/hedge/compare, registry refactor, tests, Excel caller-scoped registry keying, and GitHub Actions for verify and multi-platform Java release with native artifact staging. ChangesJava Bindings & Hedge Advisor
Sequence DiagramsequenceDiagram
participant JavaApp
participant ConvexAnalytics
participant ConvexFfi
participant NativeLib
JavaApp->>ConvexFfi: ensure native library loaded
JavaApp->>ConvexAnalytics: call typed API (price/risk/hedge)
ConvexAnalytics->>ConvexFfi: build JSON request -> rpc(...)
ConvexFfi->>NativeLib: convex_* JSON RPC call (C ABI)
NativeLib-->>ConvexFfi: returns owned C-string pointer
ConvexFfi->>ConvexAnalytics: unwrap envelope -> JsonNode
ConvexAnalytics-->>JavaApp: typed result (Pricing/Risk/Hedge)
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (2)
bindings/java/src/main/java/com/convex/Mark.java (1)
17-63: ⚡ Quick winGive
Markvalue semantics.As an immutable wrapper around a single wire value, identity-based equality is going to be surprising in collections and tests. Implement
equals/hashCodefromtextso the type behaves like the value object its API suggests.Suggested change
`@Override` public String toString() { return text; } + + `@Override` + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Mark other)) return false; + return text.equals(other.text); + } + + `@Override` + public int hashCode() { + return text.hashCode(); + } }🤖 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 `@bindings/java/src/main/java/com/convex/Mark.java` around lines 17 - 63, Add value semantics to Mark by implementing equals(Object) and hashCode() that delegate to the internal text field: equals should return true for the same object, then check that the other is a Mark (instanceof), cast and compare this.text to other.text (safely handling null), and hashCode should return Objects.hashCode(text) (or text.hashCode() if non-null). Implement these methods on the Mark class so collections and tests compare by text rather than by identity.bindings/java/src/main/java/com/convex/Specs.java (1)
41-49: ⚡ Quick winReject conflicting identifiers instead of silently picking the first.
If a caller sets both CUSIP and ISIN/name, this helper drops the later values with no signal. That makes the public builder state misleading and can serialize the wrong identity.
♻️ Proposed fix
static void applyIdentifier(ObjectNode n, String cusip, String isin, String name) { + int count = 0; + if (cusip != null) count++; + if (isin != null) count++; + if (name != null) count++; + if (count > 1) { + throw new IllegalArgumentException("spec accepts only one of cusip, isin, or name"); + } + if (cusip != null) { n.put("cusip", cusip); } else if (isin != null) { n.put("isin", isin); } else if (name != null) {🤖 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 `@bindings/java/src/main/java/com/convex/Specs.java` around lines 41 - 49, The applyIdentifier helper currently silently prefers cusip over isin/name; change it to detect conflicting non-null identifiers and reject them: in Specs.applyIdentifier(ObjectNode n, String cusip, String isin, String name) count the non-null identifier arguments and if more than one is provided throw an IllegalArgumentException with a clear message; otherwise, when exactly one is non-null write the corresponding field ("cusip", "isin", or "name") to the ObjectNode as before. Ensure the exception is used to surface invalid builder state rather than dropping values.
🤖 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 @.github/workflows/java-release.yml:
- Around line 4-12: Add an explicit minimal permissions: block to the workflow
so the tag-triggered job doesn't inherit broad repo token scope; update the
top-level YAML (near the existing on: push: tags: and env: keys) to declare only
the required scopes (for a tag build/deploy typically at minimum permissions:
contents: read and packages: write, and add id-token: write only if you use
OIDC) so the workflow jobs have least privilege.
- Around line 27-29: The workflow uses mutable action tags (e.g.,
actions/checkout@v6, dtolnay/rust-toolchain@stable, actions/upload-artifact@v4,
actions/download-artifact@v4, actions/setup-java@v4) which must be replaced with
exact commit SHAs; locate every uses: entry in the java-release.yml that matches
those action identifiers and replace the tag (e.g., `@v6`, `@stable`, `@v4`) with the
corresponding pinned full commit SHA from the action's repository (verify the
correct commit for the intended version), ensuring you update all occurrences of
actions/checkout, dtolnay/rust-toolchain, actions/upload-artifact,
actions/download-artifact, and actions/setup-java consistently.
In @.github/workflows/java.yml:
- Around line 6-17: Update the workflow path filters so changes to convex-ffi's
dependency crates trigger the job: modify both the push and pull_request "paths"
lists (currently listing 'bindings/java/**', 'crates/convex-ffi/**',
'crates/convex-analytics/**', '.github/workflows/java.yml') to also include the
dependency crates—e.g. add 'crates/convex-bonds/**', 'crates/convex-core/**',
and 'crates/convex-curves/**' or broaden to 'crates/**' or 'crates/convex-**/**'
so edits to those crates won't skip the Java/native integration build.
- Around line 29-47: Replace tag-based GitHub Action refs in the workflow (e.g.,
actions/checkout@v6, dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2,
actions/setup-java@v4) with the corresponding immutable full commit SHAs; locate
each uses: entry in .github/workflows/java.yml and update the ref to the
specific commit SHA from the action's repository (e.g.,
actions/checkout@<full-sha>) so the workflow is pinned to exact versions and
future tag drift cannot change behavior.
- Line 29: Update the checkout step using actions/checkout@v6 to disable
persisted Git credentials by adding persist-credentials: false to that step;
locate the workflow job where "uses: actions/checkout@v6" is declared and add
the persist-credentials: false input so the job only checks out code without
saving GitHub credentials for later steps.
In `@bindings/java/README.md`:
- Around line 12-16: Label the unlabeled fenced block that lists the packages
(the block containing "com.convex", "com.convex.internal", and
"crates/convex-ffi") by adding a language identifier to the opening fence (e.g.
change ``` to ```text or ```plain) so the README's code fence is explicitly
marked as plain text and satisfies markdownlint; leave the block content
unchanged and only modify the opening fence token.
In `@bindings/java/src/main/java/com/convex/Bond.java`:
- Around line 19-45: The Bond wrapper currently exposes handle() and describe()
after close(), so add a closed state flag (e.g., a private final AtomicBoolean
closed or volatile boolean closed) initialized in the Bond(long) constructor,
set it to true in close() after calling cleanable.clean(), and update handle()
and describe() to check the flag and throw IllegalStateException (e.g., "Bond is
closed") if closed; ensure close() remains idempotent (only sets the flag and
calls clean once) and keep constructor, handle(), describe(), and close() as the
touchpoints to implement this guard.
In `@bindings/java/src/main/java/com/convex/CallableBond.java`:
- Around line 78-88: The build() method currently forces at least one call entry
and permits a MAKE_WHOLE style without a spread; change it so the method only
rejects creation when both calls and puts are empty (i.e., allow putable-only
bonds by checking calls.isEmpty() && puts.isEmpty()), and add a validation that
if callStyle equals MAKE_WHOLE then makeWholeSpreadBps must be non-null (throw
IllegalArgumentException with a clear message if it's missing); keep the
existing spec population (spec.put("call_style", ...)) but only write
"make_whole_spread_bps" when makeWholeSpreadBps is present after the new
validation.
In `@bindings/java/src/main/java/com/convex/FloatingRateNote.java`:
- Around line 41-47: The Builder setters in FloatingRateNote (methods
rateIndex(...), maturity(...), issue(...), frequency(...), dayCount(...),
currency(...), faceValue(...)) currently allow replacing defaults with null and
defer failure to build()/wire(); update each setter to reject null immediately
(e.g., use Objects.requireNonNull or throw IllegalArgumentException) and return
this, so invalid nulls fail fast; apply the same null-check pattern to the other
Builder setters in this class that mirror these methods and ensure
build()/wire() can assume non-null values.
In `@bindings/java/src/main/java/com/convex/HedgeAdvisor.java`:
- Around line 112-123: The hedge strategy currently overwrites the caller's
curve id with the hardcoded string "discount"; in HedgeAdvisor.strategy replace
the hardcoded req.put("curve_id", "discount") with the caller-provided id (e.g.
use curve.id() or curve.getId() on the YieldCurve) so the request preserves the
original curve identifier supplied by PositionRiskBuilder/RiskProfile instead of
forcing "discount".
- Around line 141-144: The current code in HedgeAdvisor that builds a
ComparisonReport from Json.unwrap(ConvexFfi.compare(Json.write(req))) does not
verify that the returned JSON contains a non-null "report" field, causing a
later NPE; update the code that reads result/get("report") to validate
result.hasNonNull("report") (or result.has("report") &&
!result.get("report").isNull()) and if missing throw a clear boundary error
(e.g., IllegalStateException or a specific boundary exception) with context
including the raw result and the request, so you fail fast instead of
constructing a ComparisonReport with a null JsonNode; keep the existing
extraction of "narrative" but only construct ComparisonReport when "report" is
present.
In `@bindings/java/src/main/java/com/convex/internal/Json.java`:
- Around line 77-85: The current dbl(JsonNode node, String field) and
optDbl(JsonNode node, String field) silently coerce errors (returning Double.NaN
or 0.0) for missing or non-numeric JSON; change dbl to fail-fast by validating
the field exists and is a numeric node and throw a ConvexException (with a clear
message including field name and offending value) when missing or non-numeric,
and change optDbl to only return OptionalDouble.empty() when the field is
missing or null but otherwise validate that a present field is numeric and throw
ConvexException on non-numeric values; update references to dbl and optDbl
accordingly so callers get explicit errors for schema/FFI mismatches.
In `@bindings/java/src/main/java/com/convex/RiskProfile.java`:
- Around line 55-57: RiskProfile.raw() currently returns the backing JsonNode
(field node) which can be mutated by callers; change the method to return a
defensive deep copy so consumers cannot modify the internal state. Update
RiskProfile.raw() to return node.deepCopy() (as a JsonNode) instead of node,
ensuring the backing field 'node' stays immutable to callers while keeping the
method signature intact.
In `@bindings/java/src/main/java/com/convex/Specs.java`:
- Around line 20-37: The fixedRate helper currently only validates couponRate,
issue, and maturity; add null checks for frequency, dayCount, currency, and
faceValue before using their methods so callers fail fast. Specifically, in
Specs.fixedRate call require(frequency, "frequency") before frequency.wire(),
require(dayCount, "dayCount") before dayCount.wire(), require(currency,
"currency") before currency.wire(), and require(faceValue, "faceValue") (or
otherwise enforce non-null) before putting face_value to avoid NPEs and silent
null JSON.
In `@bindings/java/src/main/java/com/convex/ZeroCouponBond.java`:
- Around line 37-55: Reject nulls for the builder fields to fail fast: add null
checks in Builder.compounding(Compounding), Builder.dayCount(DayCount),
Builder.currency(Currency) and Builder.faceValue(BigDecimal) that throw an
IllegalArgumentException or NPE when passed null, and/or add validation at the
start of Builder.build() (e.g., require compounding, dayCount, currency and
faceValue before calling .wire()) so that wire() is never invoked on a null and
Bond.fromSpecJson receives valid JSON.
In `@bindings/java/src/main/resources/native/README.md`:
- Around line 6-21: Update the two fenced code blocks in
bindings/java/src/main/resources/native/README.md to include language
identifiers: add "text" (or "bash") to the first block that lists the native/
tree entries and add "sh" to the second block that shows the cargo build and cp
commands so markdownlint stops complaining and the shell snippet renders
correctly.
In `@crates/convex-ffi/src/schemas.rs`:
- Around line 24-26: Add the missing response schemas for the advisor verbs by
registering the corresponding response shape constants alongside the existing
request entries: for each of "RiskProfileRequest", "HedgeRequest", and
"CompareRequest" add their response counterparts (e.g., the RiskProfile,
HedgeProposal, and the compare response/report schema) to the schema registry
used in this file (the same map/block where RISK_PROFILE_REQUEST, HEDGE_REQUEST,
COMPARE_REQUEST are referenced); update the matching symbol names (e.g.,
RISK_PROFILE_RESPONSE, HEDGE_PROPOSAL_RESPONSE, COMPARE_RESPONSE or whatever
response constant names your crate defines) and ensure you also add equivalent
entries in the later section referenced (lines ~227-273) so the introspection
API sees both request and response shapes for each advisor RPC.
In `@crates/convex-ffi/tests/smoke.rs`:
- Around line 12-19: The zero_coupon_5y and frn_5y helpers still use hard-coded
identifiers and can collide in parallel tests; update those functions to
generate per-build unique IDs using the existing SEQ/uid helper (the same
pattern used by fixed_rate_5pct). Locate zero_coupon_5y and frn_5y in the test
file and replace any string literals used as register/identifier names with
calls to uid("your_prefix_") (choose distinct prefixes like "zc_" and "frn_"),
ensuring all places that referenced the old literal now use the returned uid so
each test run gets a unique name.
---
Nitpick comments:
In `@bindings/java/src/main/java/com/convex/Mark.java`:
- Around line 17-63: Add value semantics to Mark by implementing equals(Object)
and hashCode() that delegate to the internal text field: equals should return
true for the same object, then check that the other is a Mark (instanceof), cast
and compare this.text to other.text (safely handling null), and hashCode should
return Objects.hashCode(text) (or text.hashCode() if non-null). Implement these
methods on the Mark class so collections and tests compare by text rather than
by identity.
In `@bindings/java/src/main/java/com/convex/Specs.java`:
- Around line 41-49: The applyIdentifier helper currently silently prefers cusip
over isin/name; change it to detect conflicting non-null identifiers and reject
them: in Specs.applyIdentifier(ObjectNode n, String cusip, String isin, String
name) count the non-null identifier arguments and if more than one is provided
throw an IllegalArgumentException with a clear message; otherwise, when exactly
one is non-null write the corresponding field ("cusip", "isin", or "name") to
the ObjectNode as before. Ensure the exception is used to surface invalid
builder state rather than dropping values.
🪄 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: 2885fa51-04e2-42f8-9f45-c156c6cf19e6
📒 Files selected for processing (48)
.github/workflows/java-release.yml.github/workflows/java.ymlbindings/java/.gitignorebindings/java/README.mdbindings/java/native-headers/convex.hbindings/java/pom.xmlbindings/java/src/main/java/com/convex/Bond.javabindings/java/src/main/java/com/convex/CallStyle.javabindings/java/src/main/java/com/convex/CallableBond.javabindings/java/src/main/java/com/convex/ComparisonReport.javabindings/java/src/main/java/com/convex/Compounding.javabindings/java/src/main/java/com/convex/Constraints.javabindings/java/src/main/java/com/convex/Convex.javabindings/java/src/main/java/com/convex/ConvexAnalytics.javabindings/java/src/main/java/com/convex/ConvexException.javabindings/java/src/main/java/com/convex/Currency.javabindings/java/src/main/java/com/convex/DayCount.javabindings/java/src/main/java/com/convex/FixedRateBond.javabindings/java/src/main/java/com/convex/FloatingRateNote.javabindings/java/src/main/java/com/convex/Frequency.javabindings/java/src/main/java/com/convex/HedgeAdvisor.javabindings/java/src/main/java/com/convex/HedgeProposal.javabindings/java/src/main/java/com/convex/Interpolation.javabindings/java/src/main/java/com/convex/Mark.javabindings/java/src/main/java/com/convex/RateIndex.javabindings/java/src/main/java/com/convex/RiskProfile.javabindings/java/src/main/java/com/convex/SinkingFundBond.javabindings/java/src/main/java/com/convex/Specs.javabindings/java/src/main/java/com/convex/SpreadType.javabindings/java/src/main/java/com/convex/YieldCurve.javabindings/java/src/main/java/com/convex/ZeroCouponBond.javabindings/java/src/main/java/com/convex/internal/ConvexFfi.javabindings/java/src/main/java/com/convex/internal/Json.javabindings/java/src/main/java/com/convex/internal/NativeLoader.javabindings/java/src/main/java/com/convex/internal/NativeRef.javabindings/java/src/main/resources/native/README.mdbindings/java/src/test/java/com/convex/SmokeTest.javacrates/convex-analytics/src/dto.rscrates/convex-ffi/build.rscrates/convex-ffi/cbindgen.tomlcrates/convex-ffi/src/build.rscrates/convex-ffi/src/dispatch.rscrates/convex-ffi/src/lib.rscrates/convex-ffi/src/registry.rscrates/convex-ffi/src/schemas.rscrates/convex-ffi/tests/smoke.rsexcel/Convex.Excel/Cx.csexcel/Convex.Excel/Functions.cs
Comment on lines
+29
to
+47
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Cache cargo | ||
| uses: Swatinem/rust-cache@v2 | ||
|
|
||
| - name: Build convex-ffi cdylib | ||
| run: cargo build -p convex-ffi --release | ||
|
|
||
| - name: Stage native library | ||
| run: | | ||
| dest=bindings/java/src/main/resources/native/linux-x86_64 | ||
| mkdir -p "$dest" | ||
| cp target/release/libconvex_ffi.so "$dest/" | ||
|
|
||
| - name: Set up JDK 22 | ||
| uses: actions/setup-java@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/java.yml"
echo "== File exists? =="
ls -l "$FILE"
echo "== Lines 1-120 (for context) =="
cat -n "$FILE" | sed -n '1,120p'
echo "== uses: entries with line numbers =="
rg -n "^\s*-\s*uses:\s*" "$FILE" || true
echo "== All uses: occurrences (broader) =="
rg -n "uses:\s*" "$FILE" || trueRepository: sujitn/convex
Length of output: 2210
Pin the third-party GitHub Actions to full commit SHAs.
.github/workflows/java.yml uses tag-based actions (actions/checkout@v6, dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2, actions/setup-java@v4) rather than immutable commit SHAs, leaving the workflow exposed to supply-chain drift.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 29-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 29-29: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 47-47: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/java.yml around lines 29 - 47, Replace tag-based GitHub
Action refs in the workflow (e.g., actions/checkout@v6,
dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2, actions/setup-java@v4)
with the corresponding immutable full commit SHAs; locate each uses: entry in
.github/workflows/java.yml and update the ref to the specific commit SHA from
the action's repository (e.g., actions/checkout@<full-sha>) so the workflow is
pinned to exact versions and future tag drift cannot change behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
20 issues found across 48 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="bindings/java/src/main/java/com/convex/RiskProfile.java">
<violation number="1" location="bindings/java/src/main/java/com/convex/RiskProfile.java:49">
P2: Bucket parsing defaults missing values to `0.0`, which can silently hide malformed key-rate data.</violation>
</file>
<file name="bindings/java/src/main/java/com/convex/Bond.java">
<violation number="1" location="bindings/java/src/main/java/com/convex/Bond.java:19">
P2: After `close()` is called, the `handle` field still holds the original value and `handle()` / `describe()` remain callable, passing a freed handle to native code. This can cause undefined behavior in the native layer. Consider using `AtomicLong` to track closed state and reject access after release.</violation>
</file>
<file name="bindings/java/src/main/java/com/convex/CallableBond.java">
<violation number="1" location="bindings/java/src/main/java/com/convex/CallableBond.java:79">
P2: This validation rejects putable-only bonds — `put()` is a public method on the builder, but `build()` unconditionally requires at least one call entry. Should be `if (calls.isEmpty() && puts.isEmpty())` to allow pure putable bonds. Additionally, `MAKE_WHOLE` style is accepted without `makeWholeSpreadBps`, which will push an invalid spec downstream.</violation>
</file>
<file name="bindings/java/src/main/java/com/convex/internal/Json.java">
<violation number="1" location="bindings/java/src/main/java/com/convex/internal/Json.java:79">
P2: `dbl()` silently coerces invalid JSON types to `0.0` via `asDouble()`, which can hide malformed native responses as valid analytics values.</violation>
<violation number="2" location="bindings/java/src/main/java/com/convex/internal/Json.java:85">
P2: `optDbl()` can return `OptionalDouble.of(0.0)` for non-numeric JSON due to `asDouble()` coercion, masking invalid payloads.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| JsonNode arr = node.get("key_rate_buckets"); | ||
| if (arr != null && arr.isArray()) { | ||
| for (JsonNode b : arr) { | ||
| out.add(new Bucket(b.path("tenor_years").asDouble(), b.path("partial_dv01").asDouble())); |
There was a problem hiding this comment.
P2: Bucket parsing defaults missing values to 0.0, which can silently hide malformed key-rate data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bindings/java/src/main/java/com/convex/RiskProfile.java, line 49:
<comment>Bucket parsing defaults missing values to `0.0`, which can silently hide malformed key-rate data.</comment>
<file context>
@@ -0,0 +1,59 @@
+ JsonNode arr = node.get("key_rate_buckets");
+ if (arr != null && arr.isArray()) {
+ for (JsonNode b : arr) {
+ out.add(new Bucket(b.path("tenor_years").asDouble(), b.path("partial_dv01").asDouble()));
+ }
+ }
</file context>
| */ | ||
| public final class Bond implements AutoCloseable { | ||
|
|
||
| private final long handle; |
There was a problem hiding this comment.
P2: After close() is called, the handle field still holds the original value and handle() / describe() remain callable, passing a freed handle to native code. This can cause undefined behavior in the native layer. Consider using AtomicLong to track closed state and reject access after release.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bindings/java/src/main/java/com/convex/Bond.java, line 19:
<comment>After `close()` is called, the `handle` field still holds the original value and `handle()` / `describe()` remain callable, passing a freed handle to native code. This can cause undefined behavior in the native layer. Consider using `AtomicLong` to track closed state and reject access after release.</comment>
<file context>
@@ -0,0 +1,46 @@
+ */
+public final class Bond implements AutoCloseable {
+
+ private final long handle;
+ private final Cleaner.Cleanable cleanable;
+
</file context>
| } | ||
|
|
||
| public Bond build() { | ||
| if (calls.isEmpty()) { |
There was a problem hiding this comment.
P2: This validation rejects putable-only bonds — put() is a public method on the builder, but build() unconditionally requires at least one call entry. Should be if (calls.isEmpty() && puts.isEmpty()) to allow pure putable bonds. Additionally, MAKE_WHOLE style is accepted without makeWholeSpreadBps, which will push an invalid spec downstream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bindings/java/src/main/java/com/convex/CallableBond.java, line 79:
<comment>This validation rejects putable-only bonds — `put()` is a public method on the builder, but `build()` unconditionally requires at least one call entry. Should be `if (calls.isEmpty() && puts.isEmpty())` to allow pure putable bonds. Additionally, `MAKE_WHOLE` style is accepted without `makeWholeSpreadBps`, which will push an invalid spec downstream.</comment>
<file context>
@@ -0,0 +1,110 @@
+ }
+
+ public Bond build() {
+ if (calls.isEmpty()) {
+ throw new IllegalArgumentException("callable bond requires at least one call entry");
+ }
</file context>
- ffi(schemas): add advisor response schemas (RiskProfile, HedgeProposal, CompareResponse) so each advisor verb has request/response symmetry. - ffi(tests): give the zero_coupon_5y / frn_5y helpers unique per-build ids (uid) so parallel runs don't evict each other's handle; apply rustfmt. - java: fail fast on null builder fields in Specs.fixedRate, ZeroCouponBond, and FloatingRateNote instead of NPE-ing on .wire() / emitting null JSON. - ci: declare least-privilege `permissions: contents: read` on both Java workflows; trigger java.yml when convex-ffi's dependency crates change (convex-core/-math/-curves/-bonds). - docs: add language identifiers to the native/README fenced code blocks.
- java: match the Rust enums — Currency gains the 11 missing ISO codes (NZD/SEK/NOK/DKK/HKD/SGD/CNY/INR/BRL/MXN/ZAR); DayCount drops Thirty360Icma/ Simple/Daily (no such DayCountConvention variants) and adds Act365Leap/ ActActAfb/Thirty360EIsda/Thirty360German. - java: HedgeAdvisor uses the position's own curve_id (from provenance) instead of hardcoding "discount", and rejects a compare response missing "report". - java: wrap System.load failures as ConvexException; validate FRN cap >= floor and that make-whole call style carries a spread. - java: drop CallableBond.put() — the FFI's callable builder ignores put_schedule, so it was unwired surface. - ffi: cbindgen build script reruns on any src change; constrain the position field to an object in the hedge/compare request schemas. - ci(java-release): pin actions to commit SHAs (release pipeline carries publishing secrets) and set persist-credentials: false; java.yml also sets persist-credentials: false.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/convex-ffi/build.rs (1)
33-37: ⚡ Quick winDon’t fail the Rust cdylib build on cbindgen—gate a CI check that regenerates the committed header instead.
crates/convex-ffi/build.rswrites the header to$OUT_DIR/convex.h; the checked-in consumer headerbindings/java/native-headers/convex.his refreshed manually via thecbindgenCLI, and Java doesn’t consume the header at build time. Panicking here would breakcargo build -p convex-ffiwithout fixing the “stale checked-in header” risk. Add a CI/release step that runscbindgen --config crates/convex-ffi/cbindgen.toml --crate convex-ffi --output bindings/java/native-headers/convex.hand fails if it can’t generate.🤖 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-ffi/build.rs` around lines 33 - 37, The build script in crates/convex-ffi/build.rs currently surfaces cbindgen failures as a warning (Err(e) => println!("cargo:warning=...")), which is correct—do not change it to panic; instead add a CI/release check that runs cbindgen --config crates/convex-ffi/cbindgen.toml --crate convex-ffi --output bindings/java/native-headers/convex.h and fails the CI job if cbindgen errors, ensuring the committed header is regenerated and kept up-to-date while allowing cargo build -p convex-ffi to succeed locally; keep the Err(e) warning in build.rs (refer to the Err(e) arm) and implement a new CI step that executes the above command and exits non-zero on failure.
🤖 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.
Nitpick comments:
In `@crates/convex-ffi/build.rs`:
- Around line 33-37: The build script in crates/convex-ffi/build.rs currently
surfaces cbindgen failures as a warning (Err(e) =>
println!("cargo:warning=...")), which is correct—do not change it to panic;
instead add a CI/release check that runs cbindgen --config
crates/convex-ffi/cbindgen.toml --crate convex-ffi --output
bindings/java/native-headers/convex.h and fails the CI job if cbindgen errors,
ensuring the committed header is regenerated and kept up-to-date while allowing
cargo build -p convex-ffi to succeed locally; keep the Err(e) warning in
build.rs (refer to the Err(e) arm) and implement a new CI step that executes the
above command and exits non-zero on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d775335-66b4-4652-b2d2-7dfe59ad52e2
📒 Files selected for processing (10)
.github/workflows/java-release.yml.github/workflows/java.ymlbindings/java/src/main/java/com/convex/CallableBond.javabindings/java/src/main/java/com/convex/Currency.javabindings/java/src/main/java/com/convex/DayCount.javabindings/java/src/main/java/com/convex/FloatingRateNote.javabindings/java/src/main/java/com/convex/HedgeAdvisor.javabindings/java/src/main/java/com/convex/internal/NativeLoader.javacrates/convex-ffi/build.rscrates/convex-ffi/src/schemas.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- bindings/java/src/main/java/com/convex/CallableBond.java
- crates/convex-ffi/src/schemas.rs
- .github/workflows/java.yml
- bindings/java/src/main/java/com/convex/FloatingRateNote.java
- bindings/java/src/main/java/com/convex/internal/NativeLoader.java
- bindings/java/src/main/java/com/convex/HedgeAdvisor.java
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
Adds JVM bindings for the analytics library, expands the C FFI to cover the
hedge advisor, fixes a registry concurrency hazard, and fixes an Excel handle
bug that surfaced as intermittent stale/
#ERRORcells on recalc.FFI (Rust)
convex_risk_profile/convex_hedge/convex_comparewrapping therisk/hedging module (position risk, the strategy builders,
compare_hedges,narrate). TheRiskProfileis value-serializable and round-trips back intothe hedge/compare calls — no server-side session.
with_objectnow clones the object'sArcoutunder the read lock and runs the caller's closure after releasing it. Holding
the lock across analytics nested a second read lock (e.g. KRD re-fetching the
curve) and could deadlock under concurrent calls.
registry_key: optional eviction-slot key onBondIdentifierand thecurve specs, preferred over cusip/isin/name. Lets a host scope objects to the
owning cell rather than the security identifier (used by the Excel fix below).
build.rs+cbindgen.toml).Java bindings (
bindings/java)io.github.sujitn:convex-java, JDK 22+,using the Panama Foreign Function & Memory API — no JNI/C shim. A typed API
(builders,
BigDecimal/LocalDate, immutable result records,AutoCloseablehandles) hides the JSON-RPC boundary.
bonds, curves, pricing, risk, spreads, cashflows, make-whole, and the hedge
advisor.
NativeLoaderextracts the platform cdylib bundled in the jar.java.yml(build cdylib +mvn verifyon JDK 22) andjava-release.yml(per-platform native matrix + signed deploy to Maven Central).
Excel add-in
registry_key= the calling cell (xlfCaller) so twocells referencing the same CUSIP no longer evict each other's handle. Excel's
calc order across cells is nondeterministic, so the losing cell would
intermittently show stale/
#ERRORvalues on recalc.PRICE/RISK/SPREAD/CASHFLOWS/MW/CURVE.QUERY) are nowIsThreadSafefor multi-threaded recalc; builders stayon the main thread (they mutate the registry and use
xlfCaller).Testing
cargo test -p convex-ffi— 28 pass (incl. advisor flow + per-cell evictionregression); full workspace builds.
mvn verifyon JDK 25 —SmokeTest7/7 against the realconvex_ffi.dll(FFM marshalling, all bond builders, analytics, hedge-advisor round-trip,
handle-leak check).
mvn -Prelease packagebuilds jar + sources + javadoc.dotnet build(net472).Notes
registry_keyis optional and backward compatible (no C ABI change).MAVEN_CENTRAL_*, GPG key); theworkflow and release profile are in place but the publish step is untested.
Hull-White calibration, piecewise bootstrap, FRN projection/cap-floor.
Summary by CodeRabbit
New Features
Documentation
CI / Release
Tests
Bug Fixes