Skip to content

feat(dashpay): authorize a token purchase from a DashConnect link - #1109

Open
romchornyi wants to merge 7 commits into
feat/devnet-supportfrom
feat/dashconnect-token-purchase
Open

feat(dashpay): authorize a token purchase from a DashConnect link#1109
romchornyi wants to merge 7 commits into
feat/devnet-supportfrom
feat/dashconnect-token-purchase

Conversation

@romchornyi

Copy link
Copy Markdown
Contributor

Issue being fixed or feature implemented

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 outright with Expected 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-dev without it.

What was done?

  • The dash-st path parses once through the SDK's new parseStateTransition and 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 became handleStateTransition and the old body is now a private continuation, so no rule moved.
  • A token purchase gets its own approval sheet (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.
  • 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 SDK's signer selects itself (a HIGH login key can post but not spend). An identity without one fails with a visible message rather than a silent no-op.
  • approveLogin gained 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.
  • Two helpers shared with the connection sheet were promoted out of it rather than copied.

Also included, because the branch builds against a bumped platform: the WalletStartupStatus switch fix. It is submitted separately as #1108 against develop, since develop does not currently compile against v4.2-dev head 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-dev head: an older pin could not read the rankedCountable at-form introduced by dashpay/platform#4531, which Yappr's token contract uses, and failed with Proof 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-st key registration) path was verified by reading the final code rather than re-exercised, since its body is unchanged.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

`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.
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 35c05d16-94cb-48d4-bbd7-47065b04eb95

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 2, 2026

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit 57557d7) · triage: critical · Phase 2 only (queue backlog)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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); agent phase1-reviewer, glm-5.3-flash — ffi-engineer (completed); agent phase1-reviewer, glm-5.3-flash — security-auditor (completed); agent phase1-reviewer
  • Fresh verifier: gpt-5.6-sol — verifier; agent sol-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.

Comment thread DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift
Comment thread DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift
Comment thread DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift Outdated
Comment thread DashWallet/Sources/Models/DashConnect/DashConnectModels.swift
jeanpierreroma and others added 2 commits September 8, 2026 23:15
One conflict, in `onQRScanned`: the base gained a guard that refuses a
second request while one is on screen, plus the generation checks that
keep a superseded request from publishing; this branch generalized the
`dash-st` path from key registration to any state transition. Kept both —
the guard now also covers `pendingTokenPurchase`, which presents a sheet
of its own, and the flag the base still called
`isProcessingKeyRegistration` follows this branch's
`isProcessingStateTransition`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RstH83XqqRc6oudvpu7UxZ
…ified token id

Six findings from the automated review of #1109, plus one merge repair.

**A failed purchase is no longer offered for a blind retry.** Every
approval builds, signs and submits a NEW direct purchase against the
identity's next nonce, and the SDK reports one opaque error for both "the
transition was rejected" and "it was submitted and waiting on the outcome
failed". The old catch called all of it an incomplete request and left the
sheet ready to approve again, so a purchase Platform had already accepted
could be bought a second time. `approveTokenPurchase` now throws
`DashConnectTokenPurchaseFailure`, which says which side of submission the
failure came from: everything up to and including authentication is
`beforeSubmission` and keeps today's retry-in-place, while anything out of
`tokenPurchase(...)` is `outcomeUnknown` — the sheet closes and the user
is told to check this identity's tokens and credit balance before buying
again.

**The token id on the sheet is now the token that gets bought.** The id
was copied from the untrusted payload while the purchase is rebuilt from
the contract id and position alone, so a crafted payload could display one
token and buy another. The request is now refused unless the payload's id
matches `calculateTokenId(contractId:position:)` — the protocol formula
the SDK itself derives from.

Also:

- `expectedTotalCost` is a ceiling, not an exact price: Platform rejects a
  higher current price and charges a lower one. Both comments claiming the
  shown and charged amounts "cannot diverge" said more than that.
- The sheet renders the price at full credit precision. Credits are 1e11
  per DASH, and the eight-decimal rendering rounded away a sub-duff
  remainder that is nevertheless charged — on a money-authorization
  surface.
- The credits-per-DASH divisor comes from `PlatformCreditsFormatter`
  instead of a second literal definition.
- Tests cover the token-purchase parse branch and the credits→DASH
  conversion, including zero, a whole DASH, a whole-duff value and a
  single credit (the sub-duff case the display used to drop).

The merge repair: this branch carried its own copy of the
`WalletStartupStatus` cases while the same fix landed on develop as #1108,
so the merge left the switch with two of each. Kept develop's.

Built against platform `v4.2-dev` (which now carries dashpay/platform#4584);
succeeds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RstH83XqqRc6oudvpu7UxZ

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Final validation — Phase 2 only (queue backlog)

Five prior findings are fixed; the remaining precision finding is reproducible at the exact head. The approval sheet still labels raw token amounts as whole tokens without accounting for denomination, and second-request feedback is routed to the wrong sheet. Focused Swift syntax checks with and without DASHPAY and diff checking passed; full workspace, XCTest, and end-to-end validation were not rerun.

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

  • Triage: critical by gpt-6-astra (effort low) — This change authorizes real token purchases from externally supplied links using CRITICAL-key signatures, so errors in intent parsing, identity ownership, price binding, or submission handling could cause unauthorized spending or loss of funds.
  • Phase 1 reviewers: not run (skipped for throughput: 23 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🔴 1 blocking | 💬 1 nitpick(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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/ApproveTokenPurchaseSheet.swift`:
- [BLOCKING] DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift:149-151: Display token quantities in their declared denomination
  This formats the raw purchase amount as an integer under the label "Tokens". The parser copies tokenCount unchanged into the request, and approveTokenPurchase passes that same value to tokenPurchase(amount:); no contract denomination is resolved or applied anywhere along this path. For an eight-decimal token, 100,000,000 base units therefore appear as 100,000,000 tokens even though they represent one token. The flow neither restricts contracts to zero-decimal tokens nor identifies the displayed quantity as base units, so the approval can materially overstate what the user is buying. Preserve the raw amount for submission, but derive the displayed quantity from verified contract conventions in the service/model, or explicitly label it as base units when denomination metadata is unavailable.
- [NITPICK] DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift:70-73: Render the full Platform-credit precision on the approval sheet
  (existing thread: https://github.com/dashpay/dashwallet-ios/pull/1109#discussion_r3954766347)
  The eleven-digit formatter fixes small-price rounding, but totalPriceDashText delegates to PlatformCreditsFormatter.dashString, which converts UInt64 credits through Double before formatting. Executing the exact-head formatter with 10,000,000,000,000,001 credits produces "100000 DASH" instead of "100000.00000000001 DASH". The submitted ceiling remains the original integer, so the approval text still loses precision for large amounts. Preserve Decimal or integer arithmetic through final rendering and add totalPriceDashText assertions above Double's exact-integer range; the new conversion tests currently exercise only totalPriceDash.

In `DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift`:
- [NITPICK] DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift:116-120: Route second-request feedback to the active purchase sheet
  The expanded guard refuses a second request while either approval sheet is open, but always assigns approveError. ConnectionsScreen.tokenPurchaseSheet(for:) passes only purchaseApproveError to the purchase sheet, so a second request during token approval is silently refused from the user's perspective. Assign purchaseApproveError when pendingTokenPurchase is set and approveError otherwise, so the explanation appears on the sheet that owns the screen.

Comment thread DashWallet/Sources/UI/DashConnect/ApproveTokenPurchaseSheet.swift Outdated
Comment thread DashWallet/Sources/UI/DashConnect/ConnectionsViewModel.swift
**The approval sheet stated a quantity it had not resolved.** `tokenCount`
arrives as base units and is spent as base units by `tokenPurchase(amount:)`;
nothing on the path read the contract's decimals, so the sheet rendered the raw
integer under "Tokens". For an eight-decimal token, one token appeared as
100,000,000 — on the screen where the user authorizes the spend.

The denomination is now resolved when the request is parsed, from the wallet's
own `PersistentToken` row for that contract and position, and travels with the
request. When the wallet does not hold the contract it says so instead of
assuming: the number is still shown, labelled "Tokens (base units)". Assuming
zero decimals would be the same overstatement, just silent.

**A second request could be refused invisibly.** The guard covers both approval
sheets but always wrote `approveError`, and the purchase sheet reads only
`purchaseApproveError` — so a request arriving during a token approval was
refused with nothing on screen. It now lands on whichever sheet is up.

Four cases pin the arithmetic: scaling by declared decimals, a zero-decimal
whole count, a fractional quantity keeping its precision, and an unknown
denomination reported as base units rather than scaled by a guess.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Final validation — Phase 2 only (queue backlog)

The token-purchase flow correctly parses and rebuilds purchases, validates the canonical token ID, preserves raw token amounts for submission, and prevents blind retries after ambiguous submission failures. Three issues remain: the approval sheet can lose precision when formatting large credit values, the active identity can change during authentication after the last ownership check, and token denomination metadata is read only from potentially stale local state.

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

  • Triage: critical by gpt-6-astra (effort low) — This is a large security- and funds-sensitive change spanning DashConnect parsing, identity ownership validation, critical-key signing, token purchase reconstruction, networking, and approval UI, where defects could authorize or charge the wrong identity or amount.
  • Phase 1 reviewers: not run (skipped for throughput: 21 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🔴 2 blocking

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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/Models/DashConnect/PlatformDashConnectDataSource.swift`:
- [BLOCKING] DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift:721-726: Recheck the active identity after authentication before submitting the purchase
  The ownership check is performed before `authorize()`, but `authorize()` awaits user interaction. If the active identity changes while the authentication prompt is open, the code proceeds with the previously captured `context` and constructs the signer for that stale identity. The purchase must re-resolve the active wallet/identity after authentication and reject if it no longer matches `request.ownerId`, immediately before creating the signer and submitting the purchase.
- [BLOCKING] DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift:666-710: Do not authorize token quantities from stale local decimals
  The quantity shown on the approval sheet is scaled using only the wallet's cached `PersistentToken.decimals` row. No current contract state is fetched or revalidated before approval, while token denomination metadata is mutable on Platform. If the contract changes its decimals after the wallet's last synchronization, the sheet can display a materially different quantity from the unchanged base-unit amount passed to `tokenPurchase`. Resolve the denomination from current verified contract state immediately before presenting or approving the request, or invalidate the approval when the current denomination differs from the displayed value; if it cannot be verified, show base units and do not imply a token-denominated quantity.

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
  (existing thread: https://github.com/dashpay/dashwallet-ios/pull/1109#discussion_r3954766347)
  `totalPriceDashText` now uses `PlatformCreditsFormatter.dashString`, but that formatter converts the `UInt64` credit amount through `Double` before formatting. `Double` cannot exactly represent every valid credit value above 2^53, so a large price can be displayed rounded even though the exact `UInt64` is still passed as `expectedTotalCost` and may be charged. Format the integer credits using integer/Decimal string arithmetic rather than `Double`, and add a regression test for a value above 2^53.

Comment thread DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift Outdated
Comment thread DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift Outdated
…on on chain

**The identity was checked before the prompt, not after.** `authorize()` waits on
the user, and the active wallet or identity can change while it is open. The
signer was built from the context captured beforehand, so the purchase could be
signed for — and debited from — an identity the user never saw on the sheet. The
context is re-resolved after authentication and must still match both
`request.ownerId` and the identity checked before the prompt.

**The quantity was scaled from a cached row.** Token denomination is mutable on
Platform, so `PersistentToken.decimals` written at the last sync can describe a
different denomination than the one the unchanged base-unit amount will be spent
under — on the screen where the user authorizes money. The decimals now come
from the contract as Platform holds it at approval time, with the same
`conventions.decimals` precedence the SDK's parser applies when it writes that
row. A contract that cannot be fetched, carries no token at the position, or
declares no decimals falls back to base units with the label that says so,
rather than to a plausible guess. The local row is no longer read at all.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Final validation — Phase 2 only (queue backlog)

At head 57557d7, nine prior findings are fixed; the approval-sheet precision issue remains at large credit values, and one new formatting-test expectation is incorrect. Standalone execution of the exact-head model and five extracted test bodies reproduced both issues, with one assertion failing under en_US. No blocking finding remains in the supplied evidence; this verification does not establish a passing workspace build or end-to-end runtime validation.

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

  • Triage: critical by gpt-6-astra (effort low) — This change authorizes on-chain token purchases from untrusted links using CRITICAL signing keys, so errors in intent parsing, identity ownership, token denomination, price validation, or submission handling could cause unauthorized or incorrect spending.
  • Phase 1 reviewers: not run (skipped for throughput: 23 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🟡 1 suggestion(s) | 💬 1 nitpick(s)

🤖 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 `DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift`:
- [SUGGESTION] DashWalletTests/DashConnect/PlatformDashConnectDataSourceTests.swift:291-293: Make token-formatting expectations match the locale-aware renderer
  The unknown-denomination branch renders through Decimal.string, which uses formatted(.number) and includes locale-dependent grouping. Executing the exact production model and this test body under en_US produces "100,000,000", so the assertion expecting "100000000" fails. The fractional test below also hardcodes a period although its NumberFormatter uses the current locale. Define the intended grouping and locale contract, then make the implementation and assertions agree; test the rendered output with deterministic locale-aware expectations.

In `DashWallet/Sources/Models/DashConnect/DashConnectModels.swift`:
- [NITPICK] DashWallet/Sources/Models/DashConnect/DashConnectModels.swift:87-88: Render the full Platform-credit precision on the approval sheet
  Using PlatformCreditsFormatter removes the former eight-fractional-digit limit, but that formatter first converts the UInt64 credits through Double. Executing the current implementation with 10_000_000_000_000_001 credits renders "100000 DASH" instead of "100000.00000000001 DASH", while tokenPurchase receives the exact integer as expectedTotalCost. The switch to an eleven-digit formatter therefore fixes ordinary sub-duff values but does not preserve full credit precision across the accepted range. Use decimal or integer arithmetic in the shared formatter and add boundary assertions against totalPriceDashText; the existing totalPriceDash assertions do not exercise the approval sheet's rendering path.

Comment on lines +291 to +293
let request = Self.purchaseRequest(credits: 1, tokenCount: 100_000_000, tokenDecimals: nil)
XCTAssertEqual(request.tokenQuantity.text, "100000000")
XCTAssertTrue(request.tokenQuantity.isBaseUnits)

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: Make token-formatting expectations match the locale-aware renderer

The unknown-denomination branch renders through Decimal.string, which uses formatted(.number) and includes locale-dependent grouping. Executing the exact production model and this test body under en_US produces "100,000,000", so the assertion expecting "100000000" fails. The fractional test below also hardcodes a period although its NumberFormatter uses the current locale. Define the intended grouping and locale contract, then make the implementation and assertions agree; test the rendered output with deterministic locale-aware expectations.

source: ['claude']

Comment on lines +87 to +88
var totalPriceDashText: String {
PlatformCreditsFormatter.dashString(totalAgreedPriceCredits)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Render the full Platform-credit precision on the approval sheet

Using PlatformCreditsFormatter removes the former eight-fractional-digit limit, but that formatter first converts the UInt64 credits through Double. Executing the current implementation with 10_000_000_000_000_001 credits renders "100000 DASH" instead of "100000.00000000001 DASH", while tokenPurchase receives the exact integer as expectedTotalCost. The switch to an eleven-digit formatter therefore fixes ordinary sub-duff values but does not preserve full credit precision across the accepted range. Use decimal or integer arithmetic in the shared formatter and add boundary assertions against totalPriceDashText; the existing totalPriceDash assertions do not exercise the approval sheet's rendering path.

source: ['claude']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants