feat: shielded funding from asset-lock proofs#3753
Merged
Conversation
…gner Adds the orchestrated entry point for shielded funding from a Core L1 asset lock, parallel to the platform-address sibling shipped in #3671. Wires the full pipeline: DPP layer: - ShieldFromAssetLockTransitionMethodsV0::try_from_asset_lock_with_bundle_and_signer: Signer-based variant that produces the outer ECDSA signature via StateTransition::sign_with_core_signer (atomic derive + sign + zeroise inside the signer's trust boundary; raw key never crosses the FFI boundary). Gated on state-transition-signing + core_key_wallet. - build_shield_from_asset_lock_transition_with_signer: async builder that wraps the new method. Wallet layer (new wallet/shielded/fund_from_asset_lock.rs): - PlatformWallet::shielded_fund_from_asset_lock(funding, recipients, asset_lock_signer, prover, settings) -> Result<(), _> - Pipeline mirrors fund_from_asset_lock for platform addresses: preflight → resolve_funding_with_is_timeout_fallback → submit_with_cl_height_retry → IS->CL fallback on Platform-side IS rejection → consume_asset_lock. - Recipient API: Vec<(OrchardAddress, Option<Credits>)> with preflight enforcing len() == 1 today. TODO at the preflight site to lift once DPP grows multi-output Orchard bundles for Type 18 (a build_output_only_bundle change shared with Type 15). - Shield amount = asset_lock_value − protocol_min_fee. Asset-lock value is read from the IS proof's TxOut when available, else looked up by outpoint in the asset-lock manager. Cleanup: - Removed wallet/shielded/operations.rs::shield_from_asset_lock (the raw-key, single-recipient stub). No external callers — the shielded_send.rs FFI module comment explicitly flagged it as unwired. The DPP-layer raw-key methods remain for the SDK trait and any external integrations. Tests: 4 preflight unit tests (empty, multi-recipient, single, single-with-amount). Orchestration itself is not unit-testable without significant SDK + Core mock infrastructure; coverage will come from drive-abci integration tests.
Adds two FFI entry points wrapping PlatformWallet::shielded_fund_from_asset_lock: - platform_wallet_manager_shielded_fund_from_asset_lock (fresh build from wallet balance) - platform_wallet_manager_shielded_resume_fund_from_asset_lock (resume by outpoint, crash-recovery shape) Both follow the address-funding pattern shipped in #3671: the asset-lock-proof signature is produced by a MnemonicResolverHandle (Keychain resolver on the Swift side), the raw key never crosses the FFI boundary. FFI is single-recipient today, matching the orchestration's preflight constraint. Multi-recipient becomes a new FFI signature when DPP grows multi-output Orchard bundles for Type 18. Also adds PlatformWallet::network() (delegating to the asset-lock manager) — the FFI needs it for constructing the MnemonicResolverCoreSigner, and the absence was a small asymmetry with PlatformAddressWallet. Drops the pre-existing 'isn't wired here yet' TODO from the module doc.
…Manager Adds two extension methods on PlatformWalletManager: - shieldedFundFromAssetLock(walletId, fundingAccountIndex, amountDuffs, recipients) - shieldedResumeFundFromAssetLock(walletId, outPointTxid, outPointVout, recipients) Wraps the new platform_wallet_manager_shielded_fund_from_asset_lock and *_resume FFI calls. Mirrors the ManagedPlatformAddressWallet.fundFromAssetLock pattern: detached Task.userInitiated, MnemonicResolver constructed on the calling actor and pinned via withExtendedLifetime across the FFI call (so the -O optimizer can't elide it and leave the vtable callback dangling). Architecture note: shielded operations live on PlatformWalletManager (process-global coordinator scope), not on a per-wallet 'ManagedShieldedWallet' object — the existing ShieldedSync, Transfer, Shield, Unshield, Withdraw methods are all on the manager, and the new funding methods follow that convention. ShieldedFundFromAssetLockRecipient is a multi-shape Sendable struct (raw 43-byte recipient + optional credits) so the call-site API doesn't change when DPP grows multi-output Orchard bundles for Type 18. Preflight today rejects empty / multi- recipient lists with a TODO at the guard site.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds external-signer DPP builders and tests, PlatformWallet orchestration and network accessor, new FFI entry points for shielded funding/resume using a MnemonicResolverHandle, and Swift async APIs and recipient type. Moves the in-file operation to an orchestrated wallet implementation. ChangesShielded Asset-Lock Funding Pipeline
Sequence Diagram(s)sequenceDiagram
participant App as Swift App
participant Manager as PlatformWalletManager
participant FFI as platform_wallet_manager_shielded_fund_from_asset_lock
participant Wallet as PlatformWallet
participant Orchestration as shielded_fund_from_asset_lock
participant Builder as build_shield_from_asset_lock_transition_with_signer
participant Signer as MnemonicResolverCoreSigner
participant Network as Dash Network
App->>Manager: shieldedFundFromAssetLock(...)
Manager->>FFI: call platform_wallet_manager_shielded_fund_from_asset_lock(ptrs, handle)
FFI->>Wallet: wallet.shielded_fund_from_asset_lock(funding, recipient, signer_handle, prover)
Wallet->>Orchestration: validate_recipients & resolve funding
Orchestration->>Builder: build_shield_from_asset_lock_transition_with_signer(...)
Builder->>Signer: request asset-lock signature via derivation path
Builder->>Network: submit signed StateTransition (via broadcast)
Network-->>Wallet: proven execution / confirmation
Wallet-->>FFI: Result
FFI-->>Manager: PlatformWalletFFIResult
Manager-->>App: return / throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fund_from_asset_lock.rs`:
- Around line 394-417: The validate_shielded_recipients function currently
permits recipient tuples with Some(amount) even though amounts are ignored;
update it to fail fast when any recipient's Option<Credits> is Some(_).
Specifically, after the empty check in validate_shielded_recipients (signature:
fn validate_shielded_recipients<T>(recipients: &[(T, Option<Credits>)]) ->
Result<(), PlatformWalletError>), iterate recipients and return
Err(PlatformWalletError::AddressOperation(...)) if any second element is
Some(_), keeping the existing single-recipient length check and error messages
otherwise.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift`:
- Around line 239-245: The recipients loop must reject any caller-supplied
recipient credits until wiring is implemented: in the
ShieldedFundFromAssetLockRecipient validation (where recipients is iterated in
PlatformWalletManagerShieldedFunding.swift) add a guard that recipient.credits
is nil and throw PlatformWalletError.invalidParameter with a clear message if it
is non-nil; keep the existing recipientRaw43 length check and combine both
validations so callers cannot pass a non-nil credits value that would be ignored
by the current Rust/FFI path.
🪄 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: 69d5fe76-4f2c-4eab-b8ba-f17113bca76a
📒 Files selected for processing (11)
packages/rs-dpp/src/shielded/builder/mod.rspackages/rs-dpp/src/shielded/builder/shield_from_asset_lock.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/methods/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/methods/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/v0/v0_methods.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/platform_wallet.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/operations.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3753 +/- ##
============================================
+ Coverage 87.17% 87.33% +0.15%
============================================
Files 2607 2590 -17
Lines 319489 317082 -2407
============================================
- Hits 278507 276911 -1596
+ Misses 40982 40171 -811
🚀 New features to boost your workflow:
|
Contributor
|
✅ 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: "ce857d44724e4c6cf939e6e88b025aba1b3eccfffe3f61cddacc49ffa2b8953b"
)Xcode manual integration:
|
Mirrors the AddressFundingFromAssetLockTransition signing_tests pattern shipped with #3671. Four tests: 1. try_from_asset_lock_with_bundle_and_signer_produces_recoverable_compact_sig_v0 — exercises the V0 impl directly via a FixedKeySigner (key_wallet::signer::Signer); pins the 65-byte recoverable compact ECDSA signature shape that Type 18 expects on storage. 2. try_from_asset_lock_with_bundle_and_signer_via_outer_dispatcher — same call routed through the outer-enum version dispatcher in methods/mod.rs; covers the version-routing arm. 3. outer_dispatcher_rejects_unknown_serialization_version — synthesises a platform-version whose shield_from_asset_lock_state_transition.default_current_version is non-zero; pins the UnknownVersionMismatch error path so a future V1 introduction can't silently coerce to V0. 4. build_shield_from_asset_lock_transition_with_signer_end_to_end — full builder path with a real output-only Orchard bundle (TestProver builds the Halo 2 proving key on first call); this is the codepath the wallet orchestration uses in production. Bumps codecov patch coverage above the 50% threshold for the shielded-fund-from-asset-lock PR (the orchestration in rs-platform-wallet remains hard to unit-test without significant SDK mocking, so the DPP layer is where the bulk of the testable new code lives). Tests gated on (state-transition-signing + core_key_wallet), matching where the new method itself is gated.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs (1)
40-43: ⚡ Quick winAssert derivation-path passthrough in the test signer.
Line 64 and Line 72 ignore the incoming derivation path, so these tests won’t fail if path wiring regresses. Make
FixedKeySignervalidate the path to cover that contract.Suggested patch
struct FixedKeySigner { secret: SecretKey, public: PublicKey, + expected_path: DerivationPath, } impl FixedKeySigner { - fn new(seed: [u8; 32]) -> Self { + fn new(seed: [u8; 32], expected_path: DerivationPath) -> Self { let secp = Secp256k1::new(); let secret = SecretKey::from_byte_array(&seed).expect("valid secret"); let public = PublicKey::from_secret_key(&secp, &secret); - Self { secret, public } + Self { + secret, + public, + expected_path, + } } } @@ async fn sign_ecdsa( &self, - _path: &DerivationPath, + path: &DerivationPath, sighash: [u8; 32], ) -> Result<(ecdsa::Signature, PublicKey), Self::Error> { + if path != &self.expected_path { + return Err("unexpected derivation path".to_string()); + } let secp = Secp256k1::new(); let msg = Message::from_digest(sighash); Ok((secp.sign_ecdsa(&msg, &self.secret), self.public)) } - async fn public_key(&self, _path: &DerivationPath) -> Result<PublicKey, Self::Error> { + async fn public_key(&self, path: &DerivationPath) -> Result<PublicKey, Self::Error> { + if path != &self.expected_path { + return Err("unexpected derivation path".to_string()); + } Ok(self.public) } }Also applies to: 62-74
🤖 Prompt for 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. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs` around lines 40 - 43, The test signer FixedKeySigner currently ignores the incoming derivation path, so add an expected_path field to FixedKeySigner (e.g., expected_derivation_path: DerivationPath or Vec<u32>) and modify its signing method (the Signer impl / sign_* function used in these tests) to assert that the provided derivation path equals expected_derivation_path before returning the fixed key signature; this ensures the test verifies derivation-path passthrough rather than silently accepting any path.
🤖 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.
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs`:
- Around line 40-43: The test signer FixedKeySigner currently ignores the
incoming derivation path, so add an expected_path field to FixedKeySigner (e.g.,
expected_derivation_path: DerivationPath or Vec<u32>) and modify its signing
method (the Signer impl / sign_* function used in these tests) to assert that
the provided derivation path equals expected_derivation_path before returning
the fixed key signature; this ensures the test verifies derivation-path
passthrough rather than silently accepting any path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 436bf67e-a1da-452a-9b0e-5dc48e330574
📒 Files selected for processing (2)
packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Type 18 shielded fund-from-asset-lock cleanly mirrors the merged Type 14 sibling: signer-routed asset-lock signature, IS/CL fallback, single-flight shield_guard, and consume_asset_lock cleanup. The codex 'blocking' on byte-identical retries is incorrect — the Halo 2 proof in build_output_only_bundle uses OsRng so retried STs naturally diverge — but the underlying contract observation (user_fee_increase is a no-op for Type 18) is real and worth documenting. Remaining findings are suggestions/nitpicks: misuse of AddressSync error variant on the shielded path, the Option field accepted but ignored on both sides of the FFI, an undocumented empty memo, and a sibling-style unsafe-block consistency nit.
🟡 3 suggestion(s) | 💬 3 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/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:166-220: submit_with_cl_height_retry's user_fee_increase bump is a no-op for Type 18; retry diversification relies on Halo 2 proof randomness
The CL-height retry wrapper documents that each attempt bumps `PutSettings.user_fee_increase` to change the ST hash and bypass Tenderdash's invalid-tx cache. For `ShieldFromAssetLock` this field is not part of the transition: `ShieldFromAssetLockTransition::set_user_fee_increase` is a no-op and `user_fee_increase()` is pinned to 0 (locked in by `test_shield_from_asset_lock_user_fee_increase_is_zero_and_setter_noop` at packages/rs-dpp/src/state_transition/mod.rs:3137). The retried submissions differ today only because `build_output_only_bundle` builds a fresh Halo 2 proof with `OsRng` on every call (packages/rs-dpp/src/shielded/builder/mod.rs:220), so the proof bytes — and hence the ST hash — diverge naturally. That's a load-bearing implicit invariant of the prover, not a property this orchestration documents or enforces. Codex flagged this as blocking on the theory the bytes are identical; they are not. But the helper's stated mechanism does not apply here, so future changes to the prover (e.g. determinising it for reproducibility) would silently regress the retry into duplicate-hash submits that Tenderdash drops. Add a colocated comment pinning the assumption, or thread an explicit diversifier (e.g. memo bump) on retry so the property is enforced rather than incidental.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:302-332: lookup_asset_lock_value_credits returns AddressSync errors for the shielded flow
Three of the four error paths in `lookup_asset_lock_value_credits` construct `PlatformWalletError::AddressSync(...)` even though this function is shielded-only and the rest of the orchestration consistently uses `ShieldedBuildError` (lines 153, 161, 336). The fourth branch in the same function (`duffs.checked_mul(CREDITS_PER_DUFF)`) correctly uses `ShieldedBuildError`, which highlights the inconsistency. Surfacing `AddressSync` will route shielded-lookup failures into address-sync error handling in callers/log filters and mislabel them in user-facing telemetry. Switch the three `AddressSync(...)` constructions to `ShieldedBuildError(...)`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:394-417: Option<Credits> per recipient is accepted at both layers but never enforced or honored
The Rust API takes `Vec<(OrchardAddress, Option<Credits>)>` and the Swift API exposes `ShieldedFundFromAssetLockRecipient.credits: UInt64?`, but the shield amount is always computed as `asset_lock_value − min_fee` and assigned to `recipients[0]`. `validate_shielded_recipients` does not reject `Some(_)`, the Swift preflight at PlatformWalletManagerShieldedFunding.swift:239-245 also does not reject it, and both FFI entry points hard-code `vec![(recipient, None)]` (packages/rs-platform-wallet-ffi/src/shielded_send.rs:415, :502). A caller who passes `credits: 42` observes behavior different from the type signature: the value is silently dropped at the FFI boundary. The doc comments do mention this, but a typed rejection is louder and matches the multi-output TODO contract. Either reject `Some(_)` in both preflights (Rust + Swift) until the multi-output path lands, or honor the value end-to-end.
Out-of-scope follow-up suggestions (4)
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.
- shield_from_asset_lock minimum-fee constant is reused from address-funding (Type 14) for Type 18 —
shield_from_asset_lock_min_feereadsrequired_asset_lock_duff_balance_for_processing_start_for_address_fundingand uses that value as the Type 18 minimum. If Type 18's actual fee diverges from Type 14's, Platform will reject the transition (the asset lock stays unconsumed, so no fund loss) but UX degrades silently. Pre-existing protocol-versioning gap.- Follow-up: Open a separate issue to add a Type-18-specific
required_asset_lock_duff_balance_for_processing_start_for_shield_fundingconstant inplatform-versionand haveshield_from_asset_lock_min_feeread from it.
- Follow-up: Open a separate issue to add a Type-18-specific
- consume_asset_lock failure after successful Platform submit only logs — If
consume_asset_lockfails after Platform accepts the shield ST, the row stays in the resumable-funding list and a user-initiated resume is deterministically rejected by Platform ('lock already consumed'). Documented as intentional and shared with the address-funding sibling, not introduced here.- Follow-up: Track separately: add an internal 'consumed-but-bookkeeping-failed' status that the resume UI can render distinctly so users do not attempt resumes guaranteed to fail.
- is_instant_lock_proof_invalid only matches InvalidInstantAssetLockProofSignatureError — The IS→CL post-submit fallback depends on a shared helper that recognises a single consensus error. If drive-abci's Type 18 path emits a different basic error variant for IS-proof rejection than Type 14, the fallback never fires for shielded callers. Pre-existing helper, not modified here.
- Follow-up: Open a separate issue to enumerate the consensus errors drive-abci emits for Type 18 asset-lock-proof rejection and either widen
is_instant_lock_proof_invalidor add a shielded-specific variant.
- Follow-up: Open a separate issue to enumerate the consensus errors drive-abci emits for Type 18 asset-lock-proof rejection and either widen
- block_on_worker re-raises worker panics across extern "C" —
packages/rs-platform-wallet-ffi/src/runtime.rs:55doesrt.spawn(future).await.expect("tokio worker panicked"). A panic inside the spawned future (Halo 2 prover, GroveDB, etc.) is re-raised on the FFI caller's thread, which is anextern "C" fn— unwinding across the C ABI is UB on iOS/Android/JNI. The new Type 18 entry points inherit the hazard but every existing consumer ofblock_on_workeralready has it; the right fix is at the runtime helper.- Follow-up: Open a separate issue to wrap
block_on_worker'sJoinErrorin a typed FFI result and gate the join withstd::panic::catch_unwindso all existing FFI entry points stop unwinding across the C ABI.
- Follow-up: Open a separate issue to wrap
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at cb15ff8. The latest delta since 31b6e66 is small and well-scoped: four new DPP-layer signing tests for ShieldFromAssetLockTransition (V0 direct path, outer-enum dispatcher, unknown-version rejection, end-to-end builder with TestProver) plus a one-line Swift import fix (SwiftDashSDKFFI → DashSDKFFI). No new defects in the delta. All six prior findings remain STILL VALID — the orchestration file and FFI entry points are byte-identical in the relevant ranges. Recommending COMMENT: only suggestions/nitpicks, no blockers.
🟡 3 suggestion(s) | 💬 3 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/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:166-224: submit_with_cl_height_retry's user_fee_increase bump is a no-op for Type 18; retry diversification rides on Halo 2 OsRng
Carried forward — STILL VALID at cb15ff82. The CL-height retry wrapper documents (lines 166–170) that each attempt bumps `PutSettings.user_fee_increase` to change the ST hash and bypass Tenderdash's invalid-tx cache. For `ShieldFromAssetLockTransition` that field is not part of the transition: `set_user_fee_increase` is a no-op and `user_fee_increase()` is pinned to 0 (locked in by `test_shield_from_asset_lock_user_fee_increase_is_zero_and_setter_noop` in `packages/rs-dpp/src/state_transition/mod.rs`). Retried submissions actually differ today only because `build_output_only_bundle` builds a fresh Halo 2 proof with `OsRng` per call — that's a load-bearing implicit prover invariant, not a property this orchestration enforces. A future determinisation of the prover (for reproducibility/audit-replay/caching) would silently regress this into duplicate-hash submits Tenderdash drops, stranding the single-use asset lock outpoint with no in-app recovery signal: each subsequent Resume would reproduce the same hash. Pin the assumption with a colocated comment in `build_and_broadcast_shielded`, or thread an explicit per-retry diversifier (e.g. perturb the currently-hardcoded `[0u8; 36]` memo) so the property is enforced rather than incidental.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:307-332: lookup_asset_lock_value_credits returns AddressSync errors on the shielded-only path
Carried forward — STILL VALID at cb15ff82 (verified inline at lines 310, 318, 328). Three of the four error paths in `lookup_asset_lock_value_credits` construct `PlatformWalletError::AddressSync(...)`, even though this function is shielded-only and the surrounding orchestration consistently uses `ShieldedBuildError` (lines 153, 160, 336). The fourth branch in the same function — the `duffs.checked_mul(CREDITS_PER_DUFF)` guard at line 335 — already uses `ShieldedBuildError`, which makes the inconsistency starker. These errors surface across the FFI as `PlatformWalletFFIResult` strings; routing them into address-sync error handling in callers/log filters mislabels them in user-facing telemetry. Switch the three `AddressSync(...)` constructions to `ShieldedBuildError(...)`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:394-461: Option<Credits> per recipient is accepted at both layers but silently dropped at the FFI boundary
Carried forward — STILL VALID at cb15ff82, and arguably reinforced by the new delta: the latest commits don't change this, but the wallet-layer test `validate_accepts_single_recipient_with_some_amount` (lines 453–460) explicitly pins that `Some(_)` is accepted without rejection. The Rust API takes `Vec<(OrchardAddress, Option<Credits>)>` and the Swift API exposes `ShieldedFundFromAssetLockRecipient.credits: UInt64?`, but the shield amount is always computed as `asset_lock_value − min_fee` and assigned to `recipients[0]`. `validate_shielded_recipients` doesn't reject `Some(_)`, the Swift preflight (`PlatformWalletManagerShieldedFunding.swift:239–245`) doesn't either, and both FFI entry points hard-code `vec![(recipient, None)]` (`packages/rs-platform-wallet-ffi/src/shielded_send.rs:415, :502`). A caller passing `credits: 42` observes behavior different from the type signature — the value is silently dropped at the FFI boundary. Doc comments at lines 57–63 mention this, but a typed rejection is louder and matches the multi-output TODO contract. Either reject `Some(_)` in both preflights (Rust + Swift) until multi-output Orchard bundles ship, or honor the value end-to-end (including a new FFI parameter).
Out-of-scope follow-up suggestions (1)
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.
- StateTransitionLike::set_user_fee_increase is a silent no-op for Type 18 — The
set_user_fee_increaseno-op onShieldFromAssetLockTransitionis intentionally pinned by a DPP test, but any caller treating the trait method as authoritative will quietly fail to bump priority. This is a DPP API ergonomics issue (silent-no-op trait method) independent of this PR — the trait surface predates Type 18.- Follow-up: Open a separate issue: 'Make StateTransitionLike::set_user_fee_increase fallible or split the trait so transitions that ignore it (Type 18) don't implement it.'
Review feedback: `lookup_asset_lock_value_credits` is unique to the shielded flow — identities and platform-addresses both take the caller-supplied `amount_duffs` and let Platform deduct its fee inside the transition. They never read the lock value at the wallet level. Type 18 is the odd one out because the Orchard `value_balance` is baked into the Halo 2 proof at build time, so the wallet has to know the shielded amount before signing. The cleaner answer is to push the responsibility back to the caller — same convention as the rest of the repo. API changes: - Wallet: `recipients: Vec<(OrchardAddress, Credits)>` (no Option; caller passes credits per recipient). - FFI: `platform_wallet_manager_shielded_fund_from_asset_lock` and `_resume` gain a `shield_amount_credits: u64` parameter; FFI passes `vec![(recipient, shield_amount_credits)]` to the wallet. - Swift: `ShieldedFundFromAssetLockRecipient.credits` becomes non-optional UInt64; caller-side preflight rejects zero amounts. Code removed: - `lookup_asset_lock_value_credits` (was reading from IS-proof output or the AssetLockManager's tracked lock list). - `shield_from_asset_lock_min_fee` (was reading the protocol fee constant). - The `CREDITS_PER_DUFF` import (no longer needed). `build_and_broadcast_shielded` retained per reviewer guidance — it's the de-duplication point between the initial submit and the IS→CL retry submit.
thepastaclaw
suggested changes
May 27, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (8d266de) is a clean refactor that fixes two prior findings (Option dropped at FFI; AddressSync error mislabels in the shielded-only helper). However, revalidation surfaced a new in-scope blocking bug introduced by this PR's shielded-funding flow: it routes asset-lock creation through AssetLockFundingType::AssetLockAddressTopUp instead of the dedicated AssetLockShieldedAddressTopUp family, so every shielded lock is derived/persisted/recovered under the platform-address bucket. Four prior findings remain STILL VALID and are carried forward; one new architectural suggestion about caller-passed shield_amount sizing is also kept.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 3 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/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:99-110: Shielded funding uses AssetLockAddressTopUp instead of the dedicated AssetLockShieldedAddressTopUp family
`shielded_fund_from_asset_lock` passes `AssetLockFundingType::AssetLockAddressTopUp` into `resolve_funding_with_is_timeout_fallback`, but the wallet has a distinct `AssetLockShieldedAddressTopUp` variant. The variant is load-bearing in three places: `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:217-250` selects `asset_lock_address_topup` (Type 14) vs `asset_lock_shielded_address_topup` (Type 18) as the BIP44 source account; `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:394-399` keys derivation re-lookup off the stored funding type; and `packages/rs-platform-wallet/src/manager/accessors.rs:744-745` maps the variants to `fundingTypeRaw` 4 vs 5 (the public persistence/UI tag). As written, every shielded fund-from-asset-lock derives funding addresses from the platform-address bucket, persists with `fundingTypeRaw == 4`, and shares state with platform-address top-ups instead of using the dedicated shielded family this PR is supposed to introduce. Resume from a stored shielded lock would re-derive against the wrong account on relaunch. Switch the call site (and the resume FFI entry point) to `AssetLockFundingType::AssetLockShieldedAddressTopUp`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:151-209: submit_with_cl_height_retry's user_fee_increase bump is a no-op for Type 18; retry hash diversification rides on Halo 2 OsRng
STILL VALID. The CL-height retry wrapper (lines 158, 193) bumps `PutSettings.user_fee_increase` between attempts so each retransmit has a fresh ST hash, bypassing Tenderdash's invalid-tx cache. For `ShieldFromAssetLockTransition` that field is not part of the transition: `set_user_fee_increase` is a no-op and `user_fee_increase()` is pinned to 0 (locked in by `test_shield_from_asset_lock_user_fee_increase_is_zero_and_setter_noop` in `packages/rs-dpp/src/state_transition/mod.rs`). Retries differ today only because `build_output_only_bundle` uses `OsRng` per call — a load-bearing implicit prover invariant, not a property this orchestration enforces. A future determinisation of the Halo 2 prover (for reproducibility/audit-replay/caching) would silently regress this into duplicate-hash submits Tenderdash drops, stranding the single-use asset-lock outpoint; each Resume with the same caller-supplied `shield_amount_credits` would reproduce the same hash. Pin the assumption with a colocated comment in `build_and_broadcast_shielded`, or thread an explicit per-retry diversifier (e.g. perturb the currently-hardcoded `[0u8; 36]` memo) so the property is enforced rather than incidental.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:139-149: Caller-passed shield_amount has no preflight against asset-lock value or Type 18 min_fee — a sizing mistake burns a single-use outpoint and ~30s of proof work
NEW in 8d266de. The refactor moved the `shield_amount = asset_lock_value − min_fee` computation out of the wallet onto the caller and the wallet now accepts the caller's value verbatim (line 148) before going into ~30s of Halo 2 proof synthesis in `build_and_broadcast_shielded`. There is no preflight that `asset_lock_value_credits ≥ shield_amount + min_required_fee`. If the caller undersizes (forgets the protocol fee or mis-computes the duffs→credits scale), Platform rejects at submit, the asset lock is single-use, and any Resume with the same `shield_amount_credits` is deterministically rejected too. Neither `rs-platform-wallet-ffi` nor `swift-sdk` exposes `ShieldFromAssetLockTransition::calculate_min_required_fee` (see `packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/state_transition_estimated_fee_validation.rs:9-22`), so Swift callers have no FFI-supported way to compute a correct amount. Two options: (a) add a pre-build preflight in `shielded_fund_from_asset_lock` that fetches the proof's lock value + protocol min_fee and rejects undersized configurations *before* the proof build, plus an FFI helper exposing the min fee; (b) revert to the wallet computing `shield_amount` (loses the multi-output forward-compat the new API was designed for). The FromWalletBalance FFI path has `amount_duffs` available — even a cheap `amount_duffs * CREDITS_PER_DUFF >= shield_amount_credits + min_fee` guard there would cover the common-case mistake.
Out-of-scope follow-up suggestions (4)
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.
- Expose ShieldFromAssetLockTransition::calculate_min_required_fee across the FFI — Related to the in-scope shield_amount sizing finding above: there is no FFI entry point that lets Swift query Type 18 (or any state-transition)
calculate_min_required_fee. Once the sizing design is settled, exposing this helper would benefit other transition types too.- Follow-up: Open a separate issue to expose state-transition min-fee queries across the FFI surface.
- block_on_worker aborts the host process on panic (pre-existing across all FFI entry points) —
block_on_workerusesrt.spawn(future).await.expect("tokio worker panicked")and no FFI entry point wraps its body incatch_unwind. Any panic during async work tears down the embedding iOS process. This PR adds two new entry points using the helper, but the panic-abort behavior is the pre-existing pattern across all 28 FFI files that callblock_on_worker— not introduced or worsened by this PR.- Follow-up: Open a separate issue/PR to add panic-to-FFI-error conversion at the runtime layer for all FFI entry points.
- Orchard value_balance overflow on caller-supplied shield_amount (DPP-layer hardening) —
shield_amount_credits: u64flows from the FFI into the DPP shielded bundle builder where it becomes the Orchardvalue_balance(an i64). A caller passing > i64::MAX would overflow inside the bundle builder. The conversion site is in DPP, not this PR's orchestration layer, and the FFI caller is trusted.- Follow-up: Open a separate issue against DPP's shielded::builder for a typed bounds check on Credits → Orchard value_balance.
- PersistentAssetLock has no shielded-recipient field family on the Swift side — The Swift persistence model notes that
fundingTypeRaw == 5will eventually need dedicatedrecipientShielded*fields when shielded funding lands. This PR does not add them. Relevant for any future resumable/UI surface but the example app does not yet wire a shielded resumable flow.- Follow-up: Open a separate issue/PR to add persisted shielded recipient/amount metadata and the resumable UI for fundingTypeRaw == 5.
… blocks
Two review-driven cleanups:
1. Add a colocated comment in shielded_fund_from_asset_lock noting
that submit_with_cl_height_retry's user_fee_increase bump is a
no-op for Type 18 (set_user_fee_increase is pinned to 0). Retries
still avoid Tenderdash's invalid-tx cache because
build_output_only_bundle draws fresh randomness from OsRng on
every call; if the prover is ever made deterministic, an
explicit diversifier would be needed.
2. Wrap MnemonicResolverCoreSigner::new calls in explicit
unsafe { ... } blocks at both FFI entry points (fresh-build and
resume), matching the Type 14 sibling in
platform_addresses/fund_from_asset_lock.rs. Compiles today
because Rust 2021 makes unsafe-fn bodies implicit unsafe
contexts, but the explicit block preserves the audit trail and
keeps the 2024-edition migration (unsafe_op_in_unsafe_fn) a no-op.
thepastaclaw
suggested changes
May 27, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (8d266de..8494ccf) is hygiene-only: explicit unsafe { } blocks around both MnemonicResolverCoreSigner::new FFI call sites and a colocated comment pinning the OsRng-keyed retry-hash diversification assumption. Both resolve prior findings. The blocking architectural bug carries forward unchanged — shielded fund-from-asset-lock still routes through AssetLockFundingType::AssetLockAddressTopUp instead of the dedicated AssetLockShieldedAddressTopUp family this PR introduces, so derivation, persistence, and recovery all happen under the platform-address bucket. Three lower-severity prior findings (caller-passed shield_amount preflight, hardcoded zero memo, IS-fallback orchestration test coverage) are also carried forward unchanged.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 2 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/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:99-115: Shielded funding routes through AssetLockAddressTopUp instead of the dedicated AssetLockShieldedAddressTopUp variant
Carried forward — STILL VALID at 8494ccfa (line 110 unchanged). `shielded_fund_from_asset_lock` passes `AssetLockFundingType::AssetLockAddressTopUp` into `resolve_funding_with_is_timeout_fallback`, but the wallet has a distinct `AssetLockShieldedAddressTopUp` variant that this PR introduces and that is load-bearing in three confirmed sites:
- `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:217-250` selects `accounts.asset_lock_address_topup` vs `accounts.asset_lock_shielded_address_topup` as the BIP44 source account.
- `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:394-399` keys derivation re-lookup off the stored funding type during resume.
- `packages/rs-platform-wallet/src/manager/accessors.rs:744-745` maps the variants to `fundingTypeRaw` 4 vs 5 — the public persistence/UI tag round-tripped through Swift's `PersistentAssetLock`.
Consequences as shipped: every shielded fund-from-asset-lock (a) derives funding addresses from and consumes gap-limit slots in the platform-address BIP44 account; (b) persists with `fundingTypeRaw == 4`, sharing recovery/UI state with Type 14 platform-address top-ups; (c) the FFI resume entry point (`shielded_send.rs:498-521`) re-enters the same orchestrator and will hand shielded-shaped intent to a wallet that recorded the original lock under the platform-address bucket, so account-key re-derivation in `recovery.rs` looks up the wrong xpub. The colocated comment at lines 99-101 even labels the call site as 'BIP44 funding family for the Core asset-lock tx', reinforcing that the wrong family is being selected. Switch the variant at line 110 to `AssetLockFundingType::AssetLockShieldedAddressTopUp`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:139-149: Caller-passed shield_amount has no preflight against asset-lock value or Type 18 min fee — sizing mistakes burn a single-use outpoint and ~30s of proof work
Carried forward — STILL VALID at 8494ccfa. The delta added a comment explaining the design choice but no guard. The caller-supplied `shield_amount` flows verbatim into `build_and_broadcast_shielded` and ~30s of Halo 2 proof synthesis. There is no preflight that `asset_lock_value_credits >= shield_amount + min_required_fee`. If the caller undersizes (forgets the protocol fee or mis-computes duffs→credits), Platform rejects at submit, the asset lock is single-use, and any Resume with the same `shield_amount_credits` is deterministically rejected too — the user has no in-app recovery path.
`ShieldFromAssetLockTransition::calculate_min_required_fee` exists in DPP (see `packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/state_transition_estimated_fee_validation.rs`) but is not exposed via `rs-platform-wallet-ffi` or `swift-sdk`, so Swift callers have no FFI-supported way to compute a correct amount.
Recommended: (a) add a preflight in `shielded_fund_from_asset_lock` that fetches the proof's lock value + protocol min_fee and rejects undersized configurations *before* the proof build, plus an FFI helper exposing the min fee; or (b) at minimum a `FromWalletBalance`-path guard `amount_duffs * CREDITS_PER_DUFF >= shield_amount_credits + min_fee` at the FFI layer to catch the common-case mistake before the proof build.
Two correctness fixes from PR review: 1. **Blocking**: pass AssetLockShieldedAddressTopUp (not AssetLockAddressTopUp) into resolve_funding_with_is_timeout_fallback. The variant is load-bearing in three places: build.rs selects the BIP44 source account (asset_lock_shielded_address_topup vs asset_lock_address_topup), sync/recovery.rs keys derivation re- lookup off the stored funding type, and manager/accessors.rs maps the variant to fundingTypeRaw 4 vs 5 (the public persistence/UI tag). As written the shielded flow was deriving from the platform-address bucket and would conflate with Type 14 in persistence/UI; resume would re-derive against the wrong account. 2. Pre-flight sizing sanity check on the FromWalletBalance path: refuse obviously-undersized configurations (amount_duffs * CREDITS_PER_DUFF < shield_amount + min_fee) BEFORE broadcasting the single-use asset-lock tx or building the ~30s Halo 2 proof. Catches the common-case caller mistake (forgot the fee, or duffs/credits scale slip) before any L1 funds are committed. Best-effort only — Platform's real fee depends on state we don't track. We just verify the lower bound. Resume path skips the check (lock value is already pinned on-chain and looking it up would re-introduce the asset-lock-value lookup we deliberately removed in 8d266de). This re-uses ShieldFromAssetLock::calculate_min_required_fee's underlying platform_version constant (required_asset_lock_duff_balance_for_processing_start_for_address_funding) — same constant Platform's StateTransitionEstimatedFeeValidation reads for Type 18.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta resolves both prior blocking/suggestion findings: shielded funding now correctly routes through AssetLockFundingType::AssetLockShieldedAddressTopUp (line 162) and a FromWalletBalance sizing preflight (lines 118-138) refuses obviously-undersized configurations before broadcast and ~30s Halo 2 proof build. Two carried-forward nitpicks remain valid (hardcoded zero Orchard memo undocumented on the public API/FFI/Swift surface; IS-timeout and IS->CL fallback orchestration branches still untested) plus one new low-impact nitpick on saturating-arithmetic shape in the new preflight. No in-scope blockers — APPROVE.
💬 3 nitpick(s)
Mirrors the platform-address funding UI shipped with #3671 for the new shielded flow (Type 18, ShieldFromAssetLockTransition): - ShieldedFundFromAssetLockController — per-slot phase machine (.idle / .inFlight / .completed / .failed). Keyed on (walletId, recipientRaw43); no platformAccountIndex because shielded recipients are external Orchard addresses, not allocated from a wallet account. Phase .completed carries no balance payload (FFI returns Void — note arrives via next sync). - ShieldedFundFromAssetLockCoordinator — singleton hub keyed by the same composite, with the same 30s retention sweep for .completed rows and indefinite retention for .failed rows until user dismissal. - PlatformWalletManager extension — objc-associated-object hook matching the sibling pattern; per-manager coordinator stays example-app-only state. - ShieldedFundFromAssetLockProgressView + Section — 5-step UI. Step 5 is meaningfully longer than the address-funding sibling because Type 18 builds a ~30s Halo 2 proof inside the FFI; the step-5 footer calls that out so users don't think the app is hung. PersistentAssetLock filter on fundingTypeRaw == 5. - ShieldedFundFromAssetLockView — Form with Core funding picker, recipient (defaults to wallet's own shielded default via shieldedDefaultAddress; overridable via 86-char raw hex input; bech32m parsing not wired yet), L1 amount, shield amount (auto-filled from L1 - 1mDASH conservative fee buffer, editable). Single-flight via the coordinator. Resume support shared with the same view in resume mode. WalletDetailView gets a new `+` affordance on the Shielded Balance row that opens the new sheet (parallel to the existing Platform Balance row `+`). BalanceCardView gains an onFundShielded callback alongside onFundPlatform. What's deliberately NOT mirrored: - Per-account recipient back-fill — shielded asset-lock rows don't carry a recipient stamp because the recipient is an external Orchard address, not allocated from a wallet account. - Pending Shielded Top Ups list view — deferred; the coordinator + per-slot controller machinery supports it but the surface isn't wired into WalletDetailView yet (the existing PendingPlatformFundFromAssetLocksList is address-funding-specific). Files use Xcode 16 filesystem-synchronized groups so no .pbxproj edits are needed.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (43845ed → 39282af) is exclusively SwiftExampleApp UI for shielded fund-from-asset-lock: new view, controller, coordinator, progress section, and a + entry from WalletDetailView. No Rust, FFI, DPP, or wallet orchestration changed in this delta. All three carried-forward priors (zero Orchard memo not surfaced on the FFI/Swift API; IS-timeout and post-submit IS→CL recovery branches still uncovered; preflight saturating-arithmetic shape) re-verify STILL VALID against HEAD and are kept as nitpicks. One new suggestion in this delta: the new shielded progress section binds to activeLocks.first while its coordinator explicitly markets concurrent per-recipient fundings per wallet, so two in-flight Type-18 shields on one wallet can render each other's status. No in-scope blockers — recommending COMMENT.
🟡 1 suggestion(s) | 💬 3 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/Views/ShieldedFundFromAssetLockProgressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockProgressView.swift:55-71: Concurrent shielded fundings on the same wallet both bind to `activeLocks.first` and can render each other's status
`ShieldedFundFromAssetLockCoordinator` explicitly markets per-recipient concurrency on its `SlotKey(walletId, recipientRaw43)` (coordinator docstring lines 10-12: "a wallet can shield to many distinct Orchard recipients concurrently"), but this progress section queries `PersistentAssetLock` only by `(walletId, fundingTypeRaw == 5)`, sorts by `updatedAt` descending, and then derives every step from `activeLocks.first`. If two Type-18 fundings are in flight on the same wallet, both controllers observe whichever lock most recently updated, so one slot's progress view can jump to the other slot's stage or failure state. The sibling `AddressFundFromAssetLockProgressSection` uses the same shape, but identity-side concurrency is rarer per wallet — the shielded coordinator advertises concurrent recipients as a first-class case. Either serialize Type-18 fundings per wallet in the coordinator, or thread the tracked outpoint (or another per-attempt key) into the query so each section follows its own row.
Two review-driven cleanups from the latest bot pass: 1. **Preflight checked arithmetic.** The sizing guard introduced in 43845ed used `saturating_mul`/`saturating_add`, which silently saturated both sides to `u64::MAX` for pathologically-large duff inputs (~1.8e16 duffs ≈ 18M DASH) and bypassed the `<` comparison. Switch to `checked_*` and reject explicitly on overflow — the guard's intent is now honest in the type system, not just in comments. 2. **Per-wallet UI serialization** in `ShieldedFundFromAssetLockCoordinator`. The coordinator docstring advertised per-recipient concurrency, but the Rust `shield_guard` mutex on `PlatformWallet` actually serializes all shield-class ops per wallet — so two concurrent fundings to different recipients on the same wallet would (a) all block on `shield_guard` Rust-side, and (b) confuse the progress view, which queries `PersistentAssetLock` only by `(walletId, fundingTypeRaw == 5)` and would show the same lock's status to both controllers. Change `startFunding` to return a new `StartFundingResult` enum: - `.started(controller)` — fresh start or same-recipient re-presentation; bind progress UI to the returned controller. - `.blockedByOtherWalletFunding(blocker)` — another shielded funding is already in flight on this wallet (different recipient). Surfaces a typed "wait" error in `ShieldedFundFromAssetLockView` pointing at the blocker's recipient. Per-recipient slot-key deduplication (the existing behaviour) is unchanged: same-recipient double-tap still returns the existing controller. The new check kicks in only on a different recipient + same-wallet collision.
User feedback: the prior UI exposed two number fields (L1 amount + shielded credits) where the address-funding and identity flows only need one. The double-field UX leaked the Type-18-specific 'shield_amount baked into Halo 2 proof at build time' constraint to the demo user. Now the form shows a single "Amount" field (DASH, = L1 lock size), matching the address-funding sibling. The view derives shieldAmountCredits internally as `lock_credits − conservative fee buffer` (1mDASH buffer in duffs, well above the protocol minimum) and passes both numbers to the Rust API. Footer line reports both values so the user sees what gets locked and what arrives in the shielded balance after the fee. Resume path uses the same derivation against the persisted lock's `amountDuffs` — no field needed at all on resume, the recipient is the only choice. Behind the scenes the Rust API is unchanged (`amountDuffs` + `shieldAmountCredits` per recipient); the previous reviewer's "caller passes shield_amount" point still holds, the derivation just lives on the Swift side now where it belongs.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3753 at head 7a9c8f0. Latest delta (39282af..7a9c8f0) FIXED two prior findings: the FromWalletBalance preflight now uses checked_mul/checked_add with explicit ShieldedBuildError on overflow (fund_from_asset_lock.rs:119-160), and Swift-side per-wallet shield serialization now matches the Rust shield_guard mutex (ShieldedFundFromAssetLockCoordinator.swift:126-149 + view:447-463). STILL VALID: hardcoded zero Orchard memo with no FFI/Swift surface (line 353), uncovered IS-timeout + post-submit IS->CL recovery branches (lines 191-211, 249-285), and two example-app UI gaps (WalletDetailView has no Pending Shielded list / resume sheet binding to ShieldedFundFromAssetLockView's resumeFromLock parameter; OptionsView's network-switch gate observes only registrationCoordinator and not the new shieldedFundFromAssetLockCoordinator.hasInFlightFundings). NEW in latest delta: the .failed-retry path explicitly skips scheduleRetentionSweep on the assumption that the original sweep is still running, but the sweep Task already returned on case .failed, so a subsequent successful retry never gets auto-purged and the slot stays .completed indefinitely. Dropped one out-of-scope codex-ffi finding (block_on_worker panic propagation lives in pre-existing runtime.rs not touched by this PR, shared by every FFI call in the crate). No blocking issues in scope; recommending COMMENT.
🟡 3 suggestion(s) | 💬 2 nitpick(s)
2 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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift:132-139: New in latest delta: .failed-retry path leaves a successfully-completed controller stranded because the original retention sweep already exited
On the `.failed` restart branch, `startFunding` reuses the existing controller, calls `existing.submit(body:)`, and explicitly skips `scheduleRetentionSweep`. The inline comment justifies that with "Sweep was already scheduled when the controller was first created" — but the sweep Task hits `case .failed: return` at line 216 as soon as the first attempt fails, so by the time the retry runs there is no live sweep. If the retry then succeeds, the controller transitions to `.completed` with no Task watching for the 30s auto-purge, and (a) it sticks in `controllers` indefinitely, (b) `activeControllers()` keeps surfacing it, and (c) any new `startFunding` to the same `(walletId, recipientRaw43)` slot returns `.started(existing)` because `case .inFlight, .completed` short-circuits at line 113 — locking that recipient out of new shielded fundings until app restart. Either spawn a fresh `scheduleRetentionSweep(key:controller:)` on the retry branch (it is idempotent once the previous one returned), or change the sweep's `.failed` arm to keep polling so a later `.completed` transition still purges.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:114-120: [Carried forward — STILL VALID] Wallet detail still exposes no Pending Shielded / Resume UI for shielded asset-lock fundings introduced by this PR
STILL VALID at 7a9c8f04. This screen wires up `PendingPlatformFundFromAssetLocksList` and the platform-address resume sheet (line 205-207), but has no equivalent for `shieldedFundFromAssetLockCoordinator`. `ShieldedFundFromAssetLockView` already accepts an optional `resumeFromLock: PersistentAssetLock` and routes through `shieldedResumeFundFromAssetLock` (View.swift:54-58, 399-417), and `ShieldedFundFromAssetLockProgressView` exists — but no call site in the example app ever constructs either with a non-nil lock. Consequence: orphaned shielded asset locks at statuses 1..3 (e.g. after sheet dismissal during the ~30s Halo 2 build, or a mid-flight crash) have no UI path to reach `shieldedResumeFundFromAssetLock`, and the PR's advertised "survive view dismissal / resume later" behaviour for the new flow is unreachable from this surface. Mirroring `PendingPlatformFundFromAssetLocksList` with a shielded sibling that filters on `fundingTypeRaw == 5` and opens `ShieldedFundFromAssetLockView(wallet:, resumeFromLock:)` would close the gap.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:141-150: [Carried forward — STILL VALID] Network-switch in-flight gate ignores the new shielded coordinator's `hasInFlightFundings`
STILL VALID at 7a9c8f04. `ShieldedFundFromAssetLockCoordinator.hasInFlightFundings` (Coordinator.swift:60-65) was added specifically so the network picker can refuse mid-flight tear-down during the long FFI build (Halo 2 proof + asset-lock broadcast + IS/CL wait), but the only gate on the picker is `NetworkPickerInFlightGate(coordinator: walletManager.registrationCoordinator, ...)` and the footer is `NetworkInFlightFooter(coordinator: walletManager.registrationCoordinator)` — both observe identity registrations only. A network switch during a shielded fund-from-asset-lock will still tear down the FFI manager underneath the in-flight call, can strand the single-use L1 outpoint mid-pipeline, and combines badly with the missing Pending Shielded surface (the orphaned lock then has no UI to resume it). Extend the gate (and the footer copy) to also block when `walletManager.shieldedFundFromAssetLockCoordinator.hasInFlightFundings` is true.
thepastaclaw
suggested changes
May 27, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (7a9c8f0..a0afac9) is a single Swift example-app commit collapsing the shielded funding form to one Amount field, but it introduces a new client-side regression: a 1,000,000-duff (0.01 DASH) fee buffer that is 20x the actual versioned protocol minimum (50,000 duffs), which makes the default amount and any persisted lock below 1M duffs non-submittable and contradicts the section footer. Five prior findings are STILL VALID and carried forward (retention-sweep retry bug, missing Pending Shielded UI, network-switch gate missing the shielded coordinator, hardcoded zero Orchard memo, uncovered IS/CL recovery branches).
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)
2 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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:505-540: New in latest delta: 1,000,000-duff fee buffer is 20x the protocol minimum, makes the default amount non-submittable and blocks resume of persisted locks below 1M duffs
`computedShieldAmount()` (lines 505-519) always subtracts `minFeeBufferDuffs = 1_000_000` duffs (line 527), and the comment at lines 521-526 claims this mirrors `required_asset_lock_duff_balance_for_processing_start_for_address_funding`. That protocol constant is 50,000 duffs in `packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/v{1,2,3}.rs:22`, and the wallet preflight at `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:134-158` enforces that floor. Three concrete consequences:
1. Default state is broken: `amountDash = "0.001"` (line 75) = 100,000 duffs; `lockCredits` (1e8) is not greater than `feeBufferCredits` (1e9), so `computedShieldAmount()` returns nil, `canSubmit` is false, and the user opens the sheet to a permanently-disabled Submit with no inline error.
2. Resume is silently bricked for any persisted lock between 50,001 and 1,000,000 duffs: the same guard at line 517 rejects them client-side even though the Rust path would accept them. Since `resumeFromLock.amountDuffs` is fixed and not user-editable in the resume sheet, the orphaned lock becomes UI-unrecoverable for the lifetime of this build.
3. The amountSection footer (lines 232-239) still advertises `Minimum lock: \(formatDuffs(Self.minDuffs))` = 100,000 duffs while the effective floor is now ~1,000,001 duffs, contradicting the actual gating with no visible explanation.
Fix by either dropping `minFeeBufferDuffs` to the versioned protocol value (or reading it via FFI) and bumping `minDuffs`/default accordingly, or by surfacing the real effective minimum in the footer and bumping the default amount so the first-paint form is submittable.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift:132-139: [Carried forward — STILL VALID] .failed-retry path skips scheduleRetentionSweep but the original sweep already exited on .failed, stranding successful retries in .completed forever
Re-verified at a0afac95. On the `.idle/.failed` restart branch (line 119-139) `startFunding` reuses the existing controller, calls `existing.submit(body:)`, and explicitly skips `scheduleRetentionSweep` with the comment 'Sweep was already scheduled when the controller was first created.' But `scheduleRetentionSweep` returns immediately on `case .failed` at line 216, so by the time the retry runs there is no live sweep watching the controller. If the retry then transitions to `.completed`, no Task auto-purges it: (a) it stays in `controllers` indefinitely, (b) `activeControllers()` keeps surfacing it, and (c) any new `startFunding` on the same `(walletId, recipientRaw43)` slot short-circuits at line 113 (`case .inFlight, .completed: return .started(existing)`), locking that recipient slot out of new shielded fundings until app restart. Either spawn a fresh `scheduleRetentionSweep(key:controller:)` on the retry branch (it's idempotent once the prior task returned), or change the sweep's `.failed` arm to keep polling so a later `.completed` transition still purges.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:114-120: [Carried forward — STILL VALID] Wallet detail still exposes no Pending Shielded / Resume UI for the shielded asset-lock fundings introduced by this PR
Re-verified at a0afac95. `WalletDetailView` wires up `PendingPlatformFundFromAssetLocksList` and the platform-address resume sheet but has no equivalent for `shieldedFundFromAssetLockCoordinator`. `ShieldedFundFromAssetLockView` accepts `resumeFromLock: PersistentAssetLock?` and routes through `shieldedResumeFundFromAssetLock`, and `ShieldedFundFromAssetLockProgressView` exists — but no call site in the example app ever constructs them with a non-nil lock. Consequence: orphaned shielded asset locks at statuses 1..3 (e.g. after sheet dismissal during the ~30s Halo 2 build, or a mid-flight crash) have no UI path to reach `shieldedResumeFundFromAssetLock`, making the PR's advertised resume behaviour unreachable from this surface. Mirroring `PendingPlatformFundFromAssetLocksList` with a shielded sibling that filters on `fundingTypeRaw == 5` and opens `ShieldedFundFromAssetLockView(wallet:, resumeFromLock:)` would close the gap.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:141-150: [Carried forward — STILL VALID] Network-switch in-flight gate ignores the new shielded coordinator's hasInFlightFundings
Re-verified at a0afac95. `ShieldedFundFromAssetLockCoordinator.hasInFlightFundings` was added specifically so the network picker can refuse mid-flight tear-down during the long FFI build (Halo 2 proof + asset-lock broadcast + IS/CL wait), but the only gate on the picker is `NetworkPickerInFlightGate(coordinator: walletManager.registrationCoordinator, ...)` (line 143) and the footer is `NetworkInFlightFooter(coordinator: walletManager.registrationCoordinator)` (line 149) — both observe identity registrations only. A network switch during a shielded fund-from-asset-lock will tear down the FFI manager underneath the in-flight call, can strand the single-use L1 outpoint mid-pipeline, and combines badly with the missing Pending Shielded surface (the orphaned lock then has no UI to resume it). Extend the gate and the footer copy to also block when `walletManager.shieldedFundFromAssetLockCoordinator.hasInFlightFundings` is true.
…I shape)
User feedback: the prior "caller passes shield_amount" API forced
Swift to duplicate fee math the wallet already does. Worse, the
example app had to expose two numeric fields when every sibling
flow (identities, platform addresses) takes just one — confusing
demo UX.
Reverting the API to wallet-computes-it. The original first-
reviewer feedback ("be consistent with identities/platform-
addresses, caller passes amount_duffs") rested on a faulty
analogy: identities and platform-addresses don't compute fees at
the wallet level because Platform deducts them INSIDE the
transition. Shielded Type 18 can't do that — the Orchard
value_balance is baked into the Halo 2 proof at build time. The
wallet has to compute the amount before signing, so it might as
well do so consistently across all callers instead of pushing the
math to every FFI consumer.
Changes:
rs-platform-wallet:
* `shielded_fund_from_asset_lock` recipient API back to
`Vec<(OrchardAddress, Option<Credits>)>`. Preflight enforces
exactly one recipient with `None` credits (= "remainder"
semantics, mirroring Type 14). Explicit `Some(_)` still
rejected today; will be honored when DPP grows multi-output
Orchard bundles for Type 18.
* Reintroduced `lookup_asset_lock_value_credits` (reads from IS
proof's TxOut or the asset-lock manager's tracked row for
CL-only paths) and `shield_from_asset_lock_min_fee` (reads
the protocol constant). Same shape as the original first cut,
but now the answer to the reviewer's "why is shielded the
odd one out" is documented inline: Type 18 has no protocol-
level fee strategy, so the wallet does the math because
no one else can.
* Dropped the FromWalletBalance sizing preflight — wallet now
knows the value internally; no risk of caller mis-sizing.
rs-platform-wallet-ffi:
* `platform_wallet_manager_shielded_fund_from_asset_lock` and
its resume sibling lose the `shield_amount_credits`
parameter. FFI now passes `vec![(addr, None)]` to the wallet.
swift-sdk:
* `ShieldedFundFromAssetLockRecipient.credits` becomes
`UInt64?` (matches Rust); preflight enforces `nil` today
with a TODO comment about multi-output future.
swift-sdk/SwiftExampleApp:
* Single "Amount" field in the form, same shape as
`FundFromAssetLockPlatformAddressView`.
* Dropped `computedShieldAmount` and the Swift-side
`creditsPerDuff`/`minFeeBufferDuffs` constants — no
duplicate fee math.
* Controller drops `shieldAmountCredits` (the Swift side never
knows the precise amount; the terminal banner just says
"Shielded" and points the user at the next sync pass).
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head 2cf5e88. Status of the six prior findings: #1 (Swift UI 1M-duff buffer) FIXED — the latest delta deletes computedShieldAmount()/minFeeBufferDuffs and moves the math into Rust using required_asset_lock_duff_balance_for_processing_start_for_address_funding; #2 (.failed-retry sweep), #3 (no Pending Shielded UI), #4 (network-switch gate ignores shielded coordinator), #5 (zero-memo locked into new FFI), #6 (orchestration test coverage) all STILL VALID — none of those code paths changed in the latest delta. One new latest-delta finding: removing the pre-broadcast sizing guard on FromWalletBalance regresses fail-fast behavior — the wallet now broadcasts a single-use L1 asset-lock before discovering an undersized amount_duffs via checked_sub. Today's Swift UI enforces minDuffs = 100_000 (> the 50_000-duff protocol min fee), so the example app cannot trip it; the FFI itself has no such guard. Two FFI findings dropped: the extern "C" arity change is for symbols this PR is introducing (no pre-existing downstream consumers), and the block_on_worker panic-abort behavior is pre-existing helper code, not caused by this PR.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
2 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 `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:96-184: New in latest delta: pre-broadcast sizing guard on `FromWalletBalance` is gone — undersized `amount_duffs` now burns the L1 asset-lock before the wallet detects the shortfall
Three independent reviewers flagged this. The prior revision performed a `lock_credits >= shield_amount + min_required_fee` checked-math guard on the `AssetLockFunding::FromWalletBalance` arm *before* `resolve_funding_with_is_timeout_fallback` broadcast the asset-lock tx — the in-code comment was explicit that the point was to 'refuse obviously-undersized configurations BEFORE we broadcast the asset-lock tx (single-use L1 funds) or spend ~30s building a Halo 2 proof Platform would reject'. The latest delta moves all sizing math into Step 3 (lines 169-184): `lookup_asset_lock_value_credits(...).checked_sub(min_fee_credits)` runs only after Step 2 has built/broadcast the asset-lock tx and waited for an IS/CL proof. Any FFI caller passing `amount_duffs < required_asset_lock_duff_balance_for_processing_start_for_address_funding` (currently 50_000 duffs across PV1/2/3) will now (a) broadcast a real L1 asset-lock, (b) wait through the IS path, then (c) return `ShieldedBuildError("asset lock value ... below the minimum required fee")` — with the L1 outpoint stranded. Resuming on the orphaned lock deterministically hits the same `checked_sub` underflow, so the funds cannot be recovered through this code path. The example app's `minDuffs = 100_000` floor (`ShieldedFundFromAssetLockView.swift:82`) prevents the UI from triggering this today, but `platform_wallet_manager_shielded_fund_from_asset_lock` is the public FFI surface and the wallet's own Rust API has no equivalent guard. Cheap fix: re-introduce a `match` arm on `funding` in Step 1 that does `if let AssetLockFunding::FromWalletBalance { amount_duffs, .. } = &funding { if amount_duffs.checked_mul(CREDITS_PER_DUFF) <= Some(self.shield_from_asset_lock_min_fee()?) { return Err(...) } }` before `resolve_funding_with_is_timeout_fallback`. The Step 3 check should stay as the safety net for `FromExistingAssetLock` resume paths where the lock is already on-chain.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:114-210: Carried forward — STILL VALID at 2cf5e889: the shielded resume API added by this PR has no UI entry point in the example app
`WalletDetailView` renders `PendingPlatformFundFromAssetLocksList` (line 114) and wires a resume sheet at lines 205-207 for `FundFromAssetLockPlatformAddressView` only. The `.sheet(isPresented: $showShieldFromAssetLock)` at lines 208-210 always constructs `ShieldedFundFromAssetLockView(wallet:)` with no `resumeFromLock` argument. The PR adds `ShieldedFundFromAssetLockView(wallet:, resumeFromLock:)`, `shieldedResumeFundFromAssetLock`, and `ShieldedFundFromAssetLockProgressView`, but no call site in the example app ever constructs the view with a non-nil lock. Consequence: orphaned shielded asset locks at intermediate statuses (e.g. sheet dismissed during the ~30s Halo 2 build, or a mid-flight crash) have no UI path to reach the new resume API — the PR's advertised resume behavior is unreachable from this surface. Mirror `PendingPlatformFundFromAssetLocksList` with a shielded sibling that filters on `fundingTypeRaw == 5` and opens `ShieldedFundFromAssetLockView(wallet:, resumeFromLock:)`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:141-150: Carried forward — STILL VALID at 2cf5e889: network-switch in-flight gate ignores the new shielded coordinator
`ShieldedFundFromAssetLockCoordinator.hasInFlightFundings` was introduced in this PR specifically so the network picker could refuse mid-flight tear-down during the long Type 18 build (Halo 2 proof + asset-lock broadcast + IS/CL wait). But `NetworkPickerInFlightGate(coordinator: walletManager.registrationCoordinator, ...)` (lines 142-145) and `NetworkInFlightFooter(coordinator: walletManager.registrationCoordinator)` (lines 148-150) both observe identity-registrations only. A network switch during a shielded fund-from-asset-lock will still tear down the FFI manager underneath the in-flight call, can strand the single-use L1 outpoint mid-pipeline, and combines badly with the missing Pending Shielded surface (the orphaned lock then has no UI to resume it). Extend the gate (and the footer copy) to also block when `walletManager.shieldedFundFromAssetLockCoordinator.hasInFlightFundings` is true.
Two review-driven fixes: 1. **Pre-broadcast sizing guard.** The revert in 2cf5e88 removed the FromWalletBalance sizing check before resolve_funding_with_is_timeout_fallback. An undersized amount_duffs would now (a) broadcast a real L1 asset-lock, (b) wait through the IS path, then (c) underflow in Step 3's checked_sub — with the L1 outpoint stranded and a Resume on the orphaned lock deterministically hitting the same underflow. Restore the guard in Step 1: if amount_duffs * CREDITS_PER_DUFF <= protocol_min_fee, refuse to broadcast. The Step 3 check stays as the safety net for FromExistingAssetLock resumes (lock is already on-chain; sizing was decided by a prior caller). 2. **Retention sweep on .failed retry.** When a .failed controller is restarted via startFunding, the original scheduleRetentionSweep Task had already exited on the .failed arm. If the retry succeeded, the controller transitioned to .completed with no sweep watching, sticking in controllers indefinitely and locking that recipient out of new shielded fundings until app restart. Spawn a fresh sweep on the retry branch. Idempotent because the previous sweep already returned.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3753 at head 6331681. Latest delta fixes two prior findings: pre-broadcast sizing guard restored on FromWalletBalance (fund_from_asset_lock.rs:101-132) and retention sweep re-scheduled on .failed→retry (ShieldedFundFromAssetLockCoordinator.swift:131-143). Both are correct and well-scoped — no new in-scope defects in the latest delta. Four prior findings remain STILL VALID and are carried forward: example-app shielded resume UI gap, network-switch gate ignoring the shielded coordinator, hardcoded zero-memo on the new FFI ABI, and orchestration-layer test coverage gap. The codex-security blocking finding about FromExistingAssetLock 'authorization bypass' is dropped as a false positive — the threat model assumes privilege separation between code paths inside a single wallet that does not exist.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:114-210: [Carried-forward, STILL VALID] Example app has no Pending Shielded / Resume surface for Type 18 asset-lock fundings
Re-verified at head 6331681c. `WalletDetailView` still renders only `PendingPlatformFundFromAssetLocksList` (line 114) and wires the platform-address resume sheet at line 206 as `FundFromAssetLockPlatformAddressView(wallet:, resumeFromLock: lock)`. The shielded sheet at line 209 always constructs `ShieldedFundFromAssetLockView(wallet: wallet)` with no `resumeFromLock:` argument, and no other call site in the workspace passes a non-nil `resumeFromLock:`. This PR adds the shielded resume API (`ShieldedFundFromAssetLockView.resumeFromLock`, `shieldedResumeFundFromAssetLock`, `ShieldedFundFromAssetLockProgressView`) but offers no UI path to reach it: orphaned shielded asset locks left at `Broadcast` / `InstantSendLocked` / `ChainLocked` after sheet dismissal during the ~30s Halo 2 build, mid-flight crashes, or IS→CL upgrades cannot be resumed from the UI. The new pre-broadcast sizing guard narrows but doesn't eliminate the orphan-lock surface. Mirror `PendingPlatformFundFromAssetLocksList` with a shielded sibling that filters on `fundingTypeRaw == 5` and opens `ShieldedFundFromAssetLockView(wallet:, resumeFromLock:)`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift:141-150: [Carried-forward, STILL VALID] Network-switch in-flight gate ignores the new shielded coordinator's hasInFlightFundings
Re-verified at head 6331681c. `NetworkPickerInFlightGate(coordinator: walletManager.registrationCoordinator, …)` at lines 142-145 and `NetworkInFlightFooter(coordinator: walletManager.registrationCoordinator)` at lines 148-150 still observe only identity registrations; the file contains no reference to `shieldedFundFromAssetLockCoordinator`. `ShieldedFundFromAssetLockCoordinator.hasInFlightFundings` was added by this PR specifically so the network picker could refuse mid-flight tear-down during the long Type 18 build (Halo 2 proof + asset-lock broadcast + IS/CL wait). A network switch during a shielded fund-from-asset-lock tears down the FFI manager underneath the in-flight call, can strand the single-use L1 outpoint mid-pipeline, and — combined with the missing Pending Shielded surface above — leaves the user without a UI path to recover. Extend `NetworkPickerInFlightGate` and `NetworkInFlightFooter` to also observe `walletManager.shieldedFundFromAssetLockCoordinator.hasInFlightFundings`.
6 tasks
shumkov
added a commit
that referenced
this pull request
May 28, 2026
Two cleanups on top of the prior `append_many_raw` migration: **1. Removed hardcoded test wallets.** Deleted `shielded_test_wallets.rs` entirely (SEED_A/SEED_B and TestWallet), along with `owned_count`/`owned_value` fields on ShieldedSeedConfig, the OwnedLayout struct, generate_owned_note, and every test that relied on test-wallet decryption (8 in `mod tests` + the try_decrypt helper). The seeder is now a pure stress test: N random filler notes with no special-position owned tier. Wallet-balance flows in tests should go through real shielded-fund transitions post-genesis (supported via PR #3753 / shield-from-asset-lock), not via the seeder. **2. Batched seed loop.** The single up-front allocation of all `total_notes` followed by a single `append_many_raw` call is replaced with a loop: - Generate 10_000 filler notes from the seeded RNG - `ct.append_many_raw(iter)` - `ct.save()` to persist the Sinsemilla frontier - `ct.commit_mmr()` to flush the MMR overlay (required between repeated `append_many_raw` calls — without it, the next batch hits "MMR get_root failed: Inconsistent store") - Log running anchor + bulk-state root for observability Memory: peak allocation goes from O(total_notes) × ~280 bytes (216 ciphertext + 32 cmx + 32 rho + Vec overhead) to O(10k) × 280 bytes ≈ 2.7 MB regardless of N. For N=1M that's ~280 MB → ~3 MB. Determinism: `batched_generator_matches_single_call` test pins that chained `generate_filler_batch` calls produce byte-identical output to a single `generate_notes(cfg)` call sized to total_notes, so the seeded GroveDB state is invariant to BATCH_SIZE. Observability: each batch logs `sinsemilla_root` + `bulk_state_root` in hex. Comparing roots between bake runs (or between batched and non-batched paths) immediately shows where divergence happens. Verified: - `cargo check -p drive-abci` clean with and without `--cfg create_sdk_test_data` - 12 tests pass (4 unit-tests + 6 integration + 2 new batched tests): generate_notes_*, batched_generator_matches_single_call, generated_cmx_values_are_unique, empty_config_*, seeded_pool_count_*, seeded_anchor_*, seeding_with_same_config_*, different_rng_seeds_*, seeding_zero_notes_*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ShieldFromAssetLock): build → IS/CL → submit → consume, with the asset-lock-proof signature routed throughkey_wallet::signer::Signerso the raw key never crosses the FFI boundary.Vec<(OrchardAddress, Option<Credits>)>in Rust,[ShieldedFundFromAssetLockRecipient]in Swift) so the call sites don't have to change when DPP grows multi-output Orchard bundles for Type 18.shieldedFundFromAssetLock+shieldedResumeFundFromAssetLock) onPlatformWalletManager, paired with two FFI bindings.What's in here
ShieldFromAssetLockTransitionMethodsV0::try_from_asset_lock_with_bundle_and_signer(async) + outer-enum dispatcher; newbuild_shield_from_asset_lock_transition_with_signerbuilder. Gatedstate-transition-signing + core_key_wallet.rs-platform-walletwallet/shielded/fund_from_asset_lock.rswithPlatformWallet::shielded_fund_from_asset_lock. Removed unusedoperations.rs::shield_from_asset_lock(raw-key stub, no callers). AddedPlatformWallet::network().rs-platform-wallet-ffiplatform_wallet_manager_shielded_fund_from_asset_lock+_resume_fund_from_asset_lock. Asset-lock signature viaMnemonicResolverHandle.swift-sdkPlatformWalletManagerShieldedFunding.swift— preflighted,Task.detached(.userInitiated),withExtendedLifetimearound the resolver.Pipeline parity with #3671
len == 1recipient guard withTODO(multi-output)comment pointing at the sharedbuild_output_only_bundle(also used by Type 15 Shield).resolve_funding_with_is_timeout_fallback.submit_with_cl_height_retry; IS→CL fallback onis_instant_lock_proof_invalid.consume_asset_lockafter Platform accepts. No proof-attested balance changeset to write (shielded notes arrive via the next sync, unlike address-funding'sAddressInfos).Shield amount math (single-recipient case)
shield_amount = asset_lock_value_credits − min_required_feeasset_lock_value_credits = TxOut.value * CREDITS_PER_DUFF(read from the IS proof's output when available, or looked up via theAssetLockManager's tracked locks when the IS→CL fallback path produced a CL-only proof).min_required_fee=required_asset_lock_duff_balance_for_processing_start_for_address_funding * CREDITS_PER_DUFF(same constant Type 14 reads).Test plan
wallet::shielded::fund_from_asset_lock::tests(4 preflight cases) + existingshield_from_asset_lockDPP tests (27 ✓).fmt --check+clippy -D warningsclean acrossplatform-wallet,platform-wallet-ffi,dpp.Architecture notes
PlatformWalletManager(process-global coordinator scope), not a per-wallet object — the existingshieldedTransfer/shieldedShield/shieldedUnshield/shieldedWithdrawmethods are all there, and these new funding methods follow that convention. (An earlier scoping question proposed aManagedShieldedWallet, which would've broken the convention; the existing architecture doesn't have a per-wallet shielded wrapper.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests