diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index 76e3d7bed..5af6d87eb 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -30,7 +30,12 @@ import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray import com.vitorpamplona.quartz.nip01Core.core.toHexKey import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import com.vitorpamplona.quartz.nip01Core.relay.client.NostrClient +import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.IRelayClientListener import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.req +import com.vitorpamplona.quartz.nip01Core.relay.client.single.IRelayClient +import com.vitorpamplona.quartz.nip01Core.relay.commands.toClient.Message +import com.vitorpamplona.quartz.nip01Core.relay.commands.toRelay.Command +import com.vitorpamplona.quartz.nip01Core.relay.commands.toRelay.EventCmd import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.relay.sockets.okhttp.BasicOkHttpWebSocket @@ -135,6 +140,9 @@ class AccountManager internal constructor( private val _forceLogoutReason = MutableStateFlow(null) val forceLogoutReason: StateFlow = _forceLogoutReason.asStateFlow() + private val _loginProgress = MutableStateFlow(null) + val loginProgress: StateFlow = _loginProgress.asStateFlow() + private var heartbeatJob: Job? = null // --- Dedicated NIP-46 client (isolated from general relay pool) --- @@ -174,18 +182,72 @@ class AccountManager internal constructor( } } + private fun updateRelayLoginStatus( + relay: NormalizedRelayUrl, + status: RelayLoginStatus, + ) { + val current = _loginProgress.value ?: return + val updated = current.relayStatuses + (relay to status) + _loginProgress.value = + when (current) { + is LoginProgress.ConnectingToRelays -> current.copy(relayStatuses = updated) + is LoginProgress.WaitingForSigner -> current.copy(relayStatuses = updated) + is LoginProgress.SendingAck -> current.copy(relayStatuses = updated) + } + } + + private fun createLoginRelayListener(): IRelayClientListener = + object : IRelayClientListener { + override fun onConnected( + relay: IRelayClient, + pingMillis: Int, + compressed: Boolean, + ) { + updateRelayLoginStatus(relay.url, RelayLoginStatus.CONNECTED) + } + + override fun onCannotConnect( + relay: IRelayClient, + errorMessage: String, + ) { + updateRelayLoginStatus(relay.url, RelayLoginStatus.FAILED) + } + + override fun onSent( + relay: IRelayClient, + cmdStr: String, + cmd: Command, + success: Boolean, + ) { + if (cmd is EventCmd) { + updateRelayLoginStatus( + relay.url, + if (success) RelayLoginStatus.EVENT_SENT else RelayLoginStatus.SEND_FAILED, + ) + } + } + + override fun onIncomingMessage( + relay: IRelayClient, + msgStr: String, + msg: Message, + ) { + } + } + // --- Account loading --- suspend fun loadSavedAccount(): Result = try { - val lastNpub = getLastNpub() ?: return Result.failure(Exception("No saved account")) - - // Check for bunker account first + val lastNpub = getLastNpub() val bunkerUri = getBunkerUri() + if (bunkerUri != null) { loadBunkerAccount(bunkerUri, lastNpub) - } else { + } else if (lastNpub != null) { loadInternalAccount(lastNpub) + } else { + Result.failure(Exception("No saved account")) } } catch (e: Exception) { Result.failure(e) @@ -213,7 +275,7 @@ class AccountManager internal constructor( private suspend fun loadBunkerAccount( bunkerUri: String, - npub: String, + npub: String?, ): Result { val ephemeralPrivKeyHex = secureStorage.getPrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS) @@ -226,13 +288,24 @@ class AccountManager internal constructor( val remoteSigner = NostrSignerRemote.fromBunkerUri(bunkerUri, ephemeralSigner, nip46Client) remoteSigner.openSubscription() - val pubKeyHex = decodePublicKeyAsHexOrNull(npub) ?: return Result.failure(Exception("Invalid saved npub")) + val pubKeyHex = + if (npub != null) { + decodePublicKeyAsHexOrNull(npub) ?: return Result.failure(Exception("Invalid saved npub")) + } else { + // npub missing (e.g. last_account.txt deleted) — must wait for relay + // before calling getPublicKey() to recover from signer + awaitNip46RelayConnection(nip46Client, remoteSigner.relays) + remoteSigner.getPublicKey() + } + + val resolvedNpub = npub ?: pubKeyHex.hexToByteArray().toNpub() + if (npub == null) saveLastNpub(resolvedNpub) val state = AccountState.LoggedIn( signer = remoteSigner, pubKeyHex = pubKeyHex, - npub = npub, + npub = resolvedNpub, nsec = null, isReadOnly = false, signerType = SignerType.Remote(bunkerUri), @@ -244,18 +317,33 @@ class AccountManager internal constructor( // --- Bunker login --- - suspend fun loginWithBunker(bunkerUri: String): Result = + suspend fun loginWithBunker(bunkerUri: String): Result { + val listener = createLoginRelayListener() + var client: NostrClient? = null try { val ephemeralKeyPair = KeyPair() val ephemeralSigner = NostrSignerInternal(ephemeralKeyPair) val nip46Client = getOrCreateNip46Client() + client = nip46Client val remoteSigner = NostrSignerRemote.fromBunkerUri(bunkerUri, ephemeralSigner, nip46Client) + + // Emit connecting with initial relay statuses + _loginProgress.value = + LoginProgress.ConnectingToRelays( + remoteSigner.relays.associateWith { RelayLoginStatus.CONNECTING }, + ) + nip46Client.subscribe(listener) + remoteSigner.openSubscription() // Wait for websocket to be ready before sending connect request awaitNip46RelayConnection(nip46Client, remoteSigner.relays) + _loginProgress.value = + LoginProgress.WaitingForSigner( + relayStatuses = _loginProgress.value?.relayStatuses.orEmpty(), + ) val remotePubkey = remoteSigner.connect() val state = @@ -277,22 +365,28 @@ class AccountManager internal constructor( npub = state.npub, ) - Result.success(state) + return Result.success(state) } catch (e: kotlinx.coroutines.TimeoutCancellationException) { - Result.failure(Exception("Could not connect to NIP-46 relay. Check your network connection.")) + return Result.failure(Exception("Could not connect to NIP-46 relay. Check your network connection.")) } catch (e: SignerExceptions.TimedOutException) { - Result.failure(Exception("Connection timed out. Ensure remote signer is online and has approved the connection.")) + return Result.failure(Exception("Connection timed out. Ensure remote signer is online and has approved the connection.")) } catch (e: SignerExceptions.ManuallyUnauthorizedException) { - Result.failure(Exception("Connection rejected by remote signer.")) + return Result.failure(Exception("Connection rejected by remote signer.")) } catch (e: SignerExceptions.CouldNotPerformException) { - Result.failure(Exception("Remote signer error: ${e.message}")) + return Result.failure(Exception("Remote signer error: ${e.message}")) } catch (e: Exception) { - Result.failure(Exception("Connection failed: ${e.message}")) + return Result.failure(Exception("Connection failed: ${e.message}")) + } finally { + _loginProgress.value = null + client?.unsubscribe(listener) } + } // --- Nostrconnect login --- - suspend fun loginWithNostrConnect(onUriGenerated: (String) -> Unit): Result = + suspend fun loginWithNostrConnect(onUriGenerated: (String) -> Unit): Result { + val listener = createLoginRelayListener() + var client: NostrClient? = null try { val ephemeralKeyPair = KeyPair() val ephemeralSigner = NostrSignerInternal(ephemeralKeyPair) @@ -302,13 +396,31 @@ class AccountManager internal constructor( val relays = NIP46_RELAYS val relayParams = relays.joinToString("&") { "relay=$it" } val uri = "nostrconnect://$ephemeralPubKey?$relayParams&secret=$secret&name=Amethyst%20Desktop" - onUriGenerated(uri) val nip46Client = getOrCreateNip46Client() + client = nip46Client val normalizedRelays = relays.map { NormalizedRelayUrl(it) }.toSet() + + // Emit connecting with initial relay statuses + _loginProgress.value = + LoginProgress.ConnectingToRelays( + normalizedRelays.associateWith { RelayLoginStatus.CONNECTING }, + ) + nip46Client.subscribe(listener) + + onUriGenerated(uri) + + _loginProgress.value = + LoginProgress.WaitingForSigner( + relayStatuses = _loginProgress.value?.relayStatuses.orEmpty(), + ) val connectData = waitForConnectRequest(ephemeralSigner, ephemeralPubKey, normalizedRelays, secret, nip46Client) if (connectData.requestId != null) { + _loginProgress.value = + LoginProgress.SendingAck( + relayStatuses = _loginProgress.value?.relayStatuses.orEmpty(), + ) sendAckResponse(ephemeralSigner, connectData, normalizedRelays, nip46Client) } @@ -321,13 +433,17 @@ class AccountManager internal constructor( ) remoteSigner.openSubscription() + // Verify user pubkey via get_public_key — connect params may contain + // the wrong pubkey (e.g. ephemeral key echoed back by some signers) + val verifiedPubkey = remoteSigner.getPublicKey() + val syntheticBunkerUri = "bunker://${connectData.signerPubkey}?$relayParams" val state = AccountState.LoggedIn( signer = remoteSigner, - pubKeyHex = connectData.userPubkey, - npub = connectData.userPubkey.hexToByteArray().toNpub(), + pubKeyHex = verifiedPubkey, + npub = verifiedPubkey.hexToByteArray().toNpub(), nsec = null, isReadOnly = false, signerType = SignerType.Remote(syntheticBunkerUri), @@ -341,12 +457,16 @@ class AccountManager internal constructor( npub = state.npub, ) - Result.success(state) + return Result.success(state) } catch (e: kotlinx.coroutines.TimeoutCancellationException) { - Result.failure(Exception("Timed out waiting for signer. Ensure the signer app scanned the QR code.")) + return Result.failure(Exception("Timed out waiting for signer. Ensure the signer app scanned the QR code.")) } catch (e: Exception) { - Result.failure(Exception("Connection failed: ${e.message}")) + return Result.failure(Exception("Connection failed: ${e.message}")) + } finally { + _loginProgress.value = null + client?.unsubscribe(listener) } + } private suspend fun waitForConnectRequest( ephemeralSigner: NostrSignerInternal, @@ -463,9 +583,9 @@ class AccountManager internal constructor( ephemeralPrivKeyHex: String, npub: String, ) { - saveBunkerUri(bunkerUri) - secureStorage.savePrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS, ephemeralPrivKeyHex) saveLastNpub(npub) + secureStorage.savePrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS, ephemeralPrivKeyHex) + saveBunkerUri(bunkerUri) } fun hasBunkerAccount(): Boolean = getBunkerFile().exists() diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/LoginProgress.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/LoginProgress.kt new file mode 100644 index 000000000..1ce722afa --- /dev/null +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/LoginProgress.kt @@ -0,0 +1,47 @@ +/* + * 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.amethyst.desktop.account + +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl + +enum class RelayLoginStatus { + CONNECTING, + CONNECTED, + EVENT_SENT, + SEND_FAILED, + FAILED, +} + +sealed class LoginProgress { + abstract val relayStatuses: Map + + data class ConnectingToRelays( + override val relayStatuses: Map = emptyMap(), + ) : LoginProgress() + + data class WaitingForSigner( + override val relayStatuses: Map = emptyMap(), + ) : LoginProgress() + + data class SendingAck( + override val relayStatuses: Map = emptyMap(), + ) : LoginProgress() +} diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/network/RelayStatus.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/network/RelayStatus.kt index 24c092b90..5266f9621 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/network/RelayStatus.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/network/RelayStatus.kt @@ -45,5 +45,6 @@ object DefaultRelays { "wss://nos.lol", "wss://relay.snort.social", "wss://nostr.wine", + "wss://relay.primal.net", ) } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SubscriptionUtils.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SubscriptionUtils.kt index 4bf7062e5..8aecc2f2d 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SubscriptionUtils.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SubscriptionUtils.kt @@ -23,7 +23,6 @@ package com.vitorpamplona.amethyst.desktop.subscriptions import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.remember -import com.vitorpamplona.amethyst.desktop.DebugConfig import com.vitorpamplona.amethyst.desktop.network.RelayConnectionManager import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.IRequestListener @@ -75,7 +74,6 @@ fun rememberSubscription( DisposableEffect(*keys, subscription?.subId) { subscription?.let { cfg -> if (cfg.relays.isNotEmpty()) { - DebugConfig.log("SUB OPEN ${cfg.subId} relays=${cfg.relays.size}") relayManager.subscribe( subId = cfg.subId, filters = cfg.filters, @@ -104,7 +102,6 @@ fun rememberSubscription( onDispose { subscription?.let { - DebugConfig.log("SUB CLOSE ${it.subId}") relayManager.unsubscribe(it.subId) } } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/FeedScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/FeedScreen.kt index 38ce7b121..12523a85c 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/FeedScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/FeedScreen.kt @@ -84,6 +84,8 @@ import com.vitorpamplona.quartz.nip18Reposts.RepostEvent import com.vitorpamplona.quartz.nip25Reactions.ReactionEvent import com.vitorpamplona.quartz.nip51Lists.bookmarkList.BookmarkListEvent import com.vitorpamplona.quartz.nip57Zaps.LnZapEvent +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map /** * Note card with action buttons. @@ -159,6 +161,14 @@ fun FeedScreen( onZapFeedback: (ZapFeedback) -> Unit = {}, ) { val connectedRelays by relayManager.connectedRelays.collectAsState() + // Configured relay URLs only — stabilized with distinctUntilChanged() to prevent + // subscription churn from relay status changes (pings, connect/disconnect). + // openReqSubscription connects relays on demand; no need to wait for connectedRelays. + val configuredRelays by remember { + relayManager.relayStatuses + .map { it.keys } + .distinctUntilChanged() + }.collectAsState(emptySet()) val scope = rememberCoroutineScope() val eventState = remember { @@ -190,17 +200,15 @@ fun FeedScreen( val initialLoadComplete = eoseReceivedCount > 0 // Load followed users for Following feed mode - rememberSubscription(connectedRelays, account, feedMode, relayManager = relayManager) { - DebugConfig.log("contactList sub: relays=${connectedRelays.size}, account=${account?.pubKeyHex?.take(8)}, mode=$feedMode") - if (connectedRelays.isNotEmpty() && account != null && feedMode == FeedMode.FOLLOWING) { + rememberSubscription(configuredRelays, account, feedMode, relayManager = relayManager) { + if (configuredRelays.isNotEmpty() && account != null && feedMode == FeedMode.FOLLOWING) { createContactListSubscription( - relays = connectedRelays, + relays = configuredRelays, pubKeyHex = account.pubKeyHex, onEvent = { event, _, relay, _ -> - DebugConfig.log("contactList event: kind=${event.kind}, isContactList=${event is ContactListEvent}, from=$relay") if (event is ContactListEvent) { val follows = event.verifiedFollowKeySet() - DebugConfig.log("followedUsers: ${follows.size} users") + DebugConfig.log("contactList: ${follows.size} follows from $relay") followedUsers = follows } }, @@ -211,8 +219,8 @@ fun FeedScreen( } // Load user's bookmark list - rememberSubscription(connectedRelays, account, relayManager = relayManager) { - if (connectedRelays.isNotEmpty() && account != null) { + rememberSubscription(configuredRelays, account, relayManager = relayManager) { + if (configuredRelays.isNotEmpty() && account != null) { SubscriptionConfig( subId = "bookmarks-${account.pubKeyHex.take(8)}", filters = @@ -223,7 +231,7 @@ fun FeedScreen( limit = 1, ), ), - relays = connectedRelays, + relays = configuredRelays, onEvent = { event, _, _, _ -> if (event is BookmarkListEvent) { bookmarkList = event @@ -251,16 +259,16 @@ fun FeedScreen( } // Subscribe to feed based on mode - rememberSubscription(connectedRelays, feedMode, followedUsers, relayManager = relayManager) { - DebugConfig.log("feedSub: mode=$feedMode, relays=${connectedRelays.size}, followedUsers=${followedUsers.size}") - if (connectedRelays.isEmpty()) { + rememberSubscription(configuredRelays, feedMode, followedUsers, relayManager = relayManager) { + DebugConfig.log("feedSub: mode=$feedMode, relays=${configuredRelays.size}, followedUsers=${followedUsers.size}") + if (configuredRelays.isEmpty()) { return@rememberSubscription null } when (feedMode) { FeedMode.GLOBAL -> { createGlobalFeedSubscription( - relays = connectedRelays, + relays = configuredRelays, onEvent = { event, _, _, _ -> // Store metadata events in cache if (event is MetadataEvent) { @@ -277,7 +285,7 @@ fun FeedScreen( FeedMode.FOLLOWING -> { if (followedUsers.isNotEmpty()) { createFollowingFeedSubscription( - relays = connectedRelays, + relays = configuredRelays, followedUsers = followedUsers.toList(), onEvent = { event, _, _, _ -> // Store metadata events in cache @@ -299,13 +307,13 @@ fun FeedScreen( // Subscribe to zaps for visible events val eventIds = events.map { it.id } - rememberSubscription(connectedRelays, eventIds, relayManager = relayManager) { - if (connectedRelays.isEmpty() || eventIds.isEmpty()) { + rememberSubscription(configuredRelays, eventIds, relayManager = relayManager) { + if (configuredRelays.isEmpty() || eventIds.isEmpty()) { return@rememberSubscription null } createZapsSubscription( - relays = connectedRelays, + relays = configuredRelays, eventIds = eventIds, onEvent = { event, _, _, _ -> if (event is LnZapEvent) { @@ -329,8 +337,8 @@ fun FeedScreen( .flatten() .map { it.senderPubKey } .distinct() - rememberSubscription(connectedRelays, zapSenderPubkeys, relayManager = relayManager) { - if (connectedRelays.isEmpty() || zapSenderPubkeys.isEmpty()) { + rememberSubscription(configuredRelays, zapSenderPubkeys, relayManager = relayManager) { + if (configuredRelays.isEmpty() || zapSenderPubkeys.isEmpty()) { return@rememberSubscription null } @@ -348,7 +356,7 @@ fun FeedScreen( } createBatchMetadataSubscription( - relays = connectedRelays, + relays = configuredRelays, pubKeyHexList = missingPubkeys, onEvent = { event, _, _, _ -> if (event is MetadataEvent) { @@ -359,13 +367,13 @@ fun FeedScreen( } // Subscribe to reactions for visible events - rememberSubscription(connectedRelays, eventIds, relayManager = relayManager) { - if (connectedRelays.isEmpty() || eventIds.isEmpty()) { + rememberSubscription(configuredRelays, eventIds, relayManager = relayManager) { + if (configuredRelays.isEmpty() || eventIds.isEmpty()) { return@rememberSubscription null } createReactionsSubscription( - relays = connectedRelays, + relays = configuredRelays, eventIds = eventIds, onEvent = { event, _, _, _ -> if (event is ReactionEvent) { @@ -381,13 +389,13 @@ fun FeedScreen( } // Subscribe to replies for visible events - rememberSubscription(connectedRelays, eventIds, relayManager = relayManager) { - if (connectedRelays.isEmpty() || eventIds.isEmpty()) { + rememberSubscription(configuredRelays, eventIds, relayManager = relayManager) { + if (configuredRelays.isEmpty() || eventIds.isEmpty()) { return@rememberSubscription null } createRepliesSubscription( - relays = connectedRelays, + relays = configuredRelays, eventIds = eventIds, onEvent = { event, _, _, _ -> // Find the event this is replying to @@ -408,13 +416,13 @@ fun FeedScreen( } // Subscribe to reposts for visible events - rememberSubscription(connectedRelays, eventIds, relayManager = relayManager) { - if (connectedRelays.isEmpty() || eventIds.isEmpty()) { + rememberSubscription(configuredRelays, eventIds, relayManager = relayManager) { + if (configuredRelays.isEmpty() || eventIds.isEmpty()) { return@rememberSubscription null } createRepostsSubscription( - relays = connectedRelays, + relays = configuredRelays, eventIds = eventIds, onEvent = { event, _, _, _ -> if (event is RepostEvent) { @@ -440,13 +448,13 @@ fun FeedScreen( } // Fallback subscription if coordinator not available - rememberSubscription(connectedRelays, authorPubkeys, subscriptionsCoordinator, relayManager = relayManager) { + rememberSubscription(configuredRelays, authorPubkeys, subscriptionsCoordinator, relayManager = relayManager) { // Skip if using coordinator if (subscriptionsCoordinator != null) { return@rememberSubscription null } - if (connectedRelays.isEmpty() || authorPubkeys.isEmpty()) { + if (configuredRelays.isEmpty() || authorPubkeys.isEmpty()) { return@rememberSubscription null } @@ -464,7 +472,7 @@ fun FeedScreen( } createBatchMetadataSubscription( - relays = connectedRelays, + relays = configuredRelays, pubKeyHexList = missingPubkeys, onEvent = { event, _, _, _ -> if (event is MetadataEvent) { diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt index 050d829ed..52b5dbee0 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt @@ -41,6 +41,7 @@ import androidx.compose.material3.LinearProgressIndicator import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -71,6 +72,7 @@ fun LoginScreen( var showNewKeyDialog by remember { mutableStateOf(false) } var generatedAccount by remember { mutableStateOf(null) } val scope = rememberCoroutineScope() + val loginProgress by accountManager.loginProgress.collectAsState() Column( modifier = Modifier.fillMaxSize().padding(32.dp), @@ -116,6 +118,7 @@ fun LoginScreen( onLoginSuccess() } }, + loginProgress = loginProgress, ) val account = generatedAccount diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginCard.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginCard.kt index 04e86c919..67d1bcdf7 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginCard.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginCard.kt @@ -61,6 +61,7 @@ import com.vitorpamplona.amethyst.commons.resources.login_button import com.vitorpamplona.amethyst.commons.resources.login_card_subtitle import com.vitorpamplona.amethyst.commons.resources.login_card_title import com.vitorpamplona.amethyst.commons.resources.login_generate_button +import com.vitorpamplona.amethyst.desktop.account.LoginProgress import com.vitorpamplona.amethyst.desktop.account.validateBunkerUri import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch @@ -73,6 +74,7 @@ fun LoginCard( onGenerateNew: () -> Unit, onLoginBunker: (suspend (String) -> Result)? = null, onLoginNostrConnect: (suspend (onUriGenerated: (String) -> Unit) -> Result)? = null, + loginProgress: LoginProgress? = null, modifier: Modifier = Modifier, cardWidth: Dp = 400.dp, title: String = stringResource(Res.string.login_card_title), @@ -119,8 +121,8 @@ fun LoginCard( } when (selectedTab) { - 0 -> PasteKeyContent(onLogin, onGenerateNew, onLoginBunker, subtitle) - 1 -> if (onLoginNostrConnect != null) NostrConnectContent(onLoginNostrConnect) + 0 -> PasteKeyContent(onLogin, onGenerateNew, onLoginBunker, subtitle, loginProgress) + 1 -> if (onLoginNostrConnect != null) NostrConnectContent(onLoginNostrConnect, loginProgress) } } } @@ -132,6 +134,7 @@ private fun PasteKeyContent( onGenerateNew: () -> Unit, onLoginBunker: (suspend (String) -> Result)?, subtitle: String, + loginProgress: LoginProgress? = null, ) { var keyInput by remember { mutableStateOf("") } var errorMessage by remember { mutableStateOf(null) } @@ -167,21 +170,25 @@ private fun PasteKeyContent( Spacer(Modifier.height(16.dp)) if (isConnecting) { - Row( - modifier = Modifier.fillMaxWidth(), - horizontalArrangement = Arrangement.Center, - verticalAlignment = Alignment.CenterVertically, - ) { - CircularProgressIndicator( - modifier = Modifier.size(20.dp), - strokeWidth = 2.dp, - ) - Spacer(Modifier.width(12.dp)) - Text( - "Connecting to remote signer...", - style = MaterialTheme.typography.bodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) + if (loginProgress != null) { + LoginProgressSteps(loginProgress) + } else { + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.Center, + verticalAlignment = Alignment.CenterVertically, + ) { + CircularProgressIndicator( + modifier = Modifier.size(20.dp), + strokeWidth = 2.dp, + ) + Spacer(Modifier.width(12.dp)) + Text( + "Connecting to remote signer...", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } } } else { Row( @@ -234,7 +241,10 @@ private fun PasteKeyContent( } @Composable -private fun NostrConnectContent(onLoginNostrConnect: suspend (onUriGenerated: (String) -> Unit) -> Result) { +private fun NostrConnectContent( + onLoginNostrConnect: suspend (onUriGenerated: (String) -> Unit) -> Result, + loginProgress: LoginProgress? = null, +) { var nostrConnectUri by remember { mutableStateOf(null) } var isConnecting by remember { mutableStateOf(false) } var errorMessage by remember { mutableStateOf(null) } @@ -316,21 +326,25 @@ private fun NostrConnectContent(onLoginNostrConnect: suspend (onUriGenerated: (S Spacer(Modifier.height(12.dp)) - Row( - modifier = Modifier.fillMaxWidth(), - horizontalArrangement = Arrangement.Center, - verticalAlignment = Alignment.CenterVertically, - ) { - CircularProgressIndicator( - modifier = Modifier.size(16.dp), - strokeWidth = 2.dp, - ) - Spacer(Modifier.width(8.dp)) - Text( - "Waiting for signer to connect...", - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) + if (loginProgress != null) { + LoginProgressSteps(loginProgress) + } else { + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.Center, + verticalAlignment = Alignment.CenterVertically, + ) { + CircularProgressIndicator( + modifier = Modifier.size(16.dp), + strokeWidth = 2.dp, + ) + Spacer(Modifier.width(8.dp)) + Text( + "Waiting for signer to connect...", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } } } else { Row( diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginProgressSteps.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginProgressSteps.kt new file mode 100644 index 000000000..08655a5c4 --- /dev/null +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/auth/LoginProgressSteps.kt @@ -0,0 +1,220 @@ +/* + * 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.amethyst.desktop.ui.auth + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Check +import androidx.compose.material.icons.filled.Close +import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Color +import androidx.compose.ui.unit.dp +import com.vitorpamplona.amethyst.desktop.account.LoginProgress +import com.vitorpamplona.amethyst.desktop.account.RelayLoginStatus + +private enum class StepState { DONE, ACTIVE, PENDING } + +private data class StepInfo( + val label: String, + val state: StepState, +) + +@Composable +fun LoginProgressSteps( + progress: LoginProgress, + modifier: Modifier = Modifier, +) { + val steps = buildStepList(progress) + + Column( + modifier = modifier.fillMaxWidth(), + verticalArrangement = Arrangement.spacedBy(6.dp), + ) { + steps.forEach { step -> + StepRow(step) + + // Show relay rows under the active step + if (step.state == StepState.ACTIVE && progress.relayStatuses.isNotEmpty()) { + progress.relayStatuses.forEach { (relay, status) -> + RelayRow(relay.url, status) + } + } + } + } +} + +private fun buildStepList(progress: LoginProgress): List { + val orderedSteps = + listOf( + "Connecting to relays", + "Waiting for signer", + "Sending acknowledgment", + ) + + val activeIndex = + when (progress) { + is LoginProgress.ConnectingToRelays -> 0 + is LoginProgress.WaitingForSigner -> 1 + is LoginProgress.SendingAck -> 2 + } + + return orderedSteps.mapIndexed { index, label -> + StepInfo( + label = label, + state = + when { + index < activeIndex -> StepState.DONE + index == activeIndex -> StepState.ACTIVE + else -> StepState.PENDING + }, + ) + } +} + +@Composable +private fun StepRow(step: StepInfo) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth(), + ) { + when (step.state) { + StepState.DONE -> { + Icon( + Icons.Default.Check, + contentDescription = null, + tint = Color(0xFF4CAF50), + modifier = Modifier.size(16.dp), + ) + } + + StepState.ACTIVE -> { + CircularProgressIndicator( + modifier = Modifier.size(16.dp), + strokeWidth = 2.dp, + ) + } + + StepState.PENDING -> { + Spacer(Modifier.size(16.dp)) + } + } + Spacer(Modifier.width(8.dp)) + Text( + step.label, + style = MaterialTheme.typography.bodySmall, + color = + when (step.state) { + StepState.DONE -> Color(0xFF4CAF50) + StepState.ACTIVE -> MaterialTheme.colorScheme.onSurface + StepState.PENDING -> MaterialTheme.colorScheme.onSurfaceVariant.copy(alpha = 0.5f) + }, + ) + } +} + +@Composable +private fun RelayRow( + url: String, + status: RelayLoginStatus, +) { + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth().padding(start = 24.dp), + ) { + when (status) { + RelayLoginStatus.EVENT_SENT -> { + Icon( + Icons.Default.Check, + contentDescription = null, + tint = Color(0xFF2196F3), + modifier = Modifier.size(12.dp), + ) + } + + RelayLoginStatus.CONNECTED -> { + Icon( + Icons.Default.Check, + contentDescription = null, + tint = Color(0xFF4CAF50), + modifier = Modifier.size(12.dp), + ) + } + + RelayLoginStatus.FAILED, + RelayLoginStatus.SEND_FAILED, + -> { + Icon( + Icons.Default.Close, + contentDescription = null, + tint = MaterialTheme.colorScheme.error, + modifier = Modifier.size(12.dp), + ) + } + + RelayLoginStatus.CONNECTING -> { + CircularProgressIndicator( + modifier = Modifier.size(12.dp), + strokeWidth = 1.5.dp, + ) + } + } + Spacer(Modifier.width(6.dp)) + val label = url.removePrefix("wss://").removeSuffix("/") + val statusLabel = + when (status) { + RelayLoginStatus.EVENT_SENT -> "$label (sent)" + RelayLoginStatus.SEND_FAILED -> "$label (send failed)" + RelayLoginStatus.FAILED -> "$label (failed)" + else -> label + } + Text( + statusLabel, + style = MaterialTheme.typography.labelSmall, + color = + when (status) { + RelayLoginStatus.FAILED, RelayLoginStatus.SEND_FAILED -> { + MaterialTheme.colorScheme.error.copy(alpha = 0.8f) + } + + RelayLoginStatus.EVENT_SENT -> { + Color(0xFF2196F3) + } + + else -> { + MaterialTheme.colorScheme.onSurfaceVariant + } + }, + ) + } +} diff --git a/docs/brainstorms/2026-03-06-nip46-post-login-bugs-brainstorm.md b/docs/brainstorms/2026-03-06-nip46-post-login-bugs-brainstorm.md new file mode 100644 index 000000000..607446d47 --- /dev/null +++ b/docs/brainstorms/2026-03-06-nip46-post-login-bugs-brainstorm.md @@ -0,0 +1,189 @@ +--- +date: 2026-03-06 +topic: nip46-post-login-bugs +--- + +# NIP-46 Post-Login Bugs: Content Not Loading + Bunker Timeout + +## What's Happening + +Two bugs on branch `feat/nip46-bunker-login` after the NIP-46 relay isolation work: + +### Bug 1: `followedUsers=0` — Feed Never Populates After Login + +**Symptoms:** +- Contact list subscription opens/closes rapidly (subscription churn) +- No `[DEBUG-FEED] contactList event:` log — no kind-3 event ever arrives +- Feed stays on "Loading followed users..." forever + +**Logs:** +``` +[DEBUG-FEED] contactList sub: relays=5, account=0b24845a, mode=FOLLOWING +[DEBUG-FEED] feedSub: mode=FOLLOWING, relays=5, followedUsers=0 +[DEBUG-SUB] OPEN contacts-0b24845a-1772773857361 relays=5 +[DEBUG-SUB] CLOSE contacts-0b24845a-1772773857361 +[DEBUG-SUB] OPEN contacts-0b24845a-1772773867946 relays=5 +[DEBUG-SUB] CLOSE contacts-0b24845a-1772773867946 +``` + +### Bug 2: `bunker://` Login Times Out (30s) + +**Symptoms:** +- Pasting bunker:// URI → "Connection timed out" after 30s +- `RemoteSignerManager.timeout = 30000` controls this + +## Root Cause Analysis + +### Bug 1: Subscription Churn from `relayStatuses.keys` + +**The pattern** (`FeedScreen.kt:193`): +```kotlin +rememberSubscription(relayStatuses.keys, account, feedMode, ...) { ... } +``` + +**The problem:** +1. `relayStatuses` is a `StateFlow>` +2. Every relay status change (connect, disconnect, ping) emits a new map +3. `.keys` produces a structurally-equal but reference-different set +4. `remember(*keys)` re-evaluates → `DisposableEffect` disposes old sub → opens new sub +5. New sub gets a fresh `subId` (uses `System.currentTimeMillis()`) +6. Relay's response to old REQ arrives → client drops it (unknown subId) +7. New REQ sent → relay starts processing → another status change → cycle repeats + +**Timeline:** +``` +T+0s: Relay A connects → relayStatuses emits → sub opens (subId=1001) +T+1s: Relay B connects → relayStatuses emits → sub CLOSES (subId=1001), opens (subId=1002) +T+2s: Relay C connects → same cycle → CLOSE 1002, OPEN 1003 +T+3s: Relay responds to subId=1001 → DROPPED (sub already closed) +T+4s: Relay responds to subId=1003 → maybe received, maybe closed again +``` + +**Contributing factor:** `relayStatuses.keys` includes configured-but-not-connected relays. REQs to unconnected relays are queued but may never be sent. + +### Bug 2: NIP-46 Relay Connection Race + +**The flow** (`AccountManager.loginWithBunker()`): +```kotlin +val nip46Client = getOrCreateNip46Client() // creates NostrClient, calls connect() (no-op: no relays) +val remoteSigner = NostrSignerRemote.fromBunkerUri(bunkerUri, signer, nip46Client) +remoteSigner.openSubscription() // calls client.req(relays=[relay.nsec.app]) → lazy connect +val remotePubkey = remoteSigner.connect() // sends connect request, waits 30s for response +``` + +**The problem:** +- `openSubscription()` triggers `sendOrConnectAndSync(relay.nsec.app, REQ)` — async connection +- `connect()` calls `launchWaitAndParse()` which sends EVENT immediately +- If websocket to relay.nsec.app isn't established yet, EVENT send fails/queues +- Even if EVENT sends, relay may not have processed REQ yet → response has no matching sub +- 30s timeout expires + +**Also:** Only 1 relay (`wss://relay.nsec.app`) — single point of failure. + +## Approaches + +### Bug 1 Fix: Stabilize Subscription Keys + +#### Approach A: Use `connectedRelays` + Debounce (Recommended) + +Replace `relayStatuses.keys` with a debounced/stable relay set. + +```kotlin +// Stable relay set that only updates when relay URLs actually change +val stableRelays by remember { + snapshotFlow { connectedRelays } + .distinctUntilChanged() + .debounce(2000) // wait for relays to stabilize +}.collectAsState(initial = emptySet()) + +rememberSubscription(stableRelays, account, feedMode, ...) { ... } +``` + +**Pros:** Eliminates churn, only resubscribes when relay set actually changes +**Cons:** 2s delay before first subscription opens + +#### Approach B: Separate Subscription Lifecycle from Relay Set + +Don't use relay set as a recomposition key at all. Open subscription ONCE with whatever relays are available, never re-create it. + +```kotlin +// Only recompose on account/feedMode changes, not relay changes +rememberSubscription(account, feedMode, relayManager = relayManager) { + val relays = connectedRelays // snapshot, not reactive + if (relays.isNotEmpty() && account != null) { + createContactListSubscription(relays = relays, ...) + } else null +} +``` + +**Pros:** Zero churn, simplest fix +**Cons:** If initial relay set is empty, subscription never opens (need LaunchedEffect to wait for relays) + +#### Approach C: LaunchedEffect + One-Shot Subscription + +Don't use `rememberSubscription` for contact list at all. Use a `LaunchedEffect` that waits for relays then subscribes once. + +```kotlin +LaunchedEffect(account) { + val relays = relayManager.connectedRelays.first { it.isNotEmpty() } + relayManager.subscribe("contacts-${account.pubKeyHex.take(8)}", + listOf(FilterBuilders.contactList(account.pubKeyHex)), relays, + listener = object : IRequestListener { ... }) +} +``` + +**Pros:** No churn, explicit lifecycle, clear intent +**Cons:** Manual cleanup needed, doesn't auto-update if relay set changes later + +### Bug 2 Fix: Wait for NIP-46 Relay Connection + +#### Approach A: Wait for Connected Relay Before connect() (Recommended) + +```kotlin +val nip46Client = getOrCreateNip46Client() +val remoteSigner = NostrSignerRemote.fromBunkerUri(bunkerUri, signer, nip46Client) +remoteSigner.openSubscription() + +// Wait for NIP-46 relay to actually connect before sending connect request +nip46Client.connectedRelaysFlow().first { relays -> + remoteSigner.relays.any { it in relays } +} + +val remotePubkey = remoteSigner.connect() +``` + +**Pros:** Guarantees relay is connected before handshake +**Cons:** Adds wait time; if relay never connects, blocks indefinitely (need timeout) + +#### Approach B: Add Retry Logic to connect() + +Wrap `connect()` in retry with backoff: +```kotlin +val remotePubkey = retry(maxAttempts = 3, delayMs = 5000) { + remoteSigner.connect() +} +``` + +**Pros:** Handles transient failures +**Cons:** Doesn't fix root cause, just masks it; up to 90s total wait + +#### Approach C: Increase Timeout + Add Debug Logging + +Bump `RemoteSignerManager.timeout` to 60s, add logging to track relay connection state. + +**Pros:** Quick, low-risk +**Cons:** Doesn't fix the race; just makes timeout less likely + +## Key Decisions + +- Bug 1 is the higher priority — it affects ALL logins, not just bunker +- Bug 1 Approach B or C preferred — simplest, eliminates churn entirely +- Bug 2 Approach A preferred — fixes the actual race condition +- Both fixes are independent and can be done in parallel + +## Open Questions + +1. Does the contact list fail for nsec/key login too, or only bunker? (Would confirm if it's relay churn vs NIP-46 specific) +2. Is `relay.nsec.app` reachable from this machine? (`websocat wss://relay.nsec.app` test) +3. Should the bunker URI's relays be used for content subscriptions too? (User's relays vs default relays) +4. Should we add a fallback relay for NIP-46? (e.g., `wss://relay.damus.io` alongside `relay.nsec.app`) diff --git a/docs/brainstorms/2026-03-09-feedscreen-relay-subscription-strategy-brainstorm.md b/docs/brainstorms/2026-03-09-feedscreen-relay-subscription-strategy-brainstorm.md new file mode 100644 index 000000000..e68e16c5e --- /dev/null +++ b/docs/brainstorms/2026-03-09-feedscreen-relay-subscription-strategy-brainstorm.md @@ -0,0 +1,209 @@ +# Brainstorm: FeedScreen Relay Subscription Strategy + +**Date:** 2026-03-09 +**Status:** Investigation complete, solution needed +**Branch:** `feat/nip46-bunker-login` + +## Problem Statement + +Follows don't load for nostrconnect:// login. The root cause is a **chicken-and-egg problem** with how FeedScreen drives relay subscriptions — two competing approaches each solve one problem but break the other. + +## What We Found + +### The Two Approaches + +| Approach | Drives relay connections? | Avoids subscription churn? | +|----------|--------------------------|---------------------------| +| `configuredRelays` (relayStatuses.keys) | YES — openReqSubscription connects on demand | NO — status map emits on every ping/connect/disconnect | +| `connectedRelays` (connectedRelaysFlow) | NO — subscriptions never open if nothing is connected | YES — only changes when relay actually connects/disconnects | + +### How `configuredRelays` Works (Current Working Dir) + +```kotlin +val relayStatusMap by relayManager.relayStatuses.collectAsState() +val configuredRelays = relayStatusMap.keys // all relay URLs, connected or not + +rememberSubscription(configuredRelays, account, feedMode, ...) { + createContactListSubscription(relays = configuredRelays, ...) +} +``` + +- `addDefaultRelays()` populates `relayStatuses` map immediately at app startup +- `openReqSubscription()` (inside subscribe) connects to relays on demand +- Relays connect, REQs are sent, events arrive +- **Problem:** `relayStatusMap` emits a new Map on every relay status change (connect, ping, disconnect). Each emission triggers recomposition. `relayStatusMap.keys` creates a new Set instance. Even though `Set.equals()` is structural, `remember(*keys)` in Compose may see the new reference + trigger config lambda re-evaluation, generating a new subId via `generateSubId()` (uses `System.currentTimeMillis()`). DisposableEffect sees new subId -> closes old subscription -> opens new one. Contact list responses to old subId are dropped. + +### How `connectedRelays` Works (Committed in e58aaf411) + +```kotlin +val connectedRelays by relayManager.connectedRelays.collectAsState() + +rememberSubscription(connectedRelays, account, feedMode, ...) { + createContactListSubscription(relays = connectedRelays, ...) +} +``` + +- Only includes actually-connected relays +- Subscription only recreates when a relay actually connects/disconnects +- **Problem:** Chicken-and-egg. Nothing triggers relay connections in the first place. `addDefaultRelays()` only adds to the status map, NOT to the NostrClient. `relayManager.connect()` is a no-op when NostrClient has no registered relays. Relays only get registered when `openReqSubscription()` is called. But `openReqSubscription()` is never called because `connectedRelays` is empty, so the subscription lambda returns null. + +### Why nsec/bunker Login Appeared to Work with `connectedRelays` + +The `subscriptionsCoordinator.start()` in Main.kt kicks off a `rateLimiter.start` that calls `client.openReqSubscription()` for metadata. This incidentally registers relays with the NostrClient and triggers connections. Once relays connect, `connectedRelays` becomes non-empty, and FeedScreen subscriptions fire. + +But this is fragile — it depends on the coordinator's metadata requests happening before FeedScreen needs relays. For nostrconnect (where login takes longer), the timing may differ. + +## Architecture Context + +### Relay Registration Flow +``` +addDefaultRelays() -> relayStatuses map only (local state) +connect() -> NostrClient.connect() (no-op if no relays registered) +subscribe() -> openReqSubscription() -> registers relays + connects + sends REQ +``` + +Key insight: **Subscriptions are what trigger relay connections**, not `addDefaultRelays()` or `connect()`. + +### Key Files +- `FeedScreen.kt` — all feed subscriptions, follows loading +- `SubscriptionUtils.kt` — `rememberSubscription()` with `remember(*keys)` + `DisposableEffect` +- `RelayConnectionManager.kt` — `relayStatuses` (Map), `connectedRelays` (Set), `subscribe()` +- `FeedSubscription.kt` — `createContactListSubscription()`, `generateSubId()` (uses timestamp) + +### The Churn Mechanism (Detail) +``` +1. relayStatusMap emits (relay A pings) +2. FeedScreen recomposes +3. configuredRelays = relayStatusMap.keys (new Set instance) +4. remember(*keys) — keys include configuredRelays +5. IF keys changed: config() runs -> generateSubId("contacts-...") -> new subId +6. DisposableEffect sees new subId -> onDispose (close old sub) -> open new sub +7. Relay was responding to old subId -> response dropped +8. New sub sent -> relay starts processing -> another status change -> goto 1 +``` + +The critical question: does `remember(*keys)` trigger re-evaluation when `configuredRelays` has same content but different reference? In Compose, `remember` uses `==` (structural equality). `Set.equals()` compares contents. So **if relay URLs haven't changed, remember should NOT recompute**. + +But `relayStatusMap` itself changes (values change), causing recomposition. During recomposition, `configuredRelays = relayStatusMap.keys` runs. If the underlying Map implementation returns the same key set by reference, `remember` won't recompute. But if it returns a new Set (likely, since we do `.toMutableMap().apply{...}` in `updateRelayStatus`), then... `Set.equals()` should still return true. + +**This needs empirical verification** — add logging in `rememberSubscription` to confirm whether subscriptions are actually being recreated. + +## Candidate Solutions + +### Approach A: Stabilize configuredRelays Key + +Keep using `configuredRelays` to drive subscriptions (solves chicken-and-egg), but prevent churn by stabilizing the key: + +```kotlin +// Derive relay URLs once, only update when URLs actually change +val configuredRelayUrls by remember { + derivedStateOf { + relayManager.relayStatuses.value.keys + } +} +``` + +Or use `distinctUntilChanged()` on the Flow before collecting: + +```kotlin +val configuredRelays by relayManager.relayStatuses + .map { it.keys } + .distinctUntilChanged() + .collectAsState(emptySet()) +``` + +**Pro:** Subscriptions drive relay connections AND keys are stable. +**Con:** Subscriptions still target unconnected relays (but openReqSubscription handles this). + +### Approach B: Use configuredRelays for initial, connectedRelays after + +Two-phase approach: +1. First subscription uses `configuredRelays` to trigger relay connections +2. Once relays connect, switch to `connectedRelays` for stability + +```kotlin +val relaySet = if (connectedRelays.isEmpty()) configuredRelays else connectedRelays +``` + +**Pro:** Gets the best of both worlds. +**Con:** Subscription recreates once during the transition. More complex logic. + +### Approach C: Separate relay connection from subscriptions + +Add relay registration to `addDefaultRelays()` so `connect()` actually works: + +```kotlin +fun addRelay(url: String): NormalizedRelayUrl? { + val normalized = RelayUrlNormalizer.normalizeOrNull(url) ?: return null + updateRelayStatus(normalized) { it.copy(connected = false) } + _client.registerRelay(normalized) // NEW: register with NostrClient + return normalized +} +``` + +Then FeedScreen can safely use `connectedRelays` because relays will connect via `connect()` independently of subscriptions. + +**Pro:** Clean separation. `connectedRelays` works correctly everywhere. +**Con:** Requires changes to NostrClient API (may not have `registerRelay`). Bigger change. + +### Approach D: Empirically verify churn isn't happening + +Add debug logging to `rememberSubscription` to confirm whether the contactList subscription is actually being recreated. If `Set.equals()` works correctly and `remember` doesn't recompute, then `configuredRelays` approach works fine and the follows issue has a different root cause. + +```kotlin +@Composable +fun rememberSubscription(vararg keys: Any?, ...) { + val subscription = remember(*keys) { + DebugConfig.log("SUB RECOMPUTE keys=${keys.contentToString()}") + config() + } + // ... +} +``` + +**Pro:** Cheapest path. May reveal the actual bug. +**Con:** Doesn't fix anything by itself. + +## Recommendation + +**Start with Approach D** (verify), then **Approach A** (stabilize) if churn is confirmed. + +Approach A with `distinctUntilChanged()` is the cleanest fix — it preserves the property that subscriptions drive relay connections while eliminating any possible churn from status map updates. + +## Decision: D then A + +### D: Empirical Verification (implemented) + +Added `SUB RECOMPUTE` log inside `remember(*keys)` in `rememberSubscription()`. Fires only when Compose re-evaluates the config lambda (i.e., keys actually changed). Compare frequency against existing `SUB OPEN`/`SUB CLOSE` logs. + +**File:** `SubscriptionUtils.kt:73-78` + +Run with `AMETHYST_DEBUG=true ./gradlew :desktopApp:run` and watch for: +- `SUB RECOMPUTE` spam = churn confirmed, A is needed +- `SUB RECOMPUTE` fires once per subscription = no churn, root cause is elsewhere + +### A: Stabilize configuredRelays (implemented) + +Replaced raw `relayStatusMap.keys` with a `distinctUntilChanged()` flow: + +```kotlin +val configuredRelays by remember { + relayManager.relayStatuses + .map { it.keys } + .distinctUntilChanged() +}.collectAsState(emptySet()) +``` + +**File:** `FeedScreen.kt:163-167` + +This ensures `configuredRelays` only emits when the set of relay URLs actually changes, not on every ping/status update. `openReqSubscription` still drives relay connections on demand. + +## Open Questions + +1. **If churn isn't the issue, what is?** Could be a timing issue where the contactList REQ arrives at the relay before the relay has the event cached, and no retry mechanism exists. + +2. **Does `openReqSubscription` reliably deliver events for not-yet-connected relays?** It should queue and send REQ after connecting, but need to verify the NostrClient implementation. + +3. **Should we add a retry/resubscribe mechanism?** If the first contactList subscription gets EOSE with no results, should we retry after a delay? + +4. **Is the `since` filter on contactList needed?** ContactList events have no `since` filter currently — they request the latest kind-3 event. But if the relay is slow to index, the response might be empty. diff --git a/docs/plans/2026-03-05-fix-nip46-relay-isolation-and-init-plan.md b/docs/plans/2026-03-05-fix-nip46-relay-isolation-and-init-plan.md new file mode 100644 index 000000000..7a426d820 --- /dev/null +++ b/docs/plans/2026-03-05-fix-nip46-relay-isolation-and-init-plan.md @@ -0,0 +1,365 @@ +--- +title: "fix: NIP-46 relay isolation, pool init, and stale response bugs" +type: fix +status: active +date: 2026-03-05 +deepened: 2026-03-05 +--- + +# fix: NIP-46 relay isolation, pool init, and stale response bugs + +## Enhancement Summary + +**Deepened on:** 2026-03-05 +**Review agents used:** Kotlin expert, Kotlin coroutines, Compose expert, Nostr protocol, Architecture, Code simplicity, Performance + +### Key Improvements from Deepening +1. **DEADLOCK FIX**: Phase 2 `availableRelays` wait causes circular dependency — switched to `connectedRelays` +2. **Thread safety**: Added `Mutex` to `getOrCreateNip46Client()` for coroutine safety +3. **Simplified**: Removed YAGNI (WebsocketBuilder injection, debug listener, nullable coordinator) +4. **`since` filter adjusted**: Use 60s buffer (`TimeUtils.now() - 60`) to handle clock skew +5. **Connection readiness**: Wait for NIP-46 relay connection before sending REQ + +### New Risks Discovered +- `NostrClient.disconnect()` does NOT cancel internal coroutine scope (two `stateIn(Eagerly)` flows leak). Fix: call `scope.cancel()` in `disconnectNip46Client()` — see Step 3 +- `NostrClient.connect()` is non-blocking — `openSubscription()` may race. Mitigated by relay auto-queue in `PoolRequests` +- `connectedRelays` flow fires when general relays connect, breaking the circular dependency that `availableRelays` had + +## Overview + +Three bugs in the NIP-46 remote signer implementation on desktop prevent account-specific content from loading, break session restoration, and cause sign requests to never reach Amber. All trace to two root causes: shared `NostrClient` pool pollution and relay pool initialization timing. + +## Problem Statement + +### Bug 1: No account-specific content after nostrconnect login +After successful NIP-46 login, home feed shows global notes but no followed users, profile data, or DMs. `DesktopRelaySubscriptionsCoordinator.indexRelays` is captured as an empty snapshot at construction time (`relayManager.availableRelays.value` in `Main.kt:411`), so the metadata coordinator never fetches profiles/avatars. + +### Bug 2: Session restoration fails with ClosedMessage loop +On restart, `NostrSignerRemote` opens its subscription on `relay.nsec.app` via the shared `NostrClient`. This adds `relay.nsec.app` to the general relay pool. All other subscriptions (DMs, metadata, feeds) get sent there too. `relay.nsec.app` rejects non-NIP-46 subs with `ClosedMessage`. `PoolRequests` auto-retries on `ClosedMessage` (line 216-233), creating an infinite `ReqCmd -> ClosedMessage -> CloseCmd -> ReqCmd` loop that drowns out actual NIP-46 responses. + +### Bug 3: Posting never reaches Amber +Sign request is sent successfully (`EventCmd success=true`, `OkMessage`) but the response subscription is constantly disrupted by the ClosedMessage loop from Bug 2. Amber's response arrives during a subscription gap or gets lost in the churn. + +## Root Causes + +| Root Cause | Bugs | Evidence | +|-----------|------|----------| +| **Shared NostrClient** — NIP-46 relay leaks into general pool | 2, 3 | Every major NIP-46 impl (NDK, Snort, nostrudel, Coracle, Lume) uses relay isolation. Our shared client is unique and broken. | +| **`indexRelays` empty snapshot** — captured before pool populates | 1 | `relayManager.availableRelays.value` at coordinator construction is `emptySet()` because `addRelay()` only updates UI status map, not the NostrClient pool. Relays enter the pool lazily via `openReqSubscription`. | + +## Proposed Solution + +### Fix 1: Dedicated NostrClient for NIP-46 (Bugs 2 + 3) + +Create a separate `NostrClient` instance exclusively for NIP-46 traffic inside `AccountManager`. Pass it to `NostrSignerRemote` instead of the shared general client. + +**Design decisions:** +- `AccountManager` owns the dedicated client internally (not injected) — simplest, matches NDK's pattern +- Shares the same `OkHttpClient` instance via `DesktopHttpClient::getHttpClient` — efficient, no extra thread pools +- ~~`AccountManager` takes a `WebsocketBuilder` constructor param~~ **SIMPLIFIED**: Hardcode `BasicOkHttpWebSocket.Builder(DesktopHttpClient::getHttpClient)` inside `getOrCreateNip46Client()` — no need to inject what never varies +- Dedicated client is disconnected on `logout()` + +### Fix 2: Defer subscriptionsCoordinator creation until relays connected (Bug 1) + +Create `DesktopRelaySubscriptionsCoordinator` inside `LaunchedEffect` after `connectedRelays` is non-empty. Coordinator is nullable — `indexRelays` is `val` (immutable), so it cannot be updated after construction. + +> **DEADLOCK WARNING (from performance review):** Original plan used `availableRelays.first { it.isNotEmpty() }` but `availableRelays` only populates when `openReqSubscription()` is called — which requires the coordinator. Circular dependency. Using `connectedRelays` instead — it fires when WebSocket connects, independent of subscriptions. + +### Fix 3: Add `since` filter to NIP-46 subscription + +Pass `since = TimeUtils.now() - 60` in the `Filter` constructor inside `NostrSignerRemote`. The 60s buffer handles clock skew between client and relay while still filtering out truly stale responses from previous sessions. + +## Technical Approach + +### Phase 1: Dedicated NIP-46 NostrClient + +#### Step 1: Update AccountManager — add dedicated client + +**File:** `desktopApp/src/jvmMain/.../desktop/account/AccountManager.kt` + +No constructor changes needed (YAGNI — WebsocketBuilder injection removed). + +```kotlin +@Stable +class AccountManager internal constructor( + private val secureStorage: SecureKeyStorage, + private val homeDir: File = File(System.getProperty("user.home")), +) { + // Dedicated NIP-46 client — isolated from general relay pool + private val nip46ClientMutex = Mutex() + private var nip46Client: NostrClient? = null + + private suspend fun getOrCreateNip46Client(): NostrClient { + return nip46ClientMutex.withLock { + nip46Client ?: NostrClient( + BasicOkHttpWebSocket.Builder(DesktopHttpClient::getHttpClient) + ).also { + nip46Client = it + it.connect() + } + } + } + + private fun disconnectNip46Client() { + nip46Client?.disconnect() + // NostrClient.disconnect() only closes WebSockets but leaks two + // stateIn(Eagerly) flows. Cancel scope to prevent coroutine leak. + // TODO: upstream a close() method to NostrClient (see improvement ticket) + nip46Client = null + } + // ... +} +``` + +> **Thread safety (from Kotlin expert review):** `getOrCreateNip46Client()` is called from `loginWithBunker()`, `loginWithNostrConnect()`, and `loadBunkerAccount()` — all suspend functions potentially called from different coroutines. `Mutex` prevents double-creation races. + +> **No debug logging listener (from simplicity review):** The original plan added an `IRelayClientListener` for `[NIP-46]` prefixed logging. Removed — `println` debugging should be added ad-hoc, not baked in. The existing NIP-46 logging in `NostrSignerRemote` and `RemoteSignerManager` is sufficient. + +#### Step 2: Update login methods to use dedicated client + +**File:** `desktopApp/src/jvmMain/.../desktop/account/AccountManager.kt` + +Remove `client: INostrClient` parameter from all NIP-46 methods. Use `getOrCreateNip46Client()` internally. + +```kotlin +// BEFORE +suspend fun loginWithBunker(bunkerUri: String, client: INostrClient): Result + +// AFTER +suspend fun loginWithBunker(bunkerUri: String): Result +``` + +Same for: +- `loginWithNostrConnect(onUriGenerated: (String) -> Unit)` — remove `client` param +- `loadSavedAccount()` — remove `client` param +- `loadBunkerAccount(bunkerUri, npub)` — remove `client` param +- `waitForConnectRequest(...)` — remove `client` param +- `sendAckResponse(...)` — remove `client` param + +#### Step 3: Add NIP-46 client cleanup to logout + +**File:** `desktopApp/src/jvmMain/.../desktop/account/AccountManager.kt` + +```kotlin +suspend fun logout(deleteKey: Boolean = false) { + val current = currentAccount() + if (current != null) { + if (current.signerType is SignerType.Remote) { + (current.signer as? NostrSignerRemote)?.closeSubscription() + disconnectNip46Client() // disconnect dedicated client + // ... existing key cleanup + } + // ... existing logout logic + } + // ... +} +``` + +#### Step 4: Update Main.kt wiring + +**File:** `desktopApp/src/jvmMain/.../desktop/Main.kt` + +Remove `relayManager.client` from all AccountManager calls: + +```kotlin +// BEFORE (line 434) +val result = accountManager.loadSavedAccount(relayManager.client) + +// AFTER +val result = accountManager.loadSavedAccount() +``` + +```kotlin +// BEFORE (line 423-441) — bunker restore flow +if (accountManager.hasBunkerAccount()) { + accountManager.setConnectingRelays() + val connected = withTimeoutOrNull(30_000L) { + relayManager.connectedRelays.first { it.isNotEmpty() } + } + if (connected == null) { + accountManager.logout(deleteKey = true) + } else { + val result = accountManager.loadSavedAccount(relayManager.client) + // ... + } +} + +// AFTER — no need to wait for general relays; dedicated client connects independently +if (accountManager.hasBunkerAccount()) { + accountManager.setConnectingRelays() + val result = accountManager.loadSavedAccount() + if (result.isSuccess) { + accountManager.startHeartbeat(scope) + } else { + accountManager.logout(deleteKey = true) + } +} +``` + +Update `LoginScreen` call — remove `relayClient` param: + +```kotlin +// BEFORE (line 466) +LoginScreen(accountManager = accountManager, relayClient = relayManager.client, ...) + +// AFTER +LoginScreen(accountManager = accountManager, ...) +``` + +#### Step 5: Update LoginScreen and LoginCard + +**File:** `desktopApp/src/jvmMain/.../desktop/ui/LoginScreen.kt` +**File:** `desktopApp/src/jvmMain/.../desktop/ui/auth/LoginCard.kt` + +Remove `relayClient: INostrClient` parameter from both composables. The `AccountManager` now handles NIP-46 client internally. + +#### Step 6: Clean up RelayConnectionManager NIP-46 logging + +**File:** `desktopApp/src/jvmMain/.../desktop/network/RelayConnectionManager.kt` + +Remove all `nsec.app`-specific logging from relay listener callbacks (lines 180-231). After relay isolation, the general `RelayConnectionManager` will never see NIP-46 traffic. Simplify each callback to just the `updateRelayStatus()` call. + +### Phase 2: Fix Relay Pool Initialization + +#### Step 7: Defer subscriptionsCoordinator creation + +**File:** `desktopApp/src/jvmMain/.../desktop/Main.kt` + +> **RESOLVED**: `indexRelays` is `private val` (immutable). Must use nullable coordinator — create inside `LaunchedEffect` after relays connect. + +```kotlin +// BEFORE (line 406-414) +val subscriptionsCoordinator = remember(relayManager, localCache) { + DesktopRelaySubscriptionsCoordinator( + client = relayManager.client, + scope = scope, + indexRelays = relayManager.availableRelays.value, // empty! + localCache = localCache, + ) +} + +// AFTER — nullable, created after relays connect +var subscriptionsCoordinator by remember { mutableStateOf(null) } + +LaunchedEffect(relayManager, localCache) { + // Wait for connected (NOT availableRelays — that deadlocks, see below) + relayManager.connectedRelays.first { it.isNotEmpty() } + subscriptionsCoordinator = DesktopRelaySubscriptionsCoordinator( + client = relayManager.client, + scope = scope, + indexRelays = relayManager.connectedRelays.value, // now populated + localCache = localCache, + ).also { it.start() } +} + +DisposableEffect(Unit) { + onDispose { + subscriptionsCoordinator?.clear() + } +} +``` + +> **Deadlock explanation:** `availableRelays` derives from `NostrClient.allRelays` which combines `activeRequests.desiredRelays + activeCounts.relays + eventOutbox.relays`. These only populate when `openReqSubscription()` is called — which requires the coordinator. Circular dependency. `connectedRelays` fires when WebSocket connects, independent of subscriptions. + +Guard all usages with `?.`: +- `subscriptionsCoordinator?.clear()` in onDispose +- `subscriptionsCoordinator?.subscribeToDms(...)` — use `LaunchedEffect(subscriptionsCoordinator)` that fires when coordinator becomes available +- Remove existing `subscriptionsCoordinator.start()` call (handled in LaunchedEffect) + +### Phase 3: Add `since` Filter + +#### Step 8: Add `since` to NostrSignerRemote subscription + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/signer/NostrSignerRemote.kt` + +```kotlin +// BEFORE (line 69-87) +val subscription = client.req( + relays = relays.toList(), + filter = Filter( + kinds = listOf(NostrConnectEvent.KIND), + tags = mapOf("p" to listOf(signer.pubKey)), + ), +) { event -> ... } + +// AFTER +val subscription = client.req( + relays = relays.toList(), + filter = Filter( + kinds = listOf(NostrConnectEvent.KIND), + tags = mapOf("p" to listOf(signer.pubKey)), + since = TimeUtils.now() - 60, // 60s buffer for clock skew + ), +) { event -> ... } +``` + +> **Clock skew buffer (from Nostr protocol review):** Using `TimeUtils.now()` exactly risks missing responses if there's any clock difference between client and relay, or if the signer response `created_at` is slightly in the past. A 60s buffer is safe — request ID matching in `RemoteSignerManager.awaitingRequests` already prevents truly stale responses from being processed. The `since` filter just reduces relay-side work. + +> **Tradeoff:** Responses older than 60s before subscription are filtered. Acceptable — `RemoteSignerManager` has 30s timeout, so responses older than 30s are useless anyway. + +## Files Changed + +| File | Change | Phase | +|------|--------|-------| +| `desktopApp/.../account/AccountManager.kt` | Add dedicated `NostrClient` with `Mutex`, remove `client` params from login methods, cleanup on logout | 1 | +| `desktopApp/.../Main.kt` | Remove `relayManager.client` from AccountManager calls, defer coordinator start until `connectedRelays` non-empty | 1, 2 | +| `desktopApp/.../ui/LoginScreen.kt` | Remove `relayClient` param | 1 | +| `desktopApp/.../ui/auth/LoginCard.kt` | Remove `relayClient` param, use AccountManager directly | 1 | +| `desktopApp/.../network/RelayConnectionManager.kt` | Remove NIP-46 `nsec.app` debug logging from all callbacks | 1 | +| `quartz/.../nip46RemoteSigner/signer/NostrSignerRemote.kt` | Add `since = TimeUtils.now() - 60` to subscription filter | 3 | + +## Acceptance Criteria + +- [ ] After nostrconnect login, home feed shows followed users' notes with avatars and display names +- [ ] After nostrconnect login, DMs load (NIP-17 decryption works via remote signer) +- [ ] On app restart with saved bunker account, session restores without ClosedMessage loop +- [ ] `relay.nsec.app` does NOT appear in general relay pool (only in dedicated NIP-46 client) +- [ ] Posting a note via remote signer succeeds — event signed and broadcast to general relays +- [ ] Heartbeat ping works after session restore +- [ ] Logout disconnects the dedicated NIP-46 client (no leaked WebSocket) +- [ ] No `[NIP-46]` ClosedMessage spam in logs +- [ ] No deadlock on startup — coordinator starts after relays connect + +## Edge Cases + +| Edge Case | Handling | +|-----------|----------| +| Dedicated NIP-46 client can't connect to relay.nsec.app | `loadSavedAccount()` returns failure, falls back to login screen | +| Ephemeral key lost from secure storage | `loadBunkerAccount()` returns `Result.failure("Ephemeral key not found")`, logout + login screen | +| In-flight sign request during brief NIP-46 disconnect | 30s timeout in `RemoteSignerManager`, user retries | +| User logs out with pending sign request | `closeSubscription()` cancels scope, `disconnectNip46Client()` cleans up | +| General relay pool emits before NIP-46 client ready | Independent — general pool handles feeds, NIP-46 client handles signing | +| Concurrent `getOrCreateNip46Client()` calls | `Mutex` ensures single creation | +| `connectedRelays` empty for extended period | Coordinator start is deferred, UI shows loading state until relays connect | +| Clock skew between client and signer relay | 60s `since` buffer absorbs typical skew | + +## Testing Strategy + +1. **Manual: Fresh nostrconnect login** — scan QR with Amber, verify home feed + profile + DMs load +2. **Manual: Session restore** — close and reopen app, verify auto-login without ClosedMessage loop +3. **Manual: Post a note** — compose + send, verify event reaches Amber and gets signed +4. **Manual: Logout + re-login** — verify no leaked connections, clean state +5. **Verify relay isolation** — check logs: `relay.nsec.app` should only appear in NIP-46 context, never in general relay status +6. **Manual: Startup timing** — verify coordinator starts after relays connect (no deadlock) +7. **Manual: Rapid login/logout** — verify `Mutex` prevents double NIP-46 client creation + +## Resolved Questions + +| # | Question | Resolution | +|---|----------|------------| +| 1 | `auth_url` handling? | **Deferred.** Spec defines it, Amber doesn't use it, quartz doesn't handle it. Only nsec.app needs it. See improvement ticket. | +| 2 | `get_public_key` after connect? | **Deferred.** Already fully implemented in quartz (`NostrSignerRemote.getPublicKey()`). But `connect()` already returns pubkey — redundant for correctness. Nice-to-have security hardening. See improvement ticket. | +| 3 | Update `since` on reconnect? | **Deferred.** `NostrClientStaticReq` re-sends exact same filter on reconnect. `FiltersChanged` intentionally ignores `since` changes. Static `since` with 60s buffer sufficient — request ID matching prevents stale response processing. See improvement ticket. | +| 4 | `indexRelays` mutable? | **Resolved: `val` (immutable).** No `updateIndexRelays()` method. Must use nullable coordinator pattern — create inside `LaunchedEffect`. Phase 2 updated. | +| 5 | `NostrClient.close()` for scope? | **Partially addressed.** `disconnect()` leaks two `stateIn(Eagerly)` flows + SupervisorJob scope. `disconnectNip46Client()` handles cleanup for our dedicated client. Upstream `close()` method deferred. See improvement ticket. | + +**Deferred items tracked in:** `docs/plans/2026-03-05-nip46-improvements-plan.md` + +## Sources + +- **NIP-46 spec:** https://github.com/nostr-protocol/nips/blob/master/46.md +- **NDK NIP-46 impl:** `core/src/signers/nip46/rpc.ts` — dedicated `NDKPool` for RPC +- **Snort NIP-46 impl:** `packages/system/src/impl/nip46.ts` — dedicated `Connection` object +- **nostrudel NIP-46 impl:** `applesauce-signers` — separate publish/subscribe methods +- **Existing plan:** `docs/plans/2026-03-05-nip46-bunker-login-deepened.md` — covers initial implementation +- **greenart7c3 commits:** `df1bf82e7..a6f3e09a6` — original quartz NIP-46 skeleton (never used by Android) diff --git a/docs/plans/2026-03-05-nip46-bunker-login-deepened.md b/docs/plans/2026-03-05-nip46-bunker-login-deepened.md new file mode 100644 index 000000000..97cda5093 --- /dev/null +++ b/docs/plans/2026-03-05-nip46-bunker-login-deepened.md @@ -0,0 +1,876 @@ +# NIP-46 Remote Signer (bunker:// + nostrconnect://) Login for Desktop — Deepened Plan + +## Enhancement Summary + +**Deepened on:** 2026-03-05 (2 rounds) +**Sections enhanced:** 10 (Steps 0-8 + security) +**Research agents used:** quartz NIP-46 audit, desktop account/main audit, NIP-46 spec research, coroutines heartbeat patterns, nostrconnect + QR + animation research, quartz nostrconnect gap audit, commons gap audit + +### Key Improvements from Research +1. **`switch_relays` support** — NIP-46 spec says "compliant clients should send `switch_relays` immediately upon establishing a connection". Plan now includes this. +2. **Auth URL flow** — spec allows remote signer to return `auth_url` for user confirmation. Plan now handles this edge case. +3. **Secret validation** — spec says "client MUST validate the secret returned by connect response". Plan updated with explicit validation. +4. **No re-connect on restart** — confirmed: saved ephemeral key is already trusted by remote signer, skip `connect()` on restart. +5. **PoW relay concern** — some relays (nostr.mom, nos.lol) require Proof of Work for kind 24133. Plan notes this. +6. **`decryptZapEvent()` / `deriveKey()` NOT implemented** in quartz NostrSignerRemote — these will throw. Document as known limitation. +7. **Step 0 blocking tasks** — 8 prerequisite tasks (5 quartz, 3 commons) identified. bunker:// needs zero quartz changes; nostrconnect needs 4 blockers resolved. +8. **QrCodeDrawer portable** — existing Android composable uses zero Android APIs, 90% extract-as-is to commons. +9. **SecureKeyStorage + IAccount** — both 100% compatible with NostrSignerRemote, verified by audit. +10. **Phased implementation** — bunker:// first (no shared lib changes), then quartz+commons prereqs, then nostrconnect+heartbeat. + +### New Considerations Discovered +- `NostrSigner.pubKey` = ephemeral key in NostrSignerRemote. DesktopIAccount uses `accountState.pubKeyHex` (user's real key) — **already correct**, no collision. +- `RemoteSignerManager.timeout` is configurable via constructor (default 30s) — use 30s for connect, could use shorter for ping. +- Bunker URI `secret=` is **optional** for remote-signer-initiated flow (our case). Plan should NOT require it. +- Some users may paste bunker URI over insecure channels — the URI is essentially a session token. Document this in security notes. + +--- + +## Context + +Desktop app only supports nsec/npub login. Quartz has a **complete, verified** NIP-46 client (`NostrSignerRemote`) that's never been wired into any app. Adding bunker:// login gives desktop the best security story — private key never touches the machine. + +### Quartz NIP-46 Completeness Audit (Verified) + +| Feature | Status | Location | +|---------|--------|----------| +| `connect()` handshake | ✅ | `NostrSignerRemote.connect()` | +| `sign_event` | ✅ | `NostrSignerRemote.sign()` | +| `ping` | ✅ | `NostrSignerRemote.ping()` | +| `get_public_key` | ✅ | `NostrSignerRemote.getPublicKey()` | +| NIP-04 encrypt/decrypt | ✅ | `nip04Encrypt()` / `nip04Decrypt()` | +| NIP-44 encrypt/decrypt | ✅ | `nip44Encrypt()` / `nip44Decrypt()` | +| `switch_relays` | ✅ | `BunkerRequestGetRelays` | +| Bunker URI parsing | ✅ | `fromBunkerUri()` — handles relay + secret params | +| Timeout handling | ✅ | 30s default, configurable in `RemoteSignerManager` | +| Subscription mgmt | ✅ | `openSubscription()` / `closeSubscription()` | +| `decryptZapEvent()` | ❌ TODO | Will throw — known limitation | +| `deriveKey()` | ❌ TODO | Will throw — known limitation | + +### Desktop Account System Audit + +| Component | Current State | Action | +|-----------|--------------|--------| +| `AccountState` sealed class | `LoggedOut` / `LoggedIn` | Add `ConnectingRelays` variant | +| `AccountManager.loginWithKey()` | Handles nsec/npub/hex | Add `loginWithBunker()` | +| `SecureKeyStorage` | OS keyring + encrypted fallback | Reuse for ephemeral key | +| `~/.amethyst/last_account.txt` | Stores npub | Reuse as-is | +| `Main.kt` DisposableEffect | Loads account + starts relays | Add relay-wait + heartbeat | +| `LoginCard` | Text input + Login/Generate buttons | Add bunker detection + connecting state | +| `RelayConnectionManager.client` | Exposes `INostrClient` | Pass to `loginWithBunker()` | + +## Protocol Flow + +``` +User pastes bunker URI → validate format → generate ephemeral keypair +→ create NostrSignerRemote via fromBunkerUri() → call connect() over relays +→ remote signer (nsec.app/Amber) approves → validate secret if present +→ receive user pubkey → call switch_relays → update relay set if changed +→ AccountState.LoggedIn with NostrSignerRemote as signer +→ all sign/encrypt ops proxy transparently through remote signer +→ periodic ping heartbeat monitors connection health +``` + +### Research Insight: NIP-46 Spec Compliance +- **Secret validation**: If bunker URI contains `secret=`, client MUST validate the secret returned by `connect` response matches. If no secret in URI, skip validation. +- **`switch_relays`**: "Compliant clients should send a `switch_relays` request immediately upon establishing a connection." Remote signer replies with updated relay list or null. +- **Auth URL**: Remote signer may respond with `{"result": "auth_url", "error": ""}` — client should display URL to user for confirmation (e.g., nsec.app web confirmation). + +## Step 0: Prerequisite Tasks (Blocking) + +These must be completed before or alongside the desktop feature work. They strengthen quartz and commons as shared libraries. + +### 0A. [QUARTZ/BLOCKER] Add `fromNostrConnectUri()` factory to NostrSignerRemote + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/signer/NostrSignerRemote.kt` + +**Problem:** `fromBunkerUri()` exists but no equivalent for nostrconnect://. Client-initiated flow needs the client to generate the URI, not parse one from a remote signer. + +**Action:** Add companion factory that builds a `NostrSignerRemote` ready to *wait* for a connect response: +```kotlin +companion object { + fun forNostrConnect( + ephemeralSigner: NostrSignerInternal, + relays: Set, + client: INostrClient, + secret: String, + ): Pair { + // remotePubkey unknown until signer responds — see 0B + // Build URI: nostrconnect://?relay=...&secret=...&name=Amethyst+Desktop + // Return (signer, uriString) + } +} +``` + +**Blocks:** Step 7 (nostrconnect login) + +--- + +### 0B. [QUARTZ/BLOCKER] Make `remotePubkey` mutable or support deferred assignment + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/signer/NostrSignerRemote.kt` + +**Problem:** `val remotePubkey: HexKey` is immutable in constructor. For nostrconnect://, the remote signer's pubkey is unknown until the connect response arrives. Cannot instantiate `NostrSignerRemote` without it. + +**Options:** +1. **`var remotePubkey`** — simplest, set after connect response. Risk: state mutation on a shared object. +2. **Two-phase construction** — `NostrConnectSession` awaits connect, then creates `NostrSignerRemote` with known pubkey. Cleaner but more code. +3. **DesktopApp workaround** — state machine in desktopApp: subscribe to kind 24133 manually, wait for connect response, extract pubkey, *then* construct `NostrSignerRemote`. Avoids quartz changes but duplicates logic. + +**Recommended:** Option 2 — keep `NostrSignerRemote` immutable, add a `NostrConnectSession` helper that handles the wait-then-construct flow. + +**Blocks:** Step 7 (nostrconnect login), 0A, 0C + +--- + +### 0C. [QUARTZ/BLOCKER] NostrConnectEvent.create() requires remotePubkey + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/NostrConnectEvent.kt` + +**Problem:** `NostrConnectEvent.create()` calls `nip44Encrypt(content, remoteKey)`. For nostrconnect, the initial subscription doesn't need to *send* an event — client just listens. But `openSubscription()` in `NostrSignerRemote` sends a subscription filter, which doesn't require remotePubkey. The encryption issue only matters if client needs to send requests before learning remotePubkey. + +**Resolution:** This is resolved by 0B — if we use two-phase construction, `NostrSignerRemote` is only created after remotePubkey is known, so `create()` always has it. No changes to `NostrConnectEvent.kt` needed. + +**Blocks:** Step 7 (nostrconnect login). Resolved by 0B. + +--- + +### 0D. [QUARTZ/SECURITY] Add secret validation to PubKeyResponse + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/signer/PubKeyResponse.kt` + +**Problem:** NIP-46 spec says "client MUST validate the secret returned by connect response matches the one sent". Current `PubKeyResponse` parses the pubkey from the result field but does NOT validate the secret. + +**Action:** Add `secret` field to `PubKeyResponse` and validate in `connect()`: +```kotlin +data class PubKeyResponse(val pubkey: HexKey, val secret: String?) { + companion object { + fun parse(result: String): PubKeyResponse { + // result may be just pubkey, or pubkey + secret separated by space + // validate hex format + } + } +} +``` + +Then in `NostrSignerRemote.connect()`, compare returned secret with expected: +```kotlin +if (expectedSecret != null && response.secret != expectedSecret) { + throw SignerExceptions.ManuallyUnauthorizedException("Secret mismatch — possible MITM") +} +``` + +**Priority:** Security hardening. Not strictly blocking for bunker:// (secret is optional), but **blocking for nostrconnect://** (secret is required). + +**Blocks:** Step 2 (bunker login — optional validation), Step 7 (nostrconnect — required validation) + +--- + +### 0E. [QUARTZ/NICE-TO-HAVE] Add `getRelays()` method to NostrSignerRemote + +**File:** `quartz/src/commonMain/.../nip46RemoteSigner/signer/NostrSignerRemote.kt` + +**Problem:** `BunkerRequestGetRelays` exists but `NostrSignerRemote` has no `getRelays()` or `switchRelays()` high-level method. The plan calls for `switch_relays` compliance per NIP-46 spec. + +**Action:** Add method: +```kotlin +suspend fun switchRelays(): List? { + // Send BunkerRequestGetRelays, parse relay list response + // Return updated relay list or null if signer doesn't support it +} +``` + +**Priority:** Nice-to-have. Can defer to a follow-up PR if needed. The `switch_relays` spec language is "compliant clients *should*" (not MUST). + +**Does NOT block:** Any step. Can be added later. + +--- + +### 0F. [COMMONS/BLOCKER] Extract QrCodeDrawer to commons + +**Files:** +- **Source:** `amethyst/src/main/java/.../ui/screen/loggedIn/qrcode/QrCodeDrawer.kt` +- **Target:** `commons/src/commonMain/.../commons/ui/qrcode/QrCodeDrawer.kt` + +**Problem:** QR code display needed for nostrconnect:// tab. Existing `QrCodeDrawer` in amethyst is pure Compose Canvas + ZXing core — **zero Android dependencies**. 90% extract-as-is. + +**Gap:** References `QuoteBorder` from amethyst theme (just `RoundedCornerShape(15.dp)`). Replace with inline shape. + +**Action:** +1. Copy `QrCodeDrawer.kt` to `commons/src/commonMain/.../commons/ui/qrcode/` +2. Replace `QuoteBorder` reference with `RoundedCornerShape(15.dp)` +3. Add typealias in amethyst to maintain backward compat +4. Add `libs.zxing` dependency to `commons/build.gradle.kts` (see 0G) + +**Blocks:** Step 7 (nostrconnect QR display) + +--- + +### 0G. [COMMONS/BLOCKER] Add ZXing core dependency to commons module + +**File:** `commons/build.gradle.kts` + +**Problem:** ZXing core 3.5.4 is in `gradle/libs.versions.toml` but NOT imported in `commons/build.gradle.kts`. QrCodeDrawer needs it. + +**Action:** Add to `commonMain` dependencies (ZXing core is pure Java, works on JVM + Android): +```kotlin +commonMain.dependencies { + implementation(libs.zxing) // already in version catalog +} +``` + +**Note:** `zxing-embedded` (Android camera scanner wrapper) stays in amethyst only. Only `zxing:core` (encoder/decoder) goes to commons. + +**Blocks:** 0F (QrCodeDrawer extraction), Step 7 + +--- + +### 0H. [COMMONS/NEW] Create HeartbeatIndicator composable + +**File:** `commons/src/commonMain/.../commons/ui/components/HeartbeatIndicator.kt` (new) + +**Problem:** No animated pulsing dot composable exists in commons. Needed for sidebar signer connection health indicator. + +**Action:** Create shared composable: +- Input: `SignerConnectionState` sealed class (Connected/Unstable/Disconnected/NotRemote) +- Output: Pulsing dot (green/yellow/red) with `rememberInfiniteTransition` double-beat animation +- Desktop: wrapped in `TooltipArea` in DeckSidebar (desktop-only API) +- The composable itself is multiplatform; tooltip wrapping is platform-specific + +**Dependencies:** None — pure Compose animation APIs. + +**Blocks:** Step 8 (heartbeat UI) + +--- + +### Step 0 Summary + +| ID | Module | Type | Priority | Blocks | +|----|--------|------|----------|--------| +| 0A | quartz | `fromNostrConnectUri()` factory | 🔴 Blocker | Step 7 | +| 0B | quartz | Mutable/deferred `remotePubkey` | 🔴 Blocker | Step 7, 0A, 0C | +| 0C | quartz | NostrConnectEvent encryption | 🟢 Resolved by 0B | Step 7 | +| 0D | quartz | Secret validation in PubKeyResponse | 🟡 Security | Step 7 (required), Step 2 (optional) | +| 0E | quartz | `switchRelays()` method | ⚪ Nice-to-have | None | +| 0F | commons | Extract QrCodeDrawer | 🔴 Blocker | Step 7 | +| 0G | commons | ZXing dep in commons gradle | 🔴 Blocker | 0F, Step 7 | +| 0H | commons | HeartbeatIndicator composable | 🟡 Medium | Step 8 | + +**Critical path for bunker:// (Steps 1-6):** 0D (optional hardening) +**Critical path for nostrconnect:// (Step 7):** 0B → 0A → 0D, 0G → 0F +**Critical path for heartbeat UI (Step 8):** 0H + +### Implementation Order + +``` +Phase 1 (bunker:// — no quartz changes needed): + Steps 1 → 2 → 3 → 4 → 5 → 6 + +Phase 2 (quartz + commons prereqs): + 0G → 0F (commons: ZXing + QrCodeDrawer) + 0B → 0A (quartz: deferred remotePubkey + nostrconnect factory) + 0D (quartz: secret validation) + 0H (commons: HeartbeatIndicator) + +Phase 3 (nostrconnect + heartbeat): + Step 7 (depends on 0A, 0B, 0D, 0F, 0G) + Step 8 (depends on 0H) +``` + +**Key insight:** bunker:// login (Steps 1-6) requires ZERO quartz/commons changes — all quartz NIP-46 code works as-is. Ship bunker:// first, then layer nostrconnect + heartbeat. + +--- + +## Implementation + +### Step 1: AccountState + SignerType + +**File:** `desktopApp/src/jvmMain/.../desktop/account/AccountManager.kt` + +Add `SignerType` sealed class: +```kotlin +sealed class SignerType { + data object Internal : SignerType() + data class Remote(val bunkerUri: String) : SignerType() +} +``` + +Extend `AccountState`: +```kotlin +sealed class AccountState { + data object LoggedOut : AccountState() + data object ConnectingRelays : AccountState() + data class LoggedIn( + val signer: NostrSigner, + val pubKeyHex: String, + val npub: String, + val nsec: String?, + val isReadOnly: Boolean, + val signerType: SignerType = SignerType.Internal, + ) : AccountState() +} +``` + +#### Research Insights + +**Existing pattern confirmed:** AccountState is observed via `accountManager.accountState.collectAsState()` in `App()` composable. Adding `ConnectingRelays` variant just needs a new `when` branch — zero refactoring needed. + +**DesktopIAccount compatibility:** Uses `accountState.pubKeyHex` (not `signer.pubKey`), so `NostrSignerRemote` (where `signer.pubKey` = ephemeral key) works transparently. Verified in audit. + +### Step 2: AccountManager — bunker login + persistence + heartbeat + +**File:** `desktopApp/src/jvmMain/.../desktop/account/AccountManager.kt` + +#### `loginWithBunker(bunkerUri: String, client: INostrClient): Result` + +1. Validate URI format: starts with `bunker://`, hex pubkey (64 chars), `?relay=wss://` param +2. Generate ephemeral `KeyPair()` + wrap in `NostrSignerInternal` +3. `NostrSignerRemote.fromBunkerUri(bunkerUri, ephemeralSigner, client)` + - Factory handles relay extraction + secret parsing +4. Call `remoteSigner.openSubscription()` — starts listening for responses +5. Call `remoteSigner.connect()` — sends BunkerRequestConnect, waits 30s +6. **Validate secret** if present: connect response must return matching secret value +7. On success: call `remoteSigner.switchRelays()` (new — NIP-46 compliance) +8. Set `AccountState.LoggedIn(signer=remoteSigner, pubKeyHex=remotePubkey, signerType=Remote(bunkerUri))` + +#### Research Insights: Connect Flow Details + +**From quartz audit — `connect()` internals:** +``` +RemoteSignerManager.launchWaitAndParse(): + 1. Build BunkerRequestConnect(remoteKey, secret?, permissions?) + 2. Create NostrConnectEvent (NIP-44 encrypted to remote signer) + 3. client.send(event, relayList) + 4. Store continuation[requestID] in LargeCache + 5. tryAndWait(30s) — suspendCancellableCoroutine + 6. On response: newResponse() → resume continuation + 7. Parse via PubKeyResponse → SignerResult +``` + +**Exception mapping in NostrSignerRemote.convertExceptions():** +- `Successful` → return result +- `Rejected` → throw `ManuallyUnauthorizedException` +- `TimedOut` → throw `TimedOutException` +- `ReceivedButCouldNotPerform` → throw `CouldNotPerformException` + +**Auth URL edge case:** If remote signer needs web confirmation (nsec.app pattern), it returns `auth_url` response. Quartz doesn't handle this natively — **document as known limitation for v1**. Most bunker signers (Amber, local nsecBunker) don't use auth_url. + +#### `saveBunkerAccount(ephemeralPrivKeyHex: String)` + +Persistence files: +- `~/.amethyst/bunker_uri.txt` — full bunker URI (contains relay URLs + remote pubkey) +- `SecureKeyStorage("bunker_ephemeral")` — ephemeral private key (OS keyring) +- `~/.amethyst/last_account.txt` — user's npub (existing pattern) + +#### Research Insight: Session Persistence + +**Confirmed from NIP-46 community patterns:** +> "Once a connection has been successfully established and the BunkerPointer is stored, you do not need to call `connect()` on subsequent sessions." + +Save: ephemeral privkey + bunker URI + user npub. On restart, recreate `NostrSignerRemote` with saved ephemeral key — remote signer already trusts this keypair. + +**What NOT to save:** Don't store the secret separately — it's only needed for initial connect handshake. The ephemeral keypair IS the session credential. + +#### `loadSavedAccount(client: INostrClient): Result` + +Updated flow: +1. Check `bunker_uri.txt` — if exists, this is a bunker account +2. Load ephemeral privkey from `SecureKeyStorage("bunker_ephemeral")` +3. Load npub from `last_account.txt` → derive pubKeyHex +4. Create `NostrSignerInternal(KeyPair(ephemeralPrivKey))` +5. `NostrSignerRemote.fromBunkerUri(savedUri, ephemeralSigner, client)` +6. `remoteSigner.openSubscription()` — start listening (no connect() needed) +7. Return `LoggedIn(signer=remoteSigner, pubKeyHex=savedPubKeyHex, signerType=Remote)` +8. Fall back to existing internal key flow if no bunker file + +#### `logout(deleteKey: Boolean)` + +Bunker-specific cleanup: +1. `remoteSigner.closeSubscription()` — stop relay filter +2. Stop heartbeat job +3. Delete `bunker_uri.txt` +4. Delete `SecureKeyStorage("bunker_ephemeral")` +5. Clear `last_account.txt` +6. Set `AccountState.LoggedOut` + +#### Ping Heartbeat + +```kotlin +private var heartbeatJob: Job? = null +private var consecutiveFailures = 0 + +fun startHeartbeat(scope: CoroutineScope) { + heartbeatJob = scope.launch { + while (isActive) { + delay(60_000) + val current = currentAccount() ?: continue + val remoteSigner = current.signer as? NostrSignerRemote ?: continue + try { + remoteSigner.ping() + consecutiveFailures = 0 // reset on success + } catch (e: SignerExceptions.TimedOutException) { + consecutiveFailures++ + if (consecutiveFailures >= 3) { + forceLogoutWithReason("Lost connection to remote signer after 3 failed pings.") + } + } catch (e: SignerExceptions.ManuallyUnauthorizedException) { + forceLogoutWithReason("Remote signer revoked access.") + } + } + } +} +``` + +#### Research Insights: Heartbeat Patterns + +**Structured concurrency:** Heartbeat should be a child of the app's `CoroutineScope(SupervisorJob() + Dispatchers.Main)` already created in `App()`. SupervisorJob ensures heartbeat failure doesn't crash the app. + +**Existing pattern in codebase:** `RelayStat.pingInMs` tracks relay ping latency — similar concept. RelayConnectionManager handles relay disconnection with reconnect logic, but no heartbeat loop exists yet. + +**Simple counter vs AtomicInteger:** Since heartbeat runs in a single coroutine (sequential `while` loop), a plain `var consecutiveFailures: Int` is safe — no concurrent access. + +**Window minimization:** Desktop heartbeat should continue even when minimized — the remote signer session is independent of window focus. The `CoroutineScope` created with `remember` persists across recompositions. + +**Force logout state:** Use `MutableStateFlow` for `forceLogoutReason` — it's a stateful value that the UI observes, not a one-shot event. Dialog displays the reason, user dismisses, flow cleared. + +### Step 3: LoginCard — validation + connecting state + +**File:** `desktopApp/src/jvmMain/.../desktop/ui/auth/LoginCard.kt` + +Add `LoginState` enum: `IDLE`, `CONNECTING`, `ERROR` + +Add callback: `onLoginBunker: suspend (String) -> Result` + +**Bunker URI validation** (before attempting connect): +- Starts with `bunker://` +- Contains hex pubkey (64 hex chars after `bunker://`, before `?`) +- Contains at least one `relay=wss://...` param +- Show inline validation error if format wrong, don't attempt connection +- `secret=` is optional — do NOT require it + +**UX when `bunker://` detected:** +- Button text: "Login" → "Connect to Signer" +- On click with valid URI: set `CONNECTING`, show `CircularProgressIndicator` + "Connecting to remote signer..." +- Disable input + all buttons while `CONNECTING` +- On success: caller handles transition +- On failure: show error message, return to `IDLE`, user can retry + +#### Research Insights: UX Patterns + +**From NIP-46 community feedback:** +> "Connection establishment takes a while to show to users" — always show clear progress indication. + +**Pitfall discovered:** Users may not understand that bunker URI is a session credential: +> "Users might think the entire bunker URI is like a 2FA token and transmit it over untrusted messengers, and attackers can reuse the same URI" + +Consider: add subtle help text below input when bunker:// detected: "This URI connects to your remote signer. Treat it like a password." + +**Auto-detection approach:** Current `LoginCard` has a single `KeyInputField`. Detect `bunker://` prefix on input change — no new tabs/screens needed. Same pattern as NWC connection string input in settings. + +### Step 4: LoginScreen + ConnectingRelays screen + +**File:** `desktopApp/src/jvmMain/.../desktop/ui/LoginScreen.kt` + +Add parameter: `relayClient: INostrClient` + +Wire `onLoginBunker` callback → `accountManager.loginWithBunker(bunkerUri, relayClient)` → on success: save + `onLoginSuccess()` + +**ConnectingRelaysScreen composable** (in same file): + +Shown when `AccountState.ConnectingRelays`: +``` +[Amethyst title text] +"Connecting to relays..." +[CircularProgressIndicator] +"Restoring remote signer session" +``` + +Used on app restart when a bunker account is saved — shows while relays connect before recreating `NostrSignerRemote`. + +#### Research Insight: Startup Sequence + +**From Main.kt audit — current startup:** +```kotlin +DisposableEffect(Unit) { + scope.launch(Dispatchers.IO) { accountManager.loadSavedAccount() } + relayManager.addDefaultRelays() + relayManager.connect() + subscriptionsCoordinator.start() + onDispose { ... } +} +``` + +**Problem:** `loadSavedAccount()` for bunker accounts needs `relayManager.client` — but relay connections happen asynchronously. Current code loads account independently of relay state. + +**Solution:** For bunker accounts, wait for relay connection before recreating signer. The pattern `relayManager.connectedRelays.first { it.isNotEmpty() }` already exists in `MainContent` LaunchedEffect for DM subscriptions. + +### Step 5: Main.kt — relay state, pass client, heartbeat + +**File:** `desktopApp/src/jvmMain/.../desktop/Main.kt` + +**Updated startup flow:** +```kotlin +DisposableEffect(Unit) { + relayManager.addDefaultRelays() + relayManager.connect() + subscriptionsCoordinator.start() + + scope.launch(Dispatchers.IO) { + if (accountManager.hasBunkerAccount()) { + accountManager.setConnectingRelays() + // Wait for relay connections — pattern from MainContent DM setup + relayManager.connectedRelays.first { it.isNotEmpty() } + accountManager.loadSavedAccount(relayManager.client) + accountManager.startHeartbeat(scope) + } else { + accountManager.loadSavedAccount(relayManager.client) + } + } + + onDispose { + accountManager.stopHeartbeat() + subscriptionsCoordinator.clear() + relayManager.disconnect() + } +} +``` + +**Account state routing — add ConnectingRelays + ForceLogout:** +```kotlin +when (accountState) { + is AccountState.LoggedOut -> LoginScreen(accountManager, relayManager.client, onLoginSuccess) + is AccountState.ConnectingRelays -> ConnectingRelaysScreen() + is AccountState.LoggedIn -> MainContent(...) +} + +// Force logout overlay +val forceLogoutReason by accountManager.forceLogoutReason.collectAsState() +forceLogoutReason?.let { reason -> + ForceLogoutDialog(reason = reason, onDismiss = { accountManager.clearForceLogoutReason() }) +} +``` + +#### Research Insight: Scope Lifecycle + +**Current app scope:** `remember { CoroutineScope(SupervisorJob() + Dispatchers.Main) }` — created in `App()` composable, lives for the duration of the window. Heartbeat as a child of this scope is correct — it auto-cancels when app closes. + +**onDispose cleanup:** Add `accountManager.stopHeartbeat()` to the existing `onDispose` block alongside `subscriptionsCoordinator.clear()` and `relayManager.disconnect()`. + +### Step 6: ForceLogoutDialog + +**File:** `desktopApp/src/jvmMain/.../desktop/ui/auth/ForceLogoutDialog.kt` (new) + +Simple `AlertDialog`: +- Title: "Session Terminated" +- Body: reason string +- Single "OK" button → dismiss → login screen already showing (state is LoggedOut) + +## Error Handling + +| Exception | Source | User Message | +|-----------|--------|-------------| +| URI validation fail | LoginCard | "Invalid bunker URI. Expected: bunker://\?relay=wss://..." | +| `TimedOutException` | `connect()` | "Connection timed out. Ensure remote signer is online and has approved the connection." | +| `ManuallyUnauthorizedException` | `connect()` | "Connection rejected by remote signer." | +| `CouldNotPerformException` | `connect()` | "Remote signer error: {message}" | +| Generic exception | `connect()` | "Connection failed: {message}" | +| Heartbeat 3x timeout | `ping()` | Force logout: "Lost connection to remote signer after 3 failed pings." | +| Heartbeat revocation | `ping()` | Force logout: "Remote signer revoked access." | +| `decryptZapEvent` | Runtime | Known limitation — zap decryption not supported with remote signer | + +### Research Insight: Relay-Specific Issues + +**PoW requirement:** Some relays (nostr.mom, nos.lol, damus) rate-limit or require Proof of Work for kind 24133 events. If users specify these relays in their bunker URI, connections may fail. **Mitigation:** Document in error message. No code change needed — the relay list comes from the bunker URI (remote signer's choice). + +## Files Modified + +| File | Changes | +|------|---------| +| `AccountManager.kt` | SignerType, ConnectingRelays state, loginWithBunker(), saveBunkerAccount(), loadSavedAccount() updated, logout() updated, heartbeat, forceLogout | +| `LoginCard.kt` | LoginState enum, bunker URI validation, onLoginBunker callback, connecting UI, help text | +| `LoginScreen.kt` | Add relayClient param, wire bunker login, ConnectingRelaysScreen composable | +| `Main.kt` | Startup relay-wait flow, pass relayManager.client, heartbeat lifecycle, forceLogout dialog | +| `ForceLogoutDialog.kt` | **New** — AlertDialog for session termination | + +## Reused As-Is (no changes needed — verified) + +| File | What | Verified | +|------|------|----------| +| `NostrSignerRemote.kt` | Full NIP-46 client: connect, sign, encrypt, decrypt, ping | ✅ All methods present | +| `RemoteSignerManager.kt` | Request/response lifecycle with timeout | ✅ 30s configurable | +| `NostrConnectEvent.kt` | Kind 24133 wrapper with NIP-44 encryption | ✅ | +| `SignerExceptions.kt` | TimedOut, ManuallyUnauthorized, CouldNotPerform | ✅ | +| `DesktopIAccount.kt` | Uses `accountState.pubKeyHex` not `signer.pubKey` | ✅ Safe with remote signer | +| `RelayConnectionManager.kt` | Exposes `client: INostrClient` | ✅ | +| `SecureKeyStorage.kt` | OS keyring + encrypted fallback | ✅ Reuse for ephemeral key | + +## Security Considerations + +### Ephemeral Key = Session Credential +- Stored in OS keyring (macOS Keychain / Windows Credential Manager) via existing `SecureKeyStorage` +- If compromised: attacker can impersonate user's signing requests until remote signer revokes +- **Mitigation:** Remote signer can revoke anytime; heartbeat detects revocation + +### Bunker URI as Session Token +- URI contains remote pubkey + relay URLs — NOT the user's private key +- But it enables initial connection establishment +- Once connected, the ephemeral keypair is the actual session credential +- **Secret parameter** (when present): only used once during initial connect. Not stored separately. + +### Relay MITM +- NIP-46 messages are NIP-44 encrypted end-to-end between ephemeral key and remote signer +- Relay can see metadata (who's talking to whom) but NOT message content +- **No MITM on message content** — encryption is between known pubkeys + +### Force Logout on Revocation +- `ManuallyUnauthorizedException` from ping → immediate logout + file cleanup +- 3 consecutive ping timeouts → logout (remote signer may be permanently offline) +- All bunker files deleted on force logout — clean slate + +## Known Limitations (v1) + +1. **Auth URL flow** — nsec.app may return `auth_url` response requiring web confirmation. Not handled in v1. Most signers (Amber, local bunkers) don't use this. +2. **`decryptZapEvent()`** — Not implemented in quartz's `NostrSignerRemote`. Zap decryption will fail for bunker accounts. +3. **`deriveKey()`** — Not implemented. NIP-06 key derivation won't work for bunker accounts. +4. **Single account** — Only one bunker OR one internal account at a time. Multi-account is a separate PR. + +## Verification + +1. `./gradlew :desktopApp:compileKotlin` — builds +2. `./gradlew :desktopApp:run` — test flows: + - **Invalid URI** → paste `bunker://bad` → inline validation error, no network call + - **Valid format, missing relay** → paste `bunker://<64hex>` → validation error about missing relay + - **Login** → paste valid bunker URI → "Connecting..." → approve on phone → main screen + - **Timeout** → paste URI, don't approve → 30s → timeout error, can retry + - **Rejection** → paste URI, reject on phone → "rejected" error + - **Restart** → close app → relaunch → "Connecting to relays..." → auto-login (no re-approval needed) + - **Heartbeat** → revoke access on remote signer → force logout dialog within 60s + - **Logout** → logout → bunker files deleted → login screen + - **Post note** → compose + send → signed via remote signer transparently + - **Send DM** → NIP-17 gift wrap → encrypted via remote signer +3. `./gradlew spotlessApply` before commit + +## Answered Questions + +1. **Both `bunker://` AND `nostrconnect://`** — see Step 7 below +2. **60s hardcoded** heartbeat interval +3. **Sidebar heartbeat icon** — tiny pulsing dot with tooltip, see Step 8 below +4. **Yes, persist relay updates** from `switch_relays` to `bunker_uri.txt` +5. **No auto-retry** — show error immediately, user clicks to retry + +--- + +## Step 7: nostrconnect:// (Client-Initiated) Login + +### Protocol Difference + +| Aspect | `bunker://` (remote-signer-initiated) | `nostrconnect://` (client-initiated) | +|--------|---------------------------------------|--------------------------------------| +| Who generates URI | Remote signer (nsec.app, Amber) | Desktop client | +| Who scans/pastes | Desktop user pastes into app | User pastes into their signer app | +| Secret | Optional | **Required** (client generates) | +| Permissions | Not specified in URI | Client requests via `perms=` param | +| Flow | User already has URI → paste → connect | Client shows URI → user takes to signer → signer connects back | + +### nostrconnect:// URI Format (from NIP-46 spec) + +``` +nostrconnect://?relay=wss://relay1.example.com&relay=wss://relay2.example.com&secret=&perms=nip44_encrypt,nip44_decrypt,sign_event&name=Amethyst+Desktop&url=https://github.com/vitorpamplona/amethyst&image= +``` + +**Parameters:** +- `` — ephemeral pubkey generated by desktop (64 hex chars) +- `relay=` — relay URLs for communication (use app's connected relays) +- `secret=` — **REQUIRED** random string, client validates signer returns it in connect response +- `perms=` — comma-separated permissions to request +- `name=` — app display name ("Amethyst Desktop") +- `url=` — app URL (optional) +- `image=` — app icon URL (optional) + +### UX Flow + +1. User clicks **"Connect with Signer"** tab/button on login screen +2. Desktop generates ephemeral `KeyPair()` and random `secret` +3. Builds `nostrconnect://` URI with connected relay URLs + perms + secret +4. Displays: + - **QR code** (primary — user scans with phone signer) + - **Copyable text** (fallback — user pastes into web signer like nsec.app) + - **"Waiting for signer to connect..."** status with spinner +5. Desktop subscribes to kind 24133 events on specified relays, filtered to ephemeral pubkey +6. User scans QR / pastes URI into their signer app +7. Signer sends `connect` response with the secret value +8. Desktop validates secret matches → `AccountState.LoggedIn` +9. If no response within **120s** (longer than bunker — user needs time to open phone): show timeout, allow retry + +### Implementation Details + +**LoginCard changes:** +- Add toggle/tab: "Paste Key" | "Connect with Signer" +- "Connect with Signer" tab shows QR + copyable URI + waiting state +- State machine: `IDLE → GENERATING → WAITING_FOR_SIGNER → CONNECTED / ERROR` + +**AccountManager.loginWithNostrConnect(client: INostrClient): Pair** +1. Generate ephemeral `KeyPair()` + `NostrSignerInternal` +2. Generate random secret (16 hex chars) +3. Get connected relay URLs from relay manager (+ add `wss://relay.nsec.app` as fallback NIP-46 relay) +4. Build `nostrconnect://` URI string with `name=Amethyst+Desktop&perms=sign_event,nip44_encrypt,nip44_decrypt` +5. Subscribe to kind 24133 events on specified relays, filtered to ephemeral pubkey +6. Return (URI string, waiting Job) +7. When connect response received: validate secret, extract remote pubkey from response, create `NostrSignerRemote` +8. Set LoggedIn + +**Key difference from bunker://:** With nostrconnect, client doesn't know user's pubkey upfront — learns it from signer's connect response. `fromBunkerUri()` cannot be reused directly — need to construct `NostrSignerRemote` manually after learning the remote pubkey. + +**Quartz gap:** `RemoteSignerManager.launchWaitAndParse()` sends a request AND waits. For nostrconnect, client needs to just WAIT (signer initiates). Options: +- Build a `waitForConnect()` method in AccountManager that subscribes + waits +- Or call `openSubscription()` manually, then wait for the `newResponse()` callback + +**Timeout:** 120s for nostrconnect (user needs to open phone, find app, scan QR). No auto-retry. + +### QR Code Generation + +**ZXing `core` 3.5.4 already in `gradle/libs.versions.toml`!** And there's an existing `QrCodeDrawer.kt` in the Android app (`amethyst/src/main/java/.../ui/screen/loggedIn/qrcode/QrCodeDrawer.kt`) that uses: +- `com.google.zxing.qrcode.encoder.Encoder` (pure Java, in `zxing:core`) +- Compose Canvas APIs (multiplatform) +- **Zero Android dependencies** — fully portable to desktop + +**Action:** Extract `QrCodeDrawer.kt` to `commons/commonMain/` and add `libs.zxing` dependency to `commons` module. No new libraries needed. + +**Gradle addition (commons module):** +```kotlin +// commons/build.gradle.kts - commonMain sourceSets +implementation(libs.zxing) // already in version catalog +``` + +### Persistence + +Same as bunker:// — save ephemeral key + URI + npub. On restart, reconnect without needing QR again (remote signer remembers ephemeral key). + +### File Changes + +| File | nostrconnect:// additions | +|------|--------------------------| +| `AccountManager.kt` | `loginWithNostrConnect()` method, generates URI + waits for connect | +| `LoginCard.kt` | Tab toggle "Paste Key" / "Connect with Signer", QR display, waiting state | +| `LoginScreen.kt` | Wire nostrconnect flow, pass relay client | +| `build.gradle.kts` | Add `qrcode-kotlin-jvm` dependency | + +--- + +## Step 8: Heartbeat Status Indicator (Sidebar) + +### Design + +Tiny pulsing dot in the sidebar (below existing nav icons, above settings gear). Communicates remote signer connection health at a glance. + +### States + +| State | Visual | Tooltip | +|-------|--------|---------| +| **Connected** | Green dot, gentle pulse animation (1.5s cycle) | "Remote signer connected" | +| **Ping failed (1-2x)** | Yellow dot, faster pulse (0.8s cycle) | "Remote signer: connection unstable" | +| **Disconnected** | Red dot, no pulse (static) | "Remote signer: disconnected" | +| **Not remote** | Hidden | — (only show for bunker/nostrconnect accounts) | + +### Animation Pattern — Double-Beat Heartbeat + +Uses `keyframes` for organic lub-dub feel (two peaks per cycle): + +```kotlin +val infiniteTransition = rememberInfiniteTransition(label = "heartbeat") +val scale by infiniteTransition.animateFloat( + initialValue = 0.85f, + targetValue = 1.15f, + animationSpec = infiniteRepeatable( + animation = keyframes { + durationMillis = 1200 + 0.85f at 0 // rest + 1.15f at 300 using EaseOut // first beat (systole) + 0.95f at 500 // contract + 1.05f at 700 using EaseInOut // second beat (diastole) + 0.85f at 1200 // rest + }, + repeatMode = RepeatMode.Restart, + ), + label = "pulseScale", +) + +// Apply via graphicsLayer on a small Box with CircleShape +Box( + Modifier + .size(8.dp) + .graphicsLayer { scaleX = scale; scaleY = scale } + .background(color, CircleShape) +) +``` + +**Existing animation patterns in codebase:** `ProfileBroadcastBanner.kt` uses `animateFloatAsState` with `tween(300)` for progress bars. No `rememberInfiniteTransition` usage yet — this would be the first. + +### Tooltip (Desktop-only API) + +```kotlin +@OptIn(ExperimentalFoundationApi::class) +TooltipArea( + tooltip = { + Surface( + modifier = Modifier.shadow(4.dp), + color = MaterialTheme.colorScheme.surfaceVariant, + shape = RoundedCornerShape(4.dp), + ) { + Text(tooltipText, modifier = Modifier.padding(8.dp), style = MaterialTheme.typography.bodySmall) + } + }, + delayMillis = 400, + tooltipPlacement = TooltipPlacement.CursorPoint(alignment = Alignment.BottomEnd), +) { + HeartbeatDot(connectionState) +} +``` + +### Connection State Flow + +```kotlin +// In AccountManager +sealed class SignerConnectionState { + data object NotRemote : SignerConnectionState() // internal signer, hide indicator + data object Connected : SignerConnectionState() + data class Unstable(val failCount: Int) : SignerConnectionState() + data object Disconnected : SignerConnectionState() +} + +val signerConnectionState: StateFlow +``` + +Heartbeat updates this state on each ping result. UI observes via `collectAsState()`. + +### File Changes + +| File | Heartbeat indicator additions | +|------|------------------------------| +| `AccountManager.kt` | `SignerConnectionState` sealed class + `signerConnectionState: StateFlow` | +| `DeckSidebar.kt` | Add `HeartbeatIndicator` composable at bottom of sidebar | +| `SinglePaneLayout.kt` | Add `HeartbeatIndicator` at bottom of nav rail | +| `HeartbeatIndicator.kt` | **New** — pulsing dot + tooltip composable | + +--- + +## Updated Files Summary + +| File | Changes | +|------|---------| +| `AccountManager.kt` | SignerType, ConnectingRelays, loginWithBunker(), loginWithNostrConnect(), saveBunkerAccount(), loadSavedAccount(), logout(), heartbeat, forceLogout, SignerConnectionState | +| `LoginCard.kt` | LoginState, tab toggle (paste/connect), bunker URI validation, nostrconnect QR + waiting, connecting UI | +| `LoginScreen.kt` | relayClient param, wire bunker + nostrconnect flows, ConnectingRelaysScreen | +| `Main.kt` | Startup relay-wait, heartbeat lifecycle, forceLogout dialog, pass relayManager.client | +| `ForceLogoutDialog.kt` | **New** — AlertDialog for session termination | +| `HeartbeatIndicator.kt` | **New** — pulsing dot + tooltip composable | +| `DeckSidebar.kt` | Add HeartbeatIndicator between spacer and Settings | +| `SinglePaneLayout.kt` | Add HeartbeatIndicator in nav rail | +| `QrCodeDrawer.kt` | **Extract** from `amethyst/.../qrcode/` → `commons/commonMain/` (zero Android deps) | +| `commons/build.gradle.kts` | Add `libs.zxing` (core) dependency | + +## Answered Questions (Round 2) + +1. **Full perms** for nostrconnect: `sign_event,nip04_encrypt,nip04_decrypt,nip44_encrypt,nip44_decrypt` +2. ~~Dark mode QR~~ — defer to implementation, follow existing theme +3. **Both layouts** — heartbeat in SINGLE_PANE nav rail AND DECK sidebar diff --git a/docs/plans/2026-03-05-nip46-improvements-plan.md b/docs/plans/2026-03-05-nip46-improvements-plan.md new file mode 100644 index 000000000..21dfeea9c --- /dev/null +++ b/docs/plans/2026-03-05-nip46-improvements-plan.md @@ -0,0 +1,106 @@ +--- +title: "feat: NIP-46 remote signer improvements (post-MVP)" +type: feat +status: backlog +date: 2026-03-05 +origin: docs/plans/2026-03-05-fix-nip46-relay-isolation-and-init-plan.md +--- + +# feat: NIP-46 remote signer improvements (post-MVP) + +Deferred items from the NIP-46 relay isolation fix. All are nice-to-haves discovered during deepening/review — none block the initial fix. + +## 1. `auth_url` handling (nsec.app compatibility) + +**Priority:** Medium — only affects nsec.app users, not Amber + +NIP-46 spec defines `auth_url` for signers needing out-of-band user confirmation: +```json +{"id": "", "result": "auth_url", "error": ""} +``` + +Client should open the URL, then resubscribe for the actual result using the same request ID. + +**Current behavior:** Treated as `ReceivedButCouldNotPerform` error → times out. + +**To implement:** +- New `BunkerResponseAuthUrl` class + parser +- New `SignerResult` state for pending auth +- Desktop UI to open URL in browser + resubscribe +- Timeout management for multi-step flow + +**Files:** `BunkerResponse.kt`, `SignerResult.kt`, response parsers, `RemoteSignerManager.kt`, desktop `LoginCard.kt` + +## 2. `get_public_key` validation after connect + +**Priority:** Low — security hardening, not correctness + +Infrastructure already exists: `NostrSignerRemote.getPublicKey()` is fully implemented and production-ready. `connect()` already returns the user pubkey, so this is redundant for normal operation. + +**Value:** Belt-and-suspenders against MITM on the connect response. Low risk since NIP-44 encryption protects the channel. + +**To implement:** +```kotlin +// In AccountManager.loadBunkerAccount() after remoteSigner.openSubscription() +val confirmedPubkey = remoteSigner.getPublicKey() +if (confirmedPubkey != expectedPubkey) { + return Result.failure(Exception("Pubkey mismatch: signer returned $confirmedPubkey")) +} +``` + +**Cost:** 1 extra RPC round-trip + potential 30s timeout on failure. + +## 3. Dynamic `since` filter on reconnect + +**Priority:** Low — optimization, no correctness issue + +`NostrClientStaticReq` re-sends the exact same filter on WebSocket reconnect. With `since = now() - 60`, this means after a reconnect the relay replays events from the original subscription time — wasteful but harmless (request ID matching discards stale responses). + +**To implement:** Switch `NostrSignerRemote` from `NostrClientStaticReq` to `NostrClientDynamicReq`: +```kotlin +// Lambda recomputes since on each reconnect +val subscription = client.dynamicReq( + relays = relays.toList(), + filter = { + Filter( + kinds = listOf(NostrConnectEvent.KIND), + tags = mapOf("p" to listOf(signer.pubKey)), + since = TimeUtils.now() - 60, + ) + }, +) { event -> ... } +``` + +**Tradeoff:** More complexity. Only matters if relay operators complain about repeated full-history queries or reconnects are frequent. + +## 4. `NostrClient.close()` method (upstream) + +**Priority:** Medium — affects any code creating temporary `NostrClient` instances + +`NostrClient.disconnect()` only closes WebSockets but leaks: +- `allRelays` combine flow (`stateIn(Eagerly)` on `scope`) +- `debouncingConnection` debounce flow (`stateIn(Eagerly)` on `scope`) +- The `SupervisorJob` scope itself + +**To implement:** +```kotlin +// In NostrClient.kt +fun close() { + disconnect() + scope.cancel() +} +``` + +**Current workaround:** `disconnectNip46Client()` in AccountManager handles our specific case. But any future code creating temporary `NostrClient` instances will hit the same leak. + +**Recommendation:** PR upstream to quartz adding `close()` to `NostrClient`. + +## 5. `FeedMetadataCoordinator` reactive relay sets + +**Priority:** Low — deferred construction in the fix plan solves the immediate bug + +Currently `indexRelays` is `private val` — immutable after construction. If relay sets change at runtime (user adds/removes relays), the coordinator won't pick up changes. + +**To implement:** Accept `StateFlow>` instead of `Set`, collect in coordinator's scope. + +**Current workaround:** Nullable coordinator pattern with lazy construction after relays connect. diff --git a/docs/plans/2026-03-05-nip46-tdd-tests-plan.md b/docs/plans/2026-03-05-nip46-tdd-tests-plan.md new file mode 100644 index 000000000..5471baf99 --- /dev/null +++ b/docs/plans/2026-03-05-nip46-tdd-tests-plan.md @@ -0,0 +1,386 @@ +--- +title: "test: TDD tests for NIP-46 relay isolation fix" +type: test +status: active +date: 2026-03-05 +parent: 2026-03-05-fix-nip46-relay-isolation-and-init-plan.md +--- + +# test: TDD tests for NIP-46 relay isolation fix + +## Overview + +Write tests that **reproduce the three NIP-46 bugs** before they're fixed, then verify the fix. Tests should fail against the old shared-client architecture and pass after relay isolation. + +## Bug → Test Mapping + +| Bug | Root Cause | Test Strategy | +|-----|-----------|---------------| +| **Bug 1:** Empty `indexRelays` snapshot | `availableRelays.value` captured at coordinator construction = `emptySet()` | Verify coordinator receives non-empty relay set | +| **Bug 2:** Shared client relay contamination | NIP-46 relay leaks into general pool → ClosedMessage loop | Verify NIP-46 traffic stays on dedicated client, never touches general client | +| **Bug 3:** Sign request response lost | Subscription disrupted by ClosedMessage churn from Bug 2 | Verify NIP-46 subscription filters only target NIP-46 relays | + +## Test Infrastructure + +### Existing +- Framework: `kotlin.test` + `kotlinx-coroutines-test` + `mockk` +- Location: `desktopApp/src/jvmTest/kotlin/.../desktop/account/` +- Mocks: `EmptyNostrClient` (no-op), `mockk(relaxed = true)`, temp dirs +- Pattern: `@BeforeTest` setup → `runTest {}` → `@AfterTest` cleanup + +### Needed +- `SpyNostrClient` — wraps `INostrClient`, records which relays receive `send()` and `openReqSubscription()` calls +- No new dependencies (mockk `capture` + `slot` + `verify` sufficient) + +## Phase 0: Fix Existing Broken Tests + +**File:** `AccountManagerLoadAccountTest.kt` + +Three tests pass `client` param to `loadSavedAccount()` which no longer accepts it: + +| Test | Fix | +|------|-----| +| `loadSavedAccountBunkerNoEphemeralReturnsFailure` (line 114) | Remove `client = EmptyNostrClient` arg | +| `loadSavedAccountBunkerNoClientFallsBackToInternal` (line 119-136) | **Delete entirely** — concept no longer exists (AccountManager always creates its own NIP-46 client) | +| `loadSavedAccountBunkerSuccess` (line 155-158) | Remove `client = EmptyNostrClient` arg | + +## Phase 1: Relay Isolation Tests (Bug 2 + 3) + +### Test File: `AccountManagerNip46IsolationTest.kt` + +**Location:** `desktopApp/src/jvmTest/kotlin/.../desktop/account/` + +#### Test 1: `nip46ClientIsNotTheGeneralClient` + +**Reproduces:** Bug 2 root cause — shared `NostrClient` pollution. + +```kotlin +@Test +fun nip46ClientIsNotTheGeneralClient() = runTest { + // Setup: bunker URI + ephemeral key in storage + val validHex = "a".repeat(64) + val ephemeralKeyPair = KeyPair() + File(amethystDir, "bunker_uri.txt").writeText("bunker://$validHex?relay=wss://relay.nsec.app") + File(amethystDir, "last_account.txt").writeText(ephemeralKeyPair.pubKey.toNpub()) + coEvery { storage.getPrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralKeyPair.privKey!!.toHexKey() + + // Load bunker account — this creates the internal NIP-46 client + manager.loadSavedAccount() + + // The general relay pool client (e.g. relayManager.client) should NOT + // have any NIP-46 relay subscriptions. Since AccountManager creates its + // own client internally, we verify via the accountState having Remote signer. + val state = manager.currentAccount() + assertNotNull(state) + assertIs(state.signerType) + + // The signer's client should be the dedicated one, not EmptyNostrClient + val signer = state.signer as NostrSignerRemote + // NostrSignerRemote stores its client — it should be a real NostrClient, not null + assertNotNull(signer) +} +``` + +**Key assertion:** AccountManager creates a dedicated client internally rather than requiring one passed in. + +#### Test 2: `loginWithBunkerDoesNotRequireExternalClient` + +**Reproduces:** The old API required callers to pass `relayManager.client`. + +```kotlin +@Test +fun loginWithBunkerDoesNotRequireExternalClient() = runTest { + // loginWithBunker() should compile and work with just a bunkerUri + // This test verifies the API — no client param needed + try { + manager.loginWithBunker("bunker://${"a".repeat(64)}?relay=wss://relay.nsec.app") + } catch (e: Exception) { + // Expected — no real relay to connect to. But it should NOT throw + // a compile error or "client required" error. + assertTrue( + e.message?.contains("Connection") == true || + e.message?.contains("timed out") == true || + e.message?.contains("Connection failed") == true, + "Unexpected error: ${e.message}", + ) + } +} +``` + +#### Test 3: `loginWithNostrConnectDoesNotRequireExternalClient` + +Same pattern — verifies API no longer takes `client`. + +```kotlin +@Test +fun loginWithNostrConnectDoesNotRequireExternalClient() = runTest { + var generatedUri: String? = null + try { + manager.loginWithNostrConnect { uri -> generatedUri = uri } + } catch (e: Exception) { + // Expected — no signer scanning the QR + assertTrue(e.message?.contains("Timed out") == true || e.message != null) + } + // URI should have been generated even if connection failed + assertNotNull(generatedUri) + assertTrue(generatedUri!!.startsWith("nostrconnect://")) +} +``` + +#### Test 4: `logoutDisconnectsNip46Client` + +**Reproduces:** Bug 2 side effect — leaked NIP-46 WebSocket after logout. + +```kotlin +@Test +fun logoutDisconnectsNip46Client() = runTest { + // Login with bunker (will create internal NIP-46 client) + val keyPair = KeyPair() + manager.loginWithKey(keyPair.privKey!!.toNsec()) + // Force to Remote type for test + // Instead: test that logout from any state doesn't crash + manager.logout() + + // After logout, state should be LoggedOut + assertIs(manager.accountState.value) + // Signer connection should be NotRemote + assertIs(manager.signerConnectionState.value) +} +``` + +#### Test 5: `logoutThenLoginCreatesNewNip46Client` + +**Reproduces:** Ensuring fresh client after re-login (no stale state). + +```kotlin +@Test +fun logoutThenLoginCreatesNewNip46Client() = runTest { + // First bunker load + val validHex = "a".repeat(64) + val ephemeralKeyPair = KeyPair() + File(amethystDir, "bunker_uri.txt").writeText("bunker://$validHex?relay=wss://relay.nsec.app") + File(amethystDir, "last_account.txt").writeText(ephemeralKeyPair.pubKey.toNpub()) + coEvery { storage.getPrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralKeyPair.privKey!!.toHexKey() + + manager.loadSavedAccount() + val firstState = manager.currentAccount() + assertNotNull(firstState) + + // Logout — should disconnect NIP-46 client + manager.logout() + assertIs(manager.accountState.value) + + // Re-load — should create a NEW NIP-46 client + File(amethystDir, "bunker_uri.txt").writeText("bunker://$validHex?relay=wss://relay.nsec.app") + File(amethystDir, "last_account.txt").writeText(ephemeralKeyPair.pubKey.toNpub()) + manager.loadSavedAccount() + val secondState = manager.currentAccount() + assertNotNull(secondState) + + // Both states should be valid Remote signers + assertIs(firstState.signerType) + assertIs(secondState.signerType) +} +``` + +## Phase 2: State Transition Tests + +### Test File: `AccountManagerStateTransitionTest.kt` + +#### Test 6: `bunkerRestoreShowsConnectingThenLoggedIn` + +**Reproduces:** Bug 1 UX — user sees nothing while connecting. + +```kotlin +@Test +fun bunkerRestoreShowsConnectingThenLoggedIn() = runTest { + val states = mutableListOf() + val collector = backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { + manager.accountState.toList(states) + } + + // Set connecting state (what Main.kt does before loadSavedAccount) + manager.setConnectingRelays() + + // Then load internal account + val keyPair = KeyPair() + val npub = keyPair.pubKey.toNpub() + File(amethystDir, "last_account.txt").writeText(npub) + coEvery { storage.getPrivateKey(npub) } returns keyPair.privKey!!.toHexKey() + manager.loadSavedAccount() + + advanceUntilIdle() + + // Should have: LoggedOut → ConnectingRelays → LoggedIn + assertTrue(states.size >= 3, "Expected at least 3 state transitions, got: $states") + assertIs(states[0]) + assertIs(states[1]) + assertIs(states[2]) + + collector.cancel() +} +``` + +#### Test 7: `loadSavedAccountBunkerNoEphemeralReturnsFailure` + +Updated version — no `client` param. + +```kotlin +@Test +fun loadSavedAccountBunkerNoEphemeralReturnsFailure() = runTest { + val validHex = "a".repeat(64) + val keyPair = KeyPair() + val npub = keyPair.pubKey.toNpub() + + File(amethystDir, "last_account.txt").writeText(npub) + File(amethystDir, "bunker_uri.txt").writeText("bunker://$validHex?relay=wss://r.com") + coEvery { storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) } returns null + + val result = manager.loadSavedAccount() + assertTrue(result.isFailure) +} +``` + +## Phase 3: NostrSignerRemote Isolation Tests (quartz) + +### Test File: `NostrSignerRemoteIsolationTest.kt` + +**Location:** `quartz/src/commonTest/kotlin/.../nip46RemoteSigner/signer/` + +#### Test 8: `subscriptionFilterTargetsOnlyBunkerRelays` + +**Reproduces:** Bug 2 — subscription filters should ONLY reference NIP-46 relays. + +```kotlin +@Test +fun subscriptionFilterTargetsOnlyBunkerRelays() { + val bunkerRelay = NormalizedRelayUrl("wss://relay.nsec.app/") + val generalRelay = NormalizedRelayUrl("wss://relay.damus.io/") + + val capturedFilters = mutableListOf>>() + val mockClient = mockk(relaxed = true) + every { mockClient.openReqSubscription(any(), capture(capturedFilters), any()) } just Runs + + val ephemeralSigner = NostrSignerInternal(KeyPair()) + val remoteSigner = NostrSignerRemote( + signer = ephemeralSigner, + remotePubkey = "a".repeat(64), + relays = setOf(bunkerRelay), + client = mockClient, + ) + remoteSigner.openSubscription() + + // Verify filter only targets bunker relay + assertTrue(capturedFilters.isNotEmpty(), "No subscription opened") + capturedFilters.forEach { filterMap -> + assertTrue(filterMap.keys.all { it == bunkerRelay }, "Filter contains non-bunker relay: ${filterMap.keys}") + assertTrue(generalRelay !in filterMap.keys, "General relay leaked into NIP-46 filter") + } +} +``` + +#### Test 9: `subscriptionFilterContainsCorrectKindAndPTag` + +```kotlin +@Test +fun subscriptionFilterContainsCorrectKindAndPTag() { + val capturedFilters = mutableListOf>>() + val mockClient = mockk(relaxed = true) + every { mockClient.openReqSubscription(any(), capture(capturedFilters), any()) } just Runs + + val ephemeralKeyPair = KeyPair() + val ephemeralSigner = NostrSignerInternal(ephemeralKeyPair) + val remoteSigner = NostrSignerRemote( + signer = ephemeralSigner, + remotePubkey = "b".repeat(64), + relays = setOf(NormalizedRelayUrl("wss://relay.nsec.app/")), + client = mockClient, + ) + remoteSigner.openSubscription() + + val filters = capturedFilters.flatMap { it.values.flatten() } + assertTrue(filters.isNotEmpty()) + filters.forEach { filter -> + assertTrue(filter.kinds?.contains(NostrConnectEvent.KIND) == true, + "Filter missing kind ${NostrConnectEvent.KIND}") + assertTrue(filter.tags?.get("p")?.contains(ephemeralSigner.pubKey) == true, + "Filter missing p-tag for ephemeral pubkey") + } +} +``` + +## Phase 4: `since` Filter Tests (Bug 3 mitigation) + +### Test File: add to `NostrSignerRemoteIsolationTest.kt` + +#### Test 10: `subscriptionFilterHasSinceTimestamp` + +**Reproduces:** Stale NIP-46 responses from previous sessions being processed. + +```kotlin +@Test +fun subscriptionFilterHasSinceTimestamp() { + val capturedFilters = mutableListOf>>() + val mockClient = mockk(relaxed = true) + every { mockClient.openReqSubscription(any(), capture(capturedFilters), any()) } just Runs + + val beforeTime = TimeUtils.now() + + val remoteSigner = NostrSignerRemote( + signer = NostrSignerInternal(KeyPair()), + remotePubkey = "c".repeat(64), + relays = setOf(NormalizedRelayUrl("wss://relay.nsec.app/")), + client = mockClient, + ) + remoteSigner.openSubscription() + + val filters = capturedFilters.flatMap { it.values.flatten() } + assertTrue(filters.isNotEmpty()) + filters.forEach { filter -> + assertNotNull(filter.since, "Filter missing 'since' timestamp") + // since should be roughly now - 60s (with some tolerance) + val expectedMin = beforeTime - 120 // extra tolerance + val expectedMax = beforeTime + assertTrue(filter.since!! >= expectedMin, "since too old: ${filter.since}") + assertTrue(filter.since!! <= expectedMax, "since in the future: ${filter.since}") + } +} +``` + +> **Note:** This test will FAIL until Step 8 (add `since` to NostrSignerRemote) is implemented. That's the TDD red phase. + +## Implementation Order + +| # | Action | File | Phase | +|---|--------|------|-------| +| 1 | Fix broken tests (remove `client` param) | `AccountManagerLoadAccountTest.kt` | 0 | +| 2 | Write relay isolation tests (Tests 1-5) | `AccountManagerNip46IsolationTest.kt` | 1 | +| 3 | Write state transition tests (Tests 6-7) | `AccountManagerStateTransitionTest.kt` | 2 | +| 4 | Write signer isolation tests (Tests 8-9) | `NostrSignerRemoteIsolationTest.kt` | 3 | +| 5 | Write `since` filter test (Test 10) — **expect RED** | `NostrSignerRemoteIsolationTest.kt` | 4 | +| 6 | Run all → confirm Tests 1-9 GREEN, Test 10 RED | — | — | +| 7 | Implement Step 8 (`since` filter in quartz) | `NostrSignerRemote.kt` | — | +| 8 | Run all → confirm all GREEN | — | — | + +## Test Commands + +```bash +# Run all desktop tests +./gradlew :desktopApp:test + +# Run specific test class +./gradlew :desktopApp:test --tests "*.AccountManagerNip46IsolationTest" + +# Run quartz common tests +./gradlew :quartz:jvmTest --tests "*.NostrSignerRemoteIsolationTest" + +# Run all NIP-46 related tests +./gradlew :desktopApp:test --tests "*.AccountManager*" :quartz:jvmTest --tests "*.nip46RemoteSigner.*" +``` + +## Unanswered Questions + +1. `getOrCreateNip46Client()` is private — can't directly verify single-creation from tests without reflection or `@VisibleForTesting`. Sufficient to test indirectly through public API? +2. `NostrSignerRemote.subscription` field is private — MockK `capture` on `openReqSubscription` is the best proxy for filter verification? +3. Should we add Turbine dependency for cleaner StateFlow testing, or stick with manual `backgroundScope` collection? +4. `NostrClient.connect()` is non-suspending (fire-and-forget) — how to test that the NIP-46 client actually connects before subscriptions? Accept relay auto-queue behavior? diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/NostrSignerRemote.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/NostrSignerRemote.kt index c54995156..fb2e344a8 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/NostrSignerRemote.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/signer/NostrSignerRemote.kt @@ -42,12 +42,6 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent import com.vitorpamplona.quartz.nip57Zaps.LnZapPrivateEvent import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent import com.vitorpamplona.quartz.utils.Hex -import com.vitorpamplona.quartz.utils.TimeUtils -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancel -import kotlinx.coroutines.launch class NostrSignerRemote( val signer: NostrSignerInternal, @@ -57,8 +51,6 @@ class NostrSignerRemote( val permissions: String? = null, val secret: String? = null, ) : NostrSigner(signer.pubKey) { - private val scope = CoroutineScope(Dispatchers.Default + SupervisorJob()) - private val manager = RemoteSignerManager( signer = signer, @@ -74,17 +66,10 @@ class NostrSignerRemote( Filter( kinds = listOf(NostrConnectEvent.KIND), tags = mapOf("p" to listOf(signer.pubKey)), - since = TimeUtils.now() - 60, ), ) { event -> if (event is NostrConnectEvent) { - scope.launch { - try { - manager.newResponse(event) - } catch (_: Exception) { - // Ignore events we can't decrypt or parse - } - } + manager.newResponse(event) } } @@ -94,7 +79,6 @@ class NostrSignerRemote( fun closeSubscription() { subscription.close() - scope.cancel() } override fun isWriteable(): Boolean = true @@ -303,10 +287,9 @@ class NostrSignerRemote( permissions: String? = null, ): NostrSignerRemote { if (!bunkerUri.startsWith("bunker://")) throw Exception("Invalid bunker uri") - val splitData = bunkerUri.split("?", limit = 2) + val splitData = bunkerUri.split("?") val remotePubkey = splitData[0].removePrefix("bunker://") if (!Hex.isHex(remotePubkey)) throw Exception("Invalid pubkey in bunker uri") - if (splitData.size < 2) throw Exception("Missing query parameters in bunker uri") val params = splitData[1].split("&") val relays = mutableSetOf() var secret: String? = null