Skip to content

feat(maya)!: swap deposits on the SDK deferred surface — dashj construction deleted - #1535

Open
HashEngineering wants to merge 14 commits into
feat/kotlin-sdk-phase1from
feat/maya-sdk-route
Open

feat(maya)!: swap deposits on the SDK deferred surface — dashj construction deleted#1535
HashEngineering wants to merge 14 commits into
feat/kotlin-sdk-phase1from
feat/maya-sdk-route

Conversation

@HashEngineering

@HashEngineering HashEngineering commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Maya/SwapKit swap deposits were the last L1 send still built by dashj. This moves them onto the Kotlin SDK's deferred build/broadcast surface and deletes the dashj leg, the same replace-then-delete treatment BIP70 got in #1531 — items 4 and 6 of #1520.

A MAYACHAIN UTXO deposit is not an ordinary send: the Asgard vault output must be VOUT0, the swap memo must be a zero-value OP_RETURN at VOUT1, change must go back to VIN0's address (MAYAChain identifies the depositor by the first input and pays refunds there), and no BIP-69 reordering may touch any of it. The SDK builder had no way to express that shape, which is why this path had to stay on dashj until now.

⚠️ Build dependencies — this branch does NOT build on a clean checkout

The committed pin is org.dashj:dash-sdk-android:0.1.0-v41int18-SNAPSHOT — byte-identical to what the base branch feat/kotlin-sdk-phase1 already uses, so this PR does not change the build for anyone reviewing it. That published AAR does not carry the drain surface, so the Maya deposit code will not compile against it until rows 4 and 5 below land and an official intN AAR includes them.

Testing was done against a locally built 0.1.0-v41int18-maya8-SNAPSHOT, which exists only in the author's local Maven repository. Nobody else can build this branch until rows 4 and 5 land and an official intN AAR carries them — that is a statement about this PR's readiness to be built by a reviewer, not a request to take it on trust.

The integration line has moved on. It is now at v41int21, which also lacks the drain surface, so the fold target is no longer int18 — it is int21 + #922 + #928 + #4324. Re-cutting the local AAR against int21 and reconstructing the Maya additions on top of the newer SdkL1SendService.kt is review item 2, which is deliberately not part of this round: int21 also carries the pooled ALL_SPENDABLE funding default, and folding onto it changes the drain's sweep scope, so it needs its own MAX run against the network rather than a mechanical conflict resolution. Treat the numbers in this description as belonging to the int18 fold.

What that local AAR is built from, so it can be reproduced:

This branch is rebased onto feat/kotlin-sdk-phase1 @ 8476ae798.

Required, in dependency order:

# PR What it provides State
1 rust-dashcore#922 key-wallet: add_op_return, preserve_output_order, change_to_first_input, OP_RETURN-aware fee sizing merged
2 platform#4286 FFI + Swift bindings for those controls merged
3 platform#4288 JNI + Kotlin bindings — the Android half merged
4 rust-dashcore#928 key-wallet: a drain may carry a zero-value OP_RETURN, so a memo can ride along with SelectionStrategy::All open — approved, CI green
5 platform#4324 selectionStrategy on buildSignedPayment and SignedCoreTransaction.deliverableAmountDuffs — how a host asks for a drain and reads what it pays open — blocked on the v4.2-dev key-wallet pin, which predates #928

Rows 4 and 5 are what the newest commit here depends on; without them a drain-with-memo is rejected by the engine and the deliverable amount cannot be read back.

What was done?

