fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
Open
Claudius-Maginificent wants to merge 62 commits into
Open
fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554Claudius-Maginificent wants to merge 62 commits into
Claudius-Maginificent wants to merge 62 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>
Contributor
|
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Collaborator
|
✅ Review complete (commit 30e612b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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>
Contributor
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>
Contributor
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
suggested changes
Apr 28, 2026
Collaborator
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
suggested changes
Apr 28, 2026
Collaborator
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>
Contributor
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 88.25% 88.24% -0.01%
============================================
Files 2494 2506 +12
Lines 304580 307938 +3358
============================================
+ Hits 268812 271745 +2933
- Misses 35768 36193 +425
🚀 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
Collaborator
|
Correction: the immediately previous review |
…_assert (#3554 review) Audit F-001 flagged three "should be unreachable" Err sites in `select_inputs_deduct_from_input` (transfer.rs:511-557) that shipped as generic AddressOperation strings without a debug_assert companion. A real logic regression there would have been swallowed in release without panic, log, or metric. Add debug_assert! before each Err so test/dev builds surface the regression at the violation site instead of relying on a downstream validate_structure rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…k to platform#3040, saturating donor redistribution (#3554 review) Audit findings F-002, F-004, F-008 against packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs. F-002 (MEDIUM): private fn comments carried multi-paragraph phase narration that violated the coding-best-practices ≤3 line cap for internal commentary. Trimmed each block to the minimum that explains *why* something is non-obvious; moved decision-history out of source (this commit message is the new home). Sites: lines 146-156 (auto_select_inputs rustdoc), 210-216, 432-453, 558-568, 711-715, 744-748, 776-784. F-004 (MEDIUM): replaced the prose-only "KNOWN BUG — platform #3040" block with a single-line `// TODO(platform#3040)` directly above both the `estimate_fee_for_inputs` call and `REDUCE_OUTPUT_FEE_SAFETY_MULTIPLE` const, so refactors can't separate the rationale from the heuristic. Added `tracking_issue = "platform#3040"` field to the `tracing::warn!` call so the upstream link surfaces in logs. F-008 (LOW): replaced bare `donor_consumed - shift` / `last_consumed + shift` with `saturating_sub`/`saturating_add` (lines 678-679) for consistency with the file-wide saturating-arithmetic policy on monetary `Credits` (= u64). Underflow/overflow remained unreachable by construction; this is defense in depth + audit-grep cleanliness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fields (#3554 review) Audit F-003 (MEDIUM) at packages/rs-platform-wallet/src/error.rs:78-92. The previous `#[error(...)]` template hard-coded both failure-shape sections (`funded_outputs={...} sub_min_count={...} sub_min_aggregate={...}`), so the common funded-only path rendered `sub_min_count=0 sub_min_aggregate=0 credits` — misleading operators into thinking dust coexists when it does not. Replaced the static template with a `format_no_selectable_inputs` helper referenced from the `#[error(...)]` attribute. The helper conditionally includes only the contributing failure shape(s): - funded_outputs only → `funded_outputs=[…]` - sub_min only → `sub_min_count=N sub_min_aggregate=M credits` - both → both, joined - neither (defensive) → `no funded inputs survived the auto-selection filter` The `min_input_amount` and remediation hint remain in every rendering. The existing `detect_no_selectable_inputs_combines_both_cases` test still passes unchanged because it exercises the both-shapes branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allet-auto-select-inputs
thepastaclaw
reviewed
May 9, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review at HEAD 22149b0. The core selector correctness work is solid; remaining findings are docs/architecture/test-coverage suggestions. Three findings recur across reviewers with high confidence: stale InputSelection::Auto public docstring, the new transfer_with_change_address documenting full-spend semantics that are incompatible with DeductFromInput(*) fee strategies, and the is_empty_no_records() / CoreChangeSet::is_empty() asymmetry on addresses_derived (verified at changeset.rs:206 vs core_bridge.rs:319-326). Two test-coverage gaps round out the list. No blocking issues.
Reviewed commit: 22149b0
🟡 5 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🟡 suggestion: `InputSelection::Auto` docstring describes the pre-PR selector contract
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
Verified at HEAD: the doc still says Auto consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." InputSelection is pub, so this is part of the crate's API surface, and the implementation diverges on five externally observable axes:
transfer()(transfer.rs:71-84) rejects every fee strategy other than[DeductFromInput(0)]and[ReduceOutput(0)]forAuto.build_auto_select_candidatessorts candidates balance-descending, not by derivation index.- Addresses with balance below the protocol's
min_input_amountare filtered out. - Any address that also appears as a destination output is excluded (protocol forbids same-address input/output).
- Empty-candidate failures surface as the typed
PlatformWalletError::NoSelectableInputspayload (transfer.rs:294-302; error.rs:77-99).
Downstream callers reading the docstring get the wrong mental model for spend order, fragmentation profile, accepted fee strategies, and the structured failure payload.
💡 Suggested change
/// Automatically select inputs from the account, consuming addresses
/// in balance-descending order until the required amount plus
/// estimated fees is covered. Addresses with balance below the
/// protocol's per-input minimum (`min_input_amount`) are skipped,
/// and any address that is also a destination output is excluded
/// (the protocol forbids the same address being both input and
/// output of a single transition); when this filtering empties the
/// candidate set, `transfer()` returns the typed
/// [`PlatformWalletError::NoSelectableInputs`] payload so callers
/// can offer rotation vs. consolidation guidance.
///
/// Supported fee strategies: `[DeductFromInput(0)]` and
/// `[ReduceOutput(0)]`. Other strategies must use `Explicit`.
Auto,
🟡 suggestion: `is_empty_no_records()` no longer matches `CoreChangeSet::is_empty()` on `addresses_derived`
packages/rs-platform-wallet/src/changeset/core_bridge.rs (lines 319-326)
Verified: CoreChangeSet::is_empty() at changeset.rs:199-207 includes && self.addresses_derived.is_empty() as the final clause, but the adapter's fast-path helper at core_bridge.rs:319-326 omits it. The fast-path is the only caller of the helper and decides whether persister.store(...) runs at core_bridge.rs:82-87.
Under current event projections this is not currently a dropped-callback bug: WalletEvent::TransactionDetected always populates records (line 137), and WalletEvent::BlockProcessed unconditionally sets last_processed_height = Some(*height) (line 187), so the helper returns false for both shapes regardless of addresses_derived. The asymmetry is still a maintenance hazard — the subsystem now has two definitions of "empty core changeset" in the same file, and any future event variant or projection refactor that produces a derived-address-only batch would silently bypass persistence while CoreChangeSet's own contract treats it as persisted state. Cheap, defensive fix: mirror the authoritative predicate.
💡 Suggested change
fn is_empty_no_records(&self) -> bool {
self.records.is_empty()
&& self.spent_utxos.is_empty()
&& self.new_utxos.is_empty()
&& self.instant_locks_for_non_final_records.is_empty()
&& self.last_processed_height.is_none()
&& self.synced_height.is_none()
&& self.addresses_derived.is_empty()
}
🤖 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`:
- [SUGGESTION] lines 27-30: `InputSelection::Auto` docstring describes the pre-PR selector contract
Verified at HEAD: the doc still says `Auto` consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." `InputSelection` is `pub`, so this is part of the crate's API surface, and the implementation diverges on five externally observable axes:
1. `transfer()` (transfer.rs:71-84) rejects every fee strategy other than `[DeductFromInput(0)]` and `[ReduceOutput(0)]` for `Auto`.
2. `build_auto_select_candidates` sorts candidates **balance-descending**, not by derivation index.
3. Addresses with balance below the protocol's `min_input_amount` are filtered out.
4. Any address that also appears as a destination output is excluded (protocol forbids same-address input/output).
5. Empty-candidate failures surface as the typed `PlatformWalletError::NoSelectableInputs` payload (transfer.rs:294-302; error.rs:77-99).
Downstream callers reading the docstring get the wrong mental model for spend order, fragmentation profile, accepted fee strategies, and the structured failure payload.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 159-233: `transfer_with_change_address` documents "every input spent in full" but that is incompatible with `[DeductFromInput(*)]`
Verified against the protocol at `packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs:11-44`: the `inputs` map passed into `deduct_fee_from_outputs_or_remaining_balance_of_inputs` carries the **remaining balance after the transfer** (e.g., "if address A had 10 and we're sending 3, the input shows A: 7 remaining"). `DeductFromInput(0)` deducts the fee from that remaining balance.
The new public method's docstring at transfer.rs:159-170 states `change_addr` absorbs `Σ inputs − Σ user_outputs` and "The map's values must equal the full balances the caller wants consumed." Under that contract the lex-smallest input's remaining balance is 0, so any fee under `[DeductFromInput(0)]` is uncovered → `fee_fully_covered = false` → on-chain rejection.
No fund loss (the protocol catches it deterministically), but the public API ships a documented usage that fails on chain for the common `[DeductFromInput(0)]` path with no client-side guard. Pick one: (a) reject `[DeductFromInput(*)]` in `transfer_with_change_address` with a clear redirect to `[ReduceOutput(0)]`; or (b) keep admission but tighten the docstring to say the caller must reserve fee headroom on input 0 under `DeductFromInput`, and explain that `change_amount = (Σ consumed) − (Σ user_outputs)` may then be smaller than `(Σ balances) − (Σ user_outputs)`.
- [SUGGESTION] lines 659-813: `select_inputs_reduce_output` is covered by only a single happy-path test
Verified at HEAD: the only `reduce_output_*` test in the module is `reduce_output_happy_path_single_input` (transfer.rs:1164-1183), which exercises one address with an oversized balance and the trivial 1-input trim. Every non-trivial branch in `select_inputs_reduce_output` (transfer.rs:659-813) is uncovered:
- Phase 1 insufficient-balance error (lines 702-708)
- Module-internal sub-min defensive guard (lines 712-724)
- Phase 2 multi-input surplus trim (lines 728-738)
- Phase 3 donor redistribution success — candidates `[(big, X), (medium, Y)]` where `Y - surplus < min_input_amount` so the trim drops the last entry below the per-input minimum and a donor lifts it back (lines 744-766)
- Phase 3 redistribution failure — no donor has `min_input_amount + shift` of headroom (lines 756-762)
- `output_0 < estimated_fee` rejection (lines 778-784)
- CMT-001 `tracing::warn` band (lines 786-804)
Phase 3 is the most non-trivial logic in the helper; a future regression there will compile cleanly and only surface as runtime rejection. Suggested fixture for the redistribution-success case: `[(addr_a, 109_000_000), (addr_b, 30_000)]` with `total_output = 109_020_000` (last gets trimmed to 10k, donor lifts it to 100k).
- [SUGGESTION] lines 179-233: `transfer_with_change_address` is a new public API with no direct test coverage
PA-001b adds three unit tests at transfer.rs:1227-1287, but every one of them targets only the private `augment_outputs_with_change` helper. The new public async method `transfer_with_change_address` (lines 179-233) has no test exercising any of its branches. In particular, no test pins:
1. The Auto-rejection error message and variant (lines 211-218) — easy to verify with a small construction since the rejection happens before any I/O.
2. The `ExplicitWithNonces` sum path (line 207-209) — the `(_n, c)` destructure is not type-checked anywhere outside compile time; if the tuple shape ever flips, the bug ships silently.
3. The interaction between `change_addr`'s lex order vs. `user_target`'s lex order under `[ReduceOutput(0)]` — when `change_addr < user_target`, the change output is index 0 and absorbs the fee; when `change_addr > user_target`, the user's intended output is reduced. Either ordering is valid, but the asymmetry isn't documented in the API contract.
`augment_outputs_with_change` is the hot helper; the public method itself is the API surface downstream callers (Swift app, FFI) will exercise. End-to-end coverage on the Auto-rejection branch and at least one Explicit sum path would prevent silent regressions in the public surface.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] lines 319-326: `is_empty_no_records()` no longer matches `CoreChangeSet::is_empty()` on `addresses_derived`
Verified: `CoreChangeSet::is_empty()` at `changeset.rs:199-207` includes `&& self.addresses_derived.is_empty()` as the final clause, but the adapter's fast-path helper at `core_bridge.rs:319-326` omits it. The fast-path is the only caller of the helper and decides whether `persister.store(...)` runs at `core_bridge.rs:82-87`.
Under current event projections this is not currently a dropped-callback bug: `WalletEvent::TransactionDetected` always populates `records` (line 137), and `WalletEvent::BlockProcessed` unconditionally sets `last_processed_height = Some(*height)` (line 187), so the helper returns `false` for both shapes regardless of `addresses_derived`. The asymmetry is still a maintenance hazard — the subsystem now has two definitions of "empty core changeset" in the same file, and any future event variant or projection refactor that produces a derived-address-only batch would silently bypass persistence while `CoreChangeSet`'s own contract treats it as persisted state. Cheap, defensive fix: mirror the authoritative predicate.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all carryover and new findings against HEAD 0a5aa2e. The selector rewrite is sound; remaining issues are correctness/docs/coverage on adjacent surface. Highest-impact item is the new transfer_with_change_address API admitting [DeductFromInput(*)] strategies that the protocol cannot satisfy under the documented full-spend contract — both reviewers converged on this and DPP-side verification (deduct_fee_from_inputs_and_outputs/v0/mod.rs:11-50 plus address_balances_and_nonces.rs:113-122 confirming inputs map = consumed-amount → remaining = balance − consumed) confirms it. No fund loss, no consensus risk; protocol rejects deterministically.
Reviewed commit: 0a5aa2e
🟡 6 suggestion(s) | 💬 2 nitpick(s)
3 additional findings
🟡 suggestion: `InputSelection::Auto` docstring describes the pre-PR selection policy
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
InputSelection is pub, so this is API documentation. The doc still says Auto consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." The current implementation diverges on five externally observable axes (verified in transfer.rs):
transfer()(transfer.rs:74-84) only accepts fee strategies[DeductFromInput(0)]and[ReduceOutput(0)]underAuto.build_auto_select_candidatessorts candidates balance-descending, not by derivation index (transfer.rs:384).- Addresses with balance below
min_input_amountare filtered out (transfer.rs:382). - Any address that also appears as a destination output is excluded — the protocol forbids the same address being both input and output (transfer.rs:382).
- Empty-candidate failures surface as the typed
PlatformWalletError::NoSelectableInputspayload (transfer.rs:294-302).
Downstream callers reading the docstring get the wrong mental model for spend order, fragmentation profile, accepted fee strategies, and the structured failure payload.
💡 Suggested change
/// Automatically select inputs from the account, consuming addresses
/// in balance-descending order until the required amount plus
/// estimated fees is covered. Addresses with balance below the
/// protocol's per-input minimum (`min_input_amount`) are skipped,
/// and any address that is also a destination output is excluded
/// (the protocol forbids the same address being both input and
/// output of a single transition); when this filtering empties the
/// candidate set, `transfer()` returns the typed
/// [`PlatformWalletError::NoSelectableInputs`] payload so callers
/// can offer rotation vs. consolidation guidance.
///
/// Supported fee strategies: `[DeductFromInput(0)]` and
/// `[ReduceOutput(0)]`. Other strategies must use `Explicit`.
Auto,
🟡 suggestion: `is_empty_no_records()` no longer matches `CoreChangeSet::is_empty()` on `addresses_derived`
packages/rs-platform-wallet/src/changeset/core_bridge.rs (lines 319-326)
Verified at HEAD: CoreChangeSet::is_empty() (changeset.rs:199-207) ends with && self.addresses_derived.is_empty(), but the adapter's fast-path helper (core_bridge.rs:319-326) omits that clause. The fast-path is the sole gate on whether persister.store(...) runs (core_bridge.rs:82-87).
Under current event projections this is not a dropped-callback bug: WalletEvent::TransactionDetected always populates records, and WalletEvent::BlockProcessed unconditionally sets last_processed_height = Some(*height), so the helper returns false for both shapes regardless of addresses_derived. The asymmetry remains a maintenance hazard — two different definitions of "empty core changeset" in the same subsystem, and any future event variant or projection refactor producing a derived-address-only batch would silently bypass persistence while CoreChangeSet treats it as persisted state. Mirror the authoritative predicate.
💡 Suggested change
fn is_empty_no_records(&self) -> bool {
self.records.is_empty()
&& self.spent_utxos.is_empty()
&& self.new_utxos.is_empty()
&& self.instant_locks_for_non_final_records.is_empty()
&& self.last_processed_height.is_none()
&& self.synced_height.is_none()
&& self.addresses_derived.is_empty()
}
🟡 suggestion: New structured `NoSelectableInputs` variant is flattened to `ErrorUnknown + message` across the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
This PR adds a structured PlatformWalletError::NoSelectableInputs { funded_outputs, sub_min_count, sub_min_aggregate, min_input_amount } (rs-platform-wallet/src/error.rs:77-87) so Rust callers can branch on the failure shape without parsing message strings. The FFI mapping at rs-platform-wallet-ffi/src/error.rs:157-161 still maps every variant to PlatformWalletFFIResultCode::ErrorUnknown and only forwards error.to_string(), so Swift/iOS callers crossing the FFI boundary fall back to string inspection — defeating the cross-language value of the new variant.
The FFI file is outside this PR's diff, so this is a follow-up surface item, but the new variant is the right opportunity to introduce a more specific result code (e.g., ErrorInvalidParameter or a new ErrorNoSelectableInputs) so iOS callers can distinguish funded-output overlap vs. all-sub-minimum without parsing the message format.
🤖 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`:
- [SUGGESTION] lines 159-233: `transfer_with_change_address` admits `[DeductFromInput(*)]` strategies that its full-spend contract cannot satisfy
The `Some(change_addr)` branch documents and implements a full-spend contract (transfer.rs:159-170): each explicit input's map value equals the entire balance to be consumed, and `change_addr` absorbs `Σ inputs − Σ user_outputs`. `fee_strategy` is then forwarded unchanged to `transfer()` (transfer.rs:224-232) without any compatibility check.
Verified against the protocol path: `address_balances_and_nonces.rs:120-122` computes `remaining_balance = actual_balance − requested_amount`, then `validate_fees_of_event/v0/mod.rs:201-207` feeds those remaining balances into `deduct_fee_from_outputs_or_remaining_balance_of_inputs_v0`, which only sees the post-transfer remainders (`deduct_fee_from_inputs_and_outputs/v0/mod.rs:11-50`). Under the full-spend contract every input's remaining balance is 0, so any `DeductFromInput(i)` step cannot deduct and `fee_fully_covered` is false → deterministic on-chain rejection.
No fund loss (the protocol catches it), but the public API ships a documented usage that fails on chain for the common `[DeductFromInput(0)]` path with no client-side guard. Pick one of: (a) reject `[DeductFromInput(*)]` at this boundary with a redirect to `[ReduceOutput(0)]`; or (b) tighten the docstring to state that callers using `DeductFromInput` must reserve fee headroom on the lex-smallest input, in which case `change_amount` is smaller than the documented `(Σ balances) − (Σ user_outputs)`.
- [SUGGESTION] lines 659-813: `select_inputs_reduce_output` has only a single happy-path test for the most non-trivial selector logic
The only `reduce_output_*` test in the module is `reduce_output_happy_path_single_input` (transfer.rs:1165), exercising one address with an oversized balance and a trivial 1-input trim. Every non-trivial branch in `select_inputs_reduce_output` (transfer.rs:659-813) is uncovered:
- Phase 1 insufficient-balance error (702-708)
- Module-internal sub-min defensive guard (712-724)
- Phase 2 multi-input surplus trim (728-738)
- Phase 3 donor redistribution success — `[(big, X), (medium, Y)]` where `Y − surplus < min_input_amount` so the trim drops the tail and a donor lifts it back (744-766)
- Phase 3 redistribution failure — no donor has `min_input_amount + shift` of headroom (756-762)
- `output_0 < estimated_fee` rejection (778-784)
- CMT-001 `tracing::warn` band (786-804)
Phase 3 is the most non-trivial logic in the helper; a future regression there will compile cleanly and only surface as runtime chain-side rejection. Suggested fixture for redistribution success: `[(addr_a, 109_000_000), (addr_b, 30_000)]` with `total_output = 109_020_000` (tail trims to 10k, donor lifts to 100k).
- [SUGGESTION] lines 179-233: `transfer_with_change_address` is a new public API with no direct test coverage
The three PA-001b unit tests at transfer.rs:1231-1287 target only the private `augment_outputs_with_change` helper. The new public async `transfer_with_change_address` (lines 179-233) has no test exercising any of its branches. In particular, no test pins:
1. The `Auto`-rejection error message and variant (lines 211-218) — easy to verify without a signer since the rejection happens before any I/O.
2. The `ExplicitWithNonces` sum path (lines 207-209) — the `(_n, c)` destructure isn't type-checked anywhere outside compile time; if the tuple shape ever flips the bug ships silently.
3. The interaction between `change_addr`'s lex order vs. `user_target`'s lex order under `[ReduceOutput(0)]` — when `change_addr < user_target`, the change output is index 0 and absorbs the fee; when `change_addr > user_target`, the user's intended output is reduced. Either ordering is valid, but the asymmetry is not documented in the API contract.
End-to-end coverage on at least the `Auto`-rejection branch (pure error-path, no signer needed) would prevent silent regressions in the public surface.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs`:
- [SUGGESTION] lines 27-30: `InputSelection::Auto` docstring describes the pre-PR selection policy
`InputSelection` is `pub`, so this is API documentation. The doc still says `Auto` consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." The current implementation diverges on five externally observable axes (verified in transfer.rs):
1. `transfer()` (transfer.rs:74-84) only accepts fee strategies `[DeductFromInput(0)]` and `[ReduceOutput(0)]` under `Auto`.
2. `build_auto_select_candidates` sorts candidates **balance-descending**, not by derivation index (transfer.rs:384).
3. Addresses with balance below `min_input_amount` are filtered out (transfer.rs:382).
4. Any address that also appears as a destination output is excluded — the protocol forbids the same address being both input and output (transfer.rs:382).
5. Empty-candidate failures surface as the typed `PlatformWalletError::NoSelectableInputs` payload (transfer.rs:294-302).
Downstream callers reading the docstring get the wrong mental model for spend order, fragmentation profile, accepted fee strategies, and the structured failure payload.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] lines 319-326: `is_empty_no_records()` no longer matches `CoreChangeSet::is_empty()` on `addresses_derived`
Verified at HEAD: `CoreChangeSet::is_empty()` (changeset.rs:199-207) ends with `&& self.addresses_derived.is_empty()`, but the adapter's fast-path helper (core_bridge.rs:319-326) omits that clause. The fast-path is the sole gate on whether `persister.store(...)` runs (core_bridge.rs:82-87).
Under current event projections this is not a dropped-callback bug: `WalletEvent::TransactionDetected` always populates `records`, and `WalletEvent::BlockProcessed` unconditionally sets `last_processed_height = Some(*height)`, so the helper returns `false` for both shapes regardless of `addresses_derived`. The asymmetry remains a maintenance hazard — two different definitions of "empty core changeset" in the same subsystem, and any future event variant or projection refactor producing a derived-address-only batch would silently bypass persistence while `CoreChangeSet` treats it as persisted state. Mirror the authoritative predicate.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: New structured `NoSelectableInputs` variant is flattened to `ErrorUnknown + message` across the FFI boundary
This PR adds a structured `PlatformWalletError::NoSelectableInputs { funded_outputs, sub_min_count, sub_min_aggregate, min_input_amount }` (rs-platform-wallet/src/error.rs:77-87) so Rust callers can branch on the failure shape without parsing message strings. The FFI mapping at rs-platform-wallet-ffi/src/error.rs:157-161 still maps every variant to `PlatformWalletFFIResultCode::ErrorUnknown` and only forwards `error.to_string()`, so Swift/iOS callers crossing the FFI boundary fall back to string inspection — defeating the cross-language value of the new variant.
The FFI file is outside this PR's diff, so this is a follow-up surface item, but the new variant is the right opportunity to introduce a more specific result code (e.g., `ErrorInvalidParameter` or a new `ErrorNoSelectableInputs`) so iOS callers can distinguish funded-output overlap vs. all-sub-minimum without parsing the message format.
lklimek
reviewed
May 13, 2026
…o-rejection Close the coverage gap the reviewer raised four times (CMT-002, 004, 005, 006) and CMT-007's wrapper test: - `reduce_output_phase3_donor_lift_success` — two-input prefix where the trim drops the last entry below `min_input_amount` and the donor has the headroom to lift it back; pins the post-shift consumption for both entries. - `reduce_output_phase3_redistribution_fails_when_no_donor` — last entry trimmed below min_input but no peer can donate without dropping below min_input itself; selector must error out. - `reduce_output_phase1_insufficient_aggregate_balance` — aggregate candidate balance below total_output; selector must reject with the insufficient-balance diagnostic. - `transfer_with_change_address_rejects_auto_selection` — covers the public wrapper's Auto-rejection branch end-to-end with a mock SDK, no signer required (the rejection short-circuits before any I/O). - `validate_change_address_rejects_both_collision_shapes` — pins the new collision-check helper's two failure paths and the accept path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rbose comments - CMT-001 (option b): `transfer_with_change_address` rustdoc now spells out the DeductFromInput fee-headroom requirement explicitly: the caller MUST keep the targeted input's map value strictly below its on-chain balance by ≥ estimated fee, and the resulting `change_amount = (Σ consumed) − (Σ user_outputs)` may be smaller than `(Σ balances) − (Σ user_outputs)`. No runtime reject — the contract is documented instead. - CMT-013/014: trim verbose internal comments per the coding-best- practices comment-budget. Persister sync rationale, saturating- arithmetic recitals, defensive post-check explanations, and the donor rationale all collapse to ≤3 lines. Public rustdoc on `transfer_with_change_address` stays in the 5-10 line teaching budget. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CMT-008: tighten the inline donor-selection rationale to spell out the balance-descending iteration order and call out the local re-sort. CMT-009: surface the module-internal sub-`min_input_amount` guard in the `select_inputs_reduce_output` rustdoc so direct callers (tests, future consumers) see the invariant without reading the body. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…llision helpers - Add `saturating_sum_credits` in the platform_addresses module and route every production-path Credits sum through it. The file-wide policy comment already advertised saturating arithmetic; the three raw `Iterator::sum::<Credits>()` calls (input_sum in both Explicit branches and user_output_sum in augment_outputs_with_change) were forward traps. - Add `validate_change_address` so the change-address override now rejects collisions with both user_outputs AND input addresses before building the augmented output map. Previously a caller supplying a change_addr already present in `inputs` produced a transition the protocol rejects deterministically with `OutputAddressAlsoInputError`; we now fail fast with a clear local diagnostic. - Replace the filter-map-sum at `other_total` with `accumulated.saturating_sub(fee_target_balance)` — identical semantics, fewer lines, drops another non-saturating `.sum()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`PlatformAddress` already implements `std::fmt::Display` returning the same `P2PKH(<hex>)` / `P2SH(<hex>)` shape. Drop the local helper and let callers rely on the standard format machinery directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iants
The combined `NoSelectableInputs { funded_outputs, sub_min_count,
sub_min_aggregate, min_input_amount }` variant forced callers to
inspect numeric fields to learn which failure shape they had hit.
Re-split into two discrete variants — `OnlyOutputAddressesFunded` and
`OnlyDustInputs` — so callers can match the diagnostic via the type
system. When both shapes apply, `OnlyOutputAddressesFunded` wins:
rotating the receive address is the more actionable fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 47-line "Free runner disk space" step is unrelated to the auto_select_inputs bug fix that owns this PR. Restore the workflow to its v3.1-dev state per reviewer request; split-out the cleanup change into a separate PR if still wanted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`platform_wallet_info_create_from_mnemonic` now takes 3 args (network, mnemonic, out_handle); the integration tests still passed a 4th `std::ptr::null()` between `mnemonic` and `out_handle`, breaking the mac nextest job. Sync both call sites with the current FFI signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-existing `matches!(result, Err(_))` patterns trip `clippy::redundant_pattern_matching` under the workspace's `-D warnings` gate. Swap to `result.is_err()` so the clippy step stays green for the crates this PR touches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
All 17 actionable review threads addressed. 10 commits landed at CMT → SHA mapping
CMT-012 was triaged as a false positive earlier and is not part of this batch. Deviation notesTwo places where the implementation deviates from the literal reviewer suggestion — flagging here so nobody has to dig:
Bonus commitsTwo extras beyond the fix brief:
CI fix bundled
Quality gates
🤖 Co-authored by Claudius the Magnificent |
7 tasks
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review at HEAD c5511a7. The selector rewrite, typed error split, and new collision/auto-rejection tests close most prior findings. Three independent reviewers (Claude general, Claude rust-quality, Codex general, Codex rust-quality) converge on a stale InputSelection::Auto public docstring, and four reviewers converge on is_empty_no_records() drifting from CoreChangeSet::is_empty() on the addresses_derived clause. Both FFI agents flag that the new typed OnlyOutputAddressesFunded / OnlyDustInputs payloads still flatten to ErrorUnknown at the FFI boundary, losing the typed contract for Swift callers. No blockers.
Reviewed commit: c5511a7
🟡 3 suggestion(s) | 💬 2 nitpick(s)
4 additional findings
🟡 suggestion: `InputSelection::Auto` docstring no longer matches the implemented contract
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 37-40)
Verified at HEAD (mod.rs:37-40). InputSelection is pub, so its docs are part of this crate's API surface. The Auto doc still claims inputs are consumed "from lowest derivation index to highest until the required amount plus estimated fees is covered." The implementation diverges on five externally observable axes:
transfer()only accepts fee strategies[DeductFromInput(0)]and[ReduceOutput(0)]underAuto; other strategies return a typed error.build_auto_select_candidatessorts candidates balance-descending, not by derivation index.- Addresses with balance below
min_input_amountare filtered out. - Any address that also appears as a destination output is excluded — the protocol forbids the same address as both input and output of a state transition.
- Empty-candidate failures surface as the typed
PlatformWalletError::OnlyOutputAddressesFundedorOnlyDustInputspayloads, so callers branch on the failure shape without parsing message strings.
Downstream callers reading the docstring get the wrong mental model for spend order, eligibility rules, accepted fee strategies, and the structured failure payload they need to handle.
💡 Suggested change
/// Automatically select inputs from the account, consuming addresses
/// in balance-descending order until the required amount plus
/// estimated fees is covered. Addresses with balance below the
/// protocol's per-input minimum (`min_input_amount`) are skipped,
/// and any address that also appears as a destination output is
/// excluded (the protocol forbids the same address being both input
/// and output of a single transition); when this filtering empties
/// the candidate set, `transfer()` returns the typed
/// [`PlatformWalletError::OnlyOutputAddressesFunded`] or
/// [`PlatformWalletError::OnlyDustInputs`] payload so callers can
/// offer rotation vs. consolidation guidance.
///
/// Supported fee strategies: `[DeductFromInput(0)]` and
/// `[ReduceOutput(0)]`. Other strategies must use `Explicit`.
Auto,
🟡 suggestion: `is_empty_no_records()` diverges from `CoreChangeSet::is_empty()` on `addresses_derived`
packages/rs-platform-wallet/src/changeset/core_bridge.rs (lines 319-326)
Verified at HEAD: CoreChangeSet::is_empty() at changeset.rs:199-207 ends with && self.addresses_derived.is_empty(), but the adapter's fast-path helper at core_bridge.rs:319-326 omits that clause. The fast-path is the sole gate on whether persister.store(...) runs — the entry point that forwards derived-address updates across the Rust→Swift FFI boundary via on_persist_account_address_pools_fn.
Under current event projections this is not a live dropped-callback bug: WalletEvent::TransactionDetected always populates records and WalletEvent::BlockProcessed unconditionally sets last_processed_height = Some(*height), so the helper returns false for both shapes regardless of addresses_derived. The asymmetry remains a maintenance hazard — the subsystem has two definitions of "empty core changeset", and any future event variant or projection refactor producing a derived-address-only batch would silently bypass persistence (and the Swift callback) while CoreChangeSet itself treats it as persisted state. Cheap defensive fix: mirror the authoritative predicate.
💡 Suggested change
fn is_empty_no_records(&self) -> bool {
self.records.is_empty()
&& self.spent_utxos.is_empty()
&& self.new_utxos.is_empty()
&& self.instant_locks_for_non_final_records.is_empty()
&& self.last_processed_height.is_none()
&& self.synced_height.is_none()
&& self.addresses_derived.is_empty()
}
🟡 suggestion: New typed `OnlyOutputAddressesFunded` / `OnlyDustInputs` variants flatten to `ErrorUnknown` at the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
This PR (commit aba7ca3) replaces the prior combined NoSelectableInputs struct variant with two typed variants — OnlyOutputAddressesFunded { funded_outputs, min_input_amount } and OnlyDustInputs { sub_min_count, sub_min_aggregate, min_input_amount } (rs-platform-wallet/src/error.rs:77-103). The stated Rust-side rationale is precisely the FFI use case: callers can branch on the failure shape (rotate to a fresh receive address vs. consolidate dust) without parsing message strings.
Verified at HEAD (rs-platform-wallet-ffi/src/error.rs:157-161): the blanket impl From<PlatformWalletError> for PlatformWalletFFIResult still maps every variant to PlatformWalletFFIResultCode::ErrorUnknown and only forwards error.to_string(). These variants are reachable via the exported platform_address_wallet_transfer() path, and Swift's PlatformWalletError.init(result:) dispatches purely on result.code, so Swift/iOS callers cannot distinguish the two failure shapes without substring-matching the message — exactly the pattern the typed split was designed to avoid. Add dedicated result codes (e.g. ErrorOutputCollision / ErrorDustInputs, or a single ErrorNoSelectableInputs) and a match-arm conversion so the typed contract crosses the language boundary intact.
💬 nitpick: New balance-hydration path in `initialize_from_persisted` has no direct test coverage
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs (lines 101-140)
The new hydration block (wallet.rs:111-125) is a real behavioral fix: without it, restoring a wallet from persistence would leave account.address_credit_balance empty until a fresh BLAST round fired on_address_found for every known address, so auto_select_inputs would surface OnlyDustInputs/Insufficient balance to the caller despite the persisted balances being non-zero. There is no unit or integration test that exercises the restore→transfer flow; the failure mode would be silent from the user's perspective and only surface as a regression report from a Swift integration test. A test wiring initialize_from_persisted with a non-empty PlatformAddressSyncStartState and asserting account.address_credit_balance(&p2pkh) == funds.balance for each persisted address would pin the invariant and document the write-lock-drop-before-read-lock ordering for future maintainers.
🤖 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`:
- [SUGGESTION] lines 37-40: `InputSelection::Auto` docstring no longer matches the implemented contract
Verified at HEAD (mod.rs:37-40). `InputSelection` is `pub`, so its docs are part of this crate's API surface. The `Auto` doc still claims inputs are consumed "from lowest derivation index to highest until the required amount plus estimated fees is covered." The implementation diverges on five externally observable axes:
1. `transfer()` only accepts fee strategies `[DeductFromInput(0)]` and `[ReduceOutput(0)]` under `Auto`; other strategies return a typed error.
2. `build_auto_select_candidates` sorts candidates **balance-descending**, not by derivation index.
3. Addresses with balance below `min_input_amount` are filtered out.
4. Any address that also appears as a destination output is excluded — the protocol forbids the same address as both input and output of a state transition.
5. Empty-candidate failures surface as the typed `PlatformWalletError::OnlyOutputAddressesFunded` or `OnlyDustInputs` payloads, so callers branch on the failure shape without parsing message strings.
Downstream callers reading the docstring get the wrong mental model for spend order, eligibility rules, accepted fee strategies, and the structured failure payload they need to handle.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] lines 319-326: `is_empty_no_records()` diverges from `CoreChangeSet::is_empty()` on `addresses_derived`
Verified at HEAD: `CoreChangeSet::is_empty()` at `changeset.rs:199-207` ends with `&& self.addresses_derived.is_empty()`, but the adapter's fast-path helper at `core_bridge.rs:319-326` omits that clause. The fast-path is the sole gate on whether `persister.store(...)` runs — the entry point that forwards derived-address updates across the Rust→Swift FFI boundary via `on_persist_account_address_pools_fn`.
Under current event projections this is not a live dropped-callback bug: `WalletEvent::TransactionDetected` always populates `records` and `WalletEvent::BlockProcessed` unconditionally sets `last_processed_height = Some(*height)`, so the helper returns `false` for both shapes regardless of `addresses_derived`. The asymmetry remains a maintenance hazard — the subsystem has two definitions of "empty core changeset", and any future event variant or projection refactor producing a derived-address-only batch would silently bypass persistence (and the Swift callback) while `CoreChangeSet` itself treats it as persisted state. Cheap defensive fix: mirror the authoritative predicate.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: New typed `OnlyOutputAddressesFunded` / `OnlyDustInputs` variants flatten to `ErrorUnknown` at the FFI boundary
This PR (commit aba7ca3caf) replaces the prior combined `NoSelectableInputs` struct variant with two typed variants — `OnlyOutputAddressesFunded { funded_outputs, min_input_amount }` and `OnlyDustInputs { sub_min_count, sub_min_aggregate, min_input_amount }` (rs-platform-wallet/src/error.rs:77-103). The stated Rust-side rationale is precisely the FFI use case: callers can branch on the failure shape (rotate to a fresh receive address vs. consolidate dust) without parsing message strings.
Verified at HEAD (rs-platform-wallet-ffi/src/error.rs:157-161): the blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` still maps every variant to `PlatformWalletFFIResultCode::ErrorUnknown` and only forwards `error.to_string()`. These variants are reachable via the exported `platform_address_wallet_transfer()` path, and Swift's `PlatformWalletError.init(result:)` dispatches purely on `result.code`, so Swift/iOS callers cannot distinguish the two failure shapes without substring-matching the message — exactly the pattern the typed split was designed to avoid. Add dedicated result codes (e.g. `ErrorOutputCollision` / `ErrorDustInputs`, or a single `ErrorNoSelectableInputs`) and a `match`-arm conversion so the typed contract crosses the language boundary intact.
Comment on lines
+161
to
+245
| /// Transfer credits with an explicit "change address" override. | ||
| /// | ||
| /// When `output_change_address` is `Some(change_addr)`, the wrapper adds | ||
| /// a change output absorbing `Σ consumed − Σ user_outputs` so the | ||
| /// `Σ inputs == Σ outputs` invariant holds. When `None`, this is a | ||
| /// passthrough to [`Self::transfer`]. | ||
| /// | ||
| /// The change branch requires [`InputSelection::Explicit`] or | ||
| /// [`InputSelection::ExplicitWithNonces`] — auto-selection trims inputs | ||
| /// to a covering prefix and has no concept of a residual. | ||
| /// | ||
| /// Under `[DeductFromInput(*)]` the caller MUST reserve fee headroom on | ||
| /// the targeted input (i.e. its map value must be strictly below its | ||
| /// on-chain balance by at least the estimated fee); otherwise the chain | ||
| /// rejects the transition with `fee_fully_covered = false`. In that | ||
| /// case `change_amount = (Σ consumed) − (Σ user_outputs)` is smaller | ||
| /// than `(Σ balances) − (Σ user_outputs)`. Under `[ReduceOutput(0)]` | ||
| /// callers may pass the full balances; output 0 absorbs the fee. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// [`PlatformWalletError::AddressOperation`] when the change branch is | ||
| /// requested with [`InputSelection::Auto`], when `change_addr` collides | ||
| /// with `user_outputs` or `inputs`, or when `Σ inputs ≤ Σ user_outputs`. | ||
| #[allow(clippy::too_many_arguments)] // mirrors `transfer` plus the change-address override. | ||
| pub async fn transfer_with_change_address<S: Signer<PlatformAddress> + Send + Sync>( | ||
| &self, | ||
| account_index: u32, | ||
| input_selection: InputSelection, | ||
| user_outputs: BTreeMap<PlatformAddress, Credits>, | ||
| output_change_address: Option<PlatformAddress>, | ||
| fee_strategy: AddressFundsFeeStrategy, | ||
| platform_version: Option<&PlatformVersion>, | ||
| address_signer: &S, | ||
| ) -> Result<PlatformAddressChangeSet, PlatformWalletError> { | ||
| let Some(change_addr) = output_change_address else { | ||
| return self | ||
| .transfer( | ||
| account_index, | ||
| input_selection, | ||
| user_outputs, | ||
| fee_strategy, | ||
| platform_version, | ||
| address_signer, | ||
| ) | ||
| .await; | ||
| }; | ||
|
|
||
| let (input_sum, augmented_selection) = match input_selection { | ||
| InputSelection::Explicit(ref inputs) => { | ||
| validate_change_address(&change_addr, &user_outputs, inputs.keys())?; | ||
| ( | ||
| saturating_sum_credits(inputs.values().copied()), | ||
| InputSelection::Explicit(inputs.clone()), | ||
| ) | ||
| } | ||
| InputSelection::ExplicitWithNonces(ref inputs) => { | ||
| validate_change_address(&change_addr, &user_outputs, inputs.keys())?; | ||
| ( | ||
| saturating_sum_credits(inputs.values().map(|(_n, c)| *c)), | ||
| InputSelection::ExplicitWithNonces(inputs.clone()), | ||
| ) | ||
| } | ||
| InputSelection::Auto => { | ||
| return Err(PlatformWalletError::AddressOperation( | ||
| "output_change_address: Some(_) requires InputSelection::Explicit \ | ||
| or ExplicitWithNonces — the auto-selector trims inputs to a covering \ | ||
| prefix and has no concept of a residual to route to a change address" | ||
| .to_string(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| let outputs_with_change = | ||
| augment_outputs_with_change(user_outputs, change_addr, input_sum)?; | ||
|
|
||
| self.transfer( | ||
| account_index, | ||
| augmented_selection, | ||
| outputs_with_change, | ||
| fee_strategy, | ||
| platform_version, | ||
| address_signer, | ||
| ) | ||
| .await |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: transfer_with_change_address explicit-input branches lack direct test coverage
The new public wrapper has meaningful logic of its own: it sums explicit inputs, validates change_addr against both user_outputs and the input map, augments outputs, then forwards to transfer(). The only direct test against transfer_with_change_address() is the Auto-rejection case. Helper-level tests cover validate_change_address() and augment_outputs_with_change() separately, but no end-to-end test pins the wrapper's explicit branches — especially the ExplicitWithNonces sum path (saturating_sum_credits(inputs.values().map(|(_n, c)| *c))). If the (AddressNonce, Credits) tuple shape ever flips, the bug compiles cleanly and only surfaces at runtime as a fee-headroom or balance mismatch. A small unit test feeding a fixed ExplicitWithNonces map plus a fresh change_addr through the wrapper would pin the contract.
source: ['claude', 'codex']
6 tasks
…allet-auto-select-inputs
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the cited source on head 30e612bdd3a8445ddd16f9f78c89613414ac8a51. The selector and restore work are largely sound, but four suggestion-level issues remain: one stale public API contract, one duplicated emptiness predicate that can drop future persisted state, one new typed error surface that still collapses at the FFI boundary, and one subtle restore path that still lacks direct regression coverage.
Reviewed commit: 30e612b
🟡 4 suggestion(s)
4 additional findings
🟡 suggestion: `InputSelection::Auto` still documents the old selector behavior
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 37-40)
The public doc comment still says auto-selection consumes addresses from lowest derivation index upward until outputs plus estimated fees are covered, but that is no longer the contract implemented by this PR. transfer() now only allows [DeductFromInput(0)] and [ReduceOutput(0)] for Auto, build_auto_select_candidates() filters out sub-minimum balances and addresses that also appear in outputs, and selection proceeds from the surviving candidates in balance-descending order. Leaving the old contract in the public enum docs will mislead downstream callers about spend order, eligibility, supported fee strategies, and the new typed empty-candidate failures.
💡 Suggested change
/// Automatically select inputs from the account, consuming addresses
/// in balance-descending order until the required amount plus
/// estimated fees is covered. Addresses with balance below the
/// protocol's per-input minimum (`min_input_amount`) are skipped,
/// and any address that also appears as a destination output is
/// excluded. Supported fee strategies: `[DeductFromInput(0)]` and
/// `[ReduceOutput(0)]`; other strategies must use `Explicit`.
Auto,
🟡 suggestion: `is_empty_no_records()` has drifted from the authoritative `CoreChangeSet::is_empty()` contract
packages/rs-platform-wallet/src/changeset/core_bridge.rs (lines 319-326)
spawn_wallet_event_adapter() uses core.is_empty_no_records() as the gate before calling persister.store(...), but this helper no longer matches CoreChangeSet::is_empty(): it ignores addresses_derived even though CoreChangeSet::is_empty() treats that field as persisted state. Current event shapes happen to also populate records or last_processed_height, so this does not drop a live event today, but it leaves a real correctness trap at the Rust→Swift persistence boundary: a future derived-address-only core changeset would be discarded before persistence or callbacks run.
💡 Suggested change
fn is_empty_no_records(&self) -> bool {
self.records.is_empty()
&& self.spent_utxos.is_empty()
&& self.new_utxos.is_empty()
&& self.instant_locks_for_non_final_records.is_empty()
&& self.last_processed_height.is_none()
&& self.synced_height.is_none()
&& self.addresses_derived.is_empty()
}
🟡 suggestion: New typed auto-selection failures still collapse to `ErrorUnknown` across the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-160)
This PR introduced structured Rust-side failures (OnlyOutputAddressesFunded and OnlyDustInputs) so callers can distinguish recovery paths without parsing strings, and those variants are reachable from the exported transfer path. The FFI conversion still maps every PlatformWalletError to PlatformWalletFFIResultCode::ErrorUnknown, and Swift reconstructs its domain error from that numeric code in PlatformWalletResult.swift:144-164. As a result, the new typed contract stops at the Rust boundary and Swift/iOS callers are pushed back to brittle message inspection.
🟡 suggestion: `initialize_from_persisted` adds restart-critical balance hydration with no direct regression test
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs (lines 101-129)
initialize_from_persisted() now does more than rebuild the provider: it first pushes each persisted balance back into ManagedPlatformAccount with set_address_credit_balance(...) before calling PlatformPaymentAddressProvider::from_persisted(). That ordering is important because the transfer and withdrawal auto-selectors read account.address_credit_balance() immediately after restore; without this hydration they would report zero spendable funds until a fresh BLAST round rediscovered each known address. I did not find a unit or integration test that restores a non-empty PlatformAddressSyncStartState and asserts that balances are usable immediately after startup, so this new behavior can regress silently.
🤖 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`:
- [SUGGESTION] lines 37-40: `InputSelection::Auto` still documents the old selector behavior
The public doc comment still says auto-selection consumes addresses from lowest derivation index upward until outputs plus estimated fees are covered, but that is no longer the contract implemented by this PR. `transfer()` now only allows `[DeductFromInput(0)]` and `[ReduceOutput(0)]` for `Auto`, `build_auto_select_candidates()` filters out sub-minimum balances and addresses that also appear in `outputs`, and selection proceeds from the surviving candidates in balance-descending order. Leaving the old contract in the public enum docs will mislead downstream callers about spend order, eligibility, supported fee strategies, and the new typed empty-candidate failures.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] lines 319-326: `is_empty_no_records()` has drifted from the authoritative `CoreChangeSet::is_empty()` contract
`spawn_wallet_event_adapter()` uses `core.is_empty_no_records()` as the gate before calling `persister.store(...)`, but this helper no longer matches `CoreChangeSet::is_empty()`: it ignores `addresses_derived` even though `CoreChangeSet::is_empty()` treats that field as persisted state. Current event shapes happen to also populate `records` or `last_processed_height`, so this does not drop a live event today, but it leaves a real correctness trap at the Rust→Swift persistence boundary: a future derived-address-only core changeset would be discarded before persistence or callbacks run.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] lines 157-160: New typed auto-selection failures still collapse to `ErrorUnknown` across the FFI boundary
This PR introduced structured Rust-side failures (`OnlyOutputAddressesFunded` and `OnlyDustInputs`) so callers can distinguish recovery paths without parsing strings, and those variants are reachable from the exported transfer path. The FFI conversion still maps every `PlatformWalletError` to `PlatformWalletFFIResultCode::ErrorUnknown`, and Swift reconstructs its domain error from that numeric code in `PlatformWalletResult.swift:144-164`. As a result, the new typed contract stops at the Rust boundary and Swift/iOS callers are pushed back to brittle message inspection.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs`:
- [SUGGESTION] lines 101-129: `initialize_from_persisted` adds restart-critical balance hydration with no direct regression test
`initialize_from_persisted()` now does more than rebuild the provider: it first pushes each persisted balance back into `ManagedPlatformAccount` with `set_address_credit_balance(...)` before calling `PlatformPaymentAddressProvider::from_persisted()`. That ordering is important because the transfer and withdrawal auto-selectors read `account.address_credit_balance()` immediately after restore; without this hydration they would report zero spendable funds until a fresh BLAST round rediscovered each known address. I did not find a unit or integration test that restores a non-empty `PlatformAddressSyncStartState` and asserts that balances are usable immediately after startup, so this new behavior can regress silently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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