feat(dashpay): authorize a token purchase from a DashConnect link - #1109
feat(dashpay): authorize a token purchase from a DashConnect link#1109romchornyi wants to merge 3 commits into
Conversation
`WalletStartupStatus` gained `seedBindingUnverified` and `identityScanIncomplete`, so the DashPay readiness log stopped compiling. Both are logged as warnings rather than errors: unlike `discoveryFailed`, which is a local fault nothing will clear on its own, each of these is recoverable on a later start — a rerun with the matching signer completes the queued contact-account work, and an incomplete identity scan stays on record so the next launch re-scans instead of taking the warm shortcut.
A `dash-st:` link was parsed strictly as an `IdentityUpdateTransition` — the DashConnect key-registration step — so a dApp asking the wallet to authorize a token purchase failed with "Expected IdentityUpdateTransition, got Batch(...)". Yappr's own fallback was to ask the user to paste a CRITICAL private key into a web page, which is exactly what DashConnect exists to avoid. The `dash-st` path now parses once through the SDK's new `parseStateTransition` and switches on the kind rather than on a thrown error. Identity updates keep today's key-registration behaviour unchanged, including its wrong-identity, unexpected-mutation and mismatched-key validation. A token purchase gets an approval sheet showing what is being authorized — token count, the total price rendered in DASH, and which identity of the user's will be charged — and only then signs. The wallet never signs the bytes it was handed. It reads the intent from them, shows it, and rebuilds the purchase through `tokenPurchase`, passing the parsed price as `expectedTotalCost` so the amount shown and the amount charged cannot diverge. Ownership is checked before the sheet is built and again immediately before signing, since the sheet can sit open while the active identity changes. Signing uses a CRITICAL key, which the signer selects itself; an identity without one fails with a visible message rather than a silent no-op. Two helpers shared with the connection sheet were promoted out of it rather than copied.
`approveLogin` logged that it started and then went silent until it finished, so a stall anywhere across key derivation, the Platform document write and the connection preview looked identical from a log — the whole approval was one opaque span. Adds four markers around the steps that can block, which is what let a report of "the QR never disappears" be narrowed to the wallet having completed its side and the dApp not picking the document up.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Phase 2 deferred (commit 43bfabd) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
The purchase flow correctly parses intent, verifies identity ownership, rebuilds the operation through the SDK, and requires explicit authorization rather than signing untrusted transition bytes. One blocking issue remains around retrying an ambiguously submitted purchase, along with narrower display-integrity, precision, documentation, duplication, and test-coverage findings.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: ffi-engineer); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-5.6-sol (agent: sol-verifier, role: verifier)
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
Review provenance
- Phase 1 reviewers:
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— ffi-engineer (completed); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed); agentphase1-reviewer - Fresh verifier:
gpt-5.6-sol— verifier; agentsol-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 3 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift`:
- [BLOCKING] DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift:177-185: An ambiguous post-submit failure can turn Retry into a second purchase
Each call to `approveTokenPurchase` builds, signs, and submits a new direct purchase. The SDK can fail while awaiting confirmation after the transition has already reached Platform, but this catch path describes every error as an incomplete request and leaves the same sheet ready to approve again. If the first transition was accepted despite a finality timeout or similar DAPI failure, the retry can use the identity's next nonce and buy the tokens a second time. Distinguish deterministic pre-submit failures from ambiguous post-submit outcomes; for an ambiguous result, dismiss or disable retry and tell the user to verify the identity's tokens and credit balance before initiating another purchase.
In `DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`:
- [SUGGESTION] DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift:635-658: Validate the displayed token ID against the purchase being rebuilt
The approval request copies `tokenId` directly from the untrusted serialized transition, but approval rebuilds the operation solely from `dataContractId` and `tokenContractPosition`; the SDK derives the actual token ID and never receives `request.tokenId`. A crafted payload can therefore display the identifier of one token while purchasing another token selected by the contract and position. Validate that the parsed ID equals the token ID derived from those fields before presenting the sheet, or remove the Token ID row so the UI does not authenticate a value that execution ignores.
- [NITPICK] DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift:672-675: Describe expectedTotalCost as a maximum rather than an exact-price guarantee
Platform rejects the purchase when the current required price exceeds `expectedTotalCost`, but a lower current price is accepted and the lower amount is charged. The comment therefore overstates the invariant by claiming that the charged amount cannot diverge from the displayed amount. The corresponding `DashConnectTokenPurchaseRequest.totalAgreedPriceCredits` comment should also describe this value as the maximum approved cost.
In `DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift`:
- [SUGGESTION] DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift:165-199: Add compile-ready coverage for token-purchase parsing and price conversion
The existing parser test exercises only the renamed identity-update branch. The newly introduced token-purchase mapping, including owner ID, contract ID, token ID, position, count, and agreed price, has no regression coverage despite being directly testable through the parser's injected closure. Add a token-purchase branch test and boundary cases for converting Platform credits to DASH, including zero, a normal whole-duff value, and a value with sub-duff credit precision. These tests remain valuable as compile-ready coverage while the repository's unit-test runner is broken.
In `DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift`:
- [NITPICK] DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift:70-73: Render the full Platform-credit precision on the approval sheet
Platform credits have 11 decimal places relative to DASH, but `formattedDashAmount` limits the displayed amount to 8 fractional digits. When `totalAgreedPriceCredits` is not divisible by 1,000, the sheet rounds away the sub-duff remainder while the exact credit amount is passed to `expectedTotalCost` and charged. The discrepancy is less than one duff, but this is a money-authorization surface and the displayed amount should preserve all 11 credit-backed decimal places.
In `DashWallet/Sources/Models/DashConnect/DashConnectModels.swift`:
- [NITPICK] DashWallet/Sources/Models/DashConnect/DashConnectModels.swift:63-70: Reuse the existing credits-per-DASH definition
This introduces another literal definition of the Platform credits conversion even though `PlatformCreditsFormatter.creditsPerDash` is already internal and available to this module. The same divisor also has private copies in two payment views. Reusing the existing definition here prevents the money-display conversion from drifting independently.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- DashUIKit's pinned Swift tools version is incompatible with documented Xcode 16 support — The workspace pins DashUIKit revision
83cf65a84834f6a7a1f0a82446ea1258ea86f37d, whose Swift tools requirement cannot be resolved by Xcode 16.4's Swift 6.1 toolchain. That pin predates this PR, so it is not caused by the token-purchase work, but it conflicts with the repository's documented Xcode 16+ requirement and blocks canonical builds on that supported setup.- Follow-up: Track this as a separate build-infrastructure issue: either use a DashUIKit revision compatible with Swift 6.1 or update the documented minimum Xcode/toolchain requirement.
- Synchronous FFI parse of untrusted dash-st bytes runs on the main thread with no payload-size cap — Out of scope: the previous DashConnect state-transition path already synchronously decoded and parsed the same unbounded payload on the main actor. This PR changes the accepted transition kinds but does not introduce that execution pattern.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Platform #4584 SDK-side boundary observations — Out of scope: the defer ordering and Rust panic policy are implementation details of the separately reviewed platform#4584 dependency, not code changed in this repository.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| } catch { | ||
| // Keep the sheet up so the user can retry without rescanning | ||
| // the QR. Failures — including "the identity has no CRITICAL | ||
| // key" from the SDK — surface here as the sheet's error text. | ||
| self.purchaseApproveError = String( | ||
| format: NSLocalizedString("Could not complete the DashConnect request: %@", comment: "DashConnect"), | ||
| error.localizedDescription | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: An ambiguous post-submit failure can turn Retry into a second purchase
Each call to approveTokenPurchase builds, signs, and submits a new direct purchase. The SDK can fail while awaiting confirmation after the transition has already reached Platform, but this catch path describes every error as an incomplete request and leaves the same sheet ready to approve again. If the first transition was accepted despite a finality timeout or similar DAPI failure, the retry can use the identity's next nonce and buy the tokens a second time. Distinguish deterministic pre-submit failures from ambiguous post-submit outcomes; for an ambiguous result, dismiss or disable retry and tell the user to verify the identity's tokens and credit balance before initiating another purchase.
source: ['codex']
| private func makeTokenPurchaseRequest( | ||
| _ purchase: DashConnectTokenPurchaseTransition, | ||
| context: Context | ||
| ) throws -> DashConnectTokenPurchaseRequest { | ||
| // Checked before anything is shown: a purchase that would charge a | ||
| // different identity must be refused, not rendered for approval. | ||
| guard purchase.ownerId == context.identityId else { | ||
| throw DashConnectPlatformError.tokenPurchaseWrongIdentity | ||
| } | ||
|
|
||
| let contractIdBase58 = purchase.dataContractId.toBase58String() | ||
| return DashConnectTokenPurchaseRequest( | ||
| // A connection approved earlier for the same contract names the | ||
| // app; otherwise the sheet falls back to the contract id. | ||
| appName: subject.value.first { $0.id == contractIdBase58 }?.name, | ||
| ownerId: purchase.ownerId, | ||
| dataContractId: purchase.dataContractId, | ||
| tokenId: purchase.tokenId, | ||
| tokenContractPosition: purchase.tokenContractPosition, | ||
| tokenCount: purchase.tokenCount, | ||
| totalAgreedPriceCredits: purchase.totalAgreedPrice, | ||
| walletUsername: context.storedUsername, | ||
| walletIdentityId: context.identityId.toBase58String() | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Validate the displayed token ID against the purchase being rebuilt
The approval request copies tokenId directly from the untrusted serialized transition, but approval rebuilds the operation solely from dataContractId and tokenContractPosition; the SDK derives the actual token ID and never receives request.tokenId. A crafted payload can therefore display the identifier of one token while purchasing another token selected by the contract and position. Validate that the parsed ID equals the token ID derived from those fields before presenting the sheet, or remove the Token ID row so the UI does not authenticate a value that execution ignores.
source: ['codex']
| DashConnectDetailRow( | ||
| label: NSLocalizedString("Total price", comment: "DashConnect token purchase"), | ||
| value: request.totalPriceDash.formattedDashAmount | ||
| ) |
There was a problem hiding this comment.
💬 Nitpick: Render the full Platform-credit precision on the approval sheet
Platform credits have 11 decimal places relative to DASH, but formattedDashAmount limits the displayed amount to 8 fractional digits. When totalAgreedPriceCredits is not divisible by 1,000, the sheet rounds away the sub-duff remainder while the exact credit amount is passed to expectedTotalCost and charged. The discrepancy is less than one duff, but this is a money-authorization surface and the displayed amount should preserve all 11 credit-backed decimal places.
source: ['codex']
| let signer = KeychainSigner(modelContainer: context.modelContainer) | ||
| // `expectedTotalCost` is the same credits figure the approval sheet | ||
| // rendered — Platform rejects the transition if the on-chain price | ||
| // disagrees, so the charged amount cannot diverge from the shown one. |
There was a problem hiding this comment.
💬 Nitpick: Describe expectedTotalCost as a maximum rather than an exact-price guarantee
Platform rejects the purchase when the current required price exceeds expectedTotalCost, but a lower current price is accepted and the lower amount is charged. The comment therefore overstates the invariant by claiming that the charged amount cannot diverge from the displayed amount. The corresponding DashConnectTokenPurchaseRequest.totalAgreedPriceCredits comment should also describe this value as the maximum approved cost.
| let signer = KeychainSigner(modelContainer: context.modelContainer) | |
| // `expectedTotalCost` is the same credits figure the approval sheet | |
| // rendered — Platform rejects the transition if the on-chain price | |
| // disagrees, so the charged amount cannot diverge from the shown one. | |
| // `expectedTotalCost` is the same credits figure the approval sheet | |
| // rendered. Platform rejects the transition if the on-chain price | |
| // exceeds it, so the user cannot be charged more than the shown price; | |
| // if the on-chain price falls, the purchase charges less. |
source: ['codex']
| extension DashConnectTokenPurchaseRequest { | ||
| /// Platform credits per DASH (1e11 — 1e8 duffs x 1000 credits per duff). | ||
| static let creditsPerDash: Decimal = 100_000_000_000 | ||
|
|
||
| /// The total price converted to DASH for display. | ||
| var totalPriceDash: Decimal { | ||
| Decimal(totalAgreedPriceCredits) / Self.creditsPerDash | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Reuse the existing credits-per-DASH definition
This introduces another literal definition of the Platform credits conversion even though PlatformCreditsFormatter.creditsPerDash is already internal and available to this module. The same divisor also has private copies in two payment views. Reusing the existing definition here prevents the money-display conversion from drifting independently.
| extension DashConnectTokenPurchaseRequest { | |
| /// Platform credits per DASH (1e11 — 1e8 duffs x 1000 credits per duff). | |
| static let creditsPerDash: Decimal = 100_000_000_000 | |
| /// The total price converted to DASH for display. | |
| var totalPriceDash: Decimal { | |
| Decimal(totalAgreedPriceCredits) / Self.creditsPerDash | |
| } | |
| extension DashConnectTokenPurchaseRequest { | |
| /// The total price converted to DASH for display. | |
| var totalPriceDash: Decimal { | |
| Decimal(totalAgreedPriceCredits) / Decimal(PlatformCreditsFormatter.creditsPerDash) | |
| } | |
| } |
source: ['codex']
3 participants
Issue being fixed or feature implemented
A
dash-st:link was parsed strictly as anIdentityUpdateTransition— the DashConnect key-registration step — so a dApp asking the wallet to authorize a token purchase failed outright withExpected IdentityUpdateTransition, got Batch(...TokenDirectPurchase...).Yappr's fallback without a wallet path was to ask the user to paste their CRITICAL private key into a web page, which is precisely what DashConnect exists to prevent — the approval sheet's own promise is that the app never sees your keys.
Depends on dashpay/platform#4584, which adds the parser this uses. Merge that first; this branch does not build against a
v4.2-devwithout it.What was done?
dash-stpath parses once through the SDK's newparseStateTransitionand switches on the kind, rather than branching on a thrown "expected X, got Y" error. Identity updates keep today's key-registration behaviour unchanged, including its wrong-identity, unexpected-mutation and mismatched-derived-key validation; the protocol method becamehandleStateTransitionand the old body is now a private continuation, so no rule moved.ApproveTokenPurchaseSheet) showing what is being authorized: token count, the total price rendered in DASH, the token id, and which identity of the user's will be charged.tokenPurchase, passing the parsed price asexpectedTotalCostso the amount shown and the amount charged cannot diverge.approveLogingained step markers. It logged that it started and then went silent until it finished, so a stall in key derivation, the Platform write or the connection preview looked identical from a log.Also included, because the branch builds against a bumped platform: the
WalletStartupStatusswitch fix. It is submitted separately as #1108 againstdevelop, sincedevelopdoes not currently compile againstv4.2-devhead without it; the copy here is what lets this branch build and will fold away once #1108 lands.How Has This Been Tested?
xcodebuild -workspace DashWallet.xcworkspace -scheme dashpay -sdk iphonesimulator ARCHS=arm64— succeeds.End to end on a device against devnet-moutai: a real Yappr
dash-st:purchase link opens the wallet, the sheet renders 100 tokens for 0.001 DASH against the correct identity, and the purchase completes on chain. The price shown was cross-checked against Yappr's own screen independently.Getting there also surfaced, and required, a platform bump to
v4.2-devhead: an older pin could not read therankedCountableat-form introduced by dashpay/platform#4531, which Yappr's token contract uses, and failed withProof verification error: rankedCountable value must be a boolean.Not covered: the unit-test target is the known-broken one described in
CLAUDE.md, so tests compile but were not run. The identity-update (dash-stkey registration) path was verified by reading the final code rather than re-exercised, since its body is unchanged.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only