feat(swift-sdk): seed shielded pool notes from the example app#3858
Conversation
Dash Platform enforces a 250-note anonymity-set minimum on every outgoing shielded transition. After a devnet reset without the DRIVE_SHIELDED_SNAPSHOT genesis ingest the pool starts empty and the whole shielded feature set is unusable. This adds a one-tap "Seed Pool Notes" utility to SwiftExampleApp that drives the pool up to a target note count in batches. - rs-dpp: `build_output_only_bundle` takes a `dummy_outputs` count and pads the bundle with zero-value outputs to fresh random addresses (unspendable anonymity-set fillers); threaded through both ShieldFromAssetLock builders. 0 preserves existing behavior exactly. - rs-platform-wallet: `shielded_seed_pool_notes` orchestrates ShieldFromAssetLock batches (1 real note to the wallet's own address + up to 5 fillers) until getShieldedNotesCount reaches the target, with per-batch progress, a hard mainnet gate, and retry-with-pause on FinalityTimeout (rapid batches can outrun core's unconfirmed-ancestor chain limit until a block lands). - Batch size is 6 actions, NOT the 16-action consensus cap: the Halo 2 proof grows ~2,681 B per on-wire action, so 6 actions (19,018 B) is the most that fits the 20 KiB max_state_transition_size / tenderdash max-tx-bytes (7 actions = 21,699 B is rejected as "Tx too large"). The stale "16 actions ≈ 11.8 KB" rationale comment in system_limits is corrected, and a new signing test pins the 6-action batch under max_state_transition_size with a real proof. - FFI + Swift: `platform_wallet_manager_shielded_seed_pool_notes` with a progress callback; SeedShieldedPoolView sheet (non-mainnet only). Verified live on devnet paloma (rc.1): seeded the pool 2 → 250 notes in 43 batches, after which the first IdentityCreateFromShieldedPool on the network executed cleanly (pool -0.1 DASH exactly, chain advancing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
✅ Review complete (commit 92d6b4f) |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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)
📝 WalkthroughWalkthroughThis PR adds configurable zero-value Orchard outputs for action-count padding (dpp builder), threads dummy-output counts through wallet fee/submit paths, implements a serial devnet/testnet pool-seeding orchestration with retries and progress, exposes an FFI entrypoint, and wires Swift SDK + UI for live seeding. ChangesShielded Pool Seeding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent findings worth surfacing. The blocking issue is in the new seed-pool retry loop: on FinalityTimeout the code discards the timed-out asset-lock outpoint and builds a fresh asset lock from wallet balance on the next attempt, which can strand the original lock and burn extra L1 funds during the exact long-running flow this PR adds. The non-blocking issue is documentation drift: three public-facing doc strings (FFI extern, Swift wrapper, and the Rust library method) still describe the old 16-action/15-filler batch sizing after MAX_ACTIONS_PER_BATCH was lowered to 6 in this same PR.
🔴 1 blocking | 🟡 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/rs-platform-wallet/src/wallet/shielded/seed_pool.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/seed_pool.rs:259-291: Finality retry rebuilds the asset lock instead of resuming the timed-out one
`PlatformWalletError::FinalityTimeout(out_point)` carries the exact outpoint of an already-broadcast and tracked asset lock whose IS/CL proof did not arrive in time. The retry arm captures `out_point` but does not use it — the next loop iteration calls `shielded_fund_from_asset_lock` with the unchanged `AssetLockFunding::FromWalletBalance { amount_duffs, account_index }`, which goes back through `resolve_funding_with_is_timeout_fallback`'s `FromWalletBalance` branch and builds/broadcasts a fresh L1 asset lock. The previously timed-out lock remains tracked but unconsumed.
During a 40+ batch seed this is the precise stall path the comment at lines 251–258 describes, and each retry walks user UTXOs to fund another lock instead of letting the original one finalize. Repeated stalls can therefore (a) consume multiple wallet UTXOs per failing batch, (b) leave a fan of orphaned tracked locks behind, and (c) keep timing out because the new lock chains on top of the same unconfirmed-ancestor depth the original hit.
After the first `FinalityTimeout`, switch the retry to `AssetLockFunding::FromExistingAssetLock { out_point }` so the pause+retry drives the originally tracked lock to completion (the manager already supports resume via `resume_asset_lock` and the IS→CL fallback also routes through this variant).
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:949-963: FFI doc-comment still advertises 16-action / 15-filler batches
The doc-comment that forms the C-ABI contract for `platform_wallet_manager_shielded_seed_pool_notes` claims each batch adds "up to 16 notes (1 real note … + up to 15 zero-value anonymity-set fillers)" and that a 250-note seed is "roughly 16 batches." This same PR pins `MAX_ACTIONS_PER_BATCH = 6` because 6 actions is the largest bundle that fits the 20 KiB `max_state_transition_size`; the in-tree module docstring, the `system_limits` comment, and the size-pinning test all correctly describe 6/5/~42. A 250-note seed is therefore ~42 batches (the PR description's live devnet run was 43). Out-of-tree consumers reading only the generated cbindgen header will size watchdog timeouts, progress UIs, and L1 funding ("one lock per 16 notes") off numbers that are ~3× off.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift:291-305: Swift wrapper doc-comment mirrors the stale 16/15/~16 figures
Same drift as the Rust FFI doc: `seedShieldedPoolNotes` claims each batch is "up to 16 notes (1 real note … + up to 15 zero-value anonymity-set fillers)" and "~16 batches." The actual cap (`MAX_ACTIONS_PER_BATCH = 6`) makes a 250-note seed ~42 batches. The `SeedShieldedPoolView` already uses the correct 6/5/~42 figures in its body; only this SDK-level doc lags.
…to 6-action batches Review follow-ups (#3858): - The FinalityTimeout retry now resumes the already-broadcast tracked lock via AssetLockFunding::FromExistingAssetLock { out_point } instead of building a fresh lock from wallet balance — re-funding stranded the original lock, burned an extra UTXO per attempt, and chained the new lock onto the same unconfirmed-ancestor depth that caused the stall. - FFI extern, Swift wrapper, and Rust method docs updated from the stale 16-action/15-filler/~16-batch figures to the actual 6/5/~42 (6 actions is the most that fits the 20 KiB max_state_transition_size). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/rs-platform-wallet/src/wallet/shielded/seed_pool.rs`:
- Around line 175-205: Move the early exit check so it runs before resolving the
recipient: call fetch_shielded_notes_count/sdk and compute start_notes and
batches_total_estimate and then perform the `if start_notes >=
target_total_notes` no-op return (creating SeedPoolOutcome and emitting
SeedPoolProgress) before calling seed_pool_recipient(account). This ensures
seed_pool_recipient(account) is only invoked when seeding is actually required
and avoids failing unbound shielded sub-wallets; touch symbols:
seed_pool_recipient, fetch_shielded_notes_count, start_notes,
target_total_notes, batches_total_estimate, SeedPoolOutcome, and
SeedPoolProgress.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SeedShieldedPoolView.swift`:
- Around line 190-194: The UI is showing a 0-based completed-batch counter as a
human "Batch" number; update the Text in SeedShieldedPoolView so it reflects
completed batches rather than a 1-based batch label — e.g. change the string
built around p.batchIndex/p.batchesTotalEstimate to read "Completed
\(p.batchIndex)/~\(p.batchesTotalEstimate) · \(p.poolNotesNow)/\(p.target)
notes" (or alternatively use p.batchIndex + 1 if you truly want a 1-based
"Batch" label) so the displayed meaning matches the emitted p.batchIndex value
used with ProgressView and progressFraction(p).
- Around line 57-85: The sheet allows interactive swipe-to-dismiss even when
seeding is running; add an interactiveDismissDisabled modifier driven by the
view's phase to prevent dismissing while work is in-flight. Locate the SwiftUI
view body in SeedShieldedPoolView (the NavigationStack/Form switch over phase)
and apply .interactiveDismissDisabled(phase == .inFlight) to the top-level view
(e.g., on the NavigationStack or Form) so the sheet cannot be swiped away while
phase == .inFlight; keep the existing toolbar Cancel button disabling as-is.
- Around line 150-158: The current ceiling calculation for batches uses (target
+ 5) / 6 which can overflow when parsedTarget == UInt64.max; change the logic in
the view where parsedTarget / target is used (the batches calculation) to
compute the ceiling without adding 5 — e.g., use integer division plus a
remainder check: compute batches as target / 6 plus 1 only if target % 6 != 0
(handle target == 0 appropriately), so the calculation in the
SeedShieldedPoolView that references parsedTarget/target and batches never
performs target+5 and thus cannot overflow.
🪄 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: 6a72416a-d41a-4341-9d37-c4255f6ab11e
📒 Files selected for processing (13)
packages/rs-dpp/src/shielded/builder/mod.rspackages/rs-dpp/src/shielded/builder/shield.rspackages/rs-dpp/src/shielded/builder/shield_from_asset_lock.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rspackages/rs-platform-version/src/version/system_limits/v1.rspackages/rs-platform-version/src/version/system_limits/v2.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/shielded/seed_pool.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SeedShieldedPoolView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All four prior findings from commit 0376f15 are verified fixed in the current head 430e6ce. The FinalityTimeout retry path now resumes the tracked asset lock via AssetLockFunding::FromExistingAssetLock { out_point } (seed_pool.rs:297) with an explanatory comment block, and all three doc-comments (Rust method doc, FFI C-ABI doc, Swift SDK wrapper doc) now correctly state 6 notes / 5 fillers / ~42 batches with rationale that 6 is MAX_ACTIONS_PER_BATCH bounded by 20 KiB max_state_transition_size, not the 16-action consensus cap. No new in-scope defects in the latest delta.
- Resolve the seeding recipient only after the already-satisfied no-op check, so a wallet whose pool target is already met succeeds even when the shielded sub-wallet isn't bound. - SeedShieldedPoolView: block interactive swipe-to-dismiss while a run is in flight (matches the disabled Cancel button), compute the batch estimate without the `target + 5` overflow on a pasted UInt64.max, and present batchIndex as a completed-batch count instead of "Batch 0/~N". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (4e2e672) is three small, correct fixes: reorder recipient resolution after the no-op early return so unbound shielded sub-wallets don't trip the already-satisfied path, replace (target + 5) / 6 with overflow-safe ceiling division to avoid a Swift UInt64.max trap, and add .interactiveDismissDisabled while a seed run is in flight. No in-scope findings; cumulative PR remains additive, mainnet-gated, with no consensus or serialization impact.
|
✅ 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: "3f158f14e839386d22b183235cc11d9c956d4063fb347404d6077fd3220947cf"
)Xcode manual integration:
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3858 +/- ##
=============================================
- Coverage 87.15% 70.73% -16.43%
=============================================
Files 2641 20 -2621
Lines 327793 2788 -325005
=============================================
- Hits 285701 1972 -283729
+ Misses 42092 816 -41276
🚀 New features to boost your workflow:
|
The macOS coverage job's shielded step (30-minute timeout, llvm-cov instrumented) timed out after this PR added two 16-action real proofs. Seeding no longer publishes 16-action transitions (6 is the most that fits the 20 KiB size limit), so the heavyweight proofs weren't earning their cost: - the builder padding test now stops at 5 dummies (6 actions, the seeding maximum) instead of 15; - the 16-action signing test is removed, with its value_balance assertion folded into the 6-action size-pin test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta f8d674a is a CI-budget test reduction (drops 16-action real-proving signing test, shrinks dummy-padding parametric test from {0,1,15} to {0,1,5}); the seed_pool_batch_fits_max_state_transition_size test still pins the real 6-action seeding ceiling. Cumulative PR remains sound: dummy_outputs is additive with value_balance preserved, 6-action batch is justified by on-wire size, mainnet is hard-gated, FinalityTimeout retry path is correct. One in-scope doc nit: the consensus-fee doc on shield_from_asset_lock_pool_fee was edited in this PR to say 'up to 16 for a pool-seeding batch', but the seeding batch caps at MAX_ACTIONS_PER_BATCH = 6.
💬 1 nitpick(s)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review against 92d6b4f. The only delta from f8d674a is the doc-comment fix at fund_from_asset_lock.rs:452-457 that addresses the prior 'stale 16-action pool-fee doc' nitpick — verified at current head. No new in-scope findings; the rust-quality test-coverage nit about estimate_batches using per_batch=16 is dropped as a weak style nit (the function is pure arithmetic and the production cap is already pinned by batch_size_math_matches_action_count).
2 participants
Issue being fixed or feature implemented
Dash Platform enforces a 250-note anonymity-set minimum on every outgoing shielded transition (transfer / unshield / withdrawal / identity-create-from-pool). After a devnet platform reset without the
DRIVE_SHIELDED_SNAPSHOTgenesis ingest, the pool starts empty and the entire shielded feature set is rejected with "pool has N notes but minimum 250 required" — exactly what happened on paloma after the rc.1 reset. Re-baking and redeploying a snapshot for every reset is heavy; this gives the example app a one-tap, batched way to seed the pool.What was done?
build_output_only_bundlegained adummy_outputscount — zero-value outputs to fresh random Orchard addresses (their spending keys are dropped, so they're pure unspendable anonymity-set fillers,ovk=None). Threaded through bothShieldFromAssetLockbuilders;dummy_outputs = 0preserves existing behavior exactly (existingserializes_to_min_actionstest still passes).PlatformWallet::shielded_seed_pool_notesorchestration — serialShieldFromAssetLockbatches (1 real ~1e6-credit note to the wallet's own default shielded address + up to 5 fillers) until the on-chaingetShieldedNotesCountreaches the target. Per-batch progress callback, hard mainnet gate, and retry-with-pause onFinalityTimeout(rapid back-to-back batches chain unconfirmed L1 change outputs; around ~25–30 of them IS/CL proofs stall until a core block lands — observed live).ShieldFromAssetLocksizes (real proofs): 2 actions → 8,294 B, 6 → 19,018 B, 7 → 21,699 B. The Halo 2 proof grows ~2,273 B/action, so 6 is the most that fits the 20 KiBmax_state_transition_size/ tenderdashmax-tx-bytes = 20480(a 16-action transition is rejected by the mempool as "Tx too large"). The stale "16 actions ≈ 11.8 KB" rationale comment onmax_shielded_transition_actionsinsystem_limitsv1/v2 is corrected, and a new real-proof signing test (seed_pool_batch_fits_max_state_transition_size) pins the 6-action batch under the limit so a future proof-size change fails in CI instead of on a devnet.platform_wallet_manager_shielded_seed_pool_notes(handle, wallet_id, account, target_total_notes, funding_account_index, core_signer, progress_fn, progress_ctx).SeedShieldedPoolView— target field (default 250), live "Batch i/~n · M/target notes" progress.How Has This Been Tested?
cargo fmt --all --check;cargo checkclean fordpp(shielded-client + signing features,--tests),platform-wallet(+--tests),platform-wallet-ffi../build_ios.sh --target sim --profile releaseends with "Swift/Xcode build succeeded".IdentityCreateFromShieldedPoolever executed on the network landed cleanly (pool −0.1 DASH exactly, notes 250 → 252, chain advancing — the old type-20 chain-halt did not reproduce).Breaking Changes
None.
dummy_outputs = 0callers are byte-class identical to before; the new API surface is additive; the seeding utility hard-errors on mainnet. No consensus, serialization, or protocol-version changes (thesystem_limitschange is comment-only).Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Documentation