Merge pull request #684 from synonymdev/fix/MnemonicNotFound-crash-review

fix: address PR review comments for mnemonic crash fix

Author: ovitrif

app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt

@@ -10,6 +10,7 @@ import com.synonym.vssclient.vssNewClientWithLnurlAuth
 import com.synonym.vssclient.vssStore
 import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineDispatcher
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.withContext
 import kotlinx.coroutines.withTimeout
 import to.bitkit.data.keychain.Keychain
@@ -28,24 +29,15 @@ class VssBackupClient @Inject constructor(
 ) {
     private var isSetup = CompletableDeferred<Unit>()
 
-    /**
-     * Sets up the VSS client. Returns true if setup succeeded, false if mnemonic not available.
-     * Throws on other errors.
-     */
-    suspend fun setup(walletIndex: Int = 0): Boolean = withContext(ioDispatcher) {
-        // If already set up successfully, return immediately
-        if (isSetup.isCompleted && !isSetup.isCancelled) {
-            runCatching { isSetup.await() }.onSuccess { return@withContext true }
-        }
+    suspend fun setup(walletIndex: Int = 0): Result<Boolean> = withContext(ioDispatcher) {
+        runCatching {
+            if (isSetup.isCompleted && !isSetup.isCancelled) {
+                runCatching { isSetup.await() }.onSuccess { return@runCatching true }
+            }
 
-        // Check if mnemonic is available before proceeding
-        val mnemonic = keychain.loadString(Keychain.Key.BIP39_MNEMONIC.name)
-        if (mnemonic == null) {
-            Logger.warn("VSS client setup deferred: mnemonic not available yet", context = TAG)
-            return@withContext false
-        }
+            val mnemonic = keychain.loadString(Keychain.Key.BIP39_MNEMONIC.name)
+                ?: return@runCatching false
 
-        runCatching {
             withTimeout(30.seconds) {
                 Logger.debug("VSS client setting up…", context = TAG)
                 val vssUrl = Env.vssServerUrl
@@ -72,11 +64,44 @@ class VssBackupClient @Inject constructor(
                 isSetup.complete(Unit)
                 Logger.info("VSS client setup with server: '$vssUrl'", context = TAG)
             }
+            true
         }.onFailure {
             isSetup.completeExceptionally(it)
-            Logger.error("VSS client setup error", e = it, context = TAG)
+            Logger.error("VSS client setup error", it, TAG)
+        }
+    }
+
+    class SetupRetryLogger {
+        var onSuccess: (attempt: Int) -> Unit = {}
+        var onRetry: (attempt: Int, maxAttempts: Int, delayMs: Long) -> Unit = { _, _, _ -> }
+        var onExhausted: (maxAttempts: Int) -> Unit = {}
+    }
+
+    suspend fun setupWithRetry(
+        maxAttempts: Int = 10,
+        baseDelayMs: Long = 1000L,
+        logger: SetupRetryLogger.() -> Unit,
+    ): Result<Boolean> = withContext(ioDispatcher) {
+        val log = SetupRetryLogger().apply(logger)
+        var attempt = 0
+        while (attempt < maxAttempts) {
+            val result = setup()
+            if (result.getOrDefault(false)) {
+                log.onSuccess(attempt + 1)
+                return@withContext Result.success(true)
+            }
+            if (result.isFailure) {
+                return@withContext result
+            }
+            attempt++
+            if (attempt < maxAttempts) {
+                val delayMs = baseDelayMs * attempt
+                log.onRetry(attempt, maxAttempts, delayMs)
+                delay(delayMs)
+            }
         }
-        true
+        log.onExhausted(maxAttempts)
+        Result.success(false)
     }
 
     fun reset() {

app/src/main/java/to/bitkit/repositories/BackupRepo.kt

@@ -120,7 +120,22 @@ class BackupRepo @Inject constructor(
         isObserving = true
         Logger.debug("Start observing backup statuses and data store changes", context = TAG)
 
-        scope.launch { setupVssClientWithRetry() }
+        scope.launch {
+            vssBackupClient.setupWithRetry {
+                onSuccess = { attempt ->
+                    Logger.debug("VSS client setup succeeded on attempt $attempt", context = TAG)
+                }
+                onRetry = { attempt, maxAttempts, delayMs ->
+                    Logger.debug(
+                        "VSS client setup deferred, retrying in ${delayMs}ms (attempt $attempt/$maxAttempts)",
+                        context = TAG,
+                    )
+                }
+                onExhausted = { maxAttempts ->
+                    Logger.warn("VSS client setup failed after $maxAttempts attempts", context = TAG)
+                }
+            }
+        }
 
         scope.launch {
             BackupCategory.entries.forEach { category ->
@@ -166,33 +181,6 @@ class BackupRepo @Inject constructor(
         Logger.debug("Stopped observing backup statuses and data store changes", context = TAG)
     }
 
-    private suspend fun setupVssClientWithRetry() {
-        var attempt = 0
-        val maxAttempts = 10
-        val baseDelayMs = 1000L
-
-        while (attempt < maxAttempts && isObserving) {
-            val success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
-            if (success) {
-                Logger.debug("VSS client setup succeeded on attempt ${attempt + 1}", context = TAG)
-                return
-            }
-            attempt++
-            if (attempt < maxAttempts) {
-                val delayMs = baseDelayMs * attempt // Linear backoff: 1s, 2s, 3s, ...
-                Logger.debug(
-                    "VSS client setup deferred, retrying in ${delayMs}ms (attempt $attempt/$maxAttempts)",
-                    context = TAG,
-                )
-                delay(delayMs)
-            }
-        }
-
-        if (isObserving) {
-            Logger.warn("VSS client setup failed after $maxAttempts attempts", context = TAG)
-        }
-    }
-
     private fun startBackupStatusObservers() {
         // Observe backup status changes for each category
         BackupCategory.entries.forEach { category ->
@@ -570,7 +558,7 @@ class BackupRepo @Inject constructor(
     suspend fun getLatestBackupTime(): ULong? = withContext(ioDispatcher) {
         runCatching {
             withTimeout(VSS_TIMESTAMP_TIMEOUT) {
-                vssBackupClient.setup()
+                vssBackupClient.setup().getOrThrow()
                 coroutineScope {
                     BackupCategory.entries
                         .filter { it != BackupCategory.LIGHTNING_CONNECTIONS }

app/src/test/java/to/bitkit/data/backup/VssBackupClientTest.kt

@@ -11,7 +11,6 @@ import org.mockito.kotlin.whenever
 import to.bitkit.data.keychain.Keychain
 import to.bitkit.test.BaseUnitTest
 import kotlin.test.assertFalse
-import kotlin.test.assertTrue
 
 class VssBackupClientTest : BaseUnitTest() {
 
@@ -35,7 +34,7 @@ class VssBackupClientTest : BaseUnitTest() {
 
         val result = sut.setup()
 
-        assertFalse(result)
+        assertFalse(result.getOrThrow())
     }
 
     @Test
@@ -65,8 +64,8 @@ class VssBackupClientTest : BaseUnitTest() {
         whenever(keychain.loadString(Keychain.Key.BIP39_MNEMONIC.name)).thenReturn(null)
 
         // Multiple calls should all return false without crashing
-        assertFalse(sut.setup())
-        assertFalse(sut.setup())
-        assertFalse(sut.setup())
+        assertFalse(sut.setup().getOrThrow())
+        assertFalse(sut.setup().getOrThrow())
+        assertFalse(sut.setup().getOrThrow())
     }
 }