Skip to content

fix: stop automatic profile deletion replay - #1122

Open
ben-kaufman wants to merge 10 commits into
fix/backup-observer-lifecyclefrom
fix/profile-delete-session-recovery
Open

ben-kaufman wants to merge 10 commits into
fix/backup-observer-lifecyclefrom
fix/profile-delete-session-recovery

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Current parent composition — October 28

Signed additive merge f3df6cdad712d31a5a223b746e0211860900745d incorporates #1123 at 566b2ff8ba7abdd0968544dda5cdba32d8f518f0. The six-file feature delta retains its original behavior. The two test-file adaptations only remove retired cache fields and use the parent's current initializer; existing assertions and the three-second navigation deadline are unchanged. Final combined tree: 9561f76c660ab67f004c0d0f4a389252d071e7dd.

The selected native gate passed 2,827 unit and 39 UI executions, with three disabled Trezor skips, zero failures and no failed-test retry. SwiftFormat 0.64.0 passed 0/821, with 92 excluded files. All 1,645 frozen inputs match. Complete logs, all 2,869 nested execution summaries and exported diagnostics were inspected; two successful cases omitted by the wrapper's line parser were reconciled against the raw log and XCResult. Two AppIntents notices and 15 nested QoS issue occurrences / 16 distinct stdout events remain disclosed, without a timing-cause or warning-free claim. The selected-out integration/native-fork cases are not passes.

The independent affected-composition review is clean and unchanged earlier source coverage is retained. All ten commits in the complete live-parent range verify locally and on GitHub (verified=true, reason=valid). Ready status is verified (draft=false). Current native CI passed 2,827 unit + 39 UI executions, with five explicit fixture/hardware skips, zero failures/retries. Formatter passed 0/821. Both execute 65ac6539, whose tree equals this head. All 2,871 nested summaries, complete logs/diagnostics, annotations and checksum/CRC-verified archive were inspected. The 571 duplicate-symbol messages cover the same 286-symbol set as prior evidence; actual-parent build configuration/pins match, without an exact-parent runtime baseline claim. One keyboard-queue timeout/backtrace occurred in the passing 7.670-second pending-wallet test; this is retained, with no crash/root-cause claim. Migration is skipped and no E2E pass is credited. This composition does not establish authenticated lost-ack deletion, fix the terminal-identity full-WALLET gap, or causally explain older route-appearance failures.

Prior composed head — 2026-10-27

Signed 162ea52bc5f8af5401bd9bc7ebd6706d573d363f additively incorporates current parent #1123 / e12df99e2e91944308314d82bc8d71e9aaa24f66. The six child files remain byte-identical to the previous child patch. The combined tree passed 2,987 selected unit tests with zero failures/skips and 39 UI executions; three disabled Trezor cases and the excluded expired-fork case are not passes. Full formatting: 0/852. All 1,685 inputs are unchanged. Complete 24,680-line unit and 4,739-line UI logs and nested XCResult diagnostics were inspected. Five unit QoS stacks match the parent exactly; nine UI observations retain the parent category without asserting identical occurrences. An N3 XCTest tap recovered on its second interaction attempt and is retained in the evidence.

Two consecutive independent final reviews are clean. All nine commits in the complete live-parent range are locally and GitHub verified. New-head hosted CI is pending. The full-app lost-acknowledgement deletion journey remains separately scoped; earlier navigation failures are not claimed causally repaired by this composition. Historical evidence below keeps its original head.

Twin: synonymdev/bitkit-android#1688
Dependency: #1123

Description

  • Stop restarting the complete profile deletion flow after an error: earlier contact or profile mutations may already have succeeded when their acknowledgement is lost.
  • Remove the deletion-only session refresh helper while retaining normal initialization/recovery and the existing success/failure controls.

Out of Scope

Design

N/A — existing progress, failure options and navigation.

Preview

An uncertain deletion failure remains a failure instead of silently repeating contact cleanup and profile deletion.

QA Notes

Setup

Use committed released dependencies and synthetic identities for unit tests. J1 requires a disposable test identity and a homeserver fault proxy that accepts one profile deletion, drops its acknowledgement and retains a redacted request counter. That complete-app fixture is unavailable; do not use a personal profile, reset wallets or use live funds.

Journeys

  • J1: new delete-profile-uncertain-response.xml — lost acknowledgement, honest failure feedback, no automatic deletion/sign-in replay, cancellation and restart without selecting Retry.

Automated Checks

  • Updated PubkyProfileManagerTests.swift — retained deletion/session guards after removing the replay-only helper.
  • Updated SendConfirmationLifecycleTests.swift — retain navigation state, view/controller hierarchy and screenshots only after the existing appearance assertion fails; preserve its deadline and passing-path scheduling.
  • Updated UsdtWalletUITests.swift through parent fix: serialize backup observation lifecycle #1123 — await hittable back navigation while preserving the existing payment, amount and recipient assertions.

Evidence

Signed additive merge 65774372 adopts #1123 at 1ad665f8; the original five-file profile patch remains byte-for-byte identical against that parent. No published history was rewritten. At that revision, the complete five-commit range verifies locally and on GitHub (verified=true, reason=valid); the previous-base six-commit range also verifies locally. Two consecutive independent composition reviews are clean.

Final combined native build/unit validation passed 2,906 selected tests with zero failures/skips. Full SwiftFormat 0.64.0 checked 847 files with zero violations. The two affected ordinary USDT UI cases passed with unchanged assertions in a bounded comparison after staging reachability preflight. Complete logs and nested XCResult diagnostics were inspected. Two AppIntents notices, four unit QoS observations and four UI QoS observations remain; timing causes are unproven. That revision's hosted tests failed as detailed below.

Android twin #1688 has controlled fail-before evidence: the original 223-test repository suite had two expected counterexamples (uncertain deletion replay and stale refresh reauthentication). Its repaired 219-test repository suite passes, including one-delete/error preservation and held failure after sign-out. iOS source tracing proves the equivalent automatic replay path is removed; native suites are regression coverage, not execution of J1.

The adopted parent repairs the previously captured subscription teardown SIGSEGV and has six released-native rejected-backup/lost-acknowledgement prepare/resume/restore passes at 538f45c7. Its test-only 1ad665f8 follow-up passed hosted run 40162108190: 2,909 unit passes and 39 UI passes, with one native-fixture unit and four UI skips. Those are dependency-revision evidence, not a new profile-deletion journey.

Gaps

  • J1 remains unrun: the authenticated full-app lost-acknowledgement proxy fixture is unavailable. Neither local tests nor existing hosted suites prove that journey. No historical contact-failure attribution is claimed.
  • Prior-head hosted 40158857938 failed the three-second visible-route deadline in testPendingWalletSwitchResumesAfterTags; later preparation/path assertions and all 39 UI executions passed. A concurrent keyboard queue timeout does not establish causation. Three timestamped local diagnostic trials passed without changing assertions; local iPhone 17/iOS 26.5 differs from hosted iPhone 16/iOS 26.2. This parent adoption does not claim to fix that unknown cause.
  • The first composition UI run passed one case and failed recipient entry while the native node remained starting. Saved hierarchy/recording show the network-startup screen; app logs show the VSS-backed builder had not completed and separate staging requests failed. After unauthenticated endpoint preflight, one unchanged comparison passed both cases and logged normal node startup. The initial underlying cause remains unproven; its failure evidence is retained, and no timeout/assertion was weakened.
  • The local-fork payment case was excluded because upstream Arbitrum no longer serves state at its pinned block. A separate healthy fixture with account/nonce preflight is needed; existing wallets and fixture state were preserved.
  • Manual Retry/Disconnect ownership, conditional cleanup and cross-device mutation safety remain the separately tracked work in Bind profile deletion cleanup to the original sign-in bitkit-android#1687 and follow-up fix: stop deletion after incomplete cleanup #1124. No native SDK upgrade or release is included.

Models used

  • Planning/scoping: gpt-6-astra (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh)

Latest hosted failure investigation

Earlier-head 65774372 run 40166925539 failed only SendConfirmationLifecycleTests.testPendingWalletSwitchResumesAfterTags at the three-second destination-appearance assertion. All later wallet/preparation/navigation-path assertions and all 39 UI executions passed; one native-fixture unit and four UI fixtures were skipped. Complete logs and SHA-256/CRC-verified XCResults, including nested issue groups, were inspected. Both this run and the paired profile PR reproduce a keyboard queue timeout after the tag input is removed before keyboard presentation completes. The precise causal ordering and a safe repair remain unproven. A subsequent detached diagnostic checkout at d1745fb9, with trace/failure-capture additions and unchanged product behavior/assertions, passed 2,906 selected tests on a fresh iPhone 16 simulator using CI's exact iOS 26.2 build 23C54 and Xcode 16.4. The affected case completed in 0.737 seconds. Complete local logs and nested XCResult issues were inspected; four QoS observations remain. Local macOS 27 and hardware/load still differ from the hosted macOS 15 arm64 runner. This comparison does not repair the hosted failure or validate the current PR head. No deadline was weakened and no unchanged CI retry was requested. Existing local full-suite passes remain revision-scoped evidence, not a green hosted result.

Current diagnostic follow-up

Signed ca10ace7 adds failure-only XCTest attachments to capture the actual navigation state at the appearance deadline. The three-second deadline, polling interval, assertion and production code are unchanged. A temporary wrong-route expectation still produced both original assertion failures and retained two state snapshots, seven hierarchy records and seven screenshots; the temporary expectation was removed before final validation.

The final candidate passed all 2,906 selected native unit tests, zero failures/skips, on iPhone 16/iOS 26.2 23C54 with Xcode 16.4. Full SwiftFormat checked 847 files with zero violations. Complete 20,319-line logs and nested XCResult issues were inspected: two AppIntents notices and five runtime QoS observations remain, with their timing causes unproven. Both consecutive independent final reviews are clean. The exact six-commit current-base range is verified locally and on GitHub (verified=true, reason=valid). Current-head hosted SwiftFormat passed 0/847, with its complete log inspected; the checked merge tree equals the published head. Hosted tests passed: 2,906 unit executions and 39 UI executions passed, with one native-fixture unit and four UI skips. The complete 5,850-line log and SHA-256/CRC-verified XCResult were inspected, including nested issues. Its 571 duplicate-symbol log entries (SQLite and BLAKE3; 286 distinct structured warnings) match the actual parent run’s symbol multiset; package/project inputs are unchanged. No runtime warnings or failures were recorded in this hosted XCResult. The earlier timeout did not recur, so the new failure attachments were not triggered and its cause remains unproven. This is a diagnostic change, not a demonstrated repair of the hosted timeout; no unchanged CI retry or deadline relaxation was used. The Android twin does not share this XCTest harness.

@ovitrif

ovitrif commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Triage: 3.1.0, with its Android twin synonymdev/bitkit-android#1688.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Removes refreshSessionIfPossible and isSessionRefreshableError, and folds deleteProfileWithSessionRetry into EditProfileView.deleteProfile, so one failed performDeleteProfile shows the failure options instead of signing in again and repeating contact cleanup and deletion. Reviewed 1b8a6db, full tier, as the delta on fix/restore-identity-background (stacked on #1121). Not built here: iOS does not build on Linux, so this is reasoned from the code and CI.

What I checked, and 3 candidates I ruled out

Read in full: EditProfileView.deleteProfile and performDeleteProfile, PubkyProfileManager.performProfileDeletion and deleteProfile(cleanPrivatePaykitEndpoints:expectedSession:)
Call sites traced: refreshSessionIfPossible and isSessionRefreshableError have no reference left in Bitkit/ or BitkitTests/ (grep on the PR head); the three removed tests covered only those two functions
CI: SwiftFormat green; Run Tests still running when reviewed

Ruled out

  • The restructure changes which guard decides whether the failure options appear: an error from performDeleteProfile is still caught inside the operation and gated on publicKey == session.publicKey, as before; an error thrown by performProfileDeletion itself is still gated on currentSession == session. endProfileDeletion still runs on both paths because the inner catch does not rethrow.
  • An expired session now leaves deletion failing until normal recovery: deliberate and stated in Out of Scope, with Retry and Disconnect still offered.
  • Parity with the Android twin: both now make one deletion attempt per user action, and the journey file is identical on both platforms.

Merge confidence: 4/5, the EditProfileView restructure has no test of its own (J1 is unverified) and Run Tests had not finished.

@ben-kaufman
ben-kaufman changed the base branch from fix/restore-identity-background to fix/backup-observer-lifecycle October 25, 2026 01:41

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code scan: no findings.

Checked and clean:

  • With the replay gone, a failed deletion surfaces the failure dialog after one attempt. Local identity is cleared only inside signOut(), after the profile delete and the public-endpoint removal, so an interrupted deletion leaves the user signed in with a retryable state and no orphaned remote data from this path.
  • v3.0.0 holds no persisted pending-deletion state (PrivatePaykitService.swift:90 is an in-memory var), so there's nothing to migrate.
  • Nothing still references the removed refreshSessionIfPossible (instance and static) and isSessionRefreshableError, and startup session recovery stays in resolveSessionInitialization.
  • EditProfileView.deleteProfile() keeps its isDeleting guard and session checks, and performProfileDeletion still resets the deletion flags on every path, so Paykit automation resumes after a failure.
  • Twin synonymdev/bitkit-android#1688 does the same. #1124 edits the same lines and builds on this one.

Device gate (partial): ca10ace — not run yet. profile/delete-profile-uncertain-response.xml needs a homeserver fault proxy. I'll approximate it: airplane mode during the delete, expecting the failure dialog once, exactly one Failed to delete profile and no re-sign-in or second delete; Cancel keeps the profile; Retry online completes; Disconnect from the dialog. A plain successful deletion runs too. Device results will follow in a separate review.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at ca10ace. My code scan stands with no findings, and the device gate found none.

Device gate: ca10ace — iPhone 17 Pro Max simulator, fresh wallet and profile on staging, run through a local homeserver fault proxy that drops the server's response after it applies the request. journeys: profile/delete-profile-uncertain-response.xml 6/6 passed. perf: not relevant; a plain deletion takes 12 s. Figma: n/a; l10n: no UI text changed.

  • After the lost acknowledgement, "Unable to Delete Profile" shows once. Over 70 s there's exactly one Failed to delete profile, one contact cleanup and one endpoint removal, with no sign-in and no second delete. Cancel and a relaunch run no deletion and keep the profile.
  • An online Retry right after fails once with SharedStateBusy, while the server-side write lock from the dropped request is still held. The next Retry deletes the profile and reaches onboarding.
  • Disconnect while the lock is held also fails, keeping the profile, because signOut() removes private endpoints first; that's unchanged here. Once the lock expires, Disconnect signs out in 10 s.

⚠️ Not verified: the proxy can't tell requests apart inside TLS, so the response it drops is the write-lock acquisition's, not the profile DELETE's. The app-side contract (one attempt, no replay) is the same on that path.

Not a finding: after a lost acknowledgement the write lock blocks both Retry and Disconnect for about 2 minutes. The follow-up is tracked in #1687.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⚠️ Not verified — a local lifecycle test fails on both this head and its base.

Findings: None.

QA for ca10ace: iPhone 17 Pro simulator, iOS 26.5, Xcode 27.0.

