feat: expose Platform-to-Shielded capacity preflight - #4360
Open
llbartekll wants to merge 2 commits into
Open
Conversation
llbartekll
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
shumkov
as code owners
August 10, 2026 18:14
Collaborator
|
🕓 Ready for review — next in queue (commit d958b28) |
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe wallet now plans shield capacity from cached payment-account state, exposes that plan through Rust FFI and Swift, and maps shield-specific insufficient balance and arithmetic overflow errors to dedicated result codes. ChangesShielded preflight flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftClient
participant PlatformWalletManagerShieldedSync
participant ShieldedShieldPreflightFFI
participant PlatformWallet
SwiftClient->>PlatformWalletManagerShieldedSync: shieldedShieldPreflight(walletId, paymentAccount)
PlatformWalletManagerShieldedSync->>ShieldedShieldPreflightFFI: invoke preflight
ShieldedShieldPreflightFFI->>PlatformWallet: resolve wallet and calculate cached capacity
PlatformWallet-->>ShieldedShieldPreflightFFI: capacity fields and reason
ShieldedShieldPreflightFFI-->>PlatformWalletManagerShieldedSync: FFI preflight result
PlatformWalletManagerShieldedSync-->>SwiftClient: ShieldedShieldPreflight or typed error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/shielded_send.rs (1)
599-610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the operation string gate with a shared constant.
The dedicated code depends on
operation == "shielded shield". The only producer is the literal passed at the shield call site. If that literal is edited (for example to "shield" or "Platform → shielded"), this arm stops matching and the error silently regresses toErrorShieldedInsufficientBalance→ErrorWalletOperation. The compiler reports nothing.Bind the label once and reuse it at both places.
♻️ Proposed refactor
+/// Operation label for the Platform-to-shielded entry point. The +/// `ShieldedInsufficientBalance` arm below keys on this exact value. +const SHIELD_OPERATION: &str = "shielded shield"; + fn map_spend_result( result: Result<(), PlatformWalletError>, operation: &str, ) -> PlatformWalletFFIResult {Err(e @ PlatformWalletError::ShieldedInsufficientBalance { .. }) - if operation == "shielded shield" => + if operation == SHIELD_OPERATION =>At the shield entry point (outside this range):
map_spend_result(result, SHIELD_OPERATION)🤖 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-platform-wallet-ffi/src/shielded_send.rs` around lines 599 - 610, Define a shared constant for the shield operation label and use it both at the shield entry point’s map_spend_result call and in the ShieldedInsufficientBalance match guard within the operation-result mapping logic. Replace the duplicated "shielded shield" literals with that constant while preserving the existing error mapping behavior.packages/rs-platform-wallet/src/wallet/platform_wallet.rs (1)
106-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a versioned invariant test for the shield input minimum. Current versions use
min_input_amount = 100_000, while the two-action shield fee is161_097_600; input 0 therefore clears the minimum after the fee reservation.🤖 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-platform-wallet/src/wallet/platform_wallet.rs` around lines 106 - 133, Add a versioned invariant test covering the shield input minimum, asserting current versions use min_input_amount = 100_000 and the two-action shield fee is 161_097_600. Verify input 0 remains above the protocol minimum after fee reservation, while preserving the existing behavior in the candidate-selection loop.
🤖 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-ffi/src/error.rs`:
- Around line 556-558: Update the documentation comment for the enum variant
assigned code 13, ErrorArithmeticOverflow, to remove the claim that it is
reserved or has no in-tree producer. Ensure the text states that
InputSumOverflow can map to this code, matching the mapping arm in
PlatformWalletError’s conversion.
- Around line 362-368: Record error code 41 in ERROR_CODE_REGISTRY.md and update
DashSdkError.fromPlatformWalletNative to map
PlatformWalletError.ShieldedInsufficientBalance explicitly instead of falling
back to PlatformWallet.Generic. Preserve the existing ABI code and use the
corresponding Kotlin error type for this platform payment-account shortfall.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- Around line 599-610: Define a shared constant for the shield operation label
and use it both at the shield entry point’s map_spend_result call and in the
ShieldedInsufficientBalance match guard within the operation-result mapping
logic. Replace the duplicated "shielded shield" literals with that constant
while preserving the existing error mapping behavior.
In `@packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- Around line 106-133: Add a versioned invariant test covering the shield input
minimum, asserting current versions use min_input_amount = 100_000 and the
two-action shield fee is 161_097_600. Verify input 0 remains above the protocol
minimum after fee reservation, while preserving the existing behavior in the
candidate-selection loop.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 20359b11-f5ab-44a7-bbca-a55496c44408
📒 Files selected for processing (10)
packages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet-ffi/src/shielded_types.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
thepastaclaw
suggested changes
Aug 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The shared planner improves fee-reserve and minimum-input handling, but it can advertise and select a maximum requiring more address inputs than the protocol permits, so the primary preflight guarantee is not yet reliable. The new planner also misses persisted funded addresses after relaunch, and the newly exposed error contract needs clearer funding-domain separation plus a Kotlin mirror update.
Source: reviewer backend model for all four reviewer lanes: gpt-5.6-sol; final verifier backend model: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 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/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:180-192: Account for the versioned maximum address-input count
The planner includes every qualifying address in the usable suffix and derives `max_shieldable_credits` from their complete balance, without applying `platform_version.dpp.state_transitions.max_address_inputs`. Platform rejects any address-based transition exceeding that limit; the active address-funds versions currently permit 16 inputs. A fragmented account can therefore receive `can_shield == true` and a maximum that `select_inputs(max_shieldable_credits)` represents with more than 16 inputs. The shield path fetches those inputs and then constructs the Orchard proof before the oversized transition is rejected. Pass the versioned input cap into the planner, derive capacity from an input set that can actually be represented under the cap, enforce the same cap during selection, and add a regression test with one more qualifying input than allowed.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1550-1560: Include hydrated address balances in the shield candidate set
This enumerates only the transient `account.addresses.addresses` pool. The wallet's established `PlatformAddressWallet::candidate_address_set` deliberately unions that pool with `account.address_balances`, because the derived pool can be empty immediately after relaunch while persisted funded balances are already hydrated. In that state the new preflight reports zero capacity and the shared execution planner rejects shielding even though the displayed Platform balance includes funded addresses. Build a deduplicated union of derived addresses and `address_balances` keys before reading cached balances, and cover a funded address present only in `address_balances`.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:131-138: Use a domain-specific error for Platform shield capacity
`ShieldedInsufficientBalance` is the existing note-pool selection error for shielded transfer, unshield, withdrawal, and identity creation, but this change also returns it for a transparent Platform-address shortfall. That makes the Rust error's funding domain ambiguous and leaves its display text saying “Insufficient shielded balance” for a payment-account failure. The ambiguity propagates into FFI handling: the blanket conversion maps the shared variant to payment-account code 41, while `map_spend_result` must gate that code using the exact operation label `"shielded shield"` to keep note-pool failures generic. Add a distinct variant such as `PlatformShieldCapacityExceeded` for cached selection and live address-input shortfalls, map code 41 from that variant, and retain `ShieldedInsufficientBalance` exclusively for shielded notes.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:503-527: Mirror the new shield-capacity error in Kotlin
Code 41 now reaches the existing Kotlin `PlatformWalletManager.shieldedShield` path through `take_pwffi_error`, but `fromPlatformWalletNative` has no code-41 branch and converts it to `PlatformWallet.Generic`. This loses the typed payment-account-capacity outcome that Swift receives and prevents Kotlin callers from handling the newly documented refresh-and-reconfirm contract by error type. Add a dedicated `DashSdkError.PlatformWallet` subtype and map native code 41 to it. The separately cited `packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md` is not present as a tracked file at this head, so that part of the requested change cannot be applied at the stated path unless the registry is introduced separately.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/error.rs:113-116: Update the arithmetic-overflow code documentation
The comment still says error code 13 is reserved and has no in-tree producer, but this PR adds an `InputSumOverflow` conversion to `ErrorArithmeticOverflow`. Update the comment so consumers of the FFI contract know that code 13 can now be returned when candidate input balances overflow `u64`.
Collaborator
|
The PR title checks are failing because the title is not Conventional Commits formatted. I do not have permission to edit this PR title; please rename it to |
llbartekll
changed the title
Contributor
Author
|
Addressed the remaining automated-review feedback in d958b28:
Validation: planner 12/12, hydrated union, fee invariant, live-shortfall mapping, and focused FFI mappings passed; cargo fmt and git diff check passed. The Kotlin test is committed but could not run locally because this machine has no configured Android SDK, so CI remains the authoritative Kotlin run. The Conventional Commit title checks are green. |
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
Why
The wallet previously treated the displayed Platform balance as fully shieldable. A Platform Payment account may need to retain protocol reserve and may contain inputs that cannot participate in the transition, so sending the displayed maximum could fail with
Insufficient shielded balance.Companion iOS PR: dashpay/dashwallet-ios#971
Testing
cargo test -p platform-wallet --features shielded shield_input_selection_tests(11 passed)cargo test -p platform-wallet --features shielded shield_input_fetch_error_tests(1 passed)cargo test -p platform-wallet-ffi --features shielded shield(15 passed)ErrorHandlingTests(45 passed)cargo fmt --all -- --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests