Repository navigation
fix: refresh contact methods independently - #1566
Conversation
 [Medium impact] The change affects public Paykit publishing and private payment lists shared with saved contacts. Fix the missing public-publication retry marker before merging. Findings
SummaryThis PR stops temporary preparation errors from replacing complete contact payment lists with partial lists.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Prepare enabled payment methods] --> B{Preparation succeeds?}
B -->|Yes| C[Publish complete list]
B -->|No| D[Keep previously published list]
D --> E{Private or public?}
E -->|Private| F[Maintenance prepares details again]
E -->|Public| G[Retry requires a pending flag]
G --> H[USDT failure does not set the flag]
Reviews (1) · Last reviewed commit: "fix: defer incomplete contact endpoint p..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Turns USDT preparation failures (public and private) and non-route-hint private Lightning failures into errors that defer publication, so an incomplete endpoint list can no longer replace what a contact already sees. Reviewed 1c5b490, full tier, from the code and CI (this Android branch was not built here).
What I checked, and 5 candidates I ruled out
Read in full: the diff; buildWalletEndpoints, buildPublicBolt11Endpoint, syncPublishedEndpoints, syncCurrentPublishedEndpoints, refreshPublishedBolt11ForPayment in PublicPaykitRepo.kt; buildLocalEndpoints, currentOrRotatedInvoice, privatePaymentListUpdate, preparePrivatePaymentListReservations and the publication block in PrivatePaykitRepo.kt; UsdtRepo.paymentEndpoint/operation, UsdtService.wallet; the iOS twin, synonymdev/bitkit-ios#1004.
Call sites traced: syncCurrentPublishedEndpoints (AppViewModel refresh and reconcile, both log on failure), syncPublishedEndpoints(publish = true) (ContactPaymentSettingsRepo.enable), refreshKnownSavedContactEndpoints (foreground, polling, channel ready, address type).
CI: build (assemble plus testDevDebugUnitTest), lint, detekt, build-local green; the e2e-tests-local matrix was still pending at review time.
Ruled out
- Public invoice orphaned when USDT throws after Lightning:
preparePublicBolt11Endpointmay cache a fresh bolt11 in settings before the USDT throw, but nothing is acknowledged, and the next sync reuses that cached invoice throughshouldReuseCachedBolt11and publishes it.publicPaykitCleanupPendingis untouched. - Bitcoin publication blocked forever by a USDT outage:
isUsdtPaymentOptionEnabledalready requiresEnv.isUsdtEnabled, andpaymentEndpoint()only builds the wallet locally (keychain,Rpc::new, a sqlite store in bitkit-core), with no network call. A persistent failure therefore needs a local credential or storage fault, and disabling USDT publishing still clears it, as both updated tests show. - One contact's Lightning failure drops the others:
preparePrivatePaymentListReservationscatches per contact, so the other contacts' updates still publish. The failed contact keeps its previous list, andacknowledgeLightningRefreshleaves its key in a forced refresh, so the next trigger retries it. - Cancellation swallowed:
operationusesrunSuspendCatching, andgetOrThrow()rethrows aCancellationExceptionresult. Both tests assert it. - Enable now rolls back on a USDT fault (
enablecallssyncPublishedEndpoints(publish = true).getOrThrow()): this is a behaviour change, but it follows from the PR's intent and needs the same local fault as above. Ruled out narrowly, and no test covers it.
Merge confidence: 4/5, the enable-path rollback on a USDT fault was ruled out only narrowly and is untested, and e2e was still running.
ben-kaufman
changed the title
|
Updated in 5f99653 to address the concern on iOS #1004. USDT preparation no longer blocks Bitcoin/Lightning publication. Its address is derived locally without opening the USDT database or requiring RPC/bundler configuration, with cached reuse limited to matching current wallet credentials. If no current-wallet address can be established, Bitcoin still publishes and existing maintenance retries USDT. Explicit disable removes USDT. Tests exercise the actual public/private builders with the local USDT service/repository: first-time failure, recovery, cached address reuse, wallet changes, disable, cancellation and expired Lightning renewal. The full 4,334-test suite passed, followed by 175 focused tests after final cleanup. Required compile/lint checks passed with no introduced lint findings; existing baseline findings were compared against master. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve: no findings, and all five cases passed at 5f99653.
Findings: none.
QA for 5f99653:
Tested on two Android 16 emulators with a dev debug build on regtest, staging Blocktank and the staging homeserver. USDT ran on the bitkit-docker USDT fixture.
- Wallet A received the payments. It had a Blocktank channel, so it published Lightning.
- Wallet B was linked to A as a contact. It held on-chain funds and 25 USDT, so its funding picker shows whether A's private list offers USDT.
- Endpoint reads: published endpoints were read straight from A's public Paykit records on the homeserver.
- Builds: Tests 1 to 3 ran on a debug-only build of this head with two additions. While a flag file exists,
UsdtService.paymentAddressthrows. Log lines also record each public publish,publicPaykitCleanupPendingafter each acknowledgement, and each private list A builds. Tests 4 and 5 ran on the plain head build.
Test 1: Bitcoin and Lightning publish while USDT preparation fails ✅ Once A's public invoice was paid, A published its on-chain address and a new invoice, without USDT, 15 s later (read from the homeserver).
- Before: A published on-chain, Lightning
…tg0ed50asqpqztduand USDT0x2Cf3…95AC. - Failure and payment: with the USDT failure injected, both builders logged
Deferred USDT endpoint preparation. Blocktank then paid the public invoice 5 000 sats, and A showed Received Instant Bitcoin. - Republished: 15 s later, A published on-chain and a new invoice
…jh4ezw6cqqfmv3c4, without USDT. The homeserver read 13 s after that showed the same list. - Retry marker:
publicPaykitCleanupPendingstayedtrueafter that publish. - While the failure held: A republished once a minute on its maintenance rounds, with the same invoice and no new ones.
- How it was checked: the video shows the payment on A. The endpoint lists come from the homeserver and the device log.
case1-2x.2x.mp4 |
Test 2: USDT returns after recovery ✅ 65 s after the failure ended, A's public list carried USDT again with the same address (read from the homeserver), and B's picker offered USDT again.
- Public list: USDT
0x2Cf3…95ACcame back next to the unchanged invoice and on-chain address, on A's next maintenance round.publicPaykitCleanupPendingthen cleared. - Private list: A built no contact list while its Received Bitcoin sheet from Test 3 stayed open. It rebuilt B's list with USDT 5 s after the sheet closed (device log).
- B's picker: during the failure B opened A's list version 4, and its 25 USDT row was greyed out. After recovery it opened version 5, and the USDT row was selectable. The video shows both, with the idle wait between them cut.
case2-2x.2x.mp4 |
Test 3: a contact pays with Bitcoin during the failure ✅ A's private list for B carried on-chain and Lightning only (device log), and B paid A 5 000 sats from Savings.
- Private list during the failure: A built on-chain
bcrt1qg9wq…and Lightning only for B, according to the device log. - B's payment: from Seller1566's contact screen, B tapped Pay and sent 5 000 sats from Savings to
bcrt1qg9wq…. - Address rotation: A then rotated B's private on-chain address to
bcrt1qexh43…, still without USDT.
case3-2x.2x.mp4 |
Test 4: the published USDT address is the wallet's own ✅ A's USDT Receive screen shows 0x2Cf3aB40c5475F86aC20A7080105DF4d2b3A95AC, the address in A's published USDT endpoint.
The Receive screen takes its address from the opened USDT wallet. The published endpoint now comes from usdtAddress(mnemonic, passphrase) without opening it, and the two match.
case4-2x.2x.mp4 |
Test 5: disabling USDT and changing the wallet ✅ Turning USDT (Tether) off removed USDT from A's public list within 10 s and from B's picker, and after Reset wallet the new wallet publishes its own USDT address (read from the homeserver).
- Disable: A's public list dropped USDT 10 s after the toggle.
- Private list: B then opened A's list version 7, and its USDT row was greyed out again.
- Reset: Reset wallet removed the old profile's public endpoints.
- New wallet: the new wallet's profile publishes
0xB0575fDb1FC869d2e78fb58B7135C6Dd8146BBaF, and its USDT Receive screen shows the same address. - Not driven: a credential change without a reset. The cache fingerprint guards it, and the PR's tests change the passphrase and check it.
case5-2x.2x.mp4 | case5b-2x.2x.mp4 | case5c-2x.2x.mp4 |
Covered: the full diff, 402aecb..5f99653. Device cases drove both builders, the retry marker while USDT was missing, and the locally derived address. Two changes were not triggered on device. Publishing never failed right after a new invoice, so the marker set on save (PublicPaykitRepo.kt:500) was not exercised alone. No case made private invoice creation fail for a reason other than missing route hints, so the new rethrow did not run. CI's build and e2e-tests-local pass at this head.
Twin: synonymdev/bitkit-ios#1004 matches. Both derive the address locally, publish Bitcoin methods while USDT retries, and keep a renewed public invoice pending until it publishes. The empty reconcile owner found on iOS has no Android counterpart: PaykitUsdtPaymentRepo takes the owner from paymentEndpoint().value.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- Address equivalence:
paymentAddressderives throughusdtAddress(mnemonic, passphrase), the same pathwallet()builds from (the m/44'/60'/0'/0/0 EOA, with no factory or chain input). Dropping the wallet open can't advertise a different address. - Cache: the fingerprint covers mnemonic and passphrase (null and
""derive the same key), andwipe()clears it under@Synchronized. - Gating:
paymentAddressdoesn't checkEnv.isUsdtEnabled, but every caller is still gated (isUsdtPaymentOptionEnabled,UsdtPreferencesRepo.enabledin auth approval). - Retry marker:
publicPaykitCleanupPendingandusdtPendingonly make cleanup or republish happen more often, and the extra cleanup is idempotent. - Cancellation: the
CancellationExceptionrethrow plusensureActive()is correct. - Migration: USDT
paymentEndpointisn't in v2.5.1.
Device gate (partial): not run yet. Perf-relevant (repeated work): while USDT is enabled but its endpoint is missing, each maintenance round republishes. I'll count homeserver publications per minute idle on Home against master. No journeys apply; the PR gives a manual test. The device result follows in a second review.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against merge base 402aecb6, at 5f99653e. No saved assessment baseline was available.
No new actionable code findings.
Bitcoin and Lightning receiving details still publish when local USDT address preparation fails. If USDT sharing is enabled and the address is missing, publicPaykitCleanupPending stays set so maintenance republishes and retries USDT; explicit disable ends that wait. Saving a renewed public Lightning invoice sets the same flag before publication, so a failed publish retries the new invoice. Private Lightning failures other than a missing route hint omit that contact update and leave the previously published list in place. A cached USDT address is reused only for the same mnemonic and passphrase, and wipe clears it.
Compared that publication contract with bitkit-ios#1004 at b04c016a: both derive the address locally, publish Bitcoin methods while USDT retries, and keep a renewed public invoice pending until it publishes. That was a comparison of this contract, not a full iOS review.
Validation: this review did not run the unit tests. The CI build ran testDevDebugUnitTest on this head and succeeded. Inspected PrivatePaykitRepoTest, PublicPaykitSharingAdmissionTest, and UsdtEndpointTestWallet. Native usdtAddress was not executed; paymentAddress and the USDT wallet constructor both pass the same mnemonic and passphrase into it.
Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Android, contact sharing and USDT enabled, with the USDT receive screen showing an address. Compare that address with the USDT method on a saved contact. Expected result: both use the same address.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 5f99653. The code scan from my earlier review stands; this review only adds the device result.
Device gate: 5f99653 — journeys: none listed; perf: 0 endpoint publications per minute idle on the PR and on master (180 s foreground on Home and 120–150 s backgrounded, both with the USDT fixture up and with the gateway unreachable), 2 endpoint syncs at startup on both; Figma: n/a (no UI); l10n: no UI text changed.
Method: Pixel_Tablet with Paykit public and private sharing on, USDT enabled against the local fixture (15 USDT) and one linked contact. I ran the PR head and master 402aecb on the same wallet, counted operation=list Payment Endpoint Identifiers per window, and read publicPaykitCleanupPending at the end of each window (false in every run). No Deferred USDT endpoint preparation line appeared, so the new usdtPending → cleanup path never looped. With the gateway unreachable the app logged Failed to refresh USDT with 'NetworkUnavailable', but endpoint preparation still succeeded, because the PR derives the address locally.
paymentEndpoint() fail on this head, so the deferred path needs code injection. That part of the approval rests on the code scan and PublicPaykitRepoTest/PrivatePaykitRepoTest.
CI at 5f99653: green.
5 participants
Refs #1524
This PR lets Bitcoin and Lightning receiving details refresh independently when USDT preparation is unavailable.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A — publication behavior only.
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Automated Checks
PrivatePaykitRepoTest.kt,PublicPaykitSharingAdmissionTest.kt— actual public/private builders cover first-time failure, recovery, Lightning renewal, current-wallet address reuse, wallet changes, explicit disable and cancellation.