fix: preserve contact payment setup errors

[ben-kaufman]

Twin: synonymdev/bitkit-android#1444
Related: pubky/paykit-server#54

This PR reports identity-key access errors during contact payment setup instead of silently completing with private sharing disabled.

Description

• Propagates local and shared Ring key read or decoding failures to the existing setup error handling before changing preferences or publishing endpoints.
• Preserves public-only setup for missing or non-matching keys, cancellation, and successful retry; disabling sharing does not require reading credentials.

The reported reader registry confirms that private payments were advertised as disabled, but does not establish why. These tests verify the setup failure path, not the original incident's cause. Successful setup already republishes the registry after saving the sharing preference; no additional publication defect was verified.

Out of Scope

• Existing sharing preferences are not repaired automatically; public-only sharing remains supported.
• SDK dependencies and Locks/server reconciliation are unchanged.

Design

N/A - no layout changes. Existing setup error handling is reused.

Preview

N/A - existing setup screen and error toast are reused.

QA Notes

Journeys

N/A - not drivable; see Manual Tests.

Manual Tests

• 1 Pay Contacts -> inject a local or adopted Ring key read failure -> Continue stays on setup with an error, without changing sharing preferences or publishing endpoints; restore key access and retry -> private sharing is enabled and the registry reflects it. Controlled keychain-access failure injection is not in Capabilities.

Automated Checks

• updated ContactPaymentsServiceTests.swift - access failure and cancellation leave preferences and publication untouched; retry succeeds, public-only setup remains supported, and disabling skips key reads.
• updated PubkyProfileManagerTests.swift - matching local and adopted keys grant access, missing or mismatched keys do not, and unreadable or malformed local data raises an error. Uses the real test keychain for malformed data and successful retry.
• updated SharedPubkyKeychainTests.swift - missing records return no access; read failures and malformed records throw; valid records still require identity matching.

Local validation: 180 focused native tests and simulator compilation passed. Complete formatting and translation reports match the unchanged base: 18 existing formatting findings and 1,862 translation warnings, with no introduced findings. Compiler diagnostics were compared against a fresh baseline build, with no introduced warnings. Controlled device keychain-failure injection and the original reader incident were not reproduced.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Refactors payment setup error handling for contact payments.

This PR appears safe to merge; no actionable issues were found.

What we checked:

• Failed key reads leave setup unchanged: The throwing access check runs before contact loading and the call that changes preferences or endpoints. The setup view catches the error instead of advancing.
• Tests keep wallet keys separate: Under tests, Keychain uses a separate account name. The identity-storage helper also saves and restores the test entries.

Summary

Contact payment setup now reports identity-key read and decoding errors instead of silently disabling private sharing.

• Checks credentials before changing preferences or publishing endpoints.
• Keeps public-only setup for missing or mismatched keys.
• Skips credential reads when disabling sharing.
• Adds tests for failures, cancellation, retry, and identity matching.

ben-kaufman explicitly leaves automatic repair of existing sharing preferences and SDK/server reconciliation out of scope. The PR does not claim to establish the original incident’s cause.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Change contact payment sharing] --> B{Enable sharing?}
  B -- No --> C[Remove published endpoints without reading keys]
  B -- Yes --> D[Read local or adopted identity key]
  D -- Read or decoding error --> E[Show existing setup error; leave preferences unchanged]
  D -- Missing or mismatched key --> F[Continue with public-only sharing]
  D -- Matching key --> G[Load contacts and prepare private sharing]
  F --> H[Publish public endpoints and save sharing preference]
  G --> H

Loading

_{Reviews (1) · Last reviewed commit: "fix: preserve contact payment setup erro..." · Reviewed by Greptile}

[talosmachina]

No findings. Contact payment setup now reads the identity key itself and turns a keychain read or decode failure into a setup error. Before, those failures fell back to public-only sharing. Missing or mismatched keys still give public-only, and disabling reads no key. Reviewed 140c0bdd, full tier, reasoned from the code: iOS does not build on this reviewer's Linux host, so nothing here was compiled or run.

What I checked, and 5 candidates I ruled out

Read in full: the diff, ContactPaymentsService.setEnabled (both overloads), PubkyProfileManager.activeSecretKeyHex / hasLocalSecretKey, SharedPubkyKeychain and Keychain.load.
Call sites traced: ContactPaymentsService.setEnabled(_:pubkyProfile:...) from PayContactsView.continueFlow and GeneralSettingsView.updateContactPayments. Both already toast a thrown error, so the new failure reaches the user through the existing path.
CI: validate, detect-changes and Greptile passed. Run Tests, Run Integration Tests and build-local were still pending when I posted.

Ruled out

• loadSecret changes meaning for its other callers: it is now try? readSecret(...). A read error, a malformed record and a mismatched pubky all still return nil, as isValidSecret did before.
• A non-matching local key now falls through to the adopted one: the if let localSecret ... else if let adopted chain only falls back when the local value is missing or empty, the same order as activeSecretKeyHex. The (otherSecretKeyHex, secretKeyHex, false) test case pins it.
• An access failure leaves sharing half-applied: the read happens before acquireOperation() and before any default is written, and the test compares the whole defaults dictionary before and after.
• A cancelled or superseded change keeps going after the read: Task.checkCancellation() runs on both sides of the synchronous read, and isChangeCurrent() still guards everything after it.
• Publication later disagrees with setup: canPublishPrivateEndpoints and canUsePrivateLinks still use the error-swallowing hasLocalSecretKey. Setup is now stricter than publication, never looser, so private endpoints cannot be published on a key that setup refused.

Merge confidence: 4/5, the build and unit tests were still running in CI and this review compiled nothing.

[jvsena42]

Code scan at 140c0bd: no findings. build-local, Run Tests and Run Integration Tests were still pending, so I am holding the approval until they are green.

Checked:

• What was swallowed: on master every keychain failure on the local pubky_secret_key item or the shared Ring record collapsed to false, so setup silently persisted private sharing off and published public-only. hasPrivatePaymentAccess(for:) now throws on those.
• Order: the key read at ContactPaymentsService.swift:89 comes before the contacts load, the operation lock, every defaults write and every publication. A throw leaves defaults and remote endpoints untouched.
• Public-only is preserved for a missing key (errSecItemNotFound) and for a key that belongs to another pubky. Disabling reads no keys.
• Keychain access is unchanged: same items, account, access groups and accessibility, and no writes. loadSecret stays nil-on-failure for Ring adoption and activeSecretKeyHex, and the background publication gates keep the non-throwing check, so nothing starts throwing in the background.
• Local-then-adopted precedence matches activeSecretKeyHex.
• No secret material in errors or logs: the thrown values are fixed strings.
• A nil currentSession during a session mutation returns before any key read; the toggle reverts and Pay Contacts does not navigate.
• Same contract as synonymdev/bitkit-android#1444: fail before persisting.

Not a finding, a suggestion: readSecret drops the OSStatus when it throws (SharedPubkyKeychain.swift:82-90), while Keychain.load and publishOwn log theirs. Logging it there would make a Ring-record read failure diagnosable.

Device gate: no device check needed (failure path only; keychain fault injection is not drivable).

[jvsena42]

Approving at 140c0bd: no findings. build-local and Run Tests pass at this head; Run Integration Tests and e2e were still pending.

Checked:

• What was swallowed: on master every keychain failure on the local pubky_secret_key item or the shared Ring record collapsed to false, so setup silently persisted private sharing off and published public-only. hasPrivatePaymentAccess(for:) now throws on those.
• Order: the key read at ContactPaymentsService.swift:89 comes before the contacts load, the operation lock, every defaults write and every publication. A throw leaves defaults and remote endpoints untouched.
• Public-only is preserved for a missing key (errSecItemNotFound) and for a key that belongs to another pubky. Disabling reads no keys.
• Keychain access is unchanged: same items, account, access groups and accessibility, and no writes. loadSecret stays nil-on-failure for Ring adoption and activeSecretKeyHex, and the background publication gates keep the non-throwing check, so nothing starts throwing in the background.
• Local-then-adopted precedence matches activeSecretKeyHex.
• No secret material in errors or logs: the thrown values are fixed strings.
• A nil currentSession during a session mutation returns before any key read; the toggle reverts and Pay Contacts does not navigate.
• Same contract as synonymdev/bitkit-android#1444: fail before persisting.

Not a finding, a suggestion: readSecret drops the OSStatus when it throws (SharedPubkyKeychain.swift:82-90), while Keychain.load and publishOwn log theirs. Logging it there would make a Ring-record read failure diagnosable.

Device gate: no device check needed (failure path only; keychain fault injection is not drivable).

[ovi-reviewer]

Suggestion: 👍 Approve

Review: diff 7 files.
Android merged synonymdev/bitkit-android#1444 and its code on origin/master propagates private-access failures before settings updates. This iOS change brings contact payment setup into line with that behavior while retaining its native keychain reader.

QA:
Tests running.

---

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

[ovi-reviewer]

Verdict: ✅ Approve

Review: diff 7 files.
Android merged synonymdev/bitkit-android#1444 and its code on origin/master propagates private-access failures before settings updates. This iOS change brings contact payment setup into line with that behavior while retaining its native keychain reader.
Manual Test 1 is checked in the PR description.

QA:
Tested on iOS 26.5 simulator.

🟢 Test 1

Test 1

Pay Contacts -> inject a local or adopted Ring key read failure -> Continue stays on setup with an error, without changing sharing preferences or publishing endpoints; restore key….

1.mp4

1-2.mp4

Tip

Test 1 worth a journey

Test 1

• Create a wallet and Pubky profile, then reach Pay Contacts.
• Trigger a local identity-key read failure and tap Continue.
• Verify the error and that Pay Contacts remains open without changing sharing preferences or the registry.
• Restore key access, return to Pay Contacts for the same identity, and tap Continue.
• Verify Profile opens, private sharing is enabled, and the registry advertises private payments.

---

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

[piotr-iohk]

QA review

Scope: full PR diff against its merge base at 140c0bdd, including credential lookup, setup callers, publication ordering, cancellation, retry, and regression assertions. Android comparison covered only the setup-access contract at b059b9d3, not the full Android PR.

No new actionable code findings.

Source analysis supports propagating unreadable or malformed credentials before sharing preferences or endpoint publication change, while preserving public-only setup for missing or mismatched keys and credential-free disabling. Dependency validation was inspected at Paykit rc70. E2E coverage asserts successful profile setup but does not provide controlled credential-read fault injection.

Validation: git diff --check and the contract helper's result and publication-preview validation passed locally. GitHub reports successful native unit tests, integration tests, and compilation at the reviewed head. E2E remains queued. Native tests were inspected, not executed locally; credential-failure injection and the original incident were not reproduced in this review.

Suggested additional test cases

• iOS: with Paykit UI enabled, a signed-in profile, and contact sharing off, inject a credential read failure and enable Pay Contacts in General Settings. Expect an error, the toggle returning to off, and unchanged sharing preferences/endpoints. Restore access and retry; expect sharing to enable successfully.

Device testing: not performed in this review.