Repository navigation
fix: apply fixed prices and prioritize linking - #887
Conversation
 |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request Changes
Review: diff 2 files.
Findings:
1 inline (1 HIGH)
QA:
No tests ran because the PR description says there is no user-visible change and marks both journeys and manual tests N/A.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
ben-kaufman
changed the title
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 8 files.
No new findings; the rest is in the review.
QA:
No tests ran because testing was skipped based on the earlier description's no-change and N/A entries, although the updated description now lists fixed-price-bitcoin.xml.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: full PR review at bfa8db29 against merge base a231b829, covering fixed-price intake, endpoint selection, confirmation/payment/proof paths, subscription rejection, the SDK upgrade and regression coverage.
No new actionable code findings.
The earlier fixed-price concern is addressed at this head. The four new pricing tests passed in the matching CI unit job. Local validation passed 3,049 host Swift arithmetic cases using fixture SDK types, plus a separate precision boundary check; the new helper also typechecked against checksum-verified rc71 bindings. Full app tests were not rerun locally.
Targeted comparison with Android #1437 at c36ee533 covered pricing, endpoint filtering, subscription exclusions and the matching journey. Android rejects multiplication products above 38 significant digits; iOS can accept some such products exactly. Align or document that precision boundary before relying on identical issuer support across platforms. This was a compatibility check, not a full Android review.
Suggested additional test cases
- iOS regtest, funded savings and a distinct linked issuer: send a 0.001 BTC request with fixed
btc-regtest: 0.5, complete payment, then relaunch. Expect a 50,000-sat recipient output, a submitted proof/history entry and no second payment offer. - iOS regtest, funded Lightning and a distinct linked issuer: request 10 USD at fixed
btc: 0.000021with a usable BOLT11 invoice for 21,000,000 msats. Complete payment and verify 21,000 sats settled with one proof; repeat with a fresh invoice for 21,000,001 msats and expect rejection before spending.
Device testing: not performed in this review; the added confirmation journey and funded conversion payments remain unexecuted.
|
Tested the on-chain fixed-price flow on iOS using the standalone issuer from bitkit-e2e-tests tool PR #269. Paired testing: other platform PR.
For each payable request, obtained an unused address from Receive, checked confirmation/success amounts, verified the exact confirmed recipient output, and verified exactly one issuer proof matching the transaction. Fees were separate. Fiat display used Bitkit's local market valuation. Also checked USD without a conversion rate ( Observation reproduced on both platforms: Sending a new request to an already-paid one-time on-chain address leaves the confirmation spinning in preparation. Logs report 
Session log archive is prepared; attachment upload is currently blocked. I will add it to this comment once uploaded. Additional iOS observation: BTC preparation hit transport/concurrent_update errors and completed after an app restart. This is recorded separately from the used-address failure. Fixed-price Lightning was intentionally excluded. Installed apps match the user's built AUTs by checksum; exact source provenance was not independently verified. iOS USD/rounding were repeated after log rotation to retain complete logs for those cases. These are focused on-chain results, not a full PR device pass. |
|
@piotr-iohk Documented the precision boundary in 11dfe73: cross-platform issuers should keep the normalized amount-times-rate product within 38 significant digits. Android rejects larger products; iOS may accept some exactly, without rounding. Pricing behavior is unchanged. The paired journey name is also lowercase. XML validation passed. Full formatting and translation reports match existing findings; no Swift files changed in this follow-up. The fixed-price issuer fixture and funded device cases remain outstanding, not counted as passes. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 2 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-android#1437 rejects normalized fixed-price products above 38 significant digits; the issuer guidance says to keep products within that limit, and iOS may accept some larger products exactly.
QA:
Tests wait for CI.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
@ben-kaufman Could you clarify the spinner observation reproduced on both platforms? A request using an already-paid on-chain address returns endpoint_not_payable, but the confirmation keeps loading without showing an error. Should this be addressed in these PRs, or tracked separately as an existing issue? |
|
@piotr-iohk Fixed here in cdb692a, with the matching change in Android #1437 (f8ee07784). The retry path already existed in master: Validation passed 313 focused native tests and the simulator build, with no introduced lint/format findings. The paired Also corrected the QA notes: your confirmed on-chain outputs and matching proofs are valid focused evidence. My earlier statement that funded conversion checks were outstanding missed your report posted during validation. The separate transport/concurrent_update failure is not claimed fixed; I will check its trace once you can attach it. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at cdb692a (includes "stop preparing unpayable payment requests"). No HIGH/MEDIUM. Three LOW inline comments: the Decimal product check (a refinement of @piotr-iohk's precision note; the accepted products are rounded, not exact, so the sentence added in 11dfe73 needs changing), the journey's issuer precondition, and the interop doc's fixture claim. Each can be fixed here or moved to a follow-up issue, your choice; please reply on each thread with which. This is a COMMENT rather than an approval only because the device gate could not run here and CI was still pending at posting time; nothing in the code blocks.
Checked and clean:
- Fixed-price parse path (PaykitPaymentRequestService.swift:157-176 → PaykitBitcoinRequestPricing.bitcoinPayment): rail > asset-wide > parity precedence, cross-asset unpriced rails dropped, duplicate/empty/zero/sign/exponent/whitespace/>80-char/>38-digit operands rejected, Lightning whole-sat requirement, UInt64.max/1000 cap on both branches, differing per-rail amounts rejected. Non-conversion requests take the old path unchanged.
- Approve-time amount equals pay-time amount: SendSheet.swift:368,1010 seed
wallet.sendAmountSats = request.amountSats; SendConfirmationView.swift:1207-1225 re-checksacceptsPaymentAmountand exact bolt11 msat equality beforeperformPayment; the originalamountValue("10" USD) is not consumed by any view. PaykitSubscription.init?(record:)returns nil for any conversion terms (PaykitSubscription.swift:563), so no renewal/auto-pay path can use a priced request.- cdb692a:
.endpointNotPayablenow goes throughmanager.dismissPreparingRequest(AppScene.swift:230-236), which only acts when the request is pending and is the requested presentation or not yet presented, adds it todismissedPreparingRequestIds(no auto-reopen this session) and bumps the presentation generation; the explicit Pay path removes it again (PaykitPaymentRequestManager.swift:2001). Covered by the newtestUnpayableEndpointEndsPreparationAndAllowsManualRetry. - Line-for-line parity with synonymdev/bitkit-android#1437 except the product-precision guard; both journey files and the action prose are identical. rc70→rc71 adds no storage format; v2.5.0 pins rc51 with no
conversionfield, so no migration finding.
Device gate: not run — needs an issuer that can send conversion.fixed and an already-paid one-time address (neither is in the Capabilities table; the documented paykit-server fixture cannot). @piotr-iohk drove the two fixed-price-bitcoin.xml cases and a rounding case at bfa8db2 with the standalone issuer from synonymdev/bitkit-e2e-tests#269 (#887 (comment)); the commits since are docs, the journey name and the unpayable-endpoint change, which has no device run yet. unpayable-endpoint.xml: not run.
Skipped — covered by CI at cdb692a: unit tests, integration tests, e2e (pending at posting time).
The endpoint_not_payable spinner @piotr-iohk reported is answered by cdb692a; left to that thread.
Sorry, logs from previous test session: |
|
@piotr-iohk Thanks, I checked the attached archive. It confirms endpoint_not_payable at 12:12:15 UTC, matching the preparation-feedback fix. The later relaunch shows shared-state lock contention: restoration is deferred from 12:20:59 through 12:21:44, then succeeds at 12:21:52. Backup/publication calls also report SharedStateBusy during that window. The retained files have a transport_error at 12:19:18, but no concurrent_update entry or native request/lock trace establishing the cause of the earlier preparation failure; the README also notes the rotation gap. That separate observation remains open, not fixed by pricing or the spinner change. A repeat capture needs the pre-restart failure and SDK/network error context, not only the successful restart. The precision finding is now fixed in 0143fe5 with the matching Android guard, regression tests and corrected fixture instructions. J2 still needs the used-address device retest. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-checked at 0143fe5: no findings.
PaykitBitcoinRequestPricing.swift: the combined significant-digit bound (amount + rate ≤ 38, leading and trailing zeros excluded) runs before the multiply, so the product is exact and the rounding case from the last review is rejected asunsupported_pricing. The counting matches AndroidstripTrailingZeros().precision()for the same strings.- The interop doc now describes that bound for both platforms and says fixed pricing is covered by the matching test tables, not the JSON fixture.
- The journey README and both journey descriptions state the rc71
conversion.fixedissuer and the paid-address fixture, and say to report the journey as blocked without them.
Device gate: not run — fixed-price-bitcoin.xml and unpayable-endpoint.xml need a conversion-capable issuer and a paid-address fixture that are not available here. Run Tests was still pending at this head.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Note
This review is for cdb692a, the commit it read. The pull request has moved to 0143fe5 since; its new commits get a reaudit from this review.
Reaudit: diff 5 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-android#1437 stops preparation the same way for an unpayable endpoint, keeps the request pending for manual retry, and uses the same journey actions. Its description also tells the tester to block the journey when the paid-address fixture is unavailable.
QA:
Tests wait for CI: Run Integration Tests and Run Tests failed.
Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 6 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-android#1437 applies the same combined normalized precision bound and boundary cases; both journey files match.
QA:
Tests running.
Replies:
@ben-kaufman: Fixed here in cdb692a. (comment)
I verified that the endpoint feedback fix remains in place. J2 still needs the used-address device check.
@ben-kaufman: The precision finding is now fixed in 0143fe5. (comment)
I verified the precision guard, matching boundary cases and corrected fixture instructions. The separate transport observation remains unisolated; this change does not claim to fix it.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Spinner fix retested on iOS at Passed the
Screenshots below show unavailable feedback and the subsequent ready confirmation. Full retest logs were captured and archived locally for manual attachment. Fixed-price Lightning was not tested. The separate transport/concurrent_update observation is not claimed fixed. 
|
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approved from our reviewed and tested scope at 0143fe5. Follow-up check of the precision guard and unpayable-endpoint preparation cleanup found no new actionable concern; the paired spinner retest passed. Earlier focused BTC/USD on-chain payments had verified recipient outputs and matching issuer proofs. Fixed-price Lightning remains untested, and the separate transport observation is unresolved. CI completion remains a separate merge check.
0143fe5 to
044f5de
Compare
ad411fe to
2b36210
Compare
ben-kaufman
changed the title
|
Rebased onto current master in 2b36210. The pricing and linking/rc72 changes already combined here are preserved; duplicate lifecycle commits already on master are omitted. All ten PR commits are signed and GitHub-verified. Validation: 576 focused tests, then all 2,187 unit tests plus the bundled UI smoke checks passed. Three opt-in hardware tests were skipped. Full diagnostics were checked against master: no introduced formatting, translation or compiler findings. Independent integration review found no blocker. The title and QA notes now cover the combined scope. Final-head device journeys and linking-performance follow-ups remain open; this rebase does not claim those are resolved. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 1 file.
No new findings; the rest is in the review.
This changelog-only delta changes no cross-platform behavior; the pricing and linking parity dispositions from 2b36210 remain unchanged.
QA:
Tests running.
Replies:
@greptile-apps: Updates Paykit from rc70 to rc71. (comment)
That report describes the original scope. The combined rc72 scope was reviewed at 2b36210; this delta changes only the changelog.
@piotr-iohk: Could you clarify the spinner observation reproduced on both platforms? (comment)
The endpoint-feedback fix remains in place, and your 0143fe5 retest confirms the earlier correction. Current-head device verification remains pending.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Agreed — approve. The rebased delta is changelog-only, so it introduces no new cross-platform behavior or review findings. The rc72 pricing/linking parity dispositions from Tip: You can customize Greptile's behavior for this repo with |
|
Thanks, the 17.8s after releasing the hold is useful latency evidence and remains open. Manual test 1 checks that admitted work completes safely and the same retry continues without duplicate advancement. The PR does not claim a linking deadline or add progress UI, so the elapsed time alone does not establish that scheduling check failed. For round 2, please include the redacted QA_RETRY/QA_LINK trace showing the retry ID, admission, release, SDK completion and subsequent retry, plus whether Request or Pay eventually became available. That will let us distinguish slow SDK work from a lost or duplicate retry. J3 still needs a genuine resume without restarting the process; J2 remains blocked until the already-paid destination fixture exists. J1 currently proves the quoted amounts at ready confirmation, not a completed payment. No claim that these remaining checks are fixed or passed. |
|
Applied the matching stale-contact synchronization protection from Android #1451 in 6db1097. Sharing enablement takes current contacts after public publication, and queued preparation/cleanup rechecks membership and session ownership. A newer save or re-add retains its explicit retry and endpoint state; ordinary row refreshes do not invalidate preparation, and admitted writes still finish. Cleanup also checks for newer cached payment data without suspending between comparison and removal. The held-publication and cache-interleaving regressions fail before their fixes. Final simulator test build and 223 native tests passed; complete format/translation/compiler diagnostics show no introduced findings. Controlled device-overlap QA is documented and remains unchecked. This does not establish faster linking or resolve the ongoing device timing reports. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 11 files.
No new findings; the rest is in the review.
The pair PR synonymdev/bitkit-android#1451 applies matching contact-membership and sign-in guards; both retain admitted writes and separate linking latency from retry correctness.
QA:
Tests running.
Replies:
@ben-kaufman: Manual test 1 checks that admitted work completes safely and the same retry continues without duplicate advancement. (comment)
The elapsed time alone does not show a lost or duplicate retry. I am keeping linking latency separate from retry correctness; the requested retry trace and current-head device checks remain pending.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Suggestion: 👍 Approve
Interim test report for the review; the rest resumes on its own.
QA:
Tested on four iOS 26.5 simulators (iPhone 17 Pro), regtest.
J2 not tested because mandatory device wipes consumed the request setup time; missing: a linked issuer request to the paid address.
🔴 Test 1
Test 1
On f3a2d12 + test hook: background link work took 18.281 seconds after release without visible progress.
1.mp4 | 1-r2.mp4 | 1-r3.mp4 |
test hook
Not pushed. Holds admitted link operation until release without replacing SDK result; logs retry identity.
diff --git a/Bitkit/Services/PrivatePaykitService+Contacts.swift b/Bitkit/Services/PrivatePaykitService+Contacts.swift
index ea701316..04362093 100644
--- a/Bitkit/Services/PrivatePaykitService+Contacts.swift
+++ b/Bitkit/Services/PrivatePaykitService+Contacts.swift
@@ -985,6 +985,9 @@ extension PrivatePaykitService {
publicKey: String, reason: String, schedulingSnapshot: inout PrivateMessageSchedulingSnapshot?
) async {
guard let retry = pendingMessageDrainRetries[publicKey] else { return }
+ #if DEBUG // test hook
+ Logger.info("QA_RETRY enter id=\(retry.id); index=\(retry.retryIndex)", context: "TestHook") // test hook
+ #endif // test hook
let generation = preparationGeneration
var identityInspectionFailed = false
defer {
@@ -1075,6 +1078,9 @@ extension PrivatePaykitService {
} else {
pendingMessageDrainRetries[publicKey]?.completeAttempt(at: messageRetryOperations.now())
}
+ #if DEBUG // test hook
+ Logger.info("QA_RETRY complete id=\(retry.id); retained=\(pendingMessageDrainRetries[publicKey]?.id == retry.id)", context: "TestHook") // test hook
+ #endif // test hook
}
private func isMessageRetryCurrent(publicKey: String, id: UUID, generation: Int) -> Bool {
diff --git a/Bitkit/Services/PubkyService.swift b/Bitkit/Services/PubkyService.swift
index b2bb3076..ff2d055a 100644
--- a/Bitkit/Services/PubkyService.swift
+++ b/Bitkit/Services/PubkyService.swift
@@ -895,7 +895,22 @@ actor PaykitSdkService {
priority: PaykitSdkOperationLock.Priority = .ordered
) async throws -> LinkedPeerHandshakeReport {
try await withStateRevisionTracking(priority: priority) { sdk in
- try await sdk.ensureLinkWithPeer(counterparty: counterparty, maxAdvanceSteps: maxAdvanceSteps)
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.environment["QA_HOLD_LINK"] == "1" { // test hook
+ let release = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-link-release") // test hook
+ Logger.info("QA_LINK admitted; waiting for release", context: "TestHook") // test hook
+ let deadline = Date().addingTimeInterval(90) // test hook
+ while !FileManager.default.fileExists(atPath: release.path), Date() < deadline { // test hook
+ try await Task.sleep(nanoseconds: 100_000_000) // test hook
+ } // test hook
+ Logger.info("QA_LINK released; cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+ let report = try await sdk.ensureLinkWithPeer(counterparty: counterparty, maxAdvanceSteps: maxAdvanceSteps) // test hook
+ #if DEBUG // test hook
+ Logger.info("QA_LINK SDK finished; state=\(report.state); cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ #endif // test hook
+ return report // test hook
}
}
🟢 Tests J1, J3
Test J1
On f3a2d12 + test hook: both fixed quotes reached ready confirmation at 21,000 and 50,000 sats.
J1.mp4 |
test hook
Not pushed. Allows the issuer Bitkit to send USD and rail-specific fixed quotes through its real staging SDK, and logs incoming immutable terms without changing pricing or resolution.
diff --git a/Bitkit/Services/PaykitPaymentRequestService.swift b/Bitkit/Services/PaykitPaymentRequestService.swift
index 453b6c9c..12d94eac 100644
--- a/Bitkit/Services/PaykitPaymentRequestService.swift
+++ b/Bitkit/Services/PaykitPaymentRequestService.swift
@@ -140,6 +140,12 @@ struct PaykitPaymentRequest: Identifiable, Hashable {
guard record.state != .activeRecurring else { return .failure(.recurringRequest) }
guard let terms = record.terms else { return .failure(.missingTerms) }
+ #if DEBUG // test hook
+ if case let .fixed(rates) = terms.conversion { // test hook
+ Logger.info("TEST_HOOK_J1 fixed rates=" + rates.map { "\($0.asset)=\($0.value)" }.joined(separator: ",")) // test hook
+ Logger.info("TEST_HOOK_J1 incoming amount=\(terms.amount.value) asset=\(terms.amount.asset) conversion=\(String(describing: terms.conversion)) endpoints=\(terms.acceptedPaymentEndpointIdentifiers)") // test hook
+ } // test hook
+ #endif // test hook
guard terms.recurrence == nil else { return .failure(.recurringRequest) }
let paymentDeadline: PaykitPreciseInstant?
if let deadline = terms.paymentDeadline {
@@ -768,20 +774,35 @@ struct PaykitPaymentRequestService {
else {
throw PaykitPaymentRequestError.requestUnavailable
}
+ var quoteAmount = WalletViewModel.formatBitcoinAmount(sats: draft.amountSats) // test hook
+ var quoteAsset = PaykitIssuerInterop.bitcoinAsset // test hook
+ var quoteConversion: Paykit.PaymentConversion? // test hook
+ var quoteEndpoints = acceptedPaymentEndpointIdentifiers // test hook
+ #if DEBUG // test hook
+ if draft.note == "J1 USD" || draft.note == "J1 RAIL" { // test hook
+ quoteAmount = draft.note == "J1 USD" ? "10" : "0.001" // test hook
+ quoteAsset = draft.note == "J1 USD" ? "usd" : "btc" // test hook
+ quoteConversion = draft.note == "J1 USD" // test hook
+ ? .fixed(rates: [.init(asset: "btc", value: "0.000021")]) // test hook
+ : .fixed(rates: [.init(asset: "btc", value: "2"), .init(asset: "btc-regtest", value: "0.5")]) // test hook
+ quoteEndpoints = ["btc-regtest-p2wpkh"] // test hook
+ Logger.info("TEST_HOOK_J1 outgoing amount=\(quoteAmount) asset=\(quoteAsset) conversion=\(String(describing: quoteConversion)) endpoints=\(quoteEndpoints)") // test hook
+ } // test hook
+ #endif // test hook
let metadataData = try JSONSerialization.data(withJSONObject: ["note": draft.note])
let metadataText = String(decoding: metadataData, as: UTF8.self)
let terms = try Paykit.PaymentRequestTerms(
amount: Paykit.PaymentRequestAmount(
- value: WalletViewModel.formatBitcoinAmount(sats: draft.amountSats),
- asset: PaykitIssuerInterop.bitcoinAsset
+ value: quoteAmount, // test hook
+ asset: quoteAsset // test hook
),
paymentReference: Paykit.PaymentReference(text: "bitkit-\(UUID().uuidString)"),
proposalExpiresAt: Self.timestamp(draft.expiresAt),
recurrence: nil,
- acceptedPaymentEndpointIdentifiers: acceptedPaymentEndpointIdentifiers,
+ acceptedPaymentEndpointIdentifiers: quoteEndpoints, // test hook
paymentEndpoints: nil,
requiredAppId: "bitkit",
- conversion: nil,
+ conversion: quoteConversion, // test hook
paymentDeadline: nil,
metadata: Paykit.PrivateJsonObject(text: metadataText)
)Test J3
Passed in round 2 on m1a-ios-1 after round 1 failed on m1a-ios-2 because the driver restarted the process.
J3.mp4 | J3-r2.mp4 |
Note
This result is for f3a2d12, the commit the tests ran on. The pull request has moved to 6db1097 since; its new commits get their own review, and the tests that follow drive what they can affect.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Suggestion: 👍 Approve
Interim test report for the review; the rest resumes on its own.
QA:
🟠 Tests 1, 2, J2
Test 1
Incomplete: the run ended before this test finished.
Test 2
Incomplete: The lane connection closed before controlled synchronization began.
Test J2
Incomplete: the run ended before this test finished.
🟢 Tests J1, J3
Test J1
USD incoming terms were 10 usd with fixed btc=0.000021 and btc-regtest-p2wpkh: ready confirmation showed 21,000 sats and $17.32, while 200,000 sats locally displayed $164.98 ($164.98*21000/200000=$17.3229). The next original request was 0.001 btc with btc=2 and btc-regtest=0.5: pending row and ready confirmation showed 50,000 sats and $41.19, not 100,000 or 200,000. GRAB contained arrow-right and check-mark with no spinner on both ready screens. Both were dismissed by dragging the visible Payment Request title down; no swipe payment or broadcast occurred, balance remained 200,000. An earlier container drag had not dismissed USD; the later rail request waited behind it, then was opened once from its existing pending row. shots/J1-2.png documents that intervening USD screen, not the rail. A logger-only rebuild read both existing SDK records to expose redacted rate values; neither request was resent. Recording shows visible preparation progress for <=2.5s USD and <=1.7s rail; rail Pay transition to ready <=2.5s. All timing bounds are from video frames, not driver roundtrip.
Ran on f3a2d12.
Test J3
Contact Saved showed qa887-j3-r2-peer, then Home and the existing Bitkit icon preserved PID 20144 after the five-second wait. The saved contact opened Request Or Pay on the first Pay tap with Request enabled, and Request opened the Payment Request amount screen. Closing without sending returned to Contacts with exactly one row for the peer.
Ran on f3a2d12.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Pushed c22ccf8 for the matching concern from Android review. iOS already clears pending withdrawal synchronously, but a membership change during private preparation could leave the current contacts unscheduled. Enablement now retries with the current snapshot while retaining sharing/session ownership. Cancellation cannot report success without preparation; lost ownership remains a no-op. Gated regressions failed before each fix and now pass. Final simulator test build and 225 focused native tests passed. Complete format/translation reports and compiler-warning contexts match the verified baseline, with no introduced diagnostics. Independent final review is clean; all PR commits are signed. The mirrored Contact synchronization overlap notes include pending-withdrawal enablement. That controlled device test and current-head journeys remain unchecked. This is a scheduling correctness fix, not measured linking-performance improvement. |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 3 files.
No new findings; the rest is in the review.
The pair PR synonymdev/bitkit-android#1451 covers the same pending-withdrawal overlap; iOS retries private preparation because its cleanup flag update is synchronous.
QA:
Tests running.
Test 1 failed at f3a2d12.
Tests J1, J3 passed at f3a2d12.
J2 not tested because mandatory device wipes consumed the request setup time; missing: a linked issuer request to the paid address.
Replies:
@ben-kaufman: Restored the exact previously reviewed head 044f5de and its original rc71 scope. (comment)
Your later rebase explanation supersedes the rc71-only restoration: the current PR retains the combined pricing and rc72 linking scope. Device checks remain queued.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at c22ccf8, covering the contact-linking changes since my last review. Two MEDIUMs inline, both reproduced by unit tests on a simulator at this head, and one question about a platform difference. The latest commit (fix: prepare current contacts when enabling sharing) does not touch the code either finding sits in.
link-contact-after-resume.xml needs two devices with separate Pubky identities and contact payments on; the simulator identity here has a session without Paykit capabilities.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 4 files.
No new findings; the rest is in the review.
The pair PR synonymdev/bitkit-android#1451 also keeps invalid peer metadata distinct from link recovery; iOS now matches that handling.
QA:
Tests running.
Replies:
@ben-kaufman: Restored the exact previously reviewed head (comment)
The later rebase retained the combined pricing and rc72 scope; I reviewed the follow-up against that scope. Tests are queued.
@ben-kaufman: Rebased onto current master (comment)
The contiguous delta preserves the reviewed pricing changes. Tests are queued.
@ben-kaufman: Manual test 1 checks that admitted work completes safely (comment)
I kept retry ownership and duplicate advancement separate from the documented latency report. Tests are queued.
@ben-kaufman: Applied the matching stale-contact synchronization protection (comment)
The membership and cleanup protections remain intact in this delta. Tests are queued.
@ben-kaufman: Enablement now retries with the current snapshot (comment)
The current-contact snapshot retry and cancellation check remain intact. Tests are queued.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Interim test results for 502430d The run is still going; the final review follows when it ends. This is a progress note, nothing is approved or requested here. This comment is updated as tests finish and removed when the final result posts. 3 passed so far, 1 not tested. 🟠 Test 2: not tested because simulator controls disappeared after relaunch; missing: usable simulator controls; waits for round 2On 502430d + test hook: simulator controls disappeared after relaunch before synchronization overlap could run test hookNot pushed. Holds public synchronization and private preparation; logs explicit retry IDs and deadlines without changing decisions. diff --git a/Bitkit/Services/ContactPaymentsService.swift b/Bitkit/Services/ContactPaymentsService.swift
index cbf87265..057c62fa 100644
--- a/Bitkit/Services/ContactPaymentsService.swift
+++ b/Bitkit/Services/ContactPaymentsService.swift
@@ -26,6 +26,17 @@ enum ContactPaymentsService {
static func live(wallet: WalletViewModel) -> Operations {
Operations(
syncPublicEndpoints: { publish, isChangeCurrent in
+ #if DEBUG // test hook
+ if publish, ProcessInfo.processInfo.environment["QA_HOLD_SYNC"] == "1" { // test hook
+ let release = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-sync-release") // test hook
+ Logger.info("QA_SYNC publication held", context: "TestHook") // test hook
+ let deadline = Date().addingTimeInterval(90) // test hook
+ while !FileManager.default.fileExists(atPath: release.path), Date() < deadline { // test hook
+ try await Task.sleep(nanoseconds: 100_000_000) // test hook
+ } // test hook
+ Logger.info("QA_SYNC publication released current=\(isChangeCurrent())", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
try await PublicPaykitService.syncPublishedEndpoints(
wallet: wallet,
publish: publish,
@@ -34,7 +45,18 @@ enum ContactPaymentsService {
)
},
preparePrivateEndpoints: { contactPublicKeys, requireImmediatePublication, isChangeCurrent in
- await PrivatePaykitService.shared.prepareSavedContacts(
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.environment["QA_HOLD_PRIVATE"] == "1" { // test hook
+ let release = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-private-release") // test hook
+ Logger.info("QA_SYNC private held keys=\(contactPublicKeys) current=\(isChangeCurrent())", context: "TestHook") // test hook
+ let deadline = Date().addingTimeInterval(90) // test hook
+ while !FileManager.default.fileExists(atPath: release.path), Date() < deadline { // test hook
+ try? await Task.sleep(nanoseconds: 100_000_000) // test hook
+ } // test hook
+ Logger.info("QA_SYNC private released current=\(isChangeCurrent()) cleanup=\(UserDefaults.standard.bool(forKey: PrivatePaykitService.cleanupPendingKey)) enabled=\(UserDefaults.standard.bool(forKey: PrivatePaykitService.publishingEnabledKey))", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+ return await PrivatePaykitService.shared.prepareSavedContacts( // test hook
contactPublicKeys,
wallet: wallet,
requireImmediatePublication: requireImmediatePublication,
diff --git a/Bitkit/Services/PrivatePaykitService+Contacts.swift b/Bitkit/Services/PrivatePaykitService+Contacts.swift
index a6cc3ccb..40bc9863 100644
--- a/Bitkit/Services/PrivatePaykitService+Contacts.swift
+++ b/Bitkit/Services/PrivatePaykitService+Contacts.swift
@@ -970,6 +970,9 @@ extension PrivatePaykitService {
retry.retryIndex = -1
unavailableLinkRetryAt[publicKey] = nil
pendingMessageDrainRetries[publicKey] = retry
+ #if DEBUG // test hook
+ Logger.info("QA_SYNC explicit id=\(retry.id) deadline=\(String(describing: retry.foregroundUntil))", context: "TestHook") // test hook
+ #endif // test hook
startPendingPrivateMessageDrainRetries(reason: "contact link")
}
@@ -1278,6 +1281,10 @@ extension PrivatePaykitService {
}
func rememberSavedContacts(_ publicKeys: [String], replacing: Bool) -> [String] {
+ #if DEBUG // test hook
+ Logger.info("QA_SYNC membership before=\(knownSavedContactKeys.count) incoming=\(publicKeys.count) replacing=\(replacing)", context: "TestHook") // test hook
+ for retry in pendingMessageDrainRetries.values { Logger.info("QA_SYNC retained id=\(retry.id) deadline=\(String(describing: retry.foregroundUntil))", context: "TestHook") } // test hook
+ #endif // test hook
let previousKeys = knownSavedContactKeys
let normalizedKeys = normalizedSavedContactKeys(publicKeys)
if replacing {Tests J1, J3 passed at f3a2d12. J1, J3 are rechecked on 502430d later in this run. Test J2 passed at 6db1097. J2 is rechecked on 502430d later in this run. The other 2 tests run next. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 860c7e8: every finding from my last two rounds is fixed and no thread of mine is open.
- Never-ending retry for a peer with no Paykit records: the drain arms the cooldown on
NotFound, and a missing peer with nothing queued is retired. Queued outbound work or an incomplete snapshot keeps the retry. - Transport cooldown: retirement now depends on a
NotFoundseen in the same attempt, so a contact saved during a transport error keeps its explicit retry. - Identity reads: down from 11 to at most 7 per idle attempt. The remaining reads each follow an SDK call during which the identity can change, so I withdrew the request to go lower.
link_observation_failedis no longer treated as recovery-required, matching Android.
The reproductions for these ran on a simulator at c22ccf8 and 502430d; the last commit I checked by reading, and it is the fix I had run with all 80 PrivatePaykitServiceTests passing. Run Tests was still running at this head.
link-contact-after-resume.xml needs a build of this branch and two identities that have never linked. The same journey passed on Android for the twin PR: synonymdev/bitkit-android#1451 (review).
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 3 files.
No new findings; the rest is in the review.
The pair PR synonymdev/bitkit-android#1451 also retires explicit retries only after NotFound. This fix keeps transport cooldowns from being treated as NotFound on iOS.
QA:
Tests running.
Test 1 failed at f3a2d12.
2 not tested because the controlled re-add check exceeded its setup budget; missing: a verified fresh retry and endpoints after release.
Tests J1, J3 passed at f3a2d12.
Test J2 passed at 6db1097.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
4 participants
Pricing counterpart: synonymdev/bitkit-android#1437
Twin: synonymdev/bitkit-android#1451
This PR applies fixed BTC prices to incoming one-time requests and prioritizes explicitly added or refreshed contacts, using published Paykit rc72.
Description
Out of Scope
Design
N/A - no UI changes; existing payment and contact screens are reused.
Preview
Not captured. Amounts and availability are shown in the existing screens.
QA Notes
Journeys
fixed-price-bitcoin.xml- verify USD-to-BTC, same-asset rail pricing and local fiat display on the current head.unpayable-endpoint.xml- verify rejected-destination feedback, manual retry and a later payable request.link-contact-after-resume.xml- reach Request or Pay after background/resume without re-adding the contact; record latency separately.Manual Tests
conversion.fixed, such as the standalone fixture sender. The basic BTC fixture and Bitkit Request UI cannot issue these terms.journeys/contacts/README.md: hold synchronization, save or re-add a contact, then release it; the saved contact, explicit retry deadline and endpoint assignment survive. Repeat with an identity change and with private withdrawal pending while sharing is re-enabled. Controlled blocking and retry-state inspection are not journey-runner capabilities; automated coverage is not a device pass.journeys/contacts/README.md: a NotFound retry retires without queued delivery, transport failures retain retries through cooldown, pending delivery is preserved, and identity changes during reads stop stale work. Invalid peer metadata must not appear as ordinary link recovery. Controlled SDK responses are not journey-runner capabilities.Without the issuer prerequisites, report the corresponding journey as blocked, not failed or passed.
Automated Checks
PaykitPaymentRequestServiceTests.swift- cover fixed prices, rail selection, exact rounding and precision, invoice/payment matching, invalid rates, unsupported subscriptions and unpayable-request retry.PrivatePaykitServiceTests.swift- cover independent backoff, priority expiry, preparation ownership, identity/deletion/lifecycle invalidation, failure recovery, fairness, read-only snapshot reuse, missing-peer retirement without dropping queued delivery, and fresh identity validation after reads.ContactPaymentsServiceTests.swift,ContactsManagerTests.swift,PaykitContactLifecycleTests.swiftandPrivatePaykitServiceTests.swift- cover saves and deletions during sharing enablement, obsolete cleanup at publication/SDK boundaries, retained retry ownership, ordinary row refreshes completion of admitted writes, and cancellation during sharing enablement.ad3c72248d18587bb5b6ef3c99b063fa9bf31551; no local SDK override.The current retry and contact-synchronization changes passed the simulator test build and 228 focused native tests. Each new regression was reproduced against its preceding production source. The final suite includes transport-cooldown retry retention, queued-delivery preservation, identity changes during reads, a cache update during cleanup's final session check, membership changes during enablement and cancellation before private preparation. Complete format and translation reports match the verified baseline (18 formatting findings and 1,862 translation warnings); compiler diagnostics are also verified pre-existing. Independent final review found no remaining scoped issue. On
860c7e83, CI unit tests, integration tests, the E2E build and translation validation passed. The required E2E run is queued; controlled device cases and current-head journeys remain unchecked above.Earlier reviewer device results confirmed 21,000 / 50,000 / 50,001 satoshi recipient outputs and matching issuer proofs. Exact source provenance was not independently verified. The latest QA review reports J2 passing at
6db1097, while requesting a fresh J3 run after the contact-synchronization changes. Lightning remains unverified. Two earlier controlled background/resume checks retained pending retries, but used SDK-boundary stubs rather than Homeserver durability or process-death faults. These are historical results, not current-head device passes.No end-to-end linking speedup is claimed. Eligibility lookups remain bounded and can wait for a later ordinary refresh. Startup lock contention and overall mobile performance remain open.