fix(swift-sdk): show shielded funding steps and real fee estimate#3845
Conversation
Two SwiftExampleApp fixes from a shielded end-to-end run on devnet: - The registration progress sheet always rendered the 5 asset-lock steps; the Type-20 IdentityCreateFromShieldedPool path has no asset lock, so it sat on "Building asset-lock transaction" for the whole Halo 2 proof. The controller now carries a FundingKind and the progress section renders a 4-step shielded set (select notes -> Halo 2 proof -> broadcast -> register) driven by phase + elapsed time since submit. - The Send screen's shielded fee estimates were hardcoded placeholders ~500x below the consensus-pinned fees. Added a marshal-only FFI binding (platform_wallet_shielded_estimate_fee) over rs-dpp's compute_minimum_shielded_fee / compute_shielded_unshield_fee / compute_shielded_withdrawal_fee, a thin Swift wrapper, and wired SendViewModel to it (2 Orchard actions = single-note spend with change). A unit test pins the 2-action values to the observed on-chain fees. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds two independent features: (1) shielded-transaction fee estimation via a new Rust FFI binding, Swift wrapper enum, and SendViewModel integration; and (2) identity-registration UI support for two funding paths (asset-lock and shielded-pool) with funding-aware progress step rendering and timing logic. ChangesShielded Fee Estimation
Funding-Kind Registration UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 1cb2e85) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3845 +/- ##
============================================
+ Coverage 87.04% 87.12% +0.08%
============================================
Files 2677 2641 -36
Lines 329918 327535 -2383
============================================
- Hits 287182 285370 -1812
+ Misses 42736 42165 -571
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Focused, in-scope feature PR: a marshal-only FFI fee estimator and funding-source-aware shielded registration steps. No blocking issues. Worth surfacing: the new Swift wrapper traps on negative inputs before the throws path can fire, a retry of a failed registration carries the original funding kind even if the caller passes a new one, the FFI hard-pins to PlatformVersion::latest() while builders thread sdk.version(), and the new estimator test pins exact credit values that will need rebaselining on the next fee-version bump.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:444-456: Negative numActions traps before throws path fires
`estimateShieldedFee` is a public `throws` API but accepts `numActions: Int` and feeds it straight into `UInt(numActions)`. `UInt.init(Int)` traps on negative values, so an accidental caller passing a negative number crashes the host app at the language boundary instead of surfacing an `invalidParameter` error through the documented throwing channel. The Rust side already returns a clean error on misuse, but Swift never gets there. Guard before the conversion (or change the parameter to `UInt`) so invalid input is reported through the declared error path.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift:97-113: Retry path keeps the old fundingKind on the existing controller
`startRegistration` now takes a `fundingKind` parameter, but when an existing controller is in `.idle` or `.failed` the code reuses it and submits the new body without updating `fundingKind`. `IdentityRegistrationController.fundingKind` is `let`, so once a controller is created it's frozen. A user who fails registration with one funding source and retries the same slot with the other source will see the wrong step set rendered (asset-lock steps for a shielded retry or vice versa) — the exact mismatch this PR is fixing on the fresh-controller path. Either replace the controller on the restart branch, or make `fundingKind` `private(set) var` and update it before `submit`.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:184-213: Estimator pins PlatformVersion::latest() instead of the SDK's active version
`platform_wallet_shielded_estimate_fee` resolves `compute_*_shielded_fee` against `PlatformVersion::latest()` and the doc comment claims this matches what the builders pin via `sdk.version()` — so the estimate "can't drift from the fee the builder carves". That equivalence only holds when the host's compiled-in `latest()` matches the network's active protocol version. Mid-upgrade or on a devnet pinned to an older version the displayed estimate can diverge from the consensus-pinned fee actually carved. Either accept a `protocol_version` parameter (so Swift can pass `sdk.version()`), or soften the docstring to acknowledge the drift window. The UI consequence is bounded (a labeled `~estimate` plus a placeholder fallback), so this is a hardening suggestion, not a blocker.
| public static func estimateShieldedFee( | ||
| kind: ShieldedFeeKind, | ||
| numActions: Int = 2 | ||
| ) throws -> UInt64 { | ||
| var fee: UInt64 = 0 | ||
| // `num_actions` is `usize` on the Rust side → imported as `UInt`. | ||
| try platform_wallet_shielded_estimate_fee( | ||
| kind.rawValue, | ||
| UInt(numActions), | ||
| &fee | ||
| ).check() | ||
| return fee | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Negative numActions traps before throws path fires
estimateShieldedFee is a public throws API but accepts numActions: Int and feeds it straight into UInt(numActions). UInt.init(Int) traps on negative values, so an accidental caller passing a negative number crashes the host app at the language boundary instead of surfacing an invalidParameter error through the documented throwing channel. The Rust side already returns a clean error on misuse, but Swift never gets there. Guard before the conversion (or change the parameter to UInt) so invalid input is reported through the declared error path.
| public static func estimateShieldedFee( | |
| kind: ShieldedFeeKind, | |
| numActions: Int = 2 | |
| ) throws -> UInt64 { | |
| var fee: UInt64 = 0 | |
| // `num_actions` is `usize` on the Rust side → imported as `UInt`. | |
| try platform_wallet_shielded_estimate_fee( | |
| kind.rawValue, | |
| UInt(numActions), | |
| &fee | |
| ).check() | |
| return fee | |
| } | |
| public static func estimateShieldedFee( | |
| kind: ShieldedFeeKind, | |
| numActions: Int = 2 | |
| ) throws -> UInt64 { | |
| guard numActions >= 0 else { | |
| throw PlatformWalletError.invalidParameter("numActions must be non-negative") | |
| } | |
| var fee: UInt64 = 0 | |
| // `num_actions` is `usize` on the Rust side → imported as `UInt`. | |
| try platform_wallet_shielded_estimate_fee( | |
| kind.rawValue, | |
| UInt(numActions), | |
| &fee | |
| ).check() | |
| return fee | |
| } |
source: ['codex', 'claude']
| pub unsafe extern "C" fn platform_wallet_shielded_estimate_fee( | ||
| kind: u8, | ||
| num_actions: usize, | ||
| out_fee: *mut u64, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_fee); | ||
|
|
||
| let platform_version = dpp::version::PlatformVersion::latest(); | ||
| let fee = match kind { | ||
| 0 => compute_minimum_shielded_fee(num_actions, platform_version), | ||
| 1 => compute_shielded_unshield_fee(num_actions, platform_version), | ||
| 2 => compute_shielded_withdrawal_fee(num_actions, platform_version), | ||
| other => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| format!("unknown shielded fee kind {other} (expected 0/1/2)"), | ||
| ); | ||
| } | ||
| }; | ||
| match fee { | ||
| Ok(credits) => { | ||
| *out_fee = credits; | ||
| PlatformWalletFFIResult::ok() | ||
| } | ||
| Err(e) => PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorArithmeticOverflow, | ||
| format!("shielded fee estimation failed: {e}"), | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Estimator pins PlatformVersion::latest() instead of the SDK's active version
platform_wallet_shielded_estimate_fee resolves compute_*_shielded_fee against PlatformVersion::latest() and the doc comment claims this matches what the builders pin via sdk.version() — so the estimate "can't drift from the fee the builder carves". That equivalence only holds when the host's compiled-in latest() matches the network's active protocol version. Mid-upgrade or on a devnet pinned to an older version the displayed estimate can diverge from the consensus-pinned fee actually carved. Either accept a protocol_version parameter (so Swift can pass sdk.version()), or soften the docstring to acknowledge the drift window. The UI consequence is bounded (a labeled ~estimate plus a placeholder fallback), so this is a hardening suggestion, not a blocker.
source: ['claude']
| /// Pin the fee estimator to the on-chain ground-truth values observed at the current platform | ||
| /// version with 2 actions (single-note spend + change). These are the exact credits the | ||
| /// builder carves and the consensus gate validates, so the host's "Estimated Fee" must match. | ||
| #[test] | ||
| fn estimate_fee_matches_observed_onchain_values_for_2_actions() { | ||
| unsafe { | ||
| let estimate = |kind: u8| { | ||
| let mut fee: u64 = 0; | ||
| let result = platform_wallet_shielded_estimate_fee(kind, 2, &mut fee); | ||
| assert_eq!( | ||
| result.code, | ||
| PlatformWalletFFIResultCode::Success, | ||
| "kind {kind} must succeed" | ||
| ); | ||
| fee | ||
| }; | ||
| // kind 0 — ShieldedTransfer / Shield base. | ||
| assert_eq!( | ||
| estimate(0), | ||
| 162_851_200, | ||
| "shielded transfer fee (2 actions)" | ||
| ); | ||
| // kind 1 — Unshield. | ||
| assert_eq!(estimate(1), 168_934_000, "unshield fee (2 actions)"); | ||
| // kind 2 — ShieldedWithdrawal. | ||
| assert_eq!( | ||
| estimate(2), | ||
| 275_191_200, | ||
| "shielded withdrawal fee (2 actions)" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Hardcoded ground-truth fees will need rebaselining on each fee-version bump
estimate_fee_matches_observed_onchain_values_for_2_actions pins kind 0/1/2 at 2 actions to exact credit values (162_851_200 / 168_934_000 / 275_191_200). Because the FFI itself uses PlatformVersion::latest(), any future protocol version that adjusts the shielded fee constants will break this test even though the FFI plumbing is still correct. Either source the expected values from compute_minimum_shielded_fee(2, PlatformVersion::latest()) (validates plumbing, not constants), or add a comment that this test is intentionally version-pinned for drift detection so future maintainers know to rebaseline rather than tear out.
source: ['claude']
2 participants
Issue being fixed or feature implemented
Two SwiftExampleApp issues found during a full shielded end-to-end test run on devnet:
IdentityCreateFromShieldedPool), the progress sheet still rendered the asset-lock pipeline steps ("Building asset-lock transaction", InstantSend/ChainLock waits). That path has no asset lock at all, so the view sat on the first step for the entire 30–90 s Halo 2 proof.What was done?
Fix 1 — funding-source-aware registration steps
IdentityRegistrationControllergains aFundingKind(assetLock/shieldedPool, defaultassetLock), threaded throughRegistrationCoordinator.startRegistration; only the shielded call site inCreateIdentityViewpasses.shieldedPool.RegistrationProgressSectionrenders a 4-step shielded set (Selecting shielded notes → Generating Halo 2 proof → Broadcasting transition → Registering identity) when funding is shielded. There is no per-stage signal from Rust during the opaque FFI call, so transitions are driven bycontroller.phase+ elapsed time sincelastSubmittedAt(same heuristic precedent as the existing broadcast sub-steps). Broadcast/register stay pending rather than fake-checking until the call completes. The asset-lock path is unchanged.Fix 2 — real shielded fee model via FFI
platform_wallet_shielded_estimate_fee(kind, num_actions, out_fee)inrs-platform-wallet-ffi, dispatching to rs-dpp'scompute_minimum_shielded_fee/compute_shielded_unshield_fee/compute_shielded_withdrawal_feeatPlatformVersion::latest()(the same version the builders pin). No fee logic re-derived in Swift, per the Swift SDK architectural rules.PlatformWalletManager.estimateShieldedFee(kind:numActions:).SendViewModelnow resolves the shielded flows' estimates through the FFI withnumActions: 2(single-note spend with change — the exact count isn't known until the builder selects notes). The Shield flow uses the transfer base fee since the Type-15 builder reserves exactlycompute_minimum_shielded_fee(2)as its structure-check minimum.coreToCore/platformToPlatformuntouched.How Has This Been Tested?
cargo test -p platform-wallet-ffi --features shielded estimate_fee— 2 passed), plus an unknown-kind rejection test.cargo fmt --all,cargo check -p platform-wallet-ffi --features shielded,cargo check -p platform-wallet --features shielded— all clean../build_ios.shxcframework rebuild — BUILD SUCCEEDED.xcodebuild ... clean buildagainst the rebuilt framework — exit 0.Breaking Changes
None — UI labels and a new additive FFI symbol; not consensus-breaking.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes