feat(swift-example-app): multi-recipient Core L1 send (CORE-10)#3904
Conversation
The Core->Core send flow only let you pay a single recipient, even though `core_wallet_send_to_addresses` (via the `sendToAddresses` wrapper) already takes parallel address/amount arrays and the Rust side already does coin selection + multi-output tx construction. This surfaces that capability in the UI — a UI + marshalling change only, no new FFI. SendViewModel: - Add a `CoreRecipient` row type + `additionalCoreRecipients`, with `add/removeCoreRecipient`. - `coreRecipients` builds the ordered `[(address, amountDuffs)]` list (primary row + extras) and returns nil unless EVERY row is a valid on-network Core address with amount > 0, so the batch is gated atomically. `coreSendTotalDuffs` sums it for the summary. - `canSend` (.coreToCore) and the `.coreToCore` send path now build from `coreRecipients` and pass the array straight to the existing `sendToAddresses(recipients:)`. Success message reflects the output count. All other flows untouched. SendTransactionView: - "Additional Recipients" section with an "Add recipient" button, shown only for the coreToCore flow; per-row address/amount fields, a remove control, and a per-row Core-address badge / inline hint. - "Outputs" summary (per-output + Total + Fee) when there is more than one recipient. Single-recipient remains the simple default. TEST_PLAN: CORE-10 flipped from no-UI to implemented with the entry point; removed from the SDK-only appendix. Verified on the testnet simulator: a 0.001 + 0.002 DASH send to two addresses produced one tx with three vouts (two recipients + change), txlock true at block 1,495,895; sender balance dropped by sum + fee and both recipients were credited after sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multi-recipient support to the ChangesCore Multi-Recipient Send
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 docstrings
🧪 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 f5ac4d6) |
![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/Core/ViewModels/SendViewModel.swift`:
- Around line 216-218: The coreSendTotalDuffs property uses unchecked addition
with the + operator in the reduce closure when summing amountDuffs across
recipients. Replace the unchecked addition with a safe addition approach that
detects and handles overflow. Use Swift's safe arithmetic methods like
addingReportingOverflow() in the reduce closure to safely accumulate the total,
and either log/handle overflow errors or enforce a maximum limit to prevent
silent wrapping or crashes.
🪄 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: 100117e8-404f-4de5-854b-2228694f3f95
📒 Files selected for processing (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
UI-only change that surfaces an existing Rust multi-output Core wallet API. The new Swift marshalling correctly delegates coin selection and tx construction to Rust, and gating reduces to the prior primary-only check when no extras are present. One real but edge-case UInt64 overflow in the new summary sum, plus a test-coverage gap and a minor duplicate-address UX nit.
🟡 2 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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift:216-218: UInt64 sum in `coreSendTotalDuffs` can trap on overflow with pathologically large inputs
`coreRecipients` validates each row independently as a UInt64 with no aggregate check, and `coreSendTotalDuffs` reduces with unchecked `+`. Two rows whose amounts are individually valid but whose sum exceeds `UInt64.max` would trap Swift in the form view as soon as the multi-output summary renders, before Rust ever sees the batch. Realistic Dash amounts can't get near that bound (max supply is ~2.1e15 duffs vs UInt64.max ~1.8e19), so this is an edge case driven only by deliberate huge input, but it is reachable from user input and easily mitigated by switching to `addingReportingOverflow` (or summing as `UInt64?` with `nil` on overflow) in `coreRecipients`/`coreSendTotalDuffs` and treating overflow as an invalid batch. Downgraded from the agent's blocking severity because the trap is not reachable for any plausible wallet amount and this is the example app, not consensus code.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift:185-218: Add unit coverage for the new `coreRecipients` marshalling and gating
`coreRecipients` is now the single source of truth for both `canSend` gating and the ordered array handed to `sendToAddresses`, and `coreSendTotalDuffs` drives the summary total. The PR documents only manual happy-path simulator runs. A small set of view-model tests — primary-plus-extras ordering, primary-only reduction, an invalid extra address or sub-unit amount returning nil, row removal, and (if you adopt the overflow guard) overflow returning nil — would lock down the new behavior cheaply and prevent regressions reaching the FFI boundary.
| var coreRecipients: [(address: String, amountDuffs: UInt64)]? { | ||
| var outputs: [(address: String, amountDuffs: UInt64)] = [] | ||
|
|
||
| // Primary row. The trim mirrors the existing `.coreToCore` send | ||
| // case so the marshalled address matches what the badge validated. | ||
| let primaryAddress = recipientAddress | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard isCoreAddress(primaryAddress), | ||
| let primaryDuffs = amountDuffs, primaryDuffs > 0 | ||
| else { return nil } | ||
| outputs.append((address: primaryAddress, amountDuffs: primaryDuffs)) | ||
|
|
||
| // Extra rows, in order. | ||
| for row in additionalCoreRecipients { | ||
| let address = row.address | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard isCoreAddress(address), | ||
| let rowDuffs = duffs(forRecipientAmount: row.amountString), | ||
| rowDuffs > 0 | ||
| else { return nil } | ||
| outputs.append((address: address, amountDuffs: rowDuffs)) | ||
| } | ||
|
|
||
| return outputs | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Duplicate Core addresses across rows are accepted without warning
Pasting the same Core address into the primary row and an extra row (or into two extra rows) is accepted, and Rust will happily build a tx with two outputs paying the same address. The success message "Payment sent to \(recipients.count) recipients" then counts outputs, not distinct addresses, so a single distinct destination paid twice is reported as 2 recipients. Whether to dedupe, warn, or just rephrase the success message is a product call; flagging because the new multi-output surface makes this user-visible for the first time.
source: ['claude']
There was a problem hiding this comment.
Leaving the behavior as-is: paying the same Core address across two outputs is legitimate (Rust builds two real outputs), so the count intentionally reflects outputs, not distinct addresses. Deduping would block a valid case; rewording "recipients"→"outputs" reads worse in the common single-output path. Flagging for maintainers as a product call rather than acting unilaterally.
There was a problem hiding this comment.
Resolved in f5ac4d6 — Duplicate Core addresses across rows are accepted without warning no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "ef3c2a743c5debededa318d575e04ee3e1d114a2878e4b118090bd0f9a11d9e8"
)Xcode manual integration:
|
…erflow + add tests Review follow-up on CORE-10 (#3904). - `coreSendTotalDuffs` summed recipient amounts with an unchecked `+`, which could trap (debug) or wrap (release) if two individually-valid UInt64 amounts summed past UInt64.max — reachable from raw user input even though real Dash amounts never approach that bound. - Refactor the duplicated iteration into one private `coreRecipientPlan` that builds the ordered output list AND its running total in a single pass, accumulating with `addingReportingOverflow`. Aggregate overflow now invalidates the whole batch (returns nil) — same treatment as a bad address or zero amount — so Send is disabled and the summary shows no total rather than trapping. `coreRecipients` and `coreSendTotalDuffs` both derive from it, so the gated/sent list and displayed total can never disagree. No bare `+` remains; behavior is identical for all valid inputs. - Add `SendViewModelCoreRecipientsTests` (9 tests): primary-only, primary+extras ordering, invalid extra address → nil, sub-unit / empty amount → nil, aggregate overflow → nil (asserts a single huge row is still valid first, proving it's the aggregate path), and row removal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CI note — the "Swift SDK build + tests (warnings as errors)" failure is not from this change. The build succeeded and every test in the new |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative + incremental verification at f5ac4d6 confirms both agents' conclusion: no in-scope defects. The two substantive prior findings from e7cc899 are FIXED — UInt64 aggregate overflow is guarded by addingReportingOverflow in coreRecipientPlan (SendViewModel.swift:198-234), and view-model coverage is supplied by SendViewModelCoreRecipientsTests.swift covering primary-only, ordering, invalid address, sub-unit/empty amount, aggregate overflow (using 1e11 DASH rows that overflow only on sum), and remove-and-return-to-primary. The duplicate-address nitpick is intentionally deferred (the ordering test explicitly asserts duplicate Core outputs round-trip as N distinct outputs). PR scope is Swift UI + view-model marshalling to the pre-existing sendToAddresses wrapper, so no Rust/FFI changes are required. CodeRabbit reported zero inline findings and supplied no comment IDs.
|
CI update — the re-run reproduced the same single failure, so this is persistent (environment/data), not a transient flake:
Leaving the network test untouched rather than masking an unrelated, maintainer-owned test inside a CORE-10 feature PR. Flagging for a maintainer decision: admin-merge, or quarantine/skip |
2 participants
Issue being fixed or feature implemented
Test-plan item CORE-10 (Multi-recipient Core send) — tracked in #3897 — was marked 🔌 "SDK-only, no UI". The Core→Core send flow only let you pay a single recipient, even though the FFI
core_wallet_send_to_addresses(via theManagedCoreWallet.sendToAddresses(recipients:)wrapper) already takes parallel address/amount arrays, and coin selection + multi-output tx construction already happen entirely on the Rust side. This surfaces that capability in the UI.What was done?
A UI + marshalling-only change — no new FFI, no logic moved into Swift (per
packages/swift-sdk/CLAUDE.md). Swift just collects N(address, amount)pairs and passes them through the existing wrapper.SendViewModelCoreRecipientrow value type +@Published additionalCoreRecipients, withaddCoreRecipient/removeCoreRecipient.coreRecipientsbuilds the ordered[(address, amountDuffs)]list (primary row + extras) and returnsnilunless every row is a valid on-network Core address with amount > 0 — so the batch is validated atomically.coreSendTotalDuffssums it for the summary.canSend(.coreToCore) and the.coreToCorebranch ofexecuteSendnow build fromcoreRecipientsand pass the array straight tosendToAddresses(recipients:). The success message reflects the output count ("Payment sent to N recipients"). Every other flow (shielded / platform / coreToShielded …) is byte-for-byte unchanged.SendTransactionViewcoreToCoreflow; each extra row has its own address + amount fields, a remove control, and a per-row Core-address badge / inline "Not a Core address on this network" hint.TEST_PLAN.mdHow Has This Been Tested?
Built with
packages/swift-sdk/build_ios.sh --target sim+xcodebuild(BUILD SUCCEEDED), then driven end-to-end on a booted iPhone 17 / testnet simulator (simulator-controlskill /idb):TestnetWalletto two external addresses (a second on-device wallet) in one transaction. Success alert read "Payment sent to 2 recipients".insight.testnet): txid30010050…17f840fc, block 1,495,895,txlock: true, 3 vouts —0.001 → recipient A,0.002 → recipient B,0.811… → change. One input → one tx.1.81473134 → 1.81172874 DASH= sum (300,000) + fee (260 duffs).Full write-up posted to #3897.
Breaking Changes
None. UI-only addition; the FFI/wrapper signature was already array-based and is unchanged.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation