-
Notifications
You must be signed in to change notification settings - Fork 4
feat: deep link spending hw sign #1176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ import to.bitkit.viewmodels.TransferViewModel | |
| @Composable | ||
| fun SpendingHwSignScreen( | ||
| walletId: String, | ||
| orderId: String, | ||
| viewModel: TransferViewModel, | ||
| onBackClick: () -> Unit, | ||
| onCloseClick: () -> Unit, | ||
|
|
@@ -50,7 +51,7 @@ fun SpendingHwSignScreen( | |
| ) { | ||
| val state by viewModel.spendingUiState.collectAsStateWithLifecycle() | ||
|
|
||
| val order = state.order ?: run { | ||
| val order = state.order?.takeIf { it.id == orderId } ?: run { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking — this guard breaks the existing Advanced flow.
Reproduced on device: HW detail → Transfer To Spending → 25% → Continue → Sign → Advanced → MAX → Continue lands on the wallet home screen, and the freshly created order is stranded. Logcat: and Suggest accepting |
||
| onCloseClick() | ||
| return | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,19 @@ object ScreenDeepLinks { | |
| fun isScreenDeepLink(uri: Uri): Boolean = | ||
| uri.scheme?.lowercase() == SCHEME && uri.host?.lowercase() == HOST | ||
|
|
||
| fun spendingHwSignLink(uri: Uri): SpendingHwSignLink? { | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike |
||
| if (!isScreenDeepLink(uri)) return null | ||
| val segments = uri.pathSegments.orEmpty() | ||
| val screenId = kebabId(Routes.SpendingHwSign::class) | ||
| if (segments.size != 3 || screenId == null || !segments[0].equals(screenId, ignoreCase = true)) { | ||
| return null | ||
| } | ||
| val walletId = segments[1] | ||
| val orderId = segments[2] | ||
| if (walletId.isBlank() || orderId.isBlank()) return null | ||
| return SpendingHwSignLink(walletId = walletId, orderId = orderId) | ||
| } | ||
|
|
||
| fun detachScreenUri(intent: Intent): Boolean { | ||
| val uri = intent.data ?: return false | ||
| if (!isScreenDeepLink(uri)) return false | ||
|
|
@@ -46,3 +59,8 @@ object ScreenDeepLinks { | |
| return true | ||
| } | ||
| } | ||
|
|
||
| data class SpendingHwSignLink( | ||
| val walletId: String, | ||
| val orderId: String, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ class TransferViewModel @Inject constructor( | |
| hwMiningFeeSats = 0uL, | ||
| ) | ||
| } | ||
| setTransferEffect(TransferEffect.OnOrderCreated) | ||
| setTransferEffect(TransferEffect.OnOrderCreated(newOrder.id)) | ||
| }.onFailure { e -> | ||
| setTransferEffect(TransferEffect.ToastException(e)) | ||
| } | ||
|
|
@@ -581,6 +581,31 @@ class TransferViewModel @Inject constructor( | |
|
|
||
| private suspend fun onOrderCreated(order: IBtOrder) { | ||
| settingsStore.update { it.copy(lightningSetupStep = 0) } | ||
| adoptSpendingOrder(order) | ||
| setTransferEffect(TransferEffect.OnOrderCreated(order.id)) | ||
| } | ||
|
|
||
| suspend fun prepareSpendingHwSign(walletId: String, orderId: String): Boolean { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Dev-mode only, so low severity, but cheap to guard: refuse the link (or skip the clobber) while |
||
| if (walletId.isBlank() || orderId.isBlank()) return false | ||
| if (hwWalletRepo.wallets.value.none { it.id == walletId }) { | ||
| Logger.warn("Refused spending hw sign deeplink, unknown wallet '$walletId'", context = TAG) | ||
| return false | ||
| } | ||
| val current = _spendingUiState.value.order | ||
| if (current?.id == orderId) return true | ||
|
|
||
| val order = blocktankRepo.getOrder(orderId, refresh = true).getOrNull() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth confirming this is the intent: Verified on device — I created a valid order via the Blocktank API with this node's while a locally-created order id opened the sign screen fine. That's the right behaviour if the link is only ever meant to resume a transfer started on this device; it does mean a link handed over from another device or from support tooling can never resolve. Fine to leave as-is — just flagging it so the constraint is deliberate. |
||
| if (order == null) { | ||
| Logger.warn("Refused spending hw sign deeplink, missing order '$orderId'", context = TAG) | ||
| return false | ||
| } | ||
|
|
||
| settingsStore.update { it.copy(lightningSetupStep = 0) } | ||
| adoptSpendingOrder(order) | ||
| return true | ||
| } | ||
|
|
||
| private fun adoptSpendingOrder(order: IBtOrder) { | ||
| pendingHwFundingBroadcast = null | ||
| hwFeeEstimateJob?.cancel() | ||
| hwFeeEstimateJob = null | ||
|
|
@@ -593,7 +618,6 @@ class TransferViewModel @Inject constructor( | |
| hwMiningFeeSats = 0uL, | ||
| ) | ||
| } | ||
| setTransferEffect(TransferEffect.OnOrderCreated) | ||
| } | ||
|
|
||
| private fun updateAvailableAmount() { | ||
|
|
@@ -1673,7 +1697,7 @@ data class TransferValues( | |
| ) | ||
|
|
||
| sealed interface TransferEffect { | ||
| data object OnOrderCreated : TransferEffect | ||
| data class OnOrderCreated(val orderId: String) : TransferEffect | ||
| data object OnSpendingFundingPaid : TransferEffect | ||
| data object OnHwTxSigned : TransferEffect | ||
| data class ToastException(val e: Throwable) : TransferEffect | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Dev-mode deep links can open the hardware-wallet transfer sign screen from a wallet id and Blocktank order id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This warn duplicates the specific reason already logged inside
prepareSpendingHwSign, and reuses the exact wording of the generic!handledwarn a few lines below. Observed on device — one refused link produces two WARN lines, the second of which says "Unhandled" when the link was in fact recognized and deliberately refused:Per the repo rule (NEVER duplicate error logging in
.onFailure {}if the called method already logs the same error internally), drop this line or reword it so it doesn't collide with the generic one.Separately,
consumeScreenDeepLink()now appears three times in this effect. It can't be hoisted to the top — that's what f405cd3 fixed, since the effect is keyed onpendingScreenDeepLinkand consuming early cancels the coroutine mid-prepareSpendingHwSign— but the three calls can collapse into a single one at the end by turning the two early returns into an if/else chain.