Test 1: J1 — lost profile deletion acknowledgement ✅ Failure appears once, and Cancel plus cold restart do not replay deletion.

A debug fixture provisioned a disposable authenticated identity and two contacts, then drove the real Edit Profile deletion flow and released native SDK. The proxy confirmed profile DELETE acceptance with HTTP 204 and withheld its response. No SDK result or deletion decision was substituted.

After about 31 seconds, the failure dialog appeared. The call trace and displayed counters showed one contact cleanup and one profile deletion, with no sign-in replay before Cancel. Cancel retained the session and edit route. A normal cold launch reached Home; a further observation of at least 60 seconds contained no new profile deletion. Normal startup session recovery still occurred.

j1-repeat-2x.2x.mp4

Deletion failure, Cancel and normal cold restart, at 2× speed.

This exercises the profile DELETE acknowledgement gap left open in the earlier device report, whose dropped response belonged to lock acquisition.

Test 2: U1 — affected native suites ❌ The profile-manager suite passes; one existing send lifecycle case fails.

testCustomFeeWalletSwitchSurvivesFeeScreenNavigation observes UTXO selection while expecting confirmation and records a preparation count of two instead of one. The same assertions fail at the actual base, 1ad665f8, with the same simulator model, runtime and toolchain. I checked both finalized XCResults; the new failure attachments retain the navigation state, hierarchies and screenshots. This is not a newly introduced finding, but the local validation is not green.

units-2x.2x.mp4

Native suite execution, at 2× speed; the assertion results were checked in XCResult.

Check 1: build and formatting ✅ The unmodified head builds and the changed Swift files pass formatting.

The simulator build, SwiftFormat 0.64.0 on the four changed Swift files, and git diff --check pass.

Full review of 1ad665f..ca10ace, all six changed files. J1 was driven in full. The unchanged USDT navigation cases were not re-driven; I relied on the passing hosted run. The removed refresh helper was checked in source; the failure-only diagnostics executed in Test 2. Manual Retry/Disconnect ownership and cleanup reconciliation remain outside this PR's scope.

Twin: bitkit-android#1688 fix: stop automatic profile deletion replay matches the removal of automatic full-flow replay; the XCTest diagnostics are iOS-only.

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

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full review of the complete PR diff against its merge base 1ad665f, at ca10ace. No saved assessment baseline was available.

No new actionable code findings.

A failed Edit Profile deletion stays on the existing failure alert after one performDeleteProfile attempt. Onboarding navigation runs only after that deletion returns. resolveSessionInitialization still performs startup session recovery, and performProfileDeletion still clears the in-flight deletion gate after the attempt so later Paykit work can resume.

Parity with bitkit-android#1688 at 99b446a covers the same removal of automatic full-flow replay. The shared profile/delete-profile-uncertain-response.xml journey matches. Compatibility with bitkit-ios#1123 at ff999a50 checked only its backup transport retry and one-shot wallet initialization; those paths leave profile deletion and the removed session refresh untouched. Neither comparison was a full review.

Validation: source, deletion tests, and the failure-only navigation diagnostics were inspected at this revision. This review did not run the unit suite or a simulator. Hosted SwiftFormat and Run Tests succeeded on this head. Device testing: not performed in this review.

Original-session ownership for manual Retry and Disconnect remains tracked in bitkit-android#1687. A local testCustomFeeWalletSwitchSurvivesFeeScreenNavigation failure reported on both this head and the base is outside the deletion change.

Reviewer model: grok-4.7
Reasoning effort: high

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full reassessment of the complete PR diff against its merge base 14152f41, at 7f54d7b9. The comparison base moved from 1ad665f8, and that update changes BitkitTests/SendConfirmationLifecycleTests.swift, so earlier coverage was not inherited.

No new actionable code findings.

A failed Edit Profile deletion shows the existing failure alert after one performDeleteProfile attempt. Onboarding navigation runs only after that deletion returns. The removed session-refresh helpers have no remaining callers. resolveSessionInitialization still recovers a saved session on startup and does not delete the profile. performProfileDeletion still ends the in-flight deletion gate after the attempt. The lifecycle-test change keeps the three-second appearance deadline and records the window hierarchy only after that assertion fails; the parent wallet-switch expectations remain in place.

Parity with bitkit-android#1688 at 99b446a covers the same removal of automatic full-flow replay. The shared profile/delete-profile-uncertain-response.xml journey matches. Compatibility with bitkit-ios#1123 at 14152f41 checked only its temporary backup-upload retry and one-shot wallet initialization. Those paths leave profile deletion and the removed session refresh untouched. This is not a full review of #1123.

Validation: the deletion flow, lifecycle diagnostics, journey, and the two comparison revisions were inspected in source. This review did not run the unit suite or a simulator. Hosted SwiftFormat succeeded on this head. Run Tests was still in progress. Device testing: not performed in this review. An earlier J1 device pass was reported on ca10ace, before this parent merge.

Manual Retry and Disconnect remain bound to the original sign-in in bitkit-android#1687. J1 still needs a disposable identity and a proxy that accepts one profile deletion and drops its acknowledgement.

Reviewer model: grok-4.7
Reasoning effort: high

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: None.

QA for 7f54d7b: iPhone 17 Pro simulator, iOS 26.5, Xcode 27.0.

Test 1: J1 — lost profile deletion acknowledgement ✅ Failure appears once; Cancel and cold restart do not replay deletion.

A disposable authenticated identity and two contacts exercised the real Edit Profile flow with this head's released native dependencies. The proxy confirmed profile DELETE acceptance with HTTP 204 and withheld its acknowledgement. No SDK result or deletion decision was substituted.

The failure dialog appeared after about 30 seconds. App and proxy traces confirmed one contact cleanup and one profile deletion, with no sign-in replay before Cancel. Cancel retained the session and Edit Profile route. A normal cold launch reached Home and restored the same identity; a further 74-second observation contained no new profile deletion. Normal startup session recovery still ran.

j1-repeat-2x.2x.mp4

Accepted deletion with a lost acknowledgement, Cancel and normal cold startup, at 2× speed; request counts and restored identity were checked in the traces.

Test 2: U1 — affected native suites ✅ Profile-manager and send-confirmation lifecycle tests pass.

PubkyProfileManagerTests and SendConfirmationLifecycleTests pass on the unmodified head. The previously failing testCustomFeeWalletSwitchSurvivesFeeScreenNavigation now passes; its assertions cover the expected manual coin-selection destination and successful preparation. The finalized XCResult records no failures or skips; one runtime QoS warning remains.

units-2x.2x.mp4

Native suite execution, at 2× speed; assertions and diagnostics were checked in XCResult.

The earlier failure remains recorded in the previous review.

Check 1: build, formatting and hosted coverage ✅ The original head builds, formatting passes, and current-head hosted tests pass.

The original simulator build, SwiftFormat 0.64.0 on the delta's Swift files, and git diff --check pass. I also checked the current-head hosted run, including its passing USDT navigation coverage. Its explicit fixture skips are not passes.

Delta review of ca10ace..7f54d7b, all 75 changed files and reached deletion/session paths. J1 was re-driven because the merged Paykit dependency changes write-lock handling. Inherited payment, backup and receive/layout changes have source and existing automated coverage; this run did not drive their separate fault journeys. No planned case remains unrun. Manual Retry/Disconnect ownership remains tracked in bitkit-android#1687 Bind profile deletion cleanup to the original sign-in.

Twin: bitkit-android#1688 fix: stop automatic profile deletion replay matches the removal of automatic full-flow replay; the XCTest changes are iOS-only.

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

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: No new findings.

QA for 162ea52: The cases below carry earlier device or component results; the relevant paths are unchanged at this head.

