Skip to content

fix: use fast fee for savings transfer#1091

Open
piotr-iohk wants to merge 4 commits into
masterfrom
fix/1034-savings-transfer-fast-fee
Open

fix: use fast fee for savings transfer#1091
piotr-iohk wants to merge 4 commits into
masterfrom
fix/1034-savings-transfer-fast-fee

Conversation

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Fixes #1034

This PR makes savings → spending channel funding use a faster on-chain fee rate, matching the “~10 minutes” copy and iOS behavior.

Description

  • Defaults savings → spending funding to the fast fee tier instead of the settings default.
  • Reserves a fast send-all mining fee when sizing max transfer, so “transfer all” leaves room to pay the order on-chain (fixes the tight-balance failure mode seen in multi_address_2).
  • Aligns send-all with iOS: drain only for dust change, or retry drain after a failed fixed send when the drain output still covers the order — never underpay by emptying the wallet.

Preview

N/A

QA Notes

Manual Tests

  • 1. Funded savings wallet → Transfer → Spending → confirm max amount: funding tx uses a fast-tier sat/vB (check explorer), not the settings mid/default when that mid is ~1.
  • 2. Transfer → Spending → use max → confirm: order is sized with mining-fee headroom and funding succeeds (no coin-selection / insufficient-funds toast on a normal fee market).
  • 3. Tight multi-UTXO balance (transfer all) → Transfer → Spending → confirm: channel funding progresses; leftover is not left as an unaffordable fixed send.
  • 4. regression: Transfer → Spending with a partial amount well below balance: still funds with a normal (non-drain) send when change is above dust.

Automated Checks

  • Unit tests added: fast mining-fee reserve on max sizing; dust send-all; send-all retry when drain still covers the order; no drain when send-all would underpay, in TransferViewModelTest.kt.
  • Local: just test file TransferViewModelTest.
  • CI: standard compile, unit test, and detekt checks run by the PR bot. E2E multi_address_2 should pass once this lands on the resolved e2e branch.

Made with Cursor

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR updates savings-to-spending transfers to use fast on-chain fees. The main changes are:

  • Calculates the maximum transfer from raw spendable funds with one fast-fee reserve.
  • Uses a percentage fallback when fast-fee estimation fails.
  • Drains the wallet only for dust change or a retry that can fully fund the order.
  • Adds tests for fee reservation and send-all behavior.

Confidence Score: 5/5

This looks safe to merge.

  • The updated limit calculation removes the duplicate fee reserve.
  • Fee-estimation failure now keeps conservative funding headroom.
  • Send-all retries are blocked when the resulting output cannot cover the order.
  • No blocking issue remains in the changed code.

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt Uses fast fees, reserves one mining fee when sizing transfers, and guards send-all funding against underpayment.
app/src/test/java/to/bitkit/viewmodels/TransferViewModelTest.kt Adds coverage for fee reservation, estimation fallback, dust draining, retry draining, and underpayment prevention.
changelog.d/next/1091.fixed.md Documents the updated fee selection, maximum sizing, and wallet-drain behavior.

Reviews (2): Last reviewed commit: "fix: avoid double mining fee on max tran..." | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt Outdated
Comment thread app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt
@piotr-iohk
piotr-iohk marked this pull request as draft July 17, 2026 10:34
Co-authored-by: Cursor <cursoragent@cursor.com>
@piotr-iohk
piotr-iohk marked this pull request as ready for review July 17, 2026 10:37
@piotr-iohk
piotr-iohk requested a review from jvsena42 July 17, 2026 10:42
@piotr-iohk piotr-iohk self-assigned this Jul 17, 2026
@piotr-iohk piotr-iohk added this to the 2.5.0 milestone Jul 17, 2026
/** Pays for the order and start watching it for state updates */
fun onTransferToSpendingConfirm(order: IBtOrder, speed: TransactionSpeed? = null) {
@Suppress("LongMethod")
fun onTransferToSpendingConfirm(order: IBtOrder, speed: TransactionSpeed = TransactionSpeed.Fast) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fun onTransferToSpendingConfirm(order: IBtOrder, speed: TransactionSpeed = TransactionSpeed.Fast) {
fun onTransferToSpendingConfirm(order: IBtOrder) {

A custom speed is never set anywhere, could remove this parameter

Comment on lines 236 to 237
val shouldUseSendAll =
expectedChange >= 0 && expectedChange < TRANSFER_SEND_ALL_THRESHOLD_SATS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: could apply a small refactor converting this to a range check

Comment on lines +238 to +239
val maxSendable =
if (sendAllFee >= spendableBalance) 0uL else spendableBalance - sendAllFee

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use .safe() here. It has this clamp-to-zero internal logic

Comment on lines +448 to +449
val availableAmount =
if (miningFee >= spendable) 0uL else spendable - miningFee

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use .safe() here too

).onFailure { error ->
// Match iOS: if fixed send fails (e.g. coin selection), drain only when the
// drain output still covers order.feeSat — never underpay by emptying the wallet.
if (maxSendable >= order.feeSat) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still checking this logic

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]: Transfer to spending should use 'fast' transaction speed

2 participants