Skip to content

fix: keep pending transfer in total#1058

Open
piotr-iohk wants to merge 4 commits into
masterfrom
fix/808-pending-transfer-balance
Open

fix: keep pending transfer in total#1058
piotr-iohk wants to merge 4 commits into
masterfrom
fix/808-pending-transfer-balance

Conversation

@piotr-iohk

@piotr-iohk piotr-iohk commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes #808

This PR keeps pending transfer funds in the headline total balance during savings → spending moves, matching iOS behavior and avoiding a brief inflation window while the LSP funding transaction syncs.

Description

During an in-progress transfer to spending, funds were tracked in balanceInTransferToSpending but excluded from totalSats, so the home total could drop to zero (especially on max-transfer flows). iOS already includes both in-transfer buckets in its total.

This change:

  1. Includes balanceInTransferToSpending and balanceInTransferToSavings in totalSats.
  2. Stores txTotalSats and preTransferOnchainSats on Blocktank LSP order transfers and subtracts the funding amount from displayed on-chain balance until LDK reflects the send (iOS parity).
  3. Adds Room migration 6→7 (two nullable columns on transfers; existing rows stay NULL and keep prior behavior).

Out of scope / known follow-ups:

  • To-spending guard edge cases during concurrent on-chain deposits (same heuristic as iOS).
  • Force-close to-savings total dip when LDK closing balance disappears before sweep detection (iOS has an activity fallback we have not ported yet).
  • Manual / third-party LSP setup paths are unchanged beyond the shared totalSats fix; e2e covers those flows.

Preview

Screen.Recording.2026-07-02.at.14.59.23.mov

QA Notes

Manual Tests

Migration (Room 6→7)
  • 1. Install bitkit_dev_release_2.3.1-stag-universal.apk → create/restore wallet → fund on-chain → open spending channel via Blocktank → note headline total.
  • 2. Upgrade in place (no clear data) to bitkit_dev_release_2.3.1-808fix-stag-universal.apk → app opens without crash → balance, channels, and activity history match pre-upgrade.
  • 3. regression: After migration upgrade → pull-to-refresh on Home → balances and channels still correct.
  • 4. Optional schema check on emulator (adb root): transfers table has nullable txTotalSats and preTransferOnchainSats columns after upgrade.
