feat(drive): shielded fees for Shield/ShieldFromAssetLock + shield credit conservation

[QuantumExplorer]

Issue being fixed or feature implemented

This PR does two things to the shielded pool (protocol v12 — still pre-release, behind feature gates):

1. Fixes a bug in transparent Shield that destroys credits and halts the chain.
A Shield moves credits from transparent platform addresses into the shielded pool. A wallet authorizes a maximum contribution per input, and only part of it is actually shielded — the rest is supposed to stay in the address. The old code debited the full authorized amount from each address but credited the pool only the shielded amount, so the difference was destroyed. The platform runs a credit-conservation check at the end of every block, so a single ordinary shield (the wallet routinely authorizes more than it shields) makes every validator reject the block at the same height → the chain stops.

2. Completes and corrects the shielded fee model so every shielded operation pays for the resources it uses.
Every shielded operation should pay for its expensive zero-knowledge proof verification and its storage. Transfers/unshields/withdrawals got that in #3800; this PR finishes the job for the two entry transitions (Shield, ShieldFromAssetLock), charges Shield the real metered cost instead of a flat estimate, and prices the Core withdrawal document into ShieldedWithdrawal so the proposer is properly compensated.

What was done?

1. The credit-conservation fix

Debit only what is actually shielded, and leave the rest in the source addresses (the documented behavior). The shield action transform reallocates the per-input debits so the total consumed equals exactly the shielded amount; the unused remainder stays put. Nothing about the Orchard proof depends on how the transparent side is split, so this is safe. Net result: addresses lose amount + fee, the pool gains amount, the fee pools gain fee — credits conserved.

2. The shielded fees

• Shield (transparent → pool) is charged like any other address-funded transition: GroveDB meters the real storage and processing of the note/nullifier writes, and only the ZK compute fee compute_shielded_verification_fee(num_actions) (Halo 2 proof verification + per-action processing, no storage term) is added on top — via additional_fixed_fee_cost, the same mechanism IdentityCreateFromAddresses uses. Storage is never double-counted, and conservation (deduct == book) holds by the standard machinery with no special-case override.
• ShieldFromAssetLock (Core asset lock → pool) pays compute_minimum_shielded_fee + asset_lock_base_cost (50M credits), matching how every other asset-lock-funded transition is priced. The lock is fully consumed and split into amount → pool, pool_fee → fee pools, and any leftover surplus → an optional signed surplus_output address (or, if none is set, folded into the fees but capped at 0.2 DASH; exceeding the cap without an output is rejected).
• Unshield (pool → transparent address) prices the single AddBalanceToAddress write that credits the net to the output platform address (the AddBalanceToAddress write is ~6.24M metered; its ~222-byte storage portion is priced, flat regardless of action count): compute_shielded_unshield_fee = compute_minimum_shielded_fee + 222-byte storage component. Much smaller than the withdrawal document, but priced for the same reason — so the address write is covered rather than subsidized by the proof fee.
• ShieldedWithdrawal additionally prices the Core withdrawal document it writes. That document insert (into the withdrawals contract + its indexes) costs ~110M credits — flat regardless of action count — and the base fee did not account for it, so the booking silently diverted the proof-verification fee to cover the document's storage (the proposer was paid ~5.8M for ~100M of verification work). The fee now adds a +4,100-byte storage component (compute_shielded_withdrawal_fee, byte-based so it tracks the storage rate), restoring proper storage funding and proposer compensation.

(ShieldedTransfer (pool → pool) writes no address/document, so it keeps the base compute_minimum_shielded_fee.)

Implementation details

Transparent Shield. compute_shielded_verification_fee = compute_minimum_shielded_fee minus the per-action storage estimate (proof + per-action processing only). It rides the standard PaidFromAddressInputs path, so the fragile two-sided booking override is deleted. The stateless structure floor requires amount + compute_shielded_verification_fee (a true lower bound; the authoritative metered + compute gate is validate_fees_of_event).

