diff --git a/app/src/main/java/to/bitkit/data/HwWalletStore.kt b/app/src/main/java/to/bitkit/data/HwWalletStore.kt index 08cd6ab191..d65fa6ff47 100644 --- a/app/src/main/java/to/bitkit/data/HwWalletStore.kt +++ b/app/src/main/java/to/bitkit/data/HwWalletStore.kt @@ -33,8 +33,54 @@ class HwWalletStore @Inject constructor( store.data.first().knownDevices } - suspend fun saveKnownDevices(devices: List) = withContext(ioDispatcher) { - store.updateData { it.copy(knownDevices = devices) } + /** + * @param pendingName a pending-name change to apply in the same write, or null to leave them alone. + * Splitting the two would publish a device list without its matching name change, which restarts a + * watcher for a wallet already being removed and can leave a name in both places or in neither. + */ + suspend fun saveKnownDevices( + devices: List, + pendingName: PendingNameUpdate? = null, + ) = withContext(ioDispatcher) { + store.updateData { data -> + data.copy( + knownDevices = devices, + pendingNames = pendingName?.applyTo(data.pendingNames) ?: data.pendingNames, + ) + } + Unit + } + + suspend fun loadPendingNames(): Map = withContext(ioDispatcher) { + store.data.first().pendingNames + } + + /** Stores the name of a wallet with no device entry, or drops it when [name] is null. */ + suspend fun setPendingName(walletId: String, name: String?) = withContext(ioDispatcher) { + if (walletId.isBlank()) return@withContext + store.updateData { data -> + val pendingNames = when { + name.isNullOrBlank() -> data.pendingNames - walletId + else -> data.pendingNames + (walletId to name) + } + data.copy(pendingNames = pendingNames) + } + Unit + } + + suspend fun backupSnapshot(): Map = withContext(ioDispatcher) { + store.data.first().hwWalletNames() + } + + /** + * Merges backed up names into the pending ones, so they are adopted the next time each wallet is + * paired. Names already held locally win: they were set on this device after the backup was written. + * + * Never clears: an envelope without names predates this field, and must not drop what is stored. + */ + suspend fun restoreNames(names: Map) = withContext(ioDispatcher) { + if (names.isEmpty()) return@withContext + store.updateData { it.copy(pendingNames = names + it.pendingNames) } Unit } @@ -44,7 +90,39 @@ class HwWalletStore @Inject constructor( } } +/** A pending-name change applied together with a device list write; a null [name] drops the entry. */ +data class PendingNameUpdate( + val walletId: String, + val name: String?, +) { + fun applyTo(pendingNames: Map): Map = when { + walletId.isBlank() -> pendingNames + name.isNullOrBlank() -> pendingNames - walletId + else -> pendingNames + (walletId to name) + } +} + @Serializable data class HwWalletData( val knownDevices: List = emptyList(), + /** + * Names of wallets with no [KnownDevice] entry, keyed by wallet id: restored from a backup before + * the device was paired again, or kept when the wallet was removed. Pairing consumes an entry into + * the device's own `customLabel`. + */ + val pendingNames: Map = emptyMap(), ) + +/** + * Every hardware wallet name this wallet knows, keyed by wallet id: the name of each paired wallet, + * overlaid on the pending ones. A paired name wins because it is what the user currently sees. + * + * Entries without a wallet id are skipped. Only a device stored before any account key was captured + * has none, and such an entry is filtered out of the wallet list, so it can never have been named. + */ +internal fun HwWalletData.hwWalletNames(): Map = + pendingNames + knownDevices.mapNotNull { device -> + val walletId = device.walletId.takeIf { it.isNotBlank() } ?: return@mapNotNull null + val name = device.customLabel?.takeIf { it.isNotBlank() } ?: return@mapNotNull null + walletId to name + } diff --git a/app/src/main/java/to/bitkit/models/BackupPayloads.kt b/app/src/main/java/to/bitkit/models/BackupPayloads.kt index 04dfeb68bb..d16bdab076 100644 --- a/app/src/main/java/to/bitkit/models/BackupPayloads.kt +++ b/app/src/main/java/to/bitkit/models/BackupPayloads.kt @@ -34,6 +34,8 @@ data class MetadataBackupV1( val cache: AppCacheData, val pubkySession: PubkySessionBackupV1? = null, val pubkyContactProfileOverrides: Map? = null, + /** User-set hardware wallet names, keyed by wallet id. Null in envelopes written before this field. */ + val hwWalletNames: Map? = null, ) @Serializable diff --git a/app/src/main/java/to/bitkit/repositories/ActivityRepo.kt b/app/src/main/java/to/bitkit/repositories/ActivityRepo.kt index 6385f5c1d8..f5460b37a5 100644 --- a/app/src/main/java/to/bitkit/repositories/ActivityRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/ActivityRepo.kt @@ -901,6 +901,27 @@ class ActivityRepo @Inject constructor( } } + /** + * The slice of the metadata backup's tag data that belongs to [walletId], built the same way the + * envelope builds it so a caller can preserve a wallet's tags across a deletion. + * + * Both sources are needed: Core drops the stored [PreActivityMetadata] of a wallet along with its + * activities, and those rows are not covered by [getHardwareTagsAsPreActivityMetadata], which only + * renders tags that already reached an activity. + */ + suspend fun getTagMetadataForWallet(walletId: String): Result> = + withContext(bgDispatcher) { + runSuspendCatching { + val stored = coreService.activity.getAllPreActivityMetadata() + .filter { it.walletId == walletId } + val rendered = getHardwareTagsAsPreActivityMetadata().getOrThrow() + .filter { it.walletId == walletId } + (stored + rendered).distinctBy { it.walletId to it.paymentId } + }.onFailure { + Logger.error("Failed to read tag metadata of '$walletId'", it, context = TAG) + } + } + /** * Fill in wallet ids missing from a backup envelope's `activities` slice, letting Core migrate its own * model JSON before the app decodes it. diff --git a/app/src/main/java/to/bitkit/repositories/BackupRepo.kt b/app/src/main/java/to/bitkit/repositories/BackupRepo.kt index c0ef32ee19..7c2c5b699d 100644 --- a/app/src/main/java/to/bitkit/repositories/BackupRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/BackupRepo.kt @@ -31,11 +31,13 @@ import to.bitkit.R import to.bitkit.async.appScope import to.bitkit.data.AppDb import to.bitkit.data.CacheStore +import to.bitkit.data.HwWalletStore import to.bitkit.data.SettingsStore import to.bitkit.data.WatchOnlyAccountStore import to.bitkit.data.WidgetsStore import to.bitkit.data.backup.VssBackupClient import to.bitkit.data.backup.VssBackupClientLdk +import to.bitkit.data.hwWalletNames import to.bitkit.data.resetPin import to.bitkit.di.IoDispatcher import to.bitkit.di.json @@ -91,6 +93,7 @@ class BackupRepo @Inject constructor( private val widgetsStore: WidgetsStore, private val watchOnlyAccountStore: WatchOnlyAccountStore, private val watchOnlyAccountRepo: WatchOnlyAccountRepo, + private val hwWalletStore: HwWalletStore, private val blocktankRepo: BlocktankRepo, private val activityRepo: ActivityRepo, private val pubkyRepo: PubkyRepo, @@ -298,6 +301,20 @@ class BackupRepo @Inject constructor( } dataListenerJobs.add(preActivityMetadataJob) + // METADATA - Observe hardware wallet names only: the store is also rewritten by every connect, + // and reconnect traffic must not re-upload the whole metadata envelope. + val hwWalletNamesJob = scope.launch { + hwWalletStore.data + .map { it.hwWalletNames() } + .distinctUntilChanged() + .drop(1) + .collect { + if (shouldSkipBackup()) return@collect + markBackupRequired(BackupCategory.METADATA) + } + } + dataListenerJobs.add(hwWalletNamesJob) + dataListenerJobs.add(observeBackupChanges(pubkyRepo.backupStateVersion, BackupCategory.METADATA)) dataListenerJobs.add(observeBackupChanges(privatePaykitRepo.get().backupStateVersion, BackupCategory.WALLET)) dataListenerJobs.add(observeBackupChanges(paykitSdkService.backupStateVersion, BackupCategory.WALLET)) @@ -548,6 +565,9 @@ class BackupRepo @Inject constructor( val hardwareTagMetadata = activityRepo.getHardwareTagsAsPreActivityMetadata().getOrThrow() val tagMetadata = (preActivityMetadata + hardwareTagMetadata) .distinctBy { it.walletId to it.paymentId } + // Like the tags above, this envelope is the only copy of the names, so a read failure must + // propagate and fail the backup rather than upload an empty set over the stored ones. + val hwWalletNames = hwWalletStore.backupSnapshot().takeIf { it.isNotEmpty() } val cacheData = cacheStore.data.first() val pubkySession = pubkyRepo.snapshotSessionBackupState().getOrDefault(null) val pubkyContactProfileOverrides = pubkyRepo.snapshotContactProfileOverrides().getOrDefault(null) @@ -558,6 +578,7 @@ class BackupRepo @Inject constructor( cache = cacheData, pubkySession = pubkySession, pubkyContactProfileOverrides = pubkyContactProfileOverrides, + hwWalletNames = hwWalletNames, ) json.encodeToString(payload).toByteArray() @@ -661,7 +682,13 @@ class BackupRepo @Inject constructor( .onFailure { Logger.warn("Failed to restore pubky contact profile overrides", it, context = TAG) } + // App-owned, so it takes no part in the Core field migration above and never sets needsRewrite. + // Restored names wait as pending ones until each wallet is paired again. Failing to store them + // must not discard the rest of this envelope, which has already been applied by here. + runSuspendCatching { hwWalletStore.restoreNames(parsed.hwWalletNames.orEmpty()) } + .onFailure { Logger.warn("Failed to restore hardware wallet names", it, context = TAG) } Logger.debug("Restored ${parsed.tagMetadata.size} pre-activity metadata", TAG) + Logger.debug("Restored ${parsed.hwWalletNames.orEmpty().size} hardware wallet names", TAG) return RestoredCoreBackup(createdAt = parsed.createdAt, needsRewrite = migration.changed && persisted) } diff --git a/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt b/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt index 4e368f709e..3cb350284c 100644 --- a/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt @@ -33,6 +33,7 @@ import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import to.bitkit.async.appScope import to.bitkit.data.HwWalletStore +import to.bitkit.data.PendingNameUpdate import to.bitkit.data.SettingsStore import to.bitkit.di.IoDispatcher import to.bitkit.env.Env @@ -77,6 +78,7 @@ import kotlin.time.Duration.Companion.seconds class HwWalletRepo @Inject constructor( private val trezorRepo: TrezorRepo, private val activityRepo: ActivityRepo, + private val preActivityMetadataRepo: PreActivityMetadataRepo, private val hwWalletStore: HwWalletStore, private val settingsStore: SettingsStore, @IoDispatcher private val ioDispatcher: CoroutineDispatcher, @@ -297,9 +299,11 @@ class HwWalletRepo @Inject constructor( Logger.warn("Rejected hardware session for '$walletId': opened wallet '$opened'", context = TAG) // Reading the accounts of the wrong wallet already stored it; a mistyped passphrase - // must not leave a stray watch-only wallet behind. + // must not leave a stray watch-only wallet behind. Its backup data is kept: the wallet + // is a real one the user owns, and storing it has already consumed any name restored + // for it into the entry about to be forgotten. if (opened !in watchedBefore) { - removeDevice(opened) + removeDevice(opened, keepBackupData = true) .onFailure { Logger.warn("Failed to drop unwatched wallet '$opened'", it, context = TAG) } } trezorRepo.disconnectStaleSession(deviceId) @@ -444,8 +448,15 @@ class HwWalletRepo @Inject constructor( * is stored once per transport but shares an xpub-derived identity, so forgetting a single id * would leave the tile reappearing through the other transport. Other identities on the same * device — the standard wallet, or another passphrase wallet — are left paired. + * + * @param keepBackupData whether to carry the wallet's name and tags in the backup, so re-pairing + * the device restores them. Off by default: only a user removing a wallet is asked, and internal + * cleanup of a wallet the user never meant to watch must not leave its data behind. */ - suspend fun removeDevice(walletId: String): Result = withContext(ioDispatcher) { + suspend fun removeDevice( + walletId: String, + keepBackupData: Boolean = false, + ): Result = withContext(ioDispatcher) { runSuspendCatching { watcherMutex.withLock { val knownDevices = hwWalletStore.loadKnownDevices() @@ -453,6 +464,16 @@ class HwWalletRepo @Inject constructor( // Without an entry there is nothing to forget, and the check below would pass on an // empty set: report the failure instead of telling the user the wallet was removed. require(targets.isNotEmpty()) { "Unknown hardware wallet '$walletId'" } + // Read before the deletion below, which takes the tags with the activities they are on. + val keptName = targets.firstNotNullOfOrNull { it.customLabel?.takeIf(String::isNotBlank) } + .takeIf { keepBackupData } + // Nothing has been deleted yet, so failing here costs nothing and keeps the choice with + // the user: retry, or remove the wallet without keeping its data. + val keptTagMetadata = when { + keepBackupData -> activityRepo.getTagMetadataForWallet(walletId) + .getOrElse { throw HwBackupDataUnreadableError(it) } + else -> emptyList() + } activeWatchers.toList() .filter { it.toWalletId() == walletId } .forEach { @@ -461,10 +482,25 @@ class HwWalletRepo @Inject constructor( } } activityRepo.deleteForWallet(walletId).getOrThrow() + // Written back only now: the deletion above drops the wallet's stored tag metadata + // along with its activities. Core re-attaches these once the watcher recreates them, + // so re-pairing the device brings the tags back. + if (keptTagMetadata.isNotEmpty()) { + // Nothing to roll back to at this point, and the wallet is already half removed, + // so a failure here loses the tags rather than failing the removal. + preActivityMetadataRepo.upsertPreActivityMetadata(keptTagMetadata) + } trackedWalletIds -= walletId lastPersistedHwSnapshots -= walletId - val failures = targets.mapNotNull { - trezorRepo.forgetDevice(it.id, walletKey = it.walletKey).exceptionOrNull() + // The name is stored in the same write that forgets the entries carrying it, so the + // store never publishes a device list still holding this wallet. A separate write would, + // and a reconcile reading it restarts the watcher of the wallet being removed. + val failures = targets.mapNotNull { device -> + trezorRepo.forgetDevice( + device.id, + walletKey = device.walletKey, + pendingName = PendingNameUpdate(walletId, keptName), + ).exceptionOrNull() } val remaining = hwWalletStore.loadKnownDevices() failures.firstOrNull()?.let { throw it } @@ -657,15 +693,21 @@ class HwWalletRepo @Inject constructor( knownDevices: List, watcherSettings: WatcherSettings, ) { - val persistedWalletIds = activityRepo.getWalletIds().getOrDefault(emptySet()) - .filterNot { it == WalletScope.default } - .toSet() watcherMutex.withLock { + // Read under the lock: a removal deletes a wallet's activities while holding it, and this + // set decides what to delete. Reading it first would let a removal complete in between and + // then be undone here, taking the tag metadata it deliberately kept with it. + val persistedWalletIds = activityRepo.getWalletIds().getOrDefault(emptySet()) + .filterNot { it == WalletScope.default } + .toSet() val specs = knownDevices.toWatcherSpecs(watcherSettings.electrumUrl) val desiredIds = specs.map { it.watcherId }.toSet() val knownWalletIds = knownDevices.mapNotNull { it.resolvedWalletId() }.toSet() trackedWalletIds += persistedWalletIds - val removedWalletIds = trackedWalletIds - knownWalletIds + // Only wallets that still have activities to clear. Core drops a wallet's tag metadata + // along with its activities whether or not any matched, so cleaning up a wallet that has + // none is not a no-op: it takes the metadata a removal deliberately kept. + val removedWalletIds = (trackedWalletIds - knownWalletIds).intersect(persistedWalletIds) trackedWalletIds += knownWalletIds specs.forEach { spec -> @@ -862,6 +904,13 @@ class HwPassphraseRequiredError : AppError("Passphrase needed to reopen this wal /** The entered passphrase opened a different wallet than the one being signed from. */ class HwPassphraseMismatchError : AppError("Passphrase opened a different wallet") +/** + * A removal asked to keep the wallet's backup data, but its tags could not be read. Raised before + * anything is deleted, so the wallet is untouched and the removal can be retried or repeated without + * keeping the data. + */ +class HwBackupDataUnreadableError(cause: Throwable) : AppError("Could not read the backup data", cause) + private data class HwWatcherData( val walletId: String, val addressType: String, diff --git a/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt b/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt index 2c66ba607a..a0e252bf77 100644 --- a/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt @@ -53,6 +53,7 @@ import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout import to.bitkit.async.appScope import to.bitkit.data.HwWalletStore +import to.bitkit.data.PendingNameUpdate import to.bitkit.data.SettingsStore import to.bitkit.di.IoDispatcher import to.bitkit.env.Env @@ -889,7 +890,11 @@ class TrezorRepo @Inject constructor( * credentials are only cleared once no identity of the device remains, so removing one hidden * wallet does not unpair the device for the others. */ - suspend fun forgetDevice(deviceId: String, walletKey: String? = null): Result = withContext(ioDispatcher) { + suspend fun forgetDevice( + deviceId: String, + walletKey: String? = null, + pendingName: PendingNameUpdate? = null, + ): Result = withContext(ioDispatcher) { runSuspendCatching { TrezorDebugLog.log("FORGET", "forgetDevice called for: $deviceId") // The store is the source of truth here: labels are written straight to it, so a @@ -938,7 +943,7 @@ class TrezorRepo @Inject constructor( TrezorDebugLog.log("FORGET", "Keeping credentials, another wallet still uses $deviceId") Result.success(Unit) } - saveKnownDevices(updated) + saveKnownDevices(updated, pendingName) _state.update { it.copy(knownDevices = updated.toImmutableList()) } clearCredentialsResult.getOrThrow() disconnectResult.onFailure { @@ -1141,6 +1146,10 @@ class TrezorRepo @Inject constructor( // must keep the name the user gave it instead of falling back to the device's own. val identityKey = walletKey(xpubs, deviceInfo.id) val named = previous ?: knownDevices.firstOrNull { it.walletKey == identityKey } + val resolvedWalletId = previous?.walletId?.takeIf { it.isNotBlank() } + ?: knownDevices.findHardwareWalletId(xpubs, fallback = deviceInfo.id) + val pendingName = pendingNameFor(resolvedWalletId) + val customLabel = named?.customLabel ?: pendingName val known = KnownDevice( id = deviceInfo.id, name = deviceInfo.name, @@ -1150,26 +1159,45 @@ class TrezorRepo @Inject constructor( model = features.model ?: deviceInfo.model, lastConnectedAt = clock.nowMs(), xpubs = xpubs, - customLabel = named?.customLabel, - walletId = previous?.walletId?.takeIf { it.isNotBlank() } - ?: knownDevices.findHardwareWalletId(xpubs, fallback = deviceInfo.id), - // The selection that derived these keys is authoritative, so a wallet wrongly marked - // hidden is corrected the next time it is opened rather than staying gated behind a - // passphrase forever. On-device entry cannot say which wallet was opened, so it keeps - // what the entry already knew and assumes hidden only for one it has never seen. - passphraseProtected = when (selection) { - WalletSelection.Standard -> false - is WalletSelection.Hidden -> true - WalletSelection.OnDevice -> previous?.passphraseProtected ?: true - }, + customLabel = customLabel, + walletId = resolvedWalletId, + passphraseProtected = selection.isPassphraseProtected(previous), trezorDeviceId = features.deviceId ?: previous?.trezorDeviceId, ) val updated = knownDevices.filterNot { it.isReplacedBy(known, refreshed = previous) } + known - saveKnownDevices(updated) + // The pending name is consumed in the same write as the entry that adopted it, so the name + // lives in exactly one place: leaving it pending would resurrect it once the user clears the + // entry's own label, and dropping it separately would lose it if saving the entry failed. + saveKnownDevices( + updated, + pendingName = pendingName?.let { PendingNameUpdate(resolvedWalletId, name = null) }, + ) _state.update { it.copy(knownDevices = updated.toImmutableList()) } return known } + /** + * The selection that derived a device's keys is authoritative, so a wallet wrongly marked hidden is + * corrected the next time it is opened rather than staying gated behind a passphrase forever. + * On-device entry cannot say which wallet was opened, so it keeps what the entry already knew and + * assumes hidden only for one it has never seen. + */ + private fun WalletSelection.isPassphraseProtected(previous: KnownDevice?): Boolean = when (this) { + WalletSelection.Standard -> false + is WalletSelection.Hidden -> true + WalletSelection.OnDevice -> previous?.passphraseProtected ?: true + } + + /** + * The name a wallet identity carries while it has no device entry: restored from a backup, or kept + * when the wallet was removed. Adopted the first time the identity is paired again. + */ + private suspend fun pendingNameFor(walletId: String): String? = walletId + .takeIf { it.isNotBlank() } + // Only a name: failing to read one must not stop the device being paired. + ?.let { runSuspendCatching { hwWalletStore.loadPendingNames()[it] }.getOrNull() } + ?.takeIf { it.isNotBlank() } + /** * Reads account-level extended public keys for every supported address type so a * watch-only balance can be tracked later without the device present. Permanent @@ -1232,9 +1260,9 @@ class TrezorRepo @Inject constructor( Logger.error("Failed to load known devices", it, context = TAG) }.getOrDefault(emptyList()) - private suspend fun saveKnownDevices(devices: List) { - runCatching { - hwWalletStore.saveKnownDevices(devices) + private suspend fun saveKnownDevices(devices: List, pendingName: PendingNameUpdate? = null) { + runSuspendCatching { + hwWalletStore.saveKnownDevices(devices, pendingName) }.onFailure { Logger.error("Failed to save known devices", it, context = TAG) } } diff --git a/app/src/main/java/to/bitkit/ui/screens/wallets/HardwareWalletScreen.kt b/app/src/main/java/to/bitkit/ui/screens/wallets/HardwareWalletScreen.kt index b1d49cac5c..8de971c067 100644 --- a/app/src/main/java/to/bitkit/ui/screens/wallets/HardwareWalletScreen.kt +++ b/app/src/main/java/to/bitkit/ui/screens/wallets/HardwareWalletScreen.kt @@ -49,7 +49,6 @@ import to.bitkit.ui.components.TabBar import to.bitkit.ui.components.TertiaryButton import to.bitkit.ui.components.TopBarSpacer import to.bitkit.ui.components.VerticalSpacer -import to.bitkit.ui.scaffold.AppAlertDialog import to.bitkit.ui.scaffold.AppTopBar import to.bitkit.ui.scaffold.DrawerNavIcon import to.bitkit.ui.screens.wallets.activity.components.activityListGroupedItems @@ -87,6 +86,8 @@ fun HardwareWalletScreen( onConfirmRemove = { viewModel.removeDevice(walletId) }, onDismissRemoveDialog = viewModel::onDismissRemoveDialog, onBackClick = onBackClick, + keepBackupData = uiState.keepBackupDataOnRemoval, + onKeepBackupDataChange = viewModel::onKeepBackupDataChange, ) } } @@ -101,6 +102,8 @@ private fun HardwareWalletContent( onConfirmRemove: () -> Unit, onDismissRemoveDialog: () -> Unit, onBackClick: () -> Unit, + keepBackupData: Boolean = true, + onKeepBackupDataChange: (Boolean) -> Unit = {}, ) { val hasFunds = wallet.balanceSats > 0uL val hasFundingFunds = wallet.fundingBalanceSats > 0uL @@ -215,11 +218,10 @@ private fun HardwareWalletContent( } if (showRemoveDialog) { - AppAlertDialog( - title = stringResource(R.string.hardware__remove_dialog_title, wallet.name), - text = stringResource(R.string.hardware__remove_dialog_text), - confirmText = stringResource(R.string.common__remove), - dismissText = stringResource(R.string.common__cancel), + RemoveHwWalletDialog( + walletName = wallet.name, + keepBackupData = keepBackupData, + onKeepBackupDataChange = onKeepBackupDataChange, onConfirm = onConfirmRemove, onDismiss = onDismissRemoveDialog, ) diff --git a/app/src/main/java/to/bitkit/ui/screens/wallets/HwWalletViewModel.kt b/app/src/main/java/to/bitkit/ui/screens/wallets/HwWalletViewModel.kt index bf71316d5b..fe199c38a7 100644 --- a/app/src/main/java/to/bitkit/ui/screens/wallets/HwWalletViewModel.kt +++ b/app/src/main/java/to/bitkit/ui/screens/wallets/HwWalletViewModel.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.launch import to.bitkit.R import to.bitkit.models.HwWallet import to.bitkit.models.Toast +import to.bitkit.repositories.HwBackupDataUnreadableError import to.bitkit.repositories.HwWalletRepo import to.bitkit.repositories.HwWalletRepo.Companion.DEVICE_LABEL_MAX_LENGTH import to.bitkit.ui.shared.toast.ToastEventBus @@ -34,7 +35,13 @@ class HwWalletViewModel @Inject constructor( private var renameSessionId = 0L - fun onRemoveClick(wallet: HwWallet) = _uiState.update { it.copy(isPendingRemoval = wallet) } + // Reset on open rather than on dismiss, so a back press, a failed removal or another wallet picked + // from the list all start from the default rather than from the last choice. + fun onRemoveClick(wallet: HwWallet) = _uiState.update { + it.copy(isPendingRemoval = wallet, keepBackupDataOnRemoval = true) + } + + fun onKeepBackupDataChange(value: Boolean) = _uiState.update { it.copy(keepBackupDataOnRemoval = value) } fun onDismissRemoveDialog() = _uiState.update { it.copy(isPendingRemoval = null) } @@ -112,13 +119,22 @@ class HwWalletViewModel @Inject constructor( renameSessionId == sessionId && isPendingRename?.id == walletId fun removeDevice(walletId: String) { + // Read before the update below clears the pending state this choice was made in. + val keepBackupData = _uiState.value.keepBackupDataOnRemoval + viewModelScope.launch { _uiState.update { it.copy(isPendingRemoval = null) } - hwWalletRepo.removeDevice(walletId).onFailure { + hwWalletRepo.removeDevice(walletId, keepBackupData).onFailure { + // The wallet is untouched when its backup data could not be read, so say what failed + // and point at the way through instead of asking for a retry that repeats it. + val description = when (it) { + is HwBackupDataUnreadableError -> R.string.hardware__remove_keep_error + else -> R.string.hardware__remove_error + } ToastEventBus.send( type = Toast.ToastType.ERROR, title = context.getString(R.string.common__error), - description = context.getString(R.string.hardware__remove_error), + description = context.getString(description), ) } } @@ -128,6 +144,7 @@ class HwWalletViewModel @Inject constructor( @Immutable data class HwWalletDetailUiState( val isPendingRemoval: HwWallet? = null, + val keepBackupDataOnRemoval: Boolean = true, val isPendingRename: HwWallet? = null, val labelInput: String = "", val isSavingLabel: Boolean = false, diff --git a/app/src/main/java/to/bitkit/ui/screens/wallets/RemoveHwWalletDialog.kt b/app/src/main/java/to/bitkit/ui/screens/wallets/RemoveHwWalletDialog.kt new file mode 100644 index 0000000000..9883ab546f --- /dev/null +++ b/app/src/main/java/to/bitkit/ui/screens/wallets/RemoveHwWalletDialog.kt @@ -0,0 +1,97 @@ +package to.bitkit.ui.screens.wallets + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.material3.Switch +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.tooling.preview.Preview +import androidx.compose.ui.unit.dp +import to.bitkit.R +import to.bitkit.ui.components.BodyM +import to.bitkit.ui.components.BodyMSB +import to.bitkit.ui.components.HorizontalSpacer +import to.bitkit.ui.components.VerticalSpacer +import to.bitkit.ui.scaffold.AppAlertDialog +import to.bitkit.ui.shared.modifiers.clickableAlpha +import to.bitkit.ui.theme.AppSwitchDefaults +import to.bitkit.ui.theme.AppThemeSurface +import to.bitkit.ui.theme.Colors + +/** + * Confirms removing a paired hardware wallet, offering to carry its name and tags in the backup so + * re-pairing the device restores them. Shared by the wallet screen and the hardware wallet settings. + */ +@Composable +fun RemoveHwWalletDialog( + walletName: String, + keepBackupData: Boolean, + onKeepBackupDataChange: (Boolean) -> Unit, + onConfirm: () -> Unit, + onDismiss: () -> Unit, + modifier: Modifier = Modifier, +) { + AppAlertDialog( + title = stringResource(R.string.hardware__remove_dialog_title, walletName), + confirmText = stringResource(R.string.common__remove), + dismissText = stringResource(R.string.common__cancel), + onConfirm = onConfirm, + onDismiss = onDismiss, + modifier = modifier.testTag("RemoveHwWalletDialog") + ) { + Column { + BodyM(text = stringResource(R.string.hardware__remove_dialog_text), color = Colors.White64) + VerticalSpacer(16.dp) + KeepBackupDataRow( + keepBackupData = keepBackupData, + onKeepBackupDataChange = onKeepBackupDataChange, + ) + } + } +} + +@Composable +private fun KeepBackupDataRow( + keepBackupData: Boolean, + onKeepBackupDataChange: (Boolean) -> Unit, + modifier: Modifier = Modifier, +) { + Row( + horizontalArrangement = Arrangement.SpaceBetween, + verticalAlignment = Alignment.CenterVertically, + modifier = modifier + .fillMaxWidth() + .clickableAlpha { onKeepBackupDataChange(!keepBackupData) } + .testTag("HwRemoveKeepBackupToggle") + ) { + BodyMSB( + text = stringResource(R.string.hardware__remove_dialog_keep), + modifier = Modifier.weight(1f) + ) + HorizontalSpacer(16.dp) + Switch( + checked = keepBackupData, + onCheckedChange = null, // handled by parent + colors = AppSwitchDefaults.colors, + ) + } +} + +@Preview(showBackground = true) +@Composable +private fun Preview() { + AppThemeSurface { + RemoveHwWalletDialog( + walletName = "Trezor Safe 3", + keepBackupData = true, + onKeepBackupDataChange = {}, + onConfirm = {}, + onDismiss = {}, + ) + } +} diff --git a/app/src/main/java/to/bitkit/ui/settings/general/HardwareWalletsSettingsScreen.kt b/app/src/main/java/to/bitkit/ui/settings/general/HardwareWalletsSettingsScreen.kt index 3e00fe24b2..bcc35c01c1 100644 --- a/app/src/main/java/to/bitkit/ui/settings/general/HardwareWalletsSettingsScreen.kt +++ b/app/src/main/java/to/bitkit/ui/settings/general/HardwareWalletsSettingsScreen.kt @@ -63,13 +63,13 @@ import to.bitkit.ui.components.MoneySSB import to.bitkit.ui.components.PrimaryButton import to.bitkit.ui.components.TextInput import to.bitkit.ui.components.VerticalSpacer -import to.bitkit.ui.scaffold.AppAlertDialog import to.bitkit.ui.scaffold.AppTopBar import to.bitkit.ui.scaffold.DrawerNavIcon import to.bitkit.ui.scaffold.ScreenColumn import to.bitkit.ui.scaffold.SheetTopBar import to.bitkit.ui.screens.wallets.HwWalletDetailUiState import to.bitkit.ui.screens.wallets.HwWalletViewModel +import to.bitkit.ui.screens.wallets.RemoveHwWalletDialog import to.bitkit.ui.shared.modifiers.clickableAlpha import to.bitkit.ui.shared.modifiers.sheetHeight import to.bitkit.ui.shared.util.gradientBackground @@ -96,6 +96,7 @@ fun HardwareWalletsSettingsScreen( onRemoveClick = viewModel::onRemoveClick, onConfirmRemove = { viewModel.removeDevice(it.id) }, onDismissRemoveDialog = viewModel::onDismissRemoveDialog, + onKeepBackupDataChange = viewModel::onKeepBackupDataChange, onRenameClick = viewModel::onRenameClick, onDismissRenameSheet = viewModel::onDismissRenameSheet, onLabelChange = viewModel::onLabelChange, @@ -112,6 +113,7 @@ private fun Content( onRemoveClick: (HwWallet) -> Unit = {}, onConfirmRemove: (HwWallet) -> Unit = {}, onDismissRemoveDialog: () -> Unit = {}, + onKeepBackupDataChange: (Boolean) -> Unit = {}, onRenameClick: (HwWallet) -> Unit = {}, onDismissRenameSheet: () -> Unit = {}, onLabelChange: (String) -> Unit = {}, @@ -178,11 +180,10 @@ private fun Content( } uiState.isPendingRemoval?.let { wallet -> - AppAlertDialog( - title = stringResource(R.string.hardware__remove_dialog_title, wallet.name), - text = stringResource(R.string.hardware__remove_dialog_text), - confirmText = stringResource(R.string.common__remove), - dismissText = stringResource(R.string.common__cancel), + RemoveHwWalletDialog( + walletName = wallet.name, + keepBackupData = uiState.keepBackupDataOnRemoval, + onKeepBackupDataChange = onKeepBackupDataChange, onConfirm = { onConfirmRemove(wallet) }, onDismiss = onDismissRemoveDialog, ) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 357583ed8f..1f236296fb 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -204,9 +204,11 @@ If you have funds protected by a passphrase, enter it below to add these funds to your wallet balance as well. Passphrase Remove %1$s + Keep name and tags in backup Don\'t worry, your funds are safe and your coins won\'t be deleted. Bitkit will simply stop displaying the amounts in the wallet. Remove %1$s Could not remove the hardware wallet. Please try again. + Could not keep this wallet\'s tags in your backup. Try again, or remove it without keeping them. Could not rename the hardware wallet. Please try again. Could not search for hardware wallets. Check your connection and try again. Funds transfer to savings is usually instant, but settlement may take up to <accent>14 days</accent> under certain network conditions. diff --git a/app/src/test/java/to/bitkit/repositories/ActivityRepoTest.kt b/app/src/test/java/to/bitkit/repositories/ActivityRepoTest.kt index 728a5721f8..bf8bec153e 100644 --- a/app/src/test/java/to/bitkit/repositories/ActivityRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/ActivityRepoTest.kt @@ -7,6 +7,7 @@ import com.synonym.bitkitcore.IcJitEntry import com.synonym.bitkitcore.LightningActivity import com.synonym.bitkitcore.OnchainActivity import com.synonym.bitkitcore.PaymentType +import com.synonym.bitkitcore.PreActivityMetadata import com.synonym.bitkitcore.SortDirection import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.MutableStateFlow @@ -857,6 +858,53 @@ class ActivityRepoTest : BaseUnitTest() { assertEquals(emptyList(), result) } + @Test + fun `getTagMetadataForWallet unions the stored and the rendered tag metadata`() = test { + stubHardwareTagLookup(hardwareOnchainActivity(txType = PaymentType.SENT)) + val stored = PreActivityMetadata( + walletId = HARDWARE_WALLET_ID, + paymentId = "not-yet-seen", + tags = listOf("gift"), + paymentHash = null, + txId = "not-yet-seen", + address = null, + isReceive = false, + feeRate = 0uL, + isTransfer = false, + channelId = null, + createdAt = 1uL, + ) + whenever { coreService.activity.getAllPreActivityMetadata() }.thenReturn(listOf(stored)) + + val result = sut.getTagMetadataForWallet(HARDWARE_WALLET_ID).getOrThrow() + + // Core drops both on delete, and only the rendered half comes back from the activity tags. + assertEquals(listOf("not-yet-seen", "hw-txid"), result.map { it.paymentId }) + } + + @Test + fun `getTagMetadataForWallet ignores the metadata of other wallets`() = test { + stubHardwareTagLookup(hardwareOnchainActivity(txType = PaymentType.SENT)) + val otherWallet = PreActivityMetadata( + walletId = WalletScope.default, + paymentId = "default-payment", + tags = listOf("daily"), + paymentHash = null, + txId = null, + address = null, + isReceive = true, + feeRate = 0uL, + isTransfer = false, + channelId = null, + createdAt = 1uL, + ) + whenever { coreService.activity.getAllPreActivityMetadata() }.thenReturn(listOf(otherWallet)) + + val result = sut.getTagMetadataForWallet(HARDWARE_WALLET_ID).getOrThrow() + + assertEquals(listOf("hw-txid"), result.map { it.paymentId }) + } + private suspend fun stubHardwareTagLookup(activity: Activity.Onchain) { whenever { coreService.activity.getAllActivitiesTags() } .thenReturn(listOf(ActivityTags(HARDWARE_WALLET_ID, "hw-activity", listOf("cold")))) diff --git a/app/src/test/java/to/bitkit/repositories/BackupRepoTest.kt b/app/src/test/java/to/bitkit/repositories/BackupRepoTest.kt index 164cf9d144..b8a88abf98 100644 --- a/app/src/test/java/to/bitkit/repositories/BackupRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/BackupRepoTest.kt @@ -29,6 +29,8 @@ import org.mockito.kotlin.whenever import to.bitkit.data.AppCacheData import to.bitkit.data.AppDb import to.bitkit.data.CacheStore +import to.bitkit.data.HwWalletData +import to.bitkit.data.HwWalletStore import to.bitkit.data.SettingsData import to.bitkit.data.SettingsStore import to.bitkit.data.WatchOnlyAccountAllocationState @@ -45,7 +47,9 @@ import to.bitkit.di.json import to.bitkit.models.ActivityBackupV1 import to.bitkit.models.BackupCategory import to.bitkit.models.BackupItemStatus +import to.bitkit.models.KnownDevice import to.bitkit.models.MetadataBackupV1 +import to.bitkit.models.TransportType import to.bitkit.models.WalletBackupV1 import to.bitkit.models.WalletScope import to.bitkit.models.WatchOnlyAccountRecord @@ -57,12 +61,14 @@ import to.bitkit.utils.AppError import javax.inject.Provider import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue import kotlin.time.Clock import kotlin.time.ExperimentalTime import kotlin.time.Instant @OptIn(ExperimentalCoroutinesApi::class, ExperimentalTime::class) +@Suppress("LargeClass") class BackupRepoTest : BaseUnitTest() { private val context = mock() private val cacheStore = mock() @@ -72,6 +78,7 @@ class BackupRepoTest : BaseUnitTest() { private val widgetsStore = mock() private val watchOnlyAccountStore = mock() private val watchOnlyAccountRepo = mock() + private val hwWalletStore = mock() private val blocktankRepo = mock() private val activityRepo = mock() private val pubkyRepo = mock() @@ -86,6 +93,7 @@ class BackupRepoTest : BaseUnitTest() { private val cacheData = MutableStateFlow(AppCacheData()) private val settingsData = MutableStateFlow(SettingsData()) private val widgetsData = MutableStateFlow(WidgetsData()) + private val hwWalletData = MutableStateFlow(HwWalletData()) private lateinit var sut: BackupRepo @@ -105,6 +113,9 @@ class BackupRepoTest : BaseUnitTest() { whenever { watchOnlyAccountStore.backupSnapshot() }.thenReturn( WatchOnlyAccountBackupSnapshot(emptyList(), WatchOnlyAccountAllocationState()) ) + whenever(hwWalletStore.data).thenReturn(hwWalletData) + whenever { hwWalletStore.backupSnapshot() }.thenReturn(emptyMap()) + whenever { hwWalletStore.restoreNames(any()) }.thenReturn(Unit) whenever { vssBackupClient.getObject(any()) }.thenReturn(Result.success(null)) whenever { vssBackupClient.putObject(any(), any()) } .thenReturn(Result.success(VssItem(key = BackupCategory.SETTINGS.name, value = byteArrayOf(), version = 1))) @@ -459,6 +470,122 @@ class BackupRepoTest : BaseUnitTest() { assertEquals(listOf(preActivityMetadata(), hardwareTagMetadata), payload.tagMetadata) } + @Test + fun `metadata backup carries the hardware wallet names`() = test { + stubMetadataBackupReads() + whenever { hwWalletStore.backupSnapshot() }.thenReturn(mapOf(HARDWARE_WALLET_ID to "Cold Storage")) + val dataCaptor = argumentCaptor() + + sut.triggerBackup(BackupCategory.METADATA) + + verifyBlocking(vssBackupClient) { + putObject(eq(BackupCategory.METADATA.name), dataCaptor.capture()) + } + val payload = json.decodeFromString(dataCaptor.firstValue.decodeToString()) + assertEquals(mapOf(HARDWARE_WALLET_ID to "Cold Storage"), payload.hwWalletNames) + } + + @Test + fun `metadata backup omits the hardware wallet names when none are set`() = test { + stubMetadataBackupReads() + val dataCaptor = argumentCaptor() + + sut.triggerBackup(BackupCategory.METADATA) + + verifyBlocking(vssBackupClient) { + putObject(eq(BackupCategory.METADATA.name), dataCaptor.capture()) + } + val payload = json.decodeFromString(dataCaptor.firstValue.decodeToString()) + assertNull(payload.hwWalletNames) + } + + @Test + fun `metadata backup fails when the hardware wallet names cannot be read`() = test { + stubMetadataBackupReads() + whenever { hwWalletStore.backupSnapshot() } + .doSuspendableAnswer { throw BackupRepoTestError("store unavailable") } + + val result = sut.triggerBackup(BackupCategory.METADATA) + + assertTrue(result.isFailure) + // This envelope is the only copy of the names, so uploading without them would drop them. + verify(vssBackupClient, never()).putObject(eq(BackupCategory.METADATA.name), any()) + } + + @Test + fun `metadata restore applies the backed up hardware wallet names`() = test { + val names = mapOf(HARDWARE_WALLET_ID to "Cold Storage") + stubMetadataRestore( + envelope = metadataEnvelope(metadata = listOf(preActivityMetadata()), hwWalletNames = names), + ) + + sut.performFullRestoreFromLatestBackup() + + verifyBlocking(hwWalletStore) { restoreNames(names) } + } + + @Test + fun `metadata restore keeps the stored names when the envelope carries none`() = test { + stubMetadataRestore(envelope = metadataEnvelope(metadata = listOf(preActivityMetadata()))) + + sut.performFullRestoreFromLatestBackup() + + // An envelope written before this field must not clear what this wallet already holds. + verifyBlocking(hwWalletStore) { restoreNames(emptyMap()) } + } + + @Test + fun `metadata restore records the category when the hardware wallet names cannot be stored`() = test { + stubWalletBackup() + stubMetadataRestore( + envelope = metadataEnvelope( + metadata = listOf(preActivityMetadata()), + hwWalletNames = mapOf(HARDWARE_WALLET_ID to "Cold Storage"), + ), + ) + whenever { hwWalletStore.restoreNames(any()) } + .doSuspendableAnswer { throw BackupRepoTestError("store unavailable") } + + val result = sut.performFullRestoreFromLatestBackup() + + assertTrue(result.isSuccess) + // The names are the last and the least of this envelope: the caches and tags above them were + // already applied, so losing the whole category's synced marker over a name would be wrong. + verifyBlocking(cacheStore) { updateBackupStatus(eq(BackupCategory.METADATA), any()) } + } + + @Test + fun `renaming a hardware wallet triggers a metadata backup`() = test { + stubMetadataBackupReads() + stubBackupObservers() + stubBackupStatuses( + MutableStateFlow(emptyMap()), + CompletableDeferred().apply { complete(Unit) }, + ) {} + + try { + sut.startObservingBackups() + runCurrent() + + // Reconnecting rewrites the entry without touching its name. + hwWalletData.update { HwWalletData(knownDevices = listOf(knownDevice())) } + runCurrent() + advanceTimeBy(10_000) + runCurrent() + + verify(vssBackupClient, never()).putObject(eq(BackupCategory.METADATA.name), any()) + + hwWalletData.update { HwWalletData(knownDevices = listOf(knownDevice(customLabel = "Cold Storage"))) } + runCurrent() + advanceTimeBy(10_000) + runCurrent() + + verifyBlocking(vssBackupClient) { putObject(eq(BackupCategory.METADATA.name), any()) } + } finally { + sut.stopObservingBackups() + } + } + @Test fun `activity traffic alone does not trigger a metadata backup`() = test { val activitiesChanged = MutableStateFlow(0L) @@ -643,8 +770,16 @@ class BackupRepoTest : BaseUnitTest() { ) ) - private fun metadataEnvelope(metadata: List = emptyList()) = json.encodeToString( - MetadataBackupV1(createdAt = 123, tagMetadata = metadata, cache = AppCacheData()) + private fun metadataEnvelope( + metadata: List = emptyList(), + hwWalletNames: Map? = null, + ) = json.encodeToString( + MetadataBackupV1( + createdAt = 123, + tagMetadata = metadata, + cache = AppCacheData(), + hwWalletNames = hwWalletNames, + ) ) private fun envelopeWithRawField(base: String, field: String, rawJson: String): String { @@ -659,6 +794,19 @@ class BackupRepoTest : BaseUnitTest() { tags = listOf("coffee"), ) + private fun knownDevice(customLabel: String? = null) = KnownDevice( + id = "dev1", + name = "Trezor", + path = "usb-1", + transportType = TransportType.USB, + label = null, + model = "Safe 3", + lastConnectedAt = 1, + xpubs = mapOf("nativeSegwit" to "zpubNS"), + customLabel = customLabel, + walletId = HARDWARE_WALLET_ID, + ) + private fun preActivityMetadata() = PreActivityMetadata( walletId = WalletScope.default, paymentId = "p1", @@ -741,6 +889,7 @@ class BackupRepoTest : BaseUnitTest() { widgetsStore = widgetsStore, watchOnlyAccountStore = watchOnlyAccountStore, watchOnlyAccountRepo = watchOnlyAccountRepo, + hwWalletStore = hwWalletStore, blocktankRepo = blocktankRepo, activityRepo = activityRepo, pubkyRepo = pubkyRepo, diff --git a/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt b/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt index 5eb92f8b6e..85424a167a 100644 --- a/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt @@ -5,6 +5,7 @@ import com.synonym.bitkitcore.Activity import com.synonym.bitkitcore.ComposeResult import com.synonym.bitkitcore.OnchainActivity import com.synonym.bitkitcore.PaymentType +import com.synonym.bitkitcore.PreActivityMetadata import com.synonym.bitkitcore.TransactionDetails import com.synonym.bitkitcore.TrezorException import com.synonym.bitkitcore.TrezorFeatures @@ -30,6 +31,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import to.bitkit.data.HwWalletData import to.bitkit.data.HwWalletStore +import to.bitkit.data.PendingNameUpdate import to.bitkit.data.SettingsData import to.bitkit.data.SettingsStore import to.bitkit.env.Env @@ -47,6 +49,7 @@ import to.bitkit.test.BaseUnitTest import to.bitkit.utils.AppError import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue import kotlin.time.Duration.Companion.seconds @@ -61,6 +64,7 @@ class HwWalletRepoTest : BaseUnitTest() { private val trezorRepo = mock() private val activityRepo = mock() + private val preActivityMetadataRepo = mock() private val hwWalletStore = mock() private val settingsStore = mock() @@ -113,6 +117,9 @@ class HwWalletRepoTest : BaseUnitTest() { } whenever { activityRepo.getWalletIds() }.thenReturn(Result.success(emptySet())) whenever { activityRepo.deleteForWallet(any()) }.thenReturn(Result.success(Unit)) + whenever { activityRepo.getTagMetadataForWallet(any()) }.thenReturn(Result.success(emptyList())) + whenever { preActivityMetadataRepo.upsertPreActivityMetadata(any()) }.thenReturn(Result.success(Unit)) + whenever { hwWalletStore.setPendingName(any(), anyOrNull()) }.thenReturn(Unit) } private fun passphraseCapableFeatures(): TrezorFeatures = @@ -121,6 +128,7 @@ class HwWalletRepoTest : BaseUnitTest() { private fun createRepo() = HwWalletRepo( trezorRepo = trezorRepo, activityRepo = activityRepo, + preActivityMetadataRepo = preActivityMetadataRepo, hwWalletStore = hwWalletStore, settingsStore = settingsStore, ioDispatcher = testDispatcher, @@ -933,7 +941,7 @@ class HwWalletRepoTest : BaseUnitTest() { val result = sut.removeDevice("unknown-wallet") assertTrue(result.isFailure) - verify(trezorRepo, never()).forgetDevice(any(), anyOrNull()) + verify(trezorRepo, never()).forgetDevice(any(), anyOrNull(), anyOrNull()) verify(activityRepo, never()).deleteForWallet("unknown-wallet") } @@ -943,14 +951,14 @@ class HwWalletRepoTest : BaseUnitTest() { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device, hiddenWallet), listOf(device)) wheneverStartWatcher().thenReturn(Result.success(Unit)) whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) val sut = createRepo() runCurrent() val result = sut.removeDevice(HIDDEN_WALLET_ID) assertTrue(result.isSuccess) - verify(trezorRepo).forgetDevice("dev1", "zpubHidden") + verify(trezorRepo).forgetDevice(eq("dev1"), eq("zpubHidden"), anyOrNull()) verify(trezorRepo).stopWatcher("$HIDDEN_WALLET_ID|nativeSegwit") verify(trezorRepo, never()).stopWatcher("$HARDWARE_WALLET_ID|nativeSegwit") verify(activityRepo).deleteForWallet(HIDDEN_WALLET_ID) @@ -1220,7 +1228,7 @@ class HwWalletRepoTest : BaseUnitTest() { var stored = listOf(device, hiddenWallet) whenever { hwWalletStore.loadKnownDevices() }.thenAnswer { stored } whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenAnswer { + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenAnswer { stored = stored.filterNot { it.walletId == "stray-wallet" } Result.success(Unit) } @@ -1238,10 +1246,47 @@ class HwWalletRepoTest : BaseUnitTest() { val result = sut.reconnectWithPassphrase(HIDDEN_WALLET_ID, "wrong") assertTrue(result.exceptionOrNull() is HwPassphraseMismatchError) - verify(trezorRepo).forgetDevice("dev1", "zpubStray") + verify(trezorRepo).forgetDevice(eq("dev1"), eq("zpubStray"), anyOrNull()) verify(trezorRepo).disconnectStaleSession("dev1") } + @Test + fun `reconnectWithPassphrase keeps the backup data of the wallet a wrong passphrase opened`() = test { + // The wallet is a real one the user owns, and storing it consumed the name restored for it, + // so dropping it here would erase a backed up name a typo was never meant to touch. + val strayWallet = device.copy( + xpubs = mapOf("nativeSegwit" to "zpubStray"), + walletId = "stray-wallet", + customLabel = "Hidden Stash", + passphraseProtected = true, + ) + val strayTagMetadata = listOf(preActivityMetadata().copy(walletId = "stray-wallet")) + var stored = listOf(device, hiddenWallet) + whenever { hwWalletStore.loadKnownDevices() }.thenAnswer { stored } + whenever { activityRepo.getTagMetadataForWallet("stray-wallet") } + .thenReturn(Result.success(strayTagMetadata)) + whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenAnswer { + stored = stored.filterNot { it.walletId == "stray-wallet" } + Result.success(Unit) + } + whenever { trezorRepo.connectWithWalletMode("dev1", TrezorWalletMode.PASSPHRASE_HOST, "wrong") } + .thenAnswer { + stored = stored + strayWallet + trezorState.value = TrezorState( + connected = ConnectedTrezorDevice(id = "dev1", features = mock(), walletId = "stray-wallet"), + ) + Result.success(mock()) + } + val sut = createRepo() + + val result = sut.reconnectWithPassphrase(HIDDEN_WALLET_ID, "wrong") + + assertTrue(result.exceptionOrNull() is HwPassphraseMismatchError) + verify(trezorRepo).forgetDevice(any(), anyOrNull(), eq(PendingNameUpdate("stray-wallet", "Hidden Stash"))) + verify(preActivityMetadataRepo).upsertPreActivityMetadata(strayTagMetadata) + } + @Test fun `funding account resolves the requested identity on a shared device`() = test { storeData.value = HwWalletData(knownDevices = listOf(device, hiddenWallet)) @@ -1302,6 +1347,8 @@ class HwWalletRepoTest : BaseUnitTest() { @Test fun `store removal deletes the hardware wallet activity scope`() = test { + // Only a wallet with activities left behind is cleaned up, so it must have some to clean. + whenever { activityRepo.getWalletIds() }.thenReturn(Result.success(setOf(HARDWARE_WALLET_ID))) wheneverStartWatcher().thenReturn(Result.success(Unit)) whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) createRepo() @@ -1329,6 +1376,33 @@ class HwWalletRepoTest : BaseUnitTest() { verify(activityRepo, never()).deleteForWallet(HARDWARE_WALLET_ID) } + @Test + fun `removing a wallet deletes its activities exactly once`() = test { + // A second delete would run Core's cascade again and take the tag metadata the removal + // deliberately kept. The interleaving that caused this in the field — the cleanup deciding from + // a scope set read before it takes the lock — needs real concurrency and is covered by manual + // QA; this pins the simpler invariant that the cleanup adds no delete of its own. + var persisted = setOf(HARDWARE_WALLET_ID) + whenever { activityRepo.getWalletIds() }.thenAnswer { Result.success(persisted) } + whenever { activityRepo.deleteForWallet(any()) }.thenAnswer { + persisted = emptySet() + Result.success(Unit) + } + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) + wheneverStartWatcher().thenReturn(Result.success(Unit)) + whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + runCurrent() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + storeData.value = HwWalletData(knownDevices = emptyList()) + runCurrent() + + assertTrue(result.isSuccess) + verify(activityRepo, times(1)).deleteForWallet(HARDWARE_WALLET_ID) + } + @Test fun `resetState clears store and stops active watchers`() = test { storeData.value = HwWalletData( @@ -1360,7 +1434,7 @@ class HwWalletRepoTest : BaseUnitTest() { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) wheneverStartWatcher().thenReturn(Result.success(Unit)) whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) val sut = createRepo() runCurrent() @@ -1369,19 +1443,152 @@ class HwWalletRepoTest : BaseUnitTest() { assertEquals(true, result.isSuccess) verify(trezorRepo).stopWatcher("hardware-wallet|nativeSegwit") verify(activityRepo).deleteForWallet(HARDWARE_WALLET_ID) - verify(trezorRepo).forgetDevice("dev1", "zpubNS") + verify(trezorRepo).forgetDevice(eq("dev1"), eq("zpubNS"), anyOrNull()) + } + + @Test + fun `removeDevice keeping backup data rewrites the tag metadata after deleting the activities`() = test { + val named = device.copy(customLabel = "Cold Storage") + val tagMetadata = listOf(preActivityMetadata()) + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(named), emptyList()) + whenever { activityRepo.getTagMetadataForWallet(HARDWARE_WALLET_ID) } + .thenReturn(Result.success(tagMetadata)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + assertTrue(result.isSuccess) + // Core drops the wallet's tag metadata with its activities, so the rewrite must follow the delete. + inOrder(activityRepo, preActivityMetadataRepo) { + verify(activityRepo).deleteForWallet(HARDWARE_WALLET_ID) + verify(preActivityMetadataRepo).upsertPreActivityMetadata(tagMetadata) + } + } + + @Test + fun `removeDevice keeping backup data reports unreadable data without touching the wallet`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) + whenever { activityRepo.getTagMetadataForWallet(HARDWARE_WALLET_ID) } + .thenReturn(Result.failure(AppError("core unavailable"))) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + // Raised before anything is deleted, so the wallet survives and the user keeps the choice. + assertTrue(result.exceptionOrNull() is HwBackupDataUnreadableError) + verify(activityRepo, never()).deleteForWallet(any()) + verify(trezorRepo, never()).forgetDevice(any(), anyOrNull(), anyOrNull()) + } + + @Test + fun `removeDevice keeping backup data stores the wallet name before forgetting the device`() = test { + val named = device.copy(customLabel = "Cold Storage") + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(named), emptyList()) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + assertTrue(result.isSuccess) + // Carried by the write that forgets the entry, so the store never publishes a device list + // still holding this wallet, which would restart the watcher of the wallet being removed. + verify(trezorRepo).forgetDevice( + eq("dev1"), + eq("zpubNS"), + eq(PendingNameUpdate(HARDWARE_WALLET_ID, "Cold Storage")), + ) + } + + @Test + fun `removeDevice keeping backup data stores no name for a wallet that was never renamed`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + assertTrue(result.isSuccess) + verify(trezorRepo).forgetDevice(any(), anyOrNull(), eq(PendingNameUpdate(HARDWARE_WALLET_ID, null))) + } + + @Test + fun `removeDevice keeping backup data keeps the tags of a wallet that was never renamed`() = test { + // The name and the tags are kept independently: a wallet is far more likely to carry tags than + // a name the user bothered to set, and having no name must not cost it its tags. + val tagMetadata = listOf(preActivityMetadata()) + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) + whenever { activityRepo.getTagMetadataForWallet(HARDWARE_WALLET_ID) } + .thenReturn(Result.success(tagMetadata)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + assertTrue(result.isSuccess) + assertNull(device.customLabel) + verify(preActivityMetadataRepo).upsertPreActivityMetadata(tagMetadata) + verify(trezorRepo).forgetDevice(any(), anyOrNull(), eq(PendingNameUpdate(HARDWARE_WALLET_ID, null))) + } + + @Test + fun `removeDevice without keeping backup data drops the name and the tag metadata`() = test { + val named = device.copy(customLabel = "Cold Storage") + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(named), emptyList()) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = false) + + assertTrue(result.isSuccess) + verify(trezorRepo).forgetDevice(any(), anyOrNull(), eq(PendingNameUpdate(HARDWARE_WALLET_ID, null))) + verify(activityRepo, never()).getTagMetadataForWallet(any()) + verify(preActivityMetadataRepo, never()).upsertPreActivityMetadata(any()) + } + + @Test + fun `removeDevice keeps nothing by default`() = test { + val named = device.copy(customLabel = "Cold Storage") + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(named), emptyList()) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID) + + assertTrue(result.isSuccess) + verify(trezorRepo).forgetDevice(any(), anyOrNull(), eq(PendingNameUpdate(HARDWARE_WALLET_ID, null))) + verify(preActivityMetadataRepo, never()).upsertPreActivityMetadata(any()) + } + + @Test + fun `removeDevice still removes the wallet when the tag metadata cannot be rewritten`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) + whenever { activityRepo.getTagMetadataForWallet(HARDWARE_WALLET_ID) } + .thenReturn(Result.success(listOf(preActivityMetadata()))) + whenever { preActivityMetadataRepo.upsertPreActivityMetadata(any()) } + .thenReturn(Result.failure(AppError("core unavailable"))) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) + val sut = createRepo() + + val result = sut.removeDevice(HARDWARE_WALLET_ID, keepBackupData = true) + + // The activities are already gone and the watchers stopped, so there is nothing to roll back to: + // the tags are lost rather than the removal reported as failed. + assertTrue(result.isSuccess) + verify(trezorRepo).forgetDevice(eq("dev1"), eq("zpubNS"), anyOrNull()) } @Test fun `removeDevice fails when forget reports credential cleanup failure despite the device being gone`() = test { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), emptyList()) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.failure(AppError("clear failed"))) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) } + .thenReturn(Result.failure(AppError("clear failed"))) val sut = createRepo() val result = sut.removeDevice(HARDWARE_WALLET_ID) assertEquals(true, result.isFailure) - verify(trezorRepo).forgetDevice("dev1", "zpubNS") + verify(trezorRepo).forgetDevice(eq("dev1"), eq("zpubNS"), anyOrNull()) } @Test @@ -1396,7 +1603,7 @@ class HwWalletRepoTest : BaseUnitTest() { assertEquals(true, result.isFailure) verify(trezorRepo).stopWatcher("hardware-wallet|nativeSegwit") - verify(trezorRepo, never()).forgetDevice(any(), anyOrNull()) + verify(trezorRepo, never()).forgetDevice(any(), anyOrNull(), anyOrNull()) } @Test @@ -1410,7 +1617,7 @@ class HwWalletRepoTest : BaseUnitTest() { assertTrue(result.isFailure) verify(activityRepo).deleteForWallet(HARDWARE_WALLET_ID) - verify(trezorRepo, never()).forgetDevice(any(), anyOrNull()) + verify(trezorRepo, never()).forgetDevice(any(), anyOrNull(), anyOrNull()) } @Test @@ -1421,21 +1628,21 @@ class HwWalletRepoTest : BaseUnitTest() { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(bleEntry, usbEntry), emptyList()) wheneverStartWatcher().thenReturn(Result.success(Unit)) whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) val sut = createRepo() runCurrent() sut.removeDevice(HARDWARE_WALLET_ID) verify(trezorRepo).stopWatcher("hardware-wallet|nativeSegwit") - verify(trezorRepo).forgetDevice("ble1", "zpubNS") - verify(trezorRepo).forgetDevice("usb1", "zpubNS") + verify(trezorRepo).forgetDevice(eq("ble1"), eq("zpubNS"), anyOrNull()) + verify(trezorRepo).forgetDevice(eq("usb1"), eq("zpubNS"), anyOrNull()) } @Test fun `removeDevice fails when the device is still present afterwards`() = test { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), listOf(device)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) val sut = createRepo() val result = sut.removeDevice(HARDWARE_WALLET_ID) @@ -1448,7 +1655,7 @@ class HwWalletRepoTest : BaseUnitTest() { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device), listOf(device)) wheneverStartWatcher().thenReturn(Result.success(Unit)) whenever { trezorRepo.stopWatcher(any()) }.thenReturn(Result.success(Unit)) - whenever { trezorRepo.forgetDevice(any(), anyOrNull()) }.thenReturn(Result.success(Unit)) + whenever { trezorRepo.forgetDevice(any(), anyOrNull(), anyOrNull()) }.thenReturn(Result.success(Unit)) val sut = createRepo() runCurrent() @@ -1823,6 +2030,20 @@ class HwWalletRepoTest : BaseUnitTest() { assertEquals("My Cold Wallet", sut.wallets.value.single().name) } + private fun preActivityMetadata() = PreActivityMetadata( + walletId = HARDWARE_WALLET_ID, + paymentId = "hw-txid", + tags = listOf("cold"), + paymentHash = null, + txId = "hw-txid", + address = null, + isReceive = false, + feeRate = 0uL, + isTransfer = false, + channelId = null, + createdAt = 1uL, + ) + private suspend fun wheneverStartWatcher() = whenever( trezorRepo.startWatcher( any(), diff --git a/app/src/test/java/to/bitkit/repositories/TrezorRepoTest.kt b/app/src/test/java/to/bitkit/repositories/TrezorRepoTest.kt index 3a2419e8bd..07d6156208 100644 --- a/app/src/test/java/to/bitkit/repositories/TrezorRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/TrezorRepoTest.kt @@ -28,6 +28,7 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq +import org.mockito.kotlin.isNull import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.times @@ -35,6 +36,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import to.bitkit.R import to.bitkit.data.HwWalletStore +import to.bitkit.data.PendingNameUpdate import to.bitkit.data.SettingsData import to.bitkit.data.SettingsStore import to.bitkit.env.Env @@ -112,6 +114,7 @@ class TrezorRepoTest : BaseUnitTest() { whenever(context.getString(R.string.hardware__connect_error)).thenReturn("Could not connect to your Trezor.") whenever(context.getString(R.string.hardware__device_busy)).thenReturn(DEVICE_BUSY_MESSAGE) whenever { hwWalletStore.loadKnownDevices() }.thenReturn(emptyList()) + whenever { hwWalletStore.loadPendingNames() }.thenReturn(emptyMap()) stubAccountXpubFetch() } @@ -239,7 +242,7 @@ class TrezorRepoTest : BaseUnitTest() { val result = sut.initialize() assertTrue(result.isSuccess) - verify(hwWalletStore, never()).saveKnownDevices(any()) + verify(hwWalletStore, never()).saveKnownDevices(any(), anyOrNull()) assertEquals("", sut.state.value.knownDevices.single().walletId) } @@ -719,7 +722,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) val saved = captor.firstValue.single() assertEquals(DEVICE_ID, saved.id) assertEquals(TransportType.USB, saved.transportType) @@ -765,7 +768,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals(setOf(walletId), captor.firstValue.map { it.walletId }.toSet()) } @@ -787,7 +790,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) val saved = captor.firstValue assertEquals(2, saved.size) assertEquals(standard, saved.first()) @@ -826,12 +829,101 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) val added = captor.firstValue.single { it.id == DEVICE_ID } assertEquals("No Pass", added.customLabel) assertEquals("standard-wallet", added.walletId) } + @Test + fun `connect adopts the pending name of a wallet paired again`() = test { + // Restored from a backup, or kept when the wallet was removed: pairing takes the name over. + val sharedKey = "shared-native-xpub" + val unnamed = mockKnownDevice( + id = DEVICE_ID, + xpubs = ALL_ADDRESS_TYPE_KEYS.associateWith { sharedKey }, + customLabel = null, + walletId = "standard-wallet", + ) + val features = mockFeatures() + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(unnamed)) + whenever { hwWalletStore.loadPendingNames() }.thenReturn(mapOf("standard-wallet" to "Cold Storage")) + whenever(trezorService.connect(eq(DEVICE_ID), any())).thenReturn(features) + whenever(trezorService.scan()).thenReturn(listOf(mockDeviceInfo())) + whenever( + trezorService.getPublicKey(path = any(), coin = anyOrNull(), showOnTrezor = eq(false)) + ).thenAnswer { mockPublicKeyResponse(xpub = sharedKey, path = it.getArgument(0)) } + sut = createSut() + + sut.scan() + val result = sut.connect(DEVICE_ID) + + assertTrue(result.isSuccess) + val captor = argumentCaptor>() + // Consumed in the same write as the entry that adopted it, so a failed save cannot lose it, + // and clearing the name later cannot fall back to it again. + verify(hwWalletStore).saveKnownDevices(captor.capture(), eq(PendingNameUpdate("standard-wallet", name = null))) + assertEquals("Cold Storage", captor.firstValue.single { it.id == DEVICE_ID }.customLabel) + } + + @Test + fun `connect prefers the stored custom label over a pending name`() = test { + val sharedKey = "shared-native-xpub" + val stored = mockKnownDevice( + id = DEVICE_ID, + xpubs = ALL_ADDRESS_TYPE_KEYS.associateWith { sharedKey }, + customLabel = "Renamed Here", + walletId = "standard-wallet", + ) + val features = mockFeatures() + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(stored)) + whenever { hwWalletStore.loadPendingNames() }.thenReturn(mapOf("standard-wallet" to "Cold Storage")) + whenever(trezorService.connect(eq(DEVICE_ID), any())).thenReturn(features) + whenever(trezorService.scan()).thenReturn(listOf(mockDeviceInfo())) + whenever( + trezorService.getPublicKey(path = any(), coin = anyOrNull(), showOnTrezor = eq(false)) + ).thenAnswer { mockPublicKeyResponse(xpub = sharedKey, path = it.getArgument(0)) } + sut = createSut() + + sut.scan() + val result = sut.connect(DEVICE_ID) + + assertTrue(result.isSuccess) + val captor = argumentCaptor>() + // The pending name lost, so it is stale: dropping it keeps a later rename from falling back to it. + verify(hwWalletStore).saveKnownDevices(captor.capture(), eq(PendingNameUpdate("standard-wallet", name = null))) + assertEquals("Renamed Here", captor.firstValue.single { it.id == DEVICE_ID }.customLabel) + } + + @Test + fun `connect leaves the pending name of another identity on the device alone`() = test { + val sharedKey = "shared-native-xpub" + val unnamed = mockKnownDevice( + id = DEVICE_ID, + xpubs = ALL_ADDRESS_TYPE_KEYS.associateWith { sharedKey }, + customLabel = null, + walletId = "standard-wallet", + ) + val features = mockFeatures() + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(unnamed)) + // A passphrase wallet on the same device derives its own keys, so its name is its own. + whenever { hwWalletStore.loadPendingNames() }.thenReturn(mapOf("hidden-wallet" to "Hidden Stash")) + whenever(trezorService.connect(eq(DEVICE_ID), any())).thenReturn(features) + whenever(trezorService.scan()).thenReturn(listOf(mockDeviceInfo())) + whenever( + trezorService.getPublicKey(path = any(), coin = anyOrNull(), showOnTrezor = eq(false)) + ).thenAnswer { mockPublicKeyResponse(xpub = sharedKey, path = it.getArgument(0)) } + sut = createSut() + + sut.scan() + val result = sut.connect(DEVICE_ID) + + assertTrue(result.isSuccess) + val captor = argumentCaptor>() + verify(hwWalletStore).saveKnownDevices(captor.capture(), isNull()) + assertNull(captor.firstValue.single { it.id == DEVICE_ID }.customLabel) + } + @Test fun `connect supersedes entries of a seed the device no longer carries`() = test { // A wiped and restored device reports a new device id and different keys, so nothing would @@ -851,7 +943,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) val saved = captor.firstValue.single() assertEquals("new-device-id", saved.trezorDeviceId) assertTrue(saved.xpubs.values.none { it == "old-seed-xpub" }) @@ -876,7 +968,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals(2, captor.firstValue.size) assertEquals(standard, captor.firstValue.first()) } @@ -900,7 +992,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertFalse(captor.firstValue.single().passphraseProtected) } @@ -924,7 +1016,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertTrue(captor.firstValue.single().passphraseProtected) } @@ -942,7 +1034,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) val saved = captor.firstValue.single() assertFalse(saved.passphraseProtected) } @@ -982,7 +1074,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals( mapOf( "nativeSegwit" to "native-xpub", @@ -1008,7 +1100,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals("Cold Storage", captor.lastValue.single().customLabel) } @@ -1137,7 +1229,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isFailure) assertEquals(DEVICE_BUSY_MESSAGE, sut.state.value.error) assertNull(sut.state.value.connectedDevice()) - verify(hwWalletStore, never()).saveKnownDevices(any()) + verify(hwWalletStore, never()).saveKnownDevices(any(), anyOrNull()) } @Test @@ -1184,7 +1276,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isFailure) assertNull(sut.state.value.connectedDevice()) - verify(hwWalletStore, never()).saveKnownDevices(any()) + verify(hwWalletStore, never()).saveKnownDevices(any(), anyOrNull()) } @Test @@ -1208,7 +1300,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isFailure) assertEquals("DeviceDisconnected", sut.state.value.error) assertNull(sut.state.value.connectedDevice()) - verify(hwWalletStore, never()).saveKnownDevices(any()) + verify(hwWalletStore, never()).saveKnownDevices(any(), anyOrNull()) } @Test @@ -1991,7 +2083,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals(emptyList(), captor.lastValue) assertTrue(sut.state.value.knownDevices.isEmpty()) } @@ -2127,7 +2219,7 @@ class TrezorRepoTest : BaseUnitTest() { assertTrue(result.isSuccess) val captor = argumentCaptor>() - verify(hwWalletStore).saveKnownDevices(captor.capture()) + verify(hwWalletStore).saveKnownDevices(captor.capture(), anyOrNull()) assertEquals(listOf(keptWhenStored), captor.lastValue) } diff --git a/app/src/test/java/to/bitkit/ui/screens/wallets/HwWalletViewModelTest.kt b/app/src/test/java/to/bitkit/ui/screens/wallets/HwWalletViewModelTest.kt index 3a052033da..987a0b8ae1 100644 --- a/app/src/test/java/to/bitkit/ui/screens/wallets/HwWalletViewModelTest.kt +++ b/app/src/test/java/to/bitkit/ui/screens/wallets/HwWalletViewModelTest.kt @@ -10,6 +10,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.test.advanceUntilIdle import org.junit.Before import org.junit.Test +import org.mockito.kotlin.any import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -18,6 +19,7 @@ import to.bitkit.R import to.bitkit.models.HwWallet import to.bitkit.models.Toast import to.bitkit.models.TransportType +import to.bitkit.repositories.HwBackupDataUnreadableError import to.bitkit.repositories.HwWalletRepo import to.bitkit.repositories.HwWalletRepo.Companion.DEVICE_LABEL_MAX_LENGTH import to.bitkit.test.BaseUnitTest @@ -62,6 +64,7 @@ class HwWalletViewModelTest : BaseUnitTest() { whenever(hwWalletRepo.walletsLoaded).thenReturn(MutableStateFlow(true)) whenever(context.getString(R.string.common__error)).thenReturn("Error") whenever(context.getString(R.string.hardware__remove_error)).thenReturn("Could not remove") + whenever(context.getString(R.string.hardware__remove_keep_error)).thenReturn("Could not keep the tags") whenever(context.getString(R.string.hardware__rename_error)).thenReturn("Could not rename") } @@ -98,20 +101,57 @@ class HwWalletViewModelTest : BaseUnitTest() { @Test fun `removeDevice delegates to the repo and clears the pending device`() = test { - whenever { hwWalletRepo.removeDevice("dev1") }.thenReturn(Result.success(Unit)) + whenever { hwWalletRepo.removeDevice("dev1", true) }.thenReturn(Result.success(Unit)) val sut = createSut() sut.onRemoveClick(wallet) sut.removeDevice("dev1") advanceUntilIdle() - verify(hwWalletRepo).removeDevice("dev1") + verify(hwWalletRepo).removeDevice("dev1", true) assertNull(sut.uiState.value.isPendingRemoval) } + @Test + fun `onRemoveClick offers to keep the backup data`() = test { + val sut = createSut() + + sut.onRemoveClick(wallet) + + assertEquals(true, sut.uiState.value.keepBackupDataOnRemoval) + } + + @Test + fun `onRemoveClick restores the default after a wallet was removed without keeping its data`() = test { + whenever { hwWalletRepo.removeDevice(any(), any()) }.thenReturn(Result.success(Unit)) + val sut = createSut() + sut.onRemoveClick(wallet) + sut.onKeepBackupDataChange(false) + sut.removeDevice("dev1") + advanceUntilIdle() + + // Reopening starts from the default rather than from the last choice. + sut.onRemoveClick(otherWallet) + + assertEquals(true, sut.uiState.value.keepBackupDataOnRemoval) + } + + @Test + fun `removeDevice forwards the choice to keep nothing`() = test { + whenever { hwWalletRepo.removeDevice("dev1", false) }.thenReturn(Result.success(Unit)) + val sut = createSut() + sut.onRemoveClick(wallet) + sut.onKeepBackupDataChange(false) + + sut.removeDevice("dev1") + advanceUntilIdle() + + verify(hwWalletRepo).removeDevice("dev1", false) + } + @Test fun `removeDevice sends an error toast on failure`() = test { - whenever { hwWalletRepo.removeDevice("dev1") }.thenReturn(Result.failure(AppError("nope"))) + whenever { hwWalletRepo.removeDevice(any(), any()) }.thenReturn(Result.failure(AppError("nope"))) val sut = createSut() val toasts = mutableListOf() @@ -123,6 +163,22 @@ class HwWalletViewModelTest : BaseUnitTest() { collectJob.cancel() } + @Test + fun `removeDevice explains an unreadable backup data failure`() = test { + whenever { hwWalletRepo.removeDevice(any(), any()) } + .thenReturn(Result.failure(HwBackupDataUnreadableError(AppError("core unavailable")))) + val sut = createSut() + + val toasts = mutableListOf() + val collectJob = launch { ToastEventBus.events.collect { toasts.add(it) } } + sut.removeDevice("dev1") + advanceUntilIdle() + + // The generic retry message would hide that removing without keeping the data still works. + assertEquals("Could not keep the tags", toasts.single().description) + collectJob.cancel() + } + @Test fun `onRenameClick opens the rename sheet with the current name`() = test { val sut = createSut() diff --git a/changelog.d/next/1173.added.md b/changelog.d/next/1173.added.md new file mode 100644 index 0000000000..451e942706 --- /dev/null +++ b/changelog.d/next/1173.added.md @@ -0,0 +1 @@ +The name you give a hardware wallet is now included in your backup and comes back when you pair the device again, and removing a hardware wallet asks first whether to keep its name and tags in your backup.