From b2755d83433c50cbb682186cf999df12ef094bc0 Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 3 Sep 2026 18:24:31 -0500 Subject: [PATCH] refactor(signer): Signer trait now returns SigningError instead of AppError Completes the approved Step 3 API (the enum + error types landed in e6e4922; this closes the trait half): - Signer trait: get_public_key, sign_event, pubkey_for (default), disconnect, revoke all return Result<_, SigningError> - Signing enum wrapper (pubkey/sign) returns SigningError; local sign failures map to InvalidSignature, external identity/verify failures to IdentityMismatch/InvalidSignature - EmbeddedSigner::resolve_keys + respond_to_approval speak SigningError; AppError from vault/profile ops converts via SigningError::from_app - Nip46ClientSigner: inherent methods (connect/disconnect/revoke/ parse_connect_uri/permissions/nip44) keep AppError; the trait impl wraps them with from_app - No mode branching introduced; IPC boundary conversion (From for AppError) already exists Verification: cargo test 196 passed, clippy clean, fmt clean --- src/signer/embedded.rs | 52 +++++++++++++++++++++++--------------- src/signer/mod.rs | 43 ++++++++++++++++++------------- src/signer/nip46_client.rs | 38 ++++++++++++++++++---------- 3 files changed, 81 insertions(+), 52 deletions(-) diff --git a/src/signer/embedded.rs b/src/signer/embedded.rs index 2e40d47..d947cc6 100644 --- a/src/signer/embedded.rs +++ b/src/signer/embedded.rs @@ -8,8 +8,8 @@ use nostr_sdk::prelude::*; use tokio::sync::{oneshot, Mutex}; use crate::app::App; -use crate::errors::AppError; use crate::profiles; +use crate::signer::backend::SigningError; use crate::signer::types::{ApprovalDetails, ApprovalResult, SignerType}; use crate::signer::Signer; @@ -55,20 +55,22 @@ impl EmbeddedSigner { } /// Resolve the active profile's Keys, checking vault lock state. - async fn resolve_keys(&self) -> Result { + async fn resolve_keys(&self) -> Result { let app = self.app.lock().await; let npub_guard = self.active_npub.lock().await; - let npub = npub_guard.as_ref().ok_or_else(|| { - AppError::config("No active profile selected. Choose a profile first.") - })?; + let npub = npub_guard.as_ref().ok_or(SigningError::NoActiveProfile)?; if app.is_locked() { - return Err(AppError::vault_locked()); + return Err(SigningError::InternalKeyUnavailable { + detail: "vault locked".to_string(), + }); } let vault_key = app.vault_key().copied(); - let secret_hex = profiles::resolve_secret_key(&app.vault, npub, vault_key.as_ref())?; - let secret_key = profiles::parse_secret_key(&secret_hex)?; + let secret_hex = profiles::resolve_secret_key(&app.vault, npub, vault_key.as_ref()) + .map_err(|e| SigningError::from_app(&e))?; + let secret_key = + profiles::parse_secret_key(&secret_hex).map_err(|e| SigningError::from_app(&e))?; Ok(Keys::new(secret_key)) } @@ -120,10 +122,16 @@ impl EmbeddedSigner { } /// Approve or reject a pending request by index. - pub async fn respond_to_approval(&self, index: usize, approved: bool) -> Result<(), AppError> { + pub async fn respond_to_approval( + &self, + index: usize, + approved: bool, + ) -> Result<(), SigningError> { let mut pending = self.pending.lock().await; if index >= pending.len() { - return Err(AppError::config("No pending request at that index")); + return Err(SigningError::Internal { + detail: "No pending request at that index".to_string(), + }); } let entry = pending.remove(index); let _ = entry.sender.send(if approved { @@ -175,12 +183,12 @@ impl EmbeddedSigner { #[async_trait] impl Signer for EmbeddedSigner { - async fn get_public_key(&self) -> Result { + async fn get_public_key(&self) -> Result { let keys = self.resolve_keys().await?; Ok(keys.public_key()) } - async fn sign_event(&self, event: UnsignedEvent) -> Result { + async fn sign_event(&self, event: UnsignedEvent) -> Result { let keys = self.resolve_keys().await?; // Request approval for sensitive operations @@ -188,11 +196,13 @@ impl Signer for EmbeddedSigner { let approval = self.request_approval(details).await; match approval { - ApprovalResult::Approved => keys - .sign_event(event) - .map_err(|e| AppError::internal(format!("Failed to sign event: {e}"))), - ApprovalResult::Rejected => Err(AppError::config("Signing request rejected by user")), - ApprovalResult::Timeout => Err(AppError::config("Signing request timed out")), + ApprovalResult::Approved => { + keys.sign_event(event).map_err(|e| SigningError::Internal { + detail: format!("Failed to sign event: {e}"), + }) + } + ApprovalResult::Rejected => Err(SigningError::Rejected), + ApprovalResult::Timeout => Err(SigningError::Timeout), } } @@ -210,14 +220,14 @@ impl Signer for EmbeddedSigner { self.await_approval(details).await } - async fn disconnect(&self) -> Result<(), AppError> { + async fn disconnect(&self) -> Result<(), SigningError> { let mut npub_guard = self.active_npub.lock().await; *npub_guard = None; self.pending.lock().await.clear(); Ok(()) } - async fn revoke(&self) -> Result<(), AppError> { + async fn revoke(&self) -> Result<(), SigningError> { let npub = { let mut npub_guard = self.active_npub.lock().await; npub_guard.take() @@ -225,7 +235,9 @@ impl Signer for EmbeddedSigner { if let Some(npub) = npub { let mut app = self.app.lock().await; let _ = profiles::delete_profile(&mut app.vault, &npub); - app.save_vault()?; + app.save_vault().map_err(|e| SigningError::Internal { + detail: format!("Could not persist vault: {e}"), + })?; } self.pending.lock().await.clear(); Ok(()) diff --git a/src/signer/mod.rs b/src/signer/mod.rs index f44df5d..47fc612 100644 --- a/src/signer/mod.rs +++ b/src/signer/mod.rs @@ -12,34 +12,39 @@ use async_trait::async_trait; use nostr_sdk::prelude::*; use std::sync::Arc; -use crate::errors::AppError; use crate::signer::types::{ApprovalDetails, ApprovalResult, SignerType}; /// Common interface for all signer implementations. +/// +/// Every method that can fail a *signing* operation returns +/// [`Result<_, SigningError>`], so the whole signing subsystem speaks one +/// closed error type. The IPC layer converts [`SigningError`] to the app-wide +/// [`crate::errors::AppError`] at the boundary (via [`From`]) — no string +/// matching, no mode branching. #[async_trait] pub trait Signer: Send + Sync { /// Get the public key of the active signing identity. - async fn get_public_key(&self) -> Result; + async fn get_public_key(&self) -> Result; /// Resolve the public key this signer will sign user content with, /// enforcing that it matches the active profile's canonical identity. /// /// The default implementation compares `get_public_key()` against /// `profile_pubkey` using canonical hex, returning - /// [`AppError::external_signer_identity_mismatch`] on any difference. - /// External signers may override this to consult the remote signer's - /// identity. Callers must use the returned key as the event's `pubkey` - /// and must never sign user content when this errors. - async fn pubkey_for(&self, profile_pubkey: &PublicKey) -> Result { + /// [`SigningError::IdentityMismatch`] on any difference. External signers + /// may override this to consult the remote signer's identity. Callers must + /// use the returned key as the event's `pubkey` and must never sign user + /// content when this errors. + async fn pubkey_for(&self, profile_pubkey: &PublicKey) -> Result { let signer_pubkey = self.get_public_key().await?; if signer_pubkey.to_hex() != profile_pubkey.to_hex() { - return Err(AppError::external_signer_identity_mismatch()); + return Err(SigningError::IdentityMismatch); } Ok(signer_pubkey) } /// Sign an event with the active key. - async fn sign_event(&self, event: UnsignedEvent) -> Result; + async fn sign_event(&self, event: UnsignedEvent) -> Result; /// Get the type of this signer. fn get_signer_type(&self) -> SignerType; @@ -52,10 +57,10 @@ pub trait Signer: Send + Sync { async fn request_approval(&self, details: ApprovalDetails) -> ApprovalResult; /// Disconnect/stop the signer (for NIP-46, closes connection). - async fn disconnect(&self) -> Result<(), AppError>; + async fn disconnect(&self) -> Result<(), SigningError>; /// Revoke the signer authorization (for NIP-46, revokes the connection). - async fn revoke(&self) -> Result<(), AppError>; + async fn revoke(&self) -> Result<(), SigningError>; /// Get a human-readable status string for UI display. async fn status_string(&self) -> String; @@ -155,10 +160,10 @@ impl Signing { /// The public key user content will be signed with. /// /// For [`Signing::External`] this enforces identity validation and returns - /// the signer's key; it returns [`AppError::external_signer_identity_mismatch`] - /// when the signer does not control the active profile. Callers MUST use the + /// the signer's key; it returns [`SigningError::IdentityMismatch`] when the + /// signer does not control the active profile. Callers MUST use the /// returned key as the event's `pubkey`. - pub async fn pubkey(&self) -> Result { + pub async fn pubkey(&self) -> Result { match self { Signing::Local(keys) => Ok(keys.public_key()), Signing::External { @@ -174,18 +179,20 @@ impl Signing { /// For [`Signing::External`] the returned event is re-checked against the /// validated identity and verified as a well-formed signature before it is /// returned, so a misbehaving signer cannot substitute a different key. - pub async fn sign(&self, unsigned: UnsignedEvent) -> Result { + pub async fn sign(&self, unsigned: UnsignedEvent) -> Result { match self { - Signing::Local(keys) => keys.sign_event(unsigned).map_err(AppError::sign_failed), + Signing::Local(keys) => keys + .sign_event(unsigned) + .map_err(|_e| SigningError::InvalidSignature), Signing::External { signer, profile_pubkey, } => { let event = signer.sign_event(unsigned).await?; if event.pubkey != *profile_pubkey { - return Err(AppError::external_signer_identity_mismatch()); + return Err(SigningError::IdentityMismatch); } - event.verify().map_err(AppError::sign_failed)?; + event.verify().map_err(|_| SigningError::InvalidSignature)?; Ok(event) } } diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 9767a6b..8453520 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -18,6 +18,7 @@ use tokio::sync::{oneshot, Mutex}; use crate::app::App; use crate::errors::AppError; use crate::profiles; +use crate::signer::backend::SigningError; use crate::signer::permissions::Nip46Permissions; use crate::signer::types::{ ApprovalDetails, ApprovalResult, Nip46Connection, Nip46Status, PendingApproval, SignerType, @@ -826,22 +827,27 @@ impl Clone for Nip46ClientSigner { #[async_trait] impl Signer for Nip46ClientSigner { - async fn get_public_key(&self) -> Result { + async fn get_public_key(&self) -> Result { let inner = self.inner.lock().await; let connection = inner .connection .as_ref() - .ok_or_else(|| AppError::config("Not connected to a signer"))?; - PublicKey::from_hex(&connection.signer_pubkey) - .map_err(|_| AppError::config("Invalid signer public key")) + .ok_or(SigningError::NotConnected)?; + PublicKey::from_hex(&connection.signer_pubkey).map_err(|e| SigningError::Internal { + detail: format!("Invalid signer public key: {e}"), + }) } - async fn sign_event(&self, _event: UnsignedEvent) -> Result { - // For NIP-46 client, signing happens via the NIP-46 channel with user approval - // The actual flow uses request_approval + respond_to_approval - Err(AppError::config( - "NIP-46 signing uses async approval flow. Use request_approval.", - )) + async fn sign_event(&self, _event: UnsignedEvent) -> Result { + // For NIP-46 client, signing happens via the NIP-46 channel with user + // approval. The actual flow uses request_approval + + // respond_to_approval; this method exists to satisfy the object-safe + // trait and fails closed if a caller tries to bypass it. + Err(SigningError::Internal { + detail: + "NIP-46 signing uses the async approval flow; direct sign_event is not supported" + .to_string(), + }) } fn get_signer_type(&self) -> SignerType { @@ -857,12 +863,16 @@ impl Signer for Nip46ClientSigner { self.await_approval(details).await } - async fn disconnect(&self) -> Result<(), AppError> { - Nip46ClientSigner::disconnect(self).await + async fn disconnect(&self) -> Result<(), SigningError> { + Nip46ClientSigner::disconnect(self) + .await + .map_err(|e| SigningError::from_app(&e)) } - async fn revoke(&self) -> Result<(), AppError> { - Nip46ClientSigner::revoke(self).await + async fn revoke(&self) -> Result<(), SigningError> { + Nip46ClientSigner::revoke(self) + .await + .map_err(|e| SigningError::from_app(&e)) } async fn status_string(&self) -> String {