Skip to content

Add ContextVM FFI bindings#84

Open
aljazceru wants to merge 2 commits into
ContextVM:mainfrom
nostr-net:contextvm-ffi
Open

Add ContextVM FFI bindings#84
aljazceru wants to merge 2 commits into
ContextVM:mainfrom
nostr-net:contextvm-ffi

Conversation

@aljazceru

Copy link
Copy Markdown
  • only done quick smoke testing

@harsh04044 harsh04044 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall implementation looks solid, nice work getting both the C and UniFFI surfaces wired up. A few things I noticed:

supports_encryption is hardcoded to true: cvm_discover_servers sets this to true for every announcement, but the SDK's ServerAnnouncement doesn't even have a supports_encryption field. This will tell FFI consumers every server supports encryption regardless of what it actually advertised.

Global mutex blocks all concurrent FFI calls - kv.rs uses a single Mutex for the entire handle store, and the guard is held for the lifetime of the returned reference. In cvm_server_ch_recv (and the client/gateway/proxy equivalents), the guard stays alive through block_on(channel.receiver.recv()), which can block indefinitely. While that's happening, every other FFI call - even unrelated ones like cvm_keys_generate, will be stuck waiting on that same mutex. This basically makes multi-channel usage single-threaded and deadlock-prone. Probably needs per-handle locking or Arc<Mutex> per stored object instead of one global lock.

UniFFI Server is missing close() - the C API has cvm_server_ch_close but the UniFFI Server object only exposes recv, send_response, and announce. Python/Swift/Kotlin consumers have no way to shut down a server cleanly.

Duplicate set_error - same function with the same body exists in both channel.rs:790 and types.rs:632, should probably live in one place.

Crate isn't in the workspace - cargo test --all and cargo clippy from the repo root don't touch contextvm-ffi, so CI won't catch regressions here.

@aljazceru aljazceru marked this pull request as ready for review May 29, 2026 14:25
@aljazceru

Copy link
Copy Markdown
Author

@harsh04044 thanks for the review, i've added fixes and expanded the feature set a bit more, lmk if theres any more fixes needed

@harsh04044

harsh04044 commented Jun 1, 2026

Copy link
Copy Markdown

@harsh04044 thanks for the review, i've added fixes and expanded the feature set a bit more, lmk if theres any more fixes needed

Everything addressed! LGTM 🚀

@aljazceru

Copy link
Copy Markdown
Author

so i guess its time to bully @ContextVM-org

@ContextVM-org

Copy link
Copy Markdown
Collaborator

This is a strong addition overall. One design question: proxy exposes timed receive via cvm_proxy_ch_recv_timeout(), while server/client/gateway currently only expose blocking receive calls such as cvm_server_ch_recv() and cvm_client_ch_recv(). That may be perfectly fine if the intended foreign-language model is dedicated worker-thread consumption, since the SDK/rmcp already own request lifecycle timeouts. But if these bindings are meant for more general embedding in Python/Swift/Kotlin runtimes, it would be worth either documenting the blocking-thread expectation explicitly or offering timeout/try-recv variants consistently across channel types. Wdyt?

Secondary concern: the FFI boundary still has a few sharp edges around lifecycle/ownership semantics. In particular, set_error() always allocates a fresh error object, and callers must free each one with cvm_error_free(); that contract should be made extremely explicit in the docs. Similarly, the global handle store in kv.rs relies on Mutex::lock().unwrap(), which is fine internally until a panic poisons the store and turns future FFI calls into panics across the boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants