feat(dashpay): wire the DashConnect connections flow to Dash Platform - #909
feat(dashpay): wire the DashConnect connections flow to Dash Platform#909romchornyi wants to merge 6 commits into
Conversation
Brings the DashConnect connections flow across from feat/dash-connect, which branched before the Swift SDK landed and so cannot host the real Platform implementation. Only the feature itself is carried over — the 273 commits that branch is ahead by are master merges and other features already present here. The screen is split into Components/ rather than the original single 461-line file: list, row, status badge, the two empty states, the scan button and the approve-sheet presentation modifier. Wired into the project as two PBXFileSystemSynchronizedRootGroups (Models and UI), so files added under DashConnect/ are picked up without per-file bookkeeping. DashUIKit moves from master to fix/textfield-prompt-type: the components need DashIcon and SwitchView, and current DashUIKit master does not yet carry the TextField-prompt compile fix (dashpay/DashUIKit#9). Still mock-backed. The Android side (dashpay/dash-wallet feat/dash-connect) has the real protocol — dash-key:/dash-st: URIs, the loginKeyResponse contract, and the key-exchange crypto — which this branch can now implement against the SDK.
Replaces raw asset-name strings with the DashIcon enums the library now exposes, so a renamed asset becomes a compile error instead of a blank image. Five references were already broken by DashUIKit's catalog normalization and were failing silently: CrowdNodeBalanceReminderBanner warning_triangle CoinbaseMetadataProvider transaction-coinbase.received (x2) RefundAddressView info-rect OrderPreviewView stopwatch Plus SwapPortalScaffold's menu-receive.disabled / menu-send.disabled, which degraded quietly: its disabledMenuIcon falls back to the enabled icon when the asset is missing, so disabled Buy/Sell rows rendered as enabled. Targets that take the app's own IconName keep .custom(...) and source the name from DashIcon.assetName; the rest use .source or .image directly. Also converts .font(Font.dash.X) to .dashFont(.X) so the design line height is applied with the font. TextField prompts are deliberately left on .font — a prompt must stay a Text, and a line height cannot apply to Text.
Turns the mock DashConnect screen into the real passwordless-login flow ported from Android (`dashpay/dash-wallet@feat/dash-connect`), testnet only. Protocol (`Sources/Models/DashConnect/Protocol/`) - `DashConnectUri`: `dash-key:` login and `dash-st:` key-registration URIs — `<scheme>:<Base58, no checksum>?n=<m|t|d>&v=1`, no authority. - `KeyExchangeCrypto` / `LoginKeyDerivation`: HKDF, AES-GCM and the login-key derivation, asserted against the Kotlin implementation's own test vectors. - `Secp256k1`: thin wrapper over the SwiftDashSDK primitives added in dashpay/platform#4273, so the port adds no third-party crypto dependency. Data layer - `PlatformDashConnectDataSource` publishes `loginKeyResponse` to the key exchange contract on approve, and completes `dash-st:` key registration by validating the app-supplied transition against locally derived keys before rebuilding it through `updateIdentity`. - `UserDefaultsDashConnectStore` keeps the connection list across launches, scoped per (network, wallet). The list is local state by design: Platform only knows whether the document exists. - `MockDashConnectDataSource` stays behind the same protocol for previews and the mainnet-unavailable state. UI - The status now follows what is true on Platform rather than which step ran: `approved` means the derived login keys are not on the identity yet, `active` means they are. `dash-st:` is first-login-only per (identity, app), so a later login with just the `dash-key:` QR lands straight on `active` instead of sticking on `approved` with a prompt for a QR the app no longer emits. - One scan entry point — the banner under an approved row — which accepts either QR code. - The row's switch removes this wallet's record of the connection, behind a confirmation that says the app may stay signed in: a website logout is invisible to the wallet, and the wallet cannot end the app's session. Also repairs the unit-test target, which was unrunnable: it now hosts on `dashpay` (the app that actually builds on this branch), the UI-test targets are out of the scheme, and the test sources import `dashpay` accordingly. Localizable.strings are deliberately left out — the build phase rewrites all 40 locales with strings from unrelated in-flight features. They go through BartyCrouch/Transifex separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughDashConnect adds URI parsing, cryptographic key exchange, connection persistence, platform integration, approval UI, Tools navigation, assets, and comprehensive tests. The project also updates Xcode target wiring, reachability initialization, and DashUIKit references. ChangesDashConnect implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectionsScreen
participant ConnectionsViewModel
participant PlatformDashConnectDataSource
participant DashConnectStore
User->>ConnectionsScreen: Scan QR code
ConnectionsScreen->>ConnectionsViewModel: Submit scanned URI
ConnectionsViewModel->>PlatformDashConnectDataSource: Parse and approve request
PlatformDashConnectDataSource->>DashConnectStore: Save connection
PlatformDashConnectDataSource-->>ConnectionsViewModel: Return connection state
ConnectionsViewModel-->>ConnectionsScreen: Update approval or error UI
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
romchornyi
changed the title
The base finished the DashSync unlink while this branch was open, which took away two APIs DashConnect was using. - `DWEnvironment.sharedInstance().currentChain.chainType.tag == ChainType_TestNet` is gone; the testnet gate now reads `WalletEnvironment.isTestnet`. - `NSData.base58String()` came from the DashSync pod; contract ids now encode through SwiftDashSDK's `Data.toBase58String()`, which uses the same alphabet and, like the old call, emits no checksum. Conflicts resolved: - `CoinbaseMetadataProvider.makeMetadata` — kept the base's new `icon` parameter together with this branch's `DashIcon` asset reference. - `project.pbxproj` — kept the new `Secp256k1Tests.swift` reference and dropped `DSAccount+SpentInputCheck.m`, which the base deleted. Verified with a clean `dashpay` build (`pod install` first — the base removed the DashSync pod). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Device testing found the row's switch did nothing, and reading the Android
original showed the behaviour behind it was wrong too.
**The switch never received the tap.** DashUIKit's `SwitchView` owns its
gesture at its intrinsic 64×28 geometry, but the row scaled it with
`scaleEffect(0.75)` inside a smaller outer frame — `scaleEffect` changes only
the drawing, so the tap region no longer lined up with what the user saw. The
switch is now a non-interactive indicator and the row owns the gesture over the
whole visible area, which also removes the `Binding(get: { true })` workaround.
**Turning it off returns the row to `approved`, not deletion.** That matches
`PlatformDashConnectRepository.disconnect` on Android and Figma 5805:51555: the
post-toggle state is "Approved" plus the scan-to-log-in banner, with no
confirmation dialog — the toggle itself is the action. This also closes the
dead end found on device: after signing out on the app's website the user taps
the switch, rescans the `dash-key:` QR, and the key check from the previous
commit puts the row straight back to `active`. `approved` means "awaiting
login", which is exactly what a logged-out connection is; the earlier reading
of it as "keys not registered" was too narrow.
Also fixes the `Active` row wrapping its timestamp: the status badge carried
`maxWidth: .infinity` as well, so it split the row with the name column and was
capped below the width the date needs. It now takes its natural width, with
Android's 12pt gap, and both columns are single-line.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not DashConnect — this predates the branch, but the warning shows up on every launch of the QA build: Thread running at User-interactive quality-of-service class waiting on a lower QoS thread running at Utility quality-of-service class. `startMonitoring()` blocked its caller on a semaphore only the monitor's own `.utility` queue could signal, and every caller is on the main thread (`DWHomeModel` init and `retrySyncing`, `startNetworkMonitoring`, `NetworkUnavailableStateView`). `DispatchSemaphore` does not propagate the waiter's QoS, so the main thread parked behind a utility-priority thread for up to 200 ms on each start — `retrySyncing` does `stop` + `start`. The wait could not simply be deleted: `startNetworkMonitoring` reads `isReachable` immediately afterwards, so removing it would flash the offline state until the first path notification arrived. The state is now seeded from `NWPathMonitor.currentPath`, which is readable right after `start(queue:)` — the synchronous contract holds with no blocking and no semaphore. The stale `DSReachabilityManager` justification in the comment goes with it; DashSync is long unlinked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (18)
DashWallet/Sources/UI/SwapKit/SwapKitPortalView.swift (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort the
DashUIKitimport.SwiftLint reports Line 19 as unsorted. Place
import DashUIKitin the required import order.As per coding guidelines, Swift files must follow SwiftFormat/SwiftLint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/UI/SwapKit/SwapKitPortalView.swift` at line 19, Reorder the import declarations in SwapKitPortalView.swift so import DashUIKit follows the repository’s required SwiftFormat/SwiftLint alphabetical import order, without changing any other code.Sources: Coding guidelines, Linters/SAST tools
DashWallet/Sources/UI/Home/Tx Metadata/GiftCardMetadataProvider.swift (1)
102-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind
txRowMetadatainstead of force-unwrapping it.Both paths check
txRowMetadata != niland then usetxRowMetadata!. Use optional binding, such asif var existing = txRowMetadata, and store the updated value. Apply the same fix to both paths.As per coding guidelines, Swift files must follow SwiftFormat/SwiftLint.
Also applies to: 131-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/UI/Home/Tx` Metadata/GiftCardMetadataProvider.swift around lines 102 - 106, Update both txRowMetadata handling paths in the relevant metadata provider to use optional binding (for example, if var existing = txRowMetadata) instead of force-unwrapping after a nil check; apply the gift-card icon update to the bound value and assign it back as needed, preserving the existing creation path and SwiftFormat/SwiftLint style.Sources: Coding guidelines, Linters/SAST tools
DashWallet.xcodeproj/project.pbxproj (1)
13279-13304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack the temporary
DashUIKitbranch pin for removal.The
DashUIKitpackage requirement is pinned tobranch = "fix/textfield-prompt-type"instead of a released version or tag. The PR description already notes this pin is temporary until the related fix merges upstream, so this is expected for now.As per the PR objectives, "a temporary DashUIKit pin remains until its related fix merges", track this pin and switch back to a version-based requirement once the upstream fix lands, to avoid depending on a moving branch head in CI builds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet.xcodeproj/project.pbxproj` around lines 13279 - 13304, Track the temporary branch requirement in the XCRemoteSwiftPackageReference for DashUIKit. Once the upstream textfield prompt fix has merged, replace branch = "fix/textfield-prompt-type" with the appropriate released version or tag requirement, preserving the existing DashUIKit package reference and project dependency wiring.DashWallet/Sources/Models/DashConnect/DashConnectStore.swift (2)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the force unwrap in
storageKey.Line 56 force-unwraps
walletScope. The value is guarded, so the unwrap is safe today, but the repository guideline forbids force-unwrapping runtime optional properties, and SwiftLint reportsforce_unwrappinghere. Use guarded optional handling instead.As per coding guidelines: "Never force-unwrap location coordinates or runtime optional properties; use guarded optional handling with appropriate fallback or error behavior."
♻️ Proposed rewrite without the force unwrap
var storageKey: String { - let walletScope = walletIdHexProvider()?.trimmingCharacters(in: .whitespacesAndNewlines) - let walletSuffix = (walletScope?.isEmpty == false) ? walletScope! : "no-wallet" + let walletScope = walletIdHexProvider()? + .trimmingCharacters(in: .whitespacesAndNewlines) + .nilIfEmpty + let walletSuffix = walletScope ?? "no-wallet" return "dashconnect.connections.v1.\(network.rawValue).\(walletSuffix)" }If
nilIfEmptydoes not exist in the project, use this form instead:var storageKey: String { let trimmed = walletIdHexProvider()?.trimmingCharacters(in: .whitespacesAndNewlines) let walletSuffix: String if let trimmed, !trimmed.isEmpty { walletSuffix = trimmed } else { walletSuffix = "no-wallet" } return "dashconnect.connections.v1.\(network.rawValue).\(walletSuffix)" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/DashConnectStore.swift` around lines 54 - 58, Remove the force unwrap from the storageKey computed property by using guarded optional handling: assign the trimmed wallet ID only when it is non-nil and non-empty, otherwise use "no-wallet", then build the existing key format unchanged.Sources: Coding guidelines, Linters/SAST tools
133-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo private Base58 decoders implement the same algorithm. The DashConnect feature carries two independent copies of a Base58 decoder, and the project already exposes
Data.identifier(fromBase58:), whichPlatformDashConnectDataSourceuses at lines 245 and 1068. The copies have already diverged: one returnsnilfor an empty string, the other returns emptyData, and only one caches the 128-entry reverse table.
DashWallet/Sources/Models/DashConnect/DashConnectStore.swift#L133-L173: delete this decoder and validaterow.contractIdwithData.identifier(fromBase58:), keeping the existing 32-byte length check.DashWallet/Sources/Models/DashConnect/Protocol/DashConnectUri.swift#L200-L235: keep one decoder here only if the parser must accept payloads of arbitrary length thatData.identifier(fromBase58:)rejects; otherwise route this call through the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/DashConnectStore.swift` around lines 133 - 173, Remove the private base58Decode implementation in DashConnectStore.swift and validate row.contractId through Data.identifier(fromBase58:), preserving the existing 32-byte length check. In DashConnectUri.swift lines 200-235, remove its duplicate decoder and route parsing through the same shared helper unless arbitrary-length payload support is required; retain the local decoder only when that requirement is confirmed.DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/Contents.json (1)
2-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an SVG source for this new asset.
This imageset ships PNG at 1x/2x/3x. The repository guideline prefers SVG for new icons.
preserves-vector-representationhas no effect on a PNG-only imageset, so it is misleading here. If the source art is vector, exporticon.svgand use a single universal entry, as the sibling DashConnect imagesets do. If the art is raster illustration only, removepreserves-vector-representation.The
originalrendering intent is correct for a branded illustration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/Contents.json` around lines 2 - 26, Update the DashConnect empty imageset to use an SVG source with a single universal image entry when the artwork is vector, matching sibling DashConnect imagesets, and remove the PNG scale entries. If the artwork is raster-only, retain the PNG entries but remove the misleading preserves-vector-representation property; keep template-rendering-intent set to original.Sources: Coding guidelines, Learnings
DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift (1)
505-552: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated "mark active" block.
Lines 506-517 and lines 541-552 are identical. Both rewrite the pending connection to
.activewithnow(). Extract one private helper and call it from both places.♻️ Proposed helper
+ private func markConnectionActive(id: String) { + persistAndSend( + subject.value.map { connection in + guard connection.id == id else { return connection } + return DAppConnection( + id: connection.id, + name: connection.name, + url: connection.url, + status: .active, + updatedAt: now() + ) + } + ) + }Then replace both blocks with
markConnectionActive(id: pendingConnection.id).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift` around lines 505 - 552, Extract the duplicated connection-mapping logic into a private helper, such as markConnectionActive(id:), that updates the matching pending connection to .active using now() and sends the result through persistAndSend. Replace both inline persistAndSend blocks in the authorization flow with calls to this helper using pendingConnection.id.DashWalletTests/DashConnect/DashConnectStoreTests.swift (1)
132-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a nil wallet id.
makeStoreacceptswalletIdHex: String?, but every test passes a non-nil value. Theniland empty-string branches ofUserDefaultsDashConnectStore.storageKeyare untested. That branch produces the"no-wallet"suffix and contains the force unwrap flagged atDashWallet/Sources/Models/DashConnect/DashConnectStore.swiftline 56.Add a case that asserts a nil provider and a whitespace-only provider both isolate from a real wallet id.
💚 Proposed test
+ func testMissingWalletIdUsesSeparateScope() { + let noWalletStore = makeStore(network: .testnet, walletIdHex: nil) + let blankWalletStore = makeStore(network: .testnet, walletIdHex: " ") + let walletStore = makeStore(network: .testnet, walletIdHex: "wallet-a") + let connection = sampleConnection( + id: "EWR695MsqPUuW8EnTbYzD4KybNQD5n7CUDWydJYNg63F", + status: .approved, + updatedAt: Date(timeIntervalSince1970: 10) + ) + + noWalletStore.save([connection]) + + XCTAssertEqual(noWalletStore.load(), [connection]) + XCTAssertEqual(blankWalletStore.load(), [connection]) + XCTAssertEqual(walletStore.load(), []) + XCTAssertEqual(noWalletStore.storageKey, blankWalletStore.storageKey) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/DashConnectStoreTests.swift` around lines 132 - 142, Add a test alongside testWalletIdsAreIsolated that creates stores with nil and whitespace-only walletIdHex values plus a real wallet ID, then assert the nil and whitespace stores use the no-wallet isolation path and remain distinct from the real wallet store. Exercise save/load isolation for the nil and whitespace providers, and verify their storage keys do not collide with the real wallet key.DashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swift (2)
138-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
assert.Line 138 asserts the same condition that Line 139 checks. In debug builds the assert traps before the throw can run, so the
invalidPayloadLengthpath is unreachable in tests.♻️ Proposed refactor
- assert(combined.count == encryptedPayloadLength) guard combined.count == encryptedPayloadLength else { throw CryptoError.invalidPayloadLength }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swift` around lines 138 - 141, Remove the redundant assert in the key-exchange payload validation, leaving the guard that checks combined.count against encryptedPayloadLength and throws CryptoError.invalidPayloadLength. Keep the existing guard-based behavior unchanged.
164-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a dedicated constant for the identity-ID length.
Lines 166 and 179 validate
identityIdagainstloginKeyLengthwhile throwing.invalidIdentityIdLength. Both values are 32, so behavior is correct today. The mismatch between the constant and the error becomes a defect if either length changes.♻️ Proposed refactor
private static let loginKeyLength = 32 + private static let identityIdLength = 32- try requireLength(identityId, expected: loginKeyLength, error: CryptoError.invalidIdentityIdLength) + try requireLength(identityId, expected: identityIdLength, error: CryptoError.invalidIdentityIdLength)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swift` around lines 164 - 187, Introduce and use a dedicated identity-ID length constant in both deriveAuthPrivateKey and deriveEncryptionPrivateKey when validating identityId, while preserving the existing loginKeyLength validation and invalidIdentityIdLength errors.DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift (1)
481-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion pins a raw English string.
The test compares
error.localizedDescriptionagainst a literal sentence. The PR excludes localization updates, so the test passes today. Once the message is localized, the test fails on any non-English locale. Assert the error case instead and cover the copy separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift` around lines 481 - 490, Update testPendingApprovedConnectionErrorTellsUserToScanLoginQrFirst to assert the specific error case/type rather than comparing error.localizedDescription with a raw English string. Keep the user-facing message verification separate from this error-behavior test so localization changes do not make the assertion locale-dependent.DashWalletTests/DashConnect/KeyExchangeCryptoTests.swift (2)
100-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLine 117 asserts a tautology, and the test does not pin a cross-platform ciphertext.
kotlinContractIdisData(repeating: 0xcd, count: 32), socount == 32is always true and the field is unused by this test. Remove the assertion or remove the fixture.The test name implies parity with the Android implementation, but it only checks shape and round trip. A divergence in the Swift AES-GCM or HKDF path would still pass. Add the expected payload hex, as
testEncryptLoginKeyWithFixedNonceMatchesVectordoes at Line 31.💚 Proposed change
XCTAssertEqual(payload.count, 60) XCTAssertEqual(payload.prefix(kotlinFixedNonce.count), kotlinFixedNonce) XCTAssertEqual(decrypted, kotlinLoginKey) - XCTAssertEqual(kotlinContractId.count, 32) + XCTAssertEqual(payload.hexEncodedString(), "<expected hex from the Android vector>")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/KeyExchangeCryptoTests.swift` around lines 100 - 118, Update testKotlinFixedNoncePayloadHasExpectedShapeAndRoundTrips to remove the tautological kotlinContractId count assertion and unused fixture, then assert payload equality against the Android cross-platform ciphertext hex using the same vector style as testEncryptLoginKeyWithFixedNonceMatchesVector.
120-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the specific error in the rejection tests.
These tests accept any thrown error. An unrelated failure, for example an ECDH error, also satisfies them. The URI tests assert the exact error case; apply the same approach here so each validation guard is proven.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/KeyExchangeCryptoTests.swift` around lines 120 - 155, The rejection tests in testRejectsInvalidLoginKeyLength, testRejectsInvalidNonceLength, testRejectsInvalidIdentityIdLength, and testRejectsInvalidPayloadLength should assert the specific expected validation error rather than accepting any thrown error. Match each XCTAssertThrowsError result against the corresponding KeyExchangeCrypto error case, including both identity-key derivation calls, while preserving the existing invalid-input coverage.DashWallet/Sources/Models/DashConnect/Protocol/Secp256k1.swift (1)
45-60: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueThe
Data(privateKeyBytes)copies are not cleared.
defer { zero(&privateKeyBytes) }clears the[UInt8]buffer only. Lines 49, 79 create a freshDatacopy of the private key for the backend call. That copy stays in memory until it is deallocated. The exposure window is short, but the file otherwise takes care to wipe key material.Consider keeping a single
Datavalue and wiping it in thedefer, or accept the copy and document why.Also applies to: 68-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWallet/Sources/Models/DashConnect/Protocol/Secp256k1.swift` around lines 45 - 60, Update compressedPublicKey and the corresponding private-key handling in the referenced methods to ensure every Data copy containing private key material is explicitly wiped before return or error. Prefer maintaining one mutable Data buffer and wiping it via defer, while preserving the existing validation and error mapping behavior.DashWalletTests/DashConnect/Secp256k1Tests.swift (1)
46-66: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReplace the brute-force search with a pinned key pair.
The loop performs up to 16,384 iterations, and each iteration runs two ECDH operations. Line 55 computes
sharedBAon every iteration although the value is used only when Line 57 matches. A search will almost always succeed early, so the test is not flaky, but it does unnecessary elliptic-curve work on every run.Once you know a pair that yields a leading
0x00, store it as a constant. The test then becomes deterministic and instant.If you keep the search, move the
sharedBAcomputation into the matching branch.♻️ Minimal change if the search stays
let sharedAB = try Secp256k1.ecdhSharedX(privateKey: privateKeyA, publicKey: publicKeyB) - let sharedBA = try Secp256k1.ecdhSharedX(privateKey: privateKeyB, publicKey: publicKeyA) if sharedAB.first == 0x00 { + let sharedBA = try Secp256k1.ecdhSharedX(privateKey: privateKeyB, publicKey: publicKeyA) XCTAssertEqual(sharedAB, sharedBA) return (privateKeyA, publicKeyB) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/Secp256k1Tests.swift` around lines 46 - 66, Replace findLeadingZeroSharedXCandidate’s brute-force key search with constants for a known private/public key pair whose shared X begins with 0x00, preserving the existing return shape and equality assertion. If retaining the search instead, move sharedBA computation inside the sharedAB.first == 0x00 branch so ECDH is only performed for matching candidates.DashWalletTests/DashAmountFormatterTests.swift (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwiftLint
sorted_importsfires in four test files after the module rename. The rename fromdashwallettodashpaymoved the testable import out of alphabetical order in each file. Fix the order in all four, or disable the rule for test files if the violation is intentional.
DashWalletTests/DashAmountFormatterTests.swift#L19-L19: reorder@testable import dashpayrelative to the other imports on Line 19.DashWalletTests/ExchangeAddressLookupContextTests.swift#L21-L21: reorder@testable import dashpayrelative to the other imports on Line 21.DashWalletTests/String+DashWalletTests.swift#L19-L19: reorder@testable import dashpayrelative to the other imports on Line 19.DashWalletTests/SwapAddressValidatorTests.swift#L21-L21: reorder@testable import dashpayrelative to the other imports on Line 21.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashAmountFormatterTests.swift` at line 19, Reorder the `@testable` import dashpay statement to satisfy SwiftLint’s sorted_imports rule in DashWalletTests/DashAmountFormatterTests.swift:19-19, DashWalletTests/ExchangeAddressLookupContextTests.swift:21-21, DashWalletTests/String+DashWalletTests.swift:19-19, and DashWalletTests/SwapAddressValidatorTests.swift:21-21, preserving the existing imports and test behavior.Source: Linters/SAST tools
DashWalletTests/DashConnect/DashConnectDataSourceTests.swift (1)
147-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe base58 encoder is copied verbatim into two test files. Both files carry an identical 30-line
base58Encodeimplementation plus thedata(_:)concatenation helper. Extract one shared test helper so the encoder has a single definition.
DashWalletTests/DashConnect/DashConnectDataSourceTests.swift#L147-L179: remove the localbase58Encodeand call the shared helper.DashWalletTests/DashConnect/DashConnectUriTests.swift#L158-L190: remove the localbase58Encodeand call the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/DashConnect/DashConnectDataSourceTests.swift` around lines 147 - 179, Extract the duplicated base58Encode implementation and its data(_:) concatenation helper into one shared test helper. Remove the local definitions from DashWalletTests/DashConnect/DashConnectDataSourceTests.swift lines 147-179 and DashWalletTests/DashConnect/DashConnectUriTests.swift lines 158-190, and update both test files to call the shared helper.DashWalletTests/PaymentProtocolTests.swift (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the SwiftLint import-order warnings.
SwiftLint reports
sorted_importsfor both changed imports. Sort each complete import block according to the configured rule.
DashWalletTests/PaymentProtocolTests.swift#L17-L17: reorder the import block that contains@testable import dashpay.DashWalletTests/PhraseRepairEngineTests.swift#L23-L23: reorder the import block that contains@testable import dashpay.As per coding guidelines: “Follow the applicable language conventions: … SwiftFormat/SwiftLint.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DashWalletTests/PaymentProtocolTests.swift` at line 17, Resolve the SwiftLint sorted_imports warnings by reordering the complete import blocks containing `@testable` import dashpay in DashWalletTests/PaymentProtocolTests.swift:17-17 and DashWalletTests/PhraseRepairEngineTests.swift:23-23 according to the configured import-order convention.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json`:
- Around line 6-9: Update the template-rendering-intent property to "template"
in
DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json
lines 6-9 and
DashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/Contents.json
lines 6-9, preserving the existing vector representation settings.
In
`@DashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/Contents.json`:
- Around line 2-17: The menu-connections asset catalog entry should use a single
local SVG instead of the three PNG scale variants. Update the imageset
Contents.json to reference the SVG, enable preserves-vector-representation, and
configure template rendering so the consuming menu can apply tinting.
In `@DashWallet/Sources/Models/DashConnect/DashConnectStore.swift`:
- Around line 114-123: Update the logging in the connection-encoding catch block
and private logDrop method to use OSLog with explicit privacy annotations,
removing storageKey from both messages. Log only the network and relevant error
or drop reason, keeping the wallet scope private as established by
PlatformDashConnectDataSource.
In `@DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`:
- Around line 455-495: Update completeKeyRegistration and its
pendingApprovedConnectionForKeyRegistration flow to correlate the dash-st
transition with the correct approved connection instead of selecting the newest
approval. Preserve support for transitions whose contractBounds is nil, and
ensure key derivation uses the matched app contract so scanning app A after app
B succeeds. Add coverage for two approved connections scanned in non-recency
order.
- Around line 380-408: Replace the String(describing: error) matching in the
login-key document create flow with typed SDK duplicate-document error handling.
Only invoke findExistingLoginKeyResponseDocumentId and replaceDocument for the
specific duplicate error; otherwise rethrow the original error, removing the
redundant "duplicate unique" check. If no typed SDK error is available, check
whether the login-key document exists before choosing createDocument or
replaceDocument.
In `@DashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swift`:
- Around line 54-82: Update hash160 to make platform_wallet_hash160 failures
explicit by throwing CryptoError.hash160Failed, and adjust its callers to
propagate the error instead of treating an empty Data result as a valid hash.
Ensure comparisons cannot proceed with a zero-length result and preserve
successful hash computation behavior.
In `@DashWallet/Sources/UI/DashConnect/ApproveConnectionSheet.swift`:
- Around line 33-123: The fixed content VStack in body must support vertical
scrolling at large Dynamic Type sizes. Wrap the variable approval content,
including the permission sections and both Approve and Deny DashButton actions,
in a vertical ScrollView while preserving the existing layout and ensuring both
actions remain reachable after content expands.
In `@DashWallet/Sources/UI/DashConnect/Components/ConnectionRow.swift`:
- Around line 89-101: Update the activeSwitch control’s frame to provide at
least a 44-point height while preserving its existing disconnect gesture,
accessibility configuration, and switch appearance.
In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift`:
- Around line 302-306: Update showConnections() to instantiate and push the thin
hosting-controller subclass used for SwiftUI screens instead of a bare
UIHostingController, while preserving the ConnectionsScreen root view and
hidesBottomBarWhenPushed setting.
In `@DashWalletTests/AmountObjectTests.swift`:
- Line 19: Restore the SwiftLint-required import ordering for `@testable` import
dashpay in DashWalletTests/AmountObjectTests.swift (19-19),
DashWalletTests/DWAvatarUploadClientTests.swift (10-10),
DashWalletTests/DWContestedNameStatusServiceTests.swift (11-11),
DashWalletTests/DWRegistrationPhaseAdapterTests.swift (13-13),
DashWalletTests/DashConnect/LoginKeyDerivationTests.swift (2-2),
DashWalletTests/DiagnosticLogExporterTests.swift (33-33),
DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift (9-9), and
DashWalletTests/WalletWipeSerialExecutorTests.swift (11-11); move each import to
the position required by the sorted_imports rule without changing other imports.
In `@DashWalletTests/DashConnect/DashConnectDataSourceTests.swift`:
- Around line 127-137: Remove force_try from both DashConnect test fixtures by
making validKeyUri in DashConnectDataSourceTests.swift throw, replacing try!
with try, and propagating try to its callers at lines 12, 57, and 69. In
DashConnectUriTests.swift, make validKeyPayload throw, replace its try! with
try, and propagate throws through validKeyUri and all of its callers.
- Around line 109-125: Update testApprovingSameAppTwiceReplacesExistingRow to
capture the current Date immediately before calling approveLogin, then assert
the replacement connection’s updatedAt is later than that captured time instead
of comparing against the hard-coded initialDate.
---
Nitpick comments:
In `@DashWallet.xcodeproj/project.pbxproj`:
- Around line 13279-13304: Track the temporary branch requirement in the
XCRemoteSwiftPackageReference for DashUIKit. Once the upstream textfield prompt
fix has merged, replace branch = "fix/textfield-prompt-type" with the
appropriate released version or tag requirement, preserving the existing
DashUIKit package reference and project dependency wiring.
In
`@DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/Contents.json`:
- Around line 2-26: Update the DashConnect empty imageset to use an SVG source
with a single universal image entry when the artwork is vector, matching sibling
DashConnect imagesets, and remove the PNG scale entries. If the artwork is
raster-only, retain the PNG entries but remove the misleading
preserves-vector-representation property; keep template-rendering-intent set to
original.
In `@DashWallet/Sources/Models/DashConnect/DashConnectStore.swift`:
- Around line 54-58: Remove the force unwrap from the storageKey computed
property by using guarded optional handling: assign the trimmed wallet ID only
when it is non-nil and non-empty, otherwise use "no-wallet", then build the
existing key format unchanged.
- Around line 133-173: Remove the private base58Decode implementation in
DashConnectStore.swift and validate row.contractId through
Data.identifier(fromBase58:), preserving the existing 32-byte length check. In
DashConnectUri.swift lines 200-235, remove its duplicate decoder and route
parsing through the same shared helper unless arbitrary-length payload support
is required; retain the local decoder only when that requirement is confirmed.
In `@DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`:
- Around line 505-552: Extract the duplicated connection-mapping logic into a
private helper, such as markConnectionActive(id:), that updates the matching
pending connection to .active using now() and sends the result through
persistAndSend. Replace both inline persistAndSend blocks in the authorization
flow with calls to this helper using pendingConnection.id.
In `@DashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swift`:
- Around line 138-141: Remove the redundant assert in the key-exchange payload
validation, leaving the guard that checks combined.count against
encryptedPayloadLength and throws CryptoError.invalidPayloadLength. Keep the
existing guard-based behavior unchanged.
- Around line 164-187: Introduce and use a dedicated identity-ID length constant
in both deriveAuthPrivateKey and deriveEncryptionPrivateKey when validating
identityId, while preserving the existing loginKeyLength validation and
invalidIdentityIdLength errors.
In `@DashWallet/Sources/Models/DashConnect/Protocol/Secp256k1.swift`:
- Around line 45-60: Update compressedPublicKey and the corresponding
private-key handling in the referenced methods to ensure every Data copy
containing private key material is explicitly wiped before return or error.
Prefer maintaining one mutable Data buffer and wiping it via defer, while
preserving the existing validation and error mapping behavior.
In `@DashWallet/Sources/UI/Home/Tx` Metadata/GiftCardMetadataProvider.swift:
- Around line 102-106: Update both txRowMetadata handling paths in the relevant
metadata provider to use optional binding (for example, if var existing =
txRowMetadata) instead of force-unwrapping after a nil check; apply the
gift-card icon update to the bound value and assign it back as needed,
preserving the existing creation path and SwiftFormat/SwiftLint style.
In `@DashWallet/Sources/UI/SwapKit/SwapKitPortalView.swift`:
- Line 19: Reorder the import declarations in SwapKitPortalView.swift so import
DashUIKit follows the repository’s required SwiftFormat/SwiftLint alphabetical
import order, without changing any other code.
In `@DashWalletTests/DashAmountFormatterTests.swift`:
- Line 19: Reorder the `@testable` import dashpay statement to satisfy SwiftLint’s
sorted_imports rule in DashWalletTests/DashAmountFormatterTests.swift:19-19,
DashWalletTests/ExchangeAddressLookupContextTests.swift:21-21,
DashWalletTests/String+DashWalletTests.swift:19-19, and
DashWalletTests/SwapAddressValidatorTests.swift:21-21, preserving the existing
imports and test behavior.
In `@DashWalletTests/DashConnect/DashConnectDataSourceTests.swift`:
- Around line 147-179: Extract the duplicated base58Encode implementation and
its data(_:) concatenation helper into one shared test helper. Remove the local
definitions from DashWalletTests/DashConnect/DashConnectDataSourceTests.swift
lines 147-179 and DashWalletTests/DashConnect/DashConnectUriTests.swift lines
158-190, and update both test files to call the shared helper.
In `@DashWalletTests/DashConnect/DashConnectStoreTests.swift`:
- Around line 132-142: Add a test alongside testWalletIdsAreIsolated that
creates stores with nil and whitespace-only walletIdHex values plus a real
wallet ID, then assert the nil and whitespace stores use the no-wallet isolation
path and remain distinct from the real wallet store. Exercise save/load
isolation for the nil and whitespace providers, and verify their storage keys do
not collide with the real wallet key.
In `@DashWalletTests/DashConnect/KeyExchangeCryptoTests.swift`:
- Around line 100-118: Update
testKotlinFixedNoncePayloadHasExpectedShapeAndRoundTrips to remove the
tautological kotlinContractId count assertion and unused fixture, then assert
payload equality against the Android cross-platform ciphertext hex using the
same vector style as testEncryptLoginKeyWithFixedNonceMatchesVector.
- Around line 120-155: The rejection tests in testRejectsInvalidLoginKeyLength,
testRejectsInvalidNonceLength, testRejectsInvalidIdentityIdLength, and
testRejectsInvalidPayloadLength should assert the specific expected validation
error rather than accepting any thrown error. Match each XCTAssertThrowsError
result against the corresponding KeyExchangeCrypto error case, including both
identity-key derivation calls, while preserving the existing invalid-input
coverage.
In `@DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift`:
- Around line 481-490: Update
testPendingApprovedConnectionErrorTellsUserToScanLoginQrFirst to assert the
specific error case/type rather than comparing error.localizedDescription with a
raw English string. Keep the user-facing message verification separate from this
error-behavior test so localization changes do not make the assertion
locale-dependent.
In `@DashWalletTests/DashConnect/Secp256k1Tests.swift`:
- Around line 46-66: Replace findLeadingZeroSharedXCandidate’s brute-force key
search with constants for a known private/public key pair whose shared X begins
with 0x00, preserving the existing return shape and equality assertion. If
retaining the search instead, move sharedBA computation inside the
sharedAB.first == 0x00 branch so ECDH is only performed for matching candidates.
In `@DashWalletTests/PaymentProtocolTests.swift`:
- Line 17: Resolve the SwiftLint sorted_imports warnings by reordering the
complete import blocks containing `@testable` import dashpay in
DashWalletTests/PaymentProtocolTests.swift:17-17 and
DashWalletTests/PhraseRepairEngineTests.swift:23-23 according to the configured
import-order convention.
🪄 Autofix
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 Plus
Run ID: ccffb43e-3639-4075-bf8d-0e216ce9fb0a
⛔ Files ignored due to path filters (11)
DashWallet.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolvedDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/dashconnect-empty.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/dashconnect-empty@2x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/dashconnect-empty@3x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.check.circle.imageset/icon.svgis excluded by!**/*.svgDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.qr.imageset/icon.svgis excluded by!**/*.svgDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/icon.svgis excluded by!**/*.svgDashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/menu-connections.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/menu-connections@2x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/menu-connections@3x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (75)
DashWallet.xcodeproj/project.pbxprojDashWallet.xcodeproj/xcshareddata/xcschemes/dashwallet-dashpay.xcschemeDashWallet/Resources/AppAssets.xcassets/DashConnect/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect-empty.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.check.circle.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.qr.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/Contents.jsonDashWallet/Sources/Infrastructure/Networking/NetworkReachability.swiftDashWallet/Sources/Models/DashConnect/DashConnectDataSource.swiftDashWallet/Sources/Models/DashConnect/DashConnectModels.swiftDashWallet/Sources/Models/DashConnect/DashConnectStore.swiftDashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swiftDashWallet/Sources/Models/DashConnect/Protocol/DashConnectRequests.swiftDashWallet/Sources/Models/DashConnect/Protocol/DashConnectUri.swiftDashWallet/Sources/Models/DashConnect/Protocol/KeyExchangeCrypto.swiftDashWallet/Sources/Models/DashConnect/Protocol/LoginKeyDerivation.swiftDashWallet/Sources/Models/DashConnect/Protocol/Secp256k1.swiftDashWallet/Sources/UI/CrowdNode/BalanceReminder/CrowdNodeBalanceReminderBanner.swiftDashWallet/Sources/UI/CrowdNode/BalanceReminder/CrowdNodeBalanceReminderSheet.swiftDashWallet/Sources/UI/DashConnect/ApproveConnectionSheet.swiftDashWallet/Sources/UI/DashConnect/Components/ApproveSheetPresentation.swiftDashWallet/Sources/UI/DashConnect/Components/ConnectionRow.swiftDashWallet/Sources/UI/DashConnect/Components/ConnectionStatusBadge.swiftDashWallet/Sources/UI/DashConnect/Components/ConnectionsEmptyState.swiftDashWallet/Sources/UI/DashConnect/Components/ConnectionsList.swiftDashWallet/Sources/UI/DashConnect/Components/ConnectionsUnavailableState.swiftDashWallet/Sources/UI/DashConnect/Components/ScanQRButton.swiftDashWallet/Sources/UI/DashConnect/Components/ScanToCompleteBanner.swiftDashWallet/Sources/UI/DashConnect/ConnectionsScreen.swiftDashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swiftDashWallet/Sources/UI/Home/Tx Metadata/CoinbaseMetadataProvider.swiftDashWallet/Sources/UI/Home/Tx Metadata/GiftCardMetadataProvider.swiftDashWallet/Sources/UI/Home/Tx Metadata/SwapOrderMetadataProvider.swiftDashWallet/Sources/UI/Home/Views/HomeView.swiftDashWallet/Sources/UI/Menu/Settings/About/AboutDashView.swiftDashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swiftDashWallet/Sources/UI/Menu/Tools/ToolsMenuViewModel.swiftDashWallet/Sources/UI/Swap/Buy/EnterAmount/BuyEnterAmountView.swiftDashWallet/Sources/UI/Swap/Buy/Receive/BuyReceiveView.swiftDashWallet/Sources/UI/Swap/Buy/RefundAddress/RefundAddressView.swiftDashWallet/Sources/UI/Swap/Convert/SwapConvertView.swiftDashWallet/Sources/UI/Swap/OrderPreview/Components/OrderPreviewFeeRow.swiftDashWallet/Sources/UI/Swap/OrderPreview/Components/OrderPreviewTableRow.swiftDashWallet/Sources/UI/Swap/OrderPreview/Components/SwapFeeInfoSheet.swiftDashWallet/Sources/UI/Swap/OrderPreview/OrderPreviewView.swiftDashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swiftDashWallet/Sources/UI/Swap/SwapPortalScaffold.swiftDashWallet/Sources/UI/Swap/TransactionStatus/SwapTransactionFailureView.swiftDashWallet/Sources/UI/Swap/TransactionStatus/SwapTransactionPendingView.swiftDashWallet/Sources/UI/SwapKit/SwapKitPortalView.swiftDashWalletTests/AmountObjectTests.swiftDashWalletTests/DWAvatarUploadClientTests.swiftDashWalletTests/DWContestedNameStatusServiceTests.swiftDashWalletTests/DWRegistrationPhaseAdapterTests.swiftDashWalletTests/DashAmountFormatterTests.swiftDashWalletTests/DashConnect/DashConnectDataSourceTests.swiftDashWalletTests/DashConnect/DashConnectStoreTests.swiftDashWalletTests/DashConnect/DashConnectUriTests.swiftDashWalletTests/DashConnect/KeyExchangeCryptoTests.swiftDashWalletTests/DashConnect/LoginKeyDerivationTests.swiftDashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swiftDashWalletTests/DashConnect/Secp256k1Tests.swiftDashWalletTests/DashPayIdentityKeysTests.swiftDashWalletTests/DiagnosticLogExporterTests.swiftDashWalletTests/ExchangeAddressLookupContextTests.swiftDashWalletTests/PastedAmountNormalizationTests.swiftDashWalletTests/PaymentProtocolTests.swiftDashWalletTests/PhraseRepairEngineTests.swiftDashWalletTests/String+DashWalletTests.swiftDashWalletTests/SwapAddressValidatorTests.swiftDashWalletTests/SwapKitQuoteDecodingTests.swiftDashWalletTests/SwiftDashSDKCoreLifecycleTests.swiftDashWalletTests/WalletWipeSerialExecutorTests.swift
💤 Files with no reviewable changes (1)
- DashWallet.xcodeproj/xcshareddata/xcschemes/dashwallet-dashpay.xcscheme
| "properties" : { | ||
| "preserves-vector-representation" : true, | ||
| "template-rendering-intent" : "original" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use template rendering for generic SVG UI icons.
Both assets use "original" despite being generic UI glyphs. This prevents the consuming UI from applying its tint.
DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json#L6-L9: set"template-rendering-intent"to"template".DashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/Contents.json#L6-L9: set"template-rendering-intent"to"template".
As per coding guidelines, “use the correct template or original rendering intent for the consuming UI.” Based on learnings, generic SVG UI icons must use template.
📍 Affects 2 files
DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json#L6-L9(this comment)DashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/Contents.json#L6-L9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json`
around lines 6 - 9, Update the template-rendering-intent property to "template"
in
DashWallet/Resources/AppAssets.xcassets/DashConnect/dashconnect.xmark.circle.imageset/Contents.json
lines 6-9 and
DashWallet/Resources/AppAssets.xcassets/DashConnect/menu.connections.imageset/Contents.json
lines 6-9, preserving the existing vector representation settings.
Sources: Coding guidelines, Learnings
| "images" : [ | ||
| { | ||
| "filename" : "menu-connections.png", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "filename" : "menu-connections@2x.png", | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "filename" : "menu-connections@3x.png", | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace the raster menu glyph with an SVG asset.
This new generic icon uses PNG files at three scales. Use a local SVG with "preserves-vector-representation": true and template rendering instead. This preserves sharp rendering at all display scales and permits menu tinting.
As per coding guidelines, “Prefer SVG over PNG for new icons, preserve vector representation, and use the correct template or original rendering intent for the consuming UI.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@DashWallet/Resources/AppAssets.xcassets/DashConnect/menu-connections.imageset/Contents.json`
around lines 2 - 17, The menu-connections asset catalog entry should use a
single local SVG instead of the three PNG scale variants. Update the imageset
Contents.json to reference the SVG, enable preserves-vector-representation, and
configure template rendering so the consuming menu can apply tinting.
Source: Coding guidelines
| do { | ||
| defaults.set(try encoder.encode(rows), forKey: storageKey) | ||
| } catch { | ||
| NSLog("DashConnectStore: failed to encode connections for %@: %@", storageKey, error.localizedDescription) | ||
| } | ||
| } | ||
|
|
||
| private func logDrop(_ reason: String) { | ||
| NSLog("DashConnectStore: dropping stored row for %@: %@", storageKey, reason) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the wallet-scoped storage key.
storageKey embeds the wallet id hex returned by walletIdHexProvider. Both NSLog calls write that identifier to the unified log unredacted, and logDrop runs once per dropped row. A wallet id is a persistent user identifier, so this leaks a user identifier into device logs.
Use OSLog with an explicit privacy annotation, as PlatformDashConnectDataSource does at line 240. Log the network and the reason, and keep the wallet scope private.
🔒️ Proposed fix
+import OSLog
import Foundation+ private static let logger = Logger(
+ subsystem: "org.dashfoundation.dash",
+ category: "dashconnect.store")
+
func save(_ connections: [DAppConnection]) { do {
defaults.set(try encoder.encode(rows), forKey: storageKey)
} catch {
- NSLog("DashConnectStore: failed to encode connections for %@: %@", storageKey, error.localizedDescription)
+ Self.logger.error(
+ "DashConnectStore: failed to encode connections for \(self.storageKey, privacy: .private): \(error.localizedDescription, privacy: .public)"
+ )
}
}
private func logDrop(_ reason: String) {
- NSLog("DashConnectStore: dropping stored row for %@: %@", storageKey, reason)
+ Self.logger.error(
+ "DashConnectStore: dropping stored row for \(self.storageKey, privacy: .private): \(reason, privacy: .public)"
+ )
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWallet/Sources/Models/DashConnect/DashConnectStore.swift` around lines
114 - 123, Update the logging in the connection-encoding catch block and private
logDrop method to use OSLog with explicit privacy annotations, removing
storageKey from both messages. Log only the network and relevant error or drop
reason, keeping the wallet scope private as established by
PlatformDashConnectDataSource.
Source: Coding guidelines
| } catch { | ||
| let errorText = String(describing: error).lowercased() | ||
| let shouldReplace = errorText.contains("duplicate unique") | ||
| || errorText.contains("already exists") | ||
| || errorText.contains("duplicate") | ||
| guard shouldReplace else { | ||
| throw error | ||
| } | ||
|
|
||
| let existingDocumentId = try await findExistingLoginKeyResponseDocumentId( | ||
| ownerIdentityId: context.identityId, | ||
| appContractId: request.contractId, | ||
| sdk: context.sdk | ||
| ) | ||
| let signingKeyId = try selectDocumentSigningKeyId( | ||
| wallet: context.wallet, | ||
| identityId: context.identityId | ||
| ) | ||
|
|
||
| _ = try await context.wallet.replaceDocument( | ||
| ownerIdentityId: context.identityId, | ||
| contractId: Self.loginKeyExchangeContractId, | ||
| documentType: Self.loginKeyExchangeDocumentType, | ||
| documentId: existingDocumentId, | ||
| propertiesJSON: propertiesJSON, | ||
| signingKeyId: signingKeyId, | ||
| signer: signer | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the SDK error type thrown by createDocument to check for a typed duplicate case.
ast-grep run --pattern 'func createDocument($$$) async throws -> $_ { $$$ }' --lang swift .
rg -nP --type=swift -C5 'enum \w*(Document|Platform|SDK)\w*Error'
rg -nP --type=swift -C3 '(duplicate|alreadyExists|DuplicateUnique)' -g '!DashWallet/Sources/Models/DashConnect/**'Repository: dashpay/dashwallet-ios
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- document operation call sites ---'
rg -n -C4 --type swift 'createDocument|replaceDocument|findExistingLoginKeyResponseDocumentId|approveLogin' DashWallet/Sources
printf '%s\n' '--- dependency and error references ---'
rg -n -i -C3 --type swift --glob '!DashWallet/Sources/Models/DashConnect/**' \
'DashSDK|DashPlatform|PlatformError|DocumentError|duplicate|already.?exists|unique constraint|unique' .
rg -n 'dash.*sdk|platform' Package.swift Package.resolved Podfile Podfile.lock Cartfile Cartfile.resolved 2>/dev/null || trueRepository: dashpay/dashwallet-ios
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target implementation ---'
sed -n '300,465p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- relevant calls and declarations ---'
rg -n -C3 --type swift \
'createDocument|replaceDocument|findExistingLoginKeyResponseDocumentId|approveLogin' \
DashWallet/Sources/Models/DashConnect
printf '%s\n' '--- package and SDK locations ---'
find . -maxdepth 5 \( -name 'Package.swift' -o -name 'Package.resolved' -o -name '*.podspec' -o -name 'Podfile' -o -name 'Podfile.lock' \) -print
git ls-files | rg -i 'swift.?dash.?sdk|platform.*(error|document)|dashconnect|package\.resolved|podfile'Repository: dashpay/dashwallet-ios
Length of output: 21110
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- existing-document lookup ---'
sed -n '900,1035p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- resolved SDK dependencies ---'
rg -n -i -C8 'swift.?dash|dash.?sdk|platform' \
DashWallet.xcworkspace/xcshareddata/swiftpm/Package.resolved Podfile.lock
printf '%s\n' '--- all SDK document API references ---'
rg -n -i -C3 --type swift \
'createDocument|replaceDocument|query.*document|fetch.*document|document.*query|documents' \
DashWallet DashWalletTestsRepository: dashpay/dashwallet-ios
Length of output: 5165
🌐 Web query:
SwiftDashSDK createDocument replaceDocument duplicate unique error Swift
💡 Result:
In the context of the Dash Platform SDKs (including SwiftDashSDK), errors related to document operations are typically categorized by specific protocol error codes [1][2]. When performing document operations, you may encounter the following relevant errors: DocumentAlreadyPresentError (Code 4004 or 40100): This error occurs when you attempt to create a document with an ID that already exists in the state [1][2]. Because document IDs must be unique, the platform rejects a "create" transition if the ID is already occupied. DuplicateUniqueIndexError (Code 4009 or 40105): This error occurs when a document violates a unique index constraint defined in the data contract, even if the document ID itself is new [1][2]. For document updates, the Dash Platform requires a replace operation rather than a simple write [3][4]. The standard workflow for replacing a document is as follows: 1. Query for the existing document to retrieve its current revision number [3][4]. 2. Construct a new document object with the same id, the updated content, and an incremented revision (existingDoc.revision + 1n) [3][4]. 3. Submit this object using the replace() method [3][4]. Attempting to "create" a document that already exists will result in a DocumentAlreadyPresentError, and attempting to "replace" a document without correctly incrementing the revision or providing the correct base ID will result in validation failures [3][4]. If you are encountering unexpected "unique" or "duplicate" errors, ensure that your data contract indices are correctly configured and that your logic distinguishes between creating new documents and updating existing ones by using the proper SDK methods (create vs replace) and managing document revisions [3][4].
Citations:
- 1: https://dash-user-docs.readthedocs.io/projects/platform/en/1.0-dev/docs/protocol-ref/errors.html
- 2: https://dashpay.github.io/platform/error-handling/error-codes.html
- 3: https://docs.dash.org/projects/platform/en/stable/docs/tutorials/contracts-and-documents/update-documents.html
- 4: https://docs.dash.org/projects/platform/en/latest/docs/tutorials/contracts-and-documents/update-documents.html
🌐 Web query:
site:github.com/dashpay SwiftDashSDK "createDocument" "replaceDocument"
💡 Result:
In the context of Dash Platform development, SwiftDashSDK is the Swift language wrapper for the Dash Platform SDK [1][2]. While the SDK provides methods for document lifecycle management, it typically interacts with documents via a broadcast pattern or through specific create and replace operations facilitated by the underlying DAPI and state transitions [3][4]. Document Lifecycle Patterns: 1. CreateDocument: To create a document, you generally use the sdk.documents.create method [3]. This operation typically requires the data contract identifier, the document type, the identity of the owner, and the document data itself [4]. The resulting document is then included in a state transition and broadcast to the network [4]. 2. ReplaceDocument: There is no direct "replaceDocument" method in the simplified interface; instead, the update flow follows a "fetch-modify-replace" pattern [3][5]. - Fetch: You must first retrieve the existing document using sdk.documents.get to obtain its current revision number [3][5]. - Modify: You create a replacement document with the desired changes and increment the revision number (new revision = existing revision + 1) [3]. - Replace: You submit the replacement via sdk.documents.replace [3][5]. State transitions that attempt to replace a document without correctly bumping the revision number are rejected by the network to prevent conflicts [3]. Documentation and Resources: Because SwiftDashSDK is a wrapper around the Rust-based core SDK, official documentation is often found within the repository's example applications and integration tests [3][6]. The "dashnote" example application is the most authoritative technical source for understanding these document flows [3][5]. You can find implementations in the src/dash/ directory of that repository, where createNote.ts demonstrates the creation flow and updateNote.ts demonstrates the update (replace) flow [3].
Citations:
- 1: feat(swift-sdk,rs-sdk-ffi): wire devnet SDK config + auto-discover masternodes platform#3755
- 2: chore(swift-sdk): correctly refer to the swift sdk as swift in the build script and cargo profiles platform#3949
- 3: https://github.com/dashpay/platform-tutorials/tree/main/example-apps/dashnote
- 4: https://github.com/dashpay/platform/tree/master/packages/js-dash-sdk
- 5: refactor(dashnote): redesign workspace with toolbar, activity log, and sign-in hero platform-tutorials#83
- 6: test(swift-sdk): first swift sdk integration tests with local network platform#3712
🌐 Web query:
site:github.com/dashpay "DocumentAlreadyPresentError" SwiftDashSDK
💡 Result:
DocumentAlreadyPresentError is a consensus-level error code (specifically 40100) within the Dash Platform protocol [1][2]. It indicates that an attempt was made to create or register a document that already exists in the platform state (for example, a document with the same unique identifier). Key points regarding this error: - Consensus Code: It is mapped to code 40100 in the Dash Platform Protocol (DPP) consensus error definitions [1][2]. - Origin: Because this is a consensus error, it originates from the Dash Platform's validation layer (implemented in Rust), rather than being a specific client-side error exclusively within the SwiftDashSDK [1][2]. - SwiftDashSDK context: While the error is defined in the shared core (rs-dpp) used by various SDKs, when working with the SwiftDashSDK, you may encounter this error as a response when attempting state transitions (e.g., creating documents) that violate uniqueness constraints [1][2]. - Debugging: If you receive this error, it generally means your application logic is attempting to submit a document that has already been committed to the chain [3]. You should verify the identity and contract state, ensure the transaction/nonce logic is correct, and check if the document has already been created by a previous, successful transaction [4][3]. For developers using the SwiftDashSDK, interacting with the platform involves the Rust FFI layer [5][6][7]. Errors surfacing from the platform are typically caught and handled by the SDK's internal mechanisms [8][9]. If you are seeing this error unexpectedly, ensure your application correctly tracks document states and nonce values to avoid duplicate submission attempts [4][3].
Citations:
- 1: https://github.com/dashpay/platform/blob/master/packages/rs-dpp/src/errors/consensus/codes.rs
- 2: https://github.com/dashpay/platform/blob/v3.1-dev/packages/rs-dpp/src/errors/consensus/codes.rs
- 3: Document purchase is getting stuck in platform mempool platform#2412
- 4: JS Dash SDK cannot send documents transitions (number type overflow) platform#2436
- 5: feat(swift-sdk,rs-sdk-ffi): wire devnet SDK config + auto-discover masternodes platform#3755
- 6: feat(rs-sdk-ffi): expose optional platform_version in DashSDKConfig platform#3751
- 7: dashpay/platform@41d57d6
- 8: fix(swift-sdk): attribute shielded registration errors to the right step and keep unconfirmed broadcasts safe platform#3862
- 9: feat(swift-example-app): wallet-signed Transfer & Withdraw for platform addresses (ADDR-02/04) platform#3923
🌐 Web query:
site:github.com/dashpay "DuplicateUniqueIndexError" SwiftDashSDK Swift
💡 Result:
DuplicateUniqueIndexError is a consensus error (specifically code 40105) within the Dash Platform architecture [1][2]. It indicates that an operation—typically a document insertion—has attempted to violate a uniqueness constraint defined in a Data Contract [1][3]. In the context of the Dash Platform, when a document is being processed, the system validates that it does not conflict with existing documents based on indexes defined as "unique" [3]. If the fields of the document being inserted match the indexed fields of an already existing document in a way that violates the unique index requirement, the system returns a DuplicateUniqueIndexError [1][3]. While the error appears as a consensus code within the Rust-based core (rs-dpp) [1], it propagates to SDKs—including the SwiftDashSDK—as part of the error handling mechanism for document-related operations. If you encounter this error while using the SwiftDashSDK, it generally means your application logic is attempting to submit a document that duplicates an entry already present in the platform's state for a given unique index [1][3]. To resolve this: 1. Verify the Data Contract definitions for the document type being used to identify which fields are marked as unique [4][3]. 2. Ensure that the document data being sent does not conflict with existing records for those specific fields. 3. If the goal is to update an existing document rather than insert a new one, ensure you are using the correct update operation/revision, as attempting to insert a new document with an ID or index that already exists will trigger this error [1].
Citations:
- 1: https://github.com/dashpay/platform/blob/master/packages/rs-dpp/src/errors/consensus/codes.rs
- 2: https://github.com/dashpay/platform/blob/v3.1-dev/packages/rs-dpp/src/errors/consensus/codes.rs
- 3: https://github.com/dashpay/platform/blob/v4.0-dev/packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs
- 4: https://github.com/dashpay/platform/blob/v0.24.5/packages/rs-dpp/src/data_contract/validation/data_contract_validator.rs
Use typed error handling for duplicate documents.
Do not infer the error type from String(describing: error). This can miss changed or localized messages and can route unrelated DuplicateUniqueIndexError failures to replaceDocument. The "duplicate unique" check is redundant because "duplicate" already matches it.
Use a typed SDK error when available. Otherwise, determine whether the login-key document exists before selecting createDocument or replaceDocument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`
around lines 380 - 408, Replace the String(describing: error) matching in the
login-key document create flow with typed SDK duplicate-document error handling.
Only invoke findExistingLoginKeyResponseDocumentId and replaceDocument for the
specific duplicate error; otherwise rethrow the original error, removing the
redundant "duplicate unique" check. If no typed SDK error is available, check
whether the login-key document exists before choosing createDocument or
replaceDocument.
| func completeKeyRegistration(_ request: DashStRequest) async throws { | ||
| try validateNetwork(request.network) | ||
| let context = try await requireContext() | ||
| let pendingConnection = try pendingApprovedConnectionForKeyRegistration() | ||
| guard let pendingAppContractId = Self.decodeIdentifier(pendingConnection.id) else { | ||
| throw DashConnectPlatformError.noApprovedConnectionAwaitingKeyRegistration | ||
| } | ||
| // Chosen approach: (a) deserialize the scanned IdentityUpdateTransition, | ||
| // verify it only adds the exact derived login keys for our identity, | ||
| // then rebuild the equivalent `updateIdentity(...)` call through the SDK. | ||
| let transition = try await MainActor.run { | ||
| try keyRegistrationParser.parse(request.transitionBytes) | ||
| } | ||
|
|
||
| // `deriveIdentityAuthKeyAtSlot` is main-actor isolated in the SDK. | ||
| var chainKey = try await MainActor.run { | ||
| try context.wallet.deriveIdentityAuthKeyAtSlot( | ||
| identityIndex: context.identityIndex, | ||
| keyId: UInt32(LoginKeyDerivation.defaultKeyIndex), | ||
| network: context.network | ||
| ).privateKeyData | ||
| } | ||
| defer { Self.zero(&chainKey) } | ||
|
|
||
| var derivedMaterial = try Self.deriveKeyRegistrationMaterial( | ||
| chainKeyPrivateBytes: chainKey, | ||
| identityId: context.identityId, | ||
| appContractId: pendingAppContractId | ||
| ) | ||
| defer { | ||
| Self.zero(&derivedMaterial.loginKey) | ||
| Self.zero(&derivedMaterial.authenticationPrivateKey) | ||
| Self.zero(&derivedMaterial.encryptionPrivateKey) | ||
| } | ||
|
|
||
| let validated = try Self.validateKeyRegistration( | ||
| transition, | ||
| identityId: context.identityId, | ||
| appContractId: pendingAppContractId, | ||
| derivedMaterial: derivedMaterial | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that the parsed transition carries the app contract id in contractBounds,
# so the pending connection can be resolved from the QR instead of from recency.
rg -nP --type=swift -C6 'contractBounds' DashWallet/Sources/Models/DashConnect/ DashWalletTests/DashConnect/
# Inspect how the tests construct a valid transition and which contract id they bind.
rg -nP --type=swift -C15 'func makeValidTransition' DashWalletTests/DashConnect/Repository: dashpay/dashwallet-ios
Length of output: 19339
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- registration flow ---'
sed -n '450,555p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- connection helper and related state ---'
sed -n '800,850p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- transition model and parser ---'
sed -n '130,225p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- validation symbols and contract-id handling ---'
rg -n -C12 'validateKeyRegistration|appContractId|pendingApprovedConnectionForKeyRegistration' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift
printf '%s\n' '--- valid transition construction ---'
sed -n '804,850p' DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swiftRepository: dashpay/dashwallet-ios
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- validator ---'
sed -n '683,734p' DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift
printf '%s\n' '--- valid transition helper ---'
sed -n '804,855p' DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift
printf '%s\n' '--- transition fixtures and registration calls ---'
rg -n -C8 'makeValidTransition|completeKeyRegistration|transitionBytes|DashStRequest|contractBounds:' DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift
printf '%s\n' '--- request model and URI parser ---'
rg -n -C10 'struct DashStRequest|DashStRequest|parseStRequest' DashWallet/Sources DashWalletTestsRepository: dashpay/dashwallet-ios
Length of output: 41091
Correlate each dash-st: transition with its approved connection.
completeKeyRegistration selects the newest .approved connection before parsing the request. DashStRequest has no app identifier, and valid transitions can have contractBounds == nil. If apps A and B are approved, scanning A after B can derive B's keys and reject A with keyRegistrationMismatchedDerivedKey.
Use an explicit registration correlation instead of recency. Add a two-approved-connections test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`
around lines 455 - 495, Update completeKeyRegistration and its
pendingApprovedConnectionForKeyRegistration flow to correlate the dash-st
transition with the correct approved connection instead of selecting the newest
approval. Preserve support for transitions whose contractBounds is nil, and
ensure key derivation uses the matched app contract so scanning app A after app
B succeeds. Add coverage for two approved connections scanned in non-recency
order.
| private var activeSwitch: some View { | ||
| ZStack { | ||
| SwitchView(isOn: .constant(true)) | ||
| .allowsHitTesting(false) | ||
| .scaleEffect(Layout.switchScale) | ||
| } | ||
| .frame(width: Layout.switchWidth, height: Layout.switchHeight) | ||
| .contentShape(Rectangle()) | ||
| .onTapGesture(perform: onDisconnect) | ||
| .accessibilityElement() | ||
| .accessibilityAddTraits(.isButton) | ||
| .accessibilityLabel(Text(NSLocalizedString("Disconnect", comment: "DashConnect"))) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Increase the disconnect touch target.
Line 95 creates a 48×21-point target. The tap gesture exists only in this target. Use a minimum 44-point height for the disconnect control.
Proposed fix
- .frame(width: Layout.switchWidth, height: Layout.switchHeight)
+ .frame(width: max(Layout.switchWidth, 44), height: 44)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var activeSwitch: some View { | |
| ZStack { | |
| SwitchView(isOn: .constant(true)) | |
| .allowsHitTesting(false) | |
| .scaleEffect(Layout.switchScale) | |
| } | |
| .frame(width: Layout.switchWidth, height: Layout.switchHeight) | |
| .contentShape(Rectangle()) | |
| .onTapGesture(perform: onDisconnect) | |
| .accessibilityElement() | |
| .accessibilityAddTraits(.isButton) | |
| .accessibilityLabel(Text(NSLocalizedString("Disconnect", comment: "DashConnect"))) | |
| } | |
| private var activeSwitch: some View { | |
| ZStack { | |
| SwitchView(isOn: .constant(true)) | |
| .allowsHitTesting(false) | |
| .scaleEffect(Layout.switchScale) | |
| } | |
| .frame(width: max(Layout.switchWidth, 44), height: 44) | |
| .contentShape(Rectangle()) | |
| .onTapGesture(perform: onDisconnect) | |
| .accessibilityElement() | |
| .accessibilityAddTraits(.isButton) | |
| .accessibilityLabel(Text(NSLocalizedString("Disconnect", comment: "DashConnect"))) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWallet/Sources/UI/DashConnect/Components/ConnectionRow.swift` around
lines 89 - 101, Update the activeSwitch control’s frame to provide at least a
44-point height while preserving its existing disconnect gesture, accessibility
configuration, and switch appearance.
| private func showConnections() { | ||
| let screen = ConnectionsScreen(vc: vc) | ||
| let controller = UIHostingController(rootView: screen) | ||
| controller.hidesBottomBarWhenPushed = true | ||
| vc.pushViewController(controller, animated: true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a thin hosting controller for ConnectionsScreen.
showConnections() pushes a bare UIHostingController. This bypasses the navigation display protocols used by BaseNavigationController. Wrap ConnectionsScreen in a thin hosting-controller subclass before pushing it. Otherwise, the navigation back-button behavior can be incorrect.
Based on learnings: wrap SwiftUI views in a thin UIViewController subclass before pushing into BaseNavigationController; bare UIHostingController instances bypass its navigation-bar behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift` around lines 302 -
306, Update showConnections() to instantiate and push the thin
hosting-controller subclass used for SwiftUI screens instead of a bare
UIHostingController, while preserving the ConnectionsScreen root view and
hidesBottomBarWhenPushed setting.
Source: Learnings
|
|
||
| import XCTest | ||
| @testable import dashwallet | ||
| @testable import dashpay |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the SwiftLint import order.
The changed module imports produce sorted_imports warnings.
DashWalletTests/AmountObjectTests.swift#L19-L19: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/DWAvatarUploadClientTests.swift#L10-L10: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/DWContestedNameStatusServiceTests.swift#L11-L11: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/DWRegistrationPhaseAdapterTests.swift#L13-L13: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/DashConnect/LoginKeyDerivationTests.swift#L2-L2: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/DiagnosticLogExporterTests.swift#L33-L33: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift#L9-L9: Move@testable import dashpayto the SwiftLint-required position.DashWalletTests/WalletWipeSerialExecutorTests.swift#L11-L11: Move@testable import dashpayto the SwiftLint-required position.
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 19-19: Imports should be sorted
(sorted_imports)
📍 Affects 8 files
DashWalletTests/AmountObjectTests.swift#L19-L19(this comment)DashWalletTests/DWAvatarUploadClientTests.swift#L10-L10DashWalletTests/DWContestedNameStatusServiceTests.swift#L11-L11DashWalletTests/DWRegistrationPhaseAdapterTests.swift#L13-L13DashWalletTests/DashConnect/LoginKeyDerivationTests.swift#L2-L2DashWalletTests/DiagnosticLogExporterTests.swift#L33-L33DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift#L9-L9DashWalletTests/WalletWipeSerialExecutorTests.swift#L11-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWalletTests/AmountObjectTests.swift` at line 19, Restore the
SwiftLint-required import ordering for `@testable` import dashpay in
DashWalletTests/AmountObjectTests.swift (19-19),
DashWalletTests/DWAvatarUploadClientTests.swift (10-10),
DashWalletTests/DWContestedNameStatusServiceTests.swift (11-11),
DashWalletTests/DWRegistrationPhaseAdapterTests.swift (13-13),
DashWalletTests/DashConnect/LoginKeyDerivationTests.swift (2-2),
DashWalletTests/DiagnosticLogExporterTests.swift (33-33),
DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift (9-9), and
DashWalletTests/WalletWipeSerialExecutorTests.swift (11-11); move each import to
the position required by the sorted_imports rule without changing other imports.
Source: Linters/SAST tools
| func testApprovingSameAppTwiceReplacesExistingRow() async throws { | ||
| let initialDate = Date(timeIntervalSince1970: 1_773_132_300) | ||
| let existing = DAppConnection( | ||
| id: MockDashConnectDataSource.sample(.approved).id, | ||
| name: "Yappr", | ||
| url: "yap.pr", | ||
| status: .approved, | ||
| updatedAt: initialDate | ||
| ) | ||
| let dataSource = MockDashConnectDataSource(initial: [existing]) | ||
|
|
||
| _ = try await dataSource.approveLogin(MockDashConnectDataSource.sampleLoginRequest) | ||
|
|
||
| XCTAssertEqual(dataSource.connectionsSnapshot.count, 1) | ||
| XCTAssertEqual(dataSource.connectionsSnapshot.first?.id, existing.id) | ||
| XCTAssertTrue((dataSource.connectionsSnapshot.first?.updatedAt ?? .distantPast) > initialDate) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The assertion depends on the system clock.
Line 124 asserts that the new updatedAt is later than a hard-coded timestamp from March 2026. The test fails on any machine whose clock is set before that date. Capture the time immediately before the call instead.
💚 Proposed fix
let dataSource = MockDashConnectDataSource(initial: [existing])
+ let beforeApproval = Date()
_ = try await dataSource.approveLogin(MockDashConnectDataSource.sampleLoginRequest)
XCTAssertEqual(dataSource.connectionsSnapshot.count, 1)
XCTAssertEqual(dataSource.connectionsSnapshot.first?.id, existing.id)
- XCTAssertTrue((dataSource.connectionsSnapshot.first?.updatedAt ?? .distantPast) > initialDate)
+ XCTAssertTrue((dataSource.connectionsSnapshot.first?.updatedAt ?? .distantPast) >= beforeApproval)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testApprovingSameAppTwiceReplacesExistingRow() async throws { | |
| let initialDate = Date(timeIntervalSince1970: 1_773_132_300) | |
| let existing = DAppConnection( | |
| id: MockDashConnectDataSource.sample(.approved).id, | |
| name: "Yappr", | |
| url: "yap.pr", | |
| status: .approved, | |
| updatedAt: initialDate | |
| ) | |
| let dataSource = MockDashConnectDataSource(initial: [existing]) | |
| _ = try await dataSource.approveLogin(MockDashConnectDataSource.sampleLoginRequest) | |
| XCTAssertEqual(dataSource.connectionsSnapshot.count, 1) | |
| XCTAssertEqual(dataSource.connectionsSnapshot.first?.id, existing.id) | |
| XCTAssertTrue((dataSource.connectionsSnapshot.first?.updatedAt ?? .distantPast) > initialDate) | |
| } | |
| func testApprovingSameAppTwiceReplacesExistingRow() async throws { | |
| let initialDate = Date(timeIntervalSince1970: 1_773_132_300) | |
| let existing = DAppConnection( | |
| id: MockDashConnectDataSource.sample(.approved).id, | |
| name: "Yappr", | |
| url: "yap.pr", | |
| status: .approved, | |
| updatedAt: initialDate | |
| ) | |
| let dataSource = MockDashConnectDataSource(initial: [existing]) | |
| let beforeApproval = Date() | |
| _ = try await dataSource.approveLogin(MockDashConnectDataSource.sampleLoginRequest) | |
| XCTAssertEqual(dataSource.connectionsSnapshot.count, 1) | |
| XCTAssertEqual(dataSource.connectionsSnapshot.first?.id, existing.id) | |
| XCTAssertTrue((dataSource.connectionsSnapshot.first?.updatedAt ?? .distantPast) >= beforeApproval) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWalletTests/DashConnect/DashConnectDataSourceTests.swift` around lines
109 - 125, Update testApprovingSameAppTwiceReplacesExistingRow to capture the
current Date immediately before calling approveLogin, then assert the
replacement connection’s updatedAt is later than that captured time instead of
comparing against the hard-coded initialDate.
| private func validKeyUri(network: DashConnectNetwork = .testnet) -> String { | ||
| let labelData = Data(label.utf8) | ||
| let payload = data([ | ||
| Data([0x01]), | ||
| try! validEphemeralPublicKey(), | ||
| contractId, | ||
| Data([UInt8(labelData.count)]), | ||
| labelData, | ||
| ]) | ||
| return "dash-key:\(base58Encode(payload))?n=\(network.rawValue)&v=1" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
SwiftLint reports force_try as an error in two DashConnect test fixtures. Both helpers are declared non-throwing while calling the throwing Secp256k1.compressedPublicKey, so each uses try!. Make the helper throwing and propagate the error to the callers.
DashWalletTests/DashConnect/DashConnectDataSourceTests.swift#L127-L137: markvalidKeyUriasthrows, changetry!totryon Line 131, and addtryat the call sites on Lines 12, 57 and 69.DashWalletTests/DashConnect/DashConnectUriTests.swift#L143-L152: markvalidKeyPayloadasthrows, changetry!totryon Line 147, and propagatethrowsthroughvalidKeyUriand its callers.
🧰 Tools
🪛 SwiftLint (0.65.0)
[Error] 131-131: Force tries should be avoided
(force_try)
📍 Affects 2 files
DashWalletTests/DashConnect/DashConnectDataSourceTests.swift#L127-L137(this comment)DashWalletTests/DashConnect/DashConnectUriTests.swift#L143-L152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DashWalletTests/DashConnect/DashConnectDataSourceTests.swift` around lines
127 - 137, Remove force_try from both DashConnect test fixtures by making
validKeyUri in DashConnectDataSourceTests.swift throw, replacing try! with try,
and propagating try to its callers at lines 12, 57, and 69. In
DashConnectUriTests.swift, make validKeyPayload throw, replace its try! with
try, and propagate throws through validKeyUri and all of its callers.
Source: Linters/SAST tools
2 participants
Issue being fixed or feature implemented
DashConnect lets a user sign in to a Dash Platform app with their DashPay identity instead of a password. The flow already ships on Android (
dashpay/dash-wallet@feat/dash-connect); iOS had only the screen, backed entirely by mock data —MockDashConnectDataSourcewas the sole conformance toDashConnectDataSource, andparseQRignored its input and returned a fixed sample.This wires that screen to Dash Platform. Testnet only — the key exchange contract is testnet, and the screen shows an unavailable state elsewhere.
What was done?
Protocol —
Sources/Models/DashConnect/Protocol/DashConnectUri— parses the two QR codes:dash-key:(login request) anddash-st:(first-login key registration carrying a serializedIdentityUpdateTransition). Envelope is<scheme>:<Base58, no checksum>?n=<m|t|d>&v=1, no//authority.KeyExchangeCrypto/LoginKeyDerivation— HKDF, AES-GCM, and the login/auth/encryption key derivation.Secp256k1— thin wrapper over the primitives added in feat(platform-wallet): secp256k1 primitives, identity-update parsing, and scoped signing keys for DashConnect platform#4273, so this adds no third-party crypto dependency.Data layer —
Sources/Models/DashConnect/PlatformDashConnectDataSource— publishes theloginKeyResponsedocument on approve (create, falling back to replace for a re-login), and completesdash-st:registration by validating the app-supplied transition against locally derived keys before rebuilding it throughupdateIdentity. Never broadcasts foreign bytes.DashConnectStore—UserDefaults-backed, scoped per (network, wallet), with defensive decoding. The connection list is local state by design: Platform only knows whether the document exists.MockDashConnectDataSourcestays behind the same protocol for previews and the mainnet-unavailable state.UI —
Sources/UI/DashConnect/approvedmeans the derived login keys are not on the identity yet;activemeans they are.dash-st:is first-login-only per (identity, app) — after it, the app stops emitting that QR — so a later login with only thedash-key:QR now lands straight onactiveinstead of sticking onapprovedand prompting for a QR that will never appear again.loginKeyResponsedocument belongs to the wallet's identity — so the status means "I granted this app access", never "a session is open". Real revocation (deleting the document, or disabling the derived identity keys) is deliberately out of scope; the keys are deterministic in (chain key, identity id, contract id), so burning them may be irreversible, and that consensus question is unsettled.Test target repair
The unit-test target was unrunnable on this branch. It now hosts on
dashpay(the app that actually builds here), the UI-test targets are out of the test scheme, and the test sources importdashpayaccordingly. This is why 15 unrelated test files appear in the diff with a one-line@testable importchange.How Has This Been Tested?
Clean build:
xcodebuild -workspace DashWallet.xcworkspace -scheme dashpay \ -destination 'generic/platform=iOS Simulator' -configuration Debug \ CODE_SIGNING_ALLOWED=NO ARCHS=arm64 buildNew unit tests under
DashWalletTests/DashConnect/:DashConnectUriTests— envelope and payload parsing, both schemes, rejection paths.Secp256k1Tests/ key-exchange tests — asserted against the Kotlin implementation's own vectors, so the two ports are proven interoperable rather than self-consistent.DashConnectStoreTests— persistence across instances, per-(network, wallet) scoping, and that an undecodable row is dropped rather than guessed at.PlatformDashConnectDataSourceTests— transition validation, key-registration matching, and signing-key selection.Manual, on testnet, against the live Yappr instance: scan
dash-key:→ approve →loginKeyResponsepublished; scan the app's realdash-st:→ keys added to the identity, row goesActive. Then signed out on the Yappr website and back in with thedash-key:QR alone — the row staysActive, which is the defect this PR's status rule fixes. Switch-off → confirmation → row removed → empty state.Breaking Changes
None. The feature is reachable only from the Tools menu on testnet, and nothing outside
DashConnectchanges behaviour — the other touched files are the test-target repair described above.Notes for the reviewer
Localizable.stringsare intentionally not in this PR. The BartyCrouch build phase rewrites all 40 locales with strings from unrelated in-flight features; including it added ~41k lines of noise. The new keys go throughtx push -sseparately. Missing entries render as the key itself, which is the English text.Package.resolved(from an earlier commit on this branch) points atfix/textfield-prompt-type@449c6cerather thanmaster. That is fix: keep TextField prompts as Text instead of dashFont DashUIKit#9; once it merges, the pin gets bumped back tomasterhere.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit