feat: expose Platform-to-Shielded capacity preflight - #4360
Merged
Conversation
llbartekll
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
shumkov
as code owners
August 10, 2026 18:14
Collaborator
|
🕓 Ready for review — 1 ahead in queue (commit 1e691aa) |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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: preflight result
PlatformWalletManagerShieldedSync-->>SwiftClient: ShieldedShieldPreflight or typed error
Possibly related PRs
Suggested reviewers: <fixed_issue_severity>Medium</fixed_issue_severity> 🚥 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4360 +/- ##
=========================================
Coverage 87.80% 87.80%
=========================================
Files 2641 2641
Lines 336510 336510
=========================================
Hits 295468 295468
Misses 41042 41042
🚀 New features to boost your workflow:
|
thepastaclaw
approved these changes
Aug 10, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
All five prior findings are fixed at the exact head: the planner enforces the versioned input cap, includes hydrated addresses, uses a domain-specific capacity error mirrored in Kotlin, and accurately documents arithmetic overflow. The remaining Codex claim is refuted because Drive validates an input nonce before its balance, while every address debit advances that nonce; a post-fetch competing debit therefore produces the already-typed nonce-mismatch error rather than the singular insufficient-funds error.
Source: reviewers gpt-5.6-sol; final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
…d fee The fixed 1e9-credit input-0 reserve was ~6x the actual charged fee (metered storage/processing + compute, ~1.65e8 at current constants), and the preflight promoted that slop into authoritative capacity numbers: accounts fragmented into sub-1e9 addresses reported can_shield=false regardless of aggregate balance, Max understated real capacity by ~8.3e8 credits, and a Max shield left its residue below the viability threshold, permanently unshieldable. Replace the constant with shield_fee_reserve_credits(platform_version) = 2 x compute_minimum_shielded_fee(SHIELD_NUM_ACTIONS), threaded through plan_shield_inputs like the other versioned planner limits. The reserve only has to cover the execution-side metered fee taken from input 0's post-reallocation residue (the structure gate is satisfied by the claimed fee reserve_shield_fee_on_input_0 loads, which is already versioned-exact), so one extra fee of metering headroom suffices, and the value now tracks fee-constant bumps instead of freezing. The headroom test now also bounds the reserve from above (<= 4x fee) so an oversized reserve can't silently reintroduce the capacity loss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
QuantumExplorer
approved these changes
Aug 11, 2026
bfoss765
added a commit
that referenced
this pull request
Aug 11, 2026
The proposed-allocation era this file tracked has largely settled: #4308 merged the reservation trio (34-36, 438153d), #4348 took 37-40 (6373e00) and #4360 took 41 (e0b8baa) - all now in the merged table. Codes 29/32/33 lapse to RESERVED (their in-repo owners closed without merging), the shielded-invite claim loses 37 to merged ABI and takes the frontier on revival, the frontier moves 38 -> 42, the #4196 'still outstanding' note is corrected to its 2026-08-03 close-out, and the 2026-08-04 heads-of-record table is labeled the historical snapshot it is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
that referenced
this pull request
Aug 11, 2026
…ng, unlist open #4243 The 31-vs-33 decision died with its owners (#4256 closed 2026-08-06, successor #4311 closed 2026-08-10): 33 is RESERVED, a revival chooses fresh. Code 13's stale rustdoc was already corrected in-tree by #4360. And #4243 is still open — it does not belong in the closed-and-recreated group. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
3 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