fix(dashpay): stop offering Pay for a contact we cannot pay - #984
Merged
Conversation
A mainnet user tapped Pay on a DashPay contact and got an alert reading "Invalid identity data: No DashpayExternalAccount found for contact <id> — call register_external_contact_account first". That is an internal SDK diagnostic naming an API only the SDK can call, so it reads as a demand the user cannot act on. Being `.established` was the only gate on the Pay CTA, but established means the contact requests are mutual — paying additionally needs the DIP-15 external account those requests are the input to. The two states are not the same, and the UI conflated them. - `ContactItem` now carries `paymentChannelBroken`, read from the `PersistentDashpayContactRequest` rows the persister already mirrors it onto. When the SDK has permanently given up on building the channel, the profile sheet shows an explanation in place of the Pay button. It says what actually fixes it — the flag only clears when the CONTACT sends a fresh request — so the user has something to do rather than a disabled control. - A failed send no longer surfaces the raw SDK string. The missing-account case becomes "the payment channel isn't ready yet, try again in a few minutes", which is true: the channel is built in the background from the counterparty's request. Only that one known diagnostic is translated; every other error keeps its own message rather than being hidden behind something generic. The broken flag alone would not have helped the user who reported this: their 27 legacy contacts were queued rather than broken, which dashpay/platform#4372 fixes. This is the other half — the app should never route a user into a spend it knows cannot complete, whatever the reason.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe contact model now reports permanently broken payment channels. The contact service populates this state from contact-request rows. The contact profile hides payment actions, shows recovery guidance, and translates known payment failures into retry messages. ChangesPayment channel handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftDashSDKContactsService
participant ContactItem
participant ContactProfileSheet
participant WalletSendService
SwiftDashSDKContactsService->>ContactItem: Set paymentChannelBroken from contact-request rows
ContactItem->>ContactProfileSheet: Provide contact state
ContactProfileSheet->>ContactProfileSheet: Show notice or Pay action
ContactProfileSheet->>WalletSendService: Send contact payment
WalletSendService->>WalletSendService: Map missing external-account diagnostic
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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/ContactProfileSheet.swift`:
- Around line 747-778: Move the SDK-specific diagnostic translation from
PayContactSheet.payFailureMessage(for:) into its `@MainActor` ObservableObject
ViewModel, and have the payment failure flow use that ViewModel mapping. Remove
the mapping helper and related SDK diagnostic knowledge from the View so
PayContactSheet only renders state and forwards user actions, while preserving
the existing localized message and fallback to error.localizedDescription.
🪄 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: c0f17512-4707-4cb4-a279-eb7722a6382b
📒 Files selected for processing (3)
DashWallet/Sources/Infrastructure/SwiftDashSDK/Contacts/ContactItem.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/Contacts/SwiftDashSDKContactsService.swiftDashWallet/Sources/UI/DashPay/Contacts/SwiftUI/ContactProfileSheet.swift
Comment on lines
+747
to
+778
| errorMessage = Self.payFailureMessage(for: error) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// User-facing text for a failed contact payment. | ||
| /// | ||
| /// The SDK surfaces a missing DIP-15 external account as its own | ||
| /// internal diagnostic — "Invalid identity data: No | ||
| /// DashpayExternalAccount found for contact <id> — call | ||
| /// register_external_contact_account first". That reached users | ||
| /// verbatim in an alert: unreadable, and it names an API only the | ||
| /// SDK can call, so it reads as "do something" when there is | ||
| /// nothing for the user to do. | ||
| /// | ||
| /// The channel is built in the background from the counterparty's | ||
| /// contact request, so the honest advice is to wait and retry. Any | ||
| /// other failure keeps its own message — this deliberately | ||
| /// translates one known string rather than swallowing every error | ||
| /// behind a generic one. | ||
| static func payFailureMessage(for error: Error) -> String { | ||
| let description = error.localizedDescription | ||
| guard description.contains("DashpayExternalAccount") | ||
| || description.contains("register_external_contact_account") | ||
| else { | ||
| return description | ||
| } | ||
| return NSLocalizedString( | ||
| "This contact's payment channel isn't ready yet. It's still being set up in the background — please try again in a few minutes.", | ||
| comment: "DashPay Contacts") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move SDK diagnostic mapping out of PayContactSheet.
payFailureMessage(for:) maps an SDK-specific diagnostic in a View struct. Move this logic into an @MainActor ObservableObject ViewModel. Keep PayContactSheet limited to rendering state and forwarding user actions.
As per coding guidelines, “Keep SwiftUI views lightweight” and keep “network mapping” and “protocol constants” out of View structs.
🤖 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/ContactProfileSheet.swift`
around lines 747 - 778, Move the SDK-specific diagnostic translation from
PayContactSheet.payFailureMessage(for:) into its `@MainActor` ObservableObject
ViewModel, and have the payment failure flow use that ViewModel mapping. Remove
the mapping helper and related SDK diagnostic knowledge from the View so
PayContactSheet only renders state and forwards user actions, while preserving
the existing localized message and fallback to error.localizedDescription.
Source: Coding guidelines
…dary Review flagged the translation helper living in a SwiftUI View, against the repo guideline that FFI/SDK knowledge belongs in a ViewModel or a service. Moved into `WalletSendService.sendToContact`, which owns the SDK call, rather than into a ViewModel: the diagnostic now stops at the boundary that produces it and never reaches the presentation layer at all, and every present and future caller of `sendToContact` gets the same treatment instead of each re-implementing it. The sheet is back to plain `error.localizedDescription`. Behaviour is unchanged — same localized message, same narrow match, everything else still surfaces its own error.
Contributor
Author
|
Addressed in 4db0aa5 (the review above ran against the earlier commit — I moved it into Same localized message, same deliberately narrow match — any other error still surfaces its own text rather than being hidden behind something generic. Simulator build passes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
A mainnet user tapped Pay on a DashPay contact and got this alert:
That is an internal SDK diagnostic, shown verbatim, naming an API only the SDK can call. It reads as an instruction to the user for something the user cannot do.
The underlying reason is that
.establishedwas the only gate on the Pay CTA. Established means the contact requests are mutual — paying additionally needs the DIP-15 external contact account those requests are the input to. The app treated "we're friends" and "I can pay them" as the same state; they are not.Root cause of why the account was missing is fixed separately in dashpay/platform#4372 (legacy Android/dashj contact requests were rejected by our key-purpose policy). This PR is the other half: the app should never route a user into a spend it already knows cannot complete, whatever the reason.
What was done?
Surface the channel state the SDK already persists
ContactItemgainspaymentChannelBroken, read from thePersistentDashpayContactRequestrows — the persister already mirrorsEstablishedContact.payment_channel_brokenonto both directions, so this needs no new SDK API or FFI.Don't offer a spend that cannot succeed
ContactProfileSheetrenders an explanation in place of the Pay button. It states what actually fixes it — "Ask them to send you a new contact request" — because the flag only clears when the contact sends a fresh request. A greyed-out button with no reason would leave the user tapping and guessing.Translate the one diagnostic that reaches users
How Has This Been Tested?
dashpayscheme, iOS Simulator, Debug — builds clean.Not yet verified on device against a real broken channel. The reporting user's 27 legacy contacts are queued, not broken, so their wallet does not exercise the broken-channel branch; it exercises the error-message branch. Reproducing the broken branch needs a contact whose request permanently fails validation.
What I would ask the reporter to confirm on a TestFlight build (together with dashpay/platform#4372):
register_external_contact_accountstring.Breaking Changes
None.
ContactItemis an internal read model; the new field defaults to what the persisted rows already carry. No contact that is payable today loses its Pay button.Checklist:
Summary by CodeRabbit