MayaBlockchainApiImpl rewritten on the SDK (buildAndSendSwapTx, used by both the direct Maya backend and SwapKit's Maya-protocol legs):

  1. Build — new SdkL1SendService.buildDeferredMayaDeposit wraps the SDK's atomic select + reserve + sign with the three new builder controls, returning signed bytes with the inputs reserved and nothing broadcast. Memo size is checked before anything is reserved (an asset contract address plus a destination address can exceed the 80-byte OP_RETURN limit).
  2. Verify before broadcastingverifyMayaDepositShape asserts the full deposit shape from the signed bytes: vault at VOUT0 for the exact amount, a zero-value OP_RETURN at VOUT1 whose script is byte-for-byte the expected push of the memo, at most one further output, and when present that change is P2PKH paying VIN0's own address. A mis-shaped deposit to a vault strands funds, so this replaces the old post-completeTx output count check with something considerably stricter. Decoding uses the SDK's own consensus decoder; a decode failure counts as a failed check, never a broadcastable pass.
  3. Broadcast with the BIP70 failure discipline — a provably-pre-network refusal releases the reservation (recoverable, funds untouched); an ambiguous outcome keeps the reservation and reports non-retryable, because releasing would let a rebuilt retry select different inputs and pay the vault twice. Cancellation is never converted into a failure result for the same reason.
  4. Max sells retry the build once with a 10 000-duff fee reserve carved out when the engine reports the balance cannot cover the mining fee — a pre-broadcast failure by construction, so the retry is safe.

Deleted (replace-then-delete): all dashj transaction construction on this path — manual SendRequest, the OP_RETURN script building, the output clear-and-re-add dance, re-signing, the fresh-Transaction confidence workaround — plus the now-orphaned manual-tx surface it was the last consumer of (WalletSendPaymentService.completeTransaction / signTransaction / sendTransaction) and the unused IncorrectSwapOutputCount. MayaBlockchainApiImpl itself has zero dashj imports: the only dashj left on the flow is the transition-only ReservationLockMirror (extracted here, dies with Phase 2) which locks the reserved outpoints so the background CoinJoin mixer cannot double-select them.

Two field-test bugs fixed after the first live swap:

  • The confirmation screen burned its full 10-second lock timeout. Two compounding causes: the ViewModel watched a change-only stream, but on the SDK route the InstantSend lock typically lands before the send call returns, so the subscription missed it; and WalletDataAdapter.waitUntilLocked preferred the held dashj wallet whenever the tx existed there — true for every bridged SDK send, whose dashj confidence is frozen because no peergroup ever delivers it a lock. The seam path (live engine lock state, race-free current-state replay) now runs first. CrowdNode's top-up wait, the only other caller, had the same exposure and is fixed with it.
  • Home-screen swap rows stayed stale until a long-press-History cache wipe. The update chain was intact — the swap-orders write re-emitted metadata and the display cache rebuilt the row with the fresh title — but the SDK-stamped shape freeze then discarded it, since that guard exists precisely to stop dashj rebuilds from rewriting SDK-authored rows. Swap decoration is the exception it must admit: it is metadata-authoritative (the swap-orders table whose change triggered the rebuild), not a dashj recomputation. Both merge sites now pass icon/title/status through when the rebuild carries swap metadata, while value, exchange rate, contact identity and the filter bucket stay frozen. A rebuild without swap metadata still never undresses a swap row.

SwapKit needed no separate work: its Maya-protocol legs delegate to the same rewritten buildAndSendSwapTx, and its NEAR-Intents legs already ride the SDK-routed neutral send on this branch.

How Has This Been Tested?

Mainnet, end to end, real funds — 2026-08-05, Pixel 9 emulator, prodDebug, SwapKit Maya route, tx 59f7d755…d8ce: 0.05318952 DASH → 3.5205806 RUNE, completed. SDK deferred build reserved the inputs and priced the deposit at a 343-duff fee (the engine's OP_RETURN-aware sizing), shape verification passed, broadcast, engine InstantSend lock 1.6 s later, mined in block 2516852, display bridge committed the row, and SwapKit tracking ran PENDING → COMPLETED with the outbound RUNE tx 041fb277…. Both fixes above were verified on a follow-up swap.

Mainnet MAX sell, real funds — 2026-08-07, against the maya8 AAR, tx a5c99aec…c873. (Earlier revisions of this description called this run "testnet"; the transaction is on mainnet — the label was wrong, the run was real.) The drain measured a 7,442,734-duff deposit (423-duff fee, 72-byte memo) and the build delivered exactly that. It first exposed a bug in this PR's own gate: verifyMayaDepositShape compared VOUT0 against the quote (7,442,725) and aborted a sound deposit over a 9-duff difference. A MAX sell builds as a drain, so the engine sets the vault amount and the quote is only a floor — which is how both guards around the build already treat it (each aborts on <, neither on >). The shape check now verifies a max sell against deliverableAmountDuffs, the value Rust computes from the registered transaction; it stays an exact comparison, which also makes it a cross-check that the decoded host bytes agree with what the engine registered. Ordinary sells still verify against the quote.

Re-run after the fix and broadcast, so this is the commit path end to end — drain built, guard cleared, transaction on the network — not a build-and-release rehearsal. The on-chain transaction confirms the drain shape exactly:

inputs (2, fully consumed) 7,443,157 duffs
VOUT0 → vault 7,442,734 duffs
VOUT1 zero-value OP_RETURN (the memo)
fee 423 duffs
change noneVOUT0 + fee == inputs to the duff

No change output is the proof it was a real drain rather than a large ordinary send, and VOUT0 equals the deliverableAmountDuffs the engine reported at build time — the two values the shape check now compares. InstantSend-locked, confirmed in block 2517981.

Automated: new MayaDepositShapeTest (17 host cases over hand-built decoded-transaction fixtures — wrong vault address/amount, wrong or displaced memo, value-carrying OP_RETURN, foreign change, non-P2PKH change, the OP_PUSHDATA1 boundary at 76+ bytes, no inputs, and the max-sell expectation source — the failing transaction above rejected against the quote, accepted against the engine's amount, and still rejected when the bytes disagree with the engine by one duff); 3 new TxDisplayCacheMergeGuardTest cases for the swap-decoration exception. Wallet payments and SDK-service suites green (747 tests), compileStagingDebugKotlin and ktlint clean.

Breaking Changes

Max deposits are measured by draining, not by estimating

maxMayaDepositDuffs read the wallet-wide spendable balance, subtracted a probe-measured fee and a change-headroom constant, and quoted the result — but the deposit funds from BIP44 account 0 alone. The quote was therefore derived from a different pool than the build spends from, so it could name an amount the build could not fund, and it under-quoted by the headroom even when it worked.

A max deposit is a drain, so this now builds one and reads what the engine says it delivers: every BIP44 UTXO selected, the memo's bytes priced into the fee, no change, the vault output set to total inputs − fee. That is the same computation the real deposit performs, so quote and deposit cannot disagree.

  • SdkDeferredPayment carries deliverableDuffs — what the transaction actually pays. Supplied by the caller for an explicit build, computed by the engine for a drain.
  • buildDeferredMayaDeposit takes drain, passing SelectionStrategy.ALL.
  • A MAX sell now builds as a drain, and the built transaction is checked against the quote before any broadcast decision. The previous guard compared a re-measurement; this compares the signed transaction that would actually reach the vault, so nothing moving in between can defeat it.
  • Removed: MAYA_DEPOSIT_PROBE_RESERVE_DUFFS, MAYA_DEPOSIT_CHANGE_HEADROOM_DUFFS, the spendable − fee − headroom arithmetic, and the wallet-wide balance read.

The CrowdNode app-locked-output refusal and the funding-gate check are unchanged — a max deposit still will not sweep protected outputs.

Verified on-device. Two separate runs, which an earlier version of this description ran together and contradicted itself over:

  1. Measurement only (emulator) — a max Maya deposit measures 27,442,985 duffs deliverable with a measured 432-duff fee for an 80-byte memo. The build is signed and released without broadcasting, so the measuring path is exercised without a transaction reaching the network.
  2. The commit path, broadcast on mainnet with real funds — the 2026-08-07 MAX sell above, tx a5c99aec…c873. The drain measured 7,442,734 duffs (423-duff fee, 72-byte memo), the built transaction paid exactly that, and after the deliverableAmountDuffs fix the deposit was broadcast, InstantSend-locked and confirmed. The chain shows two inputs fully consumed and no change output, which is what makes it a drain and not a large ordinary send.

So the broadcasting MAX drain has been exercised against the real network, for real funds. The earlier "has not been run against the network yet" sentence described run 1 and was wrong about run 2; it is removed rather than reworded, since it was the source of the contradiction.

Checklist:

  • I have added tests to cover my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • All new and existing tests passed — unit suites pass locally; CI cannot build until the dependency chain above lands (see the build-dependencies section)

Summary by CodeRabbit

  • New Features

    • Added deferred MayaChain deposit support with transaction validation, memo handling, input reservations, and explicit broadcast outcomes.
    • Improved transaction-lock tracking across SDK and wallet transactions.
    • Updated swap transaction displays to reflect refreshed titles, icons, and statuses.
  • Bug Fixes

    • Improved lock detection reliability and handling of ambiguous broadcasts.
    • Preserved swap metadata during transaction display updates.
  • Tests

    • Added coverage for Maya deposit structure validation and swap display updates.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e0475212-d1a2-40af-ad53-462b7f82ac66

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR moves Maya deposit construction and broadcasting to the Kotlin SDK deferred-payment flow. It adds deposit validation, reservation mirroring, SDK lock waiting, updated swap display merging, and tests. Manual wallet transaction APIs and an obsolete Maya exception are removed.

Changes

Maya SDK cutover

Layer / File(s) Summary
SDK deferred deposit construction
wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt, wallet/test/de/schildbach/wallet/payments/MayaDepositShapeTest.kt
The SDK validates Maya deposit inputs and builds signed deferred payments with vault, OP_RETURN, output-order, change, and reservation rules.
Maya deferred build and broadcast flow
wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt, wallet/src/de/schildbach/wallet/service/platform/sdk/ReservationLockMirror.kt, build.gradle, integrations/maya/src/main/java/...
Maya processing validates signed deposit shapes, handles fee retries and broadcast outcomes, mirrors reservations, and updates related documentation and SDK versioning.
SDK lock-state integration
wallet/src/de/schildbach/wallet/data/WalletDataAdapter.kt, integrations/maya/src/main/java/org/dash/wallet/integrations/maya/ui/MayaConversionPreviewViewModel.kt
Lock waits now use current SDK transaction state before the dashj fallback. Maya conversion handling catches and logs lock-wait failures.
Manual payment API removal
wallet/src/de/schildbach/wallet/payments/WalletSendPaymentService.kt, wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt, wallet/src/de/schildbach/wallet/payments/FakeDashSpendService.kt, integrations/maya/src/main/java/org/dash/wallet/integrations/maya/model/MayaErrorResponse.kt
Manual transaction completion, signing, and sending methods were removed. The obsolete IncorrectSwapOutputCount exception was removed.
Swap display merge behavior
wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt, wallet/test/de/schildbach/wallet/service/TxDisplayCacheMergeGuardTest.kt
Swap decoration and status can refresh during SDK-stamped row merges while values and filter flags remain preserved. Tests cover the updated merge rules.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

Possibly related PRs

  • dashpay/dash-wallet#1531 — Both PRs modify SDK-based deferred transaction building, reservation handling, broadcasting, and release behavior.

Suggested reviewers: bfoss765

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.59% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: moving Maya swap deposits to the SDK deferred surface and removing dashj transaction construction.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/maya-sdk-route

Comment @coderabbitai help to get the list of available commands.

@HashEngineering

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
wallet/test/de/schildbach/wallet/payments/MayaDepositShapeTest.kt (1)

94-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for a VOUT0 without a decodable address.

verifyMayaDepositShape returns "VOUT0 is not a plain address output" when outputs[0].address is null. No test reaches that branch. The engine can produce a non-address VOUT0 if the builder contract changes, so cover it.

💚 Proposed test
+    `@Test`
+    fun nonAddressVault0Fails() {
+        val tx = DecodedTransaction(
+            ByteArray(32),
+            listOf(input()),
+            listOf(
+                DecodedTransaction.Output(null, vaultDuffs, p2pkhScript(1)),
+                memoOutput()
+            )
+        )
+        val error = verify(tx)
+        assertNotNull(error)
+        assertTrue(error!!.contains("not a plain address output"))
+    }
+
🤖 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/MayaDepositShapeTest.kt` around
lines 94 - 107, Add a test in MayaDepositShapeTest alongside
wrongVaultAmountFails and wrongVaultAddressFails that constructs a deposit
transaction whose VOUT0 has no decodable address, invokes
verifyMayaDepositShape, and asserts the returned error is non-null and contains
"VOUT0 is not a plain address output".
integrations/maya/src/main/java/org/dash/wallet/integrations/maya/ui/MayaConversionPreviewViewModel.kt (1)

135-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Rethrow CancellationException before the general catch.

kotlinx.coroutines.CancellationException extends java.util.concurrent.CancellationException, which extends IllegalStateException. So catch (e: Exception) also swallows a cancellation of viewModelScope that arrives while the coroutine suspends in waitUntilLocked.

When that happens, the handler logs a warning, sets locked = false, and execution continues into the rest of the success branch. _showLoading.value = false and commitSwapTradeSuccessState.value = ... are non-suspending, so they still run on a screen that is already going away. The cancellation only resurfaces at the next suspension point, which is swapOrderDao.insertOrder.

MayaBlockchainApiImpl.buildAndSendSwapTx uses the explicit rethrow pattern for the same reason (see wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt Lines 356-361). Apply it here so a cancelled screen unwinds instead of continuing.

♻️ Proposed refactor
                     val locked = try {
                         withTimeoutOrNull(IS_LOCK_TIMEOUT_MS) {
                             walletDataProvider.waitUntilLocked(txId)
                         } != null
+                    } catch (e: CancellationException) {
+                        // CancellationException is an IllegalStateException, so the
+                        // general catch below would swallow an outer-scope cancel and
+                        // let the rest of the success branch run on a dead screen.
+                        throw e
                     } catch (e: Exception) {
                         log.warn("could not watch maya swap tx {} for a lock", txId, e)
                         false
                     }

Add the import:

+import kotlinx.coroutines.CancellationException

Note that withTimeoutOrNull returns null on timeout instead of throwing, so this rethrow does not defeat the timeout fallback.

🤖 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
`@integrations/maya/src/main/java/org/dash/wallet/integrations/maya/ui/MayaConversionPreviewViewModel.kt`
around lines 135 - 142, Update the try/catch around waitUntilLocked in the Maya
conversion preview flow to rethrow kotlinx.coroutines.CancellationException
before the general Exception handler, following the existing pattern in
buildAndSendSwapTx. Preserve the current warning log and false fallback for
non-cancellation errors, while allowing coroutine cancellation to unwind
immediately.
wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt (1)

378-387: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Prefer the typed SDK error over message matching.

Use DashSdkError.PlatformWallet.CoreInsufficientFunds from org.dashfoundation.dashsdk.errors before checking the "insufficient funds" message across causes. The predicate controls both the maximum-sell retry and the InsufficientFundsException mapping.

🤖 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/MayaBlockchainApiImpl.kt` around
lines 378 - 387, Update isInsufficientFunds to first detect
DashSdkError.PlatformWallet.CoreInsufficientFunds from
org.dashfoundation.dashsdk.errors while traversing the throwable cause chain,
then retain the case-insensitive “insufficient funds” message fallback across
causes. Preserve the existing predicate behavior for maximum-sell retries and
InsufficientFundsException mapping.
build.gradle (1)

11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the Maya SDK to an immutable version for release builds. dashSdkVersion uses a mutable SNAPSHOT, while SdkL1SendService.buildDeferredMayaDeposit depends on SDK-specific overload parameters and output-order behavior. Use an immutable artifact that contains this API, and retain a test for the VOUT0/VOUT1 contract.

🤖 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 `@build.gradle` at line 11, Pin dashSdkVersion in build.gradle to an immutable
released artifact containing the overloads required by
SdkL1SendService.buildDeferredMayaDeposit, replacing the mutable SNAPSHOT
version. Retain or add coverage for the VOUT0/VOUT1 output-order contract in
wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt; no
direct production-code change is required there unless needed to preserve that
behavior.
🤖 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/payments/MayaBlockchainApiImpl.kt`:
- Around line 308-323: In the SdkWriteResult.Broadcast branch, clear the
mirrored reservation locks after the successful broadcast by calling
reservationLockMirror.setLocks for the payment with locked=false. Place this
alongside the existing success handling, independently of
bridgedTransactionFactory.bridge so locks are released even when the non-fatal
display bridge returns NotBridged; leave the NotBroadcast and Ambiguous
lifecycle behavior unchanged.

---

Nitpick comments:
In `@build.gradle`:
- Line 11: Pin dashSdkVersion in build.gradle to an immutable released artifact
containing the overloads required by SdkL1SendService.buildDeferredMayaDeposit,
replacing the mutable SNAPSHOT version. Retain or add coverage for the
VOUT0/VOUT1 output-order contract in
wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt; no
direct production-code change is required there unless needed to preserve that
behavior.

In
`@integrations/maya/src/main/java/org/dash/wallet/integrations/maya/ui/MayaConversionPreviewViewModel.kt`:
- Around line 135-142: Update the try/catch around waitUntilLocked in the Maya
conversion preview flow to rethrow kotlinx.coroutines.CancellationException
before the general Exception handler, following the existing pattern in
buildAndSendSwapTx. Preserve the current warning log and false fallback for
non-cancellation errors, while allowing coroutine cancellation to unwind
immediately.

In `@wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt`:
- Around line 378-387: Update isInsufficientFunds to first detect
DashSdkError.PlatformWallet.CoreInsufficientFunds from
org.dashfoundation.dashsdk.errors while traversing the throwable cause chain,
then retain the case-insensitive “insufficient funds” message fallback across
causes. Preserve the existing predicate behavior for maximum-sell retries and
InsufficientFundsException mapping.

In `@wallet/test/de/schildbach/wallet/payments/MayaDepositShapeTest.kt`:
- Around line 94-107: Add a test in MayaDepositShapeTest alongside
wrongVaultAmountFails and wrongVaultAddressFails that constructs a deposit
transaction whose VOUT0 has no decodable address, invokes
verifyMayaDepositShape, and asserts the returned error is non-null and contains
"VOUT0 is not a plain address output".
🪄 Autofix

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: d77315a1-547b-4dc4-a840-2e164b351eaa

📥 Commits

Reviewing files that changed from the base of the PR and between c374245 and f84e705.

📒 Files selected for processing (15)
  • build.gradle
  • integrations/maya/src/main/java/org/dash/wallet/integrations/maya/api/MayaBlockchainApi.kt
  • integrations/maya/src/main/java/org/dash/wallet/integrations/maya/di/MayaModule.kt
  • integrations/maya/src/main/java/org/dash/wallet/integrations/maya/model/MayaErrorResponse.kt
  • integrations/maya/src/main/java/org/dash/wallet/integrations/maya/ui/MayaConversionPreviewViewModel.kt
  • wallet/src/de/schildbach/wallet/data/WalletDataAdapter.kt
  • wallet/src/de/schildbach/wallet/payments/FakeDashSpendService.kt
  • wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt
  • wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
  • wallet/src/de/schildbach/wallet/payments/WalletSendPaymentService.kt
  • wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt
  • wallet/src/de/schildbach/wallet/service/platform/sdk/ReservationLockMirror.kt
  • wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt
  • wallet/test/de/schildbach/wallet/payments/MayaDepositShapeTest.kt
  • wallet/test/de/schildbach/wallet/service/TxDisplayCacheMergeGuardTest.kt
💤 Files with no reviewable changes (4)
  • wallet/src/de/schildbach/wallet/payments/WalletSendPaymentService.kt
  • wallet/src/de/schildbach/wallet/payments/FakeDashSpendService.kt
  • integrations/maya/src/main/java/org/dash/wallet/integrations/maya/model/MayaErrorResponse.kt
  • wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt

Comment thread wallet/src/de/schildbach/wallet/payments/MayaBlockchainApiImpl.kt
@HashEngineering

Copy link
Copy Markdown
Collaborator Author

not ready, needs to handle the MAX button

@HashEngineering

Copy link
Copy Markdown
Collaborator Author

Follow-up pushed (4a17d29) after an audit of the MAX button, which the mainnet field test did not exercise.

The bug: the max-sell fee reserve could come in under the real fee, making the deposit pay the vault less than quoted. NEAR Intents refuses under-delivery (the deposit sits ~1h, then refunds minus 0.001 DASH), and Maya would execute a swap for an amount the user never agreed to.

Three problems with one root cause — nobody had revisited the max path for the cutover:

  • SwapKit sized the sweep with dashj (estimateNetworkFeecompleteTx on the held wallet). Post-cutover, incoming SDK transactions never reach that wallet, so its coin set is frozen at cutover time: the estimate either throws — meaning any wallet funded since the cutover simply cannot get a max quote — or prices the wrong transaction shape (no OP_RETURN output).
  • The direct Maya route had no reserve at all. It quoted the full balance and relied on a silent adjust-down retry at build time, which is precisely the under-delivery described above.
  • The retry masked the problem rather than reporting it.

The fix: SdkL1SendService.maxMayaDepositDuffs measures the fee through the real engine — it builds a throwaway deposit (same builder, same three options, the wallet's own address as a size stand-in), reads the fee off the reservation and releases it. The measurement is biased high by construction: worst-case 80-byte memo, a change output kept in the probe so the measured size matches the real deposit's three-output shape, and 1 000 duffs of headroom to keep that change clear of dust. Exposed neutrally as MayaBlockchainApi.maxSwapDepositAmount.

Both routes now quote exactly that figure and deposit exactly what they quoted. The silent retry is gone — a balance drop between quote and build aborts with a re-quote error. Re-measuring at build time uses the real memo, which can only raise the ceiling set by the worst-case quote, so the guard cannot fire spuriously.

Side effect worth noting: the maya module no longer calls estimateNetworkFee anywhere, so the last dashj on the swap path is gone.

7 new SdkL1SendServiceTest cases. Still unverified on-device: a real MAX swap (the field test was a partial-balance sell).

@HashEngineering

Copy link
Copy Markdown
Collaborator Author

One more max-sell fix (20513dd), found by asking whether the max path actually drains the wallet.

It does not — by design (see below) — but it does spend all but 1 000 duffs, which means coin selection reaches every UTXO in the wallet. spendableBalanceDuffs deliberately includes app-locked outputs (the engine has no lock concept and the FFI exposes no exclusion API), which is precisely why the send-all drain refuses to run while any lock exists. The Maya max path bypassed that guard and would have swept CrowdNode-protected funds into a swap.

Fixed by extracting the drain's fail-closed preflight — dashj wallet locks OR seam-registered locks on SDK-only txs, blocking on a check failure too — into hasProtectedOutputs, and applying it in maxMayaDepositDuffs: refuse to quote rather than build a deposit that spends protected funds. A partial (non-max) deposit is unchanged; it carries the same exposure as any ordinary send. 3 new tests.

For the record, what a MAX sell actually leaves behind:

  • 1 000 duffs (0.00001 DASH) of change headroom, plus any rounding slack between the measured and actual fee;
  • possibly a small UTXO that coin selection can skip while still covering the target;
  • anything immature (coinbase/masternode rewards), which is excluded from the spendable figure — correctly.

It is not a true send-all (SelectionStrategy::All) and cannot be: the engine has no send-all-with-OP_RETURN, since rust-dashcore#922 does not combine the two. An explicit amount plus a measured fee is the closest safe approximation, and the leftover is what makes the reserve provably sufficient rather than merely probable. A true exact drain would need send-all + OP_RETURN support upstream — worth filing against the engine if we want the last 1 000 duffs.

@HashEngineering

Copy link
Copy Markdown
Collaborator Author

@coderabbitai

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@HashEngineering I will review the MAX-sell follow-up changes, including fee measurement, quote-to-deposit consistency, and protected-output handling.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

HashEngineering and others added 7 commits August 7, 2026 09:16
…uction deleted

MayaBlockchainApiImpl now builds the MAYACHAIN deposit with the Kotlin
SDK's deferred build/broadcast primitive (buildDeferredMayaDeposit:
vault VOUT0, OP_RETURN memo VOUT1, change back to VIN0's address VOUT2,
no BIP-69 reordering — the new builder controls from platform#4286/#4288,
engine work in rust-dashcore#922), verifies the deposit shape from the
signed bytes BEFORE broadcasting (verifyMayaDepositShape — a mis-shaped
vault deposit strands funds), mirrors the reservation into wallet locks
for the transition window, and bridges the broadcast tx for display.
The SwapKit Maya-protocol legs converge automatically — they delegate
to the same buildAndSendSwapTx; the NEAR-Intents legs already ride the
neutral SDK-routed send.

Replace-then-delete, as with BIP70: the dashj leg is gone — manual
SendRequest/OP_RETURN script construction, output clearing/re-signing,
the fresh-Transaction confidence workaround, the post-completeTx output
checks, and with it the now-orphaned manual-tx surface
(WalletSendPaymentService.completeTransaction/signTransaction/
sendTransaction and IncorrectSwapOutputCount) whose only consumer was
this path.

Failure semantics: build/verify failures release the reservation
(recoverable, nothing moved); a provably pre-network broadcast refusal
releases; an AMBIGUOUS broadcast outcome keeps the reservation and
reports non-retryable — releasing would let a rebuilt retry pay the
vault twice (the BIP70 field-test lesson). Max sells retry the build
once with a 10k-duff fee reserve carved out on an engine-reported
shortfall (pre-broadcast by construction).

Pins dash-sdk-android 0.1.0-v41int13-maya2-SNAPSHOT (qa5-plus-maya +
the buildSignedPayment Maya options). Unit tests: 10-case
MayaDepositShapeTest; payments + sdk-service suites 747/747 green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Shape verification runs on the SDK's own consensus decoder
  (TransactionDecoder) instead of bitcoinj parsing; the memo check is
  now byte-for-byte against the expected OP_RETURN script, and the
  change-to-VIN0 check uses the decoder's recovered sender address.
  A decode failure counts as a failed shape check (released,
  recoverable), never a broadcastable pass.
- The transition-only lockOutput reservation mirror moves to its own
  clearly-marked helper (ReservationLockMirror) so the last dashj on
  this flow is quarantined in one Phase-2-deletable class.
- Duffs conversion is pure decimal arithmetic (no Coin types).
- MayaDepositShapeTest rebuilt on hand-built DecodedTransaction
  fixtures — 13 host cases, no dashj, no native library.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Field-tested on mainnet 2026-08-05 (tx 59f7d755…d8ce): the engine had the
InstantSend lock 1.6s after broadcast, but the confirmation screen still
waited out its whole 10s timeout. Two compounding causes:

- The ViewModel watched observeTransactionLocked, a change-only stream —
  on the SDK route the lock usually lands BEFORE the send call returns,
  so the subscription missed the event and nothing ever re-fired. It now
  calls waitUntilLocked, which returns immediately for an already-locked
  tx (same timeout as the outer bound).
- WalletDataAdapter.waitUntilLocked preferred the held dashj wallet
  whenever the tx existed there — true for every bridged SDK send — and
  a bridged copy's confidence is frozen (no peergroup ever delivers it
  an IS-lock), so that wait could never complete post-cutover. The seam
  path (live engine lock state, race-free current-state replay) now runs
  first; the dashj-confidence path remains for pre-cutover and for txs
  the SDK store never learned.

Also benefits CrowdNode's top-up lock wait, the other waitUntilLocked
caller, which had the same frozen-confidence exposure for bridged sends.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 2026-08-05 mainnet field test left the swap row stale until the
long-tap History rebuild. The chain was intact up to the last step: the
swap-orders write re-emitted presentable metadata, the display cache
computed the changed row and rebuilt it with the fresh swap decoration —
and then the SDK-stamped shape freeze threw the new title/icon/status
away (both the inline metadata-path merge and
mergeDisplayEntryPreservingSdkStamped keep the CACHED shape for
SDK-authoritative rows, because a dashj rebuild cannot be trusted to
re-derive value or direction).

The swap decoration is the exception the freeze must admit: it is
metadata-authoritative — the very swap-orders table whose change
triggered the rebuild — not a dashj recomputation. Both merge sites now
pass icon/title/status through when the rebuild carries swap metadata
(entry.swapStatus != null), while value, exchange rate, contact identity
and the filter bucket stay frozen. A rebuild WITHOUT swap metadata still
never undresses an existing swap row.

Covered by three new TxDisplayCacheMergeGuardTest cases (decoration
passes the freeze; PENDING→COMPLETED retitles; a metadata-less rebuild
keeps the swap shape); service suite green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dashj

The max-sell fee reserve could come in under the real fee, which makes the
deposit pay the vault LESS than quoted. That is never acceptable: NEAR
Intents refuses under-delivery (the deposit sits ~1h, then refunds minus
0.001 DASH) and Maya would execute a swap for an amount the user never
agreed to.

Three problems, one cause — nobody had revisited the max path for the
cutover:

- SwapKit sized the sweep with dashj (estimateNetworkFee → completeTx on
  the HELD wallet). Post-cutover incoming SDK transactions never reach
  that wallet, so its coin set is frozen at cutover time: the estimate
  either throws (any funds received since the cutover are invisible to it,
  so a max quote fails outright) or prices the wrong shape (no OP_RETURN).
- The direct Maya route had no reserve at all — it quoted the full balance
  and relied on a silent adjust-down retry at build time, i.e. exactly the
  under-delivery this commit refuses.
- The retry itself masked the problem instead of reporting it.

Replaced with a MEASURED figure: SdkL1SendService.maxMayaDepositDuffs
builds a throwaway deposit through the real engine (same builder, same
three options, the wallet's own address as a size stand-in), reads the fee
off the reservation and releases it. Biased HIGH by construction — a
worst-case 80-byte memo, a change output kept in the probe so the measured
size matches the real one, and 1 000 duffs of headroom to keep that change
clear of dust — so the reserve can never fall short. Exposed neutrally as
MayaBlockchainApi.maxSwapDepositAmount.

Both routes now quote exactly that figure and deposit exactly what they
quoted. The silent retry is gone: if the balance drops between quote and
build, the deposit aborts with a re-quote error. Re-measuring at build time
uses the REAL memo, which can only raise the ceiling set by the worst-case
quote, so the guard cannot fire spuriously.

The maya module no longer calls estimateNetworkFee anywhere — the last
dashj on the swap path is gone.

7 new SdkL1SendServiceTest cases; payments + service suites and ktlint green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A max deposit spends all but the change headroom, so coin selection
reaches every UTXO — including app-locked ones. spendableBalanceDuffs
deliberately INCLUDES app-locked outputs (the engine has no lock concept
and the FFI exposes no exclusion API), which is exactly why the send-all
drain refuses to run while any lock exists. The Maya max path bypassed
that guard and would have swept CrowdNode-protected funds into a swap.

Extracted the drain's fail-closed preflight (dashj wallet locks OR
seam-registered locks on SDK-only txs, blocking on a check failure too)
into hasProtectedOutputs, and applied it in maxMayaDepositDuffs: refuse
to quote rather than build a deposit that spends protected funds. A
partial (non-max) deposit is unchanged — it carries the same exposure as
any ordinary send. commitSwapTransaction contains the refusal as a
recoverable failure instead of letting it escape the caller's scope.

3 new tests (dashj lock, seam lock, check-throws); nothing is built or
measured in any of those cases. Suites and ktlint green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
maxMayaDepositDuffs read the WALLET-WIDE spendable balance, subtracted a
probe-measured fee and a change-headroom constant, and quoted the result —
but the deposit funds from BIP44 account 0 alone. Any wallet holding
DIP-15 contact-received or CoinJoin funds therefore quoted against money
the build could not reach: the probe failed with CoreInsufficientFunds and
MAX died. It failed safely, but it failed.

A max deposit IS a drain, so build one and read what the engine says it
delivers: every BIP44 UTXO selected, this memo's bytes priced into the fee,
no change, the vault output set to (total inputs - fee). That is the same
computation the real deposit performs, so quote and deposit cannot
disagree — which subtracting a guess from the wrong pool could not promise.

- SdkDeferredPayment carries `deliverableDuffs`: what the transaction
  actually pays. Supplied by the caller for an explicit build, computed by
  the engine for a drain.
- buildDeferredMayaDeposit takes `drain`, passing SelectionStrategy.ALL.
  A drain supplies no amount, so 0 is passed and the engine sets the
  output (requires the maya7 AAR, whose JNI accepts a zero output).
- maxMayaDepositDuffs builds a drain to an own address, reads its
  deliverable, and releases the reservation. Gone: the probe reserve, the
  change headroom, the spendable-minus-fee arithmetic. A drain the engine
  will not fund means "nothing depositable" and returns 0.
- A MAX sell now BUILDS as a drain, and the built transaction is checked
  against the quote before any broadcast decision. The pre-build guard
  compared a re-measurement; this compares the signed transaction that
  would actually reach the vault, so nothing moving in between can defeat
  it. Under-delivery is refused: Maya would execute a swap the user never
  agreed to, and NEAR Intents rejects it outright.

The CrowdNode app-locked-output refusal and the funding-gate check are
unchanged — a max deposit still will not sweep protected outputs.

Pin moved to the maya7 AAR.

Tests: five rewritten to the drain contract (the engine-computed amount,
the drain-shaped probe and its release, worst-case and explicit memo
sizing, and an unfundable drain reporting 0), plus a fake that throws to
model the engine's refusal. Wallet unit suite shows no regression against
the same baseline; the pre-existing failures are an unrelated leaked dashj
Context between test classes.

Verified on-device (testnet, emulator): drain-measured max deposit
27442985 duffs, real fee 432, 80-byte memo.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@HashEngineering

Copy link
Copy Markdown
Collaborator Author

will be rebasing against the latest phase1 branch tip

…quote

A max sell aborted pre-broadcast with "VOUT0 carries 7442734 duffs,
expected 7442725" and released a perfectly good deposit.

A MAX sell builds as a DRAIN, so the ENGINE sets the vault output to
(total inputs - fee); the app never supplies that number and the quote is
only a FLOOR. Both guards around the build already treat it that way --
each aborts on `<` and neither on `>` -- but verifyMayaDepositShape
demanded exact equality with the quote, so a drain delivering MORE than
quoted was rejected as mis-shaped. Nine duffs, and the ordinary result of
the balance moving between quote and build.

Verify a max sell against payment.deliverableDuffs instead: the value
Rust computes from the REGISTERED transaction. That keeps the check exact
rather than loosening it to a range, and turns it into a cross-check --
the decoded host bytes must agree with what the engine registered, which
is the disagreement the gate exists to catch. Ordinary sells are
unchanged: the app chose the amount, so the quote is the expectation.

Four tests, including the failing transaction end to end: rejected
against the quote, accepted against the engine's amount, and still
rejected when the bytes disagree with the engine by one duff.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@bfoss765 bfoss765 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design here is the right shape for the port — the reservation lifecycle (build/verify failure and provable pre-network refusal → release; ambiguous → keep + non-retryable), the fail-closed byte-exact shape verifier, and the drain-measured MAX with the engine-deliverable cross-check are all genuinely strong fund-safety work. Requesting changes on a small set of items, most of which your own description already declares:

Blocking

  1. SdkL1SendService.kt (companion object): MAYA_DEPOSIT_PROBE_RESERVE_DUFFS and MAYA_DEPOSIT_CHANGE_HEADROOM_DUFFS are added but never referenced by code (only KDoc), and the PR description says they were removed. The KDocs of maxMayaDepositDuffs and MayaBlockchainApi.maxSwapDepositAmount also still describe the retired spendable − measured fee − headroom model while the implementation is drain-measured with no headroom. Please delete the dead constants and rewrite both KDocs to the drain model — as written, a future reader will trust the doc and re-introduce the headroom arithmetic the drain rework deliberately killed.

  2. Coordination: this was cut against the pre-pooling SdkL1SendService.kt. The integration line has since moved to the pooled ALL_SPENDABLE funding default (v41int19+ AARs; platform #944/#4350 are now merged upstream), and there are further in-flight changes to this file on the branch tip — a merge simulation shows content conflicts in both SdkL1SendService.kt and SdkL1SendServiceTest.kt. Two consequences: (a) buildDeferredMayaDeposit and the MAX drain will select across BIP44 + BIP32 + DashPay receival accounts under the pooled default, not "every BIP44 UTXO" as the comments/tests state — change/refund provenance can be a receival-account address (still wallet-owned, funds-safe, but the docs/tests are stale); (b) funding shortfalls become the typed CoreInsufficientFunds (FFI 22) rather than the string shapes. Suggest reconstructing the Maya additions on top of the pooled version rather than resolving hunks mechanically, and re-running the testnet MAX sell under the pooled default — the drain's sweep scope is materially different.

  3. Dependency chain (declared): rust-dashcore#928 and platform#4324 are still open and the maya8 AAR is local-only, so nobody else can build this. Flagging that the integration pin has moved to v41int21, which also lacks the drain surface — the fold target is now "int21 + #922 + #928 + #4324".

Non-blocking

  1. buildDeferredMayaDeposit with drain = true doesn't run hasProtectedOutputs() — the only guard against sweeping CrowdNode-locked outputs is the caller having called maxMayaDepositDuffs first. MayaBlockchainApiImpl does that today, but the guard is a call-site convention. Suggest moving the check into the primitive itself under drain = true, same as the send-all drain — then no future caller can sweep protected funds by skipping the measurement step.

  2. MayaBlockchainApiImpl, SdkWriteResult.Broadcast arm — on the CodeRabbit thread asking for the mirror locks to be cleared after successful broadcast: I'd argue keeping them is the safe behavior and clearing would be wrong. Post-cutover the held dashj wallet never learns the deposit spent those outpoints; if the display bridge returns NotBridged, the stale lock is the only thing preventing the mixer double-selecting an already-spent coin. Worth a short code comment in the Broadcast arm saying the lock is intentionally left in place, so the next reader doesn't "fix" it.

  3. Testing section: "Testnet MAX sell — 2026-08-07 … Re-tested and successful" vs "The commit path — building the drain and broadcasting it after the guard — has not been run against the network yet." These read as contradictory — please clarify which statement covers the broadcasting MAX drain. If a MAX drain has never actually hit the network, that should gate the merge given it sweeps the wallet into a vault.

  4. Nit: MayaConversionPreviewViewModel's withTimeoutOrNull(...) { waitUntilLocked(txId) } != null works because Unit != null, but reads oddly. Fine to leave.

…red-model docs

Review items 1, 4, 5 and 7 from #1535.

The max-deposit guard was a call-site convention: buildDeferredMayaDeposit
would happily drain the wallet, and the only thing standing between a MAX
deposit and CrowdNode-locked funds was the caller having measured first.
MayaBlockchainApiImpl does measure, but a convention is one refactor away
from being skipped and the money does not come back. Move the fail-closed
check into the primitive under drain = true, where no caller can miss it.
maxMayaDepositDuffs keeps its own copy deliberately -- its probe runs
inside a catch-all that turns any failure into a quote of 0, which would
otherwise swallow the refusal and report "your maximum is 0" instead of
"you hold locked funds". Three tests cover the gap: a direct drain build
is refused for dashj locks and for seam-registered locks with nothing
reserved, and a partial deposit is still allowed through.

The docs described a model the drain rework deleted. Two constants
(MAYA_DEPOSIT_PROBE_RESERVE_DUFFS, MAYA_DEPOSIT_CHANGE_HEADROOM_DUFFS)
survived only in KDoc references, and three doc blocks still explained the
max deposit as "spendable - measured fee - headroom". Left alone, the next
reader would trust them and reintroduce the headroom arithmetic that the
drain measurement exists to remove. Delete the constants and rewrite
maxMayaDepositDuffs, MayaBlockchainApi.maxSwapDepositAmount and the
MayaBlockchainApiImpl class doc to the drain model, including why a
headroom must not come back.

Also: say in the Broadcast arm that the mirrored reservation locks are
left in place on purpose -- post-cutover the held dashj wallet never
learns the deposit spent those outpoints, so on a NotBridged result the
stale lock is the only thing keeping the mixer off an already-spent coin.
And make the IS-lock timeout read as what it is rather than relying on
Unit != null.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@HashEngineering

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Items 1, 3, 4, 5, 6 and 7 are addressed in 681190f7f; item 2 is deliberately not, and the rest of this comment explains why and what I found while scoping it.

Item 6 first — the testing claim was wrong, but in the other direction

You were right that the two statements contradicted each other, and right to say it should gate the merge. Chasing it down turned up a bigger error than the contradiction.

They describe two different runs, which is why the numbers never lined up: one was measurement-only (27,442,985 duffs, 432-duff fee, 80-byte memo), the other the 2026-08-07 MAX sell (7,442,734 duffs, 423-duff fee, 72-byte memo). The MAX sell was broadcast — and it was mainnet with real funds, not testnet. Every "testnet" label on this work was wrong, in this description and in platform#4324. Both are corrected.

The transaction is a5c99aec…c873:

inputs (2, fully consumed) 7,443,157 duffs
VOUT0 → vault 7,442,734 duffs
VOUT1 zero-value OP_RETURN
fee 423 duffs
change noneVOUT0 + fee == inputs to the duff

InstantSend-locked, confirmed in block 2517981. The absent change output is the part that matters: it proves this was a real drain and not a large ordinary send, and VOUT0 equals the deliverableAmountDuffs the engine reported at build time — the two values the shape check now compares.

So the commit path has been exercised end to end on the real network. The description no longer claims otherwise.

Item 4 — guard moved, with one wrinkle worth naming

hasProtectedOutputs() now runs inside buildDeferredMayaDeposit under drain = true, before anything is reserved, so no caller can sweep protected funds by skipping the measurement step.

maxMayaDepositDuffs keeps its own copy on purpose. Its probe runs inside a catch-all that converts any failure into a quote of 0, so relying on the primitive alone would have turned "refuse, you hold locked funds" into a silent "your maximum is 0". Both call sites now carry a comment pointing at the other so neither gets deleted as redundant.

Three tests cover the gap you identified: a direct drain build is refused for dashj locks and for seam-registered locks with nothing reserved, and a partial deposit still passes through unguarded.

Item 1 — three stale KDocs, not two

You named maxMayaDepositDuffs and MayaBlockchainApi.maxSwapDepositAmount. The MayaBlockchainApiImpl class doc had the same "balance − a MEASURED fee − change headroom" model, plus two inline comments and two test comments. All rewritten to the drain model, and both dead constants are gone. The rewritten docs say explicitly not to reintroduce a headroom or reserve constant, with the reason, since that is exactly the re-introduction you predicted.

Items 5 and 7

The Broadcast arm now says the mirrored locks are left in place deliberately, with your reasoning: post-cutover the held dashj wallet never learns those outpoints were spent, so on a NotBridged result the stale lock is the only thing keeping the mixer off an already-spent coin. The withTimeoutOrNull idiom returns an explicit true with ?: false rather than leaning on Unit != null.

Item 2 — what int21 actually is, and why the fold is not in this round

I scoped it rather than starting it. int21 is bfoss765/platform branch integration/v41-keystore-qa5 at f59a2291a — its tip commit is chore(qa5): bump rust-dashcore pin to qa/v41int21-pin @ ee2c9404, which is what names it. Against the a56a49a1e3 basis that int18 was cut from, that is 14 commits over 67 files:

  • #4329 — pooled ALL_SPENDABLE funding on the send path
  • #4350 — the same pooling on the asset-lock path (so identity registration and top-ups pool too)
  • asset-lock recovery work — #4342 / #4346 / #4347, plus a fix stopping a contact's watch-only chain from defining persisted transparent state
  • housekeeping — invitation cap, clippy/rustfmt debt, and the rust-dashcore pin bump

Two things make this more than a rebase:

0.1.0-v41int21-SNAPSHOT is not published. GitHub Packages currently tops out at 0.1.0-v41int20-SNAPSHOT (2026-08-09); there are six published versions and no int21. The fold target int21 + #922 + #928 + #4324 therefore cannot be consumed by anyone today — it has to be cut locally from a branch tip that can still move before int21 is actually published. That is a different problem from the maya8 AAR being local-only, and it stacks on top of it.

The pooled default changes ordinary sends, not just the drain. Confirmed on the int21 tip: ALL_SPENDABLE(3) exists and both sendToAddresses and buildSignedPayment default to it. Nothing in SdkL1SendService passes accountType, so on the fold every send silently widens to BIP44 + BIP32 + every DashPay receiving account. Your (a) is right, and it reaches further than buildDeferredMayaDeposit.

The intent is to adopt pooling rather than pin back to BIP44. That makes this a deletion, not an argument change: sendFromReceivalAccountOrRethrow, consolidateReceivalAccountsAndResendOrRethrow and sweepReceivalAccountToOwnBip44 exist to do on-chain, one transaction per account, what ALL_SPENDABLE now does natively in one — the same sweep-then-lock shape #4350's own commentary calls out. Retiring that machinery has a real test surface and only compiles against an int19+ AAR, which no branch here pins yet.

The open question

Should the MAX drain pool as well?

Under ALL_SPENDABLE a max deposit stops meaning "drain the BIP44 account" and becomes "drain BIP44 + BIP32 + every DashPay contact-receiving account into a MAYAChain vault." The hasProtectedOutputs preflight is fail-closed but was written for the narrower scope: it blocks on CrowdNode and seam locks, and it does not reason about receiving-account provenance. Widening the sweep without revisiting it moves more money past a check that believes it has already cleared the field.

My inclination is to pool ordinary sends and keep the drain on BIP44 explicitly, so a MAX sell sweeps exactly the account the user is looking at — but that is a product call about what "MAX" means on the sell screen, and it should be yours rather than inherited from a default. Whichever way it goes, the fold needs its own MAX run against the network, since the sweep scope is materially different from the one a5c99aec validated.

HashEngineering and others added 2 commits August 10, 2026 20:53
Pins 0.1.0-v41int21-SNAPSHOT -- the official integration AAR, with no local
suffix, so this branch builds for anyone once int21 is published. Testing
was done against a locally built 0.1.0-v41int21-maya10-SNAPSHOT (int21 with
rust-dashcore#928, platform #4286/#4288/#4324, the asset-lock fixes
#4336/#4337, and message signing #4319/#4321), which exists only in the
author's local Maven repository and is therefore deliberately NOT committed.

The SDK's send APIs default accountType to ALL_SPENDABLE from #4329, so
this pin changes funding scope without a call-site edit: sends and the MAX
Maya drain now draw on BIP44 + BIP32 + every DashPay contact-receiving
account, with change returning to BIP44. That is the intent -- a send
should reach the user's whole spendable balance, and the app-side
sweep-then-send machinery exists only because the SDK could not do this
before. No call site names an account type, deliberately.

Comments updated where they still asserted the old single-account scope:
sendToAddress's "BIP44 account 0 is the default", and
buildDeferredMayaDeposit's "a DRAIN spends every BIP44 UTXO".

Also records that the protected-outputs guard is wallet-wide rather than
per-account, so it still covers the sweep now that the pooled default has
widened it -- checked against hasAppLockedSpendableOutputs, whose narrow
per-account sibling is the separate CoinJoin-drain guard.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review item 2(b) from #1535. MayaBlockchainApiImpl.isInsufficientFunds
still walked the cause chain looking for the substring "insufficient
funds" -- written when the shortfall only reached us as key-wallet's
Display wrapped in a build failure. The SDK types it now, so match
DashSdkError.PlatformWallet.CoreInsufficientFunds (FFI 22) instead.

A matcher keyed on wording stops recognising the shortfall the moment the
wording changes, and the failure is silent: "not enough funds" degrades
into an opaque swap failure with no route to the familiar UI. The cause
chain is still walked, since the typed error can arrive wrapped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@HashEngineering

Copy link
Copy Markdown
Collaborator Author

Item 2 — the pooled fold is done, with one piece deliberately outstanding

Pushed as ec48350f9 and 75cc1514b.

What int21 actually is

Worth recording, since it wasn't resolvable when this was last discussed: int21 is bfoss765/platform branch integration/v41-keystore-qa5 at f59a2291a, whose tip commit is chore(qa5): bump rust-dashcore pin to qa/v41int21-pin @ ee2c9404. That commit landed 2026-08-10T14:09Z. Against the a56a49a1e3 basis int18 was cut from, it is 14 commits over 67 files: #4329 (pooled send funding), #4350 (pooled asset-lock funding), the #4342/#4346/#4347 asset-lock recovery work, and housekeeping.

The fold target is smaller than we thought. ee2c9404 already contains rust-dashcore#922 (merged 08-06 as dca5b05b8, verified as an ancestor), so it no longer has to be carried locally — and the local backport shim for the old 4d927c15 sizing constants is gone with it. Conversely, two merged platform PRs still have to be applied by hand, because the qa5 line never took them: #4286 and #4288. So the real target is:

int21 + #928 + #4286 + #4288 + #4324

The build

Local AAR 0.1.0-v41int21-maya10-SNAPSHOT — 19 commits on int21, adding #4336/#4337 and #4319/#4321 as well. platform-wallet 649 tests, platform-wallet-ffi 266, both clean; core_wallet_sign_message confirmed present in the shipped arm64-v8a library rather than assumed.

Three integration breaks surfaced and were fixed, all from qa5/v4.2-dev divergence:

  • DEFAULT_MAX_OP_RETURN_BYTESMAX_STANDARD_OP_RETURN_BYTES: the qa5-adapted #4286 used the name from the old pin; ee2c9404 carries feat: merchant filter event tracking #922, where it reverted. Same rename #4286 already followed upstream in a1db38df2.
  • #4337's recovery test called the 5-argument build_asset_lock_transaction; qa5 takes 7 (explicit funding account, drain, funding_path). Same adaptation int21 made for the ported #4342 tests.
  • #4319's helper built PlatformWalletInfo { balance }; this line carries a generation instead. The helper now returns the generation it installs, so the CoreWallet shares the one its wallet info holds rather than inventing a second.

The committed pin is 0.1.0-v41int21-SNAPSHOT — no local suffix, so the branch builds for anyone once int21 is published. Note that int21 is not published yet: GitHub Packages tops out at v41int20-SNAPSHOT (08-09). The maya10 AAR stays local-only and uncommitted, as maya8 did.

Your (a) — pooled scope

Confirmed and adopted rather than narrowed. The wallet compiles against maya10 with no source change and 272 tests pass, precisely because no call site names an account type — so sends and the MAX drain now draw on BIP44 + BIP32 + every DashPay contact-receiving account, change to BIP44. That is the intent: a send should reach the whole spendable balance, and the app-side sweep-then-send machinery exists only because the SDK could not do this before.

The stale assertions you flagged are corrected: sendToAddress's "BIP44 account 0 is the default" and buildDeferredMayaDeposit's "a DRAIN spends every BIP44 UTXO".

One thing I checked rather than assumed, having first mis-read it as a risk: hasProtectedOutputs is wallet-wide, not per-account — hasAppLockedSpendableOutputs asks whether the held dashj wallet tracks any app-locked output, and the per-account variant is the separate, explicitly narrow CoinJoin-drain guard. So the CrowdNode preflight still covers the sweep after the widening. Recorded in the comment there so it doesn't have to be re-derived.

Scope is also now documented SDK-side, in dashpay/platform#4324: SelectionStrategy.ALL said "drains the account", written when a drain could only mean one. It now states what a defaulted drain reaches, with a test pinning funding_sources() per selector so that anything later added to SEND_FUNDING_SOURCES — which would enlarge every defaulted drain — fails there first.

Your (b) — typed shortfall

Found one real leftover: MayaBlockchainApiImpl.isInsufficientFunds still walked the cause chain for the substring "insufficient funds". Now matches DashSdkError.PlatformWallet.CoreInsufficientFunds (FFI 22). A matcher keyed on wording stops recognising the shortfall the moment the wording changes, and it fails silently — "not enough funds" would degrade into an opaque swap failure with no route to the familiar UI. SdkL1SendService was already typed throughout.

What is NOT done

The MAX sell has not been re-run under the pooled default. You asked for that explicitly and you were right to: a5c99aec validated a drain over one account family, and this build drains three. Unit tests cannot cover it — they run against a fake source, so funding scope is the SDK's business, not theirs, and everything passes regardless.

So the fold is complete and green, and its behavioural evidence is not. I would not merge this ahead of that run.

Also unchanged: the base is current with feat/kotlin-sdk-phase1 (8476ae798) as of now — I re-checked, and both the local and remote phase1 refs are already contained. If you have in-flight SdkL1SendService.kt work not yet pushed, that still needs to land and the fold re-checked against it.

@HashEngineering

HashEngineering commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

MAX sell exercised on the pooled build — and it surfaced a real bug

The MAX sell has now been run on the pooled int21 build (0.1.0-v41int21-maya10-SNAPSHOT, ALL_SPENDABLE default), which was the outstanding evidence gap from item 2. The deposit itself works. What it exposed is a defect in how the wallet accounts for a drain after broadcast.

Reproduced on the pooled build

The MAX sell run today on int21/ALL_SPENDABLE is mainnet tx 1f608a9a…25fa — a textbook drain: 1 input of 0.05 fully consumed, VOUT0 0.04999725 to the vault, zero-value OP_RETURN at VOUT1, no change output (0.04999725 + 0.00000275 fee = 0.05 to the duff). InstantSend-locked, confirmed in block 2,519,886.

On device immediately after, the wallet's own mirror shows:

  • transactions row for 1f608a9a…: context = 0, blockHash = NULL — stuck at mempool context despite being confirmed and locked on chain.
  • txos: 1 unspent row of 0.05000000 with spendingTxid set — the drained input still counted. True balance is 0.

So the wallet reports 0.05 for a wallet it has just emptied. This is the same defect the earlier drain produced, now confirmed on the pooled build with a second independent transaction — it is reproducible, not a one-off.

The bug: a drain's inputs are never marked spent

After a MAX deposit the wallet permanently over-reports its balance by the amount it just sent. Observed: 0.12443157 DASH displayed when the true balance was 0.05000000 — inflated by exactly 0.07443157, the drain's input total.

Verified against the on-device SDK mirror rather than inferred:

  • txos: 763 rows. 760 spent rows all carry spendingTxid — healthy. Exactly 2 rows were isSpent = 0 with spendingTxid set, summing to 7,443,157 duffs, both pointing at drain a5c99aec…c873 (mainnet, confirmed block 2517981).
  • transactions: that drain sits at context = 0 with blockHash = NULL, while every other transaction in the wallet is context = 2 with a block hash. It never left mempool context.

Root cause

A drain pays the vault at VOUT0, carries a zero-value OP_RETURN at VOUT1, and has no change output — so the transaction contains no wallet-owned script.

  • dash-spv/src/sync/mempool/manager.rs:150 — the mempool filter watches wallet.watched_outpoints(), so it does see the spend. That is what writes spendingTxid.
  • dash-spv/src/sync/filters/manager.rs:841 — block/compact-filter matching watches monitored_script_pubkeys_for(...), script pubkeys only, no outpoints. So the block containing the drain never matches.
  • PlatformWalletPersistenceHandler.kt:849isSpent = txo.isSpent || context >= CONTEXT_IN_BLOCK. Mempool context correctly sets spendingTxid without flipping isSpent; the confirming context never arrives, so the flag never flips.

The SDK already documents this exact failure mode at PlatformWalletPersistenceHandler.kt:~1518 ("leaving the funding TXOs isSpent=0 (spendingTxid set) FOREVER") and carries a heal for it — but keyed only to an asset lock reaching InstantSendLocked. A Maya drain is not an asset lock, so nothing catches it.

A rescan does not fix this. It re-runs the same script-only block matching and misses the drain again for the same reason.

One cause, three symptoms

The stuck context explains everything seen on device, which is worth stating because they look like separate bugs:

Symptom Why
Balance inflated by the drained amount isSpent needs context >= CONTEXT_IN_BLOCK to flip
Home row stuck on "Sending" across restarts row status is driven by the same context, still 0
Row never titled "Conversion DASH/RUNE" downstream of the same stuck state

The tagging itself is fine: swap_orders holds a fully populated record (service swapkit, DASH→RUNE, provider, deposit address, amounts) and transaction_metadata has service = 'swapkit'. The transaction details screen reads that and correctly shows a Maya swap. Nothing is racing — the row simply never advances.

Does this affect the primary SendCoinsActivity/Fragment?

Yes, but not yet — it lands with the cutover.

This is not Maya-specific. The trigger is "a transaction with no wallet-owned output", and the send-all path produces exactly that shape. From SdkL1SendService's own contract for emptyWallet:

POST-CUTOVER it routes through the SDK drain (SelectionStrategy::All — all spendable inputs, one output worth total − fee, no change)

So:

  • Today: SendCoinsFragment's send-all is pre-cutover and still on dashj (emptyWallet && !cutoverCommitted() → NotBroadcast), which has its own transaction tracking. The bug is masked.
  • Post-cutover: the same "send max" button routes through the identical SDK drain. Sending the whole balance to any external address produces one output that is not ours, no change — and reproduces this exactly: inflated balance, row stuck on "Sending".
  • Not affected: a send-all to one of the user's own addresses. That output is wallet-owned, so the block filter matches and context advances normally. This is why the existing sweepReceivalAccountToOwnBip44 self-sweeps have never shown the problem, and why the other 760 spends in this wallet are correctly marked — ordinary sends leave change, which is a wallet-owned output.

So the blast radius is every SDK drain to a third party, and the main send screen inherits it the moment the cutover flips. Worth fixing before that, independently of Maya.

Fixes in flight

Two work streams, tracked separately:

  1. Root cause — make block/compact-filter matching watch wallet outpoints alongside script pubkeys in dash-spv/src/sync/filters/manager.rs, so spending a known UTXO matches its block regardless of that transaction's outputs. Plus a generalised SDK heal: any TXO with spendingTxid set and isSpent = 0 whose spending transaction is confirmed gets flipped (the getUnspentBySpendingTxid DAO query already exists; the general pass that used it was replaced by an inline link).
  2. Display layer — whether the row recovers once context reaches 2, or whether the rendered "Sending" state is latched by the display cache.

The acceptance test for the first should be transactions.context advancing to 2, not simply the balance settling — the balance can be corrected by hand while the underlying transaction is still stuck, so fixing to the symptom would leave the row bug in place.

HashEngineering and others added 2 commits August 11, 2026 10:35
A MAX Maya sell is now an ordinary fixed-amount send of
`spendable - mayaMaxFeeReserveDuffs(...)`, the same system the shielded
max-shield and Buy Credits already use, replacing SelectionStrategy.ALL.

The drain had to go because of what it produced, not what it computed: vault
output, zero-value OP_RETURN, no change -- a transaction with NO wallet-owned
script. Compact block filters match wallet script pubkeys only, so such a
transaction is never matched in a block, its context never reaches
CONTEXT_IN_BLOCK, and the wallet counts the spent inputs as spendable
forever. Two mainnet drains proved it: a5c99aec (balance inflated by
0.07443157) and 1f608a9a on the pooled build (reported 0.05 for a wallet it
had just emptied, row stuck on "Sending" across restarts). A rescan cannot
recover either -- it re-runs the same script-only matching.

Withholding a reserve restores a change output, so the deposit confirms and
settles like any other send. It also makes quoting stricter rather than
looser: the app names the amount and the transaction pays exactly that, so
quote and payment are equal by construction and under-delivery is
unreachable. The reserve's unused remainder returns as change, which is what
makes over-reserving lossless. A MAX sell therefore leaves a small remnant
rather than emptying to zero -- deliberate, and not surfaced, matching
shielded.

Sizing mirrors assetLockMaxFeeReserve (~148 vbytes per input, doubled) but
sizes the data carrier exactly, since a Maya quote always knows its memo
length. Floored at MAYA_MAX_RESERVE_MIN_INPUTS because the reachable dashj
UTXO count freezes post-cutover and under-reserving is the failing
direction; the overlaid count behind WalletDataProvider is the eventual
source.

Removed: the drain parameter through SdkL1SendSource, the probe build in
maxMayaDepositDuffs, and expectedVaultDuffs -- max sells verify against the
quote again, like every other sell. The app-locked-output guard STAYS and now
keys on isMaxDeposit: a reserve leaves change but does not narrow which coins
are selected, so a max deposit is still sweep-scale and can still reach
CrowdNode-locked outputs.

Revisit when the SDK computes MAX internally in the wallet engine; the
engine should own the amount rather than this arithmetic.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Maya/SwapKit MAX sell stayed titled "Sending" on the home screen forever
(across app restarts) and never rendered "Conversion DASH/RUNE", while the
transaction-details screen identified the swap correctly the whole time.
Reproduced on-device by rewinding the cached row and restarting: it did not
recover. Two independent display-layer faults combined.

1. The swap decoration was only derivable from a dashj transaction.
   TransactionRowView.fromTransaction was the sole reader of
   metadata.swapOrder, so every writer that produced it needed a resolvable
   dashj wrapper. TxDisplayCacheService's metadata flow silently writes
   nothing when no wrapper resolves -- after already assigning
   this.metadata -- so the swap-order change is consumed and never seen
   again. The row for this MAX sell was authored by CutoverUiDataService
   (the SDK path), which has no notion of swap orders at all.

2. The SDK planner then re-titled the row on every pass. Its never-touch
   carve-out keys off existing.service, but a row whose decoration was
   already lost carries service = null, so the definitive plain-send
   re-stamp claimed it -- and with the SDK record's context stuck at
   mempool, that re-stamp is permanently "Sending".

Fix: derive the decoration (convert icon, "Conversion ..."/"Converted ..."
title, swapStatus) from the swap_orders record by txid, with no dashj
transaction involved -- the same source the details screen observes. The new
pure planSwapRowDecorations runs from reconcileSwapRows on every metadata
emission (not gated on the diff) and on every DisplayCacheRefreshBus tick,
so an SDK-authored insert or re-stamp is corrected whichever writer got
there first. Idempotent: only decoration fields are touched, and a settled
row produces no write.

Swap rows also join the SDK planner's never-touch set (swapStatus != null,
in both planL1DisplaySync and planL1InstantLockRowUpdate) so a decorated row
holds stable instead of flip-flopping once per sync pass. The title choice
moves to TransactionRowView.swapTitleRes so renderer and reconciler cannot
drift.

Adds 12 host-JVM regression tests, including the full mempool -> in-block
story: a row born plain gets decorated, survives the context advance without
being re-titled, follows PENDING -> COMPLETED rather than being pinned to a
stale rendering, then settles. One test asserts a non-swap row of the same
shape is still re-stamped, so the carve-out is not over-broad.

Not addressed: the swap vault address is still marked TaxCategory.Expense
(MayaConversionPreviewViewModel) -- TaxCategory has no Trade value, and
adding one reaches into the CSV export and the category picker.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@HashEngineering

Copy link
Copy Markdown
Collaborator Author

Summary of changes since the review

Everything below is pushed. Branch head e6cee93ca.

Review items

Item State
1 — dead constants, retired-model KDocs done (681190f7f) — three stale KDocs, not two
2 — fold onto the pooled line done (ec48350f9) — int21 + pooled ALL_SPENDABLE, adopted rather than pinned back
3 — dependency chain description corrected; fold target restated
4 — guard in the primitive done (681190f7f), retargeted to isMaxDeposit in ab1bd05db
5 — mirror-lock comment done (681190f7f)
6 — contradictory testing claims corrected; both runs were mainnet, real funds, never testnet
7 — Unit != null idiom done (681190f7f)
(b) typed shortfall done (75cc1514b) — one string matcher survived in MayaBlockchainApiImpl

The substantive change: MAX no longer drains

ab1bd05db. A MAX sell is now an ordinary fixed-amount send of spendable − mayaMaxFeeReserveDuffs(...), the same system shielded max-shield and Buy Credits use. SelectionStrategy.ALL is gone from the wallet.

The reason is the bug documented above: a drain produced destination + zero-value OP_RETURN + no change, so the transaction had no wallet-owned output, compact block filters never matched its block, and the spend never landed — the balance stayed inflated indefinitely and the row never left "Sending". Withholding a reserve restores a change output, so the deposit settles like any other send.

It also makes quoting stricter, not looser: the app names the amount and the transaction pays exactly that, so quote and payment are equal by construction and under-delivery is unreachable. The reserve's remainder returns as change, so over-reserving is lossless. A MAX sell leaves a small remnant rather than emptying to zero — deliberate, unsurfaced, matching shielded.

The app-locked-output guard stayed and now keys on isMaxDeposit: a reserve leaves change but does not narrow which coins are selected, so a max deposit is still sweep-scale and can still reach CrowdNode-locked outputs.

Verified on mainnet: the MAX sell on this build worked through Dash DEX/Maya and left a change output with no stale TXO rows — the failure mode the drain produced twice.

Two issues a reviewer should know about

1. CI cannot build this branch yet. The committed pin is 0.1.0-v41int21-SNAPSHOT, deliberately suffix-free so it is buildable by anyone — but int21 is not published. GitHub Packages tops out at v41int20-SNAPSHOT. The build check will fail on dependency resolution until int21 ships.

Worse, publishing int21 alone will not be enough: the qa5/int21 line does not carry #4286 or #4288 (verified — zero references to the OP_RETURN / output-order / change-to-VIN0 controls), which this branch needs. Testing used a local 0.1.0-v41int21-maya10-SNAPSHOT that folds them in, and that AAR exists only on one machine. So the fold target for a publishable pin is int21 + #4286 + #4288.

2. The #4324 dependency is now nearly vestigial. Since MAX stopped draining, nothing in the wallet consumes selectionStrategy or the deliverable amount — except one line, SdkL1SendService.kt:1387, which still populates SdkDeferredPayment.deliverableDuffs from signed.deliverableAmountDuffs. Drop that field and platform#4324 leaves this branch's dependency chain entirely. Worth doing before merge if we would rather not block on #4324 and the v4.2-dev key-wallet pin behind it.

Still open, tracked separately

The filter gap itself is unfixed: block/compact-filter matching watches script pubkeys only, so any changeless transaction paying a third party is invisible to it. The wallet now avoids producing that shape, but post-cutover SendCoinsFragment's send-all will produce itemptyWallet routes through the SDK drain — so this is worth fixing in dash-spv independently of Maya. Reported on rust-dashcore#928 and platform#4324.

…t/maya-sdk-route

# Conflicts:
#	build.gradle
#	wallet/src/de/schildbach/wallet/service/platform/sdk/SdkL1SendService.kt
#	wallet/test/de/schildbach/wallet/service/platform/sdk/SdkL1SendServiceTest.kt
@bfoss765

Copy link
Copy Markdown
Contributor

Re-reviewed at head ce0c84531. Good progress on the fold and the pooled ALL_SPENDABLE reconstruction, and the fund-safety error paths check out — build/verify failure releases the deferred payment, NotBroadcast unlocks the mirror and releases, Ambiguous deliberately keeps both reservation and mirror locks with a non-retryable message, and CancellationException is rethrown before any Failure conversion. No broadcast on any error path. The typed CoreInsufficientFunds matcher replacing the string match is done too.

Keeping changes-requested for the following.

Blocker: does not compile against the merge target

Merged onto current base ceb206ed1 and built — one error:

SdkL1SendService.kt:862:39  Unresolved reference 'deliverableAmountDuffs'

SignedCoreTransaction.deliverableAmountDuffs is absent from both 0.1.0-v42int4 (the base pin) and 0.1.0-v41int22-SNAPSHOT (this branch's own committed pin), so the head doesn't build against either. The value also appears to be dead: deliverableDuffs (SdkL1SendService.kt:489) is written at :866 and read nowhere in wallet/ or integrations/. Deleting the field — as you suggested — makes the branch build on the current base with no other change.

Medium: the changeless-transaction bug may still be reachable

This is about the post-review MAX-reserve redesign (ab1bd05db), which hasn't been reviewed yet.

mayaMaxFeeReserveDuffs (:321-325) is max(2 × estimatedBytes, 1000). At the SDK's default 1000 duffs/kB the fee ≈ estimatedBytes, so the change output ≈ estimatedBytes with a floor near 500 duffs — while dashj's MIN_NONDUST_OUTPUT is 546. For a wallet with ~2–3 spendable UTXOs that change lands in the dust band. Your own test pins the shape: mayaMaxFeeReserveDuffs(1, 80) == 1000L (SdkL1SendServiceTest.kt:1284).

If the engine folds sub-dust change into the fee, the transaction becomes vault + OP_RETURN with no wallet-owned output — and verifyMayaDepositShape accepts that today (MayaBlockchainApiImpl.kt:87 allows 2..3 outputs; MayaDepositShapeTest.kt:81 wellFormedDepositWithoutChangePasses) → broadcast → the same permanent balance inflation as a5c99aec / 1f608a9a.

I could not determine from this repo what rs-platform-wallet does with sub-dust change (drop-to-fee vs refuse), which is what decides whether this is live or just an unenforced invariant. Worth either requiring a change output when isMaxDeposit, or giving the reserve a dust-aware floor — and confirming the engine's behaviour either way.

Item 1 regressed on the doc I named

MayaBlockchainApi.kt:51-63 is stale again: it says the value is "what a DRAIN of the funding account delivers … never reduced by a headroom or reserve constant", while the implementation is spendable − reserve (MayaBlockchainApiImpl.kt:179-180). Same for MayaBlockchainApiImpl.kt:292 and SwapKitApiAggregator.kt:647 ("biased HIGH" — now biased low). The constants deletion and the maxMayaDepositDuffs KDoc rewrite are done ✓.

Related new dead code in the same class: SdkDeferredPayment.deliverableDuffs (:476-489) whose KDoc still says "A max swap deposit must check this against the quoted amount BEFORE broadcasting" — it no longer does; plus test scaffolding FakeSource.mayaBuiltDrains (SdkL1SendServiceTest.kt:137) and drainDeliverable (:144) documented against a [drain] parameter the interface no longer has.

Item 4 weakened

hasProtectedOutputs() is in the primitive (SdkL1SendService.kt:1702-1704) ✓, but it's gated on isMaxDeposit: Boolean = false (:1669) which the caller sets (MayaBlockchainApiImpl.kt:332). Under the old design drain=true made it structural; now a MAX and a large ordinary deposit build identically, so the guard is back to "the caller remembered" — with the unguarded value as the default.

Item 6 — PR description

The description at head still describes the retired drain design end-to-end: it names pin v41int18-SNAPSHOT (actual: v41int22), says the pooled reconstruction is "deliberately not part of this round" (it's in ec48350f9), says "A MAX sell builds as a drain" and that the shape check verifies against deliverableAmountDuffs — both retired by ab1bd05db — and cites drain tx a5c99aec as evidence for a path that no longer exists. The current design only exists in code comments. Since this is the artifact reviewers read first, it's worth rewriting before the next round.

Minor

  • Orphaned KDoc: the long mayaMaxFeeReserveDuffs doc (:281-315) is followed by a second KDoc for MAYA_MAX_RESERVE_MIN_INPUTS (:315-319), so it binds to the const and the function itself (:321) ends up undocumented.
  • WalletDataAdapter.waitUntilLocked (:204-225) now consults the SDK seam before the held dashj wallet — small blast radius (MayaConversionPreviewViewModel.kt:137, CrowdNodeBlockchainApi.kt:57), but it changes pre-cutover behaviour for any tx the SDK store already knows.
  • ReservationLockMirror.setLocks (:47-60) silently no-ops when walletData.wallet == null — fail-soft (both call sites runCatching), but the no-op is invisible.

@HashEngineering

Copy link
Copy Markdown
Collaborator Author

@bfoss765 This is ready for a re-review — every item from your review is addressed, and the dependency picture has changed materially since then.

Your items: 1, 3, 4, 5, 6, 7 and (b) are done, per the earlier comments. Item 2's fold is done too: the branch is on the pooled ALL_SPENDABLE line, merged up to feat/kotlin-sdk-phase1 at 5f0ee87c7 — your pooling commit's deletions of the receival machinery were taken over our re-documented versions of the same code.

The dependency chain collapsed since your review:

  • rust-dashcore#928 merged (2026-08-13).
  • platform#4324 is bot-approved at its current head — the v4.2-dev pin now carries Feat(dashpay): log event timing for platform functions (NMA-1249) #928 and the doc findings are fixed. It awaits a human maintainer only.
  • The fold target you flagged is therefore no longer speculative; the remaining unpublished piece is the integration AAR itself.

One design change you should weigh as part of the re-review, since it happened after your pass: the MAX sell no longer drains. It is now an ordinary fixed-amount send of spendable − fee reserve (the shielded/Buy Credits pattern), because a true drain — no wallet-owned output — is invisible to compact block-filter scanning, which left spent inputs counted as unspent and the balance permanently inflated. Reproduced twice on mainnet (a5c99aec…, 1f608a9a…, details in the thread above); the reserve restores a change output and the deposit settles normally. The CrowdNode fail-closed guard stayed, keyed on isMaxDeposit, since a reserve does not narrow which coins selection reaches. Root-cause fixes for the filter gap are in progress separately (dash-wallet#1538 has the plain-language write-up).

Testing on the current head: the MAX sell was run end to end on mainnet through Dash DEX/Maya on the reserve model and settled correctly — change output present, no stale TXO rows, quote equal to payment by construction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants