Repository navigation
fix: confirm fixed Paykit requests directly - #1014
ben-kaufman wants to merge 20 commits into
Conversation
|
Triage: milestone 3.1.0, the iOS twin of synonymdev/bitkit-android#1581. Fixed Paykit requests go straight to confirmation instead of a mostly disabled amount screen. It isn't a release blocker, and it closes no issue, so this PR carries the milestone itself. |
[Medium impact] The changes reach incoming Paykit request preparation and payment confirmation. The PR appears safe to merge, with a non-blocking issue where direct on-chain confirmation ignores the saved coin-selection algorithm. Findings
SummaryFixed quoted Bitcoin requests now bind their exact payment terms before opening confirmation, skipping the locked amount screen when no enabled USDT choice is offered.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Resolve incoming request] --> B{USDT enabled and offered?}
B -->|Yes| C[Choose asset and review amount]
B -->|No| D[Bind exact Bitcoin terms]
C --> E[Confirmation]
D --> E
E --> F[Wait for wallet readiness]
F --> G[Review fee and approve]
G --> H[Recheck request and send]
Reviews (1) · Last reviewed commit: "fix: confirm fixed Paykit requests direc..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P2). Opens a fixed-quoted Bitcoin Paykit request straight on Confirm by preparing the payment terms during scan handling. Reviewed 72d89f3, full tier, reasoned from the code and CI (iOS does not build here).
What I checked, and 3 candidates I ruled out
Read in full: handleScannedData (asset-selection branch), PaymentNavigationHelper.openPaymentSheet / appropriateSendRoute / contactPaymentRoute, PaykitRequestPricing.payment(requested:endpoint:period:at:quoteId:), the incoming-request preparation block in AppScene.swift, SendSheet.shouldShowSyncOverlay and the post-setup validation
Call sites traced: every handleScannedData(_, claimedContactPaymentContext:) caller (AppScene, SubscriptionsView, SendSheet retry, SendContactSelectView, AddContactView, PaymentNavigationHelper)
CI: validate, detect-changes and Greptile green. Run Tests, Integration Tests and build-local were still pending when I reviewed.
Ruled out
- Sync overlay bypassed on Confirm:
shouldShowSyncOverlaynow exempts asset selection only whilecurrentRoot == .amount, so Confirm waits for the node. The renamedtestBelowMinimumQuotedRequestWaitsForSyncBeforeFallbackasserts that. - Stale terms after a wallet switch: the send path rechecks
terms == app.paykitPaymentTermsandisValidbefore paying (SendConfirmationView.swift:1688), andresetSendStateclears both fields (asserted in the new test). - USDT-capable requests losing the asset picker: the block is gated on
!canSelectUsdt, andtestDollarRequestOffersAssetSelectionOnlyWhenUsdtIsEnabledcovers both settings.
Merge confidence: 4/5, one P2 on a narrow path, and the test suites had not finished at review time.
jvsena42
left a comment
There was a problem hiding this comment.
Requesting changes: one MEDIUM (Back from the manual coin picker), inline with a failing test.
Checked and clean:
contactPaymentRoutemaps.quickpayand.amountto Confirm for incoming requests, so QuickPay can't auto-pay a request whose terms are now bound.- Scanner and Enter Manually call
appropriateSendRoutewithout a claimed context, so the terms branch never runs there. - Converted initial subscriptions stay manual (
conversion == nil). returnToAmountReviewIfNeededre-validates terms on a wallet switch.- The rate-unavailable case is fixed by 963d810.
The android twin synonymdev/bitkit-android#1581 prepares coin selection at swipe, so this shape is absent there.
fixed-quoted-confirmation.xml is byte-identical to Android.
Device gate: not run (MEDIUM open).
|
@ovitrif Fixed the Spending/Savings switch in 6c79bae. Already-reviewed Bitcoin requests stay on Confirm with the selected rail’s exact quoted amount, refreshed fees and a required swipe. Missing/expired terms and amounts below the Lightning minimum return to Amount; BTC/USDT selection and hardware safeguards remain. The hosted test switches between 2,500 and 5,000 sats and verifies fee preparation, with separate fallback cases. The journey now matches Android and also covers Back from manual coin selection. All 47 focused tests pass; SwiftFormat and translation validation passed with no introduced diagnostics. |
jvsena42
left a comment
There was a problem hiding this comment.
The MEDIUM is fixed at 6c79bae6. Back from the manual coin picker now stays on Confirm, and manual selection still runs before paying. I resolved the thread.
Wallet-switch rebind in the same commit, clean:
- It needs USDT off, no hardware wallet, not submitting, the previous BTC terms equal to
paykitReviewedAmount, valid new terms andmeetsMinimum. - It sets
paykitPaymentTerms,paykitReviewedAmountandsendAmountSatstogether. acceptsPaymentAmountandacceptsLightningInvoiceAmountstill re-check at pay time.- Unavailable or expired terms, or an amount below the minimum, return to Amount.
fixed-quoted-confirmation.xml is byte-identical to synonymdev/bitkit-android#1581.
Device gate (partial): not run yet. Pending: payment-requests/fixed-quoted-confirmation.xml, cross-device with synonymdev/bitkit-android#1581, plus Back from the manual coin picker on device. Code scan: no HIGH/MEDIUM open. The approval follows the device run.
|
@ben-kaufman thanks, rechecked at 6c79bae. On a Spending ↔ Savings switch, |
|
Fixed the Xcode 16.4 type-checking failure in The custom-fee lifecycle test also now allows SwiftUI to cancel and restart preparation during navigation. It still requires exactly one successful preparation, rejects cancelled results, and checks the final manual-UTXO or confirmation route. Validation: local simulator build and all 50 confirmation, swipe, lifecycle and pricing tests passed; SwiftFormat passed on all eight PR Swift files. Translation validation has no errors and no added warnings compared with master. Every PR commit verifies locally and on GitHub. Local validation used Xcode 27; the Xcode 16.4 CI rerun is pending. The cross-device journey remains a separate reviewer device gate. |
|
The hosted Xcode 16.4 compiler still timed out on the remaining confirmation modifier chain. In 05beeb5 I split that expression into smaller private computed views. Inlining them reproduces the original expression byte-for-byte, preserving task IDs, modifier order, state and payment safeguards. All 50 focused confirmation, lifecycle and pricing tests pass locally. SwiftFormat is clean for all eight changed Swift files; full translation/compiler diagnostics contain no findings introduced by this fix. Independent review is clean, and all six PR commits are verified. Reviews are re-requested; the new hosted CI run still needs to confirm the Xcode 16.4 build. |
jvsena42
left a comment
There was a problem hiding this comment.
The delta since my last review, b58c196, is clean. It only moves code: confirmationContent → confirmationLayout with the same modifier chain in the same order, so payment and authorization behaviour is unchanged. The testCustomFeeWalletSwitchSurvivesFeeScreenNavigation relaxation (successfulCoinPreparations == 1) is safe, because cancelled preparations throw before they assign anything.
CI is red because of this change, so I'm holding the approval. Unit tests on Xcode 16.4 (run 39308712256) still stop at the compiler is unable to type-check this expression in reasonable time, now at confirmationLayout (SendConfirmationView.swift:393), so no test ran. The ~15-modifier chain is still one expression. Splitting the onChange/.task group into its own computed property or a ViewModifier should get it under the limit.
Device gate (partial): not run yet. Pending: payment-requests/fixed-quoted-confirmation.xml cross-device with synonymdev/bitkit-android#1581, and Back from the manual coin picker.
|
@jvsena42 The latest commit |
|
Resolved the conflicts in signed normal merge Validation on the final combined tree: confirmation lifecycle 15/15; canonical 2,894 unit executions and 39 UI passes, four fixture skips, zero failures. Full SwiftFormat and translation checks passed with the 15 verified translation warnings retained. Complete structured/raw diagnostics and independent review were inspected; 13 runtime QoS warnings and two AppIntents notices remain documented in the PR body. All 13 current-base commits verify locally and on GitHub ( Fresh hosted checks are running. The unavailable fork skipped before funding; rendered quoted-boundary, funded/provider, physical-biometric, hardware and two-wallet settlement gaps remain open. The previous funded-fixture failure is not erased by this pass. Requesting affected Piotr/Ovi re-review and preserving João's existing request; no review is dismissed or treated as approval. |
|
Published signed normal master integration Validation: 15 focused tests; 2,928 unit and 39 UI executions passed, four fixture skips and zero failures. Full formatting passed; translations had zero errors and 15 verified base-identical warnings. Full structured/raw diagnostics were inspected and retained. All 14 current-base PR commits verify locally and on GitHub. New hosted checks need completion review; funded/provider/hardware/two-wallet acceptance remains incomplete. The three existing human requests are still pending and preserved, so no duplicate re-request was sent. Existing changes-request state remains in force. |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 9a807ef after the comparison base moved to b666ccb1. Merge base is b666ccb. No inherited coverage baseline was supplied.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Valid fixed Bitcoin quotes bind their terms and open confirmation directly. Missing or expired terms, USDT choice, and LNURL plus Savings remain on Amount. Switching rails rebinds that rail’s quote, and a Savings quote below 547 sats leaves confirmation. bitkit-android ea89ed1 keeps a single confirmation state and the same 547-sat boundary. Its confirmation can change between LNURL and Savings; iOS keeps that choice on Amount, as the journey states.
Device testing: not performed in this review. This environment has no Xcode, so the added confirmation and navigation tests were inspected and not executed. The manual-coin back path below should be checked before device testing.
Suggested additional test cases
- iOS simulator, manual coin selection, a funded wallet, and a fixed quoted Savings request. Open the request, select coins, continue, go back to the picker, then go back to the first confirmation and show details. Expected: the fee for the selected rate is visible, or Swipe to Pay stays disabled until that fee is calculated.
Earlier findings reconciled at this revision: 1 verified fixed.
Findings
- [LOW] Manual coin selection leaves a swipeable confirmation with no fee — inline at
Bitkit/Views/Wallets/Send/SendConfirmationView.swift:1797.
Reviewer model: grok-4.7
Reasoning effort: high
jvsena42
left a comment
There was a problem hiding this comment.
Code scan of 9acc68e (the only new PR commit since 7dbb5e0, besides master merges): one LOW inline (Back from the fallback Amount pushes Amount again). No HIGH or MEDIUM.
Checked and clean:
- The Savings minimum uses the per-rail quoted amount (
request.payment(using:quoteId:)), the same value written topaykitPaymentTermsandsendAmountSatsand re-checked for equality at pay time. - Every entry to a swipeable Confirm goes through it: direct open, wallet switch, hardware and the funding fallback. 547 sats is accepted and 546 refused, matching Amount and scan validation. USDT never reaches this view.
- The merge resolutions in
SendConfirmationView(63ea203) andSendSheet(ad30f8b) keep the PR's manual-coin and Amount-root handling alongside master'sstartWalletSwitch; later merges touch tests and journeys only. - synonymdev/bitkit-android#1638 applies the same floor (
SendConfirmScreen.kt:500disables the swipe at 546 and below), so the journey text holds on both.
Device gate (partial): 9a807ef — not run yet; still pending payment-requests/fixed-quoted-confirmation.xml cross-device with the Android twin, and paykit-quoted-usdt.xml. Device results will follow in a separate review.
|
Fixed the unit CI crash in signed commit 20f5444. The crash report showed an assertion reading the watch-only fake’s tracking Set concurrently with its insertion. Both affected tests now wait for the existing actor-isolated gate after tracking completes; all assertions and all 35 tests remain, with no production change. Validation: 35 focused tests passed; the required canonical selection passed 2,928 unit and 39 UI executions, with four explicit fixture skips and zero failures. Full SwiftFormat 0.64.0 passed; complete translation/test/runtime diagnostics and independent review were inspected. All 16 commits against actual master b666ccb verify locally and match GitHub’s exact set with valid signatures. New-head CI is pending; no manual CI retry. Parent811b’s failed unit run and successful E2E retain their exact attribution in the updated body. Existing João, Piotr and Ovi requests remain pending, so no duplicate request was sent. João’s changes request and the fee-refresh thread remain in force. Funded/provider/physical-hardware/two-wallet/rendered-boundary acceptance gaps are unchanged. |
|
Published signed normal merge The prior |
There was a problem hiding this comment.
Verdict:
Findings: No new product defect verified. The Savings minimum and reported fallback Back sequence hold; I could not reproduce the missing manual-selection fee.
QA for b53e300:
Tested b53e300 on an iPhone 17 Pro simulator, iOS 26.5, built with Xcode 27. Used a linked controlled Paykit issuer and regtest funds. Videos play at 2× speed; no payment was authorized.
Test 1: J1 — direct confirmation and manual coin selection ✅ The fixed quote confirmed at 5,000 sats; Largest First showed a 143-sat fee, and the original Confirm displayed 212 sats after selecting both coins and backing through the picker.
The device log confirms Largest First selected the 500,000-sat output. Manual-picker Back stayed on Confirm, and the next swipe required selection. Selecting the 500,000- and 200,000-sat coins produced the displayed 212-sat fee on both confirmations.
| Direct confirmation | Manual selection and Back |
|---|---|
J1-largest-first.2x.mp4 |
J1-manual-return-v3.2x.mp4 |
Test 2: J2 — 546-sat Savings fallback ✅ Switching from 2,500-sat Spending to 546-sat Savings opened Amount with Continue disabled; Back closed the sheet and the unpaid request reopened.
Accessibility confirmed ContinueAmount was disabled. Savings and Spending stayed at 700,000 and 500,000 sats.
J2-546-v3.2x.mp4
Test 3: J2 — exact 547-sat Savings boundary ✅ Switching to Savings retained Confirm at exactly 547 sats with a refreshed 143-sat fee and a required swipe.
J2-547.2x.mp4
Test 4: J1 — cancel during preparation ✅ Closing while the requested USD amount and disabled loading controls were visible prevented late reopening or payment; the same request reopened successfully.
J1-cancel-v3.2x.mp4
Test 5: J1 — LNURL and Savings choices ✅ Both wallets stayed available: Spending confirmed 2,500 sats, Savings confirmed 5,000 sats with its 143-sat fee, and LNURL-only confirmed directly.
| LNURL and Savings | LNURL only |
|---|---|
J1-lnurl-v2.2x.mp4 |
J1-lnurl-only-v2.2x.mp4 |
Test 6: Quote and send lifecycle regressions ❌ The focused exact-head run passed 66 tests but failed the initially-unreviewed fallback Back assertion; an isolated rerun reproduced it.
SendConfirmationLifecycleTests.testQuotedWalletSwitchRequiresReviewForUnavailableExpiredOrBelowMinimumTerms fails at BitkitTests/SendConfirmationLifecycleTests.swift:141: the synthetic Confirm-root host still has an active send sheet after Back for unreviewed. The unavailable, expired and both minimum variants passed. Pricing, routing, manual-fee preparation, LNURL and send-sheet coverage otherwise passed.
Waiting for Amount to appear and then rendering the real Amount view in separate local test-only diagnostics did not clear this assertion. Production code stayed unchanged. Normal routing sends initially unreviewed terms directly to Amount; I have not established a matching user-visible failure. Please recheck this regression on iOS 26.5 / Xcode 27. The live rail-switch fallback and Back behavior passed in Test 2.
The recording shows the hosted test run; these results come from XCTest assertions with controlled request and wallet state.
hosted-regressions.2x.mp4
I retained the earlier passing device results for unchanged insufficient-funds, deadline-expiry and BTC/USDT asset-choice behavior. USDT funding/submission, subscription authorization and hardware signing were not driven; these confirmation journeys stop before payment.
Twin: synonymdev/bitkit-android#1581 fix: confirm fixed Paykit requests directly matches fixed Bitcoin quotes and wallet availability. iOS keeps LNURL/Savings selection on Amount; Android offers it on Confirm. The shared wording follow-up remains synonymdev/bitkit-android#1610 Align fixed-quote journeys with both payment flows.
Delta review: ad30f8b..b53e300, including prior open findings and payment-path merge integration. Unrelated master features received no separate device cases.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Follow-up to QA review 5670593898 at unchanged I’m keeping the reported The reported rendered manual-fee, rail-switch fallback and exact-minimum results are retained as reviewer-reported evidence. The fee thread is now externally resolved; no payment/settlement or human approval is inferred. Independent iOS 26.5 reproduction is pending because local free disk is below the established build reserve. No source or assertion change, test retry, device reset or fixture operation has been made. Please attach the complete failing |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to b53e300. My LOW on SendConfirmationView.swift:1766 is fixed for the header Back, and I have no new findings.
811b433closes the sheet from the fallback Amount throughhasShownAmountReviewandhideSheetIfActive(presentationID:). The hide branch writes no terms and no amount.- The amount paid still can't differ from the amount shown:
validateIncomingPaymentRequestAmounts→acceptsPaymentAmountstill pins the live quote andquoteIdtowallet.sendAmountSats.
One path needs a simulator check before I call it a finding: swiping back from the left edge on the fallback Amount. UINavigationController+SwipeBack enables the gesture there. During an interactive pop the root .task runs before the path binding is written, so the hide branch may be skipped. Confirm would then stay open with nil terms and the old rail's amount, and swiping to pay would fail with amountMismatch, so no funds are at risk. If the device shows it, deciding the exit in SendAmountView's onBack and adding .allowSwipeBack(false) on that pushed Amount covers both gestures.
Device gate (partial): b53e300, not run yet. Pending: fixed-quoted-confirmation.xml steps 3 and 9, cross-device with synonymdev/bitkit-android#1638; paykit-quoted-usdt.xml; and the swipe-back check above.
|
Published 20cfeff for the initially-unreviewed Back regression reported in review 5670593898. I reproduced the unchanged b53e300 assertion on Xcode 27.1/iOS 26.5; the four-line navigation observer now reuses the existing invalid-terms dismissal guard when Amount pops to the confirmation root. All 15 unchanged lifecycle tests pass. No assertion or payment guard was weakened. Final canonical validation: 2,932 unit and 39 UI executions passed, four explicit fixture skips, zero failures. Full SwiftFormat passed; translation validation has zero errors and 15 individually verified base warnings. Complete diagnostics and independent correctness/quality review are retained. All 18 current-base commits verify locally and on GitHub. Local tree e286b0d9 is the repaired PR tree; the newer combined master543d tree is not covered. Fresh hosted checks are pending. The supplementary real-Amount/header-Back check closed the owned sheet state without authorization or submission. Interactive Back remains unverified: after one readiness-harness correction, two delivered edge swipes did not pop the temporary test window and its bounded diagnostic failed on timeout. Both diagnostic failures, screenshots and full reports are retained; the temporary test source was restored exactly before signing. This does not establish the interactive-pop root cause or full linked-request/reopen/funded acceptance. The existing lifecycle regression is fixed, while those acceptance limits remain explicit in the PR body. Existing Piotr/Ovi requests and João's changes request are preserved. No CI job was retried. |
Dismissed per @ovitrif: this review was on an older head, its threads are resolved, and the PR has moved on. @jvsena42 is off until 1 Nov; the current head needs a fresh review from @ben-kaufman or @piotr-iohk.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — the comma-region journey fails; interactive Back remains unverified.
Findings: No new defect established in the delta. The comma-region failure below also affects code unchanged from the base.
QA for 20cfeff:
Tested on an iPhone 17 Pro simulator with iOS 26.5 and an Android emulator. Videos play at 2× speed; no payment was authorized.
Test 1: Initially unreviewed fallback Back ✅ The focused lifecycle regression passed.
The XCTest result confirms the previously failing initially-unreviewed assertion and the invalid-terms variants.
hosted.2x.mp4
Test 2: Android-created fixed request ✅ A $2.50 Savings request opened and reopened on iOS confirmation at 3,097 sats.
Android showed the request as waiting for payment; iOS showed a 143-sat fee before closing. This checks both apps through confirmation, not settlement.
cross-android.2x.mp4
cross-confirm.2x.mp4
Test 3: 546-sat fallback and header Back ✅ Switching from Spending to 546-sat Savings opened Amount; header Back closed the sheet and the unpaid request reopened.
edge-final.2x.mp4
Test 4: 547-sat boundary ✅ Switching to the 547-sat Savings quote retained confirmation and showed the fee.
boundary547.2x.mp4
Test 5: LNURL and Savings ✅ The mixed request showed 2,500 sats for Spending and 5,000 for Savings, each with its confirmation; the LNURL-only request opened confirmation directly.
The Savings confirmation showed a 143-sat fee. Both cases stopped before payment.
lnurl-rails.2x.mp4
lnurl-only.2x.mp4
Test 6: Insufficient Savings ✅ A 4,000,000-sat quote was rejected with an insufficient-balance message.
balance.2x.mp4
Test 7: BTC and USDT choice ✅ An uncovered $2.50 USDT quote defaulted to Savings with USDT selectable; after funding, it defaulted to USDT.
mixed.2x.mp4
mixed-funded.2x.mp4
Test 8: Private USDT-only contact ✅ The amount screen offered only USDT and labeled the balance AVAILABLE.
The controlled contact published a private USDT-only payment list; the device log confirmed the private list was opened.
locale-contact.2x.mp4
Test 9: USDT review details ✅ The long invoice note and fee note remained separate while the payment control stayed visible during scrolling.
quote-short.2x.mp4
Test 10: Comma-region amount entry ❌ The journey expects commas, but request creation showed a dot and 150.50; direct USDT Send, Receive and contact entry showed a dot and 5.5.
The simulator's nl_BE locale and comma decimal separator were checked independently. NumberPad and the request-creation call site are unchanged from the base, so this is not attributed to the reviewed delta.
locale.2x.mp4
locale-receive.2x.mp4
locale-send.2x.mp4
locale-contact.2x.mp4
Test 11: Interactive fallback Back ⏭️ Completed and cancelled interactive transitions remain unverified.
Two edge gestures on the fallback Amount screen did not initiate a visible transition. The same automation also failed to transition on a separate settings navigation screen, so this does not establish an app defect. Header Back is covered by Test 3.
edge-final.2x.mp4
control.2x.mp4
Test 12: Expired request retry ⏭️ The request disappeared from confirmation at expiry without payment; fresh-terms review on retry remains unverified.
expiry.2x.mp4
Test 13: Remaining J1 acceptance ⏭️ Full provider, hardware and settlement acceptance was not exercised.
The device cases above stopped before payment. Earlier confirmation, manual-coin, cancellation and 546/547 checks are retained from the prior review.
Test 14: Remaining J2 acceptance ⏭️ The full subscription, six-decimal edit and counterparty checks remain incomplete.
Four Savings–USDT switches restored 12,345 sats on the proposer; payer-side subscription acceptance was not checked.
subscription.2x.mp4
Reviewed the delta from b53e300 to 20cfeff and the Back behavior reported in the prior review. The pair PR, synonymdev/bitkit-android#1581, implements the same direct-confirmation goal with the documented LNURL funding-choice difference; the cross-device request above used synonymdev/bitkit-android#1638.
Reviewed by Codex via gh-pr-review-loop (model and reasoning level not exposed).
|
Addressed the stale comma-decimal expectations from review 5676643246 in 9dacc08. J2 now expects the existing iOS dot-decimal convention while keeping comma-region coverage, all 19 actions, exact values, trailing zeros, six-decimal precision and serialized-value checks. The journey description and README identify the iOS convention. This matches Validation: XML parsing/action comparison, references and The review’s results remain attributed to |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — a six-decimal USDT request is rounded on the payment review; some acceptance cases remain unverified.
Findings: No new defect found in the reviewed documentation delta. The six-decimal display failure below is in code unchanged from the base.
QA for 9dacc08: Built and tested on an iPhone simulator with iOS 26.5, with the app launched in a comma-decimal locale. Test funds only; no payment was authorized. Videos play at 2× speed.
Test 1: USDT Receive in comma region ✅ Decimal key is a dot; 5.5 is visible.
locale-receive.2x.mp4
Test 2: USDT Send in comma region ✅ Decimal key is a dot; 5.5 is visible.
locale-send.2x.mp4
Test 3: USDT contact payment in comma region ✅ Decimal key is a dot; 5.5 is visible.
locale-contact.2x.mp4
Test 4: USD request creation in comma region ✅ 150.50 appears in the editor, review and recipient row; editing retains it.
Editing retention was checked in the accessibility snapshot.
request.2x.mp4
Test 5: USD request with trailing separator ✅ 5 followed by the decimal separator permits continuing to a 5.00 review.
trailing.2x.mp4
Test 6: USD subscription counterparty ✅ A linked payer sees a 150.50 monthly proposal and USDT-only review.
subscription-payer.2x.mp4
Test 7: Quoted USDT amount through provider failure ✅ 150.50 remains visible on the quote error and review succeeds after recovery.
Error captured in screenshot; recovery checked in accessibility snapshot.
unavailable.2x.mp4
Test 8: Six-decimal USDT request through review ❌ 0.123456 appears as 0.12 on the payment review.
No payment was authorized; the display code is unchanged from base.
six.2x.mp4
Test 9: Comma-region decimal journey ✅ The corrected dot-decimal entry and request values were observed.
Aggregate of the separately recorded decimal cases.
locale-receive.2x.mp4
locale-send.2x.mp4
locale-contact.2x.mp4
request.2x.mp4
Test 10: Interactive fallback Back ⏭️ Could not establish a cancelled or completed interactive Back transition.
Attempted edge drags and a preset swipe on a funded regtest fallback; no interactive transition started.
edge.2x.mp4
Test 11: Expired request retry ⏭️ Expiry removed the pending action, but fresh-term retry was not completed.
A short-lived regtest request expired; the flow did not reach a fresh authorization review.
expiry.2x.mp4
Test 12: Remaining J1 acceptance ⏭️ A linked payer received the 150.50 request; remaining acceptance steps are incomplete.
The linked request was driven through the payer amount screen; the full remaining settlement and hardware coverage was not completed.
linked-request.2x.mp4
Test 13: Remaining J2 acceptance ⏭️ The linked subscription and six-decimal review were exercised; remaining exact-value serialization is unverified.
No completed end-to-end serialized amount or settlement check.
subscription-payer.2x.mp4
six.2x.mp4
The six-decimal request showed 0.123456 on the amount screen, then 0.12 on review and in payment details. I did not submit it, so this checks the confirmation display only. The same formatting path exists at the base; this delta does not introduce it.
Earlier passing cases are carried forward from the previous review and earlier review, since the subsequent changes did not touch those behaviors. Reviewed the delta from 20cfeff to 9dacc08, which updates journey documentation; the corrected locale examples were driven above.
The paired Android PR #1581 has the same direct-confirmation goal, with its documented LNURL funding-choice difference.
Reviewed by Codex via gh-pr-review-loop (model and reasoning level not exposed).
Reviewed by gpt-6.1-terra-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to 9dacc08. No new findings, and 20cfeff fixes the swipe-back path I raised in the code. The new .onChange(of: navigationPath) (SendConfirmationView.swift:454-457) fires on the committed [.amount] → [] write whatever the root .task timing, and reaches the hide branch (:1771-1773). A second hit is a no-op through hideSheetIfActive(presentationID:). The amount paid is still pinned to wallet.sendAmountSats (SendSheet.swift:1218-1224).
Device gate (partial): 9dacc08, not run yet. Pending: fixed-quoted-confirmation.xml including the edge swipe on the 546-sat Savings fallback Amount, and paykit-quoted-usdt.xml.
|
The six-decimal display finding in review 5678141014 is fixed in The rendered red test failed ten precision assertions; the fix passes all ten focused tests, with saved native screenshots showing This is native static rendering coverage of the production builders, not a completed linked-request or funded journey. The test's constant-binding warning is disclosed because it bypasses the full view lifecycle. Native Back and fresh-terms retry remain open acceptance gaps. The fixture prerequisite is now explicit in the existing J1/J2 instructions, with stable case IDs. Your device results retain their original head/environment attribution; no payment, review dismissal or duplicate reviewer notification was made. New-head unit, integration and E2E checks are pending; the completed parent-head E2E result remains separately attributed in the PR description. |
There was a problem hiding this comment.
Verdict:
Findings: No new defect found in the delta.
QA for 629a874: Tested with linked test wallets on iOS 26.5 and a local USDT fixture. Videos play at 2× speed; no payment or subscription was authorized.
Test 1: Six-decimal request ✅ The incoming row, payment review and details showed 0.123456 USDT.
six.2x.mp4
Test 2: One-micro USDT request ✅ The incoming row, payment review and details showed 0.000001 USDT.
The details were also checked in the screenshot.
micro.2x.mp4
Test 3: Six-decimal subscription ✅ The proposal row and subscription review showed 0.123456 USDT.
sub.2x.mp4
Test 4: Savings-only subscription ✅ A linked payer saw the 12,345-sat proposal with Savings as its only payment method.
The proposer is on the left and payer on the right.
pair.2x.mp4
Test 5: Interactive fallback Back ⏭️ Switching from Spending to the 546-sat Savings quote opened Amount, but neither edge gesture started a visible transition.
A preset gesture and a full edge drag did not establish either a completed or cancelled transition. The prior header Back check remains applicable.
gesture.2x.mp4
Test 6: Expired request retry ⏭️ The request closed at its deadline and disappeared from pending without a payment.
No Pay action remained to exercise fresh-terms review on retry.
expiry.2x.mp4
Test 7: Six-decimal editing and serialization ⏭️ Editing fixture-provided USDT terms and checking the serialized result remain unverified.
The iOS editor creates BTC or USD terms, and the incoming fixture terms were not editable.
Test 8: Remaining acceptance ⏭️ Full provider, hardware and settlement acceptance remains incomplete.
The fixture cases above stopped before authorization; hardware and settlement were not driven.
Check 1: Exact display regression ✅ The two focused tests passed at this head.
The tests include static rendering of production confirmation and details builders for 0.123456 and 0.000001, including the exact bridge recipient, fee and total. They do not exercise the full navigation or payment lifecycle.
The earlier 9dacc08 review and 20cfeff review retain their passing evidence for behavior untouched by this delta. The former six-decimal display failure is fixed in the live request checks above.
Reviewed the delta from 9dacc08 to 629a874. The bridge recipient and maximum-total changes were covered by the static regression, not a live bridge journey. The paired synonymdev/bitkit-android#1581 matches the direct-confirmation goal, with the documented LNURL funding-choice difference.
Reviewer model: gpt-6.1-terra
Reasoning effort: high
Reviewed by gpt-6.1-terra-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
Exact USDT review amounts —
629a874e(October 28): Fixed the six-decimal display finding from review 5678141014. The actual payer confirmation header and Show Details payment, fee, recipient amount and maximum total now use the existing exact USDT formatter. Request/subscription amount text also preserves all six decimals. Quote values, serialized amounts and payment authorization are unchanged; balance overviews retain their established formatting.The rendered regression reproduced
$0.12/$<0.01for0.123456/0.000001before the fix (one failed test, ten assertions). All ten focused amount/display tests now pass. Saved native screenshots show exact headers, fee0.000001, and totals0.123457/0.000002. These tests host the real production review builders directly and verify static rendering; they bypass full view lifecycle/navigation/quote loading and do not authorize or send a payment. Their new SwiftUI constant-binding warning is retained as an explicit test boundary, not a production-race or warning-free claim.Final canonical local validation passed 2,934 unit and 39 UI executions, with four documented fixture skips and zero failures or retries. Complete diagnostics were inspected: two AppIntents warnings, ten structured QoS warnings plus the new static-builder State warning, eleven raw QoS backtraces, and 72 NaN notices remain explicitly retained alongside symbolication and simulator notices; this is not a warning-free claim. Full SwiftFormat 0.64.0: 0/846 files require formatting, 83 skipped. Translation validation: zero errors, 15 warnings matching the verified actual-base warning multiset. Two independent source perspectives covered the component and actual payer path; an initial component-only coverage gap was caught before publication and the actual payer regression added. Required source signatures: all 20 commits in the current-base PR range verified locally and matched the exact GitHub set with
verified=true/reason=valid. Validation applies to local PR tree88d9c2e3295b93490a3395510ed3605f1ce1b565, not a new combined master tree. Master68bede62added only disjoint profile journey documentation since the earlier base; no unrelated integration was performed.J1/J2 retain their IDs. Their six-decimal request case now explicitly needs fixture-provided USDT terms, because the iOS editor creates BTC/USD terms. Full linked-request, native Back, fresh-terms retry, provider/funded/hardware/settlement acceptance remains incomplete. No pending human request was recreated; João's announced absence through November 1 is preserved. Current work: implementation
gpt-6-astra/xhigh, scoped independent reviews and diagnostic auditgpt-6.1-sol/high, verified from execution metadata.Hosted checks for
629a874eare complete on workflow attempt 1: unit passed 2,960 unit and 39 UI executions, with four fixture skips and no failures; integration passed 30 tests once each, with four existing Blocktank staging skips. Verified checkouta52bbce2/ treeefec63fbcombines actual master68bede62with629a874e; this differs from local PR tree88d9c2e3. Complete logs, artifact digests, execution trees, case summaries and nested diagnostics were inspected. The payer display regression passed in 11.611 seconds. Its two introduced State/constant-Binding messages remain explicit: direct builder hosting proves static text, not the full view lifecycle or authorization flow. The 286 structured / 571 raw duplicate-symbol warnings match the verified prior warning multisets. Runtime/mock/fixture/tooling notices, symbolication limits, and two passing integration tests' internal sync/deposit recoveries remain recorded; no fresh-base runtime or warning-free claim.E2E ultimately passed 13 unique cases across 14 executions: 12 passed initially, while Lightning timed out after 30 seconds waiting for
ActivityShort-0to show9 000after restored-channel closure. The selective second built-in step passed Lightning; step 3 was skipped. One existing hardware-wallet case remained disabled. This was not a first-pass clean run or a manual retry. Failure screenshots/video were generated on the runner, but their upload was skipped after recovery; no downloadable failed visuals, device/app logs or runtime report were available. Root cause remains unproven, with no actual-base reproduction or permanent-fix claim. All five job logs, pinned harness4c0cbef0, cases and app-artifact provenance were inspected. The 76 stale-element notices, five log-stream restarts and pinned-harness dependency findings remain explicit. Formatter and translations were already inspected: 0/850 files needed formatting (87 skipped); zero translation errors and 15 verified base-matching warnings.Automated reviewer QA at this head reports exact six-decimal and one-micro request displays, a six-decimal subscription, and Savings-only proposal selection using linked test wallets and a local fixture, without authorization. This is reviewer-reported evidence, not an independent rerun or human approval. Neither edge gesture established an interactive Back transition; expiry removed the request before fresh-terms retry could be exercised. Editing/serialization, live bridge, provider, hardware and settlement gaps remain. No new delta defect was reported. Shared-navigation ownership coordination and the existing remaining acceptance gates are still required.
Current documentation correction —
9dacc089(October 28): Corrected stale J2 comma-decimal expectations to the existing iOS dot-decimal convention, including comma-region settings. All 19 actions and exact-value, trailing-zero, six-decimal and serialized-value checks remain. No app behavior changed. XML/action/reference checks,git diff --check, full SwiftFormat 0.64.0 (0/846), and one independent documentation review passed. All 19 current-base commits verify locally and on GitHub. App tests were not rerun for this documentation-only delta; earlier local/hosted results keep their original inputs. Automated review 5676643246 reports additional partial device evidence at20cfeffa, not human approval or independent execution by this monitor. Native Back, expiry retry and full J1/J2 acceptance remain incomplete. The former comma-expectation result is preserved as a failure of the earlier journey wording, not converted into a completed new journey run.Current repair —
20cfeffa(October 27): The initially-unreviewed Back assertion reported in automated review 5670593898 reproduced independently on unchangedb53e300ewith Xcode 27.1/iOS 26.5. The four-line fix observes Amount being popped to an empty navigation path and reuses the existing invalid-terms, presentation-owned dismissal guard. All 15 unchanged lifecycle tests now pass. No assertion or production payment guard was weakened; the journey also names interactive Back as an acceptance check.Final canonical validation on Xcode 26.1/iOS 26.1 passed 2,932 unit executions and 39 UI executions, four fixture skips, zero failures, including all 35 watch-only tests. Full SwiftFormat 0.64 passed (0/846; 83 exclusions); translations report zero errors and 15 warnings whose individual call/missing-language sets match actual master
543d9389. Complete structured/action/raw diagnostics and an independent frozen correctness/quality review were inspected. Eight aggregate QoS warning cases represent 19 warning-bearing executions; three raw Paykit/pubky initialization backtraces, two AppIntents build notices, simulator/negative-case/runtime messages and the unavailable-fork skip remain explicit. All 18 current-base commits verify locally and match GitHub's exactverified=true,reason=validset. Tested local PR tree:e286b0d9; the new hosted unit and integration results below cover the combined tree. No warning-free or fresh-master-runtime claim.Hosted checks at
20cfeffa: Unit 40559037614 and integration 40559037349 passed on workflow attempt 1. Unit: 2,953 unit + 39 UI executions passed, four fixture skips, zero failures, including all 15 confirmation lifecycle and 35 watch-only tests. The repaired initially-unreviewed Back regression passed. Integration: 30 unique passes in 30 executions, four existing Blocktank staging skips, zero failures or test-case reruns. Both checked out verified merged1ec3668(543d9389/20cfeffa), tree370b70b7, which differs from the local PR tree; 21 additional upstream unit cases account for the count difference. Full archive hashes, test trees, all UI/integration case summaries, build/action/runtime diagnostics and job logs were inspected. The three Trezor opt-in cases and isolated Arbitrum case remain skipped; integration retains the four staging skips.The 286 structured and 571 build-log duplicate-symbol warning multisets match the previously inspected hosted result. Runtime output still includes the keyboard queue timeout with
Last Exception Backtrace: No stack!, socket/haptic/launch notices, injected negative-case diagnostics, integration RGS/node/address-search/teardown messages and the version-probe broken-pipe exception. Associated source comparisons are retained; this is not a warning-free run or a fresh actual-base runtime reproduction. E2E 40559038210 also passed on workflow attempt 1: 13 unique passes in 13 executions, one existing hardware-transfer skip, zero failures. The first built-in test step passed; steps 2/3 were skipped. Build and runtime used the same verifiedd1ec3668/370b70b7combined tree. The executed harness was4c0cbef0(two commits beyond the previous harness), on iPhone 17/iOS 26.2. The exact pinned hardware spec retains its Blocktank-staging skip. All five full logs, case outcomes and artifact provenance were inspected. Artifact12761095558SHA-256b59616212d303b5441c1053ac883fbd32ba4af1593cb9c98e93c85816accc1a6matches API/upload/runner download; no local ZIP or runtime-report/video artifact was inspected. Retained diagnostics include 66 stale-element notices, four log-stream restarts, an address-type tap helper retry that reached its expected toast, Node/npm/tool/AppIntents notices and 76 pinned-harness dependency findings. No manual CI retry or warning-free claim. These hosted passes do not close the supplementary native gesture or other acceptance gaps below.Supplementary native check remains partial: an unpublished animated test host used real Amount with fixed-quote terms that were valid at a historical time and expired before appearance. Header Back closed the owned sheet state with an empty path and no preparation, authorization or submission. The first diagnostic execution failed an exclusive-visible-route readiness assumption and timed out before an edge gesture was delivered. One bounded rerun corrected only that temporary readiness check; actual Amount/path assertions passed, but two edge swipes did not pop Amount and the diagnostic timed out. Both failures and full diagnostics/screenshots are retained. The cause is unproven; completed/cancelled interactive Back, real linked-request setup, dismissal animation and reopening are not accepted by this check. Temporary test code was restored byte-for-byte before signing. No funds, payment, wallet reset or shared-fork access occurred. The repaired existing lifecycle regression is green; this additional gesture gap is not presented as a pass.
A later bounded prerequisite check on the same isolated iOS 26.5 simulator found two native controllers, an enabled recognizer,
allowSwipeBack=trueand a key window. The temporary host's 390×844 window differed from the 402×874 screen. After correcting only that host geometry and observing native transition state, one cancel-shaped edge drag and one full edge swipe both produced recognizerfailedstate before any interactive transition. The 100-second diagnostic ended with one failed case/four assertions: Amount/path and sheet state remained, no completed or cancelled pop occurred, and routing/preparation counts stayed zero. The geometry change did not resolve the gap; the reason for gesture rejection is still unproven, and no production defect or fix is inferred. Complete xcresults, action logs, state trace and screenshots are retained. Temporary code was restored byte-for-byte; published source remains20cfeffa. Further gesture acceptance needs the production sheet presentation to reach that boundary, rather than another unchanged synthetic-host retry.At
b53e300e, the bot reported rendered confirmation, saved/manual coin selection (143/212-sat fees), manual-picker Back, 546-sat fallback/Back/reopen, 547-sat confirmation, cancellation during preparation and LNURL/Savings selection, stopping before payment. Those remain reviewer-reported partial J1/J2 results at that head. Piotr's fee-refresh thread is externally resolved. On October 27, Ovi dismissed João's older changes request, noting its resolved threads and João's absence until November 1; this monitor did not dismiss it. Piotr/Ovi requests remain pending, João's comment retains assignment and his absence is respected, and there are zero current-head human approvals. A bot comment or resolved thread is not approval. Current shipped/provider/funded/physical-hardware/full-linked-request/two-wallet settlement acceptance remains incomplete.Earlier validation —
b53e300e(October 27): The conflict with then-current masterd2d9d28dis resolved in a signed normal merge, tested tree4516c8e1. Master's cache display and test-preference isolation changes are preserved; the USDT UI helper retains its explicit endpoint overrides and unavailable defaults. Final canonical validation passed 2,932 unit tests and 39 UI executions, four fixture skips, zero failures. The previously failing launch test passed locally, which does not establish the cause or a permanent fix for its earlier hosted timeout. Full SwiftFormat 0.64.0 passed (0/846); translations found zero errors and 15 warnings with all 20 inputs identical to actual master. Complete diagnostics and an independent frozen integration review were inspected. All 17 current-base commits verified locally and match GitHub's exact set withverified=true,reason=valid. Hosted unit 40519925128 and integration 40519925139 passed on workflow attempt 1. Unit/UI: 2,932 unit and 39 UI executions passed, four fixture skips, zero failures; all 35 watch-only account tests passed, and the earlier launch case passed in 13.385s. Integration: 30 unique passes in 30 executions, four existing Blocktank staging skips, zero test failures or test-case reruns. Both used verified checkout3c59760a(d2d9d28d/b53e300e), tree4516c8e1, exactly matching final local validation. Full artifact digests, execution trees, UI/skip summaries, structured diagnostics and compile/action/runtime logs were inspected. Hosted E2E 40519926046 also passed on workflow attempt 1: 13 unique passing cases in 13 executions, one existing hardware-case skip, zero failures. Its first test step passed; steps 2/3 were skipped. Build and runner checked out the same3c59760a/4516c8e1tree, with pinned harnessec01c02f. No manual CI retry. All three existing human requests are preserved; João's changes request remains. The later fee-thread disposition is recorded above. No approval, merge readiness or funded/provider acceptance is inferred.Final diagnostics retain 16 structured QoS warnings, three AppIntents compile notices, 12 UIKit appearance events, 24 CoreGraphics NaN lines in app stdout, and platform/negative-case runtime messages. Full address stacks are saved; no raw symbolicated backtrace was exported. No fresh base runtime or warning-free claim. Effective runner/helper and built endpoints were verified at unavailable 295xx ports; the fork skipped before funding. J1/J2, physical hardware, full linked-request/two-wallet settlement and rendered 546/547 gaps remain as documented below.
Hosted diagnostics retain the exact prior 286 structured / 571 build-log duplicate-warning multisets. Integration's existing sync helper recovered from a first-attempt Taproot sync timeout inside a passing test; no whole test was repeated. Unit runtime logged a keyboard-queue timeout with
Last Exception Backtrace: No stack!, plus socket/haptic and negative-case messages. These observations are retained without a cause or permanent-fix claim. Integration's 22 associated inputs match actuald2d9d28d; of 40 unit-runtime source files, 38 match and AppViewModel/SendConfirmationView retain this PR's changes. No fresh actual-base runtime or warning-free claim.E2E diagnostics retain 66 stale-element notices, four log-stream restarts (statuses 143/137/124/149), Node/npm deprecations and the pinned harness's 76 dependency-audit findings (8 low, 17 moderate, 44 high, 7 critical). Build/action output retains AppIntents metadata, destination/tool and ignored-base-input notices. All five job logs and 14 case outcomes were inspected. App artifact
12741962657(141,017,046 bytes) has SHA-25681978458fa352095f683215ca76521bd97cd1c7fe1a8e73e62477a4e59891883, matching API metadata, upload and runner download logs; the archive was not downloaded locally. The successful workflow did not upload runtime-report/video artifacts. These results do not establish a warning-free run or erase earlier failures and acceptance gaps.Actual master at the latest audit is
85e9d4c6: three commits and eight paths beyond the hosted base543d9389. The only overlap with the PR diff is AppScene's foreground-notification hunk, disjoint from this PR's request-preparation timing changes. The PR remains mergeable; no integration was performed. Hosted unit/integration/E2E results cover543d9389/20cfeffa; the newer85e9d4c6combined tree is unvalidated. Local results retain their separate repaired PR-tree attribution.Earlier validation —
20f54449(October 27): The unit CI crash at811b4335was traced to concurrent access in the watch-only test fake: an assertion read its tracking Set while the authorization task inserted into it. The two affected tests now await the existing actor-isolated gate after tracking has finished. All assertions and all 35 tests remain; production code is unchanged. The coordinated repair passed 35 focused tests, 2,928 unit executions and 39 UI executions, with four explicit fixture skips and zero failures. Full SwiftFormat 0.64.0 passed (0/845 files need changes); translation validation found zero errors and 15 warnings with unchanged actual-base inputs. Complete diagnostics and independent frozen review were inspected. All 16 commits against actual masterb666ccb1verify locally and match GitHub’s exact commit set withverified=true,reason=valid. Hosted integration 40501754529 passed on attempt 1: 30 unique tests passed in 30 executions, four existing Blocktank staging skips, zero failures or retries. Verified checkouta7cb2f52(b666/20f54449) has treec1e3cd27, exactly matching final local validation. The archive digest, all 34 full case summaries, tree, action/runtime output and compile/test log were inspected. Structured warning/error arrays are empty; retained 571 duplicate-symbol compile warnings match the prior integration multiset, alongside the version-probe broken-pipe exception, tool/destination notices and embedded RGS/node/address-search/teardown/fixture/launch-metric messages. All 22 associated source inputs match actual base; no fresh base runtime or warning-free claim. Hosted unit40501754193subsequently failed on attempt 1: all 2,928 unit executions (including all 35 watch-only tests) passed, but UI had 38 passes, four skips and one failure inBitkitUITests/testExampleatapp.launch(). XCTest timed out acquiring a background assertion; the failure video shows splash then Terms. Root cause remains unproven. E2E40501754542passed 13 unique cases in 13 executions on its first step, with one existing hardware skip and zero failures. The complete logs, artifact digests, test trees, failure video and diagnostics were inspected; no manual CI retry. Both use the verifieda7cb2f52/c1e3cd27checkout. The 286 structured/571 compile warnings match the preceding unit run; E2E retains 70 stale-element notices, log-stream restarts and pinned-harness dependency warnings. All three existing human requests are preserved; João’s changes request and Piotr’s fee-refresh thread remain in force.Earlier validation at
9a807ef3(October 27): Conflicts with actual masterb666ccb1are resolved in signed normal merge9a807ef3, tested tree0ce4fc92. The combined tree preserves fixed-quote amount/expiry/dust/fee and authorization safeguards, duration-only diagnostics, and master's SDK/request/proof lifecycle changes. Final local gates passed: 15 focused tests, 2,928 unit executions and 39 UI executions, four fixture skips, zero failures. Full SwiftFormat 0.64.0 reports 0/845 files needing changes (81 skipped); translations report zero errors and 15 warnings whose inputs match actual master. Complete test trees, compile/action logs, 19 structured QoS warnings, 21 raw backtraces and remaining platform notices were inspected and retained; this is not a warning-free claim or runtime baseline reproduction. All 14 commits against actual master verified locally and matched GitHub's exact set, with every signatureverified=true,reason=valid. Hosted unit, integration and E2E workflows all passed on attempt 1 at the exact locally tested merge tree. Hosted unit/UI: 2,928 unit and 39 UI executions passed, with four fixture skips. Integration: 30 passed and four Blocktank skips. E2E: 13 passed and one existing hardware-case skip; its first test step passed and steps 2/3 were skipped. Full diagnostic and helper-retry limits are recorded below. Full evidence is recorded below. All three existing human review requests remain pending; no changes request was dismissed.The canonical run verified built RPC/bundler endpoints at unavailable 29545/29546 and the fork skip before app launch or funding. No shared fork access, funding or transfer occurred. Funded/provider/hardware/two-wallet and rendered 546/547 acceptance gaps remain; the historical incidents and hosted retry/skip limits below retain their original attribution.
Earlier validation (October 26, retained at its original head): Conflicts with actual master
b8b7f3d9are resolved in signed normal merge63ea2034(tested tree21fb1809). The combined tree preserves master's wallet-switch lifecycle fix and this PR's quoted-payment guards. Final local validation passed: 2,894 unit executions and 39 UI executions, with four fixture skips and no failures. All 13 current-base PR commits verify locally and on GitHub. Hosted unit/integration checks passed. E2E completed successfully after two built-in retries of the Lightning case; 13 unique cases passed and one hardware-transfer case stayed skipped. The two earlier Lightning timeouts remain unexplained. J1/J2 and the funded/provider/hardware acceptance gaps below remain open.Fixed quoted Bitcoin requests open in the existing confirmation screen when no separate funding-source selection is needed. A fixed $2.50 Savings request skips the amount screen with disabled controls. Requests offering LNURL Spending alongside Savings retain the existing funding-choice screen because LNURL confirmation has no wallet selector.
Description
resolveMsandsetupMs) to separate endpoint resolution from subsequent setup without logging payment details.Out of Scope
Cross-platform fixed-quote journey wording and LNURL/Savings coverage: Align fixed-quote journeys with both payment flows bitkit-android#1610 (review thread). Android #1581 is already merged; the remaining gap is in the shared test instructions and platform notes.
Preparation latency (Contact payment latency on staging with the shared Paykit runtime: pin paykit-rs#171 and re-measure #868): the reported several-second delay has not been reproduced with timings. Endpoint resolution and fee/balance preparation are possible contributors; this PR does not claim to fix their latency.
Initial subscription authorization: this does not remove the extra review required for converted subscription terms.
Design
Reuses the existing send confirmation without changing its layout. Android's screen map identifies
Send (Paste) (On-chain) → Confirm Send Onchainin the Bitkit design file. The exact current frame node is unverified.Preview
Before: Preparing → Amount (locked) → Confirm.
After, when no separate funding choice is required: Preparing → Confirm. LNURL + Savings keeps its existing selection step.
QA Notes
Setup
Use the existing fixed-quote journey prerequisites: two disposable identities, a fresh fixed $2.50 Bitcoin quote, funded regtest Savings and a usable Spending channel for live switching. For the minimum boundary, use controlled rates yielding exactly 546 and 547 Savings sats as specified in
journeys/paykit-quoted-usdt.xml. No funded boundary/settlement run is claimed below.The XCTest USDT fixture is separate:
UsdtWalletUITestsnow defaults to unavailable loopback endpoints. An explicitly owned disposable fork must be selected through test-runnerBITKIT_TEST_USDT_RPC_URL/BITKIT_TEST_USDT_BUNDLER_URL; its test funds its wallet.BITKIT_TEST_USDT_OFFLINE_RPC_URLmust be unavailable. Never use another task's shared fixture. Seejourneys/README.md.Journeys
J1: new
journeys/payment-requests/fixed-quoted-confirmation.xml— LNURL/Savings selection, quoted amount and wallet valuation, saved/manual coin selection, cancel/reopen, delayed preparation, insufficient funds, expiry and BTC/USDT choice. The live two-wallet journey remains to be driven; automated coverage is listed below.J2: updated
journeys/paykit-quoted-usdt.xml— iOS dot-decimal checks in comma-region settings preserve exact values, cents, trailing zeros and six-decimal precision. Full J2 remains unverified; controlled 546/547-sat Spending/Savings boundary: below-minimum iOS terms return to Amount with Continue disabled; exact 547 remains on confirmation. Android retains confirmation but disables authorization below its equivalent minimum. Source parity checked; this revised live boundary journey is unrun.Manual Tests
N/A
Earlier master integration and local validation —
63ea2034The normal integrations retain the previous dust and fixture-endpoint fix, merged #1003's Receive coverage, and merged #1098's single wallet-switch run and resumed-preparation behavior. Quote identity, exact amounts, expiry, selected-rail minimums, routing-fee cancellation and authorization checks remain intact. No assertions were removed or weakened, and no new exclusion or authored test was added by conflict resolution.
b8b7f3d9tree, using Xcode 26.1 and an iPhone 17 Pro Max simulator on iOS 26.1, one worker and private build/package paths. The six canonical integration exclusions match the workflow; separate integration, E2E, provider or hardware completion is not claimed by this local run.b8b7f3d9matched the local range and reportedverification.verified=true,reason=validon GitHub. Both integrations are additive signed commits, with authors and prior work preserved.The earlier
536abb44candidate also passed its own canonical run before master advanced, but that result is distinct from the final run above. The earlier shared-fork incident and incomplete rendered 546/547-sat boundary, provider, physical-biometric, hardware and two-wallet settlement journeys remain explicitly unvalidated.Earlier dust/fixture fix validation —
9acc68e2Quoted Savings terms now require at least 547 sats before confirmation can rebind them. Exact valid amounts, expiry, quote identity and authorization guards are preserved. This addresses Piotr's finding.
test_fundWallet, and observed 1,000 fake USDT. Quote preparation failed before confirmation; no transfer/settlement is evidenced. This was an isolation mistake, not accepted funded QA. No fixture repair, reset or compensating write followed.2ce21025verify locally and asverified=true,reason=validon GitHub. This is an additive signed commit, with no history rewrite. At that point hosted CI was blocked by the master conflict; the current signed integration above supersedes that hold.J1/J2 and live provider, physical biometric, hardware and two-wallet settlement gaps remain open. The final safe fixture skip does not validate the failed funded journey.
Automated Checks
PaymentNavigationHelperTests.swift— exact converted amount, expiry, real BTC/USDT choice, routing, unavailable/expired-rate fallback and reviewed-term mismatch protection.PaykitPaymentRequestServiceTests.swift— hosted confirmation waits for wallet sync before rejecting a below-minimum quote.SendConfirmationLifecycleTests.swift— hosted fixed-request preparation, rail-specific quoted amounts and fees, invalid-term fallback, manual picker Back/retry, UTXO failure and fee failure/recovery.Historical validation before
ad30f8be: the 48-test focused result above predates this integration. The prior all-target compile, whitespace-only ActivityRow formatting repair, 14 verified base translation warnings and CoreSimulator boot blocker remain historical evidence.Historical author validation at
ad30f8be(tested treef76406a5, integrated master53c5ec0d):xcodebuildmcp simulator test --jsonfocused selection: 439 passed, zero failures/skips. The canonical direct Xcode fallback used the same six CI integration exclusions and one worker: 2,881 units plus 43 UI executions (39 passed, four fixture skips), zero failures. The UI skips require the disabled Trezor emulator or local Arbitrum fixture; they are not passes. Existing unfunded iPhone 17 Pro simulator, iOS 26.1.53c5ec0d. Independent combined integration review found no verified issue; PR patches and master recovery/sign-out behavior are preserved.e4bf44a2through maintainer merges of fix: unify usdt balance and amount presentation #1077/fix: reuse received payment artwork layout #1092. Those 36 paths are disjoint from this PR's 13-path delta; a clean read-only merge retained the exact PR blobs and all current-base changes. Local runtime evidence applies to the published tree. Hosted unit and integration checks subsequently passed on CI mergea6deea4d, whose tree053473bcexactly matches that reviewed combination withe4bf44a2.e4bf44a2verified locally and the exact GitHub commit set reportsverified=true,reason=validfor every commit.Hosted validation for head
ad30f8be:e4bf44a2run 40028545050. AppIntents metadata notices also occur on both. Project, workflow and dependency pins match that base; no new warning was found. The hosted xcresult records no runtime warnings.@lightning_1timed out after 30 seconds waiting for “Transfer Initiated” after closing a restored channel (lightning.e2e.ts:276at pinned E2E revisionbcf91fee). The second attempt reran only that case and passed; the other 13 were skipped and attempt 3 did not run. Complete workflow logs and per-case reporters were inspected. The close-confirmation view, transfer/wallet view models, Lightning service, toast components and E2E workflow match actual basee4bf44a2; this comparison does not prove that the timeout is pre-existing or establish its cause. After the successful retry, the workflow skipped runtime/screenshot/video artifact upload; only the built app artifact remains, so the saved failure visuals and app logs could not be inspected. Startup log-stream wrapper restarts are separate from the test retry. No manual workflow retry or code change was made, and the passing retry does not establish a permanent fix. All CI workflows for that earlier tested revision passed; this historical result does not describe the latest head. The full payment-journey limitations below remain.External device QA at exact head
ad30f8beis attributed to review 5635086275, which reports no introduced finding. On an iPhone 17 Pro / iOS 26.5 simulator with regtest funds, the reviewer checked direct 5,000-sat confirmation and a 143-sat fee, Largest First selection, manual-picker Back, cancellation during preparation, LNURL/Savings choice, LNURL-only direct confirmation, BOLT11/Savings switching at 2,500/5,000/2,500 sats, the fixed $2.50 BTC/USDT choice, and insufficient/expired-term guards. Swiping after expiry left both Bitcoin balances unchanged; fresh terms required confirmation. Hosted assertions covered invalid-term and fee/UTXO failure recovery. Videos and exact limitations are in that review.This external subset does not report a completed two-wallet fixed-quote settlement. USDT funding/submission, subscription authorization and hardware-wallet signing were not exercised; provider and physical-biometric validation remain incomplete. The full journey above stays unchecked. That earlier author-validation pass made no funded payment or wallet reset. Simulator data was preserved and the native window released.
Models used
For this integration and validation update (verified session metadata; earlier implementation attribution is not inferred):
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:high)Hosted unit/integration completion —
63ea2034, 2026-10-26Unit40410064946 and integration40410064928 succeeded on workflow attempt 1. Both checked out merge
a9eb51a06724ccd6a0cbff62487fcd8b5bf0bc77, with verified parents actual masterb8b7f3d9/ published63ea2034and tree21fb1809bb1af4f626c437015f8780139a9320b7, exactly matching the locally tested published tree. Unit: 2,894 passed. UI: 43 executions, 39 passed / four fixture skips / zero failures. Integration: 30 passed / four existing Blocktank staging-retirement skips/zero failures, with no retry execution. The four integration guards are byte-identical to actual master. Skips are not successful provider/hardware journeys.Artifact ZIP digests, all execution refs/skip reasons, complete logs and structured build/action reports were inspected. Unit has 286 structured duplicate-symbol warnings and 571 build-log warning messages; the structured multiset equals the historical b261 baseline. Project/pin/unit/integration workflow inputs match actual b8, but no actual b8 runtime baseline was run. Both full compile logs retain 571 duplicate-symbol warnings plus tool/multiple-destination notices; the integration version probe retains its broken-pipe exception. RuntimeWarnings/testFailures arrays and action warning/error nodes are empty. Integration's test-without-building status does not prove warning-free compilation. Xcode 16.4 compile paths, iPhone 16 / iOS 26.2.
E2E workflow 40410065359 completed on workflow attempt 1 using the same verified
a9eb51amerge/tree above, Xcode 26.2 (17C52), iPhone 17 / iOS 26.2 and E2E revisionec01c02ff611797c112e65b5c87c936d25720bac. Of 14 selected cases, 13 unique cases passed across 15 executions and three built-in test attempts. Attempt 1: 12 passed,@lightning_1failed, and hardware-wallet transfer-to-Spending stayed skipped. Attempt 2 reran only Lightning and failed; attempt 3 passed it. Both failures timed out after 30 seconds waiting forActivityShort-0to contain9 000after closing the restored channel (lightning.e2e.ts:287). No root cause or actual-base runtime reproduction is established; passing the third attempt does not establish a fix. The hardware skip is explicit in the pinned test source because Blocktank staging no longer creates orders.All five job logs, per-case reporters and complete available diagnostics were inspected. The workflow marks the first two steps successful through
continue-on-error; their raw exit-1 failures remain recorded here. Failure screenshots/videos were named in the runner log but were not uploaded after final success; only the app artifact remains, so those visuals and runtime reports could not be inspected. Log-stream restarts (143/137/124/124/143/143), 80 stale-element warnings, the address-type helper retry, dependency notices/audit output and the AppIntents/multiple-destination build notices remain retained. No new source change or manual CI retry was made. Rendered 546/547, funded/provider/hardware/two-wallet and other documented acceptance gaps remain explicit.Hosted integration completion —
9a807ef3, October 27Integration workflow40453530944 passed on attempt1: 30 unique tests /30 passing executions, four existing Blocktank skips, zero failures. All34 case summaries and the full execution tree confirm no retry execution. Empty runtimeWarnings/testFailures arrays and no structured action warning/error nodes. Embedded action output still contains RGS gossip failures, node/address-search/teardown diagnostics and the orphaned-RN-keychain fixture: all45 exact diagnostic groups were inspected, and all nine associated source/test files match actualb666. Hierarchy copies duplicate these log lines; this is not a warning-free or actual-base runtime reproduction claim. Skipped refund-address lifecycle/create-CJIT/orders/channel-purchase cases cite the closed staging service; their source and workflow match actual master.
The job checked out merge
a27dd5a89eba7dd6c2fd9e753131800383c565cc, with verifiedb666ccb1/9a807ef3parents and tree0ce4fc92, exactly matching the locally validated tree. Artifact digest, full job log, test trees and structured diagnostics were inspected. Compile output retains571 duplicate-symbol warnings (286 unique), the same complete multiset as the earlier actual-cc5 baseline. Current project/pins match actualb666, but no new b666 runtime reproduction is claimed. Test-without-building buildStatus=notRequested/no exported build log is not warning-free evidence; the full separate compile output is retained. The version-probe broken-pipe exception and tool/destination notices remain explicit. Runner: Xcode16.4 compile environment, iPhone16/iOS26.2; local26.1 results remain distinct.Unit and E2E completion is recorded in the following addendum; no manual retry. Funded/provider/hardware/two-wallet/rendered546/547 acceptance and historical retry/skip limits remain. Full report:
/private/tmp/usdt-monitor-20261027-0534/ci/completion-report.md.Hosted unit and E2E completion —
9a807ef3, October 27Unit 40453530938 and E2E 40453531272 passed on workflow attempt 1. Unit, E2E build and E2E runtime logs identify merge
a27dd5a89eba7dd6c2fd9e753131800383c565cc, with verified actual-masterb666ccb1/ published9a807ef3parents and tree0ce4fc929ce5dcb22d64b5800dc7451466705bb3, matching final local validation.ec01c02f. This does not establish physical-hardware acceptance.The unit archive digest, complete execution trees, all UI summaries, structured build/action diagnostics, all six unit/E2E job logs and available artifact metadata were inspected. Unit structured runtimeWarnings/testFailures are empty. Its 286 structured duplicate-symbol warnings and 571 build-log warning messages exactly match the recorded actual-cc5 multisets; current build inputs match b666, but no fresh b666 runtime baseline was run. Embedded action output retains negative-case offline/auth/storage/identity/hardware/durable-recovery and fee/UTXO diagnostics, synthetic Blocktank UUID-order failures, and simulator haptic/socket notices. Associated source comparisons and complete groups are retained; this is not a warning-free or blanket pre-existing claim.
E2E retains 68 stale-element notices, log-stream restarts 143/137/124/124, and one address-type tap/toast helper retry, separately from test retries. Tool/AppIntents/Node/npm notices remain; the pinned external E2E harness reports 76 dependency vulnerabilities (8 low, 17 moderate, 44 high, 7 critical), identical to its earlier same-revision log, with no uploaded advisory report. Only the app artifact was uploaded, and its metadata digest matches the build-upload and runner-download logs; no runtime-report or video artifact was available after success. Unit used Xcode 16.4 / iPhone 16 / iOS 26.2; E2E used Xcode 26.2 / iPhone 17 / iOS 26.2.
No manual CI retry or source change. Earlier Lightning failures/recovery and fixture incidents retain their original attribution. Funded/provider/physical-hardware/two-wallet settlement and rendered 546/547 acceptance remain incomplete. Full evidence:
/private/tmp/usdt-monitor-20261027-0636/ci/completion-report.md.Quoted review Back fix —
811b4335, October 27Returning from fallback Amount to a fixed quoted request’s Confirm root now closes that presentation, so Back no longer immediately pushes Amount again. The first fallback—including initially unreviewed terms—still requires amount review; valid terms reset the marker. The existing presentation-scoped dismissal preserves a replacement sheet. Seven production lines changed; two existing lifecycle tests were extended with a true Confirm-root host, with no new/removed tests. The existing journey now includes the iOS fallback Back and manual-coin return steps.
Unchanged
9a807ef3failed the Back/dismiss assertions for all four invalid quote variants; the frozen fix passed all 15 focused lifecycle tests and the workflow-selected canonical suite: 2,928 unit + 39 UI executions passed, four fixture skips, zero failures. Three Trezor cases remain opt-in; the isolated fork skipped at unavailable29546before app launch/funding. Exact helper/runner forwarding and built endpoints were checked. SwiftFormat 0.64: 0/845 files need changes; translations: zero errors/15 warnings, with all 21 relevant inputs identical to actualb666ccb1. Independent frozen-diff review found no verified issue. All 15 current-base PR commits verified locally and match GitHub’s exact set withverified=true/reason=valid. Normal additive push; no history rewrite.Complete logs, execution trees and 47 warning/UI/skip summaries were inspected. The 13 structured QoS warnings and 16 raw backtraces are different diagnostic surfaces; raw traces cover Pubky/mainline initialization and LDK status/sync. Two AppIntents compile notices, 11 UIKit appearance notices, simulator haptic diagnostics and 48 CoreGraphics NaN notices during passing USDT Receive/manual-entry cases remain recorded. No NaN backtrace or fresh actual-base runtime reproduction is available; those cases do not enter the changed quoted Bitcoin Confirm path. The changed source’s UTXO-unavailable logs come from an unchanged negative-case method. No blanket warning-free/pre-existing claim. Test-without-building metadata is not compile-clean evidence.
The manual-coin regression also passed with Confirm as the actual navigation root: after Continue to the second confirmation and Back through the picker, the original root recalculated using the selected coin without a fee-rate change. This proves the controlled fee-preparation call, not rendered fee text or a funded/provider journey. New-head hosted CI is pending; full linked-request/provider/physical-hardware/two-wallet settlement and rendered 546/547 acceptance remain incomplete. Previous evidence retains its original head attribution. Planning/implementation/review for this fix:
gpt-6-astra, reasoningxhigh, verified from actual execution metadata.Hosted integration at 811b433
Integration 40486729719 passed on workflow attempt 1: 30 tests passed in 30 executions, four existing Blocktank staging skips, zero failures or retry executions. Checkout
156e44dbhas actualb666ccb1/811b4335parents and treee5409e4f, exactly matching final local validation. The archive digest, all 34 case summaries, test tree, action/runtime output and full compile/test logs were inspected.Structured warning/error arrays are empty, but test-without-building is not proof of warning-free compilation. Retained 571 duplicate-symbol compile warnings (matching the prior hosted integration multiset), the version-probe broken-pipe exception, tool/destination notices and embedded RGS/node/address-search/teardown/fixture diagnostics. Associated inputs match actual base; no fresh base runtime reproduction is claimed. The subsequent unit failure and E2E completion for this parent are recorded in the crash-repair addendum below. Provider/funded/physical-hardware/full linked-request/two-wallet/rendered-fee acceptance limits and Piotr's open finding remain unchanged.
CI crash repair and diagnostic limits —
20f54449At parent
811b4335, unit workflow 40486729701 built successfully but had 2,927 unit passes and one crash intestAuthorizationSerializesReconciliationUntilTrackingStateIsPersisted; UI had 39 passes/four skips. The crash report showsXCTAssertEqualreadingtrackedAccountIndexesconcurrently with the fake’s insertion. The test and service inputs match actualb666ccb1, but no actual-base runtime reproduction is claimed. The narrow synchronization repair was coordinated with the file’s owning work before editing. Parent E2E 40486730476 passed 13 unique cases in 13 executions, with one existing hardware-case skip and no failures. Both checked out merge156e44db/ treee5409e4f(b666/811b); these results retain that parent attribution, not the new head.Final local repair validation used tree
c1e3cd27, Xcode 26.1 and the owned iPhone 17 Pro Max / iOS 26.1 simulator with one worker and private products. The complete execution tree confirms 2,928 unit + 39 UI passes; the summary groups 30 launch configurations into one. Three Trezor opt-in cases and the unavailable isolated fork skipped. Built endpoints and runner/helper forwarding were verified; runtime fork availability failed at unavailable29546before app launch or funding. No shared fixture was accessed.Full reports retain 16 structured QoS warnings and 16 raw backtraces (14 Paykit/Pubky initialization, two LDK status), two AppIntents compile notices, UIKit appearance and simulator keyboard/haptic/graphics/network notices, and negative-case storage/auth/recovery/fee diagnostics. Relevant QoS wrappers/pins match actual
b666ccb1; no fresh base runtime or warning-free claim. The passing UI run emitted 24 CoreGraphics NaN notices; the earlier identical-production811brun retains 48. No causal or permanent-fix claim is made for those notices. Hosted parent E2E retains 68 stale-element notices, helper/log-stream retries, pinned-harness dependency warnings and absent runtime-report upload. These checks do not complete J1/J2, production-provider, physical-hardware, funded settlement, full linked-request or rendered 546/547 acceptance.Native Back prerequisite diagnostic — October 27, 21:24 UTC
A temporary, isolated diagnostic at
20cfeffanow mounted the productionSendSheetthrough the same modal binding asMainNav, with setup-complete state seeded and unavailable fixture endpoints. This excludes real linked-request setup and persistence, authorization, and funded/provider acceptance. The modal, two-controller stack, enabled native recognizer, key window and hit-test were verified. One cancellation-shaped drag and one completion swipe still failed before any interactive transition began; the diagnostic failed all four relevant assertions. Adding the modal wrapper did not close the gap or establish a production defect.A standard UIKit navigation control on the same iOS 26.5 simulator did reach and cancel a genuine interactive Back transition. Its first run and one bounded recovery both expired before the completion gesture could be delivered because of turn interruption/context compaction; both remain failed, with two assertions each. No completed native pop or J1 acceptance is claimed. Full logs, nested diagnostics, failed assertions and screenshots are retained, including UIKit transition fallback and platform/accessibility notices. The temporary source was restored byte for byte, the checkout is clean, and scoped simulator services are stopped with data preserved. No production edit, source push, CI rerun or additional review request was made.
Native Back control follow-up — October 27, 22:28 UTC
The final bounded recovery of the interrupted UIKit control passed using identical built products: both cancelled and completed native transitions were observed, and the stack returned to its root. Fresh UI and log guards kept both gestures inside one live test window. Earlier interrupted failures remain retained.
The remaining prerequisite also fails in minimal SwiftUI and UIKit modals with hidden navigation bars and no payment/request/wallet objects (four and three assertions respectively). Each received one cancellation drag; the guard withheld completion when no interactive transition began. Separate SendSheet observations found both the edge and iOS 26 content-pop recognizers enabled, with delegates permitting a start. These values were unchanged by temporarily showing the bar. Both shared swipe-extension files match current master
85e9d4c6; this is source comparison, not a base runtime test.The controls do not yet separate navigation-bar visibility, modal presentation and runtime behavior, so no product cause/fix or completed J1 native Back acceptance is claimed. All logs, nested diagnostics, failure stacks and screenshots are retained. Temporary source is restored exactly and the checkout remains clean at
20cfeffa; no source push, CI rerun or reviewer notification. Existing provider, funded, physical-device and full linked-request limits remain.Native Back visibility control — October 27, 23:28 UTC
The isolated UIKit modal control passed its one selected test in 8.209 seconds on the same owned iOS 26.5 / Xcode 27.1 setup after changing only the navigation bar from hidden to visible (plus its state assertion/log and test name). Native cancellation and completion callbacks were
[true, false], and the stack returned to its root. No SendSheet/payment/request/wallet objects participated. The prior hidden-bar modal failures remain retained.This isolates visibility-dependent behavior in that control; it does not establish the production cause/fix or complete SendSheet J1 native Back acceptance. Shared swipe code matches actual master
85e9d4c6, by source comparison only. A shared-navigation follow-up requires ownership reconciliation. Full build/test trees, nested diagnostics, raw logs and screenshots were inspected. Structured warning/error arrays are empty; SDK/accessibility notices and two UIKit transition-fallback notices remain. Temporary source is restored exactly, the checkout remains clean at20cfeffa, and scoped simulator services are stopped with data preserved. No production edit, source push, CI rerun or reviewer request. Existing provider, funded, physical-device and linked-request limits remain.