Skip to content

fix: validate minimum paykit request amounts - #1457

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

@ben-kaufman ben-kaufman commented Oct 11, 2026 •

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 before balance checks so sufficient funds cannot produce “0 more needed”; close preparation while keeping the unpaid request available for manual retry.
  • Add a shared 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

  • J1 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 AppViewModelSendFlowTest.kt — five regressions cover below-minimum feedback, delayed preparation cleanup and retry, the payable boundary, genuine balance shortfall, and a small valid Lightning alternative.
  • ran a complete base-versus-final lint SARIF comparison — all 1,981 findings match the base; no introduced violations.
  • ran minimum-onchain-amount.xml on an API 36 regtest emulator 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
Contributor

RetriggerConfidence Score: 5/5

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

Summary

This PR checks the on-chain minimum before reporting a balance shortfall for incoming Paykit requests.

  • On-chain Paykit requests below 547 sats now show the minimum amount.

No actionable issues were found. Tests were inspected, not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Incoming Paykit request] --> B[Choose payment method]
  B --> C{On-chain and below 547 sats?}
  C -->|Yes| D[Show minimum amount]
  D --> E[Close preparation and keep unpaid request]
  E --> F[Manual retry remains available]
  C -->|No| G[Check available funds]
  G --> H[Confirmation or balance feedback]
Loading

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

@github-actions

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 43a184e (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@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 of 546 sats or less with the existing Amount Too Low toast before the balance check, so a funded wallet no longer sees "0 more needed". Reviewed 43a184e, light tier (one 11-line change in AppViewModel.onScanOnchain).

What I checked, and 4 candidates I ruled out

Read: the commit's diff, onScanOnchain around the new guard, validateAmount, Env.dustLimit (546), the five new tests in AppViewModelSendFlowTest.kt, the journey and README rows.
CI at review time: lint, detekt, build-local and Greptile green; build and the local e2e shards still running. This review is reasoned from the code and CI; the branch was not built here.

Ruled out

  • Off by one: amount <= dustLimit with dustLimit = 546 rejects 546 and accepts 547, matching the > dustLimit rule in validateAmount and the 547 shown in the toast. The boundary test covers both sides.
  • Rejecting a payable Lightning request: the guard runs only once payMethod is ONCHAIN, after the unified fallback, and small incoming unified request remains payable over Lightning covers that path.
  • Hardware funding source: a hardware wallet forces ONCHAIN and the guard runs before selectHardwareFundingSourceForAmount, so dust never reaches the hardware send path.
  • Request lost or auto-reopening: clearActiveContactPaymentContext(retryIncomingRequest = false) is the same cleanup the shortfall branch uses. The test keeps the request pending and shows that manual Pay retries it while a new 547-sat request still opens.

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

@ovi-reviewer ovi-reviewer 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.

Suggestion: 👍 Approve

Review: diff 6 files.
Twin: synonymdev/bitkit-ios#913 identical at commit

QA:
Tests queued.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@ovi-reviewer ovi-reviewer 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.

Suggestion: 👍 Approve

Interim test report for the review; the rest resumes on its own.

QA:

🟠 Test J1
Test J1

Incomplete: The lane disconnected during app launch and UI inspection.



Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

This branch has not been deployed

No deployments
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