Fix verification (#808)
  • 1. Savings → Spending → confirm Blocktank LSP transfer → during pending/setup: headline total stays near pre-transfer amount (no drop to zero, no brief inflation).
  • 2. Wait for channel to open → total settles correctly and in-transfer chip clears.
  • 3a. regression: Savings → Spending with send-all path (most/all of savings): total stable through pending.
    • 3b. regression: Partial-amount transfer: total stable through pending.

Automated Checks

  • Unit tests added: cover totalSats including in-transfer buckets in BalanceStateTest.kt.
  • Unit tests added: cover LSP funding on-chain subtraction while balance has not synced in DeriveBalanceStateUseCaseTest.kt.
  • Unit tests modified: update balance derivation assertions in DeriveBalanceStateUseCaseTest.kt.
  • Local: BalanceStateTest, DeriveBalanceStateUseCaseTest pass; just compile passes.
  • Changelog fragment: changelog.d/next/808.fixed.md (rename to 1058.fixed.md after PR number is known).
  • CI: standard compile, unit test, and detekt checks run by the PR bot.

piotr-iohk and others added 2 commits July 2, 2026 13:38
Comment thread app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt Fixed
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a balance display regression where the home screen total dropped to zero during a savings→spending Blocktank LSP transfer by including balanceInTransferToSpending and balanceInTransferToSavings in totalSats, and by temporarily subtracting the funding tx amount from displayed on-chain balance while LDK has not yet reflected the send.

  • BalanceState.totalSats extended to include both in-transfer buckets using saturating USat arithmetic; a Room migration 6→7 adds two nullable columns (txTotalSats, preTransferOnchainSats) to the transfers table with NULL defaults that preserve existing rows.
  • DeriveBalanceStateUseCase now computes orderPaymentsOnchainToSubtract and reduces totalOnchainSats accordingly, matching the iOS BalanceManager.getOrderPaymentOnchainToSubtract heuristic; the guard currentOnchainSats >= preTransferOnchainSats resets once LDK syncs the outgoing send.
  • TransferViewModel.onTransferToSpendingConfirm captures txTotalSats (spend-all or fee+mining) and preTransferOnchainSats (pre-broadcast on-chain total) before broadcasting and persists them through fundPaidOrder; the hardware-wallet path (onTransferToSpendingHwConfirm) is acknowledged in a previous review as a known follow-up.

Confidence Score: 5/5

Safe to merge; the core balance fix, database migration, and saturating arithmetic are all correct, and the two known follow-ups (concurrent-deposit heuristic, HW-wallet subtraction gap) are bounded, self-healing, and already tracked.

The balance derivation logic is well-reasoned and matches the iOS reference implementation. The Room migration is additive (nullable columns, existing rows stay NULL), the USat saturating arithmetic prevents overflow in the new totalSats computation, and the guard condition correctly gates the on-chain subtraction on the LDK-sync window. Unit tests cover the new paths end-to-end. The two acknowledged edge cases (concurrent deposit, HW wallet) are correctly scoped as follow-ups with bounded, self-healing impact.

No files require special attention beyond the acknowledged follow-ups already noted in previous review threads.

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/models/BalanceState.kt totalSats now includes balanceInTransferToSavings and balanceInTransferToSpending with saturating USat arithmetic; final plus() is already saturating so the missing trailing .safe() is not a bug
app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt New getOrderPaymentOnchainToSubtract correctly gates on currentOnchainSats >= preTransferOnchainSats; the known concurrent-deposit edge case (acknowledged in previous thread) causes a brief dip rather than inflation
app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt onTransferToSpendingConfirm correctly captures txTotalSats and preTransferOnchainSats before broadcasting; HW wallet path still passes null for both (known follow-up per previous review)
app/src/main/java/to/bitkit/data/AppDb.kt MIGRATION_6_7 adds two nullable INTEGER columns with DEFAULT NULL; correctly chained alongside MIGRATION_5_6; schema JSON matches the entity definition
app/src/main/java/to/bitkit/data/entities/TransferEntity.kt Two nullable Long columns added with default null; schema JSON in 7.json matches; existing rows migrate safely
app/src/main/java/to/bitkit/repositories/TransferRepo.kt createTransfer signature extended with txTotalSats/preTransferOnchainSats optional params; passed through correctly to entity
app/src/test/java/to/bitkit/models/BalanceStateTest.kt New tests cover both-buckets inclusion and ULong.MAX_VALUE saturation; totalWithHardwareSats assertion correctly updated to 190 to reflect the new totalSats calculation
app/src/test/java/to/bitkit/usecases/DeriveBalanceStateUseCaseTest.kt New test exercises the LSP funding subtraction path end-to-end; totalSats assertions added to several existing scenarios
app/src/test/java/to/bitkit/viewmodels/TransferViewModelTest.kt HW confirm test updated to assert isNull() for both new params; no new test for the regular confirm path's non-null values
app/schemas/to.bitkit.data.AppDb/7.json Schema snapshot v7 correctly records both new nullable INTEGER columns and matches the migration SQL

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User confirms Savings→Spending transfer] --> B[onTransferToSpendingConfirm]
    B --> C[getBalancesAsync: capture preTransferOnchainSats]
    C --> D{shouldUseSendAll?}
    D -- Yes --> E[txTotalSats = spendableBalance]
    D -- No --> F[txTotalSats = feeSat + miningFee]
    E --> G[sendOnChain broadcast]
    F --> G
    G --> H[fundPaidOrder: persist txTotalSats + preTransferOnchainSats to DB]
    H --> I[syncBalances triggered]
    I --> J[DeriveBalanceStateUseCase.invoke]
    J --> K[getOrderPaymentOnchainToSubtract]
    K --> L{currentOnchainSats >= preTransferOnchainSats LDK not yet synced?}
    L -- Yes --> M[subtract txTotalSats from totalOnchainSats]
    L -- No --> N[no subtraction — LDK already reflected send]
    M --> O[totalSats = totalOnchainSats + totalLightningSats + balanceInTransferToSavings + balanceInTransferToSpending]
    N --> O
    O --> P[Home screen shows stable total]
    style P fill:#22c55e,color:#fff
    style M fill:#f59e0b,color:#fff
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[User confirms Savings→Spending transfer] --> B[onTransferToSpendingConfirm]
    B --> C[getBalancesAsync: capture preTransferOnchainSats]
    C --> D{shouldUseSendAll?}
    D -- Yes --> E[txTotalSats = spendableBalance]
    D -- No --> F[txTotalSats = feeSat + miningFee]
    E --> G[sendOnChain broadcast]
    F --> G
    G --> H[fundPaidOrder: persist txTotalSats + preTransferOnchainSats to DB]
    H --> I[syncBalances triggered]
    I --> J[DeriveBalanceStateUseCase.invoke]
    J --> K[getOrderPaymentOnchainToSubtract]
    K --> L{currentOnchainSats >= preTransferOnchainSats LDK not yet synced?}
    L -- Yes --> M[subtract txTotalSats from totalOnchainSats]
    L -- No --> N[no subtraction — LDK already reflected send]
    M --> O[totalSats = totalOnchainSats + totalLightningSats + balanceInTransferToSavings + balanceInTransferToSpending]
    N --> O
    O --> P[Home screen shows stable total]
    style P fill:#22c55e,color:#fff
    style M fill:#f59e0b,color:#fff
Loading

Reviews (2): Last reviewed commit: "Merge branch 'master' into fix/808-pendi..." | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a29b8c4b34

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt Outdated
Comment thread app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt
Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt
Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt
Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt
Comment thread app/src/main/java/to/bitkit/usecases/DeriveBalanceStateUseCase.kt
@piotr-iohk piotr-iohk marked this pull request as draft July 2, 2026 15:00
Co-authored-by: Cursor <cursoragent@cursor.com>
@piotr-iohk

Copy link
Copy Markdown
Collaborator Author

Regarding:

P2 Hardware wallet path not getting the onchain-subtraction fix

onTransferToSpendingHwConfirm calls fundPaidOrder without txTotalSats or preTransferOnchainSats, so the transfer entity is stored with NULL for both fields. getOrderPaymentOnchainToSubtract skips transfers where either value is null, so the onchain balance is never reduced during the LDK sync window for HW-funded transfers. With balanceInTransferToSpending now always included in totalSats, this creates the brief inflation the PR is fixing — the total will include both the still-high totalOnchainSats (not yet synced) and the clientBalanceSat added by paidOrdersSats — until LDK syncs and drops the onchain balance on its own.

Re Greptile's outside-diff note on onTransferToSpendingHwConfirm (HW path not getting the onchain-subtraction fix):

The NULL fields for HW transfers are intentional, and the described inflation doesn't apply as stated:

getOrderPaymentOnchainToSubtract corrects LDK's totalOnchainBalanceSats, but a HW-funded transfer spends from the Trezor's UTXOs — LDK's on-chain balance never contained those sats and doesn't need reducing. Storing txTotalSats/preTransferOnchainSats here would apply a wrong subtraction to LDK's balance.

The actual double-count risk is in totalWithHardwareSats: balanceInTransferToSpending adds clientBalanceSat while the hardware wallet's watch-only balance still shows its pre-spend value. That window lasts only until the HW balance refresh picks up the unconfirmed tx (blockbook indexes mempool txs within seconds) — not until LDK sync, as suggested. Manual testing on emulator shows no visible inflation during HW transfers.

@piotr-iohk piotr-iohk marked this pull request as ready for review July 2, 2026 15:34

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5882321f27

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

.safe()
.plus(balanceInTransferToSavings.safe())
.safe()
.plus(balanceInTransferToSpending.safe())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply the funding correction to manual opens

When users open an external/manual channel, ExternalNodeViewModel records a MANUAL_SETUP transfer with a funding tx but no lspOrderId, so the new on-chain correction in DeriveBalanceStateUseCase never applies to it. With this added balanceInTransferToSpending term, if LDK still reports the pre-open on-chain balance while the channel is pending, the home total becomes stale savings plus the pending channel amount until the wallet sync catches up.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Valid observation, intentionally out of scope: manual opens go through LDK's connectOpenChannel, which funds and broadcasts from LDK's own wallet in one step, so the stale-balance window is at most one sync cycle. iOS's equivalent correction also filters on lspOrderId and skips manual opens — this is parity. Tracked in #1059: the planned hardening (funding-txid presence check instead of the totals heuristic) applies uniformly to LSP and manual transfers and covers this case.

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.

[Bug]: pending transfer not reflected in total balance

2 participants