Test 1: Lost profile deletion acknowledgement ✅ Failure appeared once, and Cancel and cold restart did not replay deletion.

The prior device report exercised the real profile DELETE with its acknowledgement withheld at 7f54d7b. The deletion, session and dependency paths are unchanged in this delta.

Test 2: Backup lifecycle and foreground recovery ✅ Controlled cleanup, restart and foreground recovery checks passed on the parent.

The parent review tested held cleanup, changes during shutdown, restart, native outage retry and foreground recovery at e12df99. The relevant backup and scene code is identical here. Its component tests do not replace a full authenticated outage journey.

Test 3: Cached USDT Home balance ✅ Cached display, refresh, privacy and expired-rate conversion checks passed in controlled fixtures.

The parent review covers the display, refresh and isolation cases at e12df99; its payment screen was not driven. The earlier controlled-cache report drove the expired-rate conversion and observed rejection. The amount entry and conversion code are unchanged since that run. These used synthetic display data and fixture endpoints, without a payment.

Test 4: USDT receive navigation ✅ The amount page slides back with its typed value and cancels the uncommitted amount.

The receive transition report drove these steps at 51e6253; the receive view is unchanged. The parent review also passed the receive navigation tests at e12df99.

The cache test-isolation issue and terminal-identity backup availability issue are already tracked upstream.

The unmodified 162ea52 simulator build succeeded. Reviewed the contiguous delta, its reached backup, USDT and deletion paths, and prior open feedback. No new device case was driven for the inherited localization and backup notification timing changes.

Twin: bitkit-android#1688 fix: stop automatic profile deletion replay matches the removal of automatic deletion replay.

Reviewer model: Unknown
Reasoning effort: Not exposed
Review execution metadata was not available in the session.


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

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full reassessment of bitkit-ios#1122 at 162ea52 after the merge base changed, with no coverage baseline supplied. The reviewed diff is the complete change from merge base e12df99.

No new actionable code findings.

An uncertain profile-deletion failure keeps the existing failure dialog after one contact cleanup and one profile deletion. Session initialization and recovery remain available. Manual Retry and Disconnect still run against the sign-in that started the deletion; that ownership remains in bitkit-android#1687.

Twin bitkit-android#1688 at 99b446a removes the same automatic full-flow replay. Dependency bitkit-ios#1123 at e12df99 was checked only for its backup write-failure path: a failed write is recorded and rethrown, and that path leaves profile deletion and the removed session refresh untouched. This is not a full review of #1123 or #1688.

Validation: source inspection at those pinned revisions. This review did not run the unit suite or a simulator. Hosted Run Tests on this head failed UsdtWalletUITests.testReceiveAndInvalidPaymentStaySeparateFromBitcoin with Failed to get matching snapshots: Timed out while evaluating UI query. That test file matches the merge base, whose Run Tests passed; the same head run reported 2988 unit executions, 0 failures, and 1 skip. Device testing: not performed in this review.

J1 still needs a disposable identity and a homeserver proxy that accepts one profile deletion and drops its acknowledgement.

Reviewer model: grok-4.7
Reasoning effort: high

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA review

Scope: Full reassessment of bitkit-ios#1122 at f3df6cda, covering the complete diff from merge base 566b2ff8. The comparison base changed; no previous coverage was inherited.

No new actionable code findings.

The deletion path now makes one orchestration attempt per explicit action and retains failure feedback. Startup recovery remains intact. Navigation diagnostics retain the existing assertion and deadline.

The Android comparison confirms matching replay removal and failure controls. Dependency #1123 at the reviewed base was checked only for backup/deletion composition and affected test contracts. Neither comparison is a full companion review.

Existing concerns remain separate: manual Retry/Disconnect ownership and terminal-identity WALLET backup availability. Their mechanisms are present at the merge base and are not repaired by this delta.

Validation: inspected matching hosted tests, including both affected native suites, and SwiftFormat; both passed on a merge tree identical to the pinned head. Local diff whitespace and journey XML/parity checks passed. Swift/Xcode were unavailable locally.

