feat(swift-example-app): production Withdraw Credits UI on identity detail (ID-10)#3906
Conversation
…etail (ID-10) Identity credit withdrawal to an L1 Dash address (test-plan ID-10) was builder-only — reachable only via Settings -> Platform State Transitions -> Identity Credit Withdrawal, which calls the low-level dash_sdk_identity_withdraw with a test signer. This adds a production entry point next to Transfer Credits. The Rust -> FFI -> Swift-SDK withdraw stack already existed and mirrors the transfer stack 1:1 (platform_wallet_withdraw_credits_with_signer -> IdentityWallet::withdraw_credits_with_external_signer -> rs-sdk identity.withdraw, with ManagedPlatformWallet.withdrawCredits already wrapping the FFI). So this is pure Swift UI — no Rust changes — and the broadcast routes through rs-platform-wallet using the wallet's keychain-backed signer, per the swift-sdk architecture rule (Swift only collects inputs + bridges). - New WithdrawCreditsView: mirrors TransferCreditsView (DASH->credits parsing, balance validation, submitGeneration guard, fresh KeychainSigner per submit), swapping the recipient picker for a network-validated destination L1-address field. Success screen shows the destination + amount; it intentionally shows no L1 txid because Platform withdrawals are pooled and paid out asynchronously by the network (no txid exists at broadcast). The credit decrease reflects via the persister -> SwiftData -> @query path. - IdentityDetailView: "Withdraw Credits" button + sheet immediately after Transfer Credits, in the same wallet-gated section. Verified end-to-end on the simulator (devnet/paloma): withdrew 0.001 DASH from a funded identity to an L1 address; "Withdrawal submitted" success, identity balance dropped 0.09659655 -> 0.09331446 DASH and the detail view reflected it. Withdrawal signing requires a TRANSFER/CRITICAL key; an older identity without one failed with a clean "no withdrawal public key" error and no balance change, so a transfer key was added via ID-07 first. TEST_PLAN ID-10 updated 🧪 -> ✅. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 19 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ChangesWithdraw Credits Feature
Sequence DiagramsequenceDiagram
participant User
participant IdentityDetailView
participant WithdrawCreditsView
participant KeychainSigner
participant ManagedPlatformWallet
User->>IdentityDetailView: tap "Withdraw Credits"
IdentityDetailView->>WithdrawCreditsView: present as sheet (walletManager injected)
User->>WithdrawCreditsView: enter destination address + DASH amount
WithdrawCreditsView->>WithdrawCreditsView: derive canSubmit (validate address, parse credits, check balance)
User->>WithdrawCreditsView: tap "Withdraw"
WithdrawCreditsView->>KeychainSigner: construct signer from identity key
WithdrawCreditsView->>ManagedPlatformWallet: withdrawCredits(amount, toAddress, signer)
ManagedPlatformWallet-->>WithdrawCreditsView: success → show success UI (async L1 payout note)
ManagedPlatformWallet-->>WithdrawCreditsView: failure → show error alert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Review complete (commit c4ce64c) |
![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
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift`:
- Around line 68-96: The NavigationStack in WithdrawCreditsView allows
swipe-to-dismiss even while a withdrawal is being submitted via the write
operation wallet.withdrawCredits. While the Cancel button is properly disabled
using isSubmitting, the interactive swipe-to-dismiss gesture remains enabled,
allowing users to dismiss the sheet and reopen it to trigger duplicate
withdrawals. Add the .interactiveDismissDisabled(isSubmitting) modifier to the
NavigationStack to prevent sheet dismissal while isSubmitting is true, ensuring
the write operation completes before the view can be dismissed.
🪄 Autofix (Beta)
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
Run ID: 989a5f4f-6744-44b8-bcc9-df12fc3b7715
📒 Files selected for processing (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
…in flight Cancel was disabled during submit, but the sheet's interactive swipe-to-dismiss was still enabled, so the user could drop the sheet mid-broadcast, reopen it, and fire a second withdrawal on this write path. Add `.interactiveDismissDisabled(isSubmitting)` to the sheet's root. Addresses CodeRabbit review on PR #3906. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Pure SwiftUI addition for the production Withdraw Credits flow that mirrors TransferCreditsView shape and correctly defers L1 address + balance validation into Rust. One real crash-class bug carried over from the copied parsedCredits pattern (Double(UInt64.max) rounds up to 2^64, so the guard admits a value that traps on UInt64 cast). One UX suggestion to use the existing network-aware DashAddress.parse so the submit button doesn't enable for obviously invalid destinations.
🔴 1 blocking | 🟡 1 suggestion(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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift:253-254: parsedCredits can crash on large amounts: Double(UInt64.max) rounds up to 2^64
`Double(UInt64.max)` is not exactly representable and rounds up to 2^64, so the `credits <= Double(UInt64.max)` guard admits a value (2^64) that is one above `UInt64`'s representable range. A user typing an amount that scales to ~2^64 credits (DASH input around 1.844e8) makes SwiftUI recompute `parsedCredits` during keystrokes, and `UInt64(credits)` traps before the submit-time re-validation can run — crashing the withdrawal sheet on input rather than producing a friendly error. The same shape was copied from `TransferCreditsView.parsedCredits`; the fix here is to switch the upper bound to a strict `<` so 2^64 is rejected, which keeps the result within UInt64's representable range.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift:263-267: Submit enables for any non-empty destination — use DashAddress.parse for local pre-validation
`canSubmit` requires only that `trimmedAddress` be non-empty, so the Withdraw button lights up for obviously malformed or wrong-network strings and the user only learns it was invalid after the round-trip into Rust. The app already has `DashAddress.parse(_, network:)` (Core/Models/DashAddress.swift) which performs Base58Check + network validation via the same Rust FFI used elsewhere, so a local `.core` check would tighten the UI without duplicating consensus logic — authoritative validation still happens in Rust on submit, matching the file header comment.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative re-review at c4ce64c. Latest delta (c4ce64c) adds .interactiveDismissDisabled(isSubmitting) to the sheet — a sound fix closing a real re-entry window during submit; no new findings in this delta. Both prior findings remain STILL VALID at the current head and are carried forward: (1) parsedCredits admits 2^64 because Double(UInt64.max) rounds up, then traps on UInt64(credits); (2) canSubmit only requires a non-empty destination instead of using the available DashAddress.parse(_, network:) Rust-backed pre-validation.
🔴 1 blocking | 🟡 1 suggestion(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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift:258-259: [Carried forward — STILL VALID] parsedCredits traps on amounts at the UInt64 boundary
Verified at current head (c4ce64c18b, lines 258–259). `Double(UInt64.max)` is not exactly representable in IEEE-754 binary64 and rounds up to `2^64` (18446744073709551616.0). The guard `credits <= Double(UInt64.max)` therefore admits a value equal to `2^64`, which then traps in `UInt64(credits)` on line 259. Because SwiftUI recomputes `parsedCredits` on every keystroke (it is read from `amountSection` during render), a single typed character around the boundary crashes the withdrawal sheet before any submit-time backend validation can run. Reaching the bucket requires ~1.84×10^8 DASH, which is unrealistic on mainnet but trivially reachable on regtest/devnet and via paste/UI automation — and the failure mode is a hard process abort on a funds-moving screen. The fix is to make the upper bound strict so values that round to `2^64` are rejected. The same pattern lives in `TransferCreditsView`; that file is unchanged by this PR and is captured as an out-of-scope follow-up.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawCreditsView.swift:268-272: [Carried forward — STILL VALID] Gate canSubmit on DashAddress.parse for local Core-address pre-validation
Verified at current head (c4ce64c18b, lines 268–272). `canSubmit` requires only `!trimmedAddress.isEmpty`, so the Withdraw button enables for malformed strings, wrong-network addresses, and fragments. Authoritative Base58Check + network validation does happen in Rust at submit time (and the `addressSection` footer advertises this), so this is not a funds-safety gap — it is a UX/defense-in-depth issue: users currently pay the round trip to discover an obviously-bad destination. The app already exposes `DashAddress.parse(_:network:)` (used throughout `SendViewModel`, `SendTransactionView`, and `QRScannerView`), which routes Core addresses through the Rust FFI for Base58Check and network validation. Requiring a parsed `.core` address matching `identity.network` in `canSubmit` (and the `submit()` pre-check) would catch the common cases locally while keeping the FFI submit path authoritative. This mirrors the precedent already established in the Core send flow.
| guard credits >= 1, credits <= Double(UInt64.max) else { return nil } | ||
| return UInt64(credits) |
There was a problem hiding this comment.
🔴 Blocking: [Carried forward — STILL VALID] parsedCredits traps on amounts at the UInt64 boundary
Verified at current head (c4ce64c, lines 258–259). Double(UInt64.max) is not exactly representable in IEEE-754 binary64 and rounds up to 2^64 (18446744073709551616.0). The guard credits <= Double(UInt64.max) therefore admits a value equal to 2^64, which then traps in UInt64(credits) on line 259. Because SwiftUI recomputes parsedCredits on every keystroke (it is read from amountSection during render), a single typed character around the boundary crashes the withdrawal sheet before any submit-time backend validation can run. Reaching the bucket requires ~1.84×10^8 DASH, which is unrealistic on mainnet but trivially reachable on regtest/devnet and via paste/UI automation — and the failure mode is a hard process abort on a funds-moving screen. The fix is to make the upper bound strict so values that round to 2^64 are rejected. The same pattern lives in TransferCreditsView; that file is unchanged by this PR and is captured as an out-of-scope follow-up.
| guard credits >= 1, credits <= Double(UInt64.max) else { return nil } | |
| return UInt64(credits) | |
| let credits = (dash * Double(Self.creditsPerDash)).rounded() | |
| guard credits >= 1, credits < Double(UInt64.max) else { return nil } | |
| return UInt64(credits) |
source: ['claude', 'codex']
| private var canSubmit: Bool { | ||
| !trimmedAddress.isEmpty | ||
| && managedWallet != nil | ||
| && (parsedCredits.map { $0 > 0 && $0 <= senderBalanceCredits } ?? false) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [Carried forward — STILL VALID] Gate canSubmit on DashAddress.parse for local Core-address pre-validation
Verified at current head (c4ce64c, lines 268–272). canSubmit requires only !trimmedAddress.isEmpty, so the Withdraw button enables for malformed strings, wrong-network addresses, and fragments. Authoritative Base58Check + network validation does happen in Rust at submit time (and the addressSection footer advertises this), so this is not a funds-safety gap — it is a UX/defense-in-depth issue: users currently pay the round trip to discover an obviously-bad destination. The app already exposes DashAddress.parse(_:network:) (used throughout SendViewModel, SendTransactionView, and QRScannerView), which routes Core addresses through the Rust FFI for Base58Check and network validation. Requiring a parsed .core address matching identity.network in canSubmit (and the submit() pre-check) would catch the common cases locally while keeping the FFI submit path authoritative. This mirrors the precedent already established in the Core send flow.
| private var canSubmit: Bool { | |
| !trimmedAddress.isEmpty | |
| && managedWallet != nil | |
| && (parsedCredits.map { $0 > 0 && $0 <= senderBalanceCredits } ?? false) | |
| } | |
| private var isValidDestinationAddress: Bool { | |
| if case .core = DashAddress.parse(trimmedAddress, network: identity.network).type { | |
| return true | |
| } | |
| return false | |
| } | |
| private var canSubmit: Bool { | |
| isValidDestinationAddress | |
| && managedWallet != nil | |
| && (parsedCredits.map { $0 > 0 && $0 <= senderBalanceCredits } ?? false) | |
| } |
source: ['claude', 'codex']
2 participants
Issue being fixed or feature implemented
Identity credit withdrawal to an L1 Dash address — iOS test-plan item ID-10 — was builder-only: reachable only via Settings → Platform State Transitions → Identity Credit Withdrawal, which calls the low-level
dash_sdk_identity_withdrawwith a test signer. There was no production "happy path" button for it. This adds one on the identity detail screen, next to Transfer Credits (the production transfer flow added in #3891).Tracked in QA issue #3897 (ID-10).
What was done?
The entire Rust → FFI → Swift-SDK withdraw stack already existed and mirrors the transfer stack 1:1:
platform_wallet_withdraw_credits_with_signer(rs-platform-wallet-ffi)IdentityWallet::withdraw_credits_with_external_signer(rs-platform-wallet)ManagedPlatformWallet.withdrawCredits(identityId:amount:toAddress:signer:)(swift-sdk)So this PR is pure Swift UI — no Rust changes. The broadcast routes through
rs-platform-walletusing the wallet's keychain-backed signer, per the swift-sdk architecture rule (Swift only collects inputs + bridges; no business logic).WithdrawCreditsView.swift— mirrorsTransferCreditsView(DASH→credits parsing, balance validation,submitGenerationguard, freshKeychainSignerper submit), swapping the recipient identity picker for a network-validated destination L1-addressTextField. Callswallet.withdrawCredits(...).@Querypath (same as transfer).IdentityDetailView.swift— "Withdraw Credits" button + sheet immediately after Transfer Credits, in the same wallet-gated section.TEST_PLAN.md— ID-10 row 🧪 → ✅ (production entry point); updated the entry-point reality-check note.How Has This Been Tested?
Built with
./build_ios.sh --target sim(BUILD SUCCEEDED,-warnings-as-errors), then driven end-to-end on the booted simulator (idb) on devnet/paloma:@Query.Prerequisite finding: withdrawal signing needs a TRANSFER/CRITICAL key. A funded identity that predates the keyId-3 derive fix (keys
[AUTH×3, ENC, DEC], no transfer key) first failed with a clean "missing key: no withdrawal public key" error and no balance change — confirming correct error handling and that the FFI bridge runs end-to-end. Adding a TRANSFER key via ID-07 (Add Key → Purpose = Transfer) made the subsequent withdrawal succeed. Newly-derived identities already get this key.Breaking Changes
None. UI-only addition to the example app; no public API or consensus changes.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation