Skip to content

fix: validate minimum paykit request amounts - #913

Open
ben-kaufman wants to merge 1 commit into
masterfrom
fix/paykit-payment-validation-20261010
Open

ben-kaufman wants to merge 1 commit into
masterfrom
fix/paykit-payment-validation-20261010

Conversation

@ben-kaufman

Copy link
Copy Markdown
Contributor

This PR fixes misleading balance feedback for on-chain Paykit requests below 547 sats.

Description

  • Validate the minimum on-chain request amount during scan routing and deferred wallet preparation so sufficient funds cannot produce “0 more needed”; close preparation while keeping the unpaid request available for manual retry.
  • Generalize the existing payment-rejection signal so an invalid amount receives the same presentation cleanup as an insufficient balance.
  • Add the matching Android QA journey so both platforms verify the minimum boundary, retry cleanup, and another payable request.

Out of Scope

  • Paykit SDK: shared-state lease contention, abandoned-write recovery waits, and server reconciliation delays.
  • Wallet networking: Electrum connection and synchronization delays.

Design

N/A — no design available. Reuses the existing minimum-invoice toast.

Preview

QA Notes

Journeys

  • new minimum-onchain-amount.xml — 100 and 546 sats show the 547-sat minimum, preparation closes, unpaid requests remain retryable, and 547 sats reaches confirmation.

Manual Tests

N/A

Automated Checks

  • added PaykitPaymentRequestServiceTests.swift — four regressions cover below-minimum scan routing and manual retry, deferred sync fallback with sheet dismissal, the payable boundary, and a small valid Lightning alternative.
  • updated PaykitPaymentRequestServiceTests.swift — existing balance-rejection assertions use the generalized rejection signal.
  • ran a complete base-versus-final SwiftFormat report comparison — all 28 findings match the base; no introduced violations.
  • ran local xcodebuild test with the unit target, CI integration exclusions, pinned packages, and iOS 26.1 — 2,367 passed with zero failures or skips; simulator diagnostics collection timed out after the tests completed, and the result bundle finalized as Passed.
  • ran minimum-onchain-amount.xml on an iOS 26.1 regtest simulator with a disposable staging issuer — minimum feedback and manual retry verified; a prepared 547-sat confirmation was dismissed without paying.

@greptile-apps

greptile-apps Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

This PR rejects on-chain Paykit requests below 547 sats before payment confirmation.

  • On-chain Paykit requests below 547 sats show the minimum and stay retryable.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Incoming Paykit request] --> B{Lightning can pay?}
  B -->|Yes| C[Lightning confirmation]
  B -->|No or on-chain only| D{Amount below 547 sats?}
  D -->|Yes| E[Show minimum and close preparation]
  E --> F[Keep unpaid request available for retry]
  D -->|No| G[Check balance and continue]
Loading

Reviews (1) · Last reviewed commit: "fix: validate minimum paykit request amo..." · Reviewed by Greptile

@talosmachina talosmachina 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.

No findings. Rejects incoming on-chain Paykit requests under 547 sats with the Amount Too Low toast at both validation points: scan routing in AppViewModel and the deferred check after sync in SendSheet. It also generalises the rejection flag so a below-minimum request is marked presented rather than deferred. Reviewed 932ed17, light tier.

What I checked, and 5 candidates I ruled out

Read: the commit's diff, handleScannedData's .onChain branch, SendSheet's post-sync validation and selectHardwareFundingSourceIfNeeded, showPaymentSetupFailure, Env.dustLimit (547), the four new tests, the journey and README rows.
CI at review time: validate, detect-changes and Greptile green; Run Tests, Run Integration Tests and build-local still running. iOS does not build on this reviewer's host, so this review is reasoned from the code and CI.

Ruled out

  • Off by one / platform drift: amount < 547 here and amount <= 546 on synonymdev/bitkit-android#1457 reject the same set, and the journey file is byte-identical across the two PRs.
  • A path that skips the check: the node-running Lightning fallback goes through validateOnchainBalance, the on-chain fallback has the explicit guard, and the deferred case (channels present but unusable, or node not running) reaches validateOnchainBalanceAndDismissIfInsufficient, which now checks first. testBelowMinimumUnifiedRequestClosesSheetAfterSyncFallback covers the last one.
  • Rejecting a payable Lightning request: the guard reads only the incoming request and runs only on on-chain routes; testLightningRequestBelowOnchainMinimumCanOpenPayment covers it.
  • Rename missing a caller: no reference to didRejectScannedPaymentForInsufficientBalance or insufficientBalance: remains in the tree.
  • Behaviour change from the generalised flag: on any rejection, handleUnavailablePaymentRoute now marks the request presented instead of deferring it. That matches the Android retryIncomingRequest = false cleanup and the PR's intent: no automatic reopen, manual retry still works.

Merge confidence: 4/5, no findings and the change has tests, but the unit and integration runs had not finished at review time.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Reviewed the full PR diff against its merge base, at 932ed17.

No new actionable code findings.

On-chain Paykit requests under 547 sats are rejected during scan routing and again when deferred send preparation falls back to on-chain. The minimum toast uses the existing invoice copy (PaymentRequestAmountTooLowToast, ₿ 547), and the unpaid request stays available for manual retry with the same presentation cleanup as an insufficient balance. Lightning still opens for a request under that amount. The boundary matches bitkit-android#1457 at 43a184ec: iOS rejects amounts below 547 and Android rejects amounts through 546, and both show 547. journeys/payment-requests/minimum-onchain-amount.xml is the same file on both pins. That journey uses an on-chain-only payer; a unified request Lightning can pay opens confirmation, and the minimum check runs when preparation falls back to on-chain.

Validation: Run Tests and Run Integration Tests passed on this revision. This review inspected the new PaykitPaymentRequestServiceTests cases and the journey.

Device testing: not performed in this review.

Ready for device testing.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA tested on iOS at 932ed17c using staging regtest and a dedicated fresh wallet/profile.

Device checks passed:

  • 100- and 546-sat on-chain contact requests show the 547-sat minimum feedback and close preparation.
  • Unpaid requests remain retryable; manual retry repeats the minimum feedback.
  • A fresh 547-sat request reaches enabled confirmation; dismissing it creates no payment.
  • An above-minimum 500,000-sat request shows the genuine balance shortfall.
  • Unpaid state survives app restart, and retry still clears preparation correctly.

Manual follow-up by Piotr: 100-sat unified contact payment requests paid successfully over Lightning in both directions (Android → iOS and iOS → Android). This is user-reported testing, without an agent recording.

Deferred Lightning preparation falling back to on-chain below the minimum remains untested on both platforms. No confirmed product defect was found in the tested scope. Approving as explicitly requested by Piotr with that coverage gap disclosed.

@jvsena42 jvsena42 left a comment

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.

No finding at 932ed17.

Checked and clean:

  • Validated amount = paid amount. Both validation sites (AppViewModel scan routing and the deferred SendSheet fallback) read incomingPaymentRequest.amountSats, the value the sheet pays. A request without an amount skips the check and keeps the zero-amount flow.
  • Every route to on-chain is covered. Node running with no channels, pure on-chain with any node state, channels present but unusable (sync timeout fallback) and node not running (lifecycle fallback) all reach the minimum check before the balance check. Lightning and LNURL routes are untouched, so a Lightning request under 547 sats still opens.
  • Rejection cleanup. didRejectScannedPayment leads to handleUnavailablePaymentRoute(paymentRejected: true), which marks the request presented, clears the retry attempt and deadline, and ends the preparation. The request stays pending, is not auto-presented again and can be retried from the bell, which repeats the same path. Nothing is accepted and the private payment list is not consumed.
  • Cancel and Back. A swipe-down before the timeout goes through the same cleanup as the coded dismissal; finishPayment touches only approved or accepted records.
  • Reentrancy. didRejectScannedPayment is reset at the start of handleScannedData and set synchronously before each return; there is no await between the set and the read in AppScene.
  • Journey. PaymentRequestsBell and PaymentRequestAmountTooLowToast exist at head and the strings match; minimum-onchain-amount.xml is byte-identical to the one in synonymdev/bitkit-android#1457.

CI is green at this head.

Device gate: 932ed17 — journeys: 0 driven by me; perf: not relevant; Figma: no screen changed; Android: same journey file on synonymdev/bitkit-android#1457.
Skipped — already driven this round: minimum-onchain-amount.xml (iOS, by @piotr-iohk at 932ed17, passed: 100, 546, 547 and 500,000 sats, manual retry and restart, in the approving review of 2026-10-12)
Skipped — covered by CI at 932ed17: unit tests, integration tests, e2e (all green)

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.

4 participants