feat: nwc#2150
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
+ Coverage 72.77% 72.95% +0.17%
==========================================
Files 356 359 +3
Lines 78079 79276 +1197
==========================================
+ Hits 56822 57834 +1012
- Misses 21257 21442 +185 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
closes #2106 |
|
I have tested this with https://nwc-playground.vercel.app/ |
There was a problem hiding this comment.
I see there is a nwc crate https://github.com/rust-nostr/nostr/tree/master/crates/nwc for the client side of this. I think it would be good to use that to add some end to end tests in for this or maybe just an example would suffice.
| return Ok(false); | ||
| } | ||
|
|
||
| handle_request(&service_keys, &client, handler.as_ref(), &event).await; |
There was a problem hiding this comment.
Right now handle_request(...).await runs inside the relay notification callback, so requests are processed serially. For a pay_invoice request this can take a while: we need to get a melt quote, pay it, and wait for the LN invoice/payment flow to complete. While that is happening, other requests from the client will not be processed.
I wonder if we eventually want to spawn request handling here, probably with a bounded concurrency limit. That said, I think it is fine to leave the implementation serial for now since this only becomes noticeable if a client sends many zaps/requests at once and payments are slow.
| .mint_quote( | ||
| PaymentMethod::Known(KnownMethod::Bolt11), | ||
| Some(amount), | ||
| request.description.clone(), |
There was a problem hiding this comment.
Not all mints support setting the description as its optional https://github.com/cashubtc/nuts/blob/main/23.md#mint-quote. So to avoid silently attempting to create an invoice without the correct description I think we should check the mint info before getting the quote if the description is set in the request.
This is important for ZAPs as the invoice description has to be set to the zap receipt I believe.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- NWC list_transactions reverses newest-first history to oldest-first (medium) - NWC
list_transactionsreturns and paginates transaction history in the opposite order, causing clients to see stale/oldest entries first. - FFI NwcService can leak its background task when dropped without stop() (medium) - Dropping an FFI NWC service without calling stop() can leave the background relay task running indefinitely, leaking connections/resources and potentially continuing to service requests.
| .map_err(|e| nip47_err(ErrorCode::Internal, e.to_string()))?; | ||
|
|
||
| // Newest first, consistent with most NWC clients' expectations. | ||
| transactions.reverse(); |
There was a problem hiding this comment.
Should this reverse() be removed? Wallet::list_transactions already returns transactions newest-first: it calls transactions.sort(), and Transaction::Ord reverses the timestamp comparison. Reversing here flips the NWC response to oldest-first, so offset=0/limit=N returns the oldest transactions instead of the newest ones the comment describes.
| /// [`NwcService::restore`] (existing connection from a persisted client | ||
| /// secret), call [`NwcService::connection_uri`] to obtain the URI for the | ||
| /// Nostr app, then [`NwcService::start`] to begin servicing requests. | ||
| #[derive(uniffi::Object)] |
There was a problem hiding this comment.
Should NwcService clean up the spawned task in Drop as well as in stop()? start() stores a JoinHandle/CancellationToken, but if an FFI consumer drops the object without first calling stop(), the spawned task is detached and continues running with its relay client until the runtime shuts down. A synchronous Drop can at least take() the task, cancel the token, and abort the handle to avoid leaking relay connections/background processing on early returns or consumer mistakes.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- make_invoice silently ignores the NIP-47 expiry request field (low) - NIP-47 clients that request a specific invoice lifetime can receive an invoice with a different mint-chosen expiry without an explicit failure.
- make_invoice omits available invoice creation timestamp (low) - NIP-47 clients receive incomplete invoice metadata from make_invoice and must re-parse or look up the invoice to obtain the creation timestamp that the wallet can already derive.
| }) | ||
| } | ||
|
|
||
| #[instrument(skip(self))] |
There was a problem hiding this comment.
WalletNwcHandler::make_invoice silently ignores the NIP-47 expiry request field. The handler receives MakeInvoiceRequest, but never reads request.expiry; it always calls Wallet::mint_quote(...) without any expiry input and returns expires_at from the mint quote's default expiry.
That means a client requesting a bounded invoice lifetime (for example, a 60-second invoice) can receive an invoice with whatever expiry the mint chooses, with no error indicating the requested lifetime could not be honored. Please either pass the requested expiry through when supported, or reject/document non-None expiry values so callers fail explicitly instead of receiving an invoice with an unexpected lifetime.
There was a problem hiding this comment.
I think we should address this and do the same as we do for description_hash
| .await | ||
| .map_err(|e| mint_quote_error(&e))?; | ||
|
|
||
| let payment_hash = payment_hash_of("e.request); |
There was a problem hiding this comment.
make_invoice returns created_at: None even though it already has the freshly minted Bolt11 invoice (quote.request) and this module already defines invoice_created_at() to extract the invoice creation timestamp. The inconsistency shows up later in lookup_invoice, which does populate created_at for pending mint quotes by parsing the same invoice.
Please populate this field in the immediate make_invoice response as well, e.g. by deriving it from quote.request, so clients get consistent NIP-47 invoice metadata without needing to parse or look the invoice up again.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- FFI NWC service reports stale running state after background task exits (medium) - FFI clients can observe
is_running() == trueafter the NWC service has stopped and cannot restart it withstart()until callingstop()to clear stale state.
Additional locations included in summary:- crates/cdk-ffi/src/nwc.rs:228
| /// Accepts hex or bech32 `nsec`. Derive a stable one from the wallet seed | ||
| /// with [`nwc_derive_service_secret_key_from_seed`]. | ||
| /// * `max_payment_msat` - Optional cap (in millisatoshis) on any single | ||
| /// `pay_invoice` request. |
There was a problem hiding this comment.
start() leaves a completed background task recorded as Some, so the public lifecycle state can become permanently stale. If service.run(...) returns because the relay loop exits or hits a fatal relay/subscription error, the spawned task just logs and finishes, but nothing clears self.task. After that, is_running() still returns true because it only checks g.is_some(), and a subsequent start() fails with "nwc service is already running" because the stale JoinHandle remains in the mutex.
This leaves FFI consumers unable to distinguish a live service from one whose task has already exited, and prevents a normal restart unless they first call stop() despite the service no longer running. Please either clear the slot when the task exits, or have is_running()/start() treat handle.is_finished() as not running and replace the stale handle.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- lookup_invoice omits pending outgoing melt quotes (medium) - NWC clients cannot observe in-progress outgoing payments via lookup_invoice; valid pending payments are reported as not found until they settle into transaction history.
| for tx in &transactions { | ||
| if tx | ||
| .payment_request | ||
| .as_deref() |
There was a problem hiding this comment.
lookup_invoice currently falls through to NotFound for outgoing payments that are still in progress. It checks settled transactions and active mint quotes, but never checks wallet.get_active_melt_quotes(), even though that API returns pending/non-expired unpaid melt quotes.
This means a client looking up a payment hash while pay_invoice is still pending (or after restart/recovery before the melt is finalized into transaction history) will get NotFound instead of a pending outgoing transaction.
Please also search active melt quotes, match by the quote request's payment hash, and return a TransactionType::Outgoing / TransactionState::Pending response with the quote amount, fee reserve/fees as appropriate, invoice, and expiry.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- list_transactions ignores the NWC
unpaidflag (medium) - NWC clients that uselist_transactionswithunpaid: trueget incomplete transaction history and cannot display pending/unpaid invoices from this wallet.
| } | ||
|
|
||
| #[instrument(skip(self))] | ||
| async fn list_transactions( |
There was a problem hiding this comment.
ListTransactionsRequest includes an unpaid flag, but this handler never reads it. It maps only transaction_type, then queries wallet.list_transactions(direction) and applies the time/pagination filters, so a request with unpaid: Some(true) returns the same settled-only history as unpaid: None/false.
That means clients asking NWC to include unpaid/pending invoices will silently miss active quotes, even though this PR already has pending quote data available (for example lookup_invoice checks active mint quotes and returns them as Pending). Please either implement unpaid: true by including active mint/melt quotes in the response, or reject/avoid advertising unsupported semantics rather than ignoring the parameter.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- get_info hardcodes NWC network as mainnet (low) - NWC clients receive an incorrect chain/network advertisement for non-mainnet wallets, which can lead to wrong UI or network-specific client behavior.
| alias: Some("CDK Cashu Wallet".to_string()), | ||
| color: None, | ||
| pubkey: None, | ||
| network: Some("mainnet".to_string()), |
There was a problem hiding this comment.
Should this be omitted or configurable instead of hardcoded to mainnet? GetInfoResponse::network is the NIP-47 chain/network advertisement, but WalletNwcHandler does not receive any network configuration and Wallet/mint info do not carry one. For a wallet backed by a testnet/signet/regtest mint, get_info will still tell NWC clients the wallet is on mainnet. Since the field is optional, returning None until the network is known (or accepting it as handler configuration) would avoid advertising a false network.
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Kotlin FFI bindings do not enable the new
nwcfeature (low) - Kotlin/Android FFI consumers cannot access the new NIP-47 NWC service API added by this PR, while the other generated bindings can.
Additional locations included in summary:- crates/cdk-ffi/src/lib.rs:15
Unanchored locations included in summary: - bindings/kotlin/rust/Cargo.toml:20
- crates/cdk-ffi/src/lib.rs:15
thesimplekid
left a comment
thesimplekid
left a comment
There was a problem hiding this comment.
Two small things we should address before merge.
I think there is a bit of clean up we can do and combine our nostr stack (nwc and npubcash) but we can do that in a future PR and release.
| TagKind::Custom(std::borrow::Cow::Borrowed("encryption")), | ||
| ["nip44_v2".to_string(), "nip04".to_string()], | ||
| ); | ||
|
|
There was a problem hiding this comment.
I think this is not to the nip47 spec. https://github.com/nostr-protocol/nips/blob/master/47.md#info-event.
It should be ["encryption", "nip44_v2 nip04"] not ["encryption", "nip44_v2", "nip04"]
| }) | ||
| } | ||
|
|
||
| #[instrument(skip(self))] |
There was a problem hiding this comment.
I think we should address this and do the same as we do for description_hash
thesimplekid
force-pushed
the
feat/nwc
branch
2 times, most recently
from
562ebaa to
7e3b3fb
Compare
Introduce a transport-agnostic cdk-nwc crate that connects to Nostr relays, advertises supported NWC commands, validates authorized requests, deduplicates event ids, and publishes encrypted responses. Add a CDK wallet handler for get_info, get_balance, make_invoice, pay_invoice, lookup_invoice, and list_transactions, including msat/unit conversion, quote lookup, and per-payment limits. Expose the service through UniFFI and add an example plus an end-to-end relay-backed NWC flow test.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin v0.17.x
git worktree add -d .worktree/backport-2150-to-v0.17.x origin/v0.17.x
cd .worktree/backport-2150-to-v0.17.x
git switch --create backport-2150-to-v0.17.x
git cherry-pick -x 10cab2ab3a8c6817e9f5a70ce5a5361e07c2430b |
3 participants
Description
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)