Merge pull request #543 from synonymdev/feat/logs-polish

feat: Logs polish

Author: ovitrif

.github/workflows/claude-code-review.yml

@@ -51,8 +51,6 @@ jobs:
 
             Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback.
 
-            Use `gh pr comment` with your Bash tool to leave your review as a comment on the PR.
-
           # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md
           # or https://code.claude.com/docs/en/cli-reference for available options
-          claude_args: '--allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*)"'
+          claude_args: '--allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*)"'

AGENTS.md

@@ -192,7 +192,7 @@ suspend fun getData(): Result<Data> = withContext(Dispatchers.IO) {
 - ALWAYS be mindful of thread safety when working with mutable lists & state
 - ALWAYS split screen composables into parent accepting viewmodel + inner private child accepting state and callbacks `Content()`
 - ALWAYS name lambda parameters in a composable function using present tense, NEVER use past tense
-- ALWAYS list 3 suggested commit messages after implementation work
+- ALWAYS list 3 suggested commit messages after implementation work for the entire set of uncommitted changes
 - NEVER use `wheneverBlocking` in unit test expression body functions wrapped in a `= test {}` lambda
 - ALWAYS wrap unit tests `setUp` methods mocking suspending calls with `runBlocking`, e.g `setUp() = runBlocking { }`
 - ALWAYS add business logic to Repository layer via methods returning `Result<T>` and use it in ViewModels

app/build.gradle.kts

@@ -45,8 +45,8 @@ android {
         applicationId = "to.bitkit"
         minSdk = 28
         targetSdk = 36
-        versionCode = 16
-        versionName = "0.0.16"
+        versionCode = 17
+        versionName = "0.0.17"
         testInstrumentationRunner = "to.bitkit.test.HiltTestRunner"
         vectorDrawables {
             useSupportLibrary = true

app/detekt-baseline.xml

@@ -33,10 +33,8 @@
     <ID>EnumNaming:BlocktankNotificationType.kt$BlocktankNotificationType$orderPaymentConfirmed</ID>
     <ID>EnumNaming:BlocktankNotificationType.kt$BlocktankNotificationType$wakeToTimeout</ID>
     <ID>ForbiddenComment:ActivityDetailScreen.kt$/* TODO: Implement assign functionality */</ID>
-    <ID>ForbiddenComment:ActivityRow.kt$// TODO: calculate confirmsIn text</ID>
     <ID>ForbiddenComment:BoostTransactionViewModel.kt$BoostTransactionUiState$// TODO: Implement dynamic time estimation</ID>
     <ID>ForbiddenComment:ContentView.kt$// TODO: display as sheet</ID>
-    <ID>ForbiddenComment:Env.kt$Env$// TODO: remove this to load from BT API instead</ID>
     <ID>ForbiddenComment:ExternalNodeViewModel.kt$ExternalNodeViewModel$// TODO: pass customFeeRate to ldk-node when supported</ID>
     <ID>ForbiddenComment:LightningConnectionsViewModel.kt$LightningConnectionsViewModel$// TODO: sort channels to get consistent index; node.listChannels returns a list in random order</ID>
     <ID>ForbiddenComment:LightningService.kt$LightningService$// TODO: cleanup sensitive data after implementing a `SecureString` value holder for Keychain return values</ID>
@@ -60,15 +58,12 @@
     <ID>LongParameterList:CoreService.kt$OnchainService$( mnemonicPhrase: String, derivationPathStr: String?, network: Network?, bip39Passphrase: String?, isChange: Boolean?, startIndex: UInt?, count: UInt?, )</ID>
     <ID>LongParameterList:WidgetsRepo.kt$WidgetsRepo$( @BgDispatcher private val bgDispatcher: CoroutineDispatcher, private val newsService: NewsService, private val factsService: FactsService, private val blocksService: BlocksService, private val weatherService: WeatherService, private val priceService: PriceService, private val widgetsStore: WidgetsStore, private val settingsStore: SettingsStore, )</ID>
     <ID>LoopWithTooManyJumpStatements:MonetaryVisualTransformation.kt$MonetaryVisualTransformation.&lt;no name provided&gt;$for</ID>
-    <ID>MagicNumber:ActivityDetailScreen.kt$40</ID>
-    <ID>MagicNumber:ActivityExploreScreen.kt$40</ID>
     <ID>MagicNumber:ActivityListViewModel.kt$ActivityListViewModel$300</ID>
     <ID>MagicNumber:AddressViewerScreen.kt$1500000L</ID>
     <ID>MagicNumber:AddressViewerScreen.kt$250000L</ID>
     <ID>MagicNumber:AddressViewerViewModel.kt$AddressViewerViewModel$300</ID>
     <ID>MagicNumber:AppStatus.kt$0.4f</ID>
     <ID>MagicNumber:ArticleModel.kt$24</ID>
-    <ID>MagicNumber:ArticleModel.kt$30</ID>
     <ID>MagicNumber:ArticleModel.kt$60</ID>
     <ID>MagicNumber:BackupNavSheetViewModel.kt$BackupNavSheetViewModel$200</ID>
     <ID>MagicNumber:ChannelDetailScreen.kt$1.5f</ID>
@@ -97,7 +92,6 @@
     <ID>MagicNumber:ShowMnemonicScreen.kt$12</ID>
     <ID>MagicNumber:ShowMnemonicScreen.kt$24</ID>
     <ID>MagicNumber:ShowMnemonicScreen.kt$300</ID>
-    <ID>MagicNumber:Slider.kt$20</ID>
     <ID>MagicNumber:SpendingConfirmScreen.kt$300</ID>
     <ID>MagicNumber:SwipeToConfirm.kt$1500</ID>
     <ID>MatchingDeclarationName:AddressType.kt$AddressTypeInfo</ID>
@@ -118,7 +112,6 @@
     <ID>MaxLineLength:HeadlineCard.kt$headline = "How Bitcoin changed El Salvador in more ways a big headline to test the text overflooooooow"</ID>
     <ID>MaxLineLength:LightningBalance.kt$is LightningBalance.ClaimableAwaitingConfirmations -&gt; "Claimable Awaiting Confirmations (Height: $confirmationHeight)"</ID>
     <ID>MaxLineLength:LightningConnectionsScreen.kt$if (showClosed) R.string.lightning__conn_closed_hide else R.string.lightning__conn_closed_show</ID>
-    <ID>MaxLineLength:LightningRepo.kt$LightningRepo$"Cannot execute $operationName: Node is ${_lightningState.value.nodeLifecycleState} and not starting"</ID>
     <ID>MaxLineLength:LightningRepo.kt$LightningRepo$"accelerateByCpfp error originalTxId: $originalTxId, satsPerVByte: $satsPerVByte destinationAddress: $destinationAddress"</ID>
     <ID>MaxLineLength:LightningRepo.kt$LightningRepo$"accelerateByCpfp success, newDestinationTxId: $newDestinationTxId originalTxId: $originalTxId, satsPerVByte: $satsPerVByte destinationAddress: $destinationAddress"</ID>
     <ID>MaxLineLength:LightningRepo.kt$LightningRepo$"bumpFeeByRbf success, replacementTxId: $replacementTxId originalTxId: $originalTxId, satsPerVByte: $satsPerVByte"</ID>
@@ -144,14 +137,11 @@
     <ID>MaximumLineLength:SettingsScreen.kt$ </ID>
     <ID>MaximumLineLength:WeatherService.kt$WeatherService$ </ID>
     <ID>MayBeConst:Env.kt$Env$val walletSyncIntervalSecs = 10_uL // TODO review</ID>
-    <ID>MayBeConst:Env.kt$Env.TransactionDefaults$/** * Minimum value in sats for an output. Outputs below the dust limit may not be processed because the fees * required to include them in a block would be greater than the value of the transaction itself. * */ val dustLimit = 546u</ID>
-    <ID>MayBeConst:Env.kt$Env.TransactionDefaults$/** Total recommended tx base fee in sats */ val recommendedBaseFee = 256u</ID>
     <ID>MemberNameEqualsClassName:Keychain.kt$Keychain$private val keychain = context.keychainDataStore</ID>
     <ID>NestedBlockDepth:Context.kt$fun Context.copyAssetToStorage(asset: String, dest: String)</ID>
     <ID>NestedBlockDepth:LogsRepo.kt$LogsRepo$private fun createZipBase64(logFiles: List&lt;LogFile&gt;): String</ID>
     <ID>NestedBlockDepth:MonetaryVisualTransformation.kt$MonetaryVisualTransformation$private fun createOffsetMapping(original: String, transformed: String): OffsetMapping</ID>
     <ID>NestedBlockDepth:ShopWebViewInterface.kt$ShopWebViewInterface$@JavascriptInterface fun postMessage(message: String)</ID>
-    <ID>NoUnusedImports:ActivityDetailScreen.kt$to.bitkit.ui.screens.wallets.activity.ActivityDetailScreen.kt</ID>
     <ID>NoWildcardImports:LightningChannel.kt$import androidx.compose.foundation.layout.*</ID>
     <ID>PrintStackTrace:ShareSheet.kt$e</ID>
     <ID>ReturnCount:AppViewModel.kt$AppViewModel$private suspend fun handleSanityChecks(amountSats: ULong)</ID>

app/src/main/java/to/bitkit/fcm/WakeNodeWorker.kt

@@ -37,7 +37,7 @@ import to.bitkit.repositories.LightningRepo
 import to.bitkit.services.CoreService
 import to.bitkit.ui.pushNotification
 import to.bitkit.utils.Logger
-import to.bitkit.utils.withPerformanceLogging
+import to.bitkit.utils.measured
 import kotlin.time.Duration.Companion.minutes
 
 @Suppress("LongParameterList")
@@ -74,7 +74,7 @@ class WakeNodeWorker @AssistedInject constructor(
         Logger.debug("${this::class.simpleName} notification payload: $notificationPayload")
 
         try {
-            withPerformanceLogging {
+            measured(TAG) {
                 lightningRepo.start(
                     walletIndex = 0,
                     timeout = timeout,
@@ -252,4 +252,8 @@ class WakeNodeWorker @AssistedInject constructor(
 
         deliverSignal.complete(Unit)
     }
+
+    companion object {
+        private const val TAG = "WakeNodeWorker"
+    }
 }

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

@@ -119,28 +119,27 @@ class LightningRepo @Inject constructor(
     ): Result<T> = withContext(bgDispatcher) {
         Logger.verbose("Operation called: $operationName", context = TAG)
 
-        if (_lightningState.value.nodeLifecycleState.isRunning()) {
+        val nodeLifecycleState = _lightningState.value.nodeLifecycleState
+        if (nodeLifecycleState.isRunning()) {
             return@withContext executeOperation(operationName, operation)
         }
 
         // If node is not in a state that can become running, fail fast
-        if (!_lightningState.value.nodeLifecycleState.canRun()) {
+        if (!nodeLifecycleState.canRun()) {
             return@withContext Result.failure(
-                Exception(
-                    "Cannot execute $operationName: Node is ${_lightningState.value.nodeLifecycleState} and not starting"
-                )
+                Exception("Cannot execute '$operationName': node is '$nodeLifecycleState' and not starting")
             )
         }
 
         val nodeRunning = withTimeoutOrNull(waitTimeout) {
-            if (_lightningState.value.nodeLifecycleState.isRunning()) {
+            if (nodeLifecycleState.isRunning()) {
                 return@withTimeoutOrNull true
             }
 
             // Otherwise, wait for it to transition to running state
-            Logger.verbose("Waiting for node runs to execute $operationName", context = TAG)
+            Logger.verbose("Waiting for node to run before executing '$operationName'", context = TAG)
             _lightningState.first { it.nodeLifecycleState.isRunning() }
-            Logger.debug("Operation executed: $operationName", context = TAG)
+            Logger.debug("Operation executed: '$operationName'", context = TAG)
             true
         } ?: false
 
@@ -159,7 +158,7 @@ class LightningRepo @Inject constructor(
             // Cancellation is expected during pull-to-refresh, rethrow per Kotlin best practices
             throw e
         } catch (e: Throwable) {
-            Logger.error("$operationName error", e, context = TAG)
+            Logger.error("Error executing '$operationName'", e, context = TAG)
             Result.failure(e)
         }
     }
@@ -312,7 +311,7 @@ class LightningRepo @Inject constructor(
         }
     }
 
-    suspend fun sync(): Result<Unit> = executeWhenNodeRunning("Sync") {
+    suspend fun sync(): Result<Unit> = executeWhenNodeRunning("sync") {
         // If sync is in progress, mark pending and skip
         if (!syncMutex.tryLock()) {
             syncPending.set(true)
@@ -531,7 +530,7 @@ class LightningRepo @Inject constructor(
         return@withContext Result.success(Unit)
     }
 
-    suspend fun connectToTrustedPeers(): Result<Unit> = executeWhenNodeRunning("Connect to trusted peers") {
+    suspend fun connectToTrustedPeers(): Result<Unit> = executeWhenNodeRunning("connectToTrustedPeers") {
         lightningService.connectToTrustedPeers()
         Result.success(Unit)
     }
@@ -544,13 +543,13 @@ class LightningRepo @Inject constructor(
         Result.success(Unit)
     }
 
-    suspend fun disconnectPeer(peer: PeerDetails): Result<Unit> = executeWhenNodeRunning("Disconnect peer") {
+    suspend fun disconnectPeer(peer: PeerDetails): Result<Unit> = executeWhenNodeRunning("disconnectPeer") {
         lightningService.disconnectPeer(peer)
         syncState()
         Result.success(Unit)
     }
 
-    suspend fun newAddress(): Result<String> = executeWhenNodeRunning("New address") {
+    suspend fun newAddress(): Result<String> = executeWhenNodeRunning("newAddress") {
         val address = lightningService.newAddress()
         Result.success(address)
     }
@@ -559,7 +558,7 @@ class LightningRepo @Inject constructor(
         amountSats: ULong? = null,
         description: String,
         expirySeconds: UInt = 86_400u,
-    ): Result<String> = executeWhenNodeRunning("Create invoice") {
+    ): Result<String> = executeWhenNodeRunning("createInvoice") {
         updateGeoBlockState()
         val invoice = lightningService.receive(amountSats, description, expirySeconds)
         Result.success(invoice)
@@ -631,12 +630,14 @@ class LightningRepo @Inject constructor(
         Logger.error("Error requesting lnurl auth, k1: $k1, callback: $callback, domain: $domain", it)
     }
 
-    suspend fun payInvoice(bolt11: String, sats: ULong? = null): Result<PaymentId> =
-        executeWhenNodeRunning("Pay invoice") {
-            val paymentId = lightningService.send(bolt11 = bolt11, sats = sats)
-            syncState()
-            Result.success(paymentId)
-        }
+    suspend fun payInvoice(
+        bolt11: String,
+        sats: ULong? = null,
+    ): Result<PaymentId> = executeWhenNodeRunning("payInvoice") {
+        val paymentId = lightningService.send(bolt11 = bolt11, sats = sats)
+        syncState()
+        Result.success(paymentId)
+    }
 
     @Suppress("LongParameterList")
     suspend fun sendOnChain(
@@ -649,46 +650,45 @@ class LightningRepo @Inject constructor(
         channelId: String? = null,
         isMaxAmount: Boolean = false,
         tags: List<String> = emptyList(),
-    ): Result<Txid> =
-        executeWhenNodeRunning("sendOnChain") {
-            require(address.isNotEmpty()) { "Send address cannot be empty" }
+    ): Result<Txid> = executeWhenNodeRunning("sendOnChain") {
+        require(address.isNotEmpty()) { "Send address cannot be empty" }
 
-            val transactionSpeed = speed ?: settingsStore.data.first().defaultTransactionSpeed
-            val satsPerVByte = getFeeRateForSpeed(transactionSpeed, feeRates).getOrThrow().toUInt()
+        val transactionSpeed = speed ?: settingsStore.data.first().defaultTransactionSpeed
+        val satsPerVByte = getFeeRateForSpeed(transactionSpeed, feeRates).getOrThrow().toUInt()
 
-            // if utxos are manually specified, use them, otherwise run auto coin select if enabled
-            val finalUtxosToSpend = utxosToSpend ?: determineUtxosToSpend(
-                sats = sats,
-                satsPerVByte = satsPerVByte,
-            )
+        // if utxos are manually specified, use them, otherwise run auto coin select if enabled
+        val finalUtxosToSpend = utxosToSpend ?: determineUtxosToSpend(
+            sats = sats,
+            satsPerVByte = satsPerVByte,
+        )
 
-            Logger.debug("UTXOs selected to spend: $finalUtxosToSpend", context = TAG)
+        Logger.debug("UTXOs selected to spend: $finalUtxosToSpend", context = TAG)
 
-            val txId = lightningService.send(
-                address = address,
-                sats = sats,
-                satsPerVByte = satsPerVByte,
-                utxosToSpend = finalUtxosToSpend,
-                isMaxAmount = isMaxAmount
-            )
+        val txId = lightningService.send(
+            address = address,
+            sats = sats,
+            satsPerVByte = satsPerVByte,
+            utxosToSpend = finalUtxosToSpend,
+            isMaxAmount = isMaxAmount
+        )
 
-            val preActivityMetadata = PreActivityMetadata(
-                paymentId = txId,
-                createdAt = nowTimestamp().toEpochMilli().toULong(),
-                tags = tags,
-                paymentHash = null,
-                txId = txId,
-                address = address,
-                isReceive = false,
-                feeRate = satsPerVByte.toULong(),
-                isTransfer = isTransfer,
-                channelId = channelId ?: "",
-            )
-            preActivityMetadataRepo.addPreActivityMetadata(preActivityMetadata)
+        val preActivityMetadata = PreActivityMetadata(
+            paymentId = txId,
+            createdAt = nowTimestamp().toEpochMilli().toULong(),
+            tags = tags,
+            paymentHash = null,
+            txId = txId,
+            address = address,
+            isReceive = false,
+            feeRate = satsPerVByte.toULong(),
+            isTransfer = isTransfer,
+            channelId = channelId ?: "",
+        )
+        preActivityMetadataRepo.addPreActivityMetadata(preActivityMetadata)
 
-            syncState()
-            Result.success(txId)
-        }
+        syncState()
+        Result.success(txId)
+    }
 
     suspend fun determineUtxosToSpend(
         sats: ULong,
@@ -731,7 +731,7 @@ class LightningRepo @Inject constructor(
         Result.success(payments)
     }
 
-    suspend fun getAddressBalance(address: String): Result<ULong> = executeWhenNodeRunning("Get address balance") {
+    suspend fun getAddressBalance(address: String): Result<ULong> = executeWhenNodeRunning("getAddressBalance") {
         runCatching {
             lightningService.getAddressBalance(address)
         }
@@ -787,7 +787,7 @@ class LightningRepo @Inject constructor(
 
     suspend fun calculateCpfpFeeRate(
         parentTxId: Txid,
-    ): Result<ULong> = executeWhenNodeRunning("Calculate CPFP fee rate") {
+    ): Result<ULong> = executeWhenNodeRunning("calculateCpfpFeeRate") {
         Result.success(lightningService.calculateCpfpFeeRate(parentTxid = parentTxId).toSatPerVbCeil())
     }
 
@@ -796,7 +796,7 @@ class LightningRepo @Inject constructor(
         channelAmountSats: ULong,
         pushToCounterpartySats: ULong? = null,
         channelConfig: ChannelConfig? = null,
-    ): Result<OpenChannelResult> = executeWhenNodeRunning("Open channel") {
+    ): Result<OpenChannelResult> = executeWhenNodeRunning("openChannel") {
         val result = lightningService.openChannel(peer, channelAmountSats, pushToCounterpartySats, channelConfig)
         syncState()
         result
@@ -931,25 +931,19 @@ class LightningRepo @Inject constructor(
         try {
             if (originalTxId.isBlank()) {
                 return@executeWhenNodeRunning Result.failure(
-                    IllegalArgumentException(
-                        "originalTxId is null or empty: $originalTxId"
-                    )
+                    IllegalArgumentException("originalTxId is null or empty: $originalTxId")
                 )
             }
 
             if (destinationAddress.isBlank()) {
                 return@executeWhenNodeRunning Result.failure(
-                    IllegalArgumentException(
-                        "destinationAddress is null or empty: $destinationAddress"
-                    )
+                    IllegalArgumentException("destinationAddress is null or empty: $destinationAddress")
                 )
             }
 
             if (satsPerVByte <= 0u) {
                 return@executeWhenNodeRunning Result.failure(
-                    IllegalArgumentException(
-                        "satsPerVByte invalid: $satsPerVByte"
-                    )
+                    IllegalArgumentException("satsPerVByte invalid: $satsPerVByte")
                 )
             }