Fix: topups - #1428
Conversation
WalkthroughUpdates include a DPP version bump, refined service binding/unbinding handling, a one-time guard for platform top-up checks, expanded top-up confidence/timeout logic, a parameter adjustment in transaction result binding, and small UI condition/visibility tweaks in ExploreDash. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant PlatformSyncService as PlatformSynchronizationService
participant TopUpRepo as TopUpRepository
App->>PlatformSyncService: checkTopUps()
alt First invocation (hasCheckedTopups == false)
PlatformSyncService->>PlatformSyncService: set hasCheckedTopups = true
PlatformSyncService->>TopUpRepo: checkTopUps(key)
TopUpRepo-->>PlatformSyncService: result/logs
else Subsequent invocations
PlatformSyncService-->>App: no-op
end
sequenceDiagram
autonumber
participant Repo as TopUpRepository
participant Network as P2P/Chain
participant DB as Local DB
Repo->>Network: Submit or detect top-up tx
Note over Repo: Compute tx status (mined / peer-broadcast / other)
alt Already submitted or BUILDING
Repo->>Repo: wait up to 30s for ISLock or block
par Observe ISLock
Network-->>Repo: IX/ChainLock signal
and Observe block
Network-->>Repo: Block confirmation
end
Note over Repo: Cancel listeners on completion or timeout
end
Repo->>DB: Insert/Update top-up record (including already-submitted)
Repo-->>Repo: Log outcomes and exceptions (selectively swallowed)
sequenceDiagram
autonumber
participant UI as BlockListFragment
participant OS as Android OS
participant Svc as BlockchainService
UI->>OS: bindService(...)
OS-->>UI: onServiceConnected
UI->>UI: serviceIsBound = true
OS-->>UI: onServiceDisconnected
UI->>UI: serviceIsBound = false
UI->>OS: onDestroy -> unbindService()
alt serviceIsBound == true
UI->>OS: unbindService (try/catch IllegalArgumentException)
UI->>UI: serviceIsBound = false
else not bound
UI-->>UI: skip unbind
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (2)
400-418: setReturns() hides outputs and adds blank rows; fix visibility + filter
- Overwrites
outputsContainer.isVisible, hiding legitimate outputs if there’s no OP RETURN.- Adds empty rows for non “OP RETURN” items.
Apply:
- binding.outputsContainer.isVisible = outputOpReturns.isNotEmpty() && outputOpReturns.contains("OP RETURN") - outputOpReturns.forEach { + val showReturns = outputOpReturns.any { it == "OP RETURN" } + binding.outputsContainer.isVisible = binding.outputsContainer.isVisible || showReturns + outputOpReturns.forEach { + if (it != "OP RETURN") return@forEach val addressView = inflater.inflate( R.layout.transaction_result_address_row, binding.transactionOutputAddressesContainer, false ) as TextView - addressView.text = when (it) { - "OP RETURN" -> when { - error -> context.getString(R.string.platform_credits_error) - completed -> context.getString(R.string.platform_credits) - else -> context.getString(R.string.platform_credits_not_transferred) - } - else -> "" - } + addressView.text = when { + error -> context.getString(R.string.platform_credits_error) + completed -> context.getString(R.string.platform_credits) + else -> context.getString(R.string.platform_credits_not_transferred) + } + addressView.tag = "returnsRow" binding.transactionOutputAddressesContainer.addView(addressView) }
420-423: setSentToReturn() nukes all outputs; remove only returns rows
removeAllViews()clears address rows too. Remove only the returns rows you added.- binding.transactionOutputAddressesContainer.removeAllViews() + val container = binding.transactionOutputAddressesContainer + for (i in container.childCount - 1 downTo 0) { + val child = container.getChildAt(i) + if (child.tag == "returnsRow") { + container.removeViewAt(i) + } + } setReturns(outputAssetLocks, LayoutInflater.from(context), error, completed)wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
200-216: Fix possible NPE in getWalletSeed() when password retrieval fails
deriveKey(password)is called even ifpasswordis null on retrieval failure, which will crash. Mirror the null-guard you used above.Apply this diff:
- val encryptionKey = wallet.keyCrypter!!.deriveKey(password) + val encryptionKey = password?.let { wallet.keyCrypter!!.deriveKey(it) } ?: return null wallet.keyChainSeed.decrypt(wallet.keyCrypter, "", encryptionKey)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (1)
148-151: Fix LiveData.observeForever leak.observeForever requires explicit removal; otherwise the ViewModel can leak.
Apply:
+ private val balanceObserver = Observer<Coin> { coin -> + savedStateHandle[BALANCE_KEY] = coin?.value + } @@ - _balance.observeForever { coin -> - savedStateHandle[BALANCE_KEY] = coin?.value - } + _balance.observeForever(balanceObserver) + } + + override fun onCleared() { + _balance.removeObserver(balanceObserver) + super.onCleared() }Ensure Observer is imported (androidx.lifecycle.Observer).
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
222-224: Handler/thread-safety and retry cleanup.
- Handler() without a Looper is deprecated; bind to main looper.
- pendingForegroundNotification is accessed across threads; mark volatile or confine to main.
- Remove retry callbacks on destroy to avoid leaks.
- Fix “foregrxound” typo.
Apply:
+import android.os.Looper @@ - private val handler = Handler() - private val delayHandler = Handler() + private val handler = Handler(Looper.getMainLooper()) + private val delayHandler = Handler(Looper.getMainLooper()) @@ - private var pendingForegroundNotification: Notification? = null + @Volatile private var pendingForegroundNotification: Notification? = null @@ - // Schedule a retry after a few seconds to see if the app comes to foregrxound + // Schedule a retry after a few seconds to see if the app comes to foreground handler.postDelayed({ if (pendingForegroundNotification != null && Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { try { startForeground(pendingForegroundNotification!!) pendingForegroundNotification = null log.info("Successfully started foreground service on retry") } catch (e: ForegroundServiceStartNotAllowedException) { log.info("Foreground service start still not allowed, will continue as background service") pendingForegroundNotification = null } } }, 5000) // Retry after 5 seconds @@ - if (wakeLock!!.isHeld) { + if (wakeLock!!.isHeld) { log.debug("wakelock still held, releasing") wakeLock!!.release() } + // also clear any scheduled foreground retry + handler.removeCallbacksAndMessages(null)Also applies to: 252-253, 1323-1339, 1400-1401
🧹 Nitpick comments (15)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (3)
265-276: LocaliconResshadows the property; rename for clarityAvoid shadowing to reduce confusion.
- val iconRes = if (isError) { + val effectiveIconRes = if (isError) { R.drawable.ic_transaction_failed } else if (iconRes != null) { iconRes!! } else if (transaction.getValue(wallet).signum() >= 0) { R.drawable.ic_transaction_received } else if (transaction.isEntirelySelf(wallet)) { R.drawable.ic_internal } else { R.drawable.ic_transaction_sent } @@ - binding.secondaryIcon.setImageResource(iconRes) + binding.secondaryIcon.setImageResource(effectiveIconRes) @@ - binding.checkIcon.setImageResource(iconRes) + binding.checkIcon.setImageResource(effectiveIconRes) @@ - binding.secondaryIcon.setImageResource(iconRes) + binding.secondaryIcon.setImageResource(effectiveIconRes)Also applies to: 284-285, 300-302
218-236: Null‑safety: avoid!!onstrResourceMap lookups can return null. Use a safe fallback to prevent crashes.
val resId = (transactionMetadata.taxCategory?.let { taxCategoryNames[it] } ?: taxCategoryNames[transactionMetadata.defaultTaxCategory]) ?: return // or set a sensible default text/visibility binding.taxCategory.text = context.getString(resId)
208-216:isErroris never updated; icon may not reflect error state on confidence changesSet it from tx confidence to keep UI consistent.
- private fun setTransactionDirection(tx: Transaction, wallet: Wallet) { - if (tx.confidence.hasErrors()) { + private fun setTransactionDirection(tx: Transaction, wallet: Wallet) { + isError = tx.confidence.hasErrors() + if (isError) { ... } else { ... } }Also applies to: 305-360
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
128-135: Remove redundant!!read that can throw an NPE and hides the intended errorThis double-read performs
get(... )!!and discards it, risking an NPE before the clearerIllegalStateException.Apply this diff:
- blockchainIdentityDataStorage.get(BlockchainIdentityConfig.IDENTITY_ID)!! - blockchainIdentityDataStorage.get(BlockchainIdentityConfig.IDENTITY_ID) - ?: throw IllegalStateException("IdentityId not found") + blockchainIdentityDataStorage.get(BlockchainIdentityConfig.IDENTITY_ID) + ?: throw IllegalStateException("IdentityId not found")integrations/coinbase/src/main/res/layout/dialog_coinbase_result.xml (1)
7-7: Consider WindowInsets over fitsSystemWindows.android:fitsSystemWindows is legacy and can behave inconsistently on gesture nav. Prefer handling insets via ViewCompat.setOnApplyWindowInsetsListener or Material components’ insets helpers.
wallet/res/layout/fragment_username_registration.xml (1)
7-7: Same note on insets handling.If possible, replace fitsSystemWindows with explicit WindowInsets handling for predictable top/bottom padding across devices.
wallet/src/de/schildbach/wallet/ui/send/SendCoinsFragment.kt (2)
175-201: Reduce noisy logs in hot path.updateView() is called frequently; log.info each time will spam release logs. Consider downgrading to debug or rate‑limiting.
Apply this diff:
- log.info("enterAmountViewModel.blockContinue = {}, viewModel.dryRunSuccessful.value = {}", enterAmountViewModel.blockContinue, viewModel.dryRunSuccessful.value) + if (log.isDebugEnabled) { + log.debug( + "blockContinue={}, dryRunSuccess={}", + enterAmountViewModel.blockContinue, + viewModel.dryRunSuccessful.value + ) + }
185-191: Avoid surfacing raw exception text to users.Showing dryRunException.toString() can leak low‑level/internal details. Prefer a generic, localized message and log the details.
- else -> dryRunException.toString() + else -> getString(R.string.send_coins_error_msg).also { + log.warn("Unhandled dry-run error: {}", dryRunException.message, dryRunException) + }features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (1)
72-75: Redact sensitive data in CTXSpendException.toString().errorBody may include PII/tokens. Also message can be blank when ResourceString is used. Include a fallback and cap/error‑redact the payload.
- override fun toString(): String { - return "CTX error: $message\n $giftCardResponse\n $errorCode: $errorBody" - } + override fun toString(): String { + val safeBody = errorBody + ?.replace(Regex("(?i)(authorization|token|api[-_ ]?key)[:=\\s]+[^,;\\s]+"), "$1=<redacted>") + ?.let { if (it.length > 500) it.take(500) + "…(truncated)" else it } + val msg = if (message.isNullOrBlank()) resourceString?.toString() ?: "<no message>" else message + val tx = txId?.let { " txId=$it" } ?: "" + return "CTX error: $msg$tx\n giftCard=$giftCardResponse\n code=$errorCode body=$safeBody" + }common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt (1)
133-141: Init via ViewModel state: OK, but avoid direct access to underscored fields.Good shift away from writing UI input directly. Prefer VM methods to keep encapsulation instead of touching
_amount/_fiatAmountfrom the Fragment.Consider small wrappers in EnterAmountViewModel:
- viewModel._amount.value = initialAmount + viewModel.setDashAmount(initialAmount) ... - viewModel._fiatAmount.value = initialAmount + viewModel.setFiatAmount(initialAmount)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt (1)
229-235: Include txid in analytics/error for better correlation.You pass
""as txid to CTXSpendException. Use the actualtxidfrom scope for richer telemetry.- analyticsService.logError( - CTXSpendException("CTXSpend returned error: rejected", giftCard, ""), + analyticsService.logError( + CTXSpendException("CTXSpend returned error: rejected", giftCard, txid), "CTX returned error: rejected ${giftCard.merchantName} for ${giftCard.fiatAmount} ${giftCard.fiatCurrency}" )wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
1400-1407: Make top‑ups “run once” thread‑safe.Ticker-driven
updateContactRequests()can overlap. Use AtomicBoolean CAS to avoid double execution.- private var hasCheckedTopups = false // only run once + private val hasCheckedTopups = java.util.concurrent.atomic.AtomicBoolean(false) // only run once private suspend fun checkTopUps() { - if (!hasCheckedTopups) { - platformRepo.getWalletEncryptionKey()?.let { - topUpRepository.checkTopUps(it) - hasCheckedTopups = true - } - } + if (hasCheckedTopups.get()) return + platformRepo.getWalletEncryptionKey()?.let { + if (hasCheckedTopups.compareAndSet(false, true)) { + topUpRepository.checkTopUps(it) + } + } }wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
344-346: Swallowed exceptions: at least log context.Silent catches obscure failures; log txid and exception at WARN.
- } catch (e: Exception) { - // swallow - } + } catch (e: Exception) { + log.warn("checkTopUps: failed processing unused topup {}", topUp.txId, e) + } ... - } catch (e: Exception) { - // swallow - } + } catch (e: Exception) { + log.warn("checkTopUps: failed processing historical topup {}", assetLockTx.txId, e) + }Also applies to: 363-365
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (1)
153-159: De-duplicate replay state emissions.Map to the boolean and distinct to avoid redundant UI updates.
Apply:
- blockchainStateProvider.observeState() - .filterNotNull() - .onEach { state -> - _isBlockchainReplaying.value = state.replaying - } - .launchIn(viewModelScope) + blockchainStateProvider.observeState() + .filterNotNull() + .map { it.replaying } + .distinctUntilChanged() + .onEach(_isBlockchainReplaying::emit) + .launchIn(viewModelScope)And add:
+import kotlinx.coroutines.flow.mapwallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
1080-1081: Avoid verifying block stores on the main thread.verifyBlockStores() may touch disk; keep off the UI thread.
Apply:
- withContext(Dispatchers.Main) { verifyBlockStores() } + verifyBlockStores()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
build.gradle(1 hunks)common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt(2 hunks)common/src/main/res/values/strings.xml(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(5 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt(7 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt(3 hunks)integrations/coinbase/src/main/res/layout/dialog_coinbase_result.xml(1 hunks)wallet/res/layout/fragment_username_registration.xml(1 hunks)wallet/res/values/strings.xml(0 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(7 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(1 hunks)wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/BlockListFragment.java(4 hunks)wallet/src/de/schildbach/wallet/ui/PeerListFragment.java(4 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/send/SendCoinsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/username/voting/RequestUserNameViewModel.kt(1 hunks)
💤 Files with no reviewable changes (1)
- wallet/res/values/strings.xml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
📚 Learning: 2025-05-08T18:11:40.249Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt
📚 Learning: 2025-08-08T16:48:49.964Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt
📚 Learning: 2025-08-25T15:02:39.634Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt:154-158
Timestamp: 2025-08-25T15:02:39.634Z
Learning: The CTXSpend getGiftCardByTxid API in CTXSpendRepository guarantees a non-null GiftCardResponse payload when returning ResponseResource.Success, so using response.value!! is safe and doesn't risk NPE despite the nullable type signature.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt
📚 Learning: 2025-05-07T14:18:11.161Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt
📚 Learning: 2025-05-06T15:46:59.440Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt
🧬 Code graph analysis (3)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
sendTransaction(1135-1189)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
wallet/src/de/schildbach/wallet/service/extensions/BlockchainServiceExt.kt (1)
registerCrowdNodeConfirmedAddressFilter(10-35)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendUserAuthFragment.kt (2)
showLoading(169-173)hideLoading(175-179)
🪛 detekt (1.23.8)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt
[warning] 300-300: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 363-363: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (20)
wallet/src/de/schildbach/wallet/ui/username/voting/RequestUserNameViewModel.kt (1)
346-356: Improved null safety handling with proper documentation.The change adds proper null safety by guarding against a potentially null
usernameRequestand includes a helpful comment explaining why the request might be null. This prevents potential crashes when the username request hasn't been persisted yet.wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (2)
162-165: Don’t hard‑code completed=true; derive from real top‑up statePassing
completed = trueunconditionally may mislabel Platform Credits. Consider deriving it (e.g., from tx confidence or actual top‑up/asset‑lock state) and pass that instead.
117-121: Confirm use of getOpReturnsOfSent() for received transactionsTransactionResultViewBinder.kt calls TransactionUtils.getOpReturnsOfSent(tx, wallet) in both branches (wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt:116,120) but TransactionUtils only defines getOpReturnsOfSent (common/src/main/java/org/dash/wallet/common/transactions/TransactionUtils.kt:100) and no getOpReturnsOfReceived exists. Confirm this is intentional (i.e., getOpReturnsOfSent returns the expected/empty data for received flows) or change to the correct API/behaviour.
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
194-194: Null-safe key derivation LGTMSwitching to
password?.let { ... }avoids an NPE when the password retrieval fails. Good fix.build.gradle (1)
7-7: SNAPSHOT dependency reduces build reproducibility.If 2.0.1-SNAPSHOT is required for this fix, plan to pin to a released version before tagging a production build. Consider enabling dependency locking to avoid silent drift.
Would you like a quick script to scan for other SNAPSHOTs and produce a lockfile proposal?
wallet/src/de/schildbach/wallet/ui/send/SendCoinsFragment.kt (1)
176-176: String source change LGTM.Using the common string for replaying state is a good centralization and avoids duplication across modules.
Please confirm InsufficientCoinJoinMoneyException is imported/visible in this scope; otherwise compilation will fail.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (3)
115-118: Nice: loading state wraps authentication.Early showLoading + hideLoading on auth cancel prevents stale spinners.
149-149: Good: log explicit “limits” error path.This will help triage customer limit issues with CTX.
173-173: Good: log explicit 500 path.Consistent with the new CTXSpendException.toString(), this should give actionable diagnostics.
common/src/main/res/values/strings.xml (1)
102-102: Centralize replaying hint — confirm duplicate removedCentralizing the replaying hint is good; confirm the wallet module's duplicate was removed to avoid resource collisions — repo search returned no files, so run to verify a single definition exists:
rg -n --hidden -uu --no-ignore 'send_coins_fragment_hint_replaying' || true # fallback: find . -type f \( -name 'strings.xml' -o -name '*.xml' \) -print0 | xargs -0 rg -n 'send_coins_fragment_hint_replaying' || truecommon/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt (1)
260-263: Single source of truth alignment: LGTM.Propagating amount changes back to VM is correct and consistent with the new init path.
wallet/src/de/schildbach/wallet/ui/BlockListFragment.java (1)
84-85: Guarded service unbinding: LGTM.Tracking
serviceIsBoundand guardingunbindService()prevents IllegalArgumentException during lifecycle churn.Also applies to: 115-117, 173-181, 219-221
wallet/src/de/schildbach/wallet/ui/PeerListFragment.java (1)
72-73: Guarded service unbinding: LGTM.Same binding flag pattern as Blocks; avoids IllegalArgumentException and cleans up state on disconnect.
Also applies to: 101-103, 167-175, 194-195
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (1)
382-405: Correct gating for fixed denominations: LGTM.
canContinue = !exceedsBalance() && !isReplaying.valuealigns with replay state.wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
307-313: Status logging: LGTM.Clearer post-send status helps support.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (2)
133-135: StateFlow exposure looks good.Public read-only StateFlow backed by private MutableStateFlow is idiomatic.
429-431: Centralized error logging is fine.Delegating to analytics here keeps UI light.
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (3)
1313-1318: Foreground start retry path is reasonable.Catching ForegroundServiceStartNotAllowedException and retrying later is a good compromise on S+.
Please confirm that scheduleRetryForegroundService is only ever called from the main thread post-initialization after applying the Handler/Looper fix above.
697-704: Mutex diagnostics are fine.Extra logs for acquisition/release help trace lifecycle races.
1253-1274: Broadcast path: confirm TransactionConfidence API usage — suspicious call to setPeerInfo(0, 1).rg shows only the call at wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt:1270 and no definition/extension of setPeerInfo in the repo; upstream bitcoinj’s TransactionConfidence API documents markBroadcastBy(...) and setConfidenceType(...), not setPeerInfo. Verify that setPeerInfo exists in your fork or replace this with the official confidence APIs (e.g., markBroadcastBy / setConfidenceType / TxConfidenceTable usage) to represent "seen by 0 peers / required 1". (bitcoinj.org)
| getWalletEncryptionKey()?.let { | ||
| val firstIdentityKey = getBlockchainIdentityKey(0, it) ?: return null | ||
| platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Regression: breaks non‑encrypted wallets in getIdentityFromPublicKeyId()
Gating the whole flow on a non‑null encryption key prevents identity lookup on non‑encrypted wallets (where a key isn’t needed). getBlockchainIdentityKey() already handles null KeyParameter when the wallet isn’t encrypted. Remove the guard and pass the (possibly null) key.
Apply this diff:
- getWalletEncryptionKey()?.let {
- val firstIdentityKey = getBlockchainIdentityKey(0, it) ?: return null
- platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash)
- }
+ val keyParam = getWalletEncryptionKey() // null for non-encrypted wallets
+ val firstIdentityKey = getBlockchainIdentityKey(0, keyParam) ?: return null
+ platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash)Please verify on a non‑encrypted wallet that identity resolution still works (e.g., top‑up/recovery path that depends on this).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo root: $(pwd)"
file="wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt"
if [ -f "$file" ]; then
echo "File exists: $file"
echo "---- lines 1000-1090 ----"
sed -n '1000,1090p' "$file" || true
echo "---- search for getWalletEncryptionKey occurrences ----"
rg -n --hidden --no-ignore -S "getWalletEncryptionKey" || true
echo "---- search for getBlockchainIdentityKey occurrences ----"
rg -n --hidden --no-ignore -S "getBlockchainIdentityKey" || true
echo "---- show function definitions mentioning getBlockchainIdentityKey ----"
rg -n --hidden --no-ignore -S "fun\s+getBlockchainIdentityKey" || true
echo "---- search for KeyParameter usages ----"
rg -n --hidden --no-ignore -S "KeyParameter" || true
echo "---- show surrounding lines for KeyParameter matches ----"
rg -n -C3 --hidden --no-ignore -S "KeyParameter" || true
else
echo "File not found: $file"
echo "Listing wallet/src..."
ls -la wallet/src || true
fiLength of output: 84395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt"
echo "---- PlatformRepo.kt (lines 150-220) ----"
sed -n '150,220p' "$file" || true
echo
echo "---- getIdentityFromPublicKeyId (lines 1036-1108) ----"
sed -n '1036,1108p' "$file" || true
echo
echo "---- search for toDecrypted occurrences ----"
rg -n --hidden --no-ignore -S "toDecrypted" || true
echo
echo "---- search for getIdentityFromPublicKeyId usages ----"
rg -n --hidden --no-ignore -S "getIdentityFromPublicKeyId" || trueLength of output: 7066
Unblock identity lookup for non‑encrypted wallets — don't gate on getWalletEncryptionKey()
getWalletEncryptionKey()?.let { … } skips lookup for non‑encrypted wallets. Use a keyParam = getWalletEncryptionKey() and call getBlockchainIdentityKey(0, keyParam) instead, but keep a safety check: if (walletApplication.wallet!!.isEncrypted && keyParam == null) return null (to avoid calling toDecrypted with a null key). File: wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt — function getIdentityFromPublicKeyId(). Verify identity resolution on a non‑encrypted wallet (top‑up/recovery paths).
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt around lines 1065
to 1068, the current code gates identity lookup on getWalletEncryptionKey()
which skips lookup for non‑encrypted wallets; change to capture keyParam =
getWalletEncryptionKey() and call getBlockchainIdentityKey(0, keyParam) (do not
wrap the entire block in keyParam?.let), but add a safety check: if
(walletApplication.wallet!!.isEncrypted && keyParam == null) return null to
avoid calling toDecrypted with a null key; then proceed to fetch the identity
via platform.stateRepository.fetchIdentityFromPubKeyHash(...) using the obtained
firstIdentityKey.
There was a problem hiding this comment.
this app doesn't use non-encrypted wallets
There was a problem hiding this comment.
@HashEngineering Thank you for the clarification! That completely resolves my concern. If the app doesn't use non-encrypted wallets, then getWalletEncryptionKey() will never return null in practice, making the null-safe chain with ?.let appropriate defensive programming rather than a regression.
The current implementation is correct for this app's context. I withdraw my previous suggestion since the scenario I was concerned about doesn't actually occur in this application.
✏️ Learnings added
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1428
File: wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt:1065-1068
Timestamp: 2025-09-23T05:19:47.850Z
Learning: The Dash wallet app doesn't use non-encrypted wallets, so getWalletEncryptionKey() will never return null in practice. Defensive null-safe programming with ?.let is appropriate for methods like getIdentityFromPublicKeyId() in PlatformRepo.kt.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
…fix/bip21-topups
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (2)
308-312: Avoid throw/catch on timeout; use withTimeoutOrNull to satisfy detekt.Prevents an expected control path from being flagged as a swallowed exception.
Apply:
- try { - withTimeout(TimeUnit.SECONDS.toMillis(30)) { - suspendCancellableCoroutine { continuation -> + val completed = withTimeoutOrNull(TimeUnit.SECONDS.toMillis(30)) { + suspendCancellableCoroutine { continuation -> ... - } - } - } catch (e: TimeoutCancellationException) { - // Timeout reached, continue with execution - log.info("topup, timeout waiting for islock, continue...") - } + } + } + if (completed == null) { + log.info("topup, timeout waiting for islock, continue...") + }
366-373: Don’t fully swallow; log at debug to help QA reproduce.Keeps the chill UX while addressing detekt warning and aiding diagnostics.
Apply:
- } catch (e: Exception) { - // swallow - } + } catch (e: Exception) { + log.debug("topup re-check failed for {}", assetLockTx.txId, e) + }Given past preference for production-friendly logging, this stays at debug and won’t spam info logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(1 hunks)wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
🧬 Code graph analysis (1)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
sendTransaction(1135-1189)
🪛 detekt (1.23.8)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt
[warning] 308-308: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 371-371: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (4)
261-266: BUILDING treated as “already sent” — confirm assumption.This avoids unnecessary rebroadcasts, and later waitForChainlock=true should still provide finality. Please confirm this matches expected UX for already-mined topups.
272-307: Listener leak fixed; small null/shadowing nit for listener removal.Fix looks correct (cancellable + cancellation cleanup). To avoid confusion between the outer confidence and the nullable callback parameter, remove listeners via the non-null owner to eliminate any null-risk and shadowing.
Apply this minimal tweak:
- confidence.removeEventListener(this) + topUpTx.confidence.removeEventListener(this) ... - confidence.removeEventListener(this) + topUpTx.confidence.removeEventListener(this) ... - continuation.invokeOnCancellation { - confidence.removeEventListener(listener) + continuation.invokeOnCancellation { + topUpTx.confidence.removeEventListener(listener)
315-321: Status logging reads well.Clear, actionable context in logs. LGTM.
335-335: Idempotent “already submitted” path is handled cleanly.Good to credit and continue without failing the flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt (1)
176-181: Microcopy typo.“thought it may be visible” → “though it may be visible”.
Apply this diff:
- binding.contactSupport.isVisible = true // force visible, thought it may be visible based on status + binding.contactSupport.isVisible = true // force visible, though it may be visible based on statusfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/ItemDetails.kt (1)
212-228: Avoid stale visibility and potential NPE on merchant.source.
- View reuse can leave
countryAvailabilityTextvisible for non‑CTX/non‑Dash items. Reset before branching.merchant.source!!can NPE; use a safe, case‑insensitive check.Apply this diff:
if (isDash) { payBtn.isVisible = true payBtnTxt.text = context.getText(R.string.explore_pay_with_dash) payBtn.setRoundedRippleBackground(R.style.PrimaryButtonTheme_Large_Blue) payBtn.setOnClickListener { onSendDashClicked?.invoke(true) } payBtn.isEnabled = merchant.active ?: true temporaryUnavailableText.isVisible = merchant.active == false - countryAvailabilityText.isVisible = false + countryAvailabilityText.isVisible = false - } else if (merchant.source!!.lowercase() == ServiceName.CTXSpend.lowercase()) { + } else if (merchant.source?.equals(ServiceName.CTXSpend, ignoreCase = true) == true) { payBtn.isVisible = true payBtnTxt.text = context.getText(R.string.explore_buy_gift_card) payBtn.setRoundedRippleBackground(R.style.PrimaryButtonTheme_Large_Orange) payBtn.setOnClickListener { onBuyGiftCardButtonClicked?.invoke() } payBtn.isEnabled = merchant.active ?: true temporaryUnavailableText.isVisible = merchant.active == false countryAvailabilityText.isVisible = true + } else { + // Ensure hidden for other sources/types + countryAvailabilityText.isVisible = false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/ItemDetails.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
📚 Learning: 2025-08-08T16:48:49.964Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/ItemDetails.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt (1)
161-165: Unreachable “10 tries” log after raising the error threshold to 20; align with a single constant.
shouldShowErrornow triggers atqueries >= 20, but the one-time log still checksqueries == 10, so it never fires. Use a shared constant and update the message.Apply this diff within the shown lines:
- "unpaid", "paid" -> state.queries >= 20 + "unpaid", "paid" -> state.queries >= MAX_POLL_TRIES @@ - if (state.queries == 10) { - ctxSpendViewModel.logError(state.error, "CTX did not deliver the card after 10 tries") + if (state.queries == MAX_POLL_TRIES) { + ctxSpendViewModel.logError(state.error, "CTX did not deliver the card after $MAX_POLL_TRIES tries") }And add this near other constants in the companion object:
private const val MAX_POLL_TRIES = 20#!/bin/bash # Verify no other mismatched thresholds or stale messages remain. rg -n -C2 -g '!**/build/**' -P '\bqueries\s*(==|>=|>|<=|<)\s*(\d+|MAX_POLL_TRIES)|MAX_POLL_TRIES|did not deliver the card after'Also applies to: 179-181
2 participants
Issue being fixed or feature implemented
Related PR's and Dependencies
dashpay/kotlin-platform#28
dashpay/dashj#286
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
Bug Fixes
Chores