feat(dashpay): QR contact exchange on the add-contact screen - #947
Conversation
Add two labeled entry points under the username search field: - "Scan QR": full-screen scanner for the new canonical dashpay://user?id=<base58-identity-id>&username=<label> payload (DashPayUserLink codec). A scanned code is never trusted as-is — the username is resolved on Platform via exact DPNS search and must belong to the scanned identity id before the existing send-request confirmation sheet opens; mismatches and foreign QRs get explicit errors. - "My QR": sheet rendering the current user's identity id + preferred username as a Dash-branded QR (QRCodeGenerator.dashStyledImage): round modules under a Dash-blue gradient, rounded finder eyes, and the Dash logo disc in the center, encoded at ECC level H so the logo overlay stays within the 30% recovery budget. Shown only once the identity owns a DPNS name — the QR encodes both. Parser tests are compile-ready in DashWalletTests (target currently broken repo-wide). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds DashPay user-link parsing, branded QR generation, and QR-based contact flows. Users can display or scan contact QR codes. Scanned links are verified against Platform data before opening contact preview. ChangesDashPay QR contacts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AddContactScreen
participant DashPayUserLink
participant SwiftDashSDKContactsService
participant Platform
participant ContactPreview
AddContactScreen->>DashPayUserLink: Parse scanned DashPay URI
DashPayUserLink-->>AddContactScreen: Return identity ID and username
AddContactScreen->>SwiftDashSDKContactsService: Resolve username
SwiftDashSDKContactsService->>Platform: Query exact DPNS username
Platform-->>SwiftDashSDKContactsService: Return identity ID
SwiftDashSDKContactsService-->>AddContactScreen: Return resolved identity
AddContactScreen->>ContactPreview: Open verified contact
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed the current head (b301831). The six-file PR diff is whitespace-clean, the project file parses, and the new production source is correctly included in the app target. I found two actionable issues in the scan flow and URI boundary.
…d scan verification - DashPayUserLink.parse now enforces the canonical wire shape: reject userinfo, port, path, fragment, duplicate parameters, and any query item other than id/username (tests added). - The scan-verification task is owned state: a new scan cancels the previous task, every UI-state write re-checks cancellation on the main actor, and dismissal cancels outright — a slow stale lookup can no longer clear the newer scan's spinner or overwrite its previewTarget/errorMessage. Co-Authored-By: Claude Fable 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: 1
🤖 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/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift`:
- Around line 396-421: The scan verification flow around the query assignment
and scanVerifyTask performs a duplicate capped prefix lookup that can overwrite
successful verification; suppress or cancel the search-field onChange lookup
while mirroring the scanned username, and replace searchUsernames with the SDK’s
exact DPNS lookup if available. Ensure the verified identity is resolved
directly without relying on a limited result page, while preserving the existing
eligibility and previewTarget flow.
🪄 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: 2f7d8ba2-fe37-4a52-b8c3-c3434dfed968
📒 Files selected for processing (6)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Infrastructure/SwiftDashSDK/Contacts/DashPayUserLink.swiftDashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swiftDashWallet/Sources/UI/Payments/Pay/QRCodeGenerator.swiftDashWallet/en.lproj/Localizable.stringsDashWalletTests/DashPayUserLinkTests.swift
Address CodeRabbit review on PR #947: the scan flow verified against a capped (100-row) prefix page — a valid QR could read as outdated when the identity fell outside the page — and mirroring the scanned name into the search field kicked off a second debounced lookup whose late failure could raise an error over a successful preview. - Verify with wallet.resolveDpnsName (exact name → owning identity, uncapped) via the new SwiftDashSDKContactsService.resolveUsername. - Leave the search field untouched on scan; no duplicate lookup runs. - New app-side ContactCandidate drives the confirmation sheet for both tapped search rows and verified scans (DpnsSearchResult has no public initializer, and a prefix page must not gate a verified scan). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift (1)
409-417: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCancel verification before parsing every scanner callback.
The previous verification task is canceled only after
DashPayUserLink.parse(value)succeeds. If a valid scan starts verification and the next scan is invalid, the prior task remains active. It can keep the spinner visible or later open the preview for the prior QR code.Cancel and clear the prior verification state before the
guard. SetisVerifyingScanonly after the new QR code parses successfully.Proposed fix
private func handleScannedCode(_ value: String) { showScanner = false + scanVerifyTask?.cancel() + scanVerifyTask = nil + isVerifyingScan = false + guard let link = DashPayUserLink.parse(value) else { errorMessage = NSLocalizedString("This isn't a DashPay user QR code.", comment: "DashPay Contacts: scanned QR is a payment/invitation/foreign code") return } - scanVerifyTask?.cancel() isVerifyingScan = true🤖 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/DashPay/Contacts/SwiftUI/AddContactScreen.swift` around lines 409 - 417, Update handleScannedCode to cancel and clear scanVerifyTask and reset the prior verification state before attempting DashPayUserLink.parse(value). Only set isVerifyingScan to true and start a new verification task after parsing succeeds, while preserving the existing invalid-code error behavior.
🧹 Nitpick comments (1)
DashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the explicit
nilinitializer.SwiftLint reports
implicit_optional_initializationon Line 61.Proposed fix
- `@State` private var previewTarget: ContactCandidate? = nil + `@State` private var previewTarget: ContactCandidate?🤖 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/DashPay/Contacts/SwiftUI/AddContactScreen.swift` at line 61, Remove the explicit nil initializer from the previewTarget state property declaration, relying on Swift’s implicit initialization for the optional while preserving its ContactCandidate? type and private `@State` behavior.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.
Outside diff comments:
In `@DashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift`:
- Around line 409-417: Update handleScannedCode to cancel and clear
scanVerifyTask and reset the prior verification state before attempting
DashPayUserLink.parse(value). Only set isVerifyingScan to true and start a new
verification task after parsing succeeds, while preserving the existing
invalid-code error behavior.
---
Nitpick comments:
In `@DashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift`:
- Line 61: Remove the explicit nil initializer from the previewTarget state
property declaration, relying on Swift’s implicit initialization for the
optional while preserving its ContactCandidate? type and private `@State`
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2041918-c6f0-49b4-8cef-f5f0aa76534a
📒 Files selected for processing (2)
DashWallet/Sources/Infrastructure/SwiftDashSDK/Contacts/SwiftDashSDKContactsService.swiftDashWallet/Sources/UI/DashPay/Contacts/SwiftUI/AddContactScreen.swift
An invalid second scan previously returned at the parse guard without canceling the first scan's in-flight verification, which could keep the spinner alive behind the error alert and open the prior QR's preview later. Cancel and clear the verification state at the top of the scanner callback; the spinner re-arms only after the new code parses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the outside-diff comment from review 4890757260 in the commit above: 🤖 Addressed by Claude Code |
#947 brought DashPayUserLink and its service support, but its UI lived in the add-contact screen this branch had already removed — so the parser shipped with nothing calling it. The scanner returns as a nav-bar button on the contacts list: their code belongs on their profile, the scanner belongs where contacts are added. A scanned code is verified before anyone is shown. DashPayUserLink.parse is offline and proves nothing, so the username claim is resolved exactly through DPNS and must return the scanned identity id; a new scan cancels the previous lookup so a slow stale one cannot deliver its result over the newer scan. A verified scan opens the same ContactSheet a search hit does, with the same actions. That needed identity-keyed collision/send/accept alongside the DpnsSearchResult ones — the app cannot construct a DpnsSearchResult, its memberwise initializer being internal to SwiftDashSDK — and the collision to relationship mapping is now shared, so a scan and a search hit cannot disagree about the same person. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scanner landed on the contacts list last commit, but the code it scans had nowhere to come from: MyDashPayUserQRSheet lived in the add-contact screen this branch replaced, so #947's QR exchange only worked in one direction. It moves to the profile sheet, unchanged — their code belongs with their identity, the scanner belongs where contacts get added. The entry appears only once a DPNS name exists, since the payload carries identity id and username together and there is nothing honest to show before that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Issue being fixed or feature implemented
The Add a New Contact screen only supported typing a username. In person, the natural flow is to show a code and scan one: this adds "My QR" (identity id + preferred username) and "Scan QR", with a confirmation asking whether to send a contact request to the scanned user.
What was done?
DashPayUserLink(new): codec for the canonical QR payloaddashpay://user?id=<base58-identity-id>&username=<label>. Parsing is pure/offline and strict — payment URIs, invitation links, bare usernames, bad base58, and non-32-byte ids are all rejected; a trailing.dashis tolerated and stripped. Android will need to adopt the same shape for cross-platform scanning.GenericQRScannerView. A scanned code is never trusted as-is — the username is resolved on Platform via exact DPNS search and the result must carry the scanned identity id. Only then does the existingAddContactPreviewSheetopen ("Send Contact Request" confirmation), which brings along all collision states for free (already a contact / pending / they-asked-us → Accept / self / missing DashPay keys). Mismatched or outdated codes and foreign QRs surface explicit errors; a "Verifying user…" overlay covers the lookup.QRCodeGenerator.dashStyledImage— round modules under a Dash-blue gradient, rounded finder eyes, and the Dash logo disc in the center. Encoded at ECC level H so the ~9% of modules the logo hides stay well inside the 30% recovery budget; modules render on a white card in both themes so camera binarization keeps its contrast.DashWalletTests/DashPayUserLinkTests.swift(unit-test target is currently broken repo-wide; tests are written ready to run once repaired).How Has This Been Tested?
dashpayscheme build (ARCHS=arm64, iOS simulator).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests