fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554Claudius-Maginificent wants to merge 92 commits into
Conversation
`auto_select_inputs` in `wallet/platform_addresses/transfer.rs` was
inserting each selected address with its FULL balance as the input's
`Credits` value, then returning as soon as accumulated covered
`output + fee`. With a bank holding ~500B credits and a 50M output, the
SDK got `inputs = {bank: 499_985_086_740}, outputs = {target: 50_000_000}`
and the protocol rejected it because address-funds-transfer enforces
`Σ inputs.credits == Σ outputs.credits` (strict equality, verified at
`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs`,
asserted on-chain by
`rs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly`,
which checks `new_balance == initial_balance - transfer_amount - fee`).
The protocol's actual semantics:
- `inputs[addr].credits` = consumed amount from `addr`
- `outputs[addr]` = credited amount to `addr`
- `Σ inputs.credits == Σ outputs.credits`
- Fee is deducted from the targeted input's REMAINING balance (post-
consumption) per `AddressFundsFeeStrategy`. `DeductFromInput(0)`
reduces the *remaining balance* by the fee — never the inputs map's
`Credits` value.
Fix: extract the selection loop into a pure module-scope helper
`select_inputs(candidates, outputs, total_output, fee_strategy,
platform_version)` that:
1. Walks candidates in DIP-17 order, tentatively appending each to a
`Vec<(address, balance)>` to drive the per-iteration fee estimate.
2. Stops when `accumulated >= total_output + estimated_fee` (the
accumulated balance must cover the fee from the last input's
remaining balance).
3. Builds the returned map front-to-back, consuming each input in
insertion order until exactly `total_output` is reached. Inputs
added solely to satisfy the per-input fee margin are excluded
from the final map — preserving Σ inputs.credits == total_output
without violating `min_input_amount`.
Side benefits:
- The pure helper is unit-testable without constructing a full
`PlatformWalletManager` + `PlatformAddressWallet`. Five tests cover
the fix:
- `single_input_oversized_balance_trims_to_output_amount`
- `two_input_selection_trims_only_the_last`
- `fee_only_tail_input_does_not_inflate_input_sum` (regression for
the Σ-inputs-greater-than-Σ-outputs case raised in Copilot review)
- `insufficient_balance_errors`
- `no_candidates_errors`
- The full per-`PlatformAddressWallet` async method `auto_select_inputs`
now just gathers `(address, balance)` candidates and calls
`select_inputs`, which keeps the testability win without changing
public API.
Doc note in `auto_select_inputs_for_withdrawal` clarifies the
asymmetry: withdrawal validates `Σ inputs > output_amount` (strictly
greater, surplus = fee), so its drain-everything strategy is correct
by design — NOT the same bug as the transfer selector. No code
change there.
Verification:
- `cargo check --tests -p platform-wallet` OK
- `cargo clippy --tests -p platform-wallet -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --lib` 115/115
Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestricts auto input-selection to a single fee-strategy shape and replaces the greedy selector with a deterministic, tested pure selector; plus small refactors and test/adaptations and a package-filter addition. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 30e612b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 341-365: When building the final `selected` map after `accumulated
>= required`, ensure the address referenced by the fee strategy
`DeductFromInput(index)` is reserved the estimated fee before returning: locate
the `chosen` prefix and the loop that constructs `selected` using
`remaining`/`total_output`, compute the required fee headroom for the
fee-bearing input (from `fee_strategy`/`DeductFromInput`) and reduce that
input's available amount by that fee (i.e., instead of consuming up to
`remaining`, cap consumption so the fee-bearing input keeps at least
`estimated_fee`), and if that causes the input to be insufficient, continue
selecting additional candidates or return an error; update uses of `selected`,
`remaining`, `accumulated`, and `required` accordingly so the returned map
guarantees the fee-bearing input still has the reserved fee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56d75786-5d1c-44b8-a304-2e962f372120
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
lklimek
changed the title
lklimek
changed the title
lklimek
changed the title
…arget CodeRabbit caught a critical bug on PR #3554's `select_inputs`: the helper ensured `Σ inputs.credits == Σ outputs.credits` (the protocol's structural invariant) but did NOT ensure that the address targeted by `DeductFromInput(0)` had post-consumption remaining balance >= the estimated fee. Worked example from CodeRabbit: candidates = [(addr_a, 20M), (addr_b, 50M)] // addr_a < addr_b lex total_output = 30M fee_strategy = [DeductFromInput(0)] Old result = {addr_a: 20M, addr_b: 10M} // Σ matches; addr_a drained Drive applies DeductFromInput(0) over inputs sorted by key (BTreeMap order), hitting addr_a — whose remaining balance is 0 — so `min(fee, 0) = 0`, `fee_fully_covered = false`, validator rejects with AddressesNotEnoughFundsError. The Wave-8 single-input live e2e accidentally avoided this because the fee target had ~1B credits left over after consumption — multi-input auto-selected transfers would have hit it on first contact. This rewrite: - Phase 1 (unchanged): pick smallest DIP-17-ordered prefix covering total_output + estimated_fee. - Phase 2: identify the fee target = lex-smallest address in the prefix (= `BTreeMap` index 0, what `DeductFromInput(0)` will hit per `rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`). - Phase 3: consume the *minimum* allowed amount from the fee target (`max(min_input_amount, total_output − Σ other balances)`) so it retains the most remaining balance for fee deduction. Error out with a descriptive AddressOperation if even that minimum leaves less than `estimated_fee` remaining. - Phase 4: distribute the rest of `total_output` across the other prefix entries in DIP-17 order. - Phase 5: defensive invariant checks. `min_input_amount` is fetched from `platform_version.dpp.state_transitions.address_funds.min_input_amount` (currently 100k across v1/v2/v3 of platform-version). For non-`[DeductFromInput(0)]` fee strategies the helper falls back to the previous "consume from front" distribution that only enforces the Σ invariant — none of the wallet's call sites use anything else today. Tests: - updated `two_input_selection_trims_only_the_last` → `two_input_selection_keeps_fee_headroom_at_index_zero` to assert the new distribution AND the headroom invariant. - updated `fee_only_tail_input_does_not_inflate_input_sum`'s expected outputs (the tail is no longer dropped — it absorbs the consumption the fee target sheds). - added `fee_target_keeps_remaining_for_fee_deduction` (CodeRabbit's exact scenario, with the headroom invariant as the load-bearing assertion). - added `fee_headroom_violation_errors` (lex-smallest address too small to retain headroom → descriptive error rather than transition the validator will reject). - `single_input_oversized_balance_trims_to_output_amount`, `insufficient_balance_errors`, `no_candidates_errors` pass unchanged. `cargo test -p platform-wallet --lib` → 117 / 117 green `cargo clippy -p platform-wallet --tests -- -D warnings` → clean `cargo fmt -p platform-wallet --check` → clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 441-461: The distribution loop that sets each consumed = min(bal,
remaining) can insert inputs below min_input_amount (e.g., consumed <
min_input_amount); after that loop (the one that inserts entries into prefix and
uses variables fee_target_consumed, remaining, consumed, fee_target_addr),
validate all non-zero consumed values against min_input_amount and either (a)
rebalance by moving consumption from fee_target_consumed or other large entries
to bump small entries up to min_input_amount while preserving total consumption,
or (b) return an error indicating distribution impossible; implement the
simplest correct choice for your flow (prefer returning an error if safe
redistribution is complex) and ensure the function returns early on failure so
subsequent debug_asserts aren’t relied on.
- Around line 349-353: The code currently silently falls back to front-trimming
for any AddressFundsFeeStrategy other than the single-item [DeductFromInput(0)]
inside transfer()/where InputSelection::Auto is handled; change this to reject
unsupported auto-selection fee strategies by returning a clear error instead of
performing the unsafe fallback. Locate the branch handling InputSelection::Auto
in transfer.rs (the block that examines fee_strategy and falls back to
front-trimming) and add a guard that checks the strategy sequence—if it is not
exactly the single DeductFromInput(0) pattern, return an Err (with a descriptive
enum/variant or mapped error) indicating unsupported fee strategy for
auto-selection so callers cannot produce inputs that sum to outputs but will
fail on-chain once fees are applied. Ensure the new error flows out of
transfer() consistently with existing error types.
- Around line 363-385: The loop that builds the DIP-17-ordered prefix (variables
prefix, accumulated, covered) currently breaks out as soon as accumulated >=
required, which prevents trying larger prefixes when Phase 3 (fee target
feasibility using fee_target_min/fee_target_max and DeductFromInput(0)
semantics) fails; change the logic so that when a covering prefix is found you
run Phase 3 checks but do not return/error on Phase 3 failure—continue the for
(address, balance) in candidates iteration (calling estimate_fee_for_inputs_pub
as before) to grow the prefix and try later candidates until either Phase 3
succeeds or all candidates are exhausted, only then set covered/error
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 857ac9b6-556f-4771-a503-d3c0069a9ecb
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
…ee-headroom bug Adds `pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction` to the `select_inputs` test module. Reconstructs the exact `inputs` map the pre-fix `auto_select_inputs` would have returned for CodeRabbit's example (candidates (20M, 50M), total_output 30M, `DeductFromInput(0)`), runs the post-consumption remaining balances through the live dpp fee-deduction code path, and asserts `fee_fully_covered == false` — i.e. the protocol rejects it with `AddressesNotEnoughFundsError`. Distinct from `fee_target_keeps_remaining_for_fee_deduction`, which asserts the new selector's output meets the headroom invariant. This reproduction proves the bug at the protocol layer rather than merely asserting "the new output looks different" — it would have stayed red without the fix in 9ea9e70. Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 118/118 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…descending Internal-only change to `auto_select_inputs`. Candidates were previously collected in DIP-17 derivation index order; now they sort by balance descending before being handed to `select_inputs`. Mirrors the dash-evo-tool allocator (`src/ui/wallets/send_screen.rs:155-157`). Effects: - Single largest balance covering `total_output + estimated_fee` => 1-input result, no multi-input case, no lex-smallest fee headroom logic firing. Common path simplified. - Multi-input cases (when the largest alone isn't enough) still go through the headroom-respecting distribution introduced in 9ea9e70 — unchanged, still correct. - No public API change. `transfer()`, `auto_select_inputs`, `select_inputs` signatures all identical. Adds `descending_order_picks_single_largest_when_sufficient` to the existing test module to lock in the common-path behavior. Other tests pass candidates directly to `select_inputs` and are order-agnostic by design — unchanged. The `fee_headroom_violation_errors` error message now includes the fee-target address, its balance, required headroom, and remaining-after-consumption to ease debugging. Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 119/119 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (2)
442-453: Fallback path for non-[DeductFromInput(0)]strategies still risks on-chain rejection.The docstring at lines 371-375 acknowledges this limitation, but the code silently proceeds with a distribution that only guarantees
Σ inputs == Σ outputswithout reserving fee headroom on the actual fee-bearing input. Iftransfer()is ever called with a different fee strategy (e.g.,DeductFromInput(1)or multi-step strategies), the returned inputs map could still fail on-chain when the targeted input lacks remaining balance for fee deduction.A previous review suggested returning an error for unsupported strategies. The current approach documents the limitation but doesn't prevent misuse. Consider whether rejecting unsupported strategies is preferable to silent fallback with potential on-chain failure.
Alternative: Reject unsupported strategies explicitly
if !single_deduct_from_input_zero { - let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); - let mut remaining = total_output; - for (addr, bal) in prefix.iter() { - if remaining == 0 { - break; - } - let consumed = (*bal).min(remaining); - selected.insert(*addr, consumed); - remaining = remaining.saturating_sub(consumed); - } - return Ok(selected); + return Err(PlatformWalletError::AddressOperation( + "Auto input selection currently supports only [DeductFromInput(0)] fee strategy. \ + Other strategies require explicit input selection.".to_string(), + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 442 - 453, The fallback branch that builds `selected` when `!single_deduct_from_input_zero` must not silently return a transfer that may fail on-chain; instead make `transfer()` validate the fee strategy up-front and return an explicit error for unsupported strategies (e.g., when the strategy is not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is false). Replace the current loop-return branch that constructs `selected` with an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or similar), and ensure callers handle that error; keep the `prefix`-consumption logic only for the supported `single_deduct_from_input_zero` path.
405-409: Phase 1 early break may cause false "fee headroom" failures when a larger prefix would succeed.A previous review noted that because
DeductFromInput(0)targets the lex-smallest address (not the first in iteration order), a later candidate joining the prefix can become the new fee target and make an otherwise-infeasible selection work. The current code breaks at the first covering prefix without checking Phase 3 feasibility.With the descending-balance sort, this scenario is less common (the first candidates are the largest balances), but it can still occur when:
- The largest-balance address is also lex-smallest
- That address has just enough to cover
total_outputbut nottotal_output + fee- Adding a lex-smaller address would shift the fee-target role to a smaller-balance address with better headroom characteristics
Consider continuing to accumulate candidates until Phase 3 succeeds or all candidates are exhausted, rather than erroring immediately on the first Phase 3 failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 405 - 409, The early break on the first prefix that meets accumulated >= required causes false failures because Phase 3 (fee-target reassignment via DeductFromInput(0)) may succeed for a larger prefix; in the loop that checks accumulated, required and sets covered=true then break, remove the immediate break and instead, after accumulated >= required, invoke the Phase 3 feasibility check (the same logic that uses DeductFromInput(0) / the fee-target selection) and only set covered=true and stop accumulating if that Phase 3 check succeeds; if Phase 3 fails, continue accumulating more candidates and only error or mark covered=false after all candidates are exhausted. Ensure you update the variables used in the Phase 3 check to reflect the extended prefix and keep the existing semantics for setting covered and returning an error when no prefix passes Phase 3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 509-521: The loop that distributes `remaining` across `prefix` may
insert entries below `min_input_amount` because it uses `consumed =
bal.min(remaining)` without validating per-input minimum; update the Phase 4
loop (the block iterating `for (addr, bal) in prefix.iter()`) to only insert a
`consumed` value if `consumed >= min_input_amount`, otherwise do not insert that
address and instead add the small remainder to the `fee_target_addr` (or
accumulate it to be merged into the fee target while preserving any fee
headroom) so no non-fee-target input can be below `min_input_amount`; retain
existing `remaining`/`selected` semantics and keep the Phase 5 `debug_assert`
intact.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 442-453: The fallback branch that builds `selected` when
`!single_deduct_from_input_zero` must not silently return a transfer that may
fail on-chain; instead make `transfer()` validate the fee strategy up-front and
return an explicit error for unsupported strategies (e.g., when the strategy is
not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is
false). Replace the current loop-return branch that constructs `selected` with
an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or
similar), and ensure callers handle that error; keep the `prefix`-consumption
logic only for the supported `single_deduct_from_input_zero` path.
- Around line 405-409: The early break on the first prefix that meets
accumulated >= required causes false failures because Phase 3 (fee-target
reassignment via DeductFromInput(0)) may succeed for a larger prefix; in the
loop that checks accumulated, required and sets covered=true then break, remove
the immediate break and instead, after accumulated >= required, invoke the Phase
3 feasibility check (the same logic that uses DeductFromInput(0) / the
fee-target selection) and only set covered=true and stop accumulating if that
Phase 3 check succeeds; if Phase 3 fails, continue accumulating more candidates
and only error or mark covered=false after all candidates are exhausted. Ensure
you update the variables used in the Phase 3 check to reflect the extended
prefix and keep the existing semantics for setting covered and returning an
error when no prefix passes Phase 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b42e9a8-3597-407f-9ca0-e9a419a38732
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR fixes the original Σ inputs == Σ outputs bug, but introduces three new protocol violations. The selector reasons about fee headroom in DIP-17 insertion order while the chain applies DeductFromInput(i) over BTreeMap key order — combined with dropping/draining tail inputs, this leaves the actual fee-bearing input with no remaining balance. The trim can also produce inputs below min_input_amount (100_000). The new tests assert only the structural invariant and would not catch any of these regressions.
Reviewed commit: aaf8be7
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 321-365: Selector reserves fee headroom in insertion order, but the protocol charges DeductFromInput(0) against the BTreeMap-first input
`select_inputs` keeps `chosen` in insertion (DIP-17) order and consumes from front to back, fully draining every input except the last to reach exactly `total_output`. It returns a `BTreeMap`, however, and the on-chain validator resolves `DeductFromInput(index)` via `remaining_balances.iter().nth(index)` (verified at `rs-dpp/.../state_transition_estimated_fee_validation.rs:48-69`), i.e. against BTreeMap key order — the lex-smallest selected address.
When the BTreeMap-first address is not the same as the insertion-order tail, all fee headroom ends up on the wrong input. The new test `two_input_selection_trims_only_the_last` demonstrates the failure mode directly: `addr_a = [0x01;20]` (BTreeMap key 0) is consumed for its full 20M balance — remaining = 0; `addr_b` keeps 40M of headroom. With `DeductFromInput(0)` the protocol charges the fee to `addr_a`, which has 0 left, so `fee_fully_covered = false` and the transition is rejected with `AddressesNotEnoughFundsError` (`rs-drive-abci/.../validate_fees_of_event/v0/mod.rs:209-224`).
The `fee_only_tail_input_does_not_inflate_input_sum` test exposes the same root cause via dropping rather than draining: `addr_a` has `total_output + 1` and is consumed for `total_output`, leaving 1 credit of remaining balance on the only returned input — far below any realistic transfer fee. The aggregate guarantee `Σ remaining ≥ fee` is irrelevant because the protocol charges the fee from one specific input, not the aggregate.
The helper must guarantee that the input the protocol will actually charge (BTreeMap-first when `fee_strategy = [DeductFromInput(0)]`) retains at least `estimated_fee` of remaining balance after consumption. Practical options: (a) compute the BTreeMap-first selected address up front and direct the trim residue to it, leaving headroom there; (b) restrict consumption on the fee-target input to `balance − estimated_fee`. The unit tests need to assert remaining balances after consumption, not just `Σ values == total_output`, to catch this class of bug.
- [BLOCKING] lines 351-365: Trimmed tail input can fall below `min_input_amount`, causing structural rejection
The trim sets `consumed = (*bal).min(remaining)` per input, with no lower bound. The transfer validator rejects any input below `platform_version.dpp.state_transitions.address_funds.min_input_amount` (currently 100_000) with `InputBelowMinimumError` (`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs:146-167`).
A perfectly normal multi-input scenario triggers this: candidates A=5M, B=100M with `total_output = 5_050_000` yields `{A: 5_000_000, B: 50_000}`, and 50_000 < 100_000. Or two 100k inputs funding a 150k output: `{first: 100_000, second: 50_000}` — also rejected. None of the new tests exercise this boundary.
Fix options: filter candidates with `balance >= min_input_amount` at the call site and either skip a tail that would trim below the minimum (rolling residue back into a prior input) or refuse to add a candidate whose post-trim consumption would violate the limit. The helper aims to encode protocol correctness, so this should be enforced inside `select_inputs` rather than left to the validator.
- [SUGGESTION] lines 412-539: Tests assert the input-sum invariant only, not full state-transition validation
`single_input_oversized_balance_trims_to_output_amount`, `two_input_selection_trims_only_the_last`, and `fee_only_tail_input_does_not_inflate_input_sum` all assert only that `Σ values == total_output`. They never assemble the result into an `AddressFundsTransferTransition` and run it through `validate_structure` + the fee-validation pipeline. As a result they pass on inputs maps that the on-chain validator deterministically rejects (see the two blocking findings above). The dpp validators are pure and don't need a live node — wiring an integration assertion into these tests would catch both classes of regression and is the right substitute for the still-pending testnet rerun referenced in the PR description.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The DeductFromInput(0) headroom bug from the prior review is fixed: the selector now targets the BTreeMap-first input via lex-smallest selection, and the new pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction test exercises dpp's actual fee-deduction path. Two prior blockers remain reachable on the current head, however: Phase 4's distribution across non-fee-target inputs has no min_input_amount floor, and the public-API fallback for fee strategies other than [DeductFromInput(0)] produces input maps the validator deterministically rejects.
Reviewed commit: 60f7850
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 504-521: Non-fee-target inputs can still be trimmed below `min_input_amount`
Phase 3 pins `fee_target_min ≥ min_input_amount` (line 484), but Phase 4 then distributes `total_output − fee_target_consumed` across the non-fee-target prefix entries with `let consumed = (*bal).min(remaining)` and inserts any positive value (lines 516-520) — there is no per-input lower bound. The validator at `packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167` iterates `self.inputs.values()` and rejects ANY input below `min_input_amount` (100_000) with `InputBelowMinimumError`.
A construction that survives Phase 1 and Phase 3 but trips the validator:
- candidates after balance-desc sort: `[(addr_X=0x01, 1_000_000), (addr_Y=0x02, 30_000)]`
- `total_output = 950_000`, `min_input_amount = 100_000`
- Phase 1 needs both inputs (e.g. `fee_for_1 = 60_000` → 1_000_000 < 1_010_000; `fee_for_2 = 80_000` → 1_030_000 ≥ 1_030_000 ✓)
- `fee_target = addr_X`, `fee_target_max = 920_000`, `other_total = 30_000`, `fee_target_min = max(100_000, 920_000) = 920_000` — 920_000 ≤ 920_000 ✓
- Phase 4: `remaining = 30_000` → `addr_Y` inserted as 30_000 < 100_000 → rejected.
Fix options: filter candidates with `balance < min_input_amount` at the call site; refuse to insert a non-fee-target consumption that would land below the minimum (rolling residue back to the fee target, which already has remaining headroom); or bail out with a descriptive error. The existing `fee_headroom_violation_errors` test exercises only the fee-target min-input path; an analogous test for the tail input would catch this.
- [BLOCKING] lines 442-454: Fallback path is reachable via public API and produces protocol-invalid input maps
`transfer()` (line 31) is `pub` and accepts an arbitrary `fee_strategy: AddressFundsFeeStrategy` from any caller. With `InputSelection::Auto`, this routes through `select_inputs()`, which only implements protocol-correct logic for the exact shape `[DeductFromInput(0)]` (line 437-440). For every other strategy, lines 442-453 fall back to a front-consume distribution that guarantees only `Σ inputs == total_output` and ignores both fee-target headroom and `min_input_amount`.
Reachable failure: with `fee_strategy = [DeductFromInput(1)]`, candidates `[(addr_b, 20M), (addr_a, 50M)]` where `addr_a < addr_b`, and `total_output = 30M`, the fallback returns `{addr_b: 20M, addr_a: 10M}`. The protocol resolves `DeductFromInput(i)` against BTreeMap key order (`packages/rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`), so index 1 points at `addr_b`, which is fully drained — fee deduction fails exactly like the original bug. `ReduceOutput(...)` strategies can produce structurally invalid trailing inputs for the same reason.
The doc on lines 371-375 acknowledges this as 'must be revisited if [strategy] changes', but the public API surface is wide open today. Either constrain the strategy at the entry point, return an explicit `Err` for unsupported shapes, or extend the fee-target/min-input logic to general strategies. Returning a known-suspect map silently is the riskier option — it forces a future caller to stumble into the same protocol rejection that motivated this PR.
- [SUGGESTION] lines 484-545: `total_output < min_input_amount` falls through to misleading 'Internal selection error'
When `total_output < min_input_amount` (e.g. caller asks to transfer 50_000 credits with min_input=100_000), the 1-input path computes `fee_target_min = max(min_input_amount, total_output) = 100_000 > total_output`, so `selected = {addr: 100_000}` and `input_sum = 100_000 ≠ total_output`. Phase 4's loop runs once with `remaining = total_output.saturating_sub(100_000) = 0`, then the flow trips the `debug_assert_eq!` at line 527 in debug builds and falls through to the line-538 'Internal selection error' branch in release.
The protocol disallows any transfer with `total_output < min_input_amount` (no input set can satisfy both `Σ inputs == total_output` and per-input `≥ min_input_amount`). This deserves an early, descriptive error like 'Transfer amount X below minimum Y' rather than the internal-error path that's documented as 'should never trip'. Add an early check at the top of `select_inputs` (or in the `transfer` entry-point on `outputs.values().sum()`).
…egy, retry on Phase 3 fail Addresses the second wave of review findings on PR #3554: 1. [BLOCKING] Phase 4 distribution no longer produces inputs below `min_input_amount`. `auto_select_inputs` now filters candidates with `balance < min_input_amount` upfront — they cannot legally appear in the inputs map. In Phase 4, when a non-fee-target tail entry would consume less than `min_input_amount`, the residue rolls back into the fee target's consumption (which has surplus headroom by construction). Returns a descriptive error if rollback would violate the fee-target headroom invariant. 2. [BLOCKING] `transfer()` rejects unsupported `fee_strategy` shapes for `InputSelection::Auto`. Auto-select currently only implements protocol-correct logic for `[DeductFromInput(0)]`; any other strategy returns `PlatformWalletError::AddressOperation` with a clear message redirecting callers to `InputSelection::Explicit`. Explicit paths still accept arbitrary strategies (caller's responsibility). 3. [BLOCKING] When Phase 3 (`fee_target_min > fee_target_max`) fails in `select_inputs`, the algorithm now extends the prefix with the next candidate and retries instead of erroring out. Larger prefixes may yield a different lex-smallest fee target with sufficient headroom. Errors out only when candidates are exhausted and no covering prefix is feasible. 4. [SUGGESTION] `select_inputs` returns an early descriptive error when `total_output < min_input_amount` — the protocol forbids this regardless of input shape, so an explicit error beats the internal "should never trip" branch that some callers were reaching. 5. [SUGGESTION] Existing selector tests now also build a minimal `AddressFundsTransferTransitionV0` and run `validate_structure`, asserting protocol-level validity in addition to the `Σ inputs == total_output` invariant. Catches future regressions without needing a live node. Coderabbit findings DUuz (#3554), DUu1 (#3554), E5L5 (#3554), thepastaclaw findings F9fo, GMHz, GMH5, GMH_, F9fv addressed. Outdated F9fk references the renamed test from before 9ea9e70. Nitpicks F9fz/GMID/F9f5/GMIH deferred (unreachable / low value). Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 121/121 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (1)
607-631: Phase 5 invariant checks are partially release-protected.The
debug_assertstatements at lines 611-624 verify critical protocol invariants:
Σ inputs == total_output— also checked in release at line 626 ✓- Fee target is BTreeMap index-0 (lex-smallest)
- Fee target retains ≥
estimated_feeremaining balance- Every input ≥
min_input_amountInvariants 2-4 are only asserted in debug builds. If any of these fail in release (due to a future regression in Phase 1-4 logic), the transition would be submitted and rejected by the protocol layer rather than caught here.
Given the algorithm's structure and test coverage, these should never trip. However, promoting invariant 3 (the fee headroom check) to a release-mode error would provide defense-in-depth for the exact bug this PR fixes.
♻️ Optional: Add release-mode check for fee headroom
if input_sum != total_output { return Err(PlatformWalletError::AddressOperation(format!( "Internal selection error: Σ inputs ({}) != total_output ({})", input_sum, total_output ))); } + + let fee_target_remaining = fee_target_balance.saturating_sub(fee_target_consumed); + if fee_target_remaining < estimated_fee { + return Err(PlatformWalletError::AddressOperation(format!( + "Internal selection error: fee target {} remaining {} < estimated fee {}", + format_address(&fee_target_addr), + fee_target_remaining, + estimated_fee, + ))); + } Ok(selected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 607 - 631, The fee-headroom check currently uses debug_assert! on fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which is only active in debug builds; replace that debug-only assertion with a release-mode runtime check inside the same scope (where selected, fee_target_balance, fee_target_consumed and estimated_fee are available) so that if the condition fails you return Err(PlatformWalletError::AddressOperation(...)) with a clear message including the computed headroom and required estimated_fee; keep the other debug_asserts as-is and ensure the new check mirrors the existing error style used for input_sum != total_output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 607-631: The fee-headroom check currently uses debug_assert! on
fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which
is only active in debug builds; replace that debug-only assertion with a
release-mode runtime check inside the same scope (where selected,
fee_target_balance, fee_target_consumed and estimated_fee are available) so that
if the condition fails you return
Err(PlatformWalletError::AddressOperation(...)) with a clear message including
the computed headroom and required estimated_fee; keep the other debug_asserts
as-is and ensure the new check mirrors the existing error style used for
input_sum != total_output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e663ab0e-42e2-48de-b207-cd8e4fa9740b
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
…allet changes Adds `rs-platform-wallet` as a filter entry in `.github/package-filters/rs-packages-no-workflows.yml`. Without this, crate-only changes under `packages/rs-platform-wallet/` evaluate to `rs-packages = '[]'` and the `rs-workspace-tests` job in `.github/workflows/tests.yml` gates off — meaning the crate's unit tests never run in CI when only that crate is touched. This gap surfaced on PR #3554 itself: five commits, 121 unit tests, none of them executed by `Rust workspace tests` (all reported as SKIPPED). Local `cargo test -p platform-wallet --lib` was the only validation. Reviewers seeing "all green" could miss that the actual Rust validation was skipped. The filter entry mirrors the existing pattern: list the crate path and inherit the SDK alias (`*sdk`) so transitive SDK changes also trigger workspace tests for the wallet, matching how `wasm-sdk` and `rs-sdk-ffi` are wired. Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…allet-auto-select-inputs
…now run) The CI filter addition in 79c2b28 made `Rust workspace tests` run on `rs-platform-wallet` for the first time in a while, surfacing three pre-existing breaks that the silently-skipped pipeline had been accumulating: 1. `src/changeset/core_bridge.rs` (`build_core_changeset`) — `field_reassign_with_default` lint. `let mut cs = CoreChangeSet::default(); cs.new_utxos = ...; cs.spent_utxos = ...;` replaced with a struct literal carrying the derived values plus `..CoreChangeSet::default()` for forward-compat fields. 2. `src/wallet/apply.rs:316` — `let_unit_value` lint. `WalletInfoInterface::update_balance` returns `()`; the `let _ = ...` discards a unit value. Calling the method directly is the intended shape. 3. `tests/spv_sync.rs:74-78` — stale field access. The integration test still walked `core.chain.synced_height` even though `CoreChangeSet` was flattened (see existing rustdoc on `synced_height` direct field). Replaced with `core.synced_height` directly. None of these are bugs — clippy hardening and a stale test field that `cargo test --lib` never compiled. Verified: - `cargo clippy --workspace --tests -- -D warnings` clean - `cargo clippy -p platform-wallet --tests -- -D warnings` clean - `cargo test -p platform-wallet --lib` 121/121 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
… work Apply claudius:coding-best-practices rules: length cap (<=2 preferred, 3 mediocre), present-state only (no Wave/PR-number history), two-tier (strict for internal, liberal for public API rustdoc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o-select Extends transfer() / auto_select_inputs to accept [ReduceOutput(0)] in addition to [DeductFromInput(0)]. Output 0 absorbs the fee, so input selection skips the fee-headroom reservation. Σ inputs == Σ outputs invariant preserved via last- input trim. 5 new tests in auto_select_tests cover happy path, multi-input trim, multi- output isolation, output-too-small error, and structural validation. Resolves PR #3549 thread r-aCky's production prerequisite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3554 +/- ##
=========================================
Coverage 87.17% 87.17%
=========================================
Files 2607 2607
Lines 319489 319489
=========================================
Hits 278507 278507
Misses 40982 40982
🚀 New features to boost your workflow:
|
…allet-auto-select-inputs # Conflicts: # packages/rs-platform-wallet/src/changeset/core_bridge.rs
lklimek
added
the
ready for final review
…educeOutput Phase 4 Annotates `select_inputs_reduce_output`'s Phase 4 fee-headroom check to document the known dpp-layer bug (platform #3040) where `estimate_min_fee` models only the static state_transition_min_fees floor and excludes storage + processing costs. For small `output[0]`, the auto-selector greenlights selections that then fail on-chain with AddressesNotEnoughFundsError. Comment-only — no behaviour change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…allet-auto-select-inputs
…ct duplicate input addresses (PR #3554 CMT-003) Mirror of the parse_outputs hardening shipped in 2eab4f7 — both explicit-input parsers previously called map.insert(addr, ...) unconditionally, so a duplicate ExplicitInputFFI/ExplicitInputWithNonceFFI silently overwrote the earlier entry. The wallet would then compute the input sum against an under-populated map, leaving the resulting state transition misbalanced. The Swift wrapper guards against this today, but Rust FFI is the authoritative gate — reject explicitly, naming the offending address hash so the caller can identify which input collided. Switched return type to Result<_, String> matching the parse_outputs precedent; no caller adjustments needed (unwrap_result_or_return! already routes through .into() and the From<String> impl was added in 2eab4f7). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Status update on the 2026-05-25T14:00 review @thepastaclaw
CMT-001 — SPV shutdown deadlock (defer)Pre-existing v3.1-dev bug. The lock-inversion in CMT-002 — Swift
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the checked-out head at 302943ad4b4fa7661ec4382a04c91d85c18161c0 and the reviewed delta from 911dffe76d9ea4dbf52d73d06eb5e2679d9445a5. The PR only changes packages/rs-platform-wallet-ffi/src/platform_address_types.rs, and that change is correct: both explicit-input parser paths now reject duplicate caller-supplied addresses and the new tests cover those error cases. I found no new in-scope defects introduced by this PR. A focused cargo test -p platform-wallet-ffi duplicate_input_address run could not complete in this environment because tenderdash-proto attempts to download Tenderdash sources during build.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- SPV runtime shutdown still has a read/write lock inversion —
packages/rs-platform-wallet/src/spv/runtime.rs:138-160still holdsself.client.read().awaitacrossclient.run().await, whilestop()begins by takingself.client.write().awaitbefore it can callc.stop(). Ifrun()is active,stop()can block behind the write lock while the read guard remains held for the lifetime of the run loop, leaving the runtime vulnerable to a shutdown hang. This is outside PR #3554's scope because the delta only changes explicit-input parsing inpackages/rs-platform-wallet-ffi/src/platform_address_types.rs.- Follow-up: Create a separate issue or author/maintainer-requested PR to let
stop()signal shutdown without first waiting on the write lock, or to avoid holding the read guard acrossclient.run().await.
- Follow-up: Create a separate issue or author/maintainer-requested PR to let
- Swift explicit-change transfers still target
ReduceOutputby Swift array index while Rust canonicalizes outputs by address —packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:296-316still appends the change output last and sendsReduceOutput(lastIndex)throughplatform_address_wallet_transfer(). That FFI entrypoint calls Rusttransfer(), which canonicalizes outputs into aBTreeMapkeyed byPlatformAddressbefore DPP interpretsReduceOutput(index); unlike Rust's separatetransfer_with_change_address()helper, this path does not reject a lexicographically smaller change address. A change address that sorts before a recipient can therefore cause the on-chain fee target to differ from the Swift-visible target. This remains outside PR #3554 because none of the changed files touch the Swift wrapper or transfer bridge.- Follow-up: Create a separate issue or author/maintainer-requested PR to route Swift explicit-change transfers through a Rust API that resolves the fee target by canonical output identity, or to pre-validate that the change address cannot sort before the intended recipient outputs.
- Auto-selection still has the documented snapshot-versus-broadcast race —
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:292-295still carries TODOQA-007, and the code still reads wallet state under a snapshot before later broadcast and persistence steps. That race is real and the file explicitly documents it, but it predates this PR and the reviewed delta does not touch the transfer execution path.- Follow-up: Create a separate issue or author/maintainer-requested PR to hold the relevant state across snapshot plus broadcast, or to re-validate balances and fee headroom immediately before signing and broadcast.
…instead of IndexMap IndexMap is not part of our official public API surface. Public `transfer` and `transfer_with_change_address` now accept `impl IntoIterator<Item = (PlatformAddress, Credits)>` and canonicalize directly to BTreeMap at the public boundary — DPP's downstream contract is already BTreeMap-keyed, so there is no intermediate IndexMap step. No behavior change. FFI callers (which build IndexMap via parse_outputs) compile unchanged — IndexMap implements IntoIterator. `BTreeMap` callers now compile too, pinned by a new unit test (`transfer_with_change_address_accepts_btreemap_outputs`) that reaches the Auto+Some(change_addr) rejection arm via a BTreeMap input. `transfer_with_change_address` previously collected `outputs_with_change` back into IndexMap solely to satisfy `transfer`'s old parameter type; that re-collection is now gone (BTreeMap is passed straight through). Module-level `use indexmap::IndexMap` moved to `auto_select_tests` — production code no longer references the type. Internal helpers (`saturating_sum_credits`, `checked_sum_credits`, `build_auto_select_candidates`, `detect_no_selectable_inputs`, `validate_change_address`, `build_transfer_persistence_entries`) keep their existing `IntoIterator` bounds — each performs a single iter walk without random access, so a concrete map would only narrow the input shape without simplifying the body. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Policy iteration — public API no longer exposes Per maintainer guidance, - pub async fn transfer<S: Signer<PlatformAddress> + ...>(
- ...,
- outputs: IndexMap<PlatformAddress, Credits>,
- ...
- ) -> Result<...> { ... }
+ pub async fn transfer<S: Signer<PlatformAddress> + ...>(
+ ...,
+ outputs: impl IntoIterator<Item = (PlatformAddress, Credits)>,
+ ...
+ ) -> Result<...> { ... }Same shape for Bonus simplificationThe original "collect to IndexMap internally" step turned out to be dead weight — both functions immediately canonicalized to Callers unchanged
Internal helpersSurveyed 6 internal helpers currently using Verification
🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The head commit ece54f9369d70d7b682c228b7349aa5b92e73405 does introduce one real in-scope regression in the Rust wallet API. The rest of the reviewed concerns are either unchanged pre-existing issues or in untouched codepaths, so they should be tracked separately rather than block this PR.
🔴 1 blocking
1 additional finding(s)
blocking: `IntoIterator` outputs reintroduce silent duplicate-destination collapse at the public API boundary
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 41)
Changing transfer() from IndexMap<PlatformAddress, Credits> to impl IntoIterator<Item = (PlatformAddress, Credits)> removes the old type-level uniqueness guarantee, and the immediate collect::<BTreeMap<_, _>>() silently keeps only the last row for any repeated address. That behavior is newly exposed by this PR: direct Rust callers can now pass Vec/slice data such as [(A, 10), (A, 20), (B, 5)], and the signed transition becomes {A: 20, B: 5} with no error. The same unchecked canonicalization was added to transfer_with_change_address() at lines 195-206, so duplicate user_outputs are also collapsed before change and fee-target validation runs. This changes the total requested outputs, can drop an intended recipient amount, and can move the effective ReduceOutput target after canonicalization. The FFI path still rejects duplicate outputs in parse_outputs(), so this is a real regression introduced by widening the Rust API surface in this commit.
🤖 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.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:41-47: `IntoIterator` outputs reintroduce silent duplicate-destination collapse at the public API boundary
Changing `transfer()` from `IndexMap<PlatformAddress, Credits>` to `impl IntoIterator<Item = (PlatformAddress, Credits)>` removes the old type-level uniqueness guarantee, and the immediate `collect::<BTreeMap<_, _>>()` silently keeps only the last row for any repeated address. That behavior is newly exposed by this PR: direct Rust callers can now pass `Vec`/slice data such as `[(A, 10), (A, 20), (B, 5)]`, and the signed transition becomes `{A: 20, B: 5}` with no error. The same unchecked canonicalization was added to `transfer_with_change_address()` at lines 195-206, so duplicate `user_outputs` are also collapsed before change and fee-target validation runs. This changes the total requested outputs, can drop an intended recipient amount, and can move the effective `ReduceOutput` target after canonicalization. The FFI path still rejects duplicate outputs in `parse_outputs()`, so this is a real regression introduced by widening the Rust API surface in this commit.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
lklimek
removed
the
ready for final review
…r APIs ece54f9 flipped `transfer` / `transfer_with_change_address` to accept `impl IntoIterator<Item = (PlatformAddress, Credits)>` and immediately `.collect::<BTreeMap<_, _>>()` it. That collect is last-write-wins on duplicate keys — a direct Rust caller passing `Vec<(A, 10), (A, 20), (B, 5)>` silently produces `{A: 20, B: 5}`, with no error, no warning, and the intended recipient amount dropped. Under `[ReduceOutput(index)]` the canonical target can shift after canonicalization, compounding the silent drop with a wrong fee target. The FFI path is unaffected — `parse_outputs` already rejects duplicates at the FFI boundary (CMT-003 in commit 2eab4f7). The regression exposed direct Rust callers only. Mirror the CMT-003 pattern at the Rust API boundary: walk the iterator once, build the `BTreeMap` while checking `contains_key`, and return a new typed `PlatformWalletError::DuplicateOutputAddress { address }` on the first duplicate. Same allocation profile as the previous collect. The new variant maps to `ErrorInvalidParameter (2)` at the FFI boundary — it is a caller-supplied parameter bug, mirroring how `parse_outputs`'s String-based duplicate rejection already routes via `From<String>`. No new FFI code is introduced; no Swift mirror change is needed. Pinned with two new unit tests: - `transfer_rejects_duplicate_output_address` - `transfer_with_change_address_rejects_duplicate_user_output_address` The existing `transfer_with_change_address_accepts_btreemap_outputs` still passes — `BTreeMap` callers with unique keys remain valid input. Flagged by @thepastaclaw on PR #3554. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Fixed in commit What changedMirror of the CMT-003 pattern at the Rust API boundary. New typed error variant: PlatformWalletError::DuplicateOutputAddress { address: PlatformAddress }Both fn collect_outputs_unique<I>(outputs: I) -> Result<BTreeMap<PlatformAddress, Credits>, PlatformWalletError>
where
I: IntoIterator<Item = (PlatformAddress, Credits)>,
{
let mut map = BTreeMap::new();
for (addr, credits) in outputs {
if map.contains_key(&addr) {
return Err(PlatformWalletError::DuplicateOutputAddress { address: addr });
}
map.insert(addr, credits);
}
Ok(map)
}A direct Rust caller passing FFI mapping
Pins
Existing Gates at HEAD
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the worktree at d991a91736bf9ff78d52bf35a4c39bc2946fb013 against the cited findings and the actual PR scope. The duplicate-output regression is fixed in the Rust transfer APIs, the new typed error is present, and the FFI layer now maps it to ErrorInvalidParameter; I found no new in-scope defects in this delta. I also re-checked the previously noted follow-ups and they still exist, but they remain outside this PR's stated goal of rejecting duplicate output addresses. Targeted cargo test commands could not be completed in this sandbox because the workspace build tries to download Tenderdash sources during dependency build.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Swift explicit-change fee targeting still uses Swift array position while Rust/DPP interpret
ReduceOutputafter canonicalization — This mismatch is still real on the current head.ManagedPlatformAddressWallet.swiftstill appends the change output and setsReduceOutput(lastIndex), while the Rust wallet documents and enforces DPP's lexicographicBTreeMapordering for outputs, so a lexicographically smaller change address can still become the actual fee-bearing output. It remains outside this PR because the delta only touches the Rust duplicate-output rejection path and FFI error mapping, not the Swift caller or the ABI contract for fee-target identity.- Follow-up: Create a separate issue or author/maintainer-requested PR to resolve explicit-change fee targeting by canonical Rust/DPP output identity rather than Swift insertion order.
- Auto-selection still relies on a wallet snapshot that can diverge before broadcast — The documented
TODO(QA-007)inauto_select_inputs()is still present: balances and fee headroom are selected from a read snapshot and then later used for signing and broadcast after state may have changed. That is a real robustness concern, but this PR explicitly targets duplicate output rejection and does not change the selection/broadcast lifecycle, so it should remain a separate tracked follow-up rather than an in-scope blocker here.- Follow-up: Create a separate issue or author-requested PR to re-validate balances and fee headroom immediately before signing/broadcast, or hold the relevant wallet state stable across selection and broadcast.
- SPV runtime shutdown still depends on acquiring a write lock after
run()holds a read lock acrossclient.run().await— This liveness risk still exists on the current head:SpvRuntime::run()keeps the read guard alive while awaitingclient.run(), andstop()still requires a write lock before it can callc.stop().await. The issue is unrelated to the transfer/output validation changes in this PR, so it should be tracked separately instead of being carried forward as an in-scope review finding for #3554.- Follow-up: Create a separate issue or author-requested PR to restructure SPV shutdown so the running client can be signaled without first waiting on the write lock.
… type signature Revert transfer() and transfer_with_change_address() from `impl IntoIterator<Item = (PlatformAddress, Credits)>` to `BTreeMap<PlatformAddress, Credits>`. Investigation of the DPP state-transition layer confirmed the immediate BTreeMap canonicalization in these functions is load-bearing: `AddressFundsTransferTransitionV0.outputs` is itself a BTreeMap on the wire, and the chain's fee-deduction logic resolves `ReduceOutput(index)` via `outputs.keys().collect::<Vec<_>>()` — see rs-dpp/src/address_funds/fee_strategy/v0.rs:29-56. Caller-supplied insertion order vanishes at serialization, so IntoIterator misleadingly implied a contract the protocol does not honor. Type-level effects: - Duplicate output addresses are now structurally impossible at the API boundary; the explicit `DuplicateOutputAddress` variant and `collect_outputs_unique` helper added in d991a91 are removed as dead weight. - FFI `parse_outputs` no longer needs IndexMap; returns BTreeMap with the same `contains_key`-based duplicate rejection. - Rustdoc on both public APIs now documents the lex-canonical contract and the `ReduceOutput(i)` index space. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Design pivot — @thepastaclaw — this supersedes the Why we pivotedInvestigation of the DPP layer confirmed the lex-order canonicalization inside
What changed in
|
| Item | Before (d991a91736) |
After (b38e1237ee) |
|---|---|---|
transfer() signature |
outputs: impl IntoIterator<Item = (PlatformAddress, Credits)> |
outputs: BTreeMap<PlatformAddress, Credits> |
transfer_with_change_address() signature |
same shape | user_outputs: BTreeMap<PlatformAddress, Credits> |
| Duplicate rejection | Runtime check in collect_outputs_unique helper → Err(DuplicateOutputAddress) |
Structural — BTreeMap cannot contain duplicates by construction |
PlatformWalletError::DuplicateOutputAddress |
Added | Removed — unreachable |
FFI parse_outputs return |
Result<IndexMap<…>, String> |
Result<BTreeMap<…>, String> — keeps the contains_key duplicate-rejection at the FFI boundary for Swift/Kotlin callers |
indexmap crate dep |
Used | Dropped from both rs-platform-wallet and rs-platform-wallet-ffi Cargo.toml |
| Rustdoc | — | Both APIs now document the lex-canonical contract and ReduceOutput(i) index space |
Net: 7 files changed, +50 / −209.
Tests
| Crate | Before | After | Δ |
|---|---|---|---|
platform-wallet --lib |
149 | 147 | −2 (dup-rejection tests removed — type system enforces it) |
platform-wallet-ffi --lib |
84 | 83 | −1 (dup-rejection FFI mapping test removed) |
Side cleanup spotted in-passing: parse_outputs_preserves_insertion_order_for_distinct_addresses was misnamed and its fixture was accidentally in lex order, so it would have passed silently under a wrong implementation. Renamed to parse_outputs_yields_lex_order_for_distinct_addresses and the fixture flipped to non-lex order so the test now actually exercises the canonicalization.
Rustdoc excerpt
/// # Output order semantics
///
/// Outputs are stored on-chain in `AddressFundsTransferTransitionV0` as a
/// `BTreeMap<PlatformAddress, Credits>` — keyed by address, iterated in
/// lexicographic order. This is also the index space resolved by the
/// fee-strategy `ReduceOutput(i)`: index 0 is the lex-smallest output
/// address, not the first one supplied by the caller. The parameter type
/// `BTreeMap` is chosen to make this canonical ordering explicit at the
/// type signature.
Gates at HEAD b38e1237ee
cargo fmt --all— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test -p platform-wallet --lib— 147 / 147cargo test -p platform-wallet-ffi --lib— 83 / 83
0 unresolved inline review threads. Ready for re-review.
🤖 Co-authored by Claudius the Magnificent AI Agent
…surface Three follow-ups on the auto-select-inputs PR: - Revert the duplicate-address error type on parse_explicit_inputs, parse_explicit_inputs_with_nonces, and parse_outputs from Result<_, String> back to Result<_, &'static str>. The formatted hex breadcrumb in the diagnostic isn't worth widening the public surface; flat constant messages are sufficient. - Add a tracing::warn! on the silent PlatformP2PKHAddress::from_address conversion drop in transfer.rs inside the filter_map building the owned-address lookup. Conversion failures are now visible in logs instead of being filtered out without a trace. - Convert four debug_assert! invariants in transfer.rs to real runtime checks (or delete redundant pairs). debug_assert! is compiled out in release builds, so the Σ-inputs-equals-Σ-outputs guarantees at the end of select_inputs_deduct_from_input and select_inputs_reduce_output weren't actually defended in production. Two debug_asserts that were paired with following if-checks are removed as duplication. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…EnoughFunds
The output-centric variant name described the wallet's internal
address-graph state ("only output addresses are funded") rather than
the user-facing problem ("not enough funds to spend"). Renamed to an
input-centric narrative matching how callers reason about the error.
No semantic change: same fields, same FFI code (ErrorNoSelectableInputs
= 14 under the umbrella mapping), same Display payload survives the
boundary — only the variant name and the Display message wording change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-reviewed head 4965e425ae921723c391d8d7f1d45c99f09f9041 with focus on the delta since d991a91736bf9ff78d52bf35a4c39bc2946fb013: the IndexMap → BTreeMap API/ordering pivot, transfer_with_change_address behavior, the OnlyOutputAddressesFunded → NotEnoughFunds rename, and the tightened platform-address FFI error surface.
No new in-scope blocking findings from this pass. The latest changes are internally consistent with the canonical BTreeMap ordering decision, keep the FFI parse helpers on BTreeMap + &'static str, preserve ErrorNoSelectableInputs mapping for the renamed wallet error, and make the formerly silent P2PKH conversion skip visible via logging.
Checks run locally:
cargo check -p platform-wallet --libcargo check -p platform-wallet-ffi --lib- platform-wallet auto-select tests: 23 passed
- platform-wallet-ffi platform address parsing tests: 4 passed
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the checked-out worktree at b38e1237eed9cff4ed269b9a2c6a5b84b34053e8 and did not confirm any new in-scope defect in this delta. The only proposed blocker does describe a real Swift/FFI semantic mismatch, but it was already present in the parent commit d991a91736bf9ff78d52bf35a4c39bc2946fb013 because transfer() already canonicalized caller-supplied outputs into a BTreeMap before fee resolution; this PR makes that contract explicit rather than introducing it.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Swift explicit-change transfers still target
ReduceOutputby Swift array position while Rust resolves indexes in canonical address order —ManagedPlatformAddressWallet.transfer(...)still appends the change output last and sendsReduceOutput(lastIndex), but the Rust side resolvesReduceOutput(i)against lexicographicBTreeMaporder. That can still charge the fee to a recipient output when the change address sorts before another output. I verified this on the current head, but it is outside this PR's scope because the same behavior already existed in the parent commit: before this refactor,wallet.transfer()accepted anIntoIteratoryet immediately canonicalized it withcollect_outputs_unique(...)into aBTreeMapbefore calling the SDK.- Follow-up: Create a separate issue or author/maintainer-requested PR to identify fee-bearing outputs by canonical Rust/DPP output identity instead of caller insertion order.
- Auto-selection still uses a wallet snapshot that can diverge before broadcast —
auto_select_inputs()still documents a pre-existing race where balances and fee headroom are selected from one wallet snapshot and later used for broadcast after state may have changed. That robustness issue remains real on the current head, but this PR does not change the selection/broadcast lifecycle and explicitly keeps the TODO in place.- Follow-up: Create a separate issue or author-requested PR to revalidate balances and fee headroom immediately before signing/broadcast, or hold the relevant wallet state stable across selection and broadcast.
- SPV shutdown still depends on acquiring a write lock while
run()awaits under a read lock —SpvRuntime::run()still holdsself.client.read().awaitacrossclient.run().await, whilestop()still needsself.client.write().awaitbefore it can callc.stop().await. That remains a liveness concern worth tracking, but it is unrelated to this transfer-output refactor and is not newly introduced here.- Follow-up: Create a separate issue or author-requested PR to restructure SPV shutdown so the running client can be signaled without first waiting on the write lock.
lklimek
added
the
ready for final review
…allet-auto-select-inputs
…d to NotEnoughFunds" This reverts commit 4965e42.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3554 at head cb532cc. Prior-finding reconciliation: the prior review at b38e123 reported zero findings — nothing to mark STILL VALID, FIXED, OUTDATED, or INTENTIONALLY DEFERRED. The actual PR-scoped latest delta is small: (1) debug_assert! invariants in select_inputs_deduct_from_input / select_inputs_reduce_output promoted to release-time runtime checks (5011b6f), (2) silent P2PKH conversion drops now log via tracing::warn!, (3) parse_outputs / parse_explicit_inputs[_with_nonces] error type narrowed from String to &'static str (dropping a hex breadcrumb from duplicate-address diagnostics — author traded diagnostic for FFI surface stability, tests updated to match), and (4) a OnlyOutputAddressesFunded rename revert (net zero). No new in-scope blocking issues; the in-scope delta is a quality improvement. Several agents flagged genuine bugs in Swift (transfer/asset-lock fee index mismatch with lex-ordered BTreeMap), an asset-lock parse_fee_strategy default, an ABI rename, and a 10506 retry griefing vector — all of these landed via the v3.1-dev merge (9639342) from sibling PRs #3626 / #3671 already merged independently, not from this PR's branch. They are real but pre-existing and out-of-scope for this PR; carried forward as follow-ups. Fix commits for two of them (0fce8fa, 4c8c368) already exist on other branches.
Out-of-scope follow-up suggestions (9)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Swift transfer's
ReduceOutput(changeIndex)targets by insertion order, but FFI canonicalizes outputs lexicographically —ManagedPlatformAddressWallet.swift:296-301passeschangeIndex = ffiOutputs.count - 1, butparse_outputs(platform_address_types.rs:203-220) collects outputs into aBTreeMap<PlatformAddress, Credits>anddeduct_fee_from_outputs_or_remaining_balance_of_inputs_v0(rs-dpp/.../v0/mod.rs:30,54-56) resolvesReduceOutput(i)againstoutputs.keys().collect()— lex order. The Rust doc ontransfer()(transfer.rs:30-39) explicitly documents this. When the change address sorts before any recipient, the fee is deducted from a recipient instead of the change output. This is a real bug at the current head, but it shipped in #3626 (c35c763504), not on this PR's branch — it's present here only because of the v3.1-dev merge96393428a5. A fix already exists on a sibling branch as0fce8fa17d fix(swift-sdk): sort transfer outputs lexicographically before ReduceOutputand4c8c36856f fix(swift-sdk): target change output by canonical order. - Swift
fundFromAssetLock/resumeFundFromAssetLockremainder index has the same insertion-vs-lex-order bug —ManagedPlatformAddressWallet.swift:460-464and:558-562computeremainderIndexfrom the caller's array position of theNonerecipient, butdecode_funding_addresses(fund_from_asset_lock.rs:222-260) builds aBTreeMap<PlatformAddress, Option<Credits>>. The chain-side fee-deduction resolvesReduceOutput(i)against the lex-sorted output keys, so when the remainder address does not sort to the same index as in the caller's array, the fee is deducted from an explicit recipient. This entered the codebase via #3671 (ef398cb61e) and reached this PR's head only through the v3.1-dev merge, not from this branch.- Follow-up: File a follow-up PR against v3.1-dev to mirror the transfer fix (
buildSortedFFIOutputspattern) in the asset-lock paths.
- Follow-up: File a follow-up PR against v3.1-dev to mirror the transfer fix (
parse_fee_strategydefault[DeductFromInput(0)]is invalid for asset-lock funding (no existing inputs) — Bothplatform_address_wallet_fund_from_asset_lock_signer(fund_from_asset_lock.rs:75) and the resume sibling (:165) callparse_fee_strategy(fee_strategy, fee_strategy_count), which defaults a null/zero pointer to[DeductFromInput(0)]. Asset-lock funding starts with zero existing address inputs, so DPP's state-transition validation rejectsDeductFromInput(0)withFeeStrategyIndexOutOfBoundsError. This default is correct for transfer/withdrawal but broken for the new asset-lock entry points. The entire fund_from_asset_lock.rs file was introduced by #3671 and is in the diff only because of the v3.1-dev merge — not this PR.- Follow-up: File a separate PR against v3.1-dev to either branch the default by entry point (asset-lock should default to
[ReduceOutput(remainder_index)]or require an explicit strategy) or makefee_strategymandatory at the asset-lock FFI surface.
- Follow-up: File a separate PR against v3.1-dev to either branch the default by entry point (asset-lock should default to
- Removing
platform_address_wallet_fund_from_asset_lockis an ABI break for out-of-tree consumers — The non-_signersymbol disappeared (91b9604d8d rename: top_up → fund_from_asset_lock across address-funding stack) without a compat shim. The crate exportsstaticlib/cdylib, so symbol names are part of its contract. Any Swift/ObjC/C consumer linking against the prior header will fail. This rename arrived via the v3.1-dev merge, not this PR.- Follow-up: If out-of-tree consumers exist, follow-up PR can keep the old symbol as a deprecation shim for one release. If only the in-tree Swift wrapper uses these FFI symbols, no action needed.
- Single-node 10506 responses on asset-lock submission can drive bounded fee-griefing —
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:256-305'ssubmit_with_cl_height_retryraisesuser_fee_increaseon any error matchingas_asset_lock_proof_cl_height_too_low, without requiring corroboration from a second endpoint. A malicious DAPI node on the client's path can fabricate repeated 10506 responses to drive the user to overpay priority fees. Credits cannot be stolen, but a real grief window. orchestration.rs was added by #3671, present here only through the v3.1-dev merge.- Follow-up: Separate PR: require corroborating evidence (second endpoint or proof) before bumping
user_fee_increase, or cap the fee bump per submission attempt.
- Follow-up: Separate PR: require corroborating evidence (second endpoint or proof) before bumping
- Pre-existing snapshot-vs-broadcast race in
auto_select_inputs(QA-007) —auto_select_inputsreads balances underwallet_manager.read()then releases the guard before broadcast, so concurrent local activity can shift the balance between snapshot and chain-side validation. Marked with aTODO(QA-007)and explicitly deferred by the author. No fund loss; bounded to legitimate-owner self-DoS via guaranteed-rejected transitions. Pre-existing, not introduced by this PR.- Follow-up: Track via QA-007: hold the guard across snapshot → build → broadcast, or re-validate headroom immediately before broadcast.
- TODO(platform#3040): static fee estimate runs ~2.3x below chain-time fee in
[ReduceOutput(0)]auto-select path —select_inputs_reduce_outputin transfer.rs documents (around lines 890-891, 908-909) that the staticestimate_fee_for_inputsundershoots the chain-time fee, mitigated by a 3x safety multiple and atracing::warn!guard. Pre-existing, tracked in platform#3040.- Follow-up: Track in platform#3040 — replace static estimate with chain-time fee API; revisit the 3x multiple if production rejection ratio justifies tightening.
- FFI
parse_*error tightening dropped offending-address hex from diagnostic (intentional) —5011b6fcf5narrowedparse_outputs/parse_explicit_inputs[_with_nonces]error type from ownedStringback to&'static str, which removedhex::encode(entry.address.hash)from duplicate-address messages. Functionally still routes throughErrorInvalidParameter; tests were rewritten to match. Acceptable trade-off (FFI surface stability vs diagnostic detail).- Follow-up: If downstream Swift/iOS callers ever lose debuggability on duplicate-address rejections, restructure the FFI to return a structured
(code, optional_address_hash)instead of widening the string error surface again.
- Follow-up: If downstream Swift/iOS callers ever lose debuggability on duplicate-address rejections, restructure the FFI to return a structured
- Panic containment at
extern "C"entry points across rs-platform-wallet-ffi — Exported FFI functions callblock_on_worker(...)which still hasexpect("tokio worker panicked")and there is nocatch_unwindwrapper at the boundary. Pre-existing pattern across many entry points, not unique to this PR.- Follow-up: Maintainer-requested PR: add a shared
catch_unwindwrapper (or panic-to-error translator) for exported entry points across rs-platform-wallet-ffi.
- Follow-up: Maintainer-requested PR: add a shared
Dependency map: which #3549 tests need this PRInvestigation of which tests in #3549 ( Behavior change recapThis PR rewrites Three layers of breakage in #3549 if this PR is absentLayer 1 — Compile fails (whole Layer 2 — Tests written explicitly to pin this PR (3 tests).
Layer 3 — Tests asserting the new invariant (~9 tests). Layer 4 — Logically independent but runtime-coupled (~46 tests). Truly independent (~2 tests). Tally
Bottom lineRoughly 12 of #3549's tests pin this PR by construction or by direct invariant assertion; another ~46 inherit a hard runtime dependency through bank funding; ~2 are genuinely independent. To run #3549 against a v3.1-dev that does NOT have this PR would require: (a) stubbing the new |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of head 3a3bc3e. Prior review at cb532cc reported no findings, so there are no carried-forward items to classify as STILL VALID / FIXED / OUTDATED / DEFERRED — the prior-finding set is empty. The latest delta (cb532cc..3a3bc3e) is purely a merge of v3.1-dev that brings in PR #3751 (DashSDKConfig.platform_version) and does not touch the rs-platform-wallet selector that is this PR's stated subject. No new in-scope findings. Six agent passes converged on an ABI-extension concern in DashSDKConfig, but that change was authored by the separately-merged PR #3751 and is out of scope for fix/rs-platform-wallet-auto-select-inputs; it is recorded as a release-coordination follow-up. The wallet selector code itself remains well-engineered (strict Σin==Σout invariant, checked arithmetic, post-Phase-4 fee recompute, validator-driven tests).
Out-of-scope follow-up suggestions (6)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- DashSDKConfig #[repr(C)] gained a trailing platform_version field — ABI-extension risk for external FFI consumers — The merge of v3.1-dev brought PR #3751 into this branch, appending
platform_version: u32to the#[repr(C)] DashSDKConfigstruct (packages/rs-sdk-ffi/src/types.rs:64-84) and unconditionally reading it fromdash_sdk_create,dash_sdk_create_extended,dash_sdk_create_trusted, anddash_sdk_create_with_callbacks. In-tree call sites and the Swift wrapper (SDK.swiftdefaultedplatformVersion: UInt32 = 0) were updated, and the cbindgen header is regenerated bybuild.rsso the production Swift target tracks the new layout. The residual risk is for any external (non-Swift, third-party, Kotlin/JNI, Android C) consumer still compiled against the previous header: it would allocate the shorter struct and Rust would read 4 bytes past its end. This is not authored by PR #3554; the wallet PR did not touch rs-sdk-ffi. It belongs against PR #3751 or as a release-coordination item, not as a blocker on this wallet fix.- Follow-up: Open a follow-up issue against rs-sdk-ffi to either (a) introduce a size/version tag on DashSDKConfig (e.g., a leading
struct_size: u32) so future trailing-field additions are forward-compatible, or (b) introduce a versionedDashSDKConfigV2+dash_sdk_create_v2pair, and confirm all downstream FFI consumers (Swift app, third-party bindings) rebuild against the new header.
- Follow-up: Open a follow-up issue against rs-sdk-ffi to either (a) introduce a size/version tag on DashSDKConfig (e.g., a leading
- Test-only Swift mock headers in SwiftTests/ don't mirror the new DashSDKConfig layout —
packages/swift-sdk/SwiftTests/SwiftDashSDK.handSwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.hstill define mock config structs withoutplatform_version. This is confined to the standalone Swift test mock target and does not affect the production DashSDKFFI boundary. Out of scope for this wallet PR.- Follow-up: Open a separate maintainer PR to update the Swift mock headers/fixtures so test scaffolding tracks the production FFI struct shape.
- TODO(QA-007): pre-existing TOCTOU between selector snapshot and broadcast in auto_select_inputs — The selector reads wallet balances under a read guard, drops the guard, then broadcasts (transfer.rs:305-308). A concurrent transfer or sync can invalidate the headroom math the selector just computed. The new selector leans harder on the snapshot than the prior code did but does not introduce the race. This is a local wallet UX concern, not consensus safety. The PR explicitly defers it.
- Follow-up: Open a follow-up issue to either hold the wallet_manager read guard across the snapshot+broadcast critical section, or re-validate selected inputs' headroom against the latest in-memory balances immediately before
transfer_address_funds.
- Follow-up: Open a follow-up issue to either hold the wallet_manager read guard across the snapshot+broadcast critical section, or re-validate selected inputs' headroom against the latest in-memory balances immediately before
- TODO(CMT-004): missing regression test for balance hydration via initialize_from_persisted — wallet.rs:107-112 carries a TODO for the absent regression test on
initialize_from_persisted's balance-restoration path. Outside this PR's diff; explicitly deferred.- Follow-up: Track in a separate test-coverage issue and add an end-to-end test that exercises the balance hydration path.
- TODO(platform#3040): static fee estimate vs chain-time fee divergence in select_inputs_reduce_output — transfer.rs:890-927 — the 3× safety multiple is explicitly heuristic; the real fix (chain-time fee API) is tracked in platform#3040. Out of scope.
- Follow-up: Follow platform#3040 to land chain-time fee evaluation and remove the safety-multiple band.
- FFI parse_outputs silently coalesces duplicate destination addresses — Pre-existing wallet-FFI behavior: parse_outputs accepts duplicate destination addresses and silently overwrites earlier entries rather than returning a typed error. The PR explicitly notes this under 'Known follow-ups'.
- Follow-up: Open a small follow-up PR to reject duplicate output addresses in rs-platform-wallet-ffi::parse_outputs with a typed error.
…_with_change_address Replaces the indexed AddressFundsFeeStrategy argument with a new FeeStrategyByAddress that resolves to consensus-indexed form AFTER the change-address augmentation, fixing a silent fee-misrouting bug. The old [ReduceOutput(0)] heuristic only caught the case where change_addr sorted lex-smaller than every user output. With change_addr inserted into the post-augmentation BTreeMap, ANY ReduceOutput(N) could shift if change landed at position <= N; the old guard missed every N>0 case. Changes: - Add FeeStrategyByAddress / FeeStrategyStepByAddress with to_indexed() that resolves addresses against the FINAL inputs / outputs maps. - Rename pub transfer() workhorse to private transfer_inner(); add a thin public transfer() facade with the original signature unchanged so the v3.1-dev..HEAD diff for transfer_inner is a name-only rename plus the auto-selector deltas already on this branch. - Rewrite transfer_with_change_address(): reject InputSelection::Auto outright, validate change_addr, augment outputs, lower the address-keyed strategy against the FINAL outputs map, delegate to transfer_inner. - Delete the [ReduceOutput(0)] lex-smaller heuristic. Address-keyed resolution is unambiguous so there is no shifting ambiguity to guard. Adds 9 unit tests covering single/multi-step lowering, all three change_addr position cases (lex-smaller, between, larger), and the N>0 shift the old heuristic missed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior review at 3a3bc3e found no issues, so there are no STILL VALID / FIXED / OUTDATED / INTENTIONALLY DEFERRED entries to carry forward. Reviewing the current head cumulatively against origin/v3.1-dev: the latest delta (35807f5, transfer.rs only) introduces a well-tested address-keyed FeeStrategyByAddress wrapper that resolves indexes post-augmentation, but lands two new in-scope issues — (a) the new FeeStrategy types are pub inside a private submodule and not re-exported, so the public wrapper signature is unreachable to downstream crates; and (b) the cumulative PR commit b38e123 switched FFI parse_outputs to a lex-ordered BTreeMap, which silently invalidates the still-unmigrated Swift changeIndex = ffiOutputs.count - 1 assumption in ManagedPlatformAddressWallet.swift and can misroute the fee to a recipient instead of the change output. Two additional suggestions are tracked for typed-error discipline and a missing UX-level fee headroom guard on the new wrapper path.
🔴 2 blocking | 🟡 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-platform-wallet/src/wallet/platform_addresses/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs:45-48: `FeeStrategyByAddress` / `FeeStrategyStepByAddress` / `FeeStrategyResolveError` are not reachable to downstream crates
`PlatformAddressWallet::transfer_with_change_address` is public and its required `fee_strategy` parameter is `FeeStrategyByAddress` (transfer.rs:357). The type is declared `pub` in `transfer.rs:23-51`, but `mod transfer;` in this file (line 15) is private and `mod.rs` only re-exports `PerAccountPlatformAddressState`, `PerWalletPlatformAddressState`, `PlatformAddressTag`, `PlatformAddressWallet` (lines 45-48). `wallet/mod.rs:16-19` and `lib.rs:67-69` do not re-export the new types either. Downstream consumers (rs-platform-wallet-ffi, dash-evo-tool, anything outside this crate) can see the method but cannot name its argument type, so the new public API compiles only for in-crate tests and is effectively unusable externally. Re-export `FeeStrategyByAddress`, `FeeStrategyStepByAddress`, and `FeeStrategyResolveError` alongside `PlatformAddressWallet`.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:281-301: Swift transfer wrapper still computes `ReduceOutput` index by insertion order after FFI switched outputs to lex-ordered BTreeMap
This PR's commit b38e1237ee changed `parse_outputs` in `packages/rs-platform-wallet-ffi/src/platform_address_types.rs:203-221` from an insertion-ordered `IndexMap` to a `BTreeMap<PlatformAddress, Credits>`, so the on-the-wire `ReduceOutput(i)` index now resolves against lex-sorted output keys, not the caller's array order. The Swift wrapper appends the change output last and sends `FeeStrategyStepFFI(step_type: 1, index: ffiOutputs.count - 1)` (line 298-300), assuming the change row is at the highest index. When the change address sorts lex-smaller than any recipient — roughly half the time for fresh HD addresses — the indexed strategy lands `ReduceOutput` on a *recipient* row, short-paying the recipient by the fee. This is exactly the misrouting class the new Rust `transfer_with_change_address` / `FeeStrategyByAddress` API was added to prevent. Either expose the address-keyed wrapper through `platform-wallet-ffi` and migrate the Swift caller to it, or compute the lex index of `resolvedChange` on the Swift side before passing it to the indexed FFI.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:391-414: `transfer_with_change_address` skips the `ReduceOutputAddress` fee-headroom guard the auto path has
`select_inputs_reduce_output` defensively rejects a transition when the targeted output cannot absorb the estimated fee (transfer.rs:1015-1029, plus a 3× safety-band warning at 1031-1049 tracking platform#3040). The new address-keyed wrapper performs no analogous check: a caller using `FeeStrategyByAddress::reduce_output(addr)` against an output whose value is below the estimated fee — common when the strategy targets the change row and `Σ inputs − Σ user_outputs` is close to the per-output minimum — passes `to_indexed`, reaches the SDK, and is only rejected on-chain with `fee_fully_covered = false`. Not a correctness regression (the chain still rejects), but it walks back the early-failure UX the auto path established. Mirror the existing `output_0 < estimated_fee` check against the named output (and ideally emit the same safety-band warning) before forwarding to `transfer_inner`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:53-57: Typed `FeeStrategyResolveError` variants are flattened into `AddressOperation(String)`
The `From<FeeStrategyResolveError> for PlatformWalletError` impl stringifies the typed error via `e.to_string()` and wraps it in the generic `AddressOperation(String)` variant. Callers (FFI bindings, SwiftExampleApp, dash-evo-tool) cannot programmatically distinguish `InputAddressNotFound(addr)` (wrong input passed), `OutputAddressNotFound(addr)` (wrong output passed), or `IndexOverflow { kind, index }` from each other or from unrelated `AddressOperation` failures. The rest of this PR introduced typed variants (`ChangeBelowMinimumOutput`, `OnlyOutputAddressesFunded`, `OnlyDustInputs`, `InputSumOverflow`) for exactly this reason; the fee-strategy resolve errors deserve the same treatment, especially because `Input/OutputAddressNotFound` are the most likely failures for a caller migrating from the indexed API. A dedicated `PlatformWalletError::FeeStrategyResolve(FeeStrategyResolveError)` variant would preserve the typed payload for diagnostics.
Triage outcomes from PR #3554 review (CMT-001..CMT-005): - CMT-001: collapse the transfer() / transfer_inner() split. Delete the thin facade and rename the workhorse in place to pub async fn transfer. The split was a diff-readability technique for the auto-selector review; with that review done, the facade is dead-weight. - CMT-002: document when to use transfer() vs transfer_with_change_address(). Adds a "When to use" section to transfer() covering input-selection variants, the Σ inputs == Σ outputs invariant ownership, and the address-keyed alternative. Complementary pointer in the wrapper docstring. - CMT-003: replicate the Auto path's ReduceOutput fee-headroom guard inside transfer_with_change_address. After lowering FeeStrategyByAddress to the indexed form, walk every ReduceOutput(i) step and reject pre-broadcast when the targeted output cannot absorb the estimated fee, plus a 3x safety-band tracing::warn! to match the Auto path's UX. Pins the rejection path with a new unit test. - CMT-004: accept_risk on flattening FeeStrategyResolveError into PlatformWalletError::AddressOperation(String) instead of promoting it to typed PlatformWalletError variants. Documented via an INTENTIONAL comment: fee-strategy resolution errors are caller-input errors distinct from wallet-state errors; keeping the enum lean is the deliberate trade-off. - CMT-005: replace "must route through the dispatcher" jargon in both select_inputs_deduct_from_input and select_inputs_reduce_output error messages. The new wording names InputSelection::Explicit and PlatformAddressWallet::transfer as the concrete next step. cargo check -p platform-wallet --lib clean. cargo clippy -p platform-wallet --lib -- -D warnings clean. cargo fmt -p platform-wallet --check clean. cargo test -p platform-wallet --lib: 161 passing (160 + 1 new CMT-003 test). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3554 at head 64e1267. Prior-finding reconciliation: (1) FeeStrategyByAddress/FeeStrategyStepByAddress/FeeStrategyResolveError unreachable to downstream crates — STILL VALID (in-scope blocker); (2) Swift ReduceOutput index uses insertion order vs FFI lex-sorted BTreeMap — STILL VALID but INTENTIONALLY DEFERRED to PR #3549 per PR description (out-of-scope, the Swift file is untouched by this PR); (3) transfer_with_change_address missing ReduceOutputAddress fee-headroom guard — FIXED at transfer.rs:424-470 with new test transfer_with_change_address_rejects_sub_fee_reduce_output_target; (4) typed FeeStrategyResolveError flattened into AddressOperation(String) — INTENTIONALLY DEFERRED, now documented via INTENTIONAL(CMT-004) comment at transfer.rs:53-60. No new defects introduced by the latest delta. One in-scope blocker remains: the new public address-keyed types are still gated behind a private module, so the wrapper this PR exists to introduce cannot be called from outside the crate.
🔴 1 blocking
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/src/wallet/platform_addresses/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs:15: Carried-forward prior finding #1 (STILL VALID): address-keyed fee-strategy API is unreachable to downstream crates
`PlatformAddressWallet::transfer_with_change_address` is declared `pub` and takes `fee_strategy: FeeStrategyByAddress`, but `mod transfer;` at `platform_addresses/mod.rs:15` is private and neither `platform_addresses/mod.rs`, `wallet/mod.rs`, nor `lib.rs` re-exports `FeeStrategyByAddress`, `FeeStrategyStepByAddress`, or `FeeStrategyResolveError`. The effective visibility of those types is `pub(in crate::wallet::platform_addresses)`, so external crates (rs-platform-wallet-ffi, the stacked PR #3549's `pa_001b_change_address_branch.rs`, and any future SDK consumer) cannot name the argument or its constructors (e.g. `FeeStrategyByAddress::reduce_output(addr)`) and therefore cannot call the new wrapper. The whole point of this PR — providing a safe address-keyed entry point that downstream callers migrate onto to retire the lex-vs-insertion-order misrouting bug class — is structurally inert outside this crate's own unit tests until these types are re-exported. The latest delta (64e12673cd) promoted `transfer_inner → transfer` and added a ReduceOutput fee-headroom guard, but did not address the reachability of the types the wrapper exists to take. The PR's own migration snippet in the description (`FeeStrategyByAddress::reduce_output(change_addr)`) is not callable as written from a downstream crate.
Issue
auto_select_inputsinpackages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rswas inserting each selected address with its full balance as the input'sCreditsvalue, then returning as soon as accumulated coveredoutput + fee. The address-funds-transfer protocol enforcesΣ inputs.credits == Σ outputs.credits(strict equality), so a bank with ~500B credits funding a 50M output produced:Verified at
rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rsand asserted on-chain byrs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly(new_balance == initial_balance - transfer_amount - fee).Protocol semantics (now respected)
inputs[addr].credits= consumed amount fromaddroutputs[addr]= credited amount toaddrΣ inputs.credits == Σ outputs.credits(strict equality)AddressFundsFeeStrategy.DeductFromInput(0)reduces the remaining balance by the fee — never the inputs map'sCreditsvalueconsumed >= min_input_amount(currently 100_000)What changed
auto_select_inputsnow respects the protocol contract:Σ inputs.credits == Σ outputs.credits(strict equality) — selection trims so the inputs map sums to exactlytotal_output.DeductFromInput(0)target — the lex-smallest selected input retains at leastestimated_feeof remaining balance.min_input_amountenforcement — candidates below the floor are filtered upfront; Phase-4 distribution rolls sub-minimum residue into the fee target.dash-evo-tool's allocator; collapses many cases to a single input.InputSelection::Autofee strategies supported:[DeductFromInput(0)]and[ReduceOutput(0)]. Any other shape returns a clear error redirecting toInputSelection::Explicit.checked_add/saturating_subonCreditsthroughout the selector path; overflow produces typed errors instead of wrapping.fee_target_min > fee_target_max, the algorithm extends the prefix with the next candidate and retries instead of erroring out.CI gap also closed in this PR:
rs-platform-walletwas missing from.github/package-filters/rs-packages-no-workflows.yml, so Rust workspace tests had been skipping silently. The filter entry mirrors the existing pattern (path +*sdkalias). The previously-hidden Rust 1.92 clippy lints (incore_bridge.rs,wallet/apply.rs:316,tests/spv_sync.rs) are cleared in the same PR because they were blocking once CI started exercising the crate.Commit history (initial 5-commit review wave for blame)
aaf8be74— initialΣ inputs == Σ outputsfix. Extracted the selection loop into a pure module-scope helperselect_inputsthat walks candidates and trims the result so the inputs map sums to exactlytotal_output.9ea9e703— fee-headroom guarantee atDeductFromInput(0)target (CodeRabbit critical). The original fix proved aggregate balance covered the fee but not that the specific fee-bearing input had remaining headroom. Now identifies the prospective fee target (lex-smallest of selected) and reserves at leastestimated_feeof remaining balance on it.687b1f86— protocol-level reproduction test. Reconstructs the OLD buggy selector output for the CodeRabbit example, feeds the post-consumptioninput_current_balancesthroughdpp::address_funds::fee_strategy::deduct_fee_from_outputs_or_remaining_balance_of_inputs, and asserts!fee_fully_covered. Proves the rejection at the protocol layer rather than asserting "the new output looks different."60f7850a— sort candidates by balance descending (mirrorsdash-evo-tool's allocator). Reduces the frequency of multi-input cases — when the largest single balance coverstotal_output + fee, the result is a 1-input map and the lex-smallest fee-target headroom logic doesn't fire at all. Bonus:fee_headroom_violation_errorsnow produces a debuggable error message.9ff937ff— second review wave (4 blocking, 1 suggestion):min_input_amountenforcement.auto_select_inputsfilters candidates <min_input_amountupfront; Phase 4 distribution rolls any sub-minimum tail residue back into the fee target's consumption rather than producing anInputBelowMinimumError-prone tail.fee_strategyrestriction intransfer().InputSelection::Autonow rejects any shape other than[DeductFromInput(0)]with a clear redirect toInputSelection::Explicit. The previous fallback path was publicly reachable but only protocol-correct for that single shape.fee_target_min > fee_target_max, the algorithm extends the prefix with the next candidate and retries instead of erroring out — larger prefixes can yield a different lex-smallest fee target with sufficient headroom.total_output < min_input_amounterror (replaces the internal-error fallthrough).assert_selection_validateshelper builds anAddressFundsTransferTransitionV0from each selector test's output and runsvalidate_structure. Catches future protocol-level regressions without depending on testnet.CI / infrastructure (3 commits)
79c2b285—ci(rs-packages-filter): trigger Rust workspace tests on rs-platform-wallet changes. The path filter at.github/package-filters/rs-packages-no-workflows.ymldidn't listrs-platform-wallet, so any crate-only change there evaluatedrs-packages = '[]'andRust workspace testssilently skipped. This PR's prior 5 commits had never been validated by Rust CI — only by localcargo test. The filter entry mirrors the existing pattern (path +*sdkalias for transitive triggers).d610502— mergev3.1-dev(9bd37f203a).3c4f9199— Rust 1.92 clippy hardening that the previously-skipped pipeline had been quietly accumulating:field_reassign_with_defaultincore_bridge.rs::build_core_changeset→ struct literal initlet_unit_valueinwallet/apply.rs:316(WalletInfoInterface::update_balancereturns()) → drop thelet _ =core.chain.synced_heightaccess intests/spv_sync.rs→ flattened tocore.synced_height(struct shape changed upstream; the test never recompiled because workspace tests were skipping)Tests (138 lib tests, all passing)
auto_select_testsmodule — 13 tests:single_input_oversized_balance_trims_to_output_amountdescending_order_picks_single_largest_when_sufficientpre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction— protocol-level reproduction (asserts!fee_fully_covered)fee_headroom_violation_errorsnon_fee_target_below_min_input_redistributesauto_select_inputs_excludes_output_addresses— confirms self-funded-transition filtertotal_output_below_min_input_amount_errorsno_candidates_errorsreduce_output_happy_path_single_input— confirms[ReduceOutput(0)]strategy parity with[DeductFromInput(0)]detect_no_selectable_inputs_combines_both_cases— pins theNoSelectableInputsvariant fields (funded_outputs,sub_min_count,sub_min_aggregate)augment_outputs_with_change_adds_residual_outputaugment_outputs_with_change_rejects_duplicate_addressaugment_outputs_with_change_rejects_no_surplusSeveral assert structural validity via
assert_selection_validates→AddressFundsTransferTransitionV0::validate_structure.Note on withdrawal selector
The
auto_select_inputs_for_withdrawalrustdoc clarifies the asymmetry: withdrawal validatesΣ inputs > output_amount(strictly greater, surplus = fee), so its drain-everything strategy is correct by design. Not the same bug; no code change.Verification
cargo fmt -p platform-wallet --check✓cargo clippy --workspace --tests -- -D warnings✓cargo test -p platform-wallet --lib— 138/138 passingTest plan
deduct_fee_from_outputs_or_remaining_balance_of_inputstransfer(),auto_select_inputs,select_inputsunchanged. Backward-compatible additions only: newWalletError::NoSelectableInputsstruct variant (replacing the prior generic insufficient-balance message) andInputSelection::Autonow accepts[ReduceOutput(0)]in addition to[DeductFromInput(0)]. Downstream exhaustive matches onWalletErrorneed updating.Provenance
Originally surfaced and fixed during work on PR #3549 (
rs-platform-wallete2e harness). Split out so the production-code fix can ship independently of the long-running e2e branch. Subsequent commits address review feedback from CodeRabbit andthepastaclawreviewers and close a CI coverage gap that was hiding pre-existing breaks onv3.1-dev.🤖 Generated with Claude Code
Triage wave (11 commits, 2026-05-21)
Applied 9 of 11 triaged findings from a fresh grumpy-review + comment-verification pass at HEAD
d5d2b3d271. 2 deferred as out-of-scope TODOs.098484f34faugment_outputs_with_changenow rejectschange_amount < min_output_amountvia typedChangeBelowMinimumOutput;platform_versionplumbed;(0, min_output_amount)band test added8ee78a4f60InputSelection::Autorustdoc rewritten to current contract (balance-desc, dust filter, output-address exclusion, supported fee strategies, typed errors)3aeed87bfais_empty_no_records()now includesaddresses_derived, matching canonicalCoreChangeSet::is_empty()6c3fb02b0cselect_inputs_reduce_outputandselect_inputs_deduct_from_input; invariant documentede2140bd475selected.len()instead ofprefix.len(); eliminates false "insufficient funds" on objectively spendable walletsb85b6a9a90OnlyOutputAddressesFundednow carriessub_min_count+sub_min_aggregateto preserve dust diagnostics under the combined casecfe3c33247checked_sum_credits+InputSumOverflowfor caller-supplied input maps intransfer_with_change_address; trusted call sites still saturate9854e38674outputsnow typed asIndexMap<PlatformAddress, Credits>so caller controls "output 0" insertion order;[ReduceOutput(0)] + Some(change_addr)rejects whenchange_addris lex-smaller than every user output9c136cd912ErrorOnlyOutputAddressesFunded = 13andErrorOnlyDustInputs = 14; SwiftPlatformWalletResultCode+ typedPlatformWalletErrormirrored; table-driven Rust test pins routing7c7a2ec5852957623756cargo fmt --allcleanupTriage decisions
Verification
cargo fmt --all --check✓cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings✓cargo test -p platform-wallet --lib— 144 passingcargo test -p platform-wallet-ffi --lib— 75 passing (incl. newtyped_errors_route_to_dedicated_codes)Known follow-ups (out of PR scope)
AddressFundsTransferTransitionoutputs are stillBTreeMapat the chain layer; the IndexMap is a wallet-only affordance. A doc comment on the DPP transition struct cross-referencing the wallet semantics would help future readers — separate PR.parse_outputsoverwrites duplicate-address entries silently. Pre-existing behavior, but worth a sanity rejection in a follow-up.🤖 Co-authored by Claudius the Magnificent AI Agent
Address-keyed fee strategy (commit
35807f5005)Reframes the
[ReduceOutput(0)] + Some(change_addr)heuristic introduced in the QA-002 commit (9854e38674). The heuristic was incomplete: it only rejected the case wherechange_addrwas lex-smaller than every user output, catchingReduceOutput(0)shifts but silently misrouting the fee forReduceOutput(N>0)wheneverchange_addrlanded at position ≤ N in the post-augmentationBTreeMap.Root cause
The wrapper inserts
change_addrintouser_outputsbefore callingtransfer. That insertion can shift the lex-order index of any user output, not just index 0. The old guard only inspected index 0.Fix
FeeStrategyByAddress/FeeStrategyStepByAddresscarryingDeductFromInputAddress(PlatformAddress)/ReduceOutputAddress(PlatformAddress)variants.to_indexed(&inputs, &outputs)resolves addresses against the final maps the signer will see — i.e. after change-augmentation — so indices are correct regardless ofchange_addr's lex position.transfer_with_change_addressnow takesFeeStrategyByAddressinstead ofAddressFundsFeeStrategy. Augmentation runs first, lowering runs second; the dead[ReduceOutput(0)]heuristic is deleted.transferis split: the original body becomes privatetransfer_inner(byte-stable rename so thev3.1-dev..HEADdiff for the workhorse remains the auto-selector deltas, nothing else); a thinpub fn transferfaçade preserves the original public signature.transfer_with_change_addressnow rejectsInputSelection::Autooutright, regardless ofchange_addr— Auto already trimsΣ inputs == Σ outputsand has no residual concept; address-keyed strategies also need known inputs up-front.Breaking change
PlatformAddressWallet::transfer_with_change_addressnow takesFeeStrategyByAddress(defined intransfer.rs) in place ofAddressFundsFeeStrategy. Callers migrate via the convenience constructors:Known downstream caller:
pa_001b_change_address_branch.rsin PR #3549 (will need migration on the stacked rebase).Tests
9 new unit tests in
auto_select_tests:InputAddressNotFound/OutputAddressNotFounderrors fire for unknown addresses.ReduceOutput(N>0)shift case the old heuristic missed — confirmsto_indexednow produces the correct index.reduce_output(change_addr)with change lex-smallest is now a legitimate, non-rejected pattern (deliberately removes the old guard).Verification
cargo check -p platform-wallet --lib✓cargo clippy -p platform-wallet --lib -- -D warnings✓cargo fmt -p platform-wallet --check✓cargo test -p platform-wallet --lib— 160 passing (was 144 + 9 new + 7 retained migrated)🤖 Co-authored by Claudius the Magnificent AI Agent