diff --git a/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt b/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt index 83f317c055..f71e02f460 100644 --- a/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt @@ -15,7 +15,11 @@ import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList import kotlinx.collections.immutable.toImmutableSet import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Job import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.async import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow @@ -30,6 +34,7 @@ import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update +import kotlinx.coroutines.job import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -103,6 +108,7 @@ class HwWalletRepo @Inject constructor( private const val WATCHER_ID_SEPARATOR = "|" private val WATCHER_START_RETRY_DELAY = 30.seconds const val DEVICE_LABEL_MAX_LENGTH = 50 + private const val MAX_FEE_ESTIMATES = 32 /** Trezor v1 (2.4.0) tracks native SegWit accounts. */ private val SUPPORTED_WATCHER_ADDRESS_TYPES = setOf(HwFundingAddressType.NATIVE_SEGWIT.settingsKey) @@ -116,6 +122,24 @@ class HwWalletRepo @Inject constructor( private var scanBluetoothVendor = HwWalletVendor.TREZOR private val scope = appScope(ioDispatcher, TAG) + + /** + * Exact-amount fee composes, shared by every caller asking for the same payment. A compose runs + * on the single core queue and takes seconds, so choosing a funding source, the confirm screen + * and a refresh that replaces a cancelled one all reuse one result. Keyed by the funding + * account's state (balance and transactions), so any change to its coins composes again; a + * failed compose is not kept. + */ + private val feeEstimates = mutableMapOf>>() + private val feeEstimatesLock = Any() + + private data class FeeEstimateKey( + val walletId: String, + val address: String, + val sats: ULong, + val satsPerVByte: ULong, + val fundingState: Int, + ) private val watcherMutex = Mutex() private val sessionMutex = Mutex() @@ -754,14 +778,59 @@ class HwWalletRepo @Inject constructor( address: String, sats: ULong, satsPerVByte: ULong, - ): Result = withContext(ioDispatcher) { - runSuspendCatching { - composeFundingOffline( - walletId = walletId, - output = ComposeOutput.Payment(address = address, amountSats = sats), - satsPerVByte = satsPerVByte, - ).fee + ): Result { + val key = FeeEstimateKey( + walletId = walletId, + address = address, + sats = sats, + satsPerVByte = satsPerVByte, + fundingState = fundingState(walletId), + ) + val estimate = synchronized(feeEstimatesLock) { + feeEstimates.keys.removeAll { it.walletId == walletId && it.fundingState != key.fundingState } + if (feeEstimates.size >= MAX_FEE_ESTIMATES && key !in feeEstimates) feeEstimates.clear() + feeEstimates[key] ?: run { + // Owned by the repository scope, so a caller that is cancelled mid-compose leaves + // the result for the next caller instead of discarding it. + // Started only once stored, so a compose that ends at once can still drop itself. + val created = scope.async(ioDispatcher, start = CoroutineStart.LAZY) { + runSuspendCatching { + composeFundingOffline( + walletId = walletId, + output = ComposeOutput.Payment(address = address, amountSats = sats), + satsPerVByte = satsPerVByte, + ).fee + }.also { result -> + // Dropped by the compose itself, not by a waiting caller, which may be gone. + if (result.isFailure) forgetFeeEstimate(key, coroutineContext.job) + } + } + feeEstimates[key] = created + created.invokeOnCompletion { cause -> if (cause != null) forgetFeeEstimate(key, created) } + created.start() + created + } + } + return estimate.await() + } + + private fun forgetFeeEstimate(key: FeeEstimateKey, estimate: Job) = synchronized(feeEstimatesLock) { + if (feeEstimates[key] === estimate) feeEstimates.remove(key) + } + + /** + * Changes whenever the funding account's coins can have changed: any transaction added, replaced + * or dropped, or a balance change. The balance alone can return to the same total over other coins. + */ + private fun fundingState(walletId: String): Int { + val funding = _watcherData.value.values.filter { + it.walletId == walletId && it.addressType == HwFundingAddressType.DEFAULT } + val balance = funding.fold(0uL) { acc, watcher -> acc + watcher.balanceSats } + val txIds = funding.flatMap { watcher -> + watcher.activities.filterIsInstance().map { it.v1.txId } + }.sorted() + return (listOf(balance.toString()) + txIds).hashCode() } /** Exact amount available after the coin-selection fee, computed offline from the account xpub. */ diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt index 1d6d166319..ee73be6dff 100644 --- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt +++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt @@ -132,6 +132,7 @@ import to.bitkit.ext.watchUntil import to.bitkit.flags.PaykitFeatureFlags import to.bitkit.models.FeeRate import to.bitkit.models.HwFundingSignedTx +import to.bitkit.models.HwWallet import to.bitkit.models.HwWalletVendor import to.bitkit.models.NewTransactionSheetDetails import to.bitkit.models.NewTransactionSheetDirection @@ -4219,8 +4220,10 @@ class AppViewModel @Inject constructor( else -> false } - // A scanned amount is the payee's, even when it equals the hardware maximum. - val wasHardwareMax = previous.hardwareWalletId != null && previous.fixedOnchainAmount == null && + // A scanned amount or a payment request's amount is the payee's, so it never follows the + // send-max amount, even when it equals what the wallet can spend. + val wasHardwareMax = activeIncomingPaymentRequest() == null && + previous.hardwareWalletId != null && previous.fixedOnchainAmount == null && previous.amount > 0uL && previous.amount == previous.hardwareAvailableSats val hardwareAvailableSats = previous.hardwareWalletId?.let { walletId -> @@ -4465,8 +4468,8 @@ class AppViewModel @Inject constructor( walletGeneration: Long? = connectionRecovery.currentGeneration(), ): Boolean { val state = _sendUiState.value - for (wallet in hwWalletRepo.wallets.value) { - val available = hardwareMaxSpendable(wallet.id, state.address, state.speed, walletGeneration) + for (wallet in hwWalletRepo.wallets.value.sortedByDescending { it.fundingBalanceSats }) { + val available = hardwareAvailableForPayment(wallet, amount, state, walletGeneration) if (amount > available) continue _sendUiState.update { it.copy( @@ -4479,13 +4482,39 @@ class AppViewModel @Inject constructor( confirmedWarnings = persistentListOf(), ) } + // The caller refreshes the confirm fee once it navigates; refreshing here too would + // start a refresh that the caller's then cancels. updateCanSwitchWallet() - refreshOnchainSendIfNeeded(walletGeneration) return true } return false } + /** + * What [wallet] can spend toward [amount]. Composing the exact payment proves it can pay and + * leaves its fee in [HwWalletRepo] for the confirm screen, so the request is composed once. + * When that fails, the max-spendable compose says how far short the wallet is. + */ + private suspend fun hardwareAvailableForPayment( + wallet: HwWallet, + amount: ULong, + state: SendUiState, + walletGeneration: Long?, + ): ULong { + val satsPerVByte = state.feeRates?.getSatsPerVByteFor(state.speed) ?: 0u + if (wallet.fundingBalanceSats >= amount && satsPerVByte > 0u) { + requireCurrentPaymentPreparation(walletGeneration) + val fee = hwWalletRepo.estimateFundingMiningFee( + walletId = wallet.id, + address = state.address, + sats = amount, + satsPerVByte = satsPerVByte.toULong(), + ).also { requireCurrentPaymentPreparation(walletGeneration) }.getOrNull() + if (fee != null) return wallet.fundingBalanceSats.safe() - fee.safe() + } + return hardwareMaxSpendable(wallet.id, state.address, state.speed, walletGeneration) + } + fun switchToLightning() { viewModelScope.launch { sendStateGeneration++ @@ -7195,7 +7224,9 @@ class AppViewModel @Inject constructor( } requireCurrentPaymentPreparation(walletGeneration) if (generation != sendStateGeneration) return@launchOnchainFeeRefresh - refreshOnchainFeeUi(walletGeneration) + // Each hardware fee is a compose of several seconds on the single core queue; signing + // would wait behind the other speeds, so they are estimated when the picker opens. + refreshOnchainFeeUi(walletGeneration, selectedSpeedOnly = currentState.hardwareWalletId != null) } } @@ -7236,8 +7267,13 @@ class AppViewModel @Inject constructor( return job } + /** + * With [selectedSpeedOnly], only the selected speed's fee is estimated and `estimates` is left + * empty, so opening the speed picker estimates the rest ([SendEvent.SpeedAndFee]). + */ private suspend fun refreshOnchainFeeUi( walletGeneration: Long? = connectionRecovery.currentGeneration(), + selectedSpeedOnly: Boolean = false, ) = withContext(bgDispatcher) { requireCurrentPaymentPreparation(walletGeneration) val generation = sendStateGeneration @@ -7245,6 +7281,24 @@ class AppViewModel @Inject constructor( if (currentState.isRetainedHardwarePayment) return@withContext updateOnchainFeeUi { it.copy(isLoading = true) } + if (selectedSpeedOnly) { + val fee = if (currentState.feeRates?.getSatsPerVByteFor(currentState.speed) != 0u) { + getFeeEstimate(currentState.speed, walletGeneration) + } else { + 0 + } + requireCurrentPaymentPreparation(walletGeneration) + if (generation != sendStateGeneration) return@withContext + updateOnchainFeeUi { + OnchainFeeUi( + rate = FeeRate.fromSpeed(currentState.speed), + sats = fee.takeIf { sats -> sats > 0 }, + isLoading = false, + ) + } + return@withContext + } + val speeds = listOf( TransactionSpeed.Fast, TransactionSpeed.Medium, diff --git a/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt b/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt index 980b221254..b83d7325e4 100644 --- a/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/HwWalletRepoTest.kt @@ -2302,6 +2302,167 @@ class HwWalletRepoTest : BaseUnitTest() { assertEquals(25_000uL, result.getOrThrow()) } + @Test + fun `estimateFundingMiningFee composes the same payment once`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) + whenever( + trezorRepo.composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + ).thenReturn( + Result.success( + listOf(ComposeResult.Success(psbt = "psbt", fee = 1_250uL, feeRate = 2.0f, totalSpent = 26_250uL)) + ) + ) + val sut = createRepo() + + val first = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + val second = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + val otherRate = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 5uL) + + assertEquals(1_250uL, first.getOrThrow()) + assertEquals(1_250uL, second.getOrThrow()) + assertEquals(true, otherRate.isSuccess) + verify(trezorRepo, times(2)).composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + } + + @Test + fun `estimateFundingMiningFee composes again when coins change at the same balance`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) + whenever( + trezorRepo.composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + ).thenReturn( + Result.success( + listOf(ComposeResult.Success(psbt = "psbt", fee = 1_250uL, feeRate = 2.0f, totalSpent = 26_250uL)) + ) + ) + val sut = createRepo() + suspend fun emitFunding(vararg txIds: String) = watcherEvents.emit( + "$HARDWARE_WALLET_ID|nativeSegwit" to WatcherEvent.TransactionsChanged( + balance = walletBalance(total = 50_000uL), + activities = txIds.map { watcherActivity(amount = 1uL, txid = it) }, + transactionDetails = emptyList(), + txCount = txIds.size.toUInt(), + blockHeight = 1u, + accountType = AccountType.NATIVE_SEGWIT, + nextUnusedExternalAddress = WATCHER_RECEIVE_ADDRESS, + ) + ) + + emitFunding("a") + runCurrent() + sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + emitFunding("a") + runCurrent() + sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + emitFunding("a", "b", "c") + runCurrent() + sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + + verify(trezorRepo, times(2)).composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + } + + @Test + fun `estimateFundingMiningFee composes again after a failure`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) + whenever( + trezorRepo.composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + ).thenReturn(Result.failure(Exception("electrum unavailable"))) + val sut = createRepo() + + val first = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + val second = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + + assertEquals(true, first.isFailure) + assertEquals(true, second.isFailure) + verify(trezorRepo, times(2)).composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + } + + @Test + fun `estimateFundingMiningFee composes again when a cancelled caller's compose fails`() = test { + whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) + val finishFirst = CompletableDeferred() + var calls = 0 + whenever { + trezorRepo.composeTransactionOffline( + extendedKey = any(), + outputs = any(), + feeRates = any(), + network = any(), + accountType = anyOrNull(), + coinSelection = any(), + fingerprint = anyOrNull(), + ) + }.doSuspendableAnswer { + calls++ + if (calls == 1) { + finishFirst.await() + Result.failure(Exception("electrum unavailable")) + } else { + Result.success( + listOf(ComposeResult.Success(psbt = "psbt", fee = 1_250uL, feeRate = 2.0f, totalSpent = 26_250uL)) + ) + } + } + val sut = createRepo() + + val first = launch { sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) } + runCurrent() + first.cancel() + finishFirst.complete(Unit) + runCurrent() + val second = sut.estimateFundingMiningFee(HARDWARE_WALLET_ID, "bc1qtest", 25_000uL, 2uL) + + assertEquals(1_250uL, second.getOrThrow()) + assertEquals(2, calls) + } + @Test fun `composeFundingTransaction does not sign when compose fails`() = test { whenever(hwWalletStore.loadKnownDevices()).thenReturn(listOf(device)) diff --git a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt index 81c6d613bc..d2701e3b08 100644 --- a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt +++ b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt @@ -13,6 +13,8 @@ import androidx.lifecycle.viewModelScope import app.cash.turbine.test import com.synonym.bitkitcore.AddressType import com.synonym.bitkitcore.FeeRates +import com.synonym.bitkitcore.IBtInfo +import com.synonym.bitkitcore.IBtInfoOnchain import com.synonym.bitkitcore.LightningActivity import com.synonym.bitkitcore.LightningInvoice import com.synonym.bitkitcore.LnurlAddressData @@ -11454,6 +11456,79 @@ class AppViewModelSendFlowTest : BaseUnitTest() { verify(context, never()).getString(R.string.wallet__lnurl_pay__error_min__description) } + @Test + fun `incoming onchain request funded by hardware composes the payment once`() = test { + val request = paymentRequest() + stubIncomingOnchainRequest(request) + val onchainInfo = mock() + val btInfo = mock() + whenever(onchainInfo.feeRates).thenReturn(FeeRates(fast = 5u, mid = 3u, slow = 1u)) + whenever(btInfo.onchain).thenReturn(onchainInfo) + whenever(blocktankRepo.blocktankState).thenReturn(MutableStateFlow(BlocktankState(info = btInfo))) + balanceState.value = BalanceState(maxSendOnchainSats = 1_000u) + hwWallets.value = persistentListOf(hardwareWallet(fundingBalanceSats = 50_000uL)) + whenever { hwWalletRepo.estimateFundingMiningFee(any(), any(), any(), any()) } + .thenReturn(Result.success(250uL)) + whenever { hwWalletRepo.maxSpendableFunding(any(), any(), any()) } + .thenReturn(Result.success(48_000uL)) + sut.setIsAuthenticated(true) + runCurrent() + + sut.openContactPayment( + paymentRequest = REGTEST_ADDRESS, + publicKey = testPublicKey, + incomingPaymentRequest = request, + ) + advanceUntilIdle() + + val state = sut.sendUiState.value + assertEquals(HARDWARE_WALLET_ID, state.hardwareWalletId) + assertEquals(SendMethod.ONCHAIN, state.payMethod) + assertEquals(49_750uL, state.hardwareAvailableSats) + assertFalse(state.onchainFeeUi.isLoading) + assertTrue(state.onchainFeeUi.estimates.isEmpty()) + verify(hwWalletRepo, never()).maxSpendableFunding(any(), any(), any()) + verify(hwWalletRepo, times(2)).estimateFundingMiningFee( + eq(HARDWARE_WALLET_ID), + eq(REGTEST_ADDRESS), + eq(request.amount.atomic), + any(), + ) + } + + @Test + fun `hardware funded request keeps its amount when the speed changes`() = test { + val request = paymentRequest() + stubIncomingOnchainRequest(request) + val onchainInfo = mock() + val btInfo = mock() + whenever(onchainInfo.feeRates).thenReturn(FeeRates(fast = 5u, mid = 3u, slow = 1u)) + whenever(btInfo.onchain).thenReturn(onchainInfo) + whenever(blocktankRepo.blocktankState).thenReturn(MutableStateFlow(BlocktankState(info = btInfo))) + balanceState.value = BalanceState(maxSendOnchainSats = 1_000u) + hwWallets.value = persistentListOf(hardwareWallet(fundingBalanceSats = 50_000uL)) + // A change-free exact payment spending every coin: balance minus its fee equals the amount. + whenever { hwWalletRepo.estimateFundingMiningFee(any(), any(), any(), any()) } + .thenReturn(Result.success(50_000uL - request.amount.atomic)) + whenever { hwWalletRepo.maxSpendableFunding(any(), any(), any()) } + .thenReturn(Result.success(48_000uL)) + sut.setIsAuthenticated(true) + runCurrent() + sut.openContactPayment( + paymentRequest = REGTEST_ADDRESS, + publicKey = testPublicKey, + incomingPaymentRequest = request, + ) + advanceUntilIdle() + assertEquals(request.amount.atomic, sut.sendUiState.value.hardwareAvailableSats) + + sut.setTransactionSpeed(TransactionSpeed.Fast) + advanceUntilIdle() + + assertEquals(request.amount.atomic, sut.sendUiState.value.amount) + assertEquals(48_000uL, sut.sendUiState.value.hardwareAvailableSats) + } + @Test fun `small incoming unified request remains payable over Lightning`() = test { val request = paymentRequest().copy(amount = PaykitAmount(PaykitAsset.BTC, 100uL)) diff --git a/changelog.d/next/1785.fixed.md b/changelog.d/next/1785.fixed.md new file mode 100644 index 0000000000..219037497a --- /dev/null +++ b/changelog.d/next/1785.fixed.md @@ -0,0 +1 @@ +Made payment requests funded from a hardware wallet ready to confirm sooner by composing the payment once.