Repository navigation
fix: apply fixed btc prices with paykit rc71 - #1437
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
There was a problem hiding this comment.
Verdict: ⛔️ Request Changes
Review: diff 1 file.
synonymdev/bitkit-ios#887 pins the same rc71 release, while iOS master still uses rc70 from synonymdev/bitkit-ios#886. Both apps derive BTC payment amounts without retaining conversion terms, so explicit BTC pricing requires the same intake protection on both platforms.
Findings:
1 inline (1 HIGH)
QA:
No tests ran because the description reports no user-visible change and marks 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
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 8 files.
New findings: 3 inline (3 LOW); the rest is in the review.
synonymdev/bitkit-ios#887 implements the same fixed BTC pricing, rounding limits and conversion-subscription rejection. Its paired journey has the same filename, name and action prose.
QA:
Tests queued.
Note
Retest Suggested J1
@ovi-reviewer retest J1
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
Tests for the review.
QA:
Tested on Android 15 emulator.
0 of 1 passed; blocked by qa env setup.
🟠 Test J1
Test J1
Blocked: Not run: the issuer cannot send fixed conversion rates.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: full PR diff against 484e6dd, reviewed at c36ee53, including pricing, affected payment paths and the rc71 SDK contract.
No new actionable code findings.
The earlier overpayment concern is verified fixed. Targeted comparison with iOS #887 at bfa8db2 found matching pricing and journey contracts; shared platform behavior depends on that companion landing.
Validation: the exact-head CI report records 3,591 passing unit tests, including the four added pricing regressions. Locally, 2,028 isolated Kotlin helper checks passed with data-only SDK types. The local app test run stopped before compilation because its JetBrains 21 toolchain URL returned HTTP 400. The published Maven AAR could not be independently inspected (HTTP 401); rc71 source and FFI contracts were inspected.
Recommended before device testing: provision a linked issuer that can publish fixed conversion terms. The reported J1 attempt could not run because its issuer lacked that capability.
Device testing: not performed in this review.
Suggested additional test cases
- Android/regtest: with distinct linked identities and funded wallets, issue separate 10 USD requests at
btc: 0.000021, then pay through on-chain and Lightning endpoints. Expect 21,000 sats received per request, fees charged separately, and one matching proof per request with the original USD terms retained. - Android/regtest: use the same pricing with an LNURL endpoint whose first invoice callback fails before dispatch. Restart, restore the callback and retry the accepted request. Expect the amount to remain 21,000 sats and exactly one payment/proof after recovery.
|
Tested the on-chain fixed-price flow on Android 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. 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 The small code/test/journey cleanups are addressed in f6dfb3a. Both issuer guides now document the shared 38-significant-digit product limit and the extra exact values iOS can accept. This documents existing behavior; it does not change pricing or rounding. Final validation: 3,591 unit tests and compilation passed; all 19 Detekt findings match existing findings, with no introduced compiler or formatting diagnostics. Both XML copies validate. The fixed-rate issuer/device and funded recovery cases remain unrun; they are not being counted as passes. |
|
@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? |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 4 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-ios#887 carries the same lowercase journey name and action prose, and both issuer guides document Android's 38-significant-digit product limit and iOS's wider exact range.
QA:
Tests running.
Replies:
@piotr-iohk: Tested the on-chain fixed-price flow on Android using the standalone issuer (comment)
@piotr-iohk I have retained your focused on-chain results and the issuer tool in synonymdev/bitkit-e2e-tests#269 for J1. Your report does not independently establish the apps' source commits, so it does not certify this head. The used-address spinner is outside this delta: endpoint preparation and its UI handling are unchanged.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
@piotr-iohk Fixed here in f8ee077, with the matching change in iOS #887 (cdb692ad). The retry path already existed in master: Validation passed all 3,593 unit tests and compilation, 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. |
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at f8ee077 (includes "stop preparing unpayable payment requests"). No HIGH/MEDIUM. Three LOW inline comments, all docs: the parse-reason vocabulary in docs/payment-requests.md, the journey's issuer precondition, and the interop doc's fixture claim (which also carries the "iOS accepts exactly" sentence that synonymdev/bitkit-ios#887 shows to be wrong). 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 (PaykitPaymentRequestRepo.kt:1781-1799 → PaykitBitcoinRequestPricing.bitcoinPayment): rail > asset-wide > parity precedence, cross-asset unpriced rails dropped, duplicate/empty/zero/sign/exponent/whitespace/>80-char/>38-digit operands rejected, exact BigDecimal multiply with a 38-digit product guard, Lightning whole-sat requirement, ULong.MAX_VALUE/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: SendConfirmScreen.kt:354 renders
preparingRequest.amountSats; AppViewModel.kt:4772-4794 refuses to send unlessuiState.amount == request.amountSatsand the bolt11 msats equal sats×1000 (strict equality, Repo.kt:136-142). The originalamountValue("10" USD) only reaches the SDK when no request is attached (PrivatePaykitRepo.kt:593, :664-677). toPaykitSubscriptionreturns null for any conversion terms (PaykitSubscription.kt:301), so no renewal/auto-pay path can use a priced request.- f8ee077: the
NotOpenedbranch (AppViewModel.kt:1325-1329) runs insidefinishIncomingPaymentRequestPreparation, which is guarded byisCurrentPaymentRequestPresentation(request, generation, preparation), sofinishPaymentRequestPreparation(paymentRequestPreparation.value)at :1472 closes this request's sheet only; the id goes intodismissedPreparingRequestIdsso it does not auto-reopen, and the explicit Pay path removes it again (:5989). Covered by the two new AppViewModelSendFlowTest cases. - Line-for-line parity with synonymdev/bitkit-ios#887 except that iOS lacks 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 c36ee53 with the standalone issuer from synonymdev/bitkit-e2e-tests#269 (#1437 (comment)); the commits since are a constant reference, 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 f8ee077: unit tests, detekt, e2e (pending at posting time).
The endpoint_not_payable spinner @piotr-iohk reported is answered by f8ee077; left to that thread.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 5 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-ios#887 carries the same immediate unavailable-endpoint feedback and identical unpayable-endpoint.xml journey.
QA:
Tests queued.
Test J1 failed at c36ee53 (review).
Replies:
@ben-kaufman: Fixed here in f8ee077, with the matching change in iOS #887. (comment)
@ben-kaufman I verified that rejected endpoints now close preparation, retain manual retry and allow the next payable request. The used-address device check remains pending.
Note
Retest Suggested J1
@ovi-reviewer retest J1
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Logs from previous test session: |
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 5 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-ios#887 fixes the same loading issue for unpayable endpoints and adds the matching unpayable-endpoint.xml journey.
QA:
Test J2 needs an issuer that can reuse a paid address. Test J1 was validated by ben-kaufman.
Note
Retest Suggested J1
@ovi-reviewer retest J1
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
@piotr-iohk Checked the attached logs: they confirm repeated endpoint_not_payable on the already-paid address, which the preparation-feedback fix addresses. They also contain SharedStateBusy during the 12:16 relaunch; that is separate from fixed-price calculation. 5c3637d now matches the conservative precision guard in iOS #887. All 3,594 unit tests pass, with no introduced lint/format findings. The paired docs and PR prerequisites explicitly require the conversion-capable issuer from bitkit-e2e-tests#269: the earlier J1 run blocked by the basic issuer is not evidence of an app failure. Your later focused funded results remain recorded, and J2 remains pending a fresh used-address retest. |
jvsena42
left a comment
There was a problem hiding this comment.
Re-checked at 5c3637d: no findings.
PaykitBitcoinRequestPricing.kt: the combined precision bound (amount + rate ≤ 38 afterstripTrailingZeros()) now runs before the multiply and matches the iOS rule in synonymdev/bitkit-ios#887, so both platforms reject the same quotes.docs/payment-requests.mdlistsunsupported_pricingand describes theendpoint_not_payablebehaviour.- The interop doc no longer claims an iOS-only range 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. Build, lint and detekt pass at this head; e2e was still pending.
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 7 files.
No new findings; the rest is in the review.
Pair PR synonymdev/bitkit-ios#887 applies the same precision limit before multiplication and adds matching boundary cases. Both journey files are identical.
QA:
J2 needs a controlled issuer that can reuse a paid address.
J1 was validated by ben-kaufman.
Note
Retest Suggested J1
@ovi-reviewer retest J1
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Spinner fix retested on Android at Passed the
The earlier attempt affected by manual taps was dismissed and excluded; this verdict uses a new request observed without intervening taps. 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
There was a problem hiding this comment.
Approved from our reviewed and tested scope at 5c3637d. 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.
5c3637d to
dc54e6e
Compare



Twin: synonymdev/bitkit-ios#887
This PR updates Paykit to rc71 and applies fixed BTC prices to incoming one-time requests.
Description
Out of Scope
Design
N/A - existing payment UI is reused.
Preview
Not captured. The added journey verifies the amounts in the existing confirmation sheet.
QA Notes
Journeys
fixed-price-bitcoin.xml- reviewer device results cover USD-to-BTC, same-asset rail pricing and local fiat display, with additional funded on-chain checks. Exact source provenance was not independently verified.unpayable-endpoint.xml- verify rejected-destination feedback, manual retry and a later payable request on device.Manual Tests
conversion.fixed, such as the standalone fixture sender. The basic BTC fixture and Bitkit Request UI cannot issue these terms.These prerequisites are not provided by the base journey environment. Without them, report the corresponding journey as blocked, not failed or passed.
Automated Checks
PaykitPaymentRequestRepoTest.kt- cover fixed USD/USDT-to-BTC amounts, same-asset prices, rail precedence, endpoint filtering, exact rounding and precision boundaries, invoice/payment matching, invalid rates, overflow and unsupported subscriptions.AppViewModelSendFlowTest.kt- cover closing rejected-endpoint preparation, feedback, manual retry and progress to another payable request.The reviewer confirmed 21,000 / 50,000 / 50,001 satoshi recipient outputs and one matching issuer proof per payment. These are focused on-chain results, not a full device pass. Lightning and the corrected used-address presentation still need device testing.
Local validation: all 3,594 unit tests passed on the final edits. Compilation passed;
all 19 Detekt findings match the baseline, with no introduced compiler or formatting diagnostics.