ShieldFromAssetLock. A signed surplus_output platform-address field (placed before the sig-excluded signature, so it can't be redirected/truncated). The transform enforces lock ≥ amount + pool_fee on every asset-lock path; the surplus is routed to the address or folded into the fee (bounded by the versioned shielded_implicit_fee_cap). The unshipped v0 asset-lock converter was collapsed into a single correct v0 (the old v0 didn't route the surplus — a latent mint hazard — and never executed on any network since v12 isn't active).

Unshield / ShieldedWithdrawal. compute_shielded_unshield_fee = base + SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES (222) × per_byte_rate; compute_shielded_withdrawal_fee = base + SHIELDED_WITHDRAWAL_DOCUMENT_STORAGE_BYTES (4100) × per_byte_rate. Each is wired through the three sites that must agree byte-for-byte: the carve (transform), the validate_minimum_shielded_fee gate (a ShieldedMinFeeKind { Base, Unshield, Withdrawal } enum picks the formula per transition), and the SDK builder; a ShieldedFeeKind { Base, Unshield, Withdrawal } threads through the wallet note-selection so each path reserves its own fee.

Also: the shielded_implicit_fee_cap version constant; the new ShieldedImplicitFeeCapExceededError (code 10826); action/converter/event/booking plumbing; FFI + wasm-dpp2 + Swift surface. All new behavior is version-gated behind v12 shielded activation.

How Has This Been Tested?

• Block-level conservation test_shield_with_inputs_greater_than_amount_conserves_credits: builds a real Orchard shield bundle (genuine Halo2 proof), runs the full block pipeline (execute + distribute fees + end-of-block sum-tree check), and asserts total credits stay balanced. Verified it fails without the fix with the exact production error CorruptedCreditsNotBalanced(... off by 20000000000) (= Σrequested − amount).
• Metered Shield: the conservation tests now also assert the booked storage_fee > 0 (real metered storage is captured) and processing_fee ≥ compute_fee.
• Withdrawal document fee: the converter metering test asserts the (new, higher) fee covers the real apply-mode write cost incl. the document, with comfortable margin; compute_minimum_shielded_fee proven byte-for-byte unchanged by the refactor.
• Fee invariants pinned: ShieldedTransfer carved-fee action count == on-wire actions.len(); compute_minimum_shielded_fee > real metered storage for the pool-paid transitions (so the min() booking split can't zero the proposer reward).
• Scope: each fee change is isolated — when Shield, Unshield, or ShieldedWithdrawal is modified, the other shielded transitions' fee paths are regression-tested unchanged (e.g. ShieldedTransfer and ShieldFromAssetLock keep the base compute_minimum_shielded_fee).

Full shielded suites pass across dpp, drive, drive-abci, and platform-wallet; a multi-agent adversarial fee audit returned no consensus/conservation bug; cargo fmt --all --check clean.

Breaking Changes

None. This is a pre-release shielded-pool feature (protocol v12, behind feature gates). No deployed network depends on the prior (broken) behavior, and the builder/SDK already produce the input shapes this PR accounts for.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds deterministic reallocation of transparent inputs so ShieldTransition debits equal the shielded amount, integrates the helper into transform_into_action_v0, adds unit tests for allocation edge cases, and includes end-to-end regression tests verifying credit conservation.

Changes

Shield Input Reallocation and Credit Conservation

Layer / File(s)	Summary
Imports and reallocation helper packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs	Adds reallocate_inputs_for_shield_amount, which deterministically allocates shield_amount across input addresses (BTreeMap order), reconstructs actual balances from remaining+requested, debits up to requested with overflow/underflow guards, and errors on missing inputs or leftover allocation.
Reallocation integration into transform packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs	Calls the helper in transform_into_action_v0 to replace inputs_with_remaining_balance with the adjusted map before reading pool state, computing fees, and creating ShieldTransitionAction.
Reallocation helper unit tests packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs	Unit tests validate single-input exact debit, deterministic non-fee-first consumption under fee strategy, multi-input debit-sum conservation, spillover allocation across addresses, missing-transition-input error, anti-mint rejection, and debug-only nonce mismatch panic.
Credit conservation regression tests packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs	Adds credit_conservation async regression tests that construct and sign real Orchard shield transitions, run them via process_state_transitions, and assert total platform credits unchanged and shielded pool increases exactly by shield_amount. Also a small import reformat.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

ready for final review

Suggested reviewers

• thepastaclaw
• shumkov

Poem

A rabbit counts each shiny coin tonight,
I hop inputs round to keep the math tight.
Debits match shields, no phantom delight,
Tests march through blocks till the ledger is right. 🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: introducing shielded fees for Shield/ShieldFromAssetLock transitions and fixing the credit conservation bug through deterministic input reallocation.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-shield-credit-conservation

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[thepastaclaw]

✅ Review complete (commit 788f515)

[codecov]

Codecov Report

❌ Patch coverage is 86.34193% with 143 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.16%. Comparing base (e3a83b3) to head (788f515).
⚠️ Report is 1 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
...state_transition_was_executed_with_proof/v0/mod.rs	64.28%	40 Missing ⚠️
...transitions/shield/transform_into_action/v0/mod.rs	92.74%	18 Missing ⚠️
...p/src/shielded/compute_minimum_shielded_fee/mod.rs	58.33%	15 Missing ⚠️
...hield_transition/v0/state_transition_validation.rs	86.40%	14 Missing ⚠️
...rc/shielded/compute_minimum_shielded_fee/v0/mod.rs	92.30%	10 Missing ⚠️
...sition_processing/validate_fees_of_event/v0/mod.rs	90.32%	6 Missing ⚠️
...ielded/shield_from_asset_lock_transition/v0/mod.rs	87.17%	5 Missing ⚠️
...rations/shielded/shielded_withdrawal_transition.rs	93.42%	5 Missing ⚠️
...vert_to_operations/shielded/unshield_transition.rs	88.63%	5 Missing ⚠️
...tate_transition_processing/execute_event/v0/mod.rs	50.00%	4 Missing ⚠️
... and 10 more

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3793      +/-   ##
============================================
- Coverage     87.17%   87.16%   -0.01%     
============================================
  Files          2627     2627              
  Lines        321631   322573     +942     
============================================
+ Hits         280366   281185     +819     
- Misses        41265    41388     +123     

Components	Coverage Δ
dpp	87.67% <84.73%> (-0.02%)	⬇️
drive	86.06% <81.45%> (-0.01%)	⬇️
drive-abci	89.50% <91.92%> (+0.01%)	⬆️
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.17% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	47.85% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

Code Review

The PR fixes a confirmed consensus-critical credit-destruction bug: the shared address-balance validation debits Σrequested while the shielded pool is only credited shield_amount, which would trip the block-end sum-tree conservation check and halt the chain. The reallocation in transform_into_action_v0 is deterministic (BTreeMap key order), arithmetically safe under the structure-validation invariant (Σrequested ≥ shield_amount), and the unit + block-level tests verify both the per-input math and the production sum-tree invariant. No blocking issues; a few suggestion/nitpick-level polish items on the new helper.

🟡 1 suggestion(s) | 💬 5 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-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs:71-103: Greedy BTreeMap-order allocation can drain the input that the default fee strategy targets
  Reallocation walks `inputs_with_remaining_balance` in BTreeMap key order and greedily debits `shield_amount` starting from the lowest-keyed `PlatformAddress`. The default fee strategy `AddressFundsFeeStrategyStep::DeductFromInput(0)` resolves the index via the same BTreeMap key order, so the input targeted for fees is also the first one fully consumed by the shield amount. For a perfectly reasonable client choice where `requested[first_addr] == shield_amount`, the subsequent fee deduction will find that input zeroed, hit `fee_fully_covered = false`, and reject the transition as `AddressesNotEnoughFundsError` even though other inputs have ample slack. No consensus exploit — every validator sees the same behavior — but the rejection mode is opaque. Either reallocate proportionally (so no input is fully zeroed while others have slack) or add a doc note on `ShieldTransitionV0::inputs` / default `AddressFundsFeeStrategyStep` requiring the first BTreeMap-ordered input to retain fee headroom.

[thepastaclaw]

Code Review

The latest push (a597479 → 063decf) fixes the prior blocker: a fee-aware two-pass allocator now consumes non-fee-strategy inputs before fee-strategy inputs (both in BTreeMap order), so a client whose fee-payer input is sized exactly to the shield amount is no longer drained before fee deduction. Determinism is preserved (BTreeMap iteration + fixed pass order) and per-input debits remain capped at requested, so credit conservation holds. A new regression test (test_reallocation_consumes_non_fee_inputs_first) plus the multi-input block-pipeline conservation test cover the fix. Remaining items are pure nitpicks (helper signature, arithmetic round-trip, discarded transition nonce, redundant inner-mod imports, and a small Pass-2 ordering comment); none are blocking.

💬 1 nitpick(s)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

[thepastaclaw]

Code Review

Cumulative incremental review at c4a4055. The latest delta (063decf..c4a4055) is a v3.1-dev merge that does not touch the shield credit-conservation code that is the substance of PR #3793. The reallocate_inputs_for_shield_amount helper and the credit-conservation test module are byte-identical to the prior reviewed SHA. All 5 prior findings reconciled as STILL VALID and carried forward; no new defects in the latest delta. CodeRabbit reported 0 inline findings and its walkthrough adds nothing actionable. Security agents found no exploitable issue; one pre-existing hardening gap (shield validate_structure missing FeeStrategyIndexOutOfBounds) is recorded out-of-scope.

💬 1 nitpick(s)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

[thepastaclaw]

Code Review

Latest delta c4a4055..efe3292 is a single docstring blank-line fix for clippy's doc_lazy_continuation in reallocate_inputs_for_shield_amount; no behavior change. Cumulatively the credit-conservation fix is correct and well-tested (block-level sum-tree conservation regression). Reconciling the 5 prior findings: all 5 are STILL VALID against the worktree at efe3292 and are carried forward as nitpicks. No new latest-delta findings from any agent and 0 CodeRabbit inline findings (CR summary noted no actionable comments).

All five items below are carried-forward prior findings re-validated at this exact head; their inline threads already exist from the previous review, so this review records the exact-SHA cumulative reconciliation without duplicating those inline comments.

💬 5 nitpick(s)

5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

[thepastaclaw]

Code Review

Latest delta (efe3292..35588c6) is a focused cleanup pass addressing 4 of 5 prior nitpicks: the helper now takes &ShieldTransitionV0 directly, the discarded transition nonce is now lock-stepped against the remaining-balance nonce via debug_assert_eq!, the BTreeMap-vs-fee-strategy-priority ordering caveat is now documented inline, and the redundant imports in the inner credit_conservation test module are removed. Prior nitpick #2 (the actual = remaining + requested round-trip) is STILL VALID at lines 141-161; both reviewers note it appears intentionally deferred. No new in-scope defects observed in the delta. The cumulative fix remains correct and conservation is empirically asserted by the block-level sum-tree regression test.

💬 1 nitpick(s)

[thepastaclaw]

Code Review

Cumulative review at 5b0ff35. The latest delta folds shielded fee accounting into Shield/ShieldFromAssetLock cleanly: conservation is enforced symmetrically (structure validation requires amount + F, execution overrides deduction and booking atomically to exactly F). The single prior finding (shield reallocation actual-balance round trip) is STILL VALID and carried forward as a nit. New findings center on CheckTx fee-reporting consistency (gas_wanted still reflects metered estimate, not flat F), an SDK helper that cannot route signed surplus, WASM input canonicalization for surplus address bytes, and a few minor cleanups in the new flat-fee branch. Codex blockers were downgraded after verification: the max_address_inputs cap is enforced via the shared validate_address_balances_and_nonces trait (Shield implements it at line 143), and the FFI ABI change is contained to a monorepo with the Swift wrapper updated in lockstep. CodeRabbit added no inline findings (summary-only walkthrough).

🟡 5 suggestion(s) | 💬 1 nitpick(s)

2 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/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs:199-233: Return the flat shielded fee from fee validation so CheckTx reports the correct gas
  For `Shield`, this branch correctly overrides `required_balance` to `shielded_flat_fee` but still returns the unmodified `estimated_fee_result` derived from GroveDB metering. CheckTx uses the returned `fee_result.total_base_fee()` as `gas_wanted`, so a valid shield is reported to Tenderdash at the lower metered estimate rather than the flat `F` actually deducted in `execute_event`. The same drift exists in the `PaidFromAssetLockToPool` arm at lines 235-275, which validates against `fees_to_add_to_pool` but returns the metered estimate. This breaks parity between what fee validation says the transition costs and what execution actually books, and feeds incorrect data into mempool admission/ordering logic that uses gas_wanted. Either rebuild a `FeeResult` representing the authoritative fee (`storage = min(metered_storage, F)`, `processing = F - storage`) and return that, or thread an explicit `total_fee` out so the caller can populate gas_wanted from the authoritative source.

In `packages/rs-sdk/src/platform/transition/shield_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/transition/shield_from_asset_lock.rs:43-52: SDK helper cannot route signed asset-lock surplus
  The transition type now carries an optional signed `surplus_output`, and the DPP builder/FFI paths expose it, but this dash-sdk helper hardcodes `None` at parameter 8. The helper accepts arbitrary `bundle` and `value_balance`, so a caller who intentionally shields less than `asset_lock_value − pool_fee` has no way to provide a surplus destination through this API and will build a transition that consensus rejects once `surplus > shielded_implicit_fee_cap` (0.2 Dash). Either expose `surplus_output: Option<PlatformAddress>` as an additional parameter, or rename/constrain this helper to a remainder-only flow so the signature reflects what it can safely build.

In `packages/wasm-dpp2/src/platform_address/address.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/platform_address/address.rs:112-119: WASM PlatformAddress byte path lacks the strict 21-byte length check used on the FFI side
  `ShieldFromAssetLockTransitionWasm::new` now accepts `surplusOutput` as `PlatformAddressLike`, which routes JS byte input through this `TryFrom<JsValue>`. The path converts the JS array to a `Vec<u8>` and calls `PlatformAddress::from_bytes` without first checking the buffer is exactly 21 bytes (1-byte variant tag + 20-byte hash). The Rust FFI side enforces an exact-21-byte check for the same field, so the WASM boundary should match — otherwise a JS caller can pass an over-long buffer for a signed surplus destination and have the trailing bytes silently dropped, producing a signature over a different address than the bytes the caller submitted. The fix narrowly is to reject non-21-byte inputs in this `Uint8Array`/array branch.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rs:351-367: Implicit-fee-cap rejection reuses `ShieldedInvalidValueBalanceError` with a free-form string
  When `surplus_output` is `None` and the asset-lock surplus exceeds `shielded_implicit_fee_cap`, the transition is rejected with `ShieldedInvalidValueBalanceError::new(format!("asset-lock surplus ({}) exceeds the implicit fee cap ({}); ..."))`. The inline comment acknowledges this is a placeholder. Consensus error variants are pinned by serialization index, so introducing a dedicated `ShieldedImplicitFeeCapExceededError` later requires appending at the end of `BasicError` — easier to define the typed variant now (carrying `surplus` and `cap` as fields) and ship it in the same version that introduces the gate, than to renumber/migrate the error space after v12.
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rs:351-367: Add direct boundary tests for the new implicit surplus-fee cap
  The new implicit-fee-cap branch (`surplus_output == None && surplus > cap → reject`) is a consensus gate but has no direct unit test exercising its boundary. The current tests use `Some(...)` for large-surplus paths and only test the converter after an action exists. Add cases that (1) accept `None` with `surplus == cap` and (2) reject `None` with `surplus == cap + 1`, both before action construction, so future changes to the cap or the comparison cannot regress silently.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27134620435
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "0e17a66e6afc4f440d6a952e5e417686026c455722c45a4f0679dffdc008785c"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[thepastaclaw]

Code Review

Cumulative incremental review at head 43d6a4c. The latest delta vs 5b0ff35 is a single-line Swift change marking withOptionalSurplusOutput nonisolated — no new functional issues introduced by the delta. All 7 prior findings remain STILL VALID against current head and are carried forward unchanged. Two converge as security/bug-class items (gas_wanted drift; WASM 21-byte gate), one is an SDK ergonomics gap, one is consensus error taxonomy, one is missing boundary tests, and two are arithmetic-style nitpicks. CodeRabbit supplied no inline findings (review paused), so no CR reactions are emitted.

🟡 4 suggestion(s)

3 additional finding(s) omitted (not in diff).

3 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-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs:199-233: [Carried forward] Fee validation returns metered estimate, not authoritative shielded flat fee — CheckTx gas_wanted under-reports the actual deduction
  Verified at head 43d6a4cc. For the transparent `Shield` arm, `required_balance` is overridden to `*flat_fee` (line 200) but the returned `ConsensusValidationResult` still carries the unmodified `estimated_fee_result` derived from `apply_drive_operations` (line 220). CheckTx maps the returned `FeeResult::total_base_fee()` into the Tenderdash `gas_wanted` field, so a valid shield is announced at the lower GroveDB-metered estimate while `execute_event` deterministically deducts the higher flat F. The same drift exists in the `PaidFromAssetLockToPool` arm (line 258-274): validation gates against `fees_to_add_to_pool` / the metered floor but returns the metered estimate. Not a consensus divergence (execution-side deduction is authoritative), but it weakens the mempool gas-admission DoS control for the new shielded fee model and creates CheckTx-vs-execution accounting drift. Either rebuild a `FeeResult` representing the authoritative flat / pool fee before returning, or thread an explicit total-fee value out so callers can populate gas_wanted from the authoritative source.

In `packages/rs-sdk/src/platform/transition/shield_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/transition/shield_from_asset_lock.rs:43-53: [Carried forward] SDK shield_from_asset_lock helper hardcodes surplus_output=None — over-cap surpluses are silently unrecoverable through this API
  Verified at head 43d6a4cc. `ShieldFromAssetLockTransition::try_from_asset_lock_with_bundle` is called with `None` at the `surplus_output` parameter (line 51). The transition supports an optional signed `surplus_output`, and the DPP builder / FFI / WASM paths all expose it, but this dash-sdk helper doesn't. A caller whose `asset_lock_value − pool_fee` exceeds the `shielded_implicit_fee_cap` (0.2 DASH) has no way to route the remainder anywhere — the helper builds a transition that consensus will deterministically reject (see `transform_into_action/v0/mod.rs:352-367`). Either expose `surplus_output: Option<PlatformAddress>` as an additional trait parameter, or rename/constrain this helper to a documented remainder-only flow.

In `packages/wasm-dpp2/src/platform_address/address.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/platform_address/address.rs:112-119: [Carried forward] WASM PlatformAddress byte path lacks the strict 21-byte length check enforced by FFI — JS callers can sign over a silently-truncated surplus_output
  Verified at head 43d6a4cc. `ShieldFromAssetLockTransitionWasm::new` accepts `surplusOutput` as `PlatformAddressLike`, routing JS byte input through `TryFrom<JsValue>`. The Uint8Array/array branch converts to `Vec<u8>` and calls `PlatformAddress::from_bytes(&bytes)` with no length pre-check. `PlatformAddress::from_bytes` (rs-dpp `address_funds/platform_address.rs:337`) uses `bincode::decode_from_slice` which does NOT require full-slice consumption — trailing bytes after a valid 21-byte prefix are silently dropped. The Rust C FFI side enforces exactly 21 bytes for the same field (`parse_optional_surplus_output` in `rs-platform-wallet-ffi/src/shielded_send.rs:62`), citing exactly this concern. Because `surplus_output` is signed (only the signature itself is excluded from `platform_signable`), a JS caller passing a 22+ byte buffer ends up signing over the truncated address rather than the bytes they submitted — a destination-binding mismatch on a signed field, with the WASM and FFI surfaces diverging in input validation for the same field. Reject non-21-byte inputs in the Uint8Array/array branch before calling `from_bytes`.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs:1: [Carried forward] Missing direct boundary tests for the implicit surplus-fee cap
  Verified at head 43d6a4cc — grepping the shield_from_asset_lock test module finds zero references to `implicit_fee_cap` / `shielded_implicit_fee_cap`. The new consensus gate (`surplus_output == None && surplus > cap → reject`, transform_into_action/v0/mod.rs:349-367) has no direct unit test exercising its boundary. A regression flipping `>` to `>=` (or vice versa) would be a silent consensus divergence. Add at least two cases: (a) `surplus_output = None` with `surplus == cap` should construct an action successfully, and (b) `surplus_output = None` with `surplus == cap + 1` should be rejected with the cap-exceeded error before action construction. These pin the exact boundary that the `v0::cap` constant defines.

[thepastaclaw]

Code Review

Latest delta fixed 4 of 7 prior findings (typed cap error, boundary tests, saturating_sub, SDK surplus_output). Three prior findings remain STILL VALID and are carried forward. One NEW blocking issue introduced: the added BasicError::ShieldedImplicitFeeCapExceededError variant has no arm in wasm-dpp's exhaustive from_basic_error match, causing cargo check -p wasm-dpp to fail (E0004 reproduced locally). Also flagging the mid-enum variant insertion against the project's pinned-order rule, though the two shifted variants are themselves unshipped, so impact is convention-only.

🔴 1 blocking | 🟡 3 suggestion(s)

3 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/wasm-dpp/src/errors/consensus/consensus_error.rs`:
- [BLOCKING] packages/wasm-dpp/src/errors/consensus/consensus_error.rs:471-967: wasm-dpp build broken: `from_basic_error` match missing arm for new `ShieldedImplicitFeeCapExceededError`
  The new `BasicError::ShieldedImplicitFeeCapExceededError` variant added in this PR is not handled in this exhaustive match (no wildcard arm). `cargo check -p wasm-dpp` fails with `error[E0004]: non-exhaustive patterns: \`&dpp::consensus::basic::BasicError::ShieldedImplicitFeeCapExceededError(_)\` not covered` at line 471. `wasm-dpp` is a workspace member, so workspace builds and any downstream consumer of the legacy WASM bindings is broken until an arm (and the corresponding wasm wrapper struct) is added. Add a `BasicError::ShieldedImplicitFeeCapExceededError(e) => generic_consensus_error!(ShieldedImplicitFeeCapExceededError, e).into()` arm alongside the existing shielded basic error arms, plus the matching `*Wasm` wrapper under `packages/wasm-dpp/src/errors/consensus/basic/state_transition/`.

In `packages/rs-dpp/src/errors/consensus/basic/basic_error.rs`:
- [SUGGESTION] packages/rs-dpp/src/errors/consensus/basic/basic_error.rs:679-686: Append the new `ShieldedImplicitFeeCapExceededError` variant instead of inserting mid-enum
  `BasicError` derives `PlatformSerialize`/`PlatformDeserialize`/`Encode`/`Decode`, which serialize by enum variant index, and the file header explicitly states `DO NOT CHANGE ORDER OF VARIANTS WITHOUT INTRODUCING OF NEW VERSION`. The new variant is inserted between `ShieldedInvalidValueBalanceError` and `ShieldedEncryptedNoteSizeMismatchError`, shifting the discriminants of the two trailing variants. Practical impact here is limited because those trailing variants were themselves added recently on `develop` (commit b7075ae0b1) and have not shipped, so no deployed wire format is broken — but the project rule is mechanical and worth honoring so future audits don't have to reason about which variants are 'safe to shift'. Move the new variant to the end of the enum.

In `packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs:199-233: [Carried forward — STILL VALID] Fee validation returns metered estimate, not the authoritative shielded flat fee
  Re-verified at head 0a23a4e8. The transparent `Shield` arm overrides `required_balance` to `*flat_fee` (line 200), but the returned `ConsensusValidationResult` still wraps the unmodified `estimated_fee_result` produced by `apply_drive_operations` (lines 219–220, 223–224). CheckTx maps `FeeResult::total_base_fee()` into Tenderdash `gas_wanted`, so valid shields are announced at the lower GroveDB-metered estimate while execution deducts the higher flat fee. The same drift exists in the `PaidFromAssetLockToPool` arm at line 258, where `required_fee` is the metered estimate rather than the authoritative `compute_minimum_shielded_fee + asset_lock_base_cost` flat fee. Not consensus-divergent (all nodes deduct identically at execute), but it weakens mempool gas-admission DoS controls and creates a CheckTx-vs-execution accounting drift. Rebuild a `FeeResult` representing the authoritative flat/pool fee before returning, or thread an explicit total-fee value out so callers can populate gas_wanted from the authoritative source.

In `packages/wasm-dpp2/src/platform_address/address.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/platform_address/address.rs:112-119: [Carried forward — STILL VALID] WASM PlatformAddress byte path lacks the strict 21-byte length check enforced by FFI
  Re-verified at head 0a23a4e8 — file unchanged in the latest delta. The Uint8Array/array branch passes raw bytes straight to `PlatformAddress::from_bytes(&bytes)`, which uses `bincode::decode_from_slice` and does not enforce full-slice consumption. A 22+ byte input whose first 21 bytes decode validly is silently truncated to the prefix. The Rust C FFI for the same field enforces exactly 21 bytes (`packages/rs-platform-wallet-ffi/src/shielded_send.rs`). Because `surplus_output` is part of the signed `ShieldFromAssetLockTransition` payload, a JS caller passing trailing bytes signs over a destination address different from the bytes they supplied, routing the surplus to a different address than intended with no on-the-wire signal of the truncation. Reject non-21-byte inputs in this branch before calling `from_bytes`.

[thepastaclaw]

Code Review

Latest delta (43d6a4c..1f12528) cleanly resolves prior findings #2, #4, #5, and #6. Three prior findings remain STILL VALID (CheckTx gas_wanted drift on shielded flat-fee arms, WASM PlatformAddress byte-length laxity that lets JS sign over a truncated surplus_output, and the shield reallocation arithmetic nit). One new blocking issue in this delta: the new ShieldedImplicitFeeCapExceededError variant is inserted in the middle of BasicError, shifting the bincode discriminants of two trailing variants and violating the explicit in-file rule that variant order must not change without a new version. Trivial mechanical fix: append the variant at the tail.

🔴 1 blocking | 🟡 2 suggestion(s)

2 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/rs-dpp/src/errors/consensus/basic/basic_error.rs`:
- [BLOCKING] packages/rs-dpp/src/errors/consensus/basic/basic_error.rs:679-680: New ShieldedImplicitFeeCapExceededError variant inserted mid-enum shifts bincode discriminants of two trailing variants
  The new variant is inserted between `ShieldedInvalidValueBalanceError` (line 677) and `ShieldedEncryptedNoteSizeMismatchError` (line 683), with `IdentityAssetLockTransactionTooManyInputsError` (line 686, from PR #3491) still after it. `BasicError` derives `Encode, Decode, PlatformSerialize, PlatformDeserialize` (lines 117–119) without explicit discriminants, so bincode encodes variants by position. The enum header explicitly states `DO NOT CHANGE ORDER OF VARIANTS WITHOUT INTRODUCING OF NEW VERSION` (lines 121–125). Inserting at this position bumps the wire discriminant of `ShieldedEncryptedNoteSizeMismatchError` and `IdentityAssetLockTransactionTooManyInputsError` by 1, so any pre-existing encoded `BasicError` (sibling branches, testnet artifacts, persisted error logs, cached check_tx results, snapshots) decodes as the wrong variant after this PR lands. The error-code integer in `codes.rs` is independent of variant order, so the fix is purely mechanical: append the new variant after `IdentityAssetLockTransactionTooManyInputsError`.

In `packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs:218-274: [Carried forward — STILL VALID] CheckTx returns metered fee estimate instead of authoritative shielded flat/pool fee
  Verified at head 1f12528f. In the `PaidFromAddressInputs` arm, `required_balance` is correctly overridden to `*flat_fee` (line 200) to gate funding, but the returned `ConsensusValidationResult` still carries the unmodified `estimated_fee_result` from `apply_drive_operations` + `add_many_to_fee_result` (lines 219–221, 223–224). The same drift exists in the `PaidFromAssetLockToPool` arm: `fees_to_add_to_pool` is the authoritative pool fee, but the returned FeeResult at lines 260–261 / 264–265 is the metered estimate. CheckTx maps the returned `FeeResult::total_base_fee()` into Tenderdash `gas_wanted`, so valid shields and shield_from_asset_lock are advertised at the lower GroveDB-metered estimate while execution actually deducts the higher flat/pool fee. Not a consensus divergence (deduction is correct on both nodes), but it weakens mempool gas-admission DoS controls and creates a CheckTx-vs-execution accounting drift visible to observability. Fix: rebuild a `FeeResult` representing the authoritative flat/pool fee before returning, or thread an explicit total-fee value out so callers can populate `gas_wanted` from the authoritative source.

In `packages/wasm-dpp2/src/platform_address/address.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/platform_address/address.rs:112-119: [Carried forward — STILL VALID] WASM PlatformAddress byte path lacks 21-byte length check; JS callers can sign over a silently-truncated surplus_output
  Verified at head 1f12528f (unchanged in this delta). The Uint8Array/array branch in `TryFrom<JsValue> for PlatformAddressWasm` converts to `Vec<u8>` and calls `PlatformAddress::from_bytes(&bytes)` with no length pre-check. `PlatformAddress::from_bytes` uses `bincode::decode_from_slice` and does NOT require full-slice consumption — trailing bytes after a valid 21-byte prefix are silently dropped. The Rust C FFI for the same field enforces exactly 21 bytes in `parse_optional_surplus_output`. Because `surplus_output` is part of the signed `ShieldFromAssetLockTransition` body (placed before the sig-excluded signature), a JS caller passing 22+ bytes signs over the truncated 21-byte prefix rather than the bytes they submitted — funds routed to a different destination with a still-valid signature, while the FFI rejects the same input. Reject non-21-byte inputs in the Uint8Array/array branch before calling `from_bytes`.

[thepastaclaw]

Code Review

Incremental review at f8a18a9. Prior findings status: (1) BasicError variant insertion — FIXED (variant tail-appended at basic_error.rs:690 with explanatory NOTE); (2) CheckTx fee-result drift — FIXED (validate_fees_of_event returns authoritative flat-fee FeeResult on both shielded arms); (3) WASM PlatformAddress 21-byte gate on TryFrom / TryFrom<&str> — FIXED (lines 123 and 167); (4) shield reallocation arithmetic round-trip — STILL VALID (nitpick, unchanged). New in this delta: Codex flagged a residual gap — the wasm_bindgen-exposed PlatformAddress.fromBytes/fromHex factories (address.rs:336-352) bypass the same 21-byte check, so JS callers can still construct a truncated PlatformAddress object and pass it as surplusOutput via the ShieldFromAssetLockTransitionWasm constructor which accepts existing address objects. No blockers; one suggestion and one carried-forward nitpick.

🟡 1 suggestion(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/wasm-dpp2/src/platform_address/address.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/platform_address/address.rs:334-352: PlatformAddress.fromBytes/fromHex bypass the new 21-byte length gate
  The TryFrom<JsValue> (lines 116-128) and TryFrom<&str> (lines 165-172) paths now correctly reject non-21-byte input before calling `PlatformAddress::from_bytes`, matching the C FFI's `parse_optional_surplus_output`. The wasm_bindgen-exposed `fromBytes` (lines 335-340) and `fromHex` (lines 343-352) factories still call `PlatformAddress::from_bytes(&bytes)` directly without that gate. Because `PlatformAddress::from_bytes` decodes via bincode's `decode_from_slice` and discards the consumed-length count, a 21-byte prefix with trailing bytes is silently truncated. A JS caller can therefore do `PlatformAddress.fromBytes(twentyOneBytesPlusGarbage)` (or the equivalent `fromHex`) and pass the resulting `PlatformAddress` object as `surplusOutput` to `ShieldFromAssetLockTransitionWasm::new`, which accepts existing `PlatformAddress` objects without re-validating their byte form. The signed transition then binds to the truncated prefix while the original padded input was never rejected — the same hazard the TryFrom paths were just hardened against. Apply the same exact-length check to both factories so every JS construction path enforces the same invariant.

[thepastaclaw]

Code Review

Incremental + cumulative verification at 1fd1bbb. Prior finding #1 (PlatformAddress.fromBytes/fromHex 21-byte gate in wasm-dpp2) is FIXED — verified at packages/wasm-dpp2/src/platform_address/address.rs:340-345 and :360-365, matching the TryFrom paths and the C FFI parse_optional_surplus_output gate. Prior finding #2 (shield reallocation actual-balance round-trip simplification at packages/rs-drive-abci/.../transform_into_action/v0/mod.rs:137-158) is STILL VALID — verified unchanged at head, carried forward as a low-confidence nitpick. No new defects introduced by the latest delta (single-file change to wasm-dpp2 PlatformAddress factories); no blockers.

💬 1 nitpick(s)

1 carried-forward finding already raised on this PR; not re-posting as a new inline comment.

🤖 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-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs`:137-158:
- [NITPICK] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v0/mod.rs:137-158: [Carried forward — STILL VALID] Simplify actual-balance reconstruction in shield reallocation
  Verified at head 1fd1bbb87e (file unchanged in latest delta). The helper recovers `actual = remaining_after_full_debit.checked_add(requested)?` (lines 139-145), then computes `new_remaining = actual.checked_sub(consumed)?` (lines 154-158). Since `consumed = remaining_to_consume.min(requested)` (line 148), the invariant `consumed <= requested` always holds, so the round-trip is algebraically `remaining_after_full_debit + (requested - consumed)` — one checked op instead of two. The direct form eliminates the defensive `CorruptedCodeExecution` branch at lines 154-158 (whose comment hand-waves that underflow is unreachable) and makes the credit-conservation invariant explicit at the arithmetic site: the unconsumed `requested - consumed` portion of this input's authorized contribution stays at the source address. Behavior is identical under the trailing `remaining_to_consume != 0` anti-mint guard at line 170. Pure style on a consensus-critical credit-conservation path — take-it-or-leave-it.

[thepastaclaw]

Code Review

Cumulative review at 09cf77f. Prior finding (Shield fee_strategy validation does not bounds-check DeductFromInput or reject ReduceOutput) is STILL VALID and carried forward — the latest delta only removes an unused block_info parameter from ShieldFromAssetLock::transform_into_action_v0 and does not touch state_transition_validation.rs. One new suggestion: ShieldFromAssetLock execution-proof query does not cover the new surplus_output credit. CodeRabbit posted no actionable inline comments (reviews paused).

🟡 2 suggestion(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-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs:207-214: [carried forward] Shield fee_strategy validation accepts unusable steps
  Carried forward from prior review and reconfirmed at head 09cf77f1 — this file is unchanged by the latest delta. `ShieldTransitionV0::validate_structure` only checks fee_strategy for empty, over-max length, and exact duplicates (lines 207–214). Two structural defects persist:

  1. `AddressFundsFeeStrategyStep::DeductFromInput(index)` is not bounds-checked against `self.inputs.len()`. The shared deduction helper at `packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs:38-52` snapshots input addresses into a `Vec` and resolves the index via `Vec::get`, so an out-of-range index silently no-ops.

  2. `AddressFundsFeeStrategyStep::ReduceOutput(_)` is silently accepted, even though `ShieldTransitionV0` (`v0/mod.rs:37-63`) has no outputs by construction. The downstream helper resolves `output_addresses.get(*index as usize)` against an empty vector and no-ops.

  The resulting malformed Shield only fails later at `validate_fees_of_event` with a generic `AddressesNotEnoughFundsError`, after expensive proof/witness verification has already executed. Not consensus-breaking — all honest nodes converge on the same rejection — but it lets a structurally-invalid fee strategy ride past the cheapest validation stage and inflates the unpaid CPU cost per rejected transition. Reject these structurally in the existing loop, matching the other address-funded transition validators.

In `packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs:390-407: ShieldFromAssetLock execution proof does not cover the new surplus_output credit
  The PR adds `surplus_output: Option<PlatformAddress>` to `ShieldFromAssetLockTransitionV0` (`packages/rs-dpp/.../shield_from_asset_lock_transition/v0/mod.rs:56`) and the action path credits that address when set. The execution-proof query for `StateTransition::ShieldFromAssetLock` here, however, still queries only `SpentAssetLockTransactions` for the outpoint; the matching verifier returns only `VerifiedAssetLockConsumed`. A light client therefore can verify that the asset lock was consumed but has no proof that the surplus address balance was credited. In today's orchestrated flow the surplus is pinned to zero, so this is latent rather than active, but as soon as a non-zero surplus is produced the proof surface is incomplete relative to other address-funded transitions. Consider merging an additional path-query for the surplus address balance when `surplus_output.is_some()` and extending the verifier result accordingly. Not consensus-critical — all nodes already agree on the credit via state application — purely a proof-completeness gap for light clients.

[thepastaclaw]

Code Review

Cumulative review at 792d1a5. The latest delta (single commit) prices the Unshield output-address write into a new compute_shielded_unshield_fee and threads it symmetrically through the SDK builder, transform_into_action, the consensus minimum-fee gate (via a new ShieldedMinFeeKind enum), and wallet note selection (ShieldedFeeKind::Unshield). The delta is internally consistent and introduces no new issues. The two prior findings remain STILL VALID at head and are carried forward: (1) Shield fee_strategy structural validation does not bounds-check DeductFromInput(index) or reject ReduceOutput(_); (2) the ShieldFromAssetLock execution proof does not cover the surplus_output address-balance write. CodeRabbit provided summary-only context (reviews paused) with 0 inline findings — nothing actionable.

🟡 2 suggestion(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-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs:207-214: [carried-forward] Shield fee_strategy validation accepts unusable steps
  Re-verified at head 792d1a51 — file unchanged. `ShieldTransitionV0::validate_structure` only checks `fee_strategy` for non-empty, max-length, and exact duplicates. It does not bounds-check `AddressFundsFeeStrategyStep::DeductFromInput(index)` against `self.inputs.len()`, and it silently accepts `AddressFundsFeeStrategyStep::ReduceOutput(_)` even though Shield has no outputs by construction. Because Shield is attacker-supplied network input, an out-of-range `DeductFromInput` or any `ReduceOutput(_)` becomes a silent no-op during fee deduction, and the transition is only rejected as a generic `AddressesNotEnoughFundsError` after expensive Orchard proof and witness validation has run on every node. The sibling `AddressFundsTransferTransitionV0::validate_structure` already performs exactly this bounds-check and emits `FeeStrategyIndexOutOfBoundsError`; Shield should match for cheap structural rejection and a more actionable error class.

In `packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs:390-407: [carried-forward] ShieldFromAssetLock execution proof does not cover the surplus_output credit
  Re-verified at head 792d1a51 — file unchanged; the proof branch still constructs only a single `PathQuery` over `RootTree::SpentAssetLockTransactions` for the asset-lock outpoint, and the matching verifier returns only `VerifiedAssetLockConsumed`. Meanwhile `ShieldFromAssetLockTransitionV0::surplus_output: Option<PlatformAddress>` is in the action path, and the action converter emits `AddressFundsOperation::AddBalanceToAddress` when `surplus_output.is_some()` and `surplus_amount > 0`. A light client can therefore verify the asset lock was consumed but has no proof that any non-zero surplus-address credit actually landed — a dishonest gateway/node could acknowledge execution while the surplus is silently misrouted on the prover's view. Compare the Unshield prove/verify pair (mod.rs:322-341 and verifier:1135-1177), which correctly merges `balances_for_clear_addresses_query` for the output address. Latent today only because orchestrated builders pin `surplus_output` to `None`/zero, but the proof surface should be completed before any wallet routinely produces a non-zero surplus. Fix direction: when `surplus_output.is_some()`, `PathQuery::merge` an additional balance-subtree path query for that address and extend the verifier result accordingly.

[thepastaclaw]

Code Review

Cumulative incremental review at 282c838. The latest delta (792d1a5..282c838) is a small, well-wired calibration: SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES 227→222, sized to the storage portion of the metered AddBalanceToAddress write, with synchronized fee constants, drive-abci unshield tests, and docs. No new defects in the delta. Both prior findings are STILL VALID at HEAD and are carried forward as suggestions: (1) Shield's structural fee_strategy validator does not bounds-check DeductFromInput and silently accepts ReduceOutput (no outputs exist on Shield); (2) the ShieldFromAssetLock execution-proof path does not cover the optional surplus_output balance write. CodeRabbit context: 0 inline findings (reviews auto-paused); nothing actionable to react to.

🟡 2 suggestion(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-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs:207-214: [carried-forward] Shield fee_strategy validation accepts unusable steps (no DeductFromInput bounds-check, no ReduceOutput rejection)
  Verified unchanged at HEAD 282c8380. `ShieldTransitionV0::validate_structure` checks `fee_strategy` only for empty, max-length, and exact-duplicate steps. It does NOT bounds-check `AddressFundsFeeStrategyStep::DeductFromInput(index)` against `self.inputs.len()`, and it silently accepts `AddressFundsFeeStrategyStep::ReduceOutput(_)` even though a Shield transition has no transparent outputs by construction.

  Because `fee_strategy` is attacker-supplied network input, an out-of-range `DeductFromInput` or any `ReduceOutput(_)` becomes a silent no-op when the shared deduction helper walks the strategy. The transition is then only rejected later as a generic `AddressesNotEnoughFundsError` — after the expensive Orchard proof verification and per-witness validation have already run on every validator. The sibling `AddressFundsTransferTransitionV0::validate_structure` already performs exactly this bounds-check and emits `FeeStrategyIndexOutOfBoundsError` with discriminator strings ("DeductFromInput" / "ReduceOutput"); Shield should mirror it for cheap structural rejection, more actionable errors, and DoS resistance. Not exploitable for funds loss today (fee accounting still settles correctly to a rejection), but the type system literally has a variant Shield can never use — exhaustive matching would catch this at compile time.

In `packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs:390-407: [carried-forward] ShieldFromAssetLock execution proof does not cover the surplus_output credit
  Verified unchanged at HEAD 282c8380. The `StateTransition::ShieldFromAssetLock` arm in `prove_state_transition_v0` still constructs only a single `PathQuery` over `RootTree::SpentAssetLockTransactions` for the asset-lock outpoint, and the matching verifier returns only `VerifiedAssetLockConsumed`.

  Meanwhile `ShieldFromAssetLockTransitionV0::surplus_output: Option<PlatformAddress>` is wired through this PR, and the action converter emits `AddressFundsOperation::AddBalanceToAddress { address: surplus_address, balance_to_add: surplus_amount }` whenever `surplus_output.is_some()` and `surplus_amount > 0`. A light/SDK client requesting an execution proof for a `ShieldFromAssetLock` with a non-zero surplus can therefore verify the asset lock was consumed, but has no cryptographic evidence that the surplus credit actually landed at the signed surplus-output address — a dishonest gateway/full node could acknowledge execution while the surplus is silently misrouted on the prover's view. Compare the Unshield prove/verify pair, which correctly merges a balance-subtree proof for the output address. Latent today only because the orchestrated single-recipient flow pins surplus to zero (and the FFI doc explicitly relies on that invariant), but the proof surface should be completed in parity with Unshield before any wallet routinely produces a non-zero surplus — otherwise v12 ShieldFromAssetLock with a surplus has materially weaker audit posture than every sibling pool-paying transition.

[thepastaclaw]

Code Review

Cumulative incremental review at HEAD 086b91d: prior finding #1 (Shield fee_strategy structural validation gap) is FIXED — the new validator bounds-checks DeductFromInput and rejects ReduceOutput with tests. Prior finding #2 (ShieldFromAssetLock execution proof omits the surplus_output balance subtree) is STILL VALID and unchanged in the latest delta; carried forward unanimously by 4 reviewers across both providers. One additional FFI panic-safety suggestion in the shared worker runtime helper consumed by the new shielded FFI entry points. No new latest-delta defects. CodeRabbit provided summary-only context with 0 inline findings (reviews paused on active branch); nothing actionable to react to.

🟡 2 suggestion(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-drive/src/prove/prove_state_transition/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs:390-407: [carried-forward] ShieldFromAssetLock execution proof does not cover the surplus_output credit
  Verified STILL VALID at HEAD 086b91d8 — unchanged by the latest delta. The `StateTransition::ShieldFromAssetLock` arm in `prove_state_transition_v0` (lines 390-407) constructs only a single `PathQuery` over `RootTree::SpentAssetLockTransactions` for the asset-lock outpoint, and the matching verifier returns only `VerifiedAssetLockConsumed`. Meanwhile this PR wires `ShieldFromAssetLockTransitionV0::surplus_output: Option<PlatformAddress>` through the action converter (`packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shield_from_asset_lock_transition.rs:60-69`), which emits `AddressFundsOperationType::AddBalanceToAddress { address: surplus_address, balance_to_add: v0.surplus_amount }` whenever `surplus_output.is_some()` and `surplus_amount > 0`.

  Impact: a light/SDK/WASM client requesting an execution proof for a `ShieldFromAssetLock` with non-zero surplus can cryptographically verify the asset lock was consumed, but has no proof that the surplus credit actually landed at the signed surplus-output address. A misbehaving full node could return a valid asset-lock-consumed proof while the surplus subtree write was silently omitted or diverted, and the verifier would still accept it. The wasm-dpp2 `VerifiedAssetLockConsumedWasm` surface inherits the same gap.

  Latent today because the orchestrated single-recipient flow pins surplus to zero, but the proof surface should be completed before any wallet routinely produces a non-zero surplus. Mirror the Unshield prove/verify pair, which merges an additional balance-subtree path query for the output address and extends the verifier result accordingly.

In `packages/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: block_on_worker re-raises worker-thread panics through C-ABI calls
  `block_on_worker` calls `rt.spawn(future).await.expect("tokio worker panicked")`, so any panic inside a future (e.g., proof generation, GroveDB access, host callback) propagates back into the calling `extern "C"` function instead of being mapped to `PlatformWalletFFIResultCode::ErrorWalletOperation`. This is consumed by the shielded FFI entry points introduced in this PR (`platform_wallet_manager_shielded_fund_from_asset_lock`, `_shielded_shield`, `_shielded_unshield`, `_shielded_withdraw`), which document a `PlatformWalletFFIResult` error channel — unwinding through that boundary aborts the Swift host process and bypasses the documented error path.

  Return a `Result` from the worker bridge (or wrap the await in `AssertUnwindSafe(...).catch_unwind()`) so worker panics become an FFI error result instead of crashing the host. Pre-existing helper, but now exercised by every new shielded FFI entry point.

[thepastaclaw]

Code Review

Incremental review at 78310f1. Prior finding #1 (ShieldFromAssetLock surplus_output proof gap) is FIXED — the prover merges the outpoint and surplus-address sub-queries and the verifier binds both sub-proofs to the same root, returning a new VerifiedAssetLockConsumedWithAddressInfos variant. Prior finding #2 (block_on_worker re-raises worker panics across the C ABI) is STILL VALID and carried forward (5/5 agents agree, file unchanged). One new latest-delta finding on the wasm-dpp2 wrapper's fromJSON path. CodeRabbit context had 0 actionable inline findings (reviews paused).

🟡 2 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: [carried-forward] block_on_worker re-raises tokio worker panics across the C ABI
  STILL VALID at HEAD 78310f1c (file unchanged since prior review). `rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") })` resumes any panic from the spawned future on the calling thread, which is the body of public `extern "C"` wallet entrypoints (e.g. `platform_wallet_manager_shielded_fund_from_asset_lock` in `shielded_send.rs` calls this helper). Rust panics must not unwind across a C ABI boundary — under the default `panic = "unwind"` strategy this is undefined behavior, and even with `panic = abort` what should be a recoverable internal failure (proof verification, GroveDB access, host callback) becomes a process abort on iOS/Swift hosts. The shielded-fees work in this PR routes new code paths through this helper, growing the surface. The crate has no `catch_unwind` upstream guard (verified by grep). Convert `JoinError` (including `is_panic()`) into the existing `PlatformWalletFFIResultCode::Error` channel, and wrap public `extern "C"` entrypoints in `std::panic::catch_unwind` so internal panics are reported as FFI errors rather than unwound across the ABI.

In `packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs:177-182: VerifiedAssetLockConsumedWithAddressInfos.fromJSON does not round-trip its toJSON output
  The newly added `VerifiedAssetLockConsumedWithAddressInfosWasm::to_json` (line 145-148) calls `normalize_js_value_for_json` so the `addressInfos` `Map` is serialised as a plain object survivable through `JSON.stringify`. `from_json` delegates to `from_object`, which reads `addressInfos` via `read_map_property` (line 17-21) — that helper does `raw.unchecked_into()` straight onto `js_sys::Map`. A JS client that does `JSON.parse(JSON.stringify(result))` and then calls `VerifiedAssetLockConsumedWithAddressInfos.fromJSON(...)` receives a value typed as `Map` but backed by a plain object, so `.size`/`.get()`/iteration on the proven surplus-output balance info will not behave as a Map. Either rebuild a real `Map` from the plain object inside `fromJSON` (iterate `Object.entries`), or skip the normalisation in `toJSON` and document the serialised shape — keep `toJSON` and `fromJSON` symmetric. Note: the same broken round-trip pattern exists in the sibling `VerifiedShieldedNullifiers*` types in this file, but this PR is the one introducing the new variant.

[QuantumExplorer]

Carried-forward bot finding — block_on_worker re-raises tokio worker panics across the extern "C" boundary (packages/rs-platform-wallet-ffi/src/runtime.rs): this file is not modified by this PR (pre-existing FFI code) and is already tracked as #3805. It is intentionally out of scope here; the automated reviewer re-surfaces it because the file is unchanged, not because this PR introduces it.

The new wasm-dpp2 fromJSON round-trip suggestion is fixed in 238f2b4 (the shared read_map_property helper now accepts a JSON-normalized plain object, fixing both address-info proof-result wrappers).

[thepastaclaw]

Code Review

Cumulative verification at cfd6ea1. Both prior findings reconciled as STILL VALID against unchanged source: (1) block_on_worker in rs-platform-wallet-ffi/src/runtime.rs:49-56 still re-raises tokio worker panics across extern "C" boundaries; (2) VerifiedAssetLockConsumedWithAddressInfos.fromJSON in wasm-dpp2 shielded.rs:177-182 still delegates to fromObject, which unchecked_intos addressInfos to js_sys::Map after toJSON normalised it to a plain object. No new latest-delta defects identified — the Step 11→7b cap-check move in ShieldFromAssetLock is sound (no intervening mutation of the consumed asset-lock value) and the new block-level conservation/sighash/boundary tests are well-targeted. CodeRabbit posted 0 inline findings (reviews auto-paused); nothing actionable from that source.

🟡 1 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: [carried-forward] block_on_worker re-raises tokio worker panics across the C ABI
  STILL VALID at cfd6ea1341 — file unchanged since the prior review. `rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") })` resumes any panic from the spawned future on the calling thread, which is the body of public `extern "C"` wallet entrypoints (e.g. `platform_wallet_manager_shielded_fund_from_asset_lock` in `shielded_send.rs` ultimately reaches this helper). Rust panics must not unwind across a C ABI boundary: under the default `panic = "unwind"` strategy this is undefined behavior, and even under `panic = abort` a recoverable internal failure (proof verification, GroveDB I/O, host-callback panic) becomes a process-wide abort on iOS/Swift hosts instead of a `PlatformWalletFFIResultCode::Error`. This PR routes new shielded code paths through the helper (Shield, ShieldFromAssetLock, Unshield, ShieldedWithdrawal builders + proof flows), widening the exposed surface; the crate has no `catch_unwind` guard at any public entrypoint. Convert `JoinError` (including `is_panic()`) into the existing `PlatformWalletFFIResultCode::Error` channel and wrap public `extern "C"` entrypoints in `std::panic::catch_unwind`.

[thepastaclaw]

Code Review

Latest delta (cfd6ea1..238f2b4) fixes the prior VerifiedAssetLockConsumedWithAddressInfos.fromJSON Map round-trip by teaching read_map_property to accept either a real Map or a plain object. Carried-forward prior finding #1 about block_on_worker resuming Tokio worker panics across the C ABI is unchanged at HEAD and remains valid. One additional in-scope round-trip bug surfaces: from_object reads initialCreditValue/remainingCreditValue with a bare JsValue::try_into::<u64>() that does not accept the numeric-string shape produced by toJSON(), so those credit fields are silently dropped after JSON.parse(JSON.stringify(...)).

🟡 2 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: block_on_worker re-raises Tokio worker panics across the C ABI
  Verified unchanged at HEAD 238f2b4e. `rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") })` resumes any panic from the spawned future on the calling thread, and the calling thread here is the body of public `extern "C"` wallet entrypoints (the shielded send/fund/withdraw FFI functions exercised by this PR's shielded fee work). A Rust panic must not unwind across a C ABI boundary: under `panic = unwind` this is undefined behavior, and under `panic = abort` an otherwise-recoverable internal failure becomes a host-process abort instead of being surfaced through the existing `PlatformWalletFFIResult` error channel. The shielded surface added/extended in this PR (real proof verification, note selection, asset-lock funding) widens the surface for an internal `unwrap`/`expect` to hit this path. Translate the `JoinError`/panic into the FFI error channel (preserving `JoinError::is_panic()` semantics) and/or wrap public extern entrypoints in `catch_unwind` so internal panics never cross the ABI.

In `packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs:177-194: fromJSON silently drops initialCreditValue/remainingCreditValue after a JSON round-trip
  `to_json()` runs the object through `normalize_js_value_for_json`, which converts `BigInt` to a JS string (see packages/wasm-dpp2/src/serialization/conversions.rs:106-113) so it survives `JSON.stringify`. After `JSON.parse(...)`, `initialCreditValue` and `remainingCreditValue` arrive as numeric strings. The current `read_opt_u64` closure does `v.try_into::<u64>().ok()`, which only accepts a `BigInt`/number JsValue — a numeric string produces `Err(_)` and `.ok()` silently maps it to `None`. The same round-trip path that the latest delta was fixing for `addressInfos` (Map -> plain object) therefore still loses the asset-lock credit values. Use the existing `crate::utils::try_to_u64` helper (utils.rs:326-379), which accepts BigInt, number, or numeric string, and propagate its error instead of swallowing it so malformed input fails loudly rather than silently producing `None`.

[thepastaclaw]

🟡 Suggestion: fromJSON silently drops initialCreditValue/remainingCreditValue after a JSON round-trip

to_json() runs the object through normalize_js_value_for_json, which converts BigInt to a JS string (see packages/wasm-dpp2/src/serialization/conversions.rs:106-113) so it survives JSON.stringify. After JSON.parse(...), initialCreditValue and remainingCreditValue arrive as numeric strings. The current read_opt_u64 closure does v.try_into::<u64>().ok(), which only accepts a BigInt/number JsValue — a numeric string produces Err(_) and .ok() silently maps it to None. The same round-trip path that the latest delta was fixing for addressInfos (Map -> plain object) therefore still loses the asset-lock credit values. Use the existing crate::utils::try_to_u64 helper (utils.rs:326-379), which accepts BigInt, number, or numeric string, and propagate its error instead of swallowing it so malformed input fails loudly rather than silently producing None.

Suggested change

	let read_opt_u64 = |name: &str| -> Option<u64> {
	js_sys::Reflect::get(&value, &name.into())
	.ok()
	.and_then(|v| {
	if v.is_undefined() || v.is_null() {
	None
	} else {
	v.try_into().ok()
	}
	})
	};
	Ok(VerifiedAssetLockConsumedWithAddressInfosWasm {
	status,
	initial_credit_value: read_opt_u64("initialCreditValue"),
	remaining_credit_value: read_opt_u64("remainingCreditValue"),
	address_infos: read_map_property(&value, "addressInfos")?,
	})
	}
	let read_opt_u64 = |name: &str| -> WasmDppResult<Option<u64>> {
	let v = js_sys::Reflect::get(&value, &name.into())
	.map_err(|_| WasmDppError::generic(format!("Missing property: {}", name)))?;
	if v.is_undefined() || v.is_null() {
	Ok(None)
	} else {
	crate::utils::try_to_u64(&v, name).map(Some)
	}
	};
	Ok(VerifiedAssetLockConsumedWithAddressInfosWasm {
	status,
	initial_credit_value: read_opt_u64("initialCreditValue")?,
	remaining_credit_value: read_opt_u64("remainingCreditValue")?,
	address_infos: read_map_property(&value, "addressInfos")?,
	})

_{source: ['codex']}

[thepastaclaw]

Code Review

Cumulative incremental review at 89efcbb. Reconciliation of 2 prior findings: (1) wallet-FFI block_on_worker panic propagation across the C ABI in packages/rs-platform-wallet-ffi/src/runtime.rs:49-56 is STILL VALID and carried forward; (2) wasm-dpp2 fromJSON round-trip drop of initialCreditValue/remainingCreditValue is FIXED in commit 5c0d27d — read_opt_u64 now accepts BigInt, base-10 strings, and safe integral f64. The new delta (merge of v3.1-dev pulling the storage-fee refund DivideByZero/underflow guard from PR #3799, plus the wasm fromJSON fix) introduces no new in-scope defects. One new latest-delta observation from codex-general about malformed credit values still silently becoming None is a DX-hardening concern not required for this PR's stated credit-conservation goal and is recorded as an out-of-scope follow-up. CodeRabbit provided summary-only context (reviews paused, 0 inline findings) — nothing actionable to react to.

🟡 1 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: Carried forward: block_on_worker re-raises Tokio worker panics across the C ABI
  STILL VALID at HEAD 89efcbbe4b — unchanged since the prior review at 238f2b4e. `rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") })` resumes any panic from the spawned future onto the calling thread, which is the body of public `extern "C"` wallet entrypoints (shielded_send.rs, identity_*, tokens/*, dpns.rs, data_contract.rs, dashpay*.rs, etc.). The crate has no `catch_unwind` wrapper at the FFI boundary, so a panic anywhere in the spawned async path (proof verification, GroveDB, decoding, signer/Orchard wiring) unwinds across a non-`extern "C-unwind"` boundary — undefined behavior, in practice an immediate process abort of the embedding iOS/Android app with no chance to surface a structured `PlatformWalletFFIResultCode::Error`. This PR materially increases the surface that flows through this helper via the new shielded-fee/credit-conservation wallet paths, keeping the prior finding live and relevant. Defense-in-depth: convert `JoinError` (including `is_panic()`) into the existing `PlatformWalletFFIResultCode::Error` channel, and wrap each public `extern "C"` entrypoint in `std::panic::catch_unwind` so internal panics become FFI errors instead of ABI-boundary unwinds.

[thepastaclaw]

Code Review

The latest proof/verification delta does not introduce a confirmed new issue. The prior FFI panic-boundary finding remains valid at 63c31f7 and is carried forward.

🟡 1 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:55: Handle Tokio worker panics before returning through C FFI
  `block_on_worker` awaits a spawned task and then calls `.expect("tokio worker panicked")` on the `JoinHandle`. If any async wallet/proof operation panics, this re-panics while an `unsafe extern "C"` wallet entrypoint is on the stack. The crate has no `catch_unwind` wrapper and does not use an `extern "C-unwind"` ABI, so an internal Rust panic can abort the host process instead of being translated into a structured `PlatformWalletFFIResultCode::Error*` result.

[thepastaclaw]

Code Review

Latest delta (63c31f7..c2f0ed6) is comment-only churn (Audit/reviewer wording → Invariant wording in tests/docs). No new in-scope findings in the latest delta. The prior finding on rs-platform-wallet-ffi/src/runtime.rs:55 (Tokio worker panic crossing the C ABI) is STILL VALID — the file is unchanged, and this PR materially expands the blast radius by adding six new unsafe extern "C" shielded entrypoints in shielded_send.rs that all funnel Halo 2 proof work through block_on_worker.

🟡 1 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: Carried forward (STILL VALID): tokio worker panic re-panics across the C FFI boundary
  Verified at head c2f0ed65: `block_on_worker` still resolves the spawned `JoinHandle` with `.expect("tokio worker panicked")`, and there is no `catch_unwind` at any `extern "C"` entrypoint in this crate. If any async wallet/proof routine panics on the worker, the `JoinError::Panic` is re-raised on the calling thread while an `unsafe extern "C"` frame is on the stack. Rust panics must not unwind across the C ABI — in practice this aborts the host (Swift/iOS) process instead of returning a structured `PlatformWalletFFIResult` error.

  This PR is the right time to address it because it materially widens the exposure. The new `packages/rs-platform-wallet-ffi/src/shielded_send.rs` adds six public extern entrypoints — `platform_wallet_manager_shielded_transfer`, `…_unshield`, `…_withdraw`, `…_shield`, `…_fund_from_asset_lock`, `…_resume_fund_from_asset_lock` — that all route Halo 2 circuit synthesis and proof verification through `block_on_worker`. Halo 2 / GroveDB code paths are exactly the kind of compute most likely to hit a panic (synthesis assertions, anchor lookup failures, deep recursion). A single bad input on any of these new entrypoints would take down the embedding app rather than failing the one call.

  Fix: convert `JoinError` (both `is_panic` and `is_cancelled`) into a structured FFI error, and ideally wrap each public `extern "C"` entrypoint in `std::panic::catch_unwind` so internal panics never cross the ABI.

[QuantumExplorer]

Reviewed

[thepastaclaw]

Code Review

Latest delta (c2f0ed6..788f515) adds two defense-in-depth checks in execute_event/v0 for PaidFromAddressInputs ops: a debug_assert that fee_refunds is empty, and a runtime fail-closed CorruptedCodeExecution if address-input fees aren't fully covered. The runtime check is well-formed; the debug_assert is asymmetric with it because it's stripped in release builds. The prior FFI worker-panic finding in rs-platform-wallet-ffi/src/runtime.rs is STILL VALID at the new head — block_on_worker still does .expect("tokio worker panicked") and no catch_unwind exists at the extern "C" boundary.

🟡 2 suggestion(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/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:49-56: Tokio worker panic re-panics across the C FFI boundary
  Carried forward from the prior review and re-validated at head 788f515a: `block_on_worker` still resolves the spawned `JoinHandle` with `.expect("tokio worker panicked")`, and no `extern "C"` entrypoint in this crate wraps its body in `std::panic::catch_unwind`. If any async wallet/proof routine panics on a tokio worker, the `JoinError::Panic` is converted into a fresh Rust panic on the calling thread while an `unsafe extern "C"` wallet/shield/fund entrypoint (called from Swift) is on the stack. Unwinding across `extern "C"` is UB on stable Rust and an abort under `panic=abort`, so an internal panic ends up crashing the host process instead of returning through the existing `PlatformWalletFFIResult` error channel. The PR substantially extends this async FFI surface (shielded_send, shield_from_asset_lock, fund_from_asset_lock builders, etc.), so the exposure grows here. Convert `JoinError`/panic into the existing FFI error result and wrap the public `extern "C"` entrypoints in `catch_unwind` so internal panics never cross the ABI.

In `packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs:153-156: Use a runtime guard, not debug_assert!, for the fee_refunds invariant
  The new `debug_assert!(individual_fee_result.fee_refunds.0.is_empty(), ...)` is compiled out in release, so validator binaries do not enforce it. The accompanying comment frames the invariant as load-bearing for credit conservation: a non-empty `fee_refunds` on a `PaidFromAddressInputs` event has no identity owner to refund at block fee distribution, so a future op that started freeing identity-attributed storage would silently misbook credits — the exact failure mode the `fee_fully_covered` guard immediately below is written to surface at the source (and which surfaces only as an opaque end-of-block `CorruptedCreditsNotBalanced` otherwise). Today no `Shield` / `IdentityCreateFromAddresses` op frees such storage, so this cannot trigger, but the asymmetry with the runtime check 30 lines below is the issue: a future PR adding such an op would pass release CI and ship a silent mint. Promote this to the same runtime fail-closed shape as the `fee_fully_covered` guard.

[thepastaclaw]

🟡 Suggestion: Use a runtime guard, not debug_assert!, for the fee_refunds invariant

The new debug_assert!(individual_fee_result.fee_refunds.0.is_empty(), ...) is compiled out in release, so validator binaries do not enforce it. The accompanying comment frames the invariant as load-bearing for credit conservation: a non-empty fee_refunds on a PaidFromAddressInputs event has no identity owner to refund at block fee distribution, so a future op that started freeing identity-attributed storage would silently misbook credits — the exact failure mode the fee_fully_covered guard immediately below is written to surface at the source (and which surfaces only as an opaque end-of-block CorruptedCreditsNotBalanced otherwise). Today no Shield / IdentityCreateFromAddresses op frees such storage, so this cannot trigger, but the asymmetry with the runtime check 30 lines below is the issue: a future PR adding such an op would pass release CI and ship a silent mint. Promote this to the same runtime fail-closed shape as the fee_fully_covered guard.

Suggested change

	debug_assert!(
	individual_fee_result.fee_refunds.0.is_empty(),
	"PaidFromAddressInputs ops must not free identity-attributed storage (fee_refunds must be empty)"
	);
	if !individual_fee_result.fee_refunds.0.is_empty() {
	return Err(Error::Execution(ExecutionError::CorruptedCodeExecution(
	"PaidFromAddressInputs ops must not free identity-attributed storage (fee_refunds must be empty)",
	)));
	}

_{source: ['claude', 'codex']}