Device testing: not performed in this review. Authored J1 remains unexecuted at this head here; it requires a disposable identity and an authenticated proxy that accepts profile DELETE and drops its acknowledgement. Earlier J1 evidence reports testing 7f54d7b, not this revision.

Reviewer model: gpt-6.1-sol
Reasoning effort: high

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: No new actionable findings.

QA for f3df6cd: The affected child suites ran on the unmodified head on iPhone 17 Pro, iOS 26.5, Xcode 27.0. The other cases retain linked passing reports whose relevant code and dependencies remain unchanged.

Test 1: J1 — lost profile deletion acknowledgement ✅ The retained device report shows one failure, with no deletion replay after Cancel or cold restart.

The prior device report at 7f54d7b accepted the real profile DELETE and withheld its acknowledgement. Deletion, contact cleanup, session recovery, and the Core/Paykit/VSS pins are unchanged here. The LDK update does not alter those paths. I did not repeat this journey.

Test 2: U1 — profile manager and send confirmation regressions ✅ Both affected suites passed in finalized XCResults.

PubkyProfileManagerTests and SendConfirmationLifecycleTests passed without failures or skips, including testCustomFeeWalletSwitchSurvivesFeeScreenNavigation and the pending wallet-switch navigation case. The recording run also passed. The existing appearance deadline and assertions are unchanged. XCResult records one nonfailing QoS priority-inversion warning.

U1-recording-2x.2x.mp4

App-hosted XCTest execution at 2×; assertions were checked in finalized XCResults. Capture gaps remain, so the video does not establish transition timing.

Test 3: metadata cache and backup lifecycle ✅ The retained parent report passes cache, backup and foreground recovery assertions.

The parent report at 566b2ff covers retired-field decoding, cleanup drains, edits during shutdown, manual admission and stopped-USDT retry. Its tested backup/cache files are identical here. Earlier authenticated lifecycle evidence retained by that report keeps its original scope.

Test 4: USDT receive navigation ✅ The retained parent device run passes amount, tab-swipe, invalid-recipient and back-navigation assertions.

The parent receive run exercised the composed receive routes. The tested views and UI test are identical here.

Test 5: USDT manual amount and recipient ✅ The retained parent device run passes decimal entry, amount retention and recipient-preserving back navigation.

The parent manual-entry run used a comma region. The tested amount, recipient and navigation paths are identical here.

Test 6: expired-rate payment conversion ✅ The retained fixture rejects both asset switches and preserves the original asset and amount.

The parent expired-rate run drove the production amount screen with a synthetic aged rate: both directions showed the rate error and kept Continue disabled. Those paths are identical here. This was an unfunded fixture, without payment submission or contact negotiation.

Check 1: build, formatting and hosted coverage ✅ The head builds, pinned formatting and translation validation pass, and hosted tests are green.

The original-head simulator test build, SwiftFormat 0.64.0 on the delta's surviving changed Swift files, translation validation and git diff --check pass. Translation validation retains 15 warnings; the build retains two AppIntents notices. The current-head hosted tests passed; their explicit fixture skips are not passes.

Reviewed the contiguous 162ea52..f3df6cd delta, reached runtime paths and prior feedback, including an independent source pass. The six-file deletion patch remains intact after the parent merge. Inherited Blocktank, fee-bump, notification and profile-fixture changes received source review without new device journeys; removed purchase screens and translations were not exhaustively re-reviewed individually. No selected case remains unrun. Manual Retry/Disconnect ownership remains deferred to bitkit-android#1687 Bind profile deletion cleanup to the original sign-in.

Twin: bitkit-android#1688 fix: stop automatic profile deletion replay matches removal of the full-flow replay and helper, with the same J1 journey. The XCTest adaptations are iOS-specific.


Reviewed by gpt-6.1-sol-high

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants