fix(nip46): fix bunker decrypt/encrypt response parsing and increase timeout

Two issues fixed: 1. BunkerResponseDeserializer produces generic BunkerResponse for decrypt/encrypt results (plain strings that aren't pubkeys or JSON). The response parsers only matched typed subtypes (BunkerResponseDecrypt/Encrypt), causing all decrypt and encrypt operations through NIP-46 bunker to fail with ReceivedButCouldNotPerform. Fix: parsers now fall back to extracting response.result directly. 2. RemoteSignerManager timeout was 30s, shorter than Amber's 60s approval window. Fix: increased to 65s with safe retry (republishes same request, preserving UUID so bunker responses always match). 3. Desktop ChatPane showed raw ciphertext on decrypt failure instead of an error message. Fix: shows "Could not decrypt the message" in italic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-05-04 10:24:05 +03:00
parent 42d2a50436
commit bc2267d79a
8 changed files with 329 additions and 34 deletions
@@ -37,7 +37,9 @@ class Nip04DecryptResponse {
                }

                else -> {
-                    SignerResult.RequestAddressed.ReceivedButCouldNotPerform()
+                    response.result?.let {
+                        SignerResult.RequestAddressed.Successful(DecryptionResult(it))
+                    } ?: SignerResult.RequestAddressed.ReceivedButCouldNotPerform("No result in response")
                }
            }
    }
@@ -37,7 +37,9 @@ class Nip04EncryptResponse {
                }

                else -> {
-                    SignerResult.RequestAddressed.ReceivedButCouldNotPerform()
+                    response.result?.let {
+                        SignerResult.RequestAddressed.Successful(EncryptionResult(it))
+                    } ?: SignerResult.RequestAddressed.ReceivedButCouldNotPerform("No result in response")
                }
            }
    }
@@ -37,7 +37,9 @@ class Nip44DecryptResponse {
                }

                else -> {
-                    SignerResult.RequestAddressed.ReceivedButCouldNotPerform()
+                    response.result?.let {
+                        SignerResult.RequestAddressed.Successful(DecryptionResult(it))
+                    } ?: SignerResult.RequestAddressed.ReceivedButCouldNotPerform("No result in response")
                }
            }
    }
@@ -37,7 +37,9 @@ class Nip44EncryptResponse {
                }

                else -> {
-                    SignerResult.RequestAddressed.ReceivedButCouldNotPerform()
+                    response.result?.let {
+                        SignerResult.RequestAddressed.Successful(EncryptionResult(it))
+                    } ?: SignerResult.RequestAddressed.ReceivedButCouldNotPerform("No result in response")
                }
            }
    }
@@ -29,15 +29,17 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponse
 import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent
 import com.vitorpamplona.quartz.utils.cache.LargeCache
 import com.vitorpamplona.quartz.utils.tryAndWait
+import kotlinx.coroutines.delay
 import kotlin.coroutines.Continuation
 import kotlin.coroutines.resume

 class RemoteSignerManager(
-    val timeout: Long = 30000,
+    val timeout: Long = 65_000,
    val client: INostrClient,
    val signer: NostrSignerInternal,
    val remoteKey: String,
    val relayList: Set<NormalizedRelayUrl>,
+    val maxRetries: Int = 1,
 ) {
    private val awaitingRequests = LargeCache<String, Continuation<BunkerResponse>>()

@@ -48,15 +50,14 @@ class RemoteSignerManager(
    }

    /**
-     * Launches the signer, waits and parses the result
+     * Launches the signer, waits and parses the result.
+     *
+     * Builds the request once and republishes the same event on retry to ensure
+     * the bunker's response (keyed by request ID) can always be matched.
     *
     * @param bunkerRequestBuilder The BunkerRequest to be sent.
     * @param parser A function that parses the BunkerResponse into a [SignerResult.RequestAddressed<T>].
     * @return The result after parsing the BunkerResponse using the provided parser.
-     *
-     * This function uses the [tryAndWait] utility to implement a timeout on the Bunker Request approval.
-     * It assigns a unique ID to the request and keeps a continuation to resume once the result is received.
-     * If the timeout occurs or the continuation is cancelled, the request ID is cleaned up from [awaitingRequests].
     */
    suspend fun <T : IResult> launchWaitAndParse(
        bunkerRequestBuilder: () -> BunkerRequest,
@@ -69,20 +70,34 @@ class RemoteSignerManager(
                remoteKey = remoteKey,
                signer = signer,
            )
-        val result =
-            tryAndWait(timeout) { continuation ->
-                continuation.invokeOnCancellation {
-                    awaitingRequests.remove(request.id)
+
+        var attempt = 0
+        while (true) {
+            val result =
+                tryAndWait(timeout) { continuation ->
+                    continuation.invokeOnCancellation {
+                        awaitingRequests.remove(request.id)
+                    }
+
+                    awaitingRequests.put(request.id, continuation)
+
+                    client.publish(event, relayList = relayList)
                }

-                awaitingRequests.put(request.id, continuation)
+            when {
+                result != null -> {
+                    return parser(result)
+                }

-                client.publish(event, relayList = relayList)
+                attempt >= maxRetries -> {
+                    return SignerResult.RequestAddressed.TimedOut()
+                }
+
+                else -> {
+                    attempt++
+                    delay(2_000L)
+                }
            }
-
-        return when (result) {
-            null -> SignerResult.RequestAddressed.TimedOut()
-            else -> parser(result)
        }
    }
 }
@@ -0,0 +1,225 @@
+/*
+ * Copyright (c) 2025 Vitor Pamplona
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
+ * Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+package com.vitorpamplona.quartz.nip46RemoteSigner.signer
+
+import com.vitorpamplona.quartz.nip01Core.core.Event
+import com.vitorpamplona.quartz.nip01Core.core.HexKey
+import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
+import com.vitorpamplona.quartz.nip01Core.relay.client.EmptyNostrClient
+import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient
+import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.RelayConnectionListener
+import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.SubscriptionListener
+import com.vitorpamplona.quartz.nip01Core.relay.client.single.IRelayClient
+import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
+import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
+import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal
+import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestPing
+import com.vitorpamplona.quartz.utils.Hex
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.test.runTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertIs
+
+class RemoteSignerManagerRetryTest {
+    private val signer = NostrSignerInternal(KeyPair())
+    private val remoteKeyPair = KeyPair()
+    private val remoteKey = Hex.encode(remoteKeyPair.pubKey)
+    private val relay = NormalizedRelayUrl("wss://relay.test")
+
+    @Test
+    fun timeoutReturnsTimedOutAfterMaxRetries() =
+        runTest {
+            val manager =
+                RemoteSignerManager(
+                    timeout = 100, // Very short for test
+                    client = EmptyNostrClient(),
+                    signer = signer,
+                    remoteKey = remoteKey,
+                    relayList = setOf(relay),
+                    maxRetries = 0, // No retries
+                )
+
+            val result =
+                manager.launchWaitAndParse(
+                    bunkerRequestBuilder = { BunkerRequestPing() },
+                    parser = PingResponse::parse,
+                )
+
+            assertIs<SignerResult.RequestAddressed.TimedOut<PingResult>>(result)
+        }
+
+    @Test
+    fun timeoutWithRetryStillTimesOut() =
+        runTest {
+            val manager =
+                RemoteSignerManager(
+                    timeout = 100,
+                    client = EmptyNostrClient(),
+                    signer = signer,
+                    remoteKey = remoteKey,
+                    relayList = setOf(relay),
+                    maxRetries = 1, // 1 retry = 2 total attempts
+                )
+
+            val result =
+                manager.launchWaitAndParse(
+                    bunkerRequestBuilder = { BunkerRequestPing() },
+                    parser = PingResponse::parse,
+                )
+
+            assertIs<SignerResult.RequestAddressed.TimedOut<PingResult>>(result)
+        }
+
+    @Test
+    fun publishCalledOnEachAttempt() =
+        runTest {
+            var publishCount = 0
+            val countingClient = CountingNostrClient { publishCount++ }
+
+            val manager =
+                RemoteSignerManager(
+                    timeout = 100,
+                    client = countingClient,
+                    signer = signer,
+                    remoteKey = remoteKey,
+                    relayList = setOf(relay),
+                    maxRetries = 2, // 2 retries = 3 total attempts
+                )
+
+            manager.launchWaitAndParse(
+                bunkerRequestBuilder = { BunkerRequestPing() },
+                parser = PingResponse::parse,
+            )
+
+            assertEquals(3, publishCount)
+        }
+
+    @Test
+    fun requestBuilderCalledOnlyOnce() =
+        runTest {
+            var builderCount = 0
+
+            val manager =
+                RemoteSignerManager(
+                    timeout = 100,
+                    client = EmptyNostrClient(),
+                    signer = signer,
+                    remoteKey = remoteKey,
+                    relayList = setOf(relay),
+                    maxRetries = 2,
+                )
+
+            manager.launchWaitAndParse(
+                bunkerRequestBuilder = {
+                    builderCount++
+                    BunkerRequestPing()
+                },
+                parser = PingResponse::parse,
+            )
+
+            assertEquals(1, builderCount)
+        }
+
+    @Test
+    fun defaultTimeoutIs65Seconds() {
+        val manager =
+            RemoteSignerManager(
+                client = EmptyNostrClient(),
+                signer = signer,
+                remoteKey = remoteKey,
+                relayList = setOf(relay),
+            )
+
+        assertEquals(65_000L, manager.timeout)
+    }
+
+    @Test
+    fun defaultMaxRetriesIs1() {
+        val manager =
+            RemoteSignerManager(
+                client = EmptyNostrClient(),
+                signer = signer,
+                remoteKey = remoteKey,
+                relayList = setOf(relay),
+            )
+
+        assertEquals(1, manager.maxRetries)
+    }
+}
+
+private class CountingNostrClient(
+    private val onPublish: () -> Unit,
+) : INostrClient {
+    override fun connectedRelaysFlow(): StateFlow<Set<NormalizedRelayUrl>> = MutableStateFlow(emptySet())
+
+    override fun availableRelaysFlow(): StateFlow<Set<NormalizedRelayUrl>> = MutableStateFlow(emptySet())
+
+    override fun connect() {}
+
+    override fun disconnect() {}
+
+    override fun reconnect(
+        onlyIfChanged: Boolean,
+        ignoreRetryDelays: Boolean,
+    ) {}
+
+    override fun isActive(): Boolean = false
+
+    override fun syncFilters(relay: IRelayClient) {}
+
+    override fun subscribe(
+        subId: String,
+        filters: Map<NormalizedRelayUrl, List<Filter>>,
+        listener: SubscriptionListener?,
+    ) {}
+
+    override fun count(
+        subId: String,
+        filters: Map<NormalizedRelayUrl, List<Filter>>,
+    ) {}
+
+    override fun unsubscribe(subId: String) {}
+
+    override fun publish(
+        event: Event,
+        relayList: Set<NormalizedRelayUrl>,
+    ) {
+        onPublish()
+    }
+
+    override fun addConnectionListener(listener: RelayConnectionListener) {}
+
+    override fun removeConnectionListener(listener: RelayConnectionListener) {}
+
+    override fun getReqFiltersOrNull(subId: String): Map<NormalizedRelayUrl, List<Filter>>? = null
+
+    override fun getCountFiltersOrNull(subId: String): Map<NormalizedRelayUrl, List<Filter>>? = null
+
+    override fun activeRequests(url: NormalizedRelayUrl): Map<String, List<Filter>> = emptyMap()
+
+    override fun activeCounts(url: NormalizedRelayUrl): Map<String, List<Filter>> = emptyMap()
+
+    override fun activeOutboxCache(url: NormalizedRelayUrl): Set<HexKey> = emptySet()
+
+    override fun close() {}
+}
@@ -204,12 +204,21 @@ class ResponseParserTest {
    }

    @Test
-    fun nip04EncryptParseUnexpected() {
-        val response = BunkerResponsePong("req-4")
+    fun nip04EncryptParseUnexpectedNullResult() {
+        val response = BunkerResponse("req-4", null, null)
        val result = Nip04EncryptResponse.parse(response)
        assertIs<SignerResult.RequestAddressed.ReceivedButCouldNotPerform<EncryptionResult>>(result)
    }

+    @Test
+    fun nip04EncryptParseGenericBunkerResponse() {
+        // Production case: deserializer produces generic BunkerResponse with ciphertext
+        val response = BunkerResponse("req-4", "encrypted-ciphertext-data", null)
+        val result = Nip04EncryptResponse.parse(response)
+        assertIs<SignerResult.RequestAddressed.Successful<EncryptionResult>>(result)
+        assertEquals("encrypted-ciphertext-data", result.result.ciphertext)
+    }
+
    // --- Nip04DecryptResponse ---

    @Test
@@ -228,12 +237,21 @@ class ResponseParserTest {
    }

    @Test
-    fun nip04DecryptParseUnexpected() {
-        val response = BunkerResponsePong("req-5")
+    fun nip04DecryptParseUnexpectedNullResult() {
+        val response = BunkerResponse("req-5", null, null)
        val result = Nip04DecryptResponse.parse(response)
        assertIs<SignerResult.RequestAddressed.ReceivedButCouldNotPerform<DecryptionResult>>(result)
    }

+    @Test
+    fun nip04DecryptParseGenericBunkerResponse() {
+        // Production case: deserializer produces generic BunkerResponse with plaintext
+        val response = BunkerResponse("req-5", "Hello, this is the decrypted message!", null)
+        val result = Nip04DecryptResponse.parse(response)
+        assertIs<SignerResult.RequestAddressed.Successful<DecryptionResult>>(result)
+        assertEquals("Hello, this is the decrypted message!", result.result.plaintext)
+    }
+
    // --- Nip44EncryptResponse ---

    @Test
@@ -252,12 +270,21 @@ class ResponseParserTest {
    }

    @Test
-    fun nip44EncryptParseUnexpected() {
-        val response = BunkerResponseAck("req-6")
+    fun nip44EncryptParseUnexpectedNullResult() {
+        val response = BunkerResponse("req-6", null, null)
        val result = Nip44EncryptResponse.parse(response)
        assertIs<SignerResult.RequestAddressed.ReceivedButCouldNotPerform<EncryptionResult>>(result)
    }

+    @Test
+    fun nip44EncryptParseGenericBunkerResponse() {
+        // Production case: deserializer produces generic BunkerResponse with ciphertext
+        val response = BunkerResponse("req-6", "nip44-encrypted-payload", null)
+        val result = Nip44EncryptResponse.parse(response)
+        assertIs<SignerResult.RequestAddressed.Successful<EncryptionResult>>(result)
+        assertEquals("nip44-encrypted-payload", result.result.ciphertext)
+    }
+
    // --- Nip44DecryptResponse ---

    @Test
@@ -276,9 +303,18 @@ class ResponseParserTest {
    }

    @Test
-    fun nip44DecryptParseUnexpected() {
-        val response = BunkerResponseAck("req-7")
+    fun nip44DecryptParseUnexpectedNullResult() {
+        val response = BunkerResponse("req-7", null, null)
        val result = Nip44DecryptResponse.parse(response)
        assertIs<SignerResult.RequestAddressed.ReceivedButCouldNotPerform<DecryptionResult>>(result)
    }
+
+    @Test
+    fun nip44DecryptParseGenericBunkerResponse() {
+        // Production case: deserializer produces generic BunkerResponse with plaintext
+        val response = BunkerResponse("req-7", "Decrypted NIP-44 content here", null)
+        val result = Nip44DecryptResponse.parse(response)
+        assertIs<SignerResult.RequestAddressed.Successful<DecryptionResult>>(result)
+        assertEquals("Decrypted NIP-44 content here", result.result.plaintext)
+    }
 }