test(swift-sdk): add testnet identity-discovery UI test#3560
test(swift-sdk): add testnet identity-discovery UI test#3560llbartekll wants to merge 21 commits into
Conversation
Extract Identifier enum, XCUIElement helpers, and the create/delete wallet flow into Support/ for cross-class reuse. Rewrite the existing testCreateGeneratedWalletFlow on top of the extracted helpers; behavior is unchanged. Add WalletPersistenceTests with two SDK-backed integration tests: - testWalletPersistsAcrossRelaunch validates loadFromPersistor() after a cold restart (SwiftData rehydration + Keychain read). - testWalletDeletionCleanupSurvivesRelaunch guards atomic SwiftData + Keychain mnemonic cleanup; the orphan-mnemonic recovery prompt fires on relaunch if either side leaks. Both new tests use addTeardownBlock + MainActor.assumeIsolated for best-effort wallet cleanup if assertions halt mid-flow. Extend swift-example-app-ui-smoke.yml -only-testing list to include the two new tests; -parallel-testing-enabled NO and -maximum-concurrent-test-simulator-destinations 1 retained. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Imports a wallet from `UI_TEST_TESTNET_MNEMONIC`, runs DIP-9 identity discovery, and asserts the registered identity surfaces with a non-zero balance. Skipped when the env var is unset. Test is scaffolded under CreditTransferTest so the credit-transfer assertion can be re-added as a sibling method (helpers for that flow are already in WalletFlow). Adds accessibility identifiers across the views the test traverses (wallet creation, identities tab, search-wallets sheet, identity detail, options/network picker, transition form) and one accessibilityValue on the balance label so the raw credit count is parseable from XCUITest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds accessibility identifiers across the SwiftExampleApp UI; centralizes test identifier constants and XCUI helpers; implements robust UI-test wallet/identity flows and two new UI test suites; adds AppState network-switch readiness tracking; and updates the CI workflow to schedule and gate UI smoke tests. App & UI Tests — Identifiers, Helpers, Flows
CI Workflow Update
sequenceDiagram
autonumber
participant CI as CI Workflow
participant R as GitHub Runner
participant T as XCUITest Process
participant A as SwiftExampleApp (Simulator)
CI->>R: schedule / manual dispatch
R->>T: launch xcodebuild with TEST_RUNNER_UI_TEST_MNEMONIC
T->>A: drive UI via XCUIApplication
A->>A: AppState.beginNetworkSwitch() (isSwitchingNetwork guarded)
T->>R: upload result-bundle (on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 2624335) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift`:
- Around line 88-110: The status label incorrectly shows "Connected" whenever
appState.sdk != nil even while a network switch is in progress; change the logic
so isSwitchingNetwork remains true until the async switch completes (or derive
connected state from a network-specific readiness signal on the SDK) so that the
UI only shows "Connected" when the new network is actually ready; update the
state transitions in switchAppNetworkToTestnet (and any reset helpers) to
set/clear isSwitchingNetwork around the full async completion and use that
combined with appState.sdk (or a new appState.sdk.isReady flag) to drive the
Label bound to accessibilityIdentifier "options.networkStatusLabel".
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift`:
- Around line 96-101: The test is asserting a live testnet funding floor
(XCTAssertGreaterThan(balance, 0)); instead assert only that a readable balance
was returned: call readIdentityBalanceCredits(in:) and replace the
XCTAssertGreaterThan check with an assertion that the read succeeded (if the
function returns an Optional use XCTAssertNotNil(balance, "..."), or if it
returns an Int use XCTAssertGreaterThanOrEqual(balance, 0, "...") to allow
zero). Update the failure message to say the balance should be readable rather
than non-zero and keep the test focused on discovery/readability rather than
funding.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- Around line 933-969: The negative branch in assertWalletRowVisible(true/false)
can false-pass because it never scrolls and queries app-wide button/text rows
that may be off-screen; change the exists == false path to reuse
scrollToWalletRow(named:in:) to bring the specific wallet row into the viewport
(or obtain the per-wallet XCUIElement) and then assert non-existence against
that element (e.g. wait for row.exists == false or use an
XCTNSPredicateExpectation on the returned row) instead of the current app-level
buttonRow/textRow checks so off-screen elements can't make the assertion pass
incorrectly.
- Around line 492-518: The test currently assumes the picker auto-selects the
imported wallet (walletPicker / Identifier.SearchWallets.walletPicker) which is
flaky; instead explicitly drive selection to walletName: after
walletPicker.waitForExistence, open the picker UI and locate the row/button/cell
whose label equals walletName (or the table cell in
SearchWalletsForIdentitiesView) and tap it to select that wallet, then assert
the selection; alternatively, if simpler, add a deterministic cleanup step
(delete competing wallets) before importing so only walletName remains—reference
walletPicker, walletName, and SearchWalletsForIdentitiesView when updating the
helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 655844a1-5da6-46a1-acaf-193e6c0801cd
📒 Files selected for processing (16)
.github/workflows/swift-example-app-ui-smoke.ymlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SearchWalletsForIdentitiesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionInputView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/Identifiers.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/XCUIElement+Helpers.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/WalletPersistenceTests.swift
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "0ad7a668ff35d6258af4f84f41a4922a0f7fb59dec63540e42490e126c39fb6d"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR that adds accessibility identifiers and a new XCUITest (CreditTransferTest.testImportWalletAndDiscoverIdentity) plus shared helpers extracted from the existing test class. No app-side logic or consensus-critical code is touched. The main concerns are about test reliability: a verifiable race in switchAppNetworkToTestnet (the Connected label can be true via the stale SDK throughout the switch), brittle assumptions about picker default state, and ~200 lines of unused credit-transfer scaffolding likely to rot before the follow-up lands.
Reviewed commit: b3c85a2
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [BLOCKING] lines 375-439: switchAppNetworkToTestnet can return while the app is still on the previous network
`switchAppNetworkToTestnet` waits for `options.networkStatusLabel` to contain `Connected` as proof the switch finished, but the app provides no such guarantee. In `OptionsView` (lines 27-50), tapping the segmented control sets `appState.currentNetwork` and clears `isSwitchingNetwork` once the local resets finish; the actual SDK rebuild runs separately on the `currentNetwork.didSet` task in `AppState` (lines 12-17, 105-126). The status label renders `Connected` whenever `appState.sdk != nil`, and `switchNetwork` never nils the old SDK before constructing the new one (`sdk = newSDK` only after `try SDK(network:)` succeeds). On a simulator left on a non-testnet network from a previous run, the helper can therefore observe `Connected` against the stale SDK and return before testnet DAPI is bound — the discovery step then runs against the wrong backend and the test result is nondeterministic. Gate on a stronger signal: e.g. an SDK-level state transition published from `AppState` or a test-only flag flipped after `switchNetwork` completes, rather than the always-green status label.
- [SUGGESTION] lines 492-519: runIdentityDiscovery relies on "only one wallet exists" without enforcing it
`runIdentityDiscovery` trusts the picker's default-first auto-selection and only sanity-checks `walletPicker.label.contains(walletName)`. The invariant is never enforced: `SearchWalletsForIdentitiesView` queries `@Query(sort: \PersistentWallet.createdAt) hdWallets` unfiltered, and `CreditTransferTest` only deletes the row matching `expectedSenderWalletIdHex` (lines 69-76 of `CreditTransferTest.swift`). Any other persisted wallet from prior local runs still appears in the picker and, being older, becomes the default — the picker-label assertion will then fail loudly even though import and discovery are themselves working. The comment in the helper acknowledges this assumption but the test does not enforce it. Either reset `PersistentWallet` rows for unrelated wallets before discovery (e.g. a `cleanupWalletsByPrefix`-style sweep on entry, or a full SwiftData reset), or drive the picker to explicitly select `walletName` instead of trusting the default.
- [SUGGESTION] lines 624-833: Credit-transfer helpers (~200 lines) are added but unused
`navigateToIdentityCreditTransferForm`, `executeCreditTransfer`, `waitForCreditTransferSuccess`, and `cleanupWalletsByPrefix` are defined but have no callers anywhere in the test suite — grep confirms only the definitions exist. Carrying ~200 lines of unexecuted UI test helpers is risky: the most fragile string matching in the file lives here (`label CONTAINS[c] "manage identities"`, the `"💳 Manually Enter Recipient"` literal, the `transition.input.toIdentityId` identifier, the `"State Transitions"` cell label), and none of it is exercised, so it will silently rot against UI changes until the follow-up tries to use it. Note also that `executeCreditTransfer` calls `app.swipeDown()` after typing the amount — `importWallet` explicitly warns that `swipeDown` can dismiss sheets; the Transfer Credits screen is push-navigated rather than presented so it may not bite, but the inconsistency is worth verifying before the helper is wired in. Either land these helpers together with the test that uses them, or trim to what the discovery test actually needs and re-add the rest in the follow-up PR.
- [SUGGESTION] lines 466-490: Identity-discovery retry loop can close an already-open menu
The retry loop binds `for attempt in 1...3 where !sheetOpened` purely as a counter and then has to do `_ = attempt` to silence the unused-variable warning — that's a tell that the index shouldn't be bound at all (`for _ in 1...3 where !sheetOpened` is clearer). More substantively, every iteration unconditionally calls `addMenu.tap()` again. If the previous attempt actually opened the SwiftUI Menu but the search-item lookup transiently failed, the next `addMenu.tap()` will *close* the menu, putting the retry into a worse state than the first attempt. Probe the search item or the menu's selected state before re-tapping, e.g. only call `addMenu.tap()` when neither `searchMenuItem.exists` nor `addMenu.isSelected`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift`:
- [SUGGESTION] lines 53-59: Recovery-prompt guard runs before the network switch
`failIfRecoveryPromptVisible(in: app, timeout: 2)` runs immediately after `app.launch()`, then `switchAppNetworkToTestnet` runs. A wallet left over from a previous testnet run, when the simulator boots on a different default network (regtest, etc.), only triggers the orphan-mnemonic recovery prompt *after* the network switch rebinds the wallet-scoped services. In that scenario the guard runs against a state where the prompt has not yet appeared and misses it. Re-run the guard after `switchAppNetworkToTestnet`, or move the existing call to right after the switch so the prompt is detected on whichever network the test actually executes against.
Network-switch race (BLOCKING from both reviewers): lift `isSwitchingNetwork` from OptionsView's local @State to AppState. The flag now spans the full async cycle (currentNetwork.didSet → Task → switchNetwork → sdk = newSDK), so the network status label and test-side `Connected` predicate stop reading the previous network's SDK as a successful switch. Test reliability: - runIdentityDiscovery drives the wallet picker explicitly instead of trusting default-first auto-selection, so unrelated wallets on the simulator no longer hijack discovery. - assertWalletRowVisible(exists: false) scrolls to the top and sweeps down before declaring absent, closing a false-pass on long lists where SwiftUI's lazy List could leave a still-persisted row outside the accessibility tree. - CreditTransferTest drops the `balance > 0` floor (couples to live testnet funding state); readIdentityBalanceCredits already XCFails on parse error, so reaching the next line is the readability signal. - Re-run failIfRecoveryPromptVisible after switchAppNetworkToTestnet — a leftover testnet wallet on a simulator booted to a different default network only triggers the orphan-mnemonic prompt after the network switch rebinds wallet-scoped services. - cleanupWalletsByPrefix("ImportTransfer-") on entry sweeps random- suffixed wallets from prior failed runs (the deterministic walletId-based check missed them). - Discovery retry loop only re-taps the Add menu when its menu item isn't still visible from a prior attempt — re-tapping while the menu is open closes it. Trim: - Delete unused credit-transfer helpers (navigateToIdentityCreditTransferForm, executeCreditTransfer, waitForCreditTransferSuccess) and Identifier.Transition. They will return alongside the credit-transfer test in a follow-up; the corresponding app-side .accessibilityIdentifier calls on TransitionDetailView/TransitionInputView are kept (harmless). CI: - Wire CreditTransferTest into the smoke workflow with -only-testing. Self-skips when UI_TEST_TESTNET_MNEMONIC is unset, so the new line is a no-op for forks/PRs without the secret while still exercising the build path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift (1)
528-532: Wait for picker selection to propagate before asserting label text.The assertion on Line 528 can race right after
walletOption.tap(). A short predicate wait onwalletPicker.labelcontainingwalletNamewill make this less flaky on slower simulators.Proposed stabilization
- XCTAssertTrue( - walletPicker.label.contains(walletName), - "Picker shows \"\(walletPicker.label)\" but the test imported \(walletName).", - file: file, line: line - ) + let selectedWalletPredicate = NSPredicate { object, _ in + guard let element = object as? XCUIElement, element.exists else { return false } + return element.label.contains(walletName) + } + let selectedWalletResult = XCTWaiter.wait( + for: [XCTNSPredicateExpectation(predicate: selectedWalletPredicate, object: walletPicker)], + timeout: 5 + ) + XCTAssertEqual( + selectedWalletResult, + .completed, + "Picker shows \"\(walletPicker.label)\" but the test imported \(walletName).", + file: file, line: line + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift` around lines 528 - 532, The assertion on walletPicker.label can race immediately after walletOption.tap(); add an explicit predicate wait that walletPicker.label contains walletName (e.g., create an NSPredicate "label CONTAINS %@", use expectation(for:evaluatedWith:handler:) or XCTNSPredicateExpectation, then wait(for:timeout:)) before calling XCTAssertTrue so the picker selection has time to propagate; update the flow around walletOption.tap(), walletPicker and the XCTAssertTrue to perform the predicate wait first and then assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 24-28: The isSwitchingNetwork flag is being cleared
unconditionally by each spawned Task after awaiting switchNetwork(to:), which
allows an earlier task to flip it false while a later switch is still running;
fix by introducing a request token/version (e.g., an Int or UUID property like
networkSwitchRequestID) that you increment/assign before creating the Task,
capture that token inside the Task, and only set isSwitchingNetwork = false if
the captured token matches the current networkSwitchRequestID after await
switchNetwork(to:); apply this change to both places that spawn the Task so
completions are serialized/guarded and stale tasks cannot clear readiness.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift`:
- Around line 5-8: Update the stale header comment at the top of
CreditTransferTest.swift (the file-level comment in the CreditTransferTest test)
to reflect the current assertions: change the phrase that claims the test
asserts a "non-zero balance" to state it only verifies that the identity's
balance is readable/accessible (and keep or clarify that the actual
credit-transfer assertion is deferred). Edit the header comment block above the
test implementation to match the test's scope (balance readability) and remove
or adjust any misleading wording about asserting a non-zero balance.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- Around line 528-532: The assertion on walletPicker.label can race immediately
after walletOption.tap(); add an explicit predicate wait that walletPicker.label
contains walletName (e.g., create an NSPredicate "label CONTAINS %@", use
expectation(for:evaluatedWith:handler:) or XCTNSPredicateExpectation, then
wait(for:timeout:)) before calling XCTAssertTrue so the picker selection has
time to propagate; update the flow around walletOption.tap(), walletPicker and
the XCTAssertTrue to perform the predicate wait first and then assert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fed7c149-4bb2-4a06-b438-e518503b44eb
📒 Files selected for processing (6)
.github/workflows/swift-example-app-ui-smoke.ymlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/Identifiers.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/swift-example-app-ui-smoke.yml
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
Three findings from coderabbit's re-review of 7c0ae94: - AppState: overlapping network switches could cause an earlier task to clear `isSwitchingNetwork` while a later switch was still running. Add a monotonic `networkSwitchRequestID`; each spawned task captures its id at start and only clears the flag when its id still matches. - CreditTransferTest header: aligned the file-level comment with the current scope (balance readability, not non-zero balance). - WalletFlow `runIdentityDiscovery`: replaced the bare label-contains assertion after `walletOption.tap()` with an `XCTNSPredicateExpectation` wait so the picker has time to propagate the selection on slower simulators. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames the testnet mnemonic env var the XCUITest reads to `UI_TEST_MNEMONIC` and forwards it from a GitHub secret of the same name into the xcodebuild step as `TEST_RUNNER_UI_TEST_MNEMONIC` (the prefix is required — xcodebuild strips it before handing the env to the XCUITest runner process). Empty on fork PRs (GitHub withholds secrets there), at which point the test self-skips. Once a repository secret named `UI_TEST_MNEMONIC` is set, `testImportWalletAndDiscoverIdentity` runs end-to-end against testnet on every push to a non-fork branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a 23:00 UTC daily cron to the smoke workflow, alongside the existing manual `workflow_dispatch` trigger. Mirrors the repo's nightly pattern (separate workflow with its own cron, like `tests-rs-nightly-long-running.yml`). Also adds a concurrency block so a manual dispatch and a cron run don't fight over the single self-hosted macOS ARM64 runner — the older run is cancelled in favor of the newer one. The cron only takes effect once this lands on the default branch (`v3.1-dev`); GitHub Actions schedules run against the default branch's workflow file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the operational gotchas that took multiple sessions to re-discover: - `TEST_RUNNER_` prefix gate when passing `UI_TEST_MNEMONIC` via `xcodebuild test ENV=`. - `simctl erase` is the only way to clear stale Keychain entries between runs (uninstall alone leaves orphan mnemonics that trip the recovery prompt). - SwiftData migration failures from leftover stores manifest as a "Expected root tab bar" timeout, not an obvious crash. - The CI nightly cron's reliance on the self-hosted Mac being online. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift (1)
407-434:⚠️ Potential issue | 🟠 MajorWait for Testnet selection to latch before accepting “Connected”.
Right now the helper can still pass if the app remains connected on the previous network and the segmented-control tap doesn’t take effect. Add an explicit wait that
testnetButton.isSelected == truebefore asserting connection status.Suggested patch
@@ - if testnetButton.isSelected { - // Already on Testnet; status should already be Connected. - } else { + if !testnetButton.isSelected { testnetButton.tap() } + + let selectedResult = XCTWaiter.wait( + for: [ + XCTNSPredicateExpectation( + predicate: NSPredicate(format: "isSelected == true"), + object: testnetButton + ), + ], + timeout: 10 + ) + XCTAssertEqual( + selectedResult, + .completed, + "Expected Testnet segment to be selected before waiting for connected status.", + file: file, line: line + ) @@ let result = XCTWaiter.wait( for: [XCTNSPredicateExpectation(predicate: connectedPredicate, object: statusLabel)], timeout: timeout )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift` around lines 407 - 434, The helper can report "Connected" for the previous network if the segmented control tap hasn't latched; fix by waiting for the Testnet segment selection to take effect before checking network status: after tapping testnetButton (or the existing isSelected check) add an explicit wait using an NSPredicate/XCTNSPredicateExpectation that verifies testnetButton.isSelected == true (with a reasonable timeout) and assert the wait completes successfully, then proceed to build statusLabel and wait for the "Connected" predicate (retain connectedPredicate, statusLabel, and the existing XCTWaiter usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- Around line 518-520: The NSPredicate used to find walletOption is too
permissive and can match prefixes; update the predicate in the walletOption
construction (the app.buttons.matching(...) call) to require a trailing space
after the walletName token (e.g., use "label BEGINSWITH %@ " with walletName as
the argument) so it only matches the exact name token rather than any prefix;
keep the rest of the selector (firstMatch) unchanged.
- Around line 653-667: cleanupWalletsByPrefix is only checking visible buttons
and can miss off-screen wallets; change it to iterate over the full query (use
app.buttons.matching(predicate).allElementsBoundByIndex) or repeatedly scroll
the wallets list (element(Identifier.walletsScreen)) while locating elements
matching the predicate so every persisted wallet is discovered, and call
bestEffortDeleteWallet(named:in:) for each found name until none remain; ensure
the loop continues until the full query is empty rather than stopping when no
visible match is found.
---
Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- Around line 407-434: The helper can report "Connected" for the previous
network if the segmented control tap hasn't latched; fix by waiting for the
Testnet segment selection to take effect before checking network status: after
tapping testnetButton (or the existing isSelected check) add an explicit wait
using an NSPredicate/XCTNSPredicateExpectation that verifies
testnetButton.isSelected == true (with a reasonable timeout) and assert the wait
completes successfully, then proceed to build statusLabel and wait for the
"Connected" predicate (retain connectedPredicate, statusLabel, and the existing
XCTWaiter usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 032ec7d6-0160-4c45-b81b-78633330445e
📒 Files selected for processing (5)
.github/workflows/swift-example-app-ui-smoke.ymlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/README.mdpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/swift-example-app-ui-smoke.yml
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift
Three findings from coderabbit's re-review: - `switchAppNetworkToTestnet`: wait for the testnet segment's `isSelected == true` before trusting the "Connected" predicate. The AppState fix already covers the rebind-in-progress race; this closes the missed-tap window where the picker setter never fires and the previous network's "Connected" still satisfies the wait. - `runIdentityDiscovery`: anchor the wallet-option predicate with a trailing space (`BEGINSWITH "<walletName> "`) so a longer wallet name sharing a prefix can't win firstMatch. - `cleanupWalletsByPrefix`: scroll-and-sweep the full wallets list instead of bailing when no matching row is visible in the current viewport. Resets to top, then up to 10 delete+reset cycles + 10 scroll passes per call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-reviewed 296d9661.
The follow-up fixes address the reliability issues from my earlier pass: the network-status signal now stays honest across the full SDK rebind, overlapping switches no longer clear readiness early, the discovery flow explicitly selects the imported wallet, the recovery-prompt guard runs after the testnet hop, and the unused transfer scaffolding is trimmed back out. The workflow/docs wiring for UI_TEST_MNEMONIC also makes the new smoke path much easier to understand and run.
Looks good to me.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR adding a testnet UI test, accessibility identifiers, and a nightly CI smoke workflow. No consensus-critical or blocking issues found — Codex's blocking claim about overlapping network switches is a false positive (the synchronous setter runs to completion before either spawned Task executes, so both tasks read the post-update currentNetwork and build SDKs for the same target). Remaining findings are around CI artifact hygiene, fixture/secret coupling, and a real-but-narrow flakiness window in switchAppNetworkToTestnet.
Reviewed commit: 4e4cf1a
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [SUGGESTION] lines 413-427: switchAppNetworkToTestnet can return before the switch actually starts
The helper waits only for `options.networkStatusLabel` to contain `"Connected"`. That label is not network-aware: when `CreditTransferTest` runs on a simulator that was already left on mainnet/regtest from a prior run, the label reads `"Connected"` *before* the tap. `XCTNSPredicateExpectation` polls (default ~1s cadence) and the predicate can evaluate against the stale pre-tap snapshot before the `currentNetwork` `didSet` → `beginNetworkSwitch` → SwiftUI rerender chain has flipped the label to `"Switching..."`. The helper then returns and the test proceeds with wallet import + identity discovery while the SDK is still bound to the previous network. Make the readiness signal encode the post-tap transition: either expose the active network in the label (e.g. `"Connected · Testnet"`) and match on that, or wait for `appState.isSwitchingNetwork == true` first (a `"Switching..."` substring match) and only then wait for `"Connected"`.
- [SUGGESTION] lines 285-303: Mnemonic typed into plain TextField may surface in uploaded xcresult artifact
`importWallet()` types the testnet mnemonic into `mnemonicField` (`app.textFields`, not `app.secureTextFields` — confirming the field is a plain SwiftUI `TextField`). The CI workflow at `.github/workflows/swift-example-app-ui-smoke.yml` then uploads `SwiftExampleAppUITests.xcresult` as an artifact with 14-day retention readable by anyone with repo read access. xcresult bundles include the XCUITest activity log (which records `typeText` arguments) and any failure screenshots — a flake while the Create Wallet sheet is visible (e.g. the import-button-enable wait at 297-303) would capture the mnemonic. Although this is a testnet mnemonic, it controls the pre-funded identity referenced by the hardcoded fixtures in `CreditTransferTest.swift`; leaking it lets anyone with repo read access drain testnet credits, hijack the identity, or interfere with future CI runs. Either redact (paste via a pre-cleared `UIPasteboard` hop, or wire a `setLaunchArgument` path that bypasses the visible field for tests) or scope artifact retention more tightly and rotate on observed leak.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift`:
- [SUGGESTION] lines 28-36: Hardcoded sender identityId/walletId silently couple to the rotating CI secret
`expectedSenderIdentityIdBase58` and `expectedSenderWalletIdHex` are deterministic functions of the mnemonic stored in `secrets.UI_TEST_MNEMONIC`. Nothing in the repo links them: a future maintainer rotating the secret (e.g. after the funded identity is exhausted, or in response to the artifact-leak surface flagged below) will see this test fail with an opaque `"identity row not found within 60s"` timeout from `waitForIdentityRow`, with no breadcrumb that the constants here also need to be regenerated. Either drop the walletId/identityId expectations and rely on the looser "`runIdentityDiscovery` saw +N (N>0)" + "`readIdentityBalanceCredits` parses" properties already covered, or add a comment block here naming the regeneration steps (import the new mnemonic into a clean simulator, observe the resulting `wallets.walletRow.<hex>` identifier and the discovered identity's base58 ID, paste both here).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR adding XCUITest coverage for testnet wallet import and DIP-9 identity discovery, plus shared helpers and accessibility identifiers. No production code or consensus paths are touched. I found one substantive coverage mismatch around the claimed non-zero balance assertion, plus two smaller cleanup/docs issues that could make the new UI tests harder to run or diagnose.
Reviewed commit: 3823ee4
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift`:
- [SUGGESTION] lines 110-115: Discovery test never asserts a non-zero balance was actually loaded
`_ = readIdentityBalanceCredits(in: app)` only verifies that `identityDetail.balanceLabel` exposes a parseable number, which conflicts with the PR description's stated goal of asserting that the discovered identity surfaces with a non-zero balance. Two reinforcing reasons this matters: (1) the in-file comment intentionally disclaims the floor ("deliberately don't assert a non-zero floor"), so the test's coverage and the PR description are inconsistent — anyone copying the description's claim will assume balance regression coverage that doesn't exist; (2) `IdentityDetailView.onAppear` (Views/IdentityDetailView.swift:431-459) only refreshes DPNS/DashPay/tokens — it does not refresh balance, so a regression where discovery still finds the identity but stops persisting/surfacing its balance would render `0` and silently pass this test. Either tighten the PR description and the in-file comment to match what is actually asserted (parseability only), or add an explicit `XCTAssertGreaterThan(credits, 0, ...)` so the discovery+balance path is genuinely guarded.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [SUGGESTION] lines 705-752: `bestEffortDeleteWallet` teardown does not dismiss the orphan-mnemonic recovery alert
All three teardown blocks (CreditTransferTest.swift:93-100, WalletPersistenceTests.swift:39-48 and 82-91) launch a fresh app and call `bestEffortDeleteWallet`, but the helper never checks for or dismisses a `"Recover Wallet?"` alert. If a prior assertion fails after a partial wallet write that leaks Keychain or SwiftData state — exactly the scenario `testWalletDeletionCleanupSurvivesRelaunch` is designed to catch — the recovery prompt blocks the UI on the cleanup launch, the wallet row never appears, the `guard row.waitForExistence` exits silently, and the residue persists into the next run. The next test then trips `failIfRecoveryPromptVisible` with a confusing pre-existing residue rather than a clean failure attributable to its own setup. Add a best-effort dismiss of the recovery alert at the top of `bestEffortDeleteWallet`, or document this as a known gap.
Five reviewer findings + two collateral fixes from the v3.1-dev merge:
Reviewer findings:
- `switchAppNetworkToTestnet`: when we actually tapped to switch,
observe the label transition through "Switching..." before trusting
"Connected". Defends against the predicate sampling stale "Connected"
state from the previous network before SwiftUI re-renders.
- `CreditTransferTest`: hardcoded `expectedSenderIdentityIdBase58` and
`expectedSenderWalletIdHex` are deterministic functions of the secret
mnemonic — added a regeneration-steps comment block so a future
rotation doesn't surface as an opaque "identity row not found"
timeout.
- `runIdentityDiscovery` foundCount predicate: tightened from
`hasPrefix("+") && != "+0"` to also require the suffix to be all
digits, defending against future label format drift.
- Restore `XCTAssertGreaterThan(credits, 0, ...)` on the balance read.
`IdentityDetailView.onAppear` doesn't refresh balance, so a regression
that breaks balance discovery (returns 0) would silently pass the
parseability-only assertion. Updated the file header comment to match.
- `bestEffortDeleteWallet`: dismiss the orphan-mnemonic recovery alert
best-effort at the top of the helper. Otherwise a leaked partial
wallet write from an earlier failure stalls all teardown blocks
silently.
- Workflow: only upload the xcresult bundle on `failure()` (not always)
and reduce retention from 14 to 7 days. The bundle includes the
XCUITest activity log which records `typeText` arguments —
`importWallet` types the testnet mnemonic. Limiting to failures
narrows the leak surface.
Collateral from the v3.1-dev merge in 3823ee4:
- `KeyManagerTests`: `KeyFormatter.toWIF(_:isTestnet:)` was renamed to
`KeyFormatter.toWIF(_:network:)` upstream but this test file wasn't
updated. One-line signature fix; otherwise `xcodebuild test` won't
compile.
- `cleanupWalletsByPrefix`: revert the full-list scroll-and-sweep added
in c08f9fa (intended to address a coderabbit suggestion). On an
empty wallet list the blind `swipeUp` calls trip XCUITest's 60s
event-synthesis timeout per swipe, blowing test runtime out by 20+
minutes. Bounded bail-on-no-match version with a longer 10-iteration
cap is a better balance; the off-screen-rows risk is documented in
the helper comment and `simctl erase` is the right developer recovery.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swift`:
- Around line 53-63: The current requestID only prevents clearing the spinner
early but doesn’t stop a stale Task from updating shared state; modify
beginNetworkSwitch to pass the current networkSwitchRequestID (token) into
switchNetwork (e.g., call switchNetwork(to:currentNetwork, token: requestID))
and then update switchNetwork to early-return whenever the passed token no
longer equals the latest networkSwitchRequestID before writing any shared
properties (sdk, dataManager.currentNetwork, isLoading, isSwitchingNetwork) and
after each await point; ensure every mutation of shared state in switchNetwork
is guarded by comparing the token to networkSwitchRequestID so stale tasks
cannot overwrite newer state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f72d1c4c-56d8-489a-a539-c85707e4a0ce
📒 Files selected for processing (8)
.github/workflows/swift-example-app-ui-smoke.ymlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SearchWalletsForIdentitiesView.swift
✅ Files skipped from review due to trivial changes (5)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SearchWalletsForIdentitiesView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/swift-example-app-ui-smoke.yml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated against HEAD 33b6f3c. Two prior findings (non-zero balance assertion and orphan-recovery teardown) are confirmed FIXED. One blocking bug remains: the new identity-discovery helper waits for a navigation title ("Search Wallets") that the actual sheet view never sets — its title is "Re-scan for Identities" — so the new discovery test fails before the scan even runs. Two suggestions remain on test reliability and CI mnemonic-leak surface; one PR-description-only naming nit carried forward.
Reviewed commit: 33b6f3c
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [BLOCKING] line 520: Identity-discovery helper waits for a navigation title that the sheet never uses
`runIdentityDiscovery` keys its retry loop on `app.navigationBars["Search Wallets"]` (line 520) and fails with `Expected Search Wallets sheet to open...` if that nav bar never appears. But `SearchWalletsForIdentitiesView` actually sets `.navigationTitle("Re-scan for Identities")` (Views/SearchWalletsForIdentitiesView.swift:93), and a worktree-wide grep confirms no view sets `"Search Wallets"` as a navigation title. `sheetOpened` therefore stays `false` even when the menu tap succeeds and the sheet is on screen, so the new `testImportWalletAndDiscoverIdentity` test fails before it ever drives the scan. Match the title actually set by the sheet.
- [SUGGESTION] lines 514-538: Unconditional `addMenu.tap()` on each retry toggles the menu, halving effective retries
Line 523 unconditionally taps `addMenu` at the top of every loop iteration. On a SwiftUI Menu each tap toggles open/closed, so when the menu is left open by a failed item-tap (the exact failure mode the comment describes), the next iteration's tap closes it instead of refreshing it: iter1 opens, iter2 closes, iter3 opens. The two even iterations actively unwind progress — `searchMenuItem.waitForExistence(timeout: 3)` and the labeled-button fallback both fail while the menu is shut, then `searchSheetNavBar.waitForExistence(timeout: 5)` adds another 5s of dead waiting. Effective retries drop from 3 to ~2, with ~8s wasted per dead iteration. The justification comment ("Closing-and-reopening the menu forces a fresh accessibility-tree snapshot") describes a close-then-open sequence; either do that explicitly (two taps per retry when the menu is open), or only re-tap `addMenu` when `searchMenuItem.exists == false`, dismissing first if needed (e.g. by tapping outside the menu).
In `.github/workflows/swift-example-app-ui-smoke.yml`:
- [SUGGESTION] lines 198-212: Failure-only xcresult upload still archives the testnet mnemonic on the most likely failure paths
The `if: failure()` guard plus 7-day retention narrows but does not eliminate the leak surface. `importWallet` types the mnemonic into a plain `TextField` via `mnemonicField.typeText(mnemonic)` (WalletFlow.swift:285-292), and the XCUITest activity log records `typeText` arguments. Any failure that occurs after the import step — discovery timeout, balance assertion failure, the navigation-title bug above — produces an xcresult containing the mnemonic verbatim, downloadable by anyone with read access to the workflow run for 7 days. Those post-import failures are exactly the modes most likely to occur in practice. The inline comment correctly flags the tradeoff; consider stronger mitigations as follow-up: restructure the test so the secret is not entered through XCUITest at all (e.g. inject via a launch-environment hook the app reads directly), or post-process the xcresult to strip activity-log entries before upload.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up commit 33b6f3c fixes the prior non-zero balance assertion and the orphan-recovery teardown gap, but introduces a selector mismatch in the new identity-discovery helper: the test waits for a navigation bar titled Search Wallets while SearchWalletsForIdentitiesView renders Re-scan for Identities, which will gate the new end-to-end test on the wrong UI element. Remaining items are minor — an unscoped Done button match, a stale README sentence, a Switching... flake risk, and the PR-body env-var name mismatch.
Reviewed commit: 33b6f3c
🔴 1 blocking | 💬 4 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [BLOCKING] lines 520-541: Identity-discovery helper waits for a navigation bar title the sheet never renders
`runIdentityDiscovery` treats `app.navigationBars["Search Wallets"]` as the success signal for opening the discovery sheet, but `SearchWalletsForIdentitiesView` sets its actual navigation title to `"Re-scan for Identities"` (Views/SearchWalletsForIdentitiesView.swift:93) and no other view in the hierarchy uses a `Search Wallets` title. With this mismatch, `sheetOpened` cannot become true even when the menu tap successfully presents the sheet; the helper will exhaust its three-attempt retry loop and fail at the `XCTAssertTrue(sheetOpened, ...)` line before the picker, search button, or found-count assertions ever run. Because `CreditTransferTest.testImportWalletAndDiscoverIdentity` depends on this helper, the new end-to-end test is gated on a selector mismatch rather than the feature under test. Switching the wait target to the actual title (or to a stable accessibility identifier on the sheet) restores the intended behavior.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is a UI-test-and-accessibility-identifier addition. The new XCUITest discovery helper has two confirmed blocking bugs (a navigation-title string mismatch that prevents the sheet check from ever succeeding, and a +0-rejecting assertion that breaks idempotency on reused simulators since identity rows are nullified rather than deleted). Three additional suggestions cover a teardown-fallback pitfall, a negative-Int64 balance bit-pattern issue surfaced through accessibilityValue, and a documented-but-still-real testnet-mnemonic exposure on failed-upload xcresult artifacts.
Reviewed commit: 33b6f3c
Fresh dispatcher rerun for this queue item. Same-SHA automated reviews already existed, so I am posting this as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🔴 blocking: Discovery helper waits on a navigation title the sheet never uses
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:520
runIdentityDiscovery watches app.navigationBars["Search Wallets"] to confirm the sheet opened, but SearchWalletsForIdentitiesView.swift:93 sets .navigationTitle("Re-scan for Identities"). The wait can never succeed, so sheetOpened stays false through all three retry iterations and the new testImportWalletAndDiscoverIdentity will fail at XCTAssertTrue(sheetOpened, ...) before exercising any discovery logic. This is a hard failure in the new test path, not a flake — verified against the checked-out worktree at HEAD.
💡 Suggested change
let searchSheetNavBar = app.navigationBars["Re-scan for Identities"]
🔴 blocking: `+0`-rejecting predicate breaks idempotency on reused simulators
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:612-631
The foundPredicate requires "+<digits>" && label != "+0", asserting at least one new identity per run. However, PersistentWallet.identities uses @Relationship(deleteRule: .nullify, ...) (Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swift:70), so deleting a wallet only nullifies the wallet ref on the identity rows — the PersistentIdentity entries persist on the simulator. On a re-run against the same fixture mnemonic, the target identity is already locally present, discoverIdentities returns found.count == 0, the UI legitimately renders +0, and this assertion fails even though everything downstream (row presence, balance) would still pass. The smoke test is non-repeatable outside a freshly-erased simulator. The CI workflow uses a self-hosted runner, so reused simulator state is the realistic case, not the exception.
💡 Suggested change
let foundPredicate = NSPredicate { object, _ in
guard let element = object as? XCUIElement, element.exists else { return false }
let label = element.label
return label.hasPrefix("+")
&& label.dropFirst().allSatisfy(\.isNumber)
}
XCTAssertEqual(
XCTWaiter.wait(
for: [XCTNSPredicateExpectation(predicate: foundPredicate, object: foundCount)],
timeout: timeout
),
.completed,
"Expected discovery to report a result count within \(Int(timeout))s.",
file: file, line: line
)
🟡 suggestion: Orphan-recovery alert fallback can tap `Authorize` and trigger biometric flow
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:752-762
bestEffortDeleteWallet tries Cancel and Don't Recover on the Recover Wallet? alert and then falls back to recoverAlert.buttons.element(boundBy: 0).tap(). The actual alert in ContentView.swift:127-132 only declares Authorize and No (.cancel) — neither named branch matches, so the fallback runs every time. The first button by index is Authorize, which kicks off biometric recovery instead of dismissing the blocking modal, leaving teardown effectively no-oped and residue on the simulator that compounds across runs.
💡 Suggested change
let recoverAlert = app.alerts["Recover Wallet?"]
if recoverAlert.waitForExistence(timeout: 1) {
if recoverAlert.buttons["No"].exists {
recoverAlert.buttons["No"].tap()
} else if recoverAlert.buttons["Cancel"].exists {
recoverAlert.buttons["Cancel"].tap()
} else if recoverAlert.buttons["Don't Recover"].exists {
recoverAlert.buttons["Don't Recover"].tap()
}
}
🟡 suggestion: `UInt64(bitPattern: identity.balance)` masks negative-balance regressions
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift:134
PersistentIdentity.balance is Int64 while the SDK's underlying credit balance is u64. Exposing it via accessibilityValue("\(UInt64(bitPattern: identity.balance))") reinterprets the bit pattern, so a negative Int64 (which would already indicate an upstream FFI/persistence bug) silently surfaces as a huge positive UInt64. Combined with readIdentityBalanceCredits in WalletFlow stripping all non-digit characters before UInt64(...) parsing, and the test asserting only > 0, a regression that produces a negative Int64 would (a) reinterpret to ~Int64.max * 2, (b) survive the digit filter, and (c) pass the > 0 floor. The PR description notes this assertion is the regression-detection mechanism, but the current shape only proves non-zero. Either emit String(identity.balance) and reject leading - in the test parser, or add an upper-bound sanity check (legitimate testnet identity balances are nowhere near Int64.max), or assert non-negativity at the FFI boundary so views never see negative Int64.
🟡 suggestion: Failure-only xcresult upload still archives the testnet mnemonic in the activity log
.github/workflows/swift-example-app-ui-smoke.yml:198-212
WalletFlow.swift uses mnemonicField.typeText(mnemonic) with the value of UI_TEST_MNEMONIC, and Xcode records typeText arguments verbatim in the .xcresult activity log. Restricting upload to failure() with 7-day retention narrows the window but does not close it: any failed run produces an artifact from which the full mnemonic is recoverable by anyone with actions: read for the retention window. The threat model is meaningfully bounded by (a) the workflow only triggering on workflow_dispatch and schedule (not pull_request, so untrusted forks cannot attach attacker-controlled code that fails after typing), (b) testnet-only funds, and (c) the inline comment documenting the tradeoff. Still worth re-confirming the funded testnet identity is acceptable to lose and that actions: read is restricted to trusted parties. A stronger mitigation, if desired: redact the activity log out of the xcresult before upload (e.g. extract only screenshots and crash logs via xcrun xcresulttool), or set the mnemonic field via pasteboard out-of-band so it never reaches typeText arguments.
💬 nitpick: Mnemonic word-count check is fragile to non-space whitespace
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift:70-74
mnemonic.split(separator: " ").count == 12 only splits on the literal space character. A secret rotation that introduces a trailing newline, tab, or NBSP separator would either misfire this guard (e.g. tab-separated phrase counted as 1) or pass an invalid input through to importWallet (a trailing \n propagates into typeText and on some keyboards inserts a return). Trim and split on CharacterSet.whitespacesAndNewlines, then pass the rejoined canonical form into importWallet. Low-stakes — the secret is typically clean — but the kind of footgun that surfaces only after a rotation. Note: the original Claude finding cited line 524-528, but the actual location in this branch is line 70-74.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:520: Discovery helper waits on a navigation title the sheet never uses
`runIdentityDiscovery` watches `app.navigationBars["Search Wallets"]` to confirm the sheet opened, but `SearchWalletsForIdentitiesView.swift:93` sets `.navigationTitle("Re-scan for Identities")`. The wait can never succeed, so `sheetOpened` stays false through all three retry iterations and the new `testImportWalletAndDiscoverIdentity` will fail at `XCTAssertTrue(sheetOpened, ...)` before exercising any discovery logic. This is a hard failure in the new test path, not a flake — verified against the checked-out worktree at HEAD.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:612: `+0`-rejecting predicate breaks idempotency on reused simulators
The `foundPredicate` requires `"+<digits>" && label != "+0"`, asserting at least one *new* identity per run. However, `PersistentWallet.identities` uses `@Relationship(deleteRule: .nullify, ...)` (Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swift:70), so deleting a wallet only nullifies the wallet ref on the identity rows — the `PersistentIdentity` entries persist on the simulator. On a re-run against the same fixture mnemonic, the target identity is already locally present, `discoverIdentities` returns `found.count == 0`, the UI legitimately renders `+0`, and this assertion fails even though everything downstream (row presence, balance) would still pass. The smoke test is non-repeatable outside a freshly-erased simulator. The CI workflow uses a self-hosted runner, so reused simulator state is the realistic case, not the exception.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift:752: Orphan-recovery alert fallback can tap `Authorize` and trigger biometric flow
`bestEffortDeleteWallet` tries `Cancel` and `Don't Recover` on the `Recover Wallet?` alert and then falls back to `recoverAlert.buttons.element(boundBy: 0).tap()`. The actual alert in `ContentView.swift:127-132` only declares `Authorize` and `No (.cancel)` — neither named branch matches, so the fallback runs every time. The first button by index is `Authorize`, which kicks off biometric recovery instead of dismissing the blocking modal, leaving teardown effectively no-oped and residue on the simulator that compounds across runs.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift:134: `UInt64(bitPattern: identity.balance)` masks negative-balance regressions
`PersistentIdentity.balance` is `Int64` while the SDK's underlying credit balance is `u64`. Exposing it via `accessibilityValue("\(UInt64(bitPattern: identity.balance))")` reinterprets the bit pattern, so a negative `Int64` (which would already indicate an upstream FFI/persistence bug) silently surfaces as a huge positive `UInt64`. Combined with `readIdentityBalanceCredits` in WalletFlow stripping all non-digit characters before `UInt64(...)` parsing, and the test asserting only `> 0`, a regression that produces a negative `Int64` would (a) reinterpret to ~`Int64.max * 2`, (b) survive the digit filter, and (c) pass the `> 0` floor. The PR description notes this assertion is the regression-detection mechanism, but the current shape only proves non-zero. Either emit `String(identity.balance)` and reject leading `-` in the test parser, or add an upper-bound sanity check (legitimate testnet identity balances are nowhere near `Int64.max`), or assert non-negativity at the FFI boundary so views never see negative `Int64`.
- [SUGGESTION] .github/workflows/swift-example-app-ui-smoke.yml:198: Failure-only xcresult upload still archives the testnet mnemonic in the activity log
`WalletFlow.swift` uses `mnemonicField.typeText(mnemonic)` with the value of `UI_TEST_MNEMONIC`, and Xcode records `typeText` arguments verbatim in the `.xcresult` activity log. Restricting upload to `failure()` with 7-day retention narrows the window but does not close it: any failed run produces an artifact from which the full mnemonic is recoverable by anyone with `actions: read` for the retention window. The threat model is meaningfully bounded by (a) the workflow only triggering on `workflow_dispatch` and `schedule` (not `pull_request`, so untrusted forks cannot attach attacker-controlled code that fails after typing), (b) testnet-only funds, and (c) the inline comment documenting the tradeoff. Still worth re-confirming the funded testnet identity is acceptable to lose and that `actions: read` is restricted to trusted parties. A stronger mitigation, if desired: redact the activity log out of the xcresult before upload (e.g. extract only screenshots and crash logs via `xcrun xcresulttool`), or set the mnemonic field via pasteboard out-of-band so it never reaches `typeText` arguments.
…test-wallet-persistence # Conflicts: # packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift
- WalletFlow.swift: match the discovery sheet's actual nav title
("Re-scan for Identities") so testImportWalletAndDiscoverIdentity
can detect the sheet opening
- AppState.swift: thread networkSwitchRequestID into switchNetwork
and guard each shared-state mutation, so stale Tasks from an
earlier switch can't overwrite sdk/dataManager/isLoading after a
newer switch has begun
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- WalletFlow.swift: close stale menu before re-opening so each retry gets a fresh accessibility-tree snapshot instead of toggling the menu closed - WalletFlow.swift: bestEffortDeleteWallet teardown dismisses both "Recover Wallet?" and "Recover Wallets?" alert variants - README.md: match the suite description to the actual non-zero-credits assertion in testImportWalletAndDiscoverIdentity Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Discovery Self-review of the prior PR #3560 follow-up commits surfaced three second-order copies of the "Search Wallets" vs "Re-scan for Identities" defect that the blocking fix only addressed at one site: - comment block above the retry loop still pointed at the old nav-bar signal name - the rationale block claimed addMenu is re-tapped unconditionally, but the prior suggestion patch made it conditional (close-then-open) - the label-based fallback inside the loop still queried `app.buttons["Search Wallets for Identities"]`, but IdentitiesContentView labels the menu item "Re-scan for Identities" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds an XCUITest discovery+balance smoke flow, accessibility identifiers, and a nightly CI workflow. No blockers at this head: the prior navigation-title mismatch and menu-item label fallback are fixed. Three suggestion-level issues remain — a teardown alert-dismissal helper that can tap the wrong button and trigger biometric recovery, a bit-pattern reinterpretation that masks negative-balance regressions through the new >0 assertion, and a documented but still-real mnemonic exposure in failure-mode xcresult uploads. Codex's +0-rejection finding was downgraded from blocking: the test pre-cleans the existing wallet row and re-imports fresh, so discovery (a Rust-side network scan, not a SwiftData read) returns the live on-chain count.
Reviewed commit: 0e38b24
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [SUGGESTION] lines 769-778: Teardown `bestEffortDeleteWallet` can tap 'Authorize' on the recovery alert
`bestEffortDeleteWallet` tries `Cancel` then `Don't Recover`, then falls back to `recoverAlert.buttons.element(boundBy: 0)`. The alert defined in `ContentView.swift:132-138` exposes only `Button("Authorize")` and `Button("No", role: .cancel)` — neither named branch matches, so the index-0 fallback always runs for this alert shape. iOS does not guarantee that the `.cancel`-role button is at index 0, so this can tap `Authorize` and kick off the biometric recovery sheet, which XCUITest can't resolve. Teardown is then silently no-oped and the simulator carries residue into the next run. Add `"No"` to the named branches before the index-based fallback.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- [SUGGESTION] lines 139-147: `UInt64(bitPattern: identity.balance)` can mask negative-balance regressions through the new `>0` assertion
`PersistentIdentity.balance` is `Int64` while the SDK's credit balance is `u64`. `accessibilityValue("\(UInt64(bitPattern: identity.balance))")` reinterprets the bit pattern, so a negative `Int64` (which itself would indicate an upstream FFI/persistence bug) silently surfaces as a huge positive `UInt64`. `readIdentityBalanceCredits` in `WalletFlow.swift:700` then strips non-digits and parses, and `CreditTransferTest.swift:138-142` only asserts `XCTAssertGreaterThan(credits, 0, …)`. A negative-`Int64` regression would (a) reinterpret to ~`Int64.max * 2`, (b) survive the digit filter, and (c) pass the floor — defeating the new regression-detection guard described in the in-file comment. Options: emit `String(identity.balance)` and reject a leading `-` in the parser; add an upper-bound sanity check (legitimate testnet balances are nowhere near `Int64.max`); or assert non-negativity at the FFI boundary so views never see a negative `Int64`.
In `.github/workflows/swift-example-app-ui-smoke.yml`:
- [SUGGESTION] lines 198-212: Failure-mode xcresult upload still archives the testnet mnemonic in the activity log
`WalletFlow.swift:292` calls `mnemonicField.typeText(mnemonic)` on a plain SwiftUI `TextField` (not `SecureField`), and Xcode records `typeText` arguments verbatim in the `.xcresult` activity log. Restricting upload to `failure()` with 7-day retention narrows the window but doesn't close it: any failed run produces an artifact from which the full mnemonic is recoverable by anyone with `actions: read` for the retention window. The threat model is meaningfully bounded — `workflow_dispatch`/`schedule` only (no untrusted-fork PR trigger), testnet-only funds, and an inline comment documenting the tradeoff — so this is a judgment call rather than a defect. Stronger mitigations if desired: redact the activity log out of the xcresult before upload (extract only screenshots/crash logs via `xcrun xcresulttool`), set the mnemonic via pasteboard so it never reaches `typeText` arguments, or rotate the mnemonic when a failure artifact is uploaded.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds XCUITest coverage for testnet wallet import + DIP-9 identity discovery plus accessibility identifiers and a nightly CI workflow. Prior hard failures (navigation-title mismatch and missing non-zero balance assertion) are fixed at HEAD. Three suggestion-level issues remain — all flagged by both reviewers — covering a teardown helper whose named-button branches don't match this app's recovery alert, a UInt64(bitPattern:) reinterpretation that defeats the new >0 floor, and a documented-but-real testnet mnemonic exposure in failure-mode xcresult uploads.
Reviewed commit: 92cddcd
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/Support/WalletFlow.swift`:
- [SUGGESTION] lines 769-778: `bestEffortDeleteWallet` named-button branches don't match this app's recovery alert; fallback can tap `Authorize`
The recovery alert declared at `ContentView.swift:132-138` has exactly two buttons: `Button("Authorize")` and `Button("No", role: .cancel)`. The teardown helper only checks `recoverAlert.buttons["Cancel"]` and `recoverAlert.buttons["Don't Recover"]` — neither label exists on this alert — so the `recoverAlert.buttons.element(boundBy: 0).tap()` fallback runs every time the alert is shown. iOS does not guarantee that the `.cancel`-role button sits at index 0 in the accessibility tree (declaration order frequently dominates, putting `Authorize` first), so the fallback can fire the biometric-recovery path instead of dismissing the modal. Teardown then silently no-ops and simulator residue compounds across runs — directly defeating the purpose of `bestEffortDeleteWallet`. Add a `"No"` branch before the index-based fallback so the actual alert is handled by name.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- [SUGGESTION] lines 142-147: `UInt64(bitPattern: identity.balance)` reinterprets negative Int64 and bypasses the new `>0` floor
`PersistentIdentity.balance` is stored as `Int64` while the SDK's underlying credit balance is `u64`. Exposing it via `.accessibilityValue("\(UInt64(bitPattern: identity.balance))")` reinterprets the bit pattern, so a negative `Int64` (which itself would already indicate an upstream FFI/persistence bug) silently surfaces as a huge positive `UInt64`. `readIdentityBalanceCredits` (`WalletFlow.swift:696-708`) then strips non-digit characters and parses, and `CreditTransferTest.swift:137-142` only asserts `XCTAssertGreaterThan(credits, 0, …)`. A regression that produces a negative `Int64` would (a) reinterpret to ~`UInt64.max`, (b) survive the digit filter, and (c) pass the `>0` floor — defeating the regression-detection guard the in-file comment promises. Options: emit `String(identity.balance)` and reject a leading `-` in the parser; add an upper-bound sanity check (legitimate testnet balances are nowhere near `Int64.max`); or assert non-negativity at the FFI boundary so views never see a negative `Int64`.
|
Merge-readiness take: I would not merge this as-is. It is close, but there is one cleanup bug I would fix before merge and one security/ops risk that needs an explicit maintainer decision. Required before merge:
Needs an explicit call before merge:
Lower severity / not hard blockers:
So my recommendation is: fix the recovery-alert cleanup at minimum, make an explicit decision on the mnemonic artifact exposure, then update from |
- WalletFlow.cleanupWalletsByPrefix: replace the 0..<10 cap with a
signal-driven loop plus no-progress detection. The previous count cap
silently bailed when bestEffortDeleteWallet made no progress, leaving
any wallets above index 10 in place.
- WalletFlow.switchAppNetworkToTestnet: remove the "Switching..."
transition wait. The status label now includes the network name, so
we assert on "Connected to Testnet" directly instead of inferring
network identity from an intermediate state.
- WalletFlow.bestEffortDeleteWallet: dismiss the orphan-mnemonic
recovery alert by its actual button label ("No", per
ContentView.swift) instead of guessed labels ("Cancel" / "Don't
Recover") with a positional fallback that could tap Authorize and
enter the recovery flow.
- OptionsView: render "Connected to <network name>" in the network
status label, reusing the existing Network.displayName.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…value PersistentIdentity.balance is stored as Int64 (via UInt64(bitPattern:) round-trip from the SDK's UInt64). The identity-detail view previously reinterpreted that Int64 back to UInt64 for accessibilityValue, so a genuine negative balance would render as a huge positive number and the CreditTransferTest's `> 0` floor would silently pass. - IdentityDetailView: emit `identity.balance` directly (signed Int64). - readIdentityBalanceCredits: return Int64; capture the sign before the locale-separator strip so a negative balance survives parsing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…test-wallet-persistence
issues addressed. The security risk is accepted - the wallet has only testnet funds |
`app.buttons["Done"]` matches the first Done button anywhere in the accessibility tree. Today the discovery sheet is the only thing with a Done button while this code runs, so it works — but a keyboard accessory, system overlay, or future sheet stacked in front would silently tap the wrong button. Scope the lookup to the sheet's nav bar that's already captured in the same function. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all three findings against the worktree at d3503c8. Codex's blocking finding about the network-switch handler caching a wallet manager bound to the old SDK is real: AppState's currentNetwork setter only kicks off an async SDK rebuild, while the SwiftUI .onChange handler synchronously calls activateManager(...) using the still-stale platformState.sdk, and WalletManagerStore caches that mis-bound manager for the new network forever. Claude's two findings are lower-severity but valid: the xcresult upload on failure() still archives the testnet mnemonic through typeText in the activity log (documented tradeoff), and the mnemonic word-count guard only splits on literal spaces.
Reviewed commit: d3503c8
🔴 1 blocking | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: Network switch caches a wallet manager bound to the previous network's SDK
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift (lines 142-145)
AppState.currentNetwork's didSet only enqueues an async SDK rebuild via beginNetworkSwitch (AppState.swift:29-34, 53-63, 141-174) — platformState.sdk is still the previous network's SDK at the moment the SwiftUI scene's .onChange(of: platformState.currentNetwork) handler fires. That handler synchronously calls activateManager(for: newNetwork), which passes the stale platformState.sdk straight into walletManagerStore.activate(network: newNetwork, sdk: sdk). WalletManagerStore.activate then constructs a new PlatformWalletManager, calls manager.configure(sdk: oldSdk, ...), runs loadFromPersistor against the old SDK's network, and caches the result in managers[newNetwork] (WalletManagerStore.swift:74-114). PlatformWalletManager is explicitly documented as network-locked to its configure-time SDK for its lifetime, and the cache is never invalidated. After the first switch into a not-yet-warm network, that network's manager is permanently bound to the previous network's SDK and persistence scope — visible as wallets restoring/syncing under the wrong backend until the app is relaunched. Activation must wait for the SDK rebuild to land (await isSwitchingNetwork to clear, or trigger from sdk.didSet/an explicit signal after switchNetwork's sdk = newSDK) before calling walletManagerStore.activate; alternatively, build the per-network SDK locally in activateManager the same way backgroundManager(for:) does (WalletManagerStore.swift:136-148) instead of reading platformState.sdk.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [BLOCKING] lines 142-145: Network switch caches a wallet manager bound to the previous network's SDK
AppState.currentNetwork's didSet only enqueues an async SDK rebuild via beginNetworkSwitch (AppState.swift:29-34, 53-63, 141-174) — platformState.sdk is still the previous network's SDK at the moment the SwiftUI scene's .onChange(of: platformState.currentNetwork) handler fires. That handler synchronously calls activateManager(for: newNetwork), which passes the stale platformState.sdk straight into walletManagerStore.activate(network: newNetwork, sdk: sdk). WalletManagerStore.activate then constructs a new PlatformWalletManager, calls manager.configure(sdk: oldSdk, ...), runs loadFromPersistor against the old SDK's network, and caches the result in managers[newNetwork] (WalletManagerStore.swift:74-114). PlatformWalletManager is explicitly documented as network-locked to its configure-time SDK for its lifetime, and the cache is never invalidated. After the first switch into a not-yet-warm network, that network's manager is permanently bound to the previous network's SDK and persistence scope — visible as wallets restoring/syncing under the wrong backend until the app is relaunched. Activation must wait for the SDK rebuild to land (await isSwitchingNetwork to clear, or trigger from sdk.didSet/an explicit signal after switchNetwork's `sdk = newSDK`) before calling walletManagerStore.activate; alternatively, build the per-network SDK locally in activateManager the same way backgroundManager(for:) does (WalletManagerStore.swift:136-148) instead of reading platformState.sdk.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
No new code changes were pushed after d3503c83, and the blocking wallet-manager rebinding issue still applies.
Requesting changes because SwiftExampleAppApp activates the per-network PlatformWalletManager synchronously in .onChange(of: platformState.currentNetwork) while AppState is still rebuilding platformState.sdk asynchronously. On a first switch into a not-yet-warm network, activateManager(for: newNetwork) can configure and cache that network's manager with the previous network's SDK. Since WalletManagerStore returns the cached manager on later activations, the wrong-SDK binding can persist until relaunch.
Please defer manager activation until the new SDK has landed (for example, trigger activation from an explicit post-sdk = newSDK readiness signal / SDK change), or construct the manager with a fresh SDK for the target network instead of reading the currently-stale platformState.sdk.
…le-SDK race The SwiftUI `.onChange(of: platformState.currentNetwork)` handler in SwiftExampleAppApp fires synchronously the moment `currentNetwork` changes, while `AppState.switchNetwork` rebuilds `platformState.sdk` asynchronously. `activateManager` was reading `platformState.sdk` in that window — i.e. the *previous* network's SDK — and handing it to `walletManagerStore.activate`, which constructs and caches a `PlatformWalletManager` for the new network bound to that stale SDK. The manager is network-locked to its configure-time SDK and the cache is never invalidated, so the new network's manager was permanently routing through the previous network's backend. Build the per-network SDK locally inside `activateManager`, the same way `WalletManagerStore.backgroundManager(for:)` already does, so the cached manager is born correctly scoped and doesn't depend on the shared SDK pointer being up-to-date at handler-fire time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all three remaining findings against the worktree at 2624335. Codex's blocking issue is real: useDockerSetup.didSet calls beginNetworkSwitch() which rebuilds platformState.sdk for the same currentNetwork, but the only WalletManagerStore reactivation hook fires on .onChange(of: platformState.currentNetwork) (SwiftExampleAppApp.swift:142) — so a Docker toggle on regtest leaves the cached PlatformWalletManager permanently bound to the pre-toggle SDK (WalletManagerStore.swift:87-93 short-circuits on cache hit). The two lower-severity items (failure-mode xcresult mnemonic exposure, ASCII-space-only word-count guard) were independently flagged by both agents and are confirmed in the current files. Prior findings about the network-switch stale-SDK race, the bestEffortDeleteWallet alert fallback, and UInt64(bitPattern:) reinterpretation are resolved at HEAD.
Reviewed commit: 2624335
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 additional findings
🔴 blocking: Docker-setup toggle leaves the active wallet manager bound to the old SDK on regtest
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift (lines 142-145)
AppState.useDockerSetup.didSet (AppState.swift:38-46) calls beginNetworkSwitch(), which rebuilds platformState.sdk against the same currentNetwork. But this scene only reactivates WalletManagerStore from .onChange(of: platformState.currentNetwork) here at line 142, and WalletManagerStore.activate(network:sdk:) short-circuits when a manager is already cached for the network (WalletManagerStore.swift:87-93). So toggling the Docker / local-host setup on regtest swaps the shared platformState.sdk but leaves the active PlatformWalletManager configured with the pre-toggle SDK for the rest of the session. Wallet operations, persistence scope, and SPV/BLAST coordinators continue to talk to the wrong backend even though the UI reports the switch is done. Either listen for useDockerSetup (or a more general "SDK identity" key) and rebuild/reactivate the manager, or invalidate the cache entry for network before re-activating so the new SDK is picked up. Fix is in the same area as the network-switch race that was just closed; the cache invariant needs to cover same-network SDK rebuilds, not only network changes.
🟡 suggestion: Failure-only xcresult upload still archives the testnet mnemonic in the activity log
.github/workflows/swift-example-app-ui-smoke.yml (lines 198-212)
WalletFlow.importWallet types UI_TEST_MNEMONIC into a plain SwiftUI TextField (not a SecureField), and Xcode records typeText(...) arguments verbatim in the xcresult activity log. The narrowed if: failure() guard and 7-day retention shrink the leak window but don't close it: any failed run produces an artifact from which the full mnemonic is recoverable by anyone with actions: read for the retention window. Threat model is bounded (workflow_dispatch / schedule only, testnet-only funds, inline comment at 199-205 documents the tradeoff), so this is an accepted tradeoff rather than an oversight. If you want stronger mitigations: strip the activity log with xcrun xcresulttool before upload so only screenshots / crash logs ship; pass the mnemonic via pasteboard so it never becomes a typeText argument; or treat any failed-run artifact as a signal to rotate the testnet secret.
💬 nitpick: Mnemonic word-count guard splits only on the ASCII space character
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/CreditTransferTest.swift (lines 70-74)
mnemonic.split(separator: " ").count == 12 recognizes only U+0020. A future secret rotation that introduces tabs, NBSPs, or a trailing newline would either misfire this guard (tab-separated phrase counted as 1, failing here on a valid input) or pass an unsanitized string through to importWallet's typeText — a trailing \n on some keyboards inserts a return and submits the form early. Low-stakes since the secret is normally clean, but it surfaces only after rotation. Trim and split on CharacterSet.whitespacesAndNewlines, then rejoin into a canonical single-space form before passing to importWallet.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- [BLOCKING] lines 142-145: Docker-setup toggle leaves the active wallet manager bound to the old SDK on regtest
`AppState.useDockerSetup.didSet` (AppState.swift:38-46) calls `beginNetworkSwitch()`, which rebuilds `platformState.sdk` against the *same* `currentNetwork`. But this scene only reactivates `WalletManagerStore` from `.onChange(of: platformState.currentNetwork)` here at line 142, and `WalletManagerStore.activate(network:sdk:)` short-circuits when a manager is already cached for the network (WalletManagerStore.swift:87-93). So toggling the Docker / local-host setup on regtest swaps the shared `platformState.sdk` but leaves the active `PlatformWalletManager` configured with the pre-toggle SDK for the rest of the session. Wallet operations, persistence scope, and SPV/BLAST coordinators continue to talk to the wrong backend even though the UI reports the switch is done. Either listen for `useDockerSetup` (or a more general "SDK identity" key) and rebuild/reactivate the manager, or invalidate the cache entry for `network` before re-activating so the new SDK is picked up. Fix is in the same area as the network-switch race that was just closed; the cache invariant needs to cover same-network SDK rebuilds, not only network changes.
In `.github/workflows/swift-example-app-ui-smoke.yml`:
- [SUGGESTION] lines 198-212: Failure-only xcresult upload still archives the testnet mnemonic in the activity log
`WalletFlow.importWallet` types `UI_TEST_MNEMONIC` into a plain SwiftUI `TextField` (not a `SecureField`), and Xcode records `typeText(...)` arguments verbatim in the xcresult activity log. The narrowed `if: failure()` guard and 7-day retention shrink the leak window but don't close it: any failed run produces an artifact from which the full mnemonic is recoverable by anyone with `actions: read` for the retention window. Threat model is bounded (workflow_dispatch / schedule only, testnet-only funds, inline comment at 199-205 documents the tradeoff), so this is an accepted tradeoff rather than an oversight. If you want stronger mitigations: strip the activity log with `xcrun xcresulttool` before upload so only screenshots / crash logs ship; pass the mnemonic via pasteboard so it never becomes a `typeText` argument; or treat any failed-run artifact as a signal to rotate the testnet secret.
Inline posting hit GitHub HTTP 422, so I posted the verified findings as a top-level review body.
…test-wallet-persistence # Conflicts: # packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated against worktree at 4fe6259. The 2624335→4fe625908 delta is dominated by a v3.1-dev merge (#3634 asset-lock identity registration, #3653 deleteWallet wipe, CI changes); the only PR-scoped change is an in-flight-registration gate on the OptionsView network picker, which is well-formed. No new findings in the latest delta. All three prior findings from 2624335 remain STILL VALID at HEAD and are carried forward: a blocking bug where the Docker-setup toggle leaves the cached PlatformWalletManager bound to the pre-toggle SDK (currentNetwork doesn't change, so the scene handler never fires; and the cache-hit path in WalletManagerStore.activate returns early without rebinding the cached manager), plus the two open suggestion/nitpick items around testnet mnemonic exposure in failure-mode xcresult uploads and the literal-space word-count guard.
Reviewed commit: 4fe6259
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s) omitted from inline comments.
Inline posting hit GitHub HTTP 422, so I posted the verified findings as a top-level review body.
3 participants
Issue being fixed or feature implemented
Extends the SwiftExampleApp UI test suite with an end-to-end test that exercises the SDK paths the existing local-only wallet tests don't touch: importing a wallet from a known mnemonic, DIP-9 identity discovery, and reading the discovered identity's balance from Platform.
This is one regression check across roughly half the SDK —
PlatformWallet.fromMnemonic,platform_wallet_discover_identities, DIP-9 TRANSFER key derivation, DAPI lookup, balance refresh.What was done?
CreditTransferTest.testImportWalletAndDiscoverIdentity— imports a testnet wallet fromUI_TEST_TESTNET_MNEMONIC, runs identity discovery, asserts the expected identity ID surfaces with a non-zero balance. Skipped when the env var is unset, so the rest of the suite continues to run locally without test-network credentials.accessibilityValueon the balance label so the raw credit count is parseable from XCUITest (the displayed%.8f DASHrounding hides amounts below 1000 credits).WalletFlow.swiftfor testnet flows: import-with-mnemonic, network switch, identity discovery, balance read, plus credit-transfer form navigation/execution scaffolding kept in place for the follow-up that re-adds the transfer assertion.The class and file are named
CreditTransferTestso the deferred transfer assertion can be re-added as a sibling method — helpers are already wired up.How Has This Been Tested?
UI_TEST_TESTNET_MNEMONICexported through Xcode's scheme env vars (note:xcodebuild test ENV=...does not propagate env vars to the XCUITest runner — use theTEST_RUNNER_<NAME>prefix from the command line, see the file's leading comment).3ou98WEERy6ExmmHWYWsFtyhgW8rmr1giceZYTFqdAAAwith a non-zero balance.testCreateGeneratedWalletFlow,testWalletPersistsAcrossRelaunch,testWalletDeletionCleanupSurvivesRelaunch) still pass.Breaking Changes
None — accessibility identifiers and
accessibilityValueare additive; no app-side logic changed.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Accessibility
UI
Documentation
Chores