Skip to content

fix(wallet): say what actually happens after a send we could not confirm #1125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
romchornyi wants to merge 5 commits into
base: develop
Choose a base branch
from
+87 −7
Open

fix(wallet): say what actually happens after a send we could not confirm #1125

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e467a1d
fix(wallet): say what actually happens after a send we could not confirm
jeanpierreroma Sep 11, 2026
7d70df7
fix(wallet): route every send outcome through the same copy
jeanpierreroma Sep 11, 2026
3a1112c
fix(wallet): promise only the retry the shipped SDK actually performs
jeanpierreroma Sep 11, 2026
a08edba
docs(wallet): drop the last claim that retries survive closing the app
jeanpierreroma Sep 13, 2026
6270146
chore(wallet): add the broadcast-outcome strings to the source catalog
jeanpierreroma Sep 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 38 additions & 5 deletions DashWallet/Sources/Models/Transactions/WalletSendService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,38 @@ final class PreparedStandardSend: NSObject {
txidWire: Data(txHash.reversed()), address: address, amount: amount, fee: fee)

case .rejected(_, let reason):
// Nothing reached the network, so the money is provably still
// here — say that, because "wasn't sent" alone reads as a loss.
let error = WalletSendService.makeError(
code: .broadcastRejected,
description: "The transaction wasn't sent. You can try again. \(reason)"
description: NSLocalizedString(
"The transaction wasn't sent, so nothing left your wallet. You can try again.",
comment: "Send failed before any bytes reached the network"),
diagnostic: reason
)
claimLock.lock()
broadcastState = .ready
claimLock.unlock()
throw error

case .unknown(_, let reason):
// The old copy told the user to "wait for wallet synchronization"
// without saying that the wallet is the thing doing the waiting —
// and it appended the SDK's internal reason, so the dialog ended
// in "SPV broadcast saw no acceptance signal before dash-spv's
// acceptance timeout". Neither was actionable, and neither was
// localized, so a customer on a non-English device got a wall of
// English (support ticket 32189).
//
// The retry promise is now true rather than aspirational: the
// send is re-registered for rebroadcast at every launch, so it
// survives closing the app.

@thepastaclaw thepastaclaw Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Remove the remaining claim that retries survive app closure

This comment says the send is re-registered at every launch and survives closing the app. That contradicts the user-facing copy and the current SDK behavior described in the PR: dash-spv only continues retrying while the wallet remains open, and launch-time re-registration depends on platform#4659, which is not part of this head. Keeping the comment can mislead a future maintainer into removing the necessary open-app qualifier or relying on behavior that is not shipped.

Suggested change
// The retry promise is now true rather than aspirational: the
// send is re-registered for rebroadcast at every launch, so it
// survives closing the app.
// The SDK keeps retrying while the wallet remains open; the copy
// does not promise that retries survive closing the app.

source: gpt-6-astra (phase2-reviewer: general)

@romchornyi romchornyi Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a08edba8b. You are right that this was the dangerous half of the leftover: a comment contradicting the string next to it is how the qualifier gets deleted later by someone who trusts the comment, and the misleading promise lands back in front of users.

The comment now states what the shipped SDK does — retries continue while the wallet is open — and says explicitly that launch-time re-registration is dashpay/platform#4659 and not in this head, pointing at BroadcastOutcomeCopy.unknown for when the qualifier can be dropped.

🤖 Reviewed with Claude Code

@thepastaclaw thepastaclaw Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at a08edba8): The misleading comment has been replaced with wording that limits retry behavior to while the wallet remains open and explicitly identifies launch-time re-registration as a future platform change.

@thepastaclaw thepastaclaw Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at 62701467): The misleading launch-time retry claim and the suggestion that retries survive app closure have been removed; the surrounding comment now documents that the shipped SDK retries only while the wallet remains open.

let error = WalletSendService.makeError(
code: .broadcastUnknown,
description: "We couldn't confirm whether the transaction was accepted. Don't send it again; wait for wallet synchronization. \(reason)"
description: NSLocalizedString(
"We couldn't confirm the transaction reached the network. Don't send it again — the wallet keeps trying on its own, and your balance will update as soon as it goes through.",
comment: "Send dispatched but no network acceptance signal arrived"),
diagnostic: reason
)
claimLock.lock()
broadcastState = .unknown(error)
Expand Down Expand Up @@ -727,11 +746,25 @@ private extension WalletSendService {
comment: "DashPay Contacts"))
}

static func makeError(code: ErrorCode, description: String) -> NSError {
NSError(
/// `userInfo` key carrying the SDK's own explanation of a broadcast
/// outcome. Deliberately not `NSLocalizedDescriptionKey`: it is
/// engineer-facing text and must never reach a dialog, but support
/// still wants it on the error that gets logged.
static let diagnosticKey = "org.dashfoundation.dash.send.diagnostic"

@thepastaclaw thepastaclaw Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Expose diagnosticKey at module scope for cross-file diagnostics

diagnosticKey is declared inside private extension WalletSendService, so it is unavailable to tests and logging or error-inspection utilities in other files. Since the stated purpose is to retain the SDK diagnostic for support and logs, move the constant to the type's internal scope or a non-private extension. This does not affect the current error construction, which remains within the same file, but it prevents other module code from consistently inspecting the preserved diagnostic.

source: gemini-3.8-flash-high (phase1-reviewer: general, ffi-engineer, security-auditor)

@thepastaclaw thepastaclaw Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at 3a1112cb): You moved diagnosticKey onto WalletSendService with internal visibility, so logging and tests in other files can now inspect the SDK reason without relying on its literal key.

@thepastaclaw thepastaclaw Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at a08edba8): diagnosticKey is now an internal static member of WalletSendService, making it available to logging, error inspection, and tests in other files.

@thepastaclaw thepastaclaw Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at 62701467): diagnosticKey is now declared on WalletSendService at internal scope, allowing other files, logging, and tests to inspect the preserved SDK diagnostic.


static func makeError(
code: ErrorCode,
description: String,
diagnostic: String? = nil
) -> NSError {
var userInfo: [String: Any] = [NSLocalizedDescriptionKey: description]
if let diagnostic, !diagnostic.isEmpty {
userInfo[diagnosticKey] = diagnostic
}
return NSError(
domain: errorDomain,
code: code.rawValue,
userInfo: [NSLocalizedDescriptionKey: description]
userInfo: userInfo
)
}
}
Loading