Single nostr client

[tompro]

User description

📝 Description

This replaces the multi Nostr client setup with a single Nostr client that has access to multiple Nostr signers. It has wide implications so there could be a lot of pitfalls and potential issues. I have tested a lot with both public block message and private invites and shares so there should be not impact on prominent functionality. I also tested the publishing of metadata (relays, contact, ..) and fixed all issues there. There is probably still more opportunity for cleaning up/reducing code but for now I want to keep the refactoring amount low as this is just a pre-condition.

Relates to #613

---

✅ Checklist

Please ensure the following tasks are completed before requesting a review:

• My code adheres to the coding guidelines of this project.
• I have run cargo fmt.
• I have run cargo clippy.
• I have added or updated tests (if applicable).
• All CI/CD steps were successful.
• I have updated the documentation (if applicable).
• I have checked that there are no console errors or warnings.
• I have verified that the application builds without errors.
• I've described the changes made to the API. (modification, addition, deletion).

---

🚀 Changes Made

• New Features:
  • Only maintain a single relay pool instead of one per local identity

---

📋 Review Guidelines

Please focus on the following while reviewing:

• Does the code follow the repository's contribution guidelines?
• Are there any potential bugs or performance issues?
• Are there any typos or grammatical errors in the code or comments?

---

PR Type

Enhancement, Refactoring

---

Description

• Replace multi-client architecture with single shared multi-identity Nostr client

• Add sender_node_id parameter to transport API methods for explicit identity routing

• Implement determine_recipient() function to route events to correct identity

• Enable NIP-17 support for multi-identity clients via manual gift wrap construction

• Simplify NostrConsumer to use single client with unified subscription management

---

Diagram Walkthrough

flowchart LR
  A["Multiple NostrClient<br/>instances"] -->|"Refactor"| B["Single NostrClient<br/>with HashMap of signers"]
  B -->|"Route events"| C["determine_recipient()<br/>function"]
  C -->|"Decrypt/match"| D["Correct identity<br/>signer"]
  D -->|"Process"| E["Event handlers"]
  F["TransportClientApi<br/>methods"] -->|"Add sender_node_id"| G["Explicit identity<br/>routing"]

Loading

File Walkthrough

	Relevant files
Api changes	1 files transport_client.rs Add sender_node_id parameter to transport API methods        +17/-8
Refactoring	11 files block_transport.rs Update to pass sender_node_id to transport methods              +125/-145 contact_transport.rs Simplify transport access with single client                          +8/-10    bill_chain_event_handler.rs Remove unused node_id parameter from handler                          +2/-2      company_chain_event_handler.rs Remove unused node_id parameter from handler                          +3/-3      identity_chain_event_handler.rs Remove unused node_id parameter from handler                          +3/-3      restore.rs Accept node_id parameter instead of deriving from transport +3/-11    nostr_transport.rs Simplify to use single shared client instead of HashMap    +54/-101 notification_transport.rs Update to pass sender_node_id to transport methods              +22/-43  transport.rs Update unwrap_direct_message to accept event reference      +4/-4      transport_service.rs Update to pass sender_node_id to transport methods              +50/-99  context.rs Update context initialization for single client                    +3/-3
Enhancement	3 files direct_message_event_processor.rs Implement multi-identity event routing with determine_recipient +32/-25  lib.rs Create single multi-identity client instead of multiple clients +36/-43  nostr.rs Refactor NostrClient to support multiple identities with shared relay pool +516/-216
Tests	1 files test_utils.rs Update mock transport to support new API with sender_node_id +8/-7

---

[codecov]

Codecov Report

❌ Patch coverage is 41.29693% with 172 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/bcr-ebill-transport/src/nostr.rs	39.00%	147 Missing ⚠️
crates/bcr-ebill-transport/src/nostr_transport.rs	58.13%	18 Missing ⚠️
crates/bcr-ebill-transport/src/transport.rs	0.00%	4 Missing ⚠️
crates/bcr-ebill-transport/src/lib.rs	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR refactors the Nostr transport layer from maintaining multiple NostrClient instances (one per local identity/company) to a single multi-identity NostrClient with a shared relay pool. This architectural change reduces resource usage and simplifies the codebase while maintaining support for multiple identities.

Key changes include:

• Single NostrClient with internal HashMap<NodeId, Arc<Keys>> for managing multiple identity signers
• TransportClientApi methods now require explicit sender_node_id parameter to specify which identity to use
• NostrConsumer now manages a single client instead of multiple clients
• Event routing logic via new determine_recipient() function that tries decrypting with each identity for private messages

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
crates/bcr-ebill-wasm/src/context.rs	Updates initialization to use single nostr_client instead of nostr_clients
crates/bcr-ebill-transport/src/nostr.rs	Core refactoring - NostrClient now manages multiple identities with shared relay pool; adds determine_recipient() for routing events
crates/bcr-ebill-transport/src/nostr_transport.rs	Simplified to use single client; get_node_transport() now returns the shared client; removed multi-client management logic
crates/bcr-ebill-transport/src/lib.rs	Updated factory functions to create single multi-identity client instead of multiple clients
crates/bcr-ebill-transport/src/transport_service.rs	Updated method signatures to accept sender_node_id parameter; extensive test mock updates
crates/bcr-ebill-transport/src/test_utils.rs	Mock trait updated with new TransportClientApi signatures including sender_node_id
crates/bcr-ebill-transport/src/notification_transport.rs	Updated to use single client with sender_node_id parameter
crates/bcr-ebill-transport/src/contact_transport.rs	Simplified transport retrieval to use shared client
crates/bcr-ebill-transport/src/block_transport.rs	Updated to pass sender_node_id when sending chain events
crates/bcr-ebill-transport/src/handler/restore.rs	Constructor updated to accept node_id parameter
crates/bcr-ebill-transport/src/handler/identity_chain_event_handler.rs	Handler signature updated; node_id parameter now marked as unused for public events
crates/bcr-ebill-transport/src/handler/company_chain_event_handler.rs	Handler signature updated; node_id parameter now marked as unused for public events
crates/bcr-ebill-transport/src/handler/bill_chain_event_handler.rs	Handler signature updated; node_id parameter now marked as unused for public events
crates/bcr-ebill-transport/src/handler/direct_message_event_processor.rs	Updated to handle multi-identity routing using determine_recipient()
crates/bcr-ebill-api/src/service/transport_service/transport_client.rs	Trait definition updated with new method signatures and add_identity() method

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Concurrency state risk Description: Use of Mutex locking with unwrap_or_else(|e| e.into_inner()) across get_signer/has_local_signer/get_all_node_ids may mask poisoned locks and continue with potentially inconsistent shared state, risking undefined behavior under concurrency. nostr.rs [119-141] Referred Code self.signers .lock() .unwrap_or_else(|e| e.into_inner()) .get(node_id) .cloned() .ok_or_else(|| Error::Message(format!("No signer found for node_id: {}", node_id))) } /// Add a new identity to this client pub fn add_identity(&self, node_id: NodeId, keys: BcrKeys) -> Result<()> { self.signers .lock() .unwrap_or_else(|e| e.into_inner()) .insert(node_id, Arc::new(keys.get_nostr_keys())); Ok(()) } /// Check if this client has a local signer for the given node_id pub fn has_local_signer(&self, node_id: &NodeId) -> bool { self.signers .lock() ... (clipped 2 lines)
	Identity correlation risk Description: Subscribing to events for all managed public keys in a single filter may expose all local identities’ activity patterns on shared relays, potentially enabling cross-identity correlation if identities were intended to be unlinkable. nostr.rs [477-521] Referred Code if self .connected .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_ok() { self.client.connect().await; // Publish relay list for ALL identities let node_ids = self.get_all_node_ids(); let relay_urls: Vec<RelayUrl> = self .relays .iter() .filter_map(|r| RelayUrl::parse(r.as_str()).ok()) .collect(); for node_id in node_ids { if let Err(e) = self.publish_relay_list(&node_id, relay_urls.clone()).await { error!( "Failed to publish relay list for identity {}: {}", node_id, e ); ... (clipped 24 lines)
	Signature misuse risk Description: Manual signing flow for public chain events introduces risk if signer selection is incorrect; absence of explicit domain separation or context binding in signatures could enable misuse across identities if event builders are reused. nostr.rs [351-369] Referred Code let signing_keys = self.get_signer(sender_node_id)?; let event_builder = create_public_chain_event( id, event, block_time, blockchain, keys, previous_event, root_event, )?; // Build unsigned event and sign it with the explicit keys let unsigned = event_builder.build(signing_keys.public_key()); let send_event = unsigned.sign(&*signing_keys).await.map_err(|e| { error!("Failed to sign Nostr event: {e}"); Error::Crypto("Failed to sign Nostr event".to_string()) })?;
Ticket Compliance
🟡	🎫 #613 🟢 Enable users to have multiple relays with appropriate relay list publication and handling. Ensure notifications work correctly with multi-relay and multi-identity scenarios, including a way to flag/handle notification relay(s). 🔴 Ensure file uploads do not rely only on the first relay in the list. Implement or account for replication between relays. ⚪ Support multiple relays per user and across the system, not just a single default relay.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Sparse Logging: Several newly introduced critical operations (signer selection, identity routing, event sending) log only generic errors or debug messages without consistently including the acting sender identity or full action context, which may hinder reconstructing events. Referred Code async fn send_nip17_message( &self, sender_node_id: &NodeId, recipient: &BillParticipant, event: EventEnvelope, ) -> Result<()> { // Get the Keys for the specified sender identity let sender_keys = self.get_signer(sender_node_id)?; let receiver_pubkey = recipient.node_id().npub(); let message = base58::encode(&borsh::to_vec(&event)?); let event = EventBuilder::private_msg(&*sender_keys, receiver_pubkey, message, []) .await .map_err(|e| { error!("Failed to create NIP-17 event: {e}"); Error::Message(format!("Failed to create NIP-17 event: {e}")) })?; let relays = recipient.nostr_relays(); ... (clipped 83 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing Fallback: Refactored methods now assume a single transport is always available and remove previous None handling branches; when resolution fails or transport errors occur some paths only warn without alternative handling, which may cause silent drops. Referred Code sender: &NodeId, events: &HashMap<NodeId, Event<BillChainEventPayload>>, ) -> Result<()> { let node = self.get_node_transport(sender); for (node_id, event_to_process) in events.iter() { if let Some(identity) = self.resolve_identity(node_id).await { if let Err(e) = node .send_private_event(sender, &identity, event_to_process.clone().try_into()?) .await { error!("Failed to send block notification, will add it to retry queue: {e}"); self.queue_retry_message( sender, node_id, base58::encode( &borsh::to_vec(event_to_process) .map_err(|e| Error::Message(e.to_string()))?, ), ) .await?; } ... (clipped 25 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Lock Poison Handling: The code uses unwrap_or_else(|e| e.into_inner()) on poisoned Mutex locks for signers which skips validation and may operate on inconsistent state without safeguards. Referred Code self.signers .lock() .unwrap_or_else(|e| e.into_inner()) .get(node_id) .cloned() .ok_or_else(|| Error::Message(format!("No signer found for node_id: {}", node_id))) } /// Add a new identity to this client pub fn add_identity(&self, node_id: NodeId, keys: BcrKeys) -> Result<()> { self.signers .lock() .unwrap_or_else(|e| e.into_inner()) .insert(node_id, Arc::new(keys.get_nostr_keys())); Ok(()) } /// Check if this client has a local signer for the given node_id pub fn has_local_signer(&self, node_id: &NodeId) -> bool { self.signers .lock() ... (clipped 12 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use correct event builder for gift-wrapped messages To correctly implement NIP-17/59, replace EventBuilder::private_msg with EventBuilder::gift_wrap to create a gift-wrapped event instead of a NIP-04 direct message. crates/bcr-ebill-transport/src/nostr.rs [269-304] async fn send_nip17_message( &self, sender_node_id: &NodeId, recipient: &BillParticipant, event: EventEnvelope, ) -> Result<()> { // Get the Keys for the specified sender identity let sender_keys = self.get_signer(sender_node_id)?; let receiver_pubkey = recipient.node_id().npub(); let message = base58::encode(&borsh::to_vec(&event)?); - let event = EventBuilder::private_msg(&*sender_keys, receiver_pubkey, message, []) + let event = EventBuilder::gift_wrap(&*sender_keys, receiver_pubkey, message, None) .await .map_err(|e| { - error!("Failed to create NIP-17 event: {e}"); - Error::Message(format!("Failed to create NIP-17 event: {e}")) + error!("Failed to create gift wrap event: {e}"); + Error::Message(format!("Failed to create gift wrap event: {e}")) })?; let relays = recipient.nostr_relays(); if !relays.is_empty() { if let Err(e) = self.client().await?.send_event_to(&relays, &event).await { - error!("Error sending NIP-17 message to specific relays: {e}"); + error!("Error sending gift wrap message to specific relays: {e}"); return Err(Error::Network(format!( - "Failed to send NIP-17 message: {e}" + "Failed to send gift wrap message: {e}" ))); }; } else if let Err(e) = self.client().await?.send_event(&event).await { - error!("Error sending NIP-17 message: {e}"); + error!("Error sending gift wrap message: {e}"); return Err(Error::Network(format!( - "Failed to send NIP-17 message: {e}" + "Failed to send gift wrap message: {e}" ))); } Ok(()) } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that EventBuilder::private_msg creates a NIP-04 event, while the context (comments and receiver logic) implies NIP-59 (Gift Wrap) is intended, making this a functional bug.	Medium
High-level	Refactor Nostr client for multi-identity The current multi-identity implementation is a wrapper around a single-signer nostr-sdk client, requiring complex manual signing workarounds. Consider contributing multi-signer support to nostr-sdk or redesigning the client to simplify the code. Examples: crates/bcr-ebill-transport/src/nostr.rs [432-451] async fn publish_metadata(&self, node_id: &NodeId, data: &Metadata) -> Result<()> { // Get the signer for this identity let signer = self.get_signer(node_id)?; // Build and sign the metadata event with the specific identity let event = EventBuilder::metadata(data) .build(signer.public_key()) .sign(&*signer) .await .map_err(|e| { ... (clipped 10 lines) crates/bcr-ebill-transport/src/nostr.rs [339-379] async fn send_public_chain_event( &self, sender_node_id: &NodeId, id: &str, blockchain: BlockchainType, block_time: Timestamp, keys: BcrKeys, event: EventEnvelope, previous_event: Option<Event>, root_event: Option<Event>, ... (clipped 31 lines) Solution Walkthrough: Before: struct NostrClient { client: nostr_sdk::Client, // Initialized with one signer signers: HashMap<NodeId, Arc<Keys>>, // ... } impl TransportClientApi for NostrClient { async fn publish_metadata(&self, node_id: &NodeId, data: &Metadata) -> Result<()> { // Get the specific signer for the identity let signer = self.get_signer(node_id)?; // Manually build and sign the event let event = EventBuilder::metadata(data) .build(signer.public_key()) .sign(&*signer).await?; // Send the pre-signed event self.client().await?.send_event(&event).await?; Ok(()) } } After: // Hypothetical future implementation if nostr-sdk supported multi-signer struct NostrClient { client: nostr_sdk::Client, // Natively supports multiple signers // ... } impl TransportClientApi for NostrClient { async fn publish_metadata(&self, node_id: &NodeId, data: &Metadata) -> Result<()> { // The nostr-sdk client would handle selecting the correct signer // based on a provided identity parameter. self.client() .await? .set_metadata_as(node_id.public_key(), data) // Fictional method .await?; Ok(()) } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a core architectural trade-off in the PR—implementing a multi-identity wrapper over a single-identity SDK client—and accurately points out the resulting complexity of manual event signing, which impacts maintainability.	Medium
Learned best practice	Handle mutex lock failures safely Avoid using unwrap_or_else on a poisoned mutex; handle lock acquisition failure explicitly and return a meaningful error. crates/bcr-ebill-transport/src/nostr.rs [118-125] pub fn get_signer(&self, node_id: &NodeId) -> Result<Arc<Keys>> { - self.signers - .lock() - .unwrap_or_else(|e| e.into_inner()) + let guard = self.signers.lock().map_err(|_| { + Error::Message("failed to acquire signer lock".to_string()) + })?; + guard .get(node_id) .cloned() .ok_or_else(|| Error::Message(format!("No signer found for node_id: {}", node_id))) } Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Validate operation outcomes and avoid panics; handle mutex poisoning and concurrency safely (Pattern 2).	Low
Update

[zupzup]

Very, very nice! LGTM 👍