fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
Open
Claudius-Maginificent wants to merge 8 commits intov3.1-devfrom
Open
fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554Claudius-Maginificent wants to merge 8 commits intov3.1-devfrom
Claudius-Maginificent wants to merge 8 commits intov3.1-devfrom
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 3c4f919) |
![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.
Comment on lines
+281
to
+285
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: estimate_fee_for_inputs_pub is a tautological re-export
estimate_fee_for_inputs doesn't take &self and isn't routed through a trait — the only reason it stays a static method on PlatformAddressWallet is that the new estimate_fee_for_inputs_pub exists to call it from module scope. Move estimate_fee_for_inputs to a free function (or pub(super)) and delete the wrapper. The current shape adds indirection and a docstring callout without any isolation benefit.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Deferring this cleanup — the wrapper does have a real reason today (select_inputs is a free function but estimate_fee_for_inputs is a static method on PlatformAddressWallet that's also called by other impl PlatformAddressWallet methods, so flipping it to pub(super) free fn is a slightly larger move than it looks). Logging this for a follow-up alongside any future selector refactor that already touches the surrounding scope; not in PR #3554's scope. Leaving open as a deliberate non-fix.
Comment on lines
+326
to
+339
| let mut accumulated: Credits = 0; | ||
|
|
||
| for (address, balance) in candidates { | ||
| chosen.push((address, balance)); | ||
| accumulated = accumulated.saturating_add(balance); | ||
|
|
||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| chosen.len(), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: accumulated.saturating_add masks overflow in the error message
accumulated = accumulated.saturating_add(balance) quietly caps at u64::MAX. Once saturated, the loop trivially passes accumulated >= required and the trim still produces a well-formed result. But the saturation also feeds the error-path message at line 378-380, which would report misleading accumulated/required values. Practically unreachable given Credits sizing, but if the helper ever sees total_output: u64 from less-trusted state, these saturations should be a hard error, not an implicit cap.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Practically unreachable — Credits = u64 and the protocol's max_credits_supply is far below u64::MAX, so the saturating_add cap is unreachable for any legitimately-sourced balance / total_output. Switching to checked_add would be cleaner defensively but isn't a real-world bug. Logging for a future hardening pass alongside other arithmetic-safety review; not in PR #3554's scope. Leaving open as a deliberate non-fix.
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()`).
Comment on lines
+285
to
+302
| /// Module-scope re-export of the per-input fee estimator so the | ||
| /// pure [`select_inputs`] helper can be unit-tested without an | ||
| /// instance of [`PlatformAddressWallet`]. | ||
| fn estimate_fee_for_inputs_pub( | ||
| input_count: usize, | ||
| output_count: usize, | ||
| fee_strategy: &[AddressFundsFeeStrategyStep], | ||
| outputs: &BTreeMap<PlatformAddress, Credits>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Credits { | ||
| PlatformAddressWallet::estimate_fee_for_inputs( | ||
| input_count, | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: estimate_fee_for_inputs_pub is a tautological re-export (carryover)
Carryover from prior review. estimate_fee_for_inputs doesn't take &self and isn't routed through a trait — the only reason it stays a static method on PlatformAddressWallet is so module-scope callers (select_inputs plus the new test sites at lines 652-653, 728-729, 780-781, 974-975) can invoke it via this wrapper. Move estimate_fee_for_inputs to a free function (or pub(super)) and delete the wrapper.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Deferring this cleanup — the wrapper does have a real reason today (select_inputs is a free function but estimate_fee_for_inputs is a static method on PlatformAddressWallet that's also called by other impl PlatformAddressWallet methods, so flipping it to pub(super) free fn is a slightly larger move than it looks). Logging this for a follow-up alongside any future selector refactor that already touches the surrounding scope; not in PR #3554's scope. Leaving open as a deliberate non-fix.
Comment on lines
+392
to
+423
| for (address, balance) in candidates { | ||
| prefix.push((address, balance)); | ||
| accumulated = accumulated.saturating_add(balance); | ||
|
|
||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| prefix.len(), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); | ||
|
|
||
| if accumulated >= required { | ||
| covered = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !covered { | ||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| prefix.len().max(1), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Insufficient balance: available {} credits, required {} (outputs {} + estimated fee {})", | ||
| accumulated, required, total_output, estimated_fee | ||
| ))); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: accumulated.saturating_add masks overflow in the error path (carryover)
Carryover from prior review. accumulated = accumulated.saturating_add(balance) (line 394) quietly caps at u64::MAX. Once saturated, the loop trivially passes accumulated >= required and reports misleading values in the line 420-423 error message. Same applies to total_output.saturating_add(estimated_fee) (lines 403, 419) and total_output.saturating_sub(other_total) (line 484). Practically unreachable given total credit supply, but if total_output ever comes from less-trusted state these saturations should be hard errors.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Practically unreachable — Credits = u64 and the protocol's max_credits_supply is far below u64::MAX, so the saturating_add cap is unreachable for any legitimately-sourced balance / total_output. Switching to checked_add would be cleaner defensively but isn't a real-world bug. Logging for a future hardening pass alongside other arithmetic-safety review; not in PR #3554's scope. Leaving open as a deliberate non-fix.
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
The PR grew through review feedback. Each commit is a focused step.
Selector correctness (5 commits)
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 (121 lib tests, all passing)
auto_select_testsmodule — 11 tests:single_input_oversized_balance_trims_to_output_amounttwo_input_selection_keeps_fee_headroom_at_index_zero(renamed from the original..._trims_only_the_lastafter the headroom fix)fee_target_keeps_remaining_for_fee_deduction— direct regression for CodeRabbit's examplepre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction— protocol-level reproduction (asserts!fee_fully_covered)fee_only_tail_input_does_not_inflate_input_sumdescending_order_picks_single_largest_when_sufficientnon_fee_target_below_min_input_redistributestotal_output_below_min_input_amount_errorsfee_headroom_violation_errorsinsufficient_balance_errorsno_candidates_errors5 of them now also 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— 121/121 passing3c4f9199(Rust workspace tests / macOS now exercising this crate, was previously skipping)Test plan
deduct_fee_from_outputs_or_remaining_balance_of_inputstransfer(),auto_select_inputs,select_inputssignatures stable;transfer()now rejects non-[DeductFromInput(0)]forAutowith a clear error)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