feat(swift-example-app): send from Core balance to a shielded recipient#3885
Conversation
The Send Dash screen only offered Shielded and Platform as funding sources for a shielded (Orchard) recipient — Core was never selectable even with a spendable Core balance, because the `.orchard` branch of `availableSources` never appended `.core`. The only way to fund a shielded send from Core money was the two-step shield-then-send, which was not intentional. Wire Core -> shielded through the existing Type 18 ShieldFromAssetLock path (`shieldedFundFromAssetLock`): - Add a `coreToShielded` SendFlow and offer `.core` as a source for an Orchard recipient, appended last so the private shielded->shielded path stays the auto-selected default. - Map `(.orchard, .core)` -> `.coreToShielded` and dispatch `executeSend` to `shieldedFundFromAssetLock`, parsing the recipient's 43-byte Orchard address and funding from the standard BIP44 Core account with the largest confirmed balance (asset locks draw UTXOs from a single account). - Gate the flow on a minimum lock size so a sub-fee amount can't kick off a doomed lock-build + ~30s proof. - Decouple the fee-display unit from the source unit (`feeUnit(for:)`): this flow spends Core duffs but its pool fee is denominated in credits, which would otherwise render 1000x too large. The typed amount is the L1 lock size (duffs); the recipient receives lock_value - pool_fee, mirroring ShieldedFundFromAssetLockView's "lock size, not net amount" convention (Type 18's Orchard value_balance is baked into the Halo 2 proof at build time and can't be re-derived). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a new ChangesCore-to-Shielded Send Flow Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 aebecb7) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Scoped, well-commented change wiring a new Core-to-shielded source through the existing Type 18 ShieldFromAssetLock API. Two real issues: the fee estimate for the new flow omits the asset_lock_base_cost component (UI under-reports the fee), and canSend/availableSources operate on the cross-account Core total while execution funds from a single BIP44 standard account — an amount within the displayed total but exceeding the chosen account's balance kicks off the ~30s lock+proof pipeline only to fail late.
🟡 2 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/Core/ViewModels/SendViewModel.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift:262-267: Core-to-shielded fee estimate omits asset_lock_base_cost
Mapping `.coreToShielded` to `ShieldedFeeKind.transfer` dispatches the FFI to `compute_minimum_shielded_fee` only. The actual Type 18 pool fee that Rust carves (rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:478) and drive-abci enforces is `compute_minimum_shielded_fee(num_actions) + asset_lock_base_cost`. The inline comment claiming Type 18 'carves the same compute_minimum_shielded_fee base' is misleading — it carves base + asset_lock_base_cost. The Send screen therefore displays a fee lower than the network will deduct from the lock value, and the implied recipient net (`lock_value − pool_fee`) is off by the same amount. The fix likely requires a new `ShieldedFeeKind` variant on the FFI side (e.g. `.shieldFromAssetLock`) since the existing three kinds don't include the asset-lock base cost; until that exists, at minimum drop the misleading comment and flag the displayed fee as a base-only estimate so users aren't led to expect the recipient to receive `amount − base_fee`.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift:171-175: canSend / availableSources don't gate on the single funding account that executeSend will use
Two related mismatches against execution reality:
1. `canSend` for `.coreToShielded` only checks `amountDuffs >= minShieldFromCoreDuffs`. But `executeSend` (lines 567–573) funds from a single account — the largest confirmed balance among BIP44 standard accounts (`typeTag == 0 && standardTag == 0`). A user whose Core funds are split across multiple BIP44 accounts can pass `canSend`, kick off the ~30s lock-build + Halo 2 proof pipeline, and only fail at the asset-lock submission step with 'No spendable Core account to fund the shield' or a backend error — exactly the late-failure scenario the `minShieldFromCoreDuffs` floor exists to prevent. The dedicated `ShieldedFundFromAssetLockView` already gates submit on `selectedCoreAccountBalanceDuffs >= amount` for this reason.
2. `availableSources` (and `coreBalanceSnapshot` in SendTransactionView, which sums every entry from `accountBalances`) makes `.core` selectable for Orchard recipients whenever any account has balance, including non-BIP44 / non-standard accounts that the execute-time filter excludes. A wallet whose Core funds live entirely in other `typeTag`/`standardTag` combinations would see the Core source available and then fail with 'No spendable Core account to fund the shield' after the user commits.
Fix: expose the chosen funding account's confirmed balance to the view model and gate `canSend` on `amountDuffs <= funding.confirmed`; mirror the same BIP44-standard predicate in `coreBalanceSnapshot`/`availableSources` so what the UI offers matches what execute time can actually spend, or at minimum do a synchronous pre-check at the top of the `.coreToShielded` case that fails fast before the async asset-lock pipeline starts.
| case .shieldedToShielded, .platformToShielded, .coreToShielded: | ||
| // Type 18 ShieldFromAssetLock carves the same | ||
| // `compute_minimum_shielded_fee` base as the Shield/transfer | ||
| // path; its fee is denominated in credits (deducted from the | ||
| // locked value), so the view renders it with the credits unit. | ||
| kind = .transfer |
There was a problem hiding this comment.
🟡 Suggestion: Core-to-shielded fee estimate omits asset_lock_base_cost
Mapping .coreToShielded to ShieldedFeeKind.transfer dispatches the FFI to compute_minimum_shielded_fee only. The actual Type 18 pool fee that Rust carves (rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:478) and drive-abci enforces is compute_minimum_shielded_fee(num_actions) + asset_lock_base_cost. The inline comment claiming Type 18 'carves the same compute_minimum_shielded_fee base' is misleading — it carves base + asset_lock_base_cost. The Send screen therefore displays a fee lower than the network will deduct from the lock value, and the implied recipient net (lock_value − pool_fee) is off by the same amount. The fix likely requires a new ShieldedFeeKind variant on the FFI side (e.g. .shieldFromAssetLock) since the existing three kinds don't include the asset-lock base cost; until that exists, at minimum drop the misleading comment and flag the displayed fee as a base-only estimate so users aren't led to expect the recipient to receive amount − base_fee.
source: ['codex']
| case .coreToShielded: | ||
| // Funded by an L1 asset lock denominated in duffs; gate on | ||
| // the lock floor so a doomed (sub-fee) amount can't kick off | ||
| // the lock-build + proof pipeline. | ||
| return (amountDuffs ?? 0) >= Self.minShieldFromCoreDuffs |
There was a problem hiding this comment.
🟡 Suggestion: canSend / availableSources don't gate on the single funding account that executeSend will use
Two related mismatches against execution reality:
-
canSendfor.coreToShieldedonly checksamountDuffs >= minShieldFromCoreDuffs. ButexecuteSend(lines 567–573) funds from a single account — the largest confirmed balance among BIP44 standard accounts (typeTag == 0 && standardTag == 0). A user whose Core funds are split across multiple BIP44 accounts can passcanSend, kick off the ~30s lock-build + Halo 2 proof pipeline, and only fail at the asset-lock submission step with 'No spendable Core account to fund the shield' or a backend error — exactly the late-failure scenario theminShieldFromCoreDuffsfloor exists to prevent. The dedicatedShieldedFundFromAssetLockViewalready gates submit onselectedCoreAccountBalanceDuffs >= amountfor this reason. -
availableSources(andcoreBalanceSnapshotin SendTransactionView, which sums every entry fromaccountBalances) makes.coreselectable for Orchard recipients whenever any account has balance, including non-BIP44 / non-standard accounts that the execute-time filter excludes. A wallet whose Core funds live entirely in othertypeTag/standardTagcombinations would see the Core source available and then fail with 'No spendable Core account to fund the shield' after the user commits.
Fix: expose the chosen funding account's confirmed balance to the view model and gate canSend on amountDuffs <= funding.confirmed; mirror the same BIP44-standard predicate in coreBalanceSnapshot/availableSources so what the UI offers matches what execute time can actually spend, or at minimum do a synchronous pre-check at the top of the .coreToShielded case that fails fast before the async asset-lock pipeline starts.
source: ['claude', 'codex']
2 participants
Issue being fixed or feature implemented
On the SwiftExampleApp Send Dash screen, when the recipient was a shielded (Orchard) address the Send From list only offered Shielded and Platform — Core was never selectable, even with a spendable Core balance. The
.orchardbranch ofSendViewModel.availableSourcesnever appended.core, so the only way to fund a shielded send from Core money was a manual two-step (shield Core → self, then send shielded → recipient). That two-step was not intentional.The underlying capability already ships: Type 18
ShieldFromAssetLock(shieldedFundFromAssetLock) takes Core L1 funds and an arbitrary recipient Orchard address. It was simply not wired into the unified Send flow.What was done?
Wired Core → shielded into the Send flow via the existing
shieldedFundFromAssetLockpath. No Rust/FFI changes — Swift example-app only.SendViewModel.swift:coreToShieldedSendFlow(display "Shield from Core",lock.shieldicon, fee metadata).availableSourcesnow offers.corefor an Orchard recipient, appended last so the private shielded → shielded path stays the auto-selected default.updateFlowmaps(.orchard, .core)→.coreToShielded.executeSenddispatches the new flow toshieldedFundFromAssetLock: parses the recipient's 43-byte Orchard address and funds from the standard BIP44 Core account (typeTag/standardTag == 0) with the largest confirmed balance (asset locks draw UTXOs from a single account).canSendgates the flow on a minimum lock size (minShieldFromCoreDuffs, mirroringShieldedFundFromAssetLockView.minDuffs) so a sub-fee amount can't kick off a doomed lock-build + ~30s Halo 2 proof.estimateFeemaps the flow to the.transferfee kind (Type 18 carves the samecompute_minimum_shielded_feebase).SendTransactionView.swift:flowColorfor the new flow.feeUnit(for:)to decouple the fee-display unit from the source unit — this flow spends Core duffs but its pool fee is denominated in credits, which would otherwise render 1000× too large. Behaviour-preserving for every existing flow.Design notes for reviewers
lock_value − pool_fee. This mirrorsShieldedFundFromAssetLockView's "lock size, not net amount" convention rather than grossing up the fee — Type 18's Orchardvalue_balanceis baked into the Halo 2 proof at build time and can't be re-derived afterward.await, not staged progress: matches the other six flows inexecuteSend. Tradeoff: the "Sending…" overlay can sit for a minute+ while the asset-lock pipeline runs (build lock → IS-lock/ChainLock wait → proof → submit). The dedicatedShieldedFundFromAssetLockViewremains for the rich staged-progress experience. This path bypasses the shield coordinator, but the Rustshield_guardmutex still serializes shield-class ops per wallet, so concurrent attempts block rather than corrupt.How Has This Been Tested?
./build_ios.sh --target sim(which builds the example app with-warnings-as-errors) completes with** BUILD SUCCEEDED **; bothSendViewModel.swiftandSendTransactionView.swiftcompile as part of it, with zero Swift warnings/errors. (A fresh FFI build was required because the prebuilt xcframeworks on disk all predate theplatform_wallet_mnemonic_*FFI symbols this branch'sSwiftDashSDKsources need.)SendFlowswitch sites for exhaustiveness (displayName/iconName/estimatedFee/canSend/estimateFee/executeSend/flowColor/feeUnit).shieldedFundFromAssetLock/ShieldedFundFromAssetLockRecipientas used byShieldedFundFromAssetLockView;accountBalances(for:);DashAddress.parse.orchardextraction as used by the shielded → shielded flow).Breaking Changes
None. Swift example-app changes only; no consensus, protocol, FFI, or SDK public-API changes.
Checklist:
For repository code-owners and collaborators only