feat(cutover): BIP70/BIP270 via the SDK deferred build/broadcast surface (Phase 1B item 1) - #1531
Conversation
📝 WalkthroughWalkthroughThe wallet adds deferred SDK-backed BIP70/BIP270 payments with input reservations, acknowledgment-aware submission, transaction bridging, and UI preview state. Tests cover successful sends, reservation cleanup, invalid outputs, and recipient extraction. CTXSpend logging and test fixtures also receive small updates. ChangesBIP70 SDK payment cutover
Logging and test maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt (2)
68-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNew ViewModel state is not observable and is not part of a
UIState.
deferredPayment,previewFee, andcanSendPaymentare plain properties. The Fragment reads them imperatively at several points. The repository guidelines require a singleUIStatedata class exposed through a private_uiStatewith a publicuiStateviaasStateFlow(), usingStateFlowfor asynchronously updated fields.previewFeeandcanSendPaymentare both updated asynchronously fromDispatchers.IO.The existing
LiveDatafields in this class predate this PR. Consider adding the new preview state to aStateFlow-backedUIStateinstead of extending the imperative pattern.As per coding guidelines: "ViewModels should use a single
UIStatedata class rather than multiple separate flows" and "UseStateFlow(notLiveData) for asynchronously updated fields in ViewModels".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt` around lines 68 - 86, Replace the imperative deferredPayment, previewFee, and canSendPayment properties with a single StateFlow-backed UIState containing the preview data and sendability flag. Add private _uiState and public uiState via asStateFlow(), update the state from the existing IO preview/send paths, and adjust Fragment consumers to collect uiState rather than read these properties imperatively; preserve the existing fee precedence and sendability behavior.Source: Coding guidelines
276-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an injected application-scoped
CoroutineScopeoverGlobalScope.The release must outlive
viewModelScope, so a detached scope is correct here.GlobalScopehas no lifecycle owner and cannot be replaced in tests. Inject an@ApplicationScope CoroutineScopeand launch on it. This keeps the same behavior and removes theDelicateCoroutinesApiopt-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt` around lines 276 - 290, In the PaymentProtocolViewModel class's onCleared method, replace the GlobalScope.launch(Dispatchers.IO) call with an injected application-scoped CoroutineScope. Inject the scope as a constructor dependency (likely with an `@ApplicationScope` qualifier), and use it to launch the sendCoinsTaskRunner.releaseDeferredPayment(payment) operation instead. Remove the `@OptIn`(kotlinx.coroutines.DelicateCoroutinesApi::class) annotation since the injected scope no longer requires it.wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt (2)
858-858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
SdkDeferredPaymentinstead of using the fully qualified name.Line 858 is about 128 characters long. ktlint's default
max-line-lengthfor Android is 120, so this line likely fails the format check. The same fully qualified name repeats on lines 886, 887, 900, and 933. Add an import and use the short name.♻️ Proposed change
Add the import near the other
de.schildbach.wallet.service.platform.sdkimports:import de.schildbach.wallet.service.platform.sdk.SdkDeferredPaymentThen shorten the signatures:
- suspend fun buildDeferredBip70Payment(paymentIntent: PaymentIntent): de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment { + suspend fun buildDeferredBip70Payment(paymentIntent: PaymentIntent): SdkDeferredPayment {- suspend fun releaseDeferredPayment(payment: de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment) = + suspend fun releaseDeferredPayment(payment: SdkDeferredPayment) = sdkL1SendService.releaseDeferredPayment(payment)As per coding guidelines: "Format Kotlin code with ktlint".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt` at line 858, The buildDeferredBip70Payment function signature uses the fully qualified name de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment which causes line 858 to exceed ktlint's 120-character limit. Add an import statement for SdkDeferredPayment from de.schildbach.wallet.service.platform.sdk near the other service.platform.sdk imports, then replace all occurrences of the fully qualified SdkDeferredPayment name with the short name SdkDeferredPayment throughout the file (on the function signature at line 858 and all other usages on lines 886, 887, 900, and 933).Source: Coding guidelines
1007-1017: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider emitting the send analytics event on the SDK BIP70 path.
The dashj path funnels through
sendCoins, which callslogSendTxEvent(transaction, wallet)after a successful commit.sendPrebuiltDirectPaymentreturns the bridged transaction without that call. Post-cutover, BIP70 payments then stop reporting the send event. Add the call on theBridgedbranch, contained inrunCatchingso analytics cannot fail the payment.♻️ Proposed change
is de.schildbach.wallet.service.platform.sdk.BridgedTxResult.Bridged -> + { + walletData.wallet?.let { w -> + runCatching { logSendTxEvent(bridged.transaction, w) } + .onFailure { log.warn("failed to log the BIP70 send event", it) } + } bridged.transaction + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt` around lines 1007 - 1017, In the Bridged branch of the when expression that handles bridgedTransactionFactory.bridge results, add a call to logSendTxEvent before returning bridged.transaction. Wrap the logSendTxEvent call in runCatching to ensure any analytics failure does not prevent the payment from succeeding, and then return the transaction after the analytics call completes.wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt (2)
855-891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the refund address in the submitted
Paymentmessage.The comment states that the refund address "goes into the Payment message", but the test only asserts the HTTP method. Parse the recorded request body and check
refund_to. This makes the refund_to arm actually covered.♻️ Proposed change
// And the Payment message actually went over HTTP. assertEquals(1, mockWebServer.requestCount) - assertEquals("POST", mockWebServer.takeRequest().method) + val recorded = mockWebServer.takeRequest() + assertEquals("POST", recorded.method) + val submitted = Protos.Payment.parseFrom(recorded.body.readByteArray()) + assertEquals(1, submitted.refundToCount) + assertEquals(1, submitted.transactionsCount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt` around lines 855 - 891, The test comment documents that the refund address goes into the Payment message but only verifies the HTTP method. After the existing mockWebServer.takeRequest() call, parse the request body to extract the Payment message protobuf and assert that the refund_to field matches the address returned by sdkL1SendService.refundAddressOrNull(), which is mocked as "yWdXnYxGbouNoo8yMvcbZmZ3Gdp6BpySxL". This ensures the refund_to arm is actually covered and validates that the address flows through to the HTTP submission.
985-1026: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the post-ack no-release invariant.
The current tests cover release on NACK and on transport failure, plus no-release on full success. The most safety-critical rule is not covered: after an ACK, a refused or ambiguous SDK broadcast must still not release the reservation, and a
NotBridgedresult must throwBip70AckedDisplayException. Add two tests for those branches.Do you want me to generate these two test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt` around lines 985 - 1026, Add two new test methods to the SendCoinsTaskRunnerBIP70Test class to cover the post-ack no-release safety invariant. The first test should verify that after receiving an ACK, when the SDK broadcast returns a refused or ambiguous result, the payment reservation is not released. The second test should verify that after receiving an ACK, when the SDK broadcast returns a NotBridged result, a Bip70AckedDisplayException is thrown. These tests complement the existing coverage for release on NACK, transport failure, and full success scenarios.wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolFragment.kt (1)
226-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the substring match on the exception message with a typed check.
This branch decides which dialog the user sees based on
"Insufficient funds"appearing in a native error message. The message originates in the Rust key-wallet layer and is not a stable contract. Any upstream wording change silently routes a shortfall to the generic error dialog.
SdkL1SendServicealready owns a shortfall predicate (isSendAllShortfall) for the same text. Either expose that predicate for reuse, or havebuildDeferredBip70Paymenttranslate a shortfall intoInsufficientMoneyExceptionso thiswhenneeds no new branch. The second option keeps the UI free of SDK error-text knowledge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolFragment.kt` around lines 226 - 232, Replace the substring match on resource.exception?.message?.contains("Insufficient funds") in the else branch of PaymentProtocolFragment with a typed exception check. Have buildDeferredBip70Payment detect shortfalls (using the existing isSendAllShortfall predicate from SdkL1SendService or equivalent logic) and translate them into InsufficientMoneyException before returning to the caller. This way the UI checks for the typed exception (which likely already exists as a case in the when expression) rather than parsing unstable error-text strings, and removes the brittle substring match entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt`:
- Around line 223-231: Make deferred payment reservation handoff atomic to
prevent overlapping sendPayment calls from submitting the same payment. Store
deferredPayment in an atomic reference, use getAndSet(null) in sendPayment
before invoking sendPrebuiltDirectPayment, apply the same atomic clear in
onCleared, and replace assignment in createBaseSendRequest with
_deferredPayment.set(...).
In `@wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt`:
- Around line 912-917: Update the DirectPayException catch block in the
sendDirectPayment test to assert that the caught exception’s message matches the
expected NACK failure reason, instead of leaving the catch block empty. Preserve
the existing fail assertion for the no-exception path.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt`:
- Line 858: The buildDeferredBip70Payment function signature uses the fully
qualified name de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment
which causes line 858 to exceed ktlint's 120-character limit. Add an import
statement for SdkDeferredPayment from de.schildbach.wallet.service.platform.sdk
near the other service.platform.sdk imports, then replace all occurrences of the
fully qualified SdkDeferredPayment name with the short name SdkDeferredPayment
throughout the file (on the function signature at line 858 and all other usages
on lines 886, 887, 900, and 933).
- Around line 1007-1017: In the Bridged branch of the when expression that
handles bridgedTransactionFactory.bridge results, add a call to logSendTxEvent
before returning bridged.transaction. Wrap the logSendTxEvent call in
runCatching to ensure any analytics failure does not prevent the payment from
succeeding, and then return the transaction after the analytics call completes.
In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolFragment.kt`:
- Around line 226-232: Replace the substring match on
resource.exception?.message?.contains("Insufficient funds") in the else branch
of PaymentProtocolFragment with a typed exception check. Have
buildDeferredBip70Payment detect shortfalls (using the existing
isSendAllShortfall predicate from SdkL1SendService or equivalent logic) and
translate them into InsufficientMoneyException before returning to the caller.
This way the UI checks for the typed exception (which likely already exists as a
case in the when expression) rather than parsing unstable error-text strings,
and removes the brittle substring match entirely.
In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt`:
- Around line 68-86: Replace the imperative deferredPayment, previewFee, and
canSendPayment properties with a single StateFlow-backed UIState containing the
preview data and sendability flag. Add private _uiState and public uiState via
asStateFlow(), update the state from the existing IO preview/send paths, and
adjust Fragment consumers to collect uiState rather than read these properties
imperatively; preserve the existing fee precedence and sendability behavior.
- Around line 276-290: In the PaymentProtocolViewModel class's onCleared method,
replace the GlobalScope.launch(Dispatchers.IO) call with an injected
application-scoped CoroutineScope. Inject the scope as a constructor dependency
(likely with an `@ApplicationScope` qualifier), and use it to launch the
sendCoinsTaskRunner.releaseDeferredPayment(payment) operation instead. Remove
the `@OptIn`(kotlinx.coroutines.DelicateCoroutinesApi::class) annotation since the
injected scope no longer requires it.
In `@wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt`:
- Around line 855-891: The test comment documents that the refund address goes
into the Payment message but only verifies the HTTP method. After the existing
mockWebServer.takeRequest() call, parse the request body to extract the Payment
message protobuf and assert that the refund_to field matches the address
returned by sdkL1SendService.refundAddressOrNull(), which is mocked as
"yWdXnYxGbouNoo8yMvcbZmZ3Gdp6BpySxL". This ensures the refund_to arm is actually
covered and validates that the address flows through to the HTTP submission.
- Around line 985-1026: Add two new test methods to the
SendCoinsTaskRunnerBIP70Test class to cover the post-ack no-release safety
invariant. The first test should verify that after receiving an ACK, when the
SDK broadcast returns a refused or ambiguous result, the payment reservation is
not released. The second test should verify that after receiving an ACK, when
the SDK broadcast returns a NotBridged result, a Bip70AckedDisplayException is
thrown. These tests complement the existing coverage for release on NACK,
transport failure, and full success scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc4c4535-92c0-4809-93ef-1ee5b920efe0
📒 Files selected for processing (9)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/dialogs/GiftCardDetailsViewModel.ktwallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.ktwallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.ktwallet/src/de/schildbach/wallet/ui/send/PaymentProtocolFragment.ktwallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.ktwallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.ktwallet/test/de/schildbach/wallet/service/WalletTransactionMetadataProviderGiftCardTest.ktwallet/test/de/schildbach/wallet/service/WalletTransactionMetadataProviderObserveTest.ktwallet/test/de/schildbach/wallet/util/viewModels/MainViewModelTest.kt
💤 Files with no reviewable changes (1)
- wallet/test/de/schildbach/wallet/util/viewModels/MainViewModelTest.kt
| val prebuilt = deferredPayment | ||
| val transaction = if (prebuilt != null) { | ||
| // Post-cutover: submit the EXACT tx the preview showed. | ||
| // The reservation is consumed (ack → broadcast) or | ||
| // released (pre-ack failure) inside the runner either | ||
| // way — this reference is dead after the call. | ||
| deferredPayment = null | ||
| try { | ||
| sendCoinsTaskRunner.sendPrebuiltDirectPayment(prebuilt, finalPaymentIntent!!) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear deferredPayment atomically to prevent a double submission.
sendPayment launches a new coroutine on every call. It reads deferredPayment into prebuilt and only then writes null. Two overlapping calls can both read the same non-null value. The Fragment can trigger overlapping calls: confirmPayment has no re-entry guard, and errorView.setOnConfirmClickListener calls viewModel.sendPayment() again. Both coroutines then POST the same Payment message to the merchant and both broadcast.
The transaction bytes are identical, so this is not a double pay. It still produces duplicate merchant submissions, duplicate ACK handling, and a release-after-consume ordering that depends on engine idempotency. Guard the read-and-clear.
🔒️ Proposed fix
Hold the reference in an atomic and swap it:
- var deferredPayment: de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment? = null
- private set
+ private val _deferredPayment =
+ java.util.concurrent.atomic.AtomicReference<de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment?>(null)
+
+ val deferredPayment: de.schildbach.wallet.service.platform.sdk.SdkDeferredPayment?
+ get() = _deferredPayment.get()Then in sendPayment:
- val prebuilt = deferredPayment
+ val prebuilt = _deferredPayment.getAndSet(null)
val transaction = if (prebuilt != null) {
- deferredPayment = nullApply the same getAndSet(null) in onCleared, and _deferredPayment.set(...) in createBaseSendRequest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wallet/src/de/schildbach/wallet/ui/send/PaymentProtocolViewModel.kt` around
lines 223 - 231, Make deferred payment reservation handoff atomic to prevent
overlapping sendPayment calls from submitting the same payment. Store
deferredPayment in an atomic reference, use getAndSet(null) in sendPayment
before invoking sendPrebuiltDirectPayment, apply the same atomic clear in
onCleared, and replace assignment in createBaseSendRequest with
_deferredPayment.set(...).
| try { | ||
| sendCoinsTaskRunner.sendDirectPayment(sendRequest, paymentIntent) | ||
| fail("Expected DirectPayException for NACK") | ||
| } catch (e: org.dash.wallet.common.services.DirectPayException) { | ||
| // expected | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert on the caught exception so detekt does not flag a swallowed exception.
detekt reports SwallowedException at line 915. The catch block is empty. Add an assertion on the message. This also documents the expected failure reason.
💚 Proposed fix
} catch (e: org.dash.wallet.common.services.DirectPayException) {
- // expected
+ assertTrue(e.message?.contains("not acknowledged") == true)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| sendCoinsTaskRunner.sendDirectPayment(sendRequest, paymentIntent) | |
| fail("Expected DirectPayException for NACK") | |
| } catch (e: org.dash.wallet.common.services.DirectPayException) { | |
| // expected | |
| } | |
| try { | |
| sendCoinsTaskRunner.sendDirectPayment(sendRequest, paymentIntent) | |
| fail("Expected DirectPayException for NACK") | |
| } catch (e: org.dash.wallet.common.services.DirectPayException) { | |
| assertTrue(e.message?.contains("not acknowledged") == true) | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 915-915: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wallet/test/de/schildbach/wallet/payments/SendCoinsTaskRunnerBIP70Test.kt`
around lines 912 - 917, Update the DirectPayException catch block in the
sendDirectPayment test to assert that the caught exception’s message matches the
expected NACK failure reason, instead of leaving the catch block empty. Preserve
the existing fail assertion for the no-exception path.
Source: Linters/SAST tools
48d3f19 to
72f1a3d
Compare
|
Flagging CodeRabbit's finding at |
|
Note on the red CI build: the failure is inherited from the base branch, not introduced here. The build job fails with Local verification on the PR head: unit suites 686/687 (the one failure is the pre-existing pinned-AAR 🤖 Generated with Claude Code |
… route scripts/bip70-test-server.py (stdlib Python, hand-encoded BIP70 protobuf) serves a one-output PaymentRequest and acks the returned Payment; with adb reverse + a dash:?r= VIEW intent it drives the full scanned-invoice flow (PaymentProtocolFragment: fetch -> preview -> confirm -> POST -> ACK) on a device with no external service. Defaults pay 0.01 tDASH back to the testnet faucet's hot wallet (yjSvwyLB5X4dqQqVMPMu6UdrFpYZ3u9v5U, verified on-chain) so test funds recycle. Referenced from CLAUDE.md's testing section, including the post-cutover logcat markers to watch (l1DeferredBuild at preview / l1DeferredBroadcast of the same txid). This is the harness used for the on-device verification of tx 7d9bd5fb94dc7c5cf8cb647004e7e0a74213d1aa9e43766721f677fa8e84965f in PR #1531. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
left a comment
There was a problem hiding this comment.
Requesting changes on two items; everything else looks solid — the deferred build→broadcast contract is implemented correctly (reservations released under NonCancellable on pre-ack HTTP failure and nack, never after ack), the runner-level tests are good, and gating rather than deleting the dashj BIP70 leg matches this branch's established pattern.
1. Double-submission race — still live at PaymentProtocolViewModel.kt ~:223-231. val prebuilt = deferredPayment; … deferredPayment = null runs inside viewModelScope.launch(Dispatchers.IO) on a plain var, and the fragment has no in-flight guard. Two rapid sendPayment() calls both read the same reservation: both submit the same txid (no direct double-spend), but the loser's exception path calls createBaseSendRequest → builds a fresh reservation for the same invoice and can post Resource.error over the winner's success — the user sees "failed" for a paid invoice with the payment re-armed, and one retry tap is a genuine double-pay with different inputs. Fix is small: take the payment atomically (AtomicReference.getAndSet(null) or read+clear on main before the launch) plus an is-sending guard, with a regression test.
2. Rebase needed — the PR currently cannot merge. The base moved (#1525 just merged, plus our send-path work eccd9e7d9): the one real conflict is SdkL1SendService.kt, where both sides independently added ACCOUNT_TYPE_TAG_STANDARD/STANDARD_ACCOUNT_TAG_BIP44 — a forced merge is a duplicate-declaration compile error. SendCoinsTaskRunner auto-merges cleanly with the intendedRecipient/typed-exception work. Good news: the 29 failing tests at this PR's current base are all already fixed on the new base, so they disappear with the rebase.
Post-rebase nit, non-blocking: throw SendNotSdkRoutableException rather than a bare IllegalStateException for the not-routable case, for consistency with the new typed send exceptions.
Since the conflict is with code we wrote, happy to handle the rebase + constant dedupe ourselves if that's easier — say the word; the race fix and its test are probably best done by whoever picks it up first, just flag here to avoid duplicate work.
… build/broadcast surface Issue #1520 Phase 1B item 1: post-cutover, SendCoinsTaskRunner.directPay routes to directPayViaSdk instead of failing closed. The SDK builds and signs the payment-request tx with its inputs RESERVED (buildSignedPayment, platform#4185 token surface in dash-sdk-android 0.1.0-v41int9-SNAPSHOT); the BIP70 Payment message carries the SDK's signed raw bytes; only a merchant ack broadcasts (broadcastSigned) and bridges the tx into the dashj wallet via SdkBridgedTransactionFactory. A nack or transport failure releases the reservation; post-ack the reservation is never released (double-pay hazard - the merchant holds the signed tx). - SdkL1SendService: SdkDeferredPayment holder, build/broadcast/release seam methods + production impls, classifyDeferredBroadcastFailure (reservation-token errors are definitively pre-network). - SendCoinsTaskRunner: cutover gate in directPay, directPayViaSdk, pure extractBip70Recipients (P2PKH/P2SH only, fail-closed otherwise). - Tests: 4 post-cutover flow tests + recipients extraction test (20/20 in SendCoinsTaskRunnerBIP70Test); mechanical fixes for three test files that no longer compiled on this branch (swapOrderDao param, duplicate index arg, duplicate CrowdNode imports). - GiftCardDetailsViewModel: downgrade the stale "redeem url card: not supported" error log to info - redeem-url cards are supported. Verified on testnet (cutover CUT_OVER): CTX staging $5 Brinker gift card, tx 8b9dd641ed27d0606fb2867b1811582362722de6abb0ed7e919b30e0d1337486 - deferred build (inputs reserved) -> Payment POST -> ack -> broadcast -> bridge committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the post-cutover route's last dashj keychain call (wallet.freshAddress(REFUND)) with the SDK's persisted address pool: the lowest-index unused external address of BIP44 account 0 from the Room mirror (accountDao -> coreAddressDao, poolType external, !isUsed, balance 0) - the same canonical current-address pattern the SDK's own KotlinExampleApp ReceiveAddressSheet and iOS nextCoreReceiveAddress use. When the pool rows are unavailable the Payment message omits refund_to (optional per BIP70) instead of falling back to dashj. Known accepted trade-off, documented in the KDoc: the pool carries no issued-marker (isUsed flips only when seen on-chain), so refund_to will often equal the Receive screen's current address in Phase 1B. Upstream ask for dashpay/platform: a fresh_address FFI with an engine-side issued-marker for per-invoice handout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the post-cutover path Post-cutover the PaymentProtocol preview IS the payment: directPayViaSdk is split into buildDeferredBip70Payment (SDK build+sign, inputs reserved, exact fee) and sendPrebuiltDirectPayment (Payment POST -> ack -> broadcast -> bridge). PaymentProtocolViewModel builds the deferred payment at preview time - the confirm screen shows the actual fee of the actual tx, replacing the dashj completeTx dry-run - submits that same prebuilt tx on confirm, rebuilds the preview after retryable (pre-ack) failures, and releases the reservation in onCleared when abandoned. Safety: a post-ack display-bridge failure now throws the typed Bip70AckedDisplayException; the ViewModel never rebuilds after it (the merchant holds the acked tx - a rebuilt retry could double-pay). The fragment gates sends on canSendPayment (either path), reads the fee from previewFee, and maps the SDK's "Insufficient funds" build error to the same dialog as dashj's InsufficientMoneyException. Also sharpens the address-pool KDoc: the engine already exposes core_wallet_next_receive_address in rs-platform-wallet-ffi (iOS binds it directly); the upstream ask is the rs-unified-sdk-jni/Kotlin plumbing, after which unusedExternalAddress swaps its Room read for the FFI call. Verified on testnet post-cutover: CTX $5 Brinker gift card, tx 62aa6214351ff7ad6948d193cc92ddb5902c4ecbe4c1106e716ffba37b530864 (fee 260 duffs) - deferred build -> POST -> ack -> broadcast -> bridge committed; redeem-url card displayed. Unit suites: 686/687 (the one failure is the pre-existing pinned-AAR coreSendMutex canary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… route scripts/bip70-test-server.py (stdlib Python, hand-encoded BIP70 protobuf) serves a one-output PaymentRequest and acks the returned Payment; with adb reverse + a dash:?r= VIEW intent it drives the full scanned-invoice flow (PaymentProtocolFragment: fetch -> preview -> confirm -> POST -> ACK) on a device with no external service. Defaults pay 0.01 tDASH back to the testnet faucet's hot wallet (yjSvwyLB5X4dqQqVMPMu6UdrFpYZ3u9v5U, verified on-chain) so test funds recycle. Referenced from CLAUDE.md's testing section, including the post-cutover logcat markers to watch (l1DeferredBuild at preview / l1DeferredBroadcast of the same txid). This is the harness used for the on-device verification of tx 7d9bd5fb94dc7c5cf8cb647004e7e0a74213d1aa9e43766721f677fa8e84965f in PR #1531. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ions The rebased base introduced SendNotSdkRoutableException with UI mapping (classifySendFailure -> honest "payment type not supported" copy); buildDeferredBip70Payment's non-routable throw adopts it (subclass of IllegalStateException - existing tests and callers unaffected). The three test files this branch had mechanically repaired are restored to the base's own versions, which fixed the same breakages upstream. Payments/SDK/send-UI suites green, including the pinned-AAR canary the old base failed (updated for v41int11 upstream). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
38fd688 to
282d7e5
Compare
PR #1531 review (bfoss765): sendPayment()'s plain read-then-null of the deferred payment across overlapping IO coroutines let two rapid confirms take the same reservation - the loser would rebuild a fresh reservation for the same invoice and post an error over the winner's success, arming a retry that would genuinely double-pay. - deferredPayment is now AtomicReference-backed; sendPayment() takes it with getAndSet(null), createBaseSendRequest swaps the previous reservation atomically before releasing it, onCleared releases via the same atomic take. - sendPayment() is single-flight: a synchronous compareAndSet guard drops a confirm while one is in flight - on the dashj path a duplicate would have built a genuinely second transaction (pre-existing hazard, also closed). Guard released in finally. - PaymentProtocolViewModelRaceTest: duplicate-confirm-dropped + exactly-one-submission, and retryable-failure releases the guard and retries with the REBUILT reservation. - BIP70 test: assert on the expected DirectPayException instead of swallowing it (detekt SwallowedException). Payments/SDK/send-UI suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@bfoss765 all three items addressed — two were already on the head when your review landed (it appears to have caught the pre-rebase tip):
Payments/SDK/send-UI suites green on the current head, including the AAR canary against the v41int11 pin. Ready for re-review. 🤖 Generated with Claude Code |
…nconditional Management policy (final): when a function is replaced by the SDK, the dashj implementation is deleted in the same PR; dashj remains only as the Phase-3 foundation (wallet object, seed, keys, .wallet persistence - #1522), and the engine retires in Phase 2 (#1521). Deleted: - directPay's dashj leg (completeTx/sign/freshAddress/serialize/commit) and its cutover gate - directPayViaSdk is now the only implementation; sendDirectPayment(sendRequest, ...) becomes sendDirectPayment(intent, ...) - the sendPayment dashj final-request builder and the isTransactionOnNetwork bloom-rescue (dashj-confidence based) - PaymentProtocolViewModel's dashj dry-run preview, dashj send branch, baseSendRequest, and the dead commitAndBroadcast helper; the fragment's SendRequest display path (fee comes from previewFee) - the 15 legacy dashj-path BIP70 tests and their SendRequest helper (the SDK-route tests cover the protocol semantics) Added - reservation mirror (TRANSITION-ONLY, delete with Phase 2): until the dashj engine retires, a not-yet-cut-over install still has dashj-side spenders (manual sends, the background CoinJoin mixer - the original reason Wallet.lockOutput exists) that cannot see the SDK's input reservation. buildDeferredBip70Payment locks the reserved outpoints in the foundation wallet; released on abandon/nack and once acked. Regression test asserts lock-on-build / unlock-on-release with real fixture outpoints. Accepted per policy: BIP70 now requires the SDK funding gate on all installs (typed SendEngineNotSyncedException UX), rollback no longer restores a dashj BIP70 leg, and the pre-cutover mempool timeout rescue is gone. Payments / send-UI / SDK suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@bfoss765 heads-up before your re-review: the design basis changed after your review, by management directive (final): replace-then-delete — when a function moves to the SDK, the dashj implementation is deleted in the same PR; dashj stays only as Phase-3 foundation (#1522), engine retirement is Phase 2 (#1521). Concretely, One new piece worth your eyes: because not-yet-cut-over installs still have live dashj-side spenders (manual sends, the background CoinJoin mixer) that can't see the SDK's input reservation, the build now mirrors the reserved outpoints into 🤖 Generated with Claude Code |
… signal The dashj-era isTransactionOnNetwork rescue (deleted with the dashj leg) recovered payments whose HTTP response was lost after the BIP70 server had already broadcast the tx. Rebuild it SDK-side on a STRONGER signal: the engine's TransactionContext (SDK Room transactions.context — 0 mempool, 1 instantSend, 2 inBlock, 3 chainLocked). On a transport/parse failure after the Payment POST (never on an explicit nack), poll the row for ~12s; a context >= instantSend proves the server broadcast the tx (IS-locks only exist for broadcast transactions), so the payment completes as PAID: reservation kept, broadcast tolerated, bridged, live tx returned. Mempool-only deliberately does NOT count (a build-time row must not read as a broadcast). No IS-lock -> release and rethrow, as before. - SdkTxRow gains the context column; DashSdkTxRowSource populates it. - L1SendProbeService.observedTxContext: contained read-only accessor (never boots the SDK). - sendPrebuiltDirectPayment: rescue branch + extracted completeAckedPayment tail shared by ack and rescue. - Tests: rescue-success (IS-locked -> paid, no release) and no-IS-lock -> release; suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@bfoss765 heads-up: this merged at head `437cf5ffa`, five minutes before I pushed `4f28e70e2` — the field-test correction, so it is not in `c3742451b`. Follow-up opened as #1534 (cherry-picks cleanly). Two defects in what landed, from the on-device run:
Note also that local verification of #1534 is blocked: the merged line pins v41int13, which isn't in local Maven, the pin can't be downgraded (the shielded-invite path needs int13's typed error), and the int13 source isn't pushed anywhere I can fetch. If you can share the AAR or push its branch I'll re-run the suites against it. 🤖 Generated with Claude Code |
What
Implements the BIP70 item of #1520 (Phase 1B item 1) under the replace-then-delete policy (management directive, final): the BIP70/BIP270 direct-pay leg is implemented on the Kotlin SDK's deferred build/broadcast surface, and the dashj implementation it replaces is deleted in this same PR. dashj remains in this flow only as Phase-3 foundation (#1522: the wallet object as display/confidence record, keys, persistence) plus one transition-scoped guard that retires with Phase 2 (#1521).
Commits in review order:
buildSignedPayment(platform#4185 token surface in the pinned AAR) builds + signs with inputs reserved; the BIP70Paymentmessage carries the raw signed bytes; merchant ack →broadcastSigned→ bridge commit viaSdkBridgedTransactionFactory(the SDK cutover — Phase 1B: migrate fail-closed / dashj-only send & integration paths (BIP70, coin-selection, asset-lock, Maya, CrowdNode, SwapKit) #1520-prescribed record mechanism); nack/transport failure →releaseReservation; post-ack never releases (double-pay hazard). Recipients via pureextractBip70Recipients(P2PKH/P2SH only; anything else fails closed with the typedSendNotSdkRoutableException).refund_tofrom the SDK's Room address-pool mirror (lowest unused external, the KotlinExampleApp/iOS pattern); swaps tocoreWallet().nextReceiveAddress()when feat(kotlin-sdk): expose core_wallet_next_receive_address / next_change_address (Swift parity) platform#4260 lands in a pinned AAR.PaymentProtocolViewModelbuilds the deferred payment at preview time (exact fee shown), submits that same tx on confirm, releases on abandon;Bip70AckedDisplayExceptionmarks post-ack display failures non-retryable.scripts/bip70-test-server.py+ CLAUDE.md recipe (local invoice server; drives the full scanned-invoice flow viaadb reverse+ adash:?r=intent).SendNotSdkRoutableException; base's own test-file fixes adopted.AtomicReference.getAndSet(null)take + synchronous single-flight guard insendPayment(), withPaymentProtocolViewModelRaceTestcovering duplicate-confirm and retry-after-failure.e4f34bd3e— the deletion — dashjdirectPayleg, ViewModel dashj preview/send branches,baseSendRequest/fragmentSendRequestplumbing,isTransactionOnNetworkbloom rescue, and the 15 legacy dashj-path tests. Plus the reservation mirror (transition-only, tagged for the Phase 2 kill list): the SDK's reserved input outpoints are locked in the foundation wallet viaWallet.lockOutput— the same mechanism that protects CrowdNode coins from the background CoinJoin mixer — so dashj-side spenders on not-yet-cut-over installs cannot double-select them mid-preview; unlocked on release/ack, with a regression test on real fixture outpoints.Accepted per policy (explicit, so nobody is surprised)
BIP70 requires the SDK funding gate on all installs — cold start / mid-first-scan purchases fail with the typed "not synced" UX where dashj would previously have paid.
ROLLBACK_CUTOVERno longer restores a dashj BIP70 leg.The pre-cutover mempool timeout rescue (dashj bloom view) is goneRebuilt SDK-side and field-verified (437cf5ffa, corrected in4f28e70e2): on a transport failure after the Payment POST, the wallet polls the SDK store for ~20s; a row for that txid proves the transaction reached the network — the engine only records transactions it has observed, and a built-but-unbroadcast payment has no row (verified against real txids) — so the payment completes as paid. An explicit nack never rescues.Two defects the on-device test caught and fixed: the first version keyed on
context >= instantSend, which can never fire — the pinned engine takes an externally broadcast txcontext 0 → 2and never surfaces an InstantSend context orisInstantLocked, even when the network has IS-locked it. And the ViewModel used to re-arm the preview after an ambiguous failure: because the engine cannot see the mempool spend of the released inputs, the rebuild selects different inputs, so a retry tap would pay the merchant twice (reproduced: rebuilded08074bvs on-chain2c3be7d8). The preview now re-arms only on a definitive nack.Verified
62aa6214…0864; scanned-invoice preview flow tx7d9bd5fb…965f(exact fee at preview, same txid on confirm, both on the explorer); cancel path txa6cc9ead…9d24(build → back out →l1DeferredRelease, no POST, no broadcast); and the timeout rescue txed08074b…6d39— a test server broadcast the payment to testnet then returned HTTP 500, the engine observed the transaction 1.3 s later, and the payment completed as paid with the reservation kept and the tx bridged (on-chain,txlock: true). The earlier runs predate the deletion commit; the SDK path they exercised is unchanged by it.CI note
The build job fails resolving the pinned
dash-sdk-androidsnapshot (local-Maven only); the base branch fails identically. Format check is green.Phase 2/3 follow-ups this flow will need (tracked, not in scope)
Lock mirror removal (#1521), IS/CL badges for bridged txs via the SDK feed (#1521 item 2), bridge/display migration off the dashj
Transaction(#1522), Room→FFI refund-address swap (platform#4260), neutral-types sweep (#1522 items 10/14–16).🤖 Generated with Claude Code