fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554
Open
Claudius-Maginificent wants to merge 14 commits intov3.1-devfrom
Open
fix(platform-wallet): auto_select_inputs honors Σ inputs == Σ outputs#3554Claudius-Maginificent wants to merge 14 commits intov3.1-devfrom
Claudius-Maginificent wants to merge 14 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
✅ Review complete (commit 545fa99) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 341-365: When building the final `selected` map after `accumulated
>= required`, ensure the address referenced by the fee strategy
`DeductFromInput(index)` is reserved the estimated fee before returning: locate
the `chosen` prefix and the loop that constructs `selected` using
`remaining`/`total_output`, compute the required fee headroom for the
fee-bearing input (from `fee_strategy`/`DeductFromInput`) and reduce that
input's available amount by that fee (i.e., instead of consuming up to
`remaining`, cap consumption so the fee-bearing input keeps at least
`estimated_fee`), and if that causes the input to be insufficient, continue
selecting additional candidates or return an error; update uses of `selected`,
`remaining`, `accumulated`, and `required` accordingly so the returned map
guarantees the fee-bearing input still has the reserved fee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56d75786-5d1c-44b8-a304-2e962f372120
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
lklimek
changed the title
lklimek
changed the title
lklimek
changed the title
…arget CodeRabbit caught a critical bug on PR #3554's `select_inputs`: the helper ensured `Σ inputs.credits == Σ outputs.credits` (the protocol's structural invariant) but did NOT ensure that the address targeted by `DeductFromInput(0)` had post-consumption remaining balance >= the estimated fee. Worked example from CodeRabbit: candidates = [(addr_a, 20M), (addr_b, 50M)] // addr_a < addr_b lex total_output = 30M fee_strategy = [DeductFromInput(0)] Old result = {addr_a: 20M, addr_b: 10M} // Σ matches; addr_a drained Drive applies DeductFromInput(0) over inputs sorted by key (BTreeMap order), hitting addr_a — whose remaining balance is 0 — so `min(fee, 0) = 0`, `fee_fully_covered = false`, validator rejects with AddressesNotEnoughFundsError. The Wave-8 single-input live e2e accidentally avoided this because the fee target had ~1B credits left over after consumption — multi-input auto-selected transfers would have hit it on first contact. This rewrite: - Phase 1 (unchanged): pick smallest DIP-17-ordered prefix covering total_output + estimated_fee. - Phase 2: identify the fee target = lex-smallest address in the prefix (= `BTreeMap` index 0, what `DeductFromInput(0)` will hit per `rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`). - Phase 3: consume the *minimum* allowed amount from the fee target (`max(min_input_amount, total_output − Σ other balances)`) so it retains the most remaining balance for fee deduction. Error out with a descriptive AddressOperation if even that minimum leaves less than `estimated_fee` remaining. - Phase 4: distribute the rest of `total_output` across the other prefix entries in DIP-17 order. - Phase 5: defensive invariant checks. `min_input_amount` is fetched from `platform_version.dpp.state_transitions.address_funds.min_input_amount` (currently 100k across v1/v2/v3 of platform-version). For non-`[DeductFromInput(0)]` fee strategies the helper falls back to the previous "consume from front" distribution that only enforces the Σ invariant — none of the wallet's call sites use anything else today. Tests: - updated `two_input_selection_trims_only_the_last` → `two_input_selection_keeps_fee_headroom_at_index_zero` to assert the new distribution AND the headroom invariant. - updated `fee_only_tail_input_does_not_inflate_input_sum`'s expected outputs (the tail is no longer dropped — it absorbs the consumption the fee target sheds). - added `fee_target_keeps_remaining_for_fee_deduction` (CodeRabbit's exact scenario, with the headroom invariant as the load-bearing assertion). - added `fee_headroom_violation_errors` (lex-smallest address too small to retain headroom → descriptive error rather than transition the validator will reject). - `single_input_oversized_balance_trims_to_output_amount`, `insufficient_balance_errors`, `no_candidates_errors` pass unchanged. `cargo test -p platform-wallet --lib` → 117 / 117 green `cargo clippy -p platform-wallet --tests -- -D warnings` → clean `cargo fmt -p platform-wallet --check` → clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 441-461: The distribution loop that sets each consumed = min(bal,
remaining) can insert inputs below min_input_amount (e.g., consumed <
min_input_amount); after that loop (the one that inserts entries into prefix and
uses variables fee_target_consumed, remaining, consumed, fee_target_addr),
validate all non-zero consumed values against min_input_amount and either (a)
rebalance by moving consumption from fee_target_consumed or other large entries
to bump small entries up to min_input_amount while preserving total consumption,
or (b) return an error indicating distribution impossible; implement the
simplest correct choice for your flow (prefer returning an error if safe
redistribution is complex) and ensure the function returns early on failure so
subsequent debug_asserts aren’t relied on.
- Around line 349-353: The code currently silently falls back to front-trimming
for any AddressFundsFeeStrategy other than the single-item [DeductFromInput(0)]
inside transfer()/where InputSelection::Auto is handled; change this to reject
unsupported auto-selection fee strategies by returning a clear error instead of
performing the unsafe fallback. Locate the branch handling InputSelection::Auto
in transfer.rs (the block that examines fee_strategy and falls back to
front-trimming) and add a guard that checks the strategy sequence—if it is not
exactly the single DeductFromInput(0) pattern, return an Err (with a descriptive
enum/variant or mapped error) indicating unsupported fee strategy for
auto-selection so callers cannot produce inputs that sum to outputs but will
fail on-chain once fees are applied. Ensure the new error flows out of
transfer() consistently with existing error types.
- Around line 363-385: The loop that builds the DIP-17-ordered prefix (variables
prefix, accumulated, covered) currently breaks out as soon as accumulated >=
required, which prevents trying larger prefixes when Phase 3 (fee target
feasibility using fee_target_min/fee_target_max and DeductFromInput(0)
semantics) fails; change the logic so that when a covering prefix is found you
run Phase 3 checks but do not return/error on Phase 3 failure—continue the for
(address, balance) in candidates iteration (calling estimate_fee_for_inputs_pub
as before) to grow the prefix and try later candidates until either Phase 3
succeeds or all candidates are exhausted, only then set covered/error
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 857ac9b6-556f-4771-a503-d3c0069a9ecb
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
…ee-headroom bug Adds `pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction` to the `select_inputs` test module. Reconstructs the exact `inputs` map the pre-fix `auto_select_inputs` would have returned for CodeRabbit's example (candidates (20M, 50M), total_output 30M, `DeductFromInput(0)`), runs the post-consumption remaining balances through the live dpp fee-deduction code path, and asserts `fee_fully_covered == false` — i.e. the protocol rejects it with `AddressesNotEnoughFundsError`. Distinct from `fee_target_keeps_remaining_for_fee_deduction`, which asserts the new selector's output meets the headroom invariant. This reproduction proves the bug at the protocol layer rather than merely asserting "the new output looks different" — it would have stayed red without the fix in 9ea9e70. Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 118/118 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…descending Internal-only change to `auto_select_inputs`. Candidates were previously collected in DIP-17 derivation index order; now they sort by balance descending before being handed to `select_inputs`. Mirrors the dash-evo-tool allocator (`src/ui/wallets/send_screen.rs:155-157`). Effects: - Single largest balance covering `total_output + estimated_fee` => 1-input result, no multi-input case, no lex-smallest fee headroom logic firing. Common path simplified. - Multi-input cases (when the largest alone isn't enough) still go through the headroom-respecting distribution introduced in 9ea9e70 — unchanged, still correct. - No public API change. `transfer()`, `auto_select_inputs`, `select_inputs` signatures all identical. Adds `descending_order_picks_single_largest_when_sufficient` to the existing test module to lock in the common-path behavior. Other tests pass candidates directly to `select_inputs` and are order-agnostic by design — unchanged. The `fee_headroom_violation_errors` error message now includes the fee-target address, its balance, required headroom, and remaining-after-consumption to ease debugging. Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 119/119 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (2)
442-453: Fallback path for non-[DeductFromInput(0)]strategies still risks on-chain rejection.The docstring at lines 371-375 acknowledges this limitation, but the code silently proceeds with a distribution that only guarantees
Σ inputs == Σ outputswithout reserving fee headroom on the actual fee-bearing input. Iftransfer()is ever called with a different fee strategy (e.g.,DeductFromInput(1)or multi-step strategies), the returned inputs map could still fail on-chain when the targeted input lacks remaining balance for fee deduction.A previous review suggested returning an error for unsupported strategies. The current approach documents the limitation but doesn't prevent misuse. Consider whether rejecting unsupported strategies is preferable to silent fallback with potential on-chain failure.
Alternative: Reject unsupported strategies explicitly
if !single_deduct_from_input_zero { - let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); - let mut remaining = total_output; - for (addr, bal) in prefix.iter() { - if remaining == 0 { - break; - } - let consumed = (*bal).min(remaining); - selected.insert(*addr, consumed); - remaining = remaining.saturating_sub(consumed); - } - return Ok(selected); + return Err(PlatformWalletError::AddressOperation( + "Auto input selection currently supports only [DeductFromInput(0)] fee strategy. \ + Other strategies require explicit input selection.".to_string(), + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 442 - 453, The fallback branch that builds `selected` when `!single_deduct_from_input_zero` must not silently return a transfer that may fail on-chain; instead make `transfer()` validate the fee strategy up-front and return an explicit error for unsupported strategies (e.g., when the strategy is not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is false). Replace the current loop-return branch that constructs `selected` with an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or similar), and ensure callers handle that error; keep the `prefix`-consumption logic only for the supported `single_deduct_from_input_zero` path.
405-409: Phase 1 early break may cause false "fee headroom" failures when a larger prefix would succeed.A previous review noted that because
DeductFromInput(0)targets the lex-smallest address (not the first in iteration order), a later candidate joining the prefix can become the new fee target and make an otherwise-infeasible selection work. The current code breaks at the first covering prefix without checking Phase 3 feasibility.With the descending-balance sort, this scenario is less common (the first candidates are the largest balances), but it can still occur when:
- The largest-balance address is also lex-smallest
- That address has just enough to cover
total_outputbut nottotal_output + fee- Adding a lex-smaller address would shift the fee-target role to a smaller-balance address with better headroom characteristics
Consider continuing to accumulate candidates until Phase 3 succeeds or all candidates are exhausted, rather than erroring immediately on the first Phase 3 failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 405 - 409, The early break on the first prefix that meets accumulated >= required causes false failures because Phase 3 (fee-target reassignment via DeductFromInput(0)) may succeed for a larger prefix; in the loop that checks accumulated, required and sets covered=true then break, remove the immediate break and instead, after accumulated >= required, invoke the Phase 3 feasibility check (the same logic that uses DeductFromInput(0) / the fee-target selection) and only set covered=true and stop accumulating if that Phase 3 check succeeds; if Phase 3 fails, continue accumulating more candidates and only error or mark covered=false after all candidates are exhausted. Ensure you update the variables used in the Phase 3 check to reflect the extended prefix and keep the existing semantics for setting covered and returning an error when no prefix passes Phase 3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 509-521: The loop that distributes `remaining` across `prefix` may
insert entries below `min_input_amount` because it uses `consumed =
bal.min(remaining)` without validating per-input minimum; update the Phase 4
loop (the block iterating `for (addr, bal) in prefix.iter()`) to only insert a
`consumed` value if `consumed >= min_input_amount`, otherwise do not insert that
address and instead add the small remainder to the `fee_target_addr` (or
accumulate it to be merged into the fee target while preserving any fee
headroom) so no non-fee-target input can be below `min_input_amount`; retain
existing `remaining`/`selected` semantics and keep the Phase 5 `debug_assert`
intact.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 442-453: The fallback branch that builds `selected` when
`!single_deduct_from_input_zero` must not silently return a transfer that may
fail on-chain; instead make `transfer()` validate the fee strategy up-front and
return an explicit error for unsupported strategies (e.g., when the strategy is
not `FeeStrategy::DeductFromInput(0)` or `single_deduct_from_input_zero` is
false). Replace the current loop-return branch that constructs `selected` with
an Err variant (create or reuse a `TransferError::UnsupportedFeeStrategy` or
similar), and ensure callers handle that error; keep the `prefix`-consumption
logic only for the supported `single_deduct_from_input_zero` path.
- Around line 405-409: The early break on the first prefix that meets
accumulated >= required causes false failures because Phase 3 (fee-target
reassignment via DeductFromInput(0)) may succeed for a larger prefix; in the
loop that checks accumulated, required and sets covered=true then break, remove
the immediate break and instead, after accumulated >= required, invoke the Phase
3 feasibility check (the same logic that uses DeductFromInput(0) / the
fee-target selection) and only set covered=true and stop accumulating if that
Phase 3 check succeeds; if Phase 3 fails, continue accumulating more candidates
and only error or mark covered=false after all candidates are exhausted. Ensure
you update the variables used in the Phase 3 check to reflect the extended
prefix and keep the existing semantics for setting covered and returning an
error when no prefix passes Phase 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b42e9a8-3597-407f-9ca0-e9a419a38732
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
thepastaclaw
suggested changes
Apr 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR fixes the original Σ inputs == Σ outputs bug, but introduces three new protocol violations. The selector reasons about fee headroom in DIP-17 insertion order while the chain applies DeductFromInput(i) over BTreeMap key order — combined with dropping/draining tail inputs, this leaves the actual fee-bearing input with no remaining balance. The trim can also produce inputs below min_input_amount (100_000). The new tests assert only the structural invariant and would not catch any of these regressions.
Reviewed commit: aaf8be7
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 321-365: Selector reserves fee headroom in insertion order, but the protocol charges DeductFromInput(0) against the BTreeMap-first input
`select_inputs` keeps `chosen` in insertion (DIP-17) order and consumes from front to back, fully draining every input except the last to reach exactly `total_output`. It returns a `BTreeMap`, however, and the on-chain validator resolves `DeductFromInput(index)` via `remaining_balances.iter().nth(index)` (verified at `rs-dpp/.../state_transition_estimated_fee_validation.rs:48-69`), i.e. against BTreeMap key order — the lex-smallest selected address.
When the BTreeMap-first address is not the same as the insertion-order tail, all fee headroom ends up on the wrong input. The new test `two_input_selection_trims_only_the_last` demonstrates the failure mode directly: `addr_a = [0x01;20]` (BTreeMap key 0) is consumed for its full 20M balance — remaining = 0; `addr_b` keeps 40M of headroom. With `DeductFromInput(0)` the protocol charges the fee to `addr_a`, which has 0 left, so `fee_fully_covered = false` and the transition is rejected with `AddressesNotEnoughFundsError` (`rs-drive-abci/.../validate_fees_of_event/v0/mod.rs:209-224`).
The `fee_only_tail_input_does_not_inflate_input_sum` test exposes the same root cause via dropping rather than draining: `addr_a` has `total_output + 1` and is consumed for `total_output`, leaving 1 credit of remaining balance on the only returned input — far below any realistic transfer fee. The aggregate guarantee `Σ remaining ≥ fee` is irrelevant because the protocol charges the fee from one specific input, not the aggregate.
The helper must guarantee that the input the protocol will actually charge (BTreeMap-first when `fee_strategy = [DeductFromInput(0)]`) retains at least `estimated_fee` of remaining balance after consumption. Practical options: (a) compute the BTreeMap-first selected address up front and direct the trim residue to it, leaving headroom there; (b) restrict consumption on the fee-target input to `balance − estimated_fee`. The unit tests need to assert remaining balances after consumption, not just `Σ values == total_output`, to catch this class of bug.
- [BLOCKING] lines 351-365: Trimmed tail input can fall below `min_input_amount`, causing structural rejection
The trim sets `consumed = (*bal).min(remaining)` per input, with no lower bound. The transfer validator rejects any input below `platform_version.dpp.state_transitions.address_funds.min_input_amount` (currently 100_000) with `InputBelowMinimumError` (`rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rs:146-167`).
A perfectly normal multi-input scenario triggers this: candidates A=5M, B=100M with `total_output = 5_050_000` yields `{A: 5_000_000, B: 50_000}`, and 50_000 < 100_000. Or two 100k inputs funding a 150k output: `{first: 100_000, second: 50_000}` — also rejected. None of the new tests exercise this boundary.
Fix options: filter candidates with `balance >= min_input_amount` at the call site and either skip a tail that would trim below the minimum (rolling residue back into a prior input) or refuse to add a candidate whose post-trim consumption would violate the limit. The helper aims to encode protocol correctness, so this should be enforced inside `select_inputs` rather than left to the validator.
- [SUGGESTION] lines 412-539: Tests assert the input-sum invariant only, not full state-transition validation
`single_input_oversized_balance_trims_to_output_amount`, `two_input_selection_trims_only_the_last`, and `fee_only_tail_input_does_not_inflate_input_sum` all assert only that `Σ values == total_output`. They never assemble the result into an `AddressFundsTransferTransition` and run it through `validate_structure` + the fee-validation pipeline. As a result they pass on inputs maps that the on-chain validator deterministically rejects (see the two blocking findings above). The dpp validators are pure and don't need a live node — wiring an integration assertion into these tests would catch both classes of regression and is the right substitute for the still-pending testnet rerun referenced in the PR description.
Comment on lines
+326
to
+339
| let mut accumulated: Credits = 0; | ||
|
|
||
| for (address, balance) in candidates { | ||
| chosen.push((address, balance)); | ||
| accumulated = accumulated.saturating_add(balance); | ||
|
|
||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| chosen.len(), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: accumulated.saturating_add masks overflow in the error message
accumulated = accumulated.saturating_add(balance) quietly caps at u64::MAX. Once saturated, the loop trivially passes accumulated >= required and the trim still produces a well-formed result. But the saturation also feeds the error-path message at line 378-380, which would report misleading accumulated/required values. Practically unreachable given Credits sizing, but if the helper ever sees total_output: u64 from less-trusted state, these saturations should be a hard error, not an implicit cap.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Practically unreachable — Credits = u64 and the protocol's max_credits_supply is far below u64::MAX, so the saturating_add cap is unreachable for any legitimately-sourced balance / total_output. Switching to checked_add would be cleaner defensively but isn't a real-world bug. Logging for a future hardening pass alongside other arithmetic-safety review; not in PR #3554's scope. Leaving open as a deliberate non-fix.
thepastaclaw
suggested changes
Apr 28, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The DeductFromInput(0) headroom bug from the prior review is fixed: the selector now targets the BTreeMap-first input via lex-smallest selection, and the new pre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction test exercises dpp's actual fee-deduction path. Two prior blockers remain reachable on the current head, however: Phase 4's distribution across non-fee-target inputs has no min_input_amount floor, and the public-API fallback for fee strategies other than [DeductFromInput(0)] produces input maps the validator deterministically rejects.
Reviewed commit: 60f7850
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [BLOCKING] lines 504-521: Non-fee-target inputs can still be trimmed below `min_input_amount`
Phase 3 pins `fee_target_min ≥ min_input_amount` (line 484), but Phase 4 then distributes `total_output − fee_target_consumed` across the non-fee-target prefix entries with `let consumed = (*bal).min(remaining)` and inserts any positive value (lines 516-520) — there is no per-input lower bound. The validator at `packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/v0/state_transition_validation.rs:157-167` iterates `self.inputs.values()` and rejects ANY input below `min_input_amount` (100_000) with `InputBelowMinimumError`.
A construction that survives Phase 1 and Phase 3 but trips the validator:
- candidates after balance-desc sort: `[(addr_X=0x01, 1_000_000), (addr_Y=0x02, 30_000)]`
- `total_output = 950_000`, `min_input_amount = 100_000`
- Phase 1 needs both inputs (e.g. `fee_for_1 = 60_000` → 1_000_000 < 1_010_000; `fee_for_2 = 80_000` → 1_030_000 ≥ 1_030_000 ✓)
- `fee_target = addr_X`, `fee_target_max = 920_000`, `other_total = 30_000`, `fee_target_min = max(100_000, 920_000) = 920_000` — 920_000 ≤ 920_000 ✓
- Phase 4: `remaining = 30_000` → `addr_Y` inserted as 30_000 < 100_000 → rejected.
Fix options: filter candidates with `balance < min_input_amount` at the call site; refuse to insert a non-fee-target consumption that would land below the minimum (rolling residue back to the fee target, which already has remaining headroom); or bail out with a descriptive error. The existing `fee_headroom_violation_errors` test exercises only the fee-target min-input path; an analogous test for the tail input would catch this.
- [BLOCKING] lines 442-454: Fallback path is reachable via public API and produces protocol-invalid input maps
`transfer()` (line 31) is `pub` and accepts an arbitrary `fee_strategy: AddressFundsFeeStrategy` from any caller. With `InputSelection::Auto`, this routes through `select_inputs()`, which only implements protocol-correct logic for the exact shape `[DeductFromInput(0)]` (line 437-440). For every other strategy, lines 442-453 fall back to a front-consume distribution that guarantees only `Σ inputs == total_output` and ignores both fee-target headroom and `min_input_amount`.
Reachable failure: with `fee_strategy = [DeductFromInput(1)]`, candidates `[(addr_b, 20M), (addr_a, 50M)]` where `addr_a < addr_b`, and `total_output = 30M`, the fallback returns `{addr_b: 20M, addr_a: 10M}`. The protocol resolves `DeductFromInput(i)` against BTreeMap key order (`packages/rs-dpp/src/address_funds/fee_strategy/.../v0/mod.rs`), so index 1 points at `addr_b`, which is fully drained — fee deduction fails exactly like the original bug. `ReduceOutput(...)` strategies can produce structurally invalid trailing inputs for the same reason.
The doc on lines 371-375 acknowledges this as 'must be revisited if [strategy] changes', but the public API surface is wide open today. Either constrain the strategy at the entry point, return an explicit `Err` for unsupported shapes, or extend the fee-target/min-input logic to general strategies. Returning a known-suspect map silently is the riskier option — it forces a future caller to stumble into the same protocol rejection that motivated this PR.
- [SUGGESTION] lines 484-545: `total_output < min_input_amount` falls through to misleading 'Internal selection error'
When `total_output < min_input_amount` (e.g. caller asks to transfer 50_000 credits with min_input=100_000), the 1-input path computes `fee_target_min = max(min_input_amount, total_output) = 100_000 > total_output`, so `selected = {addr: 100_000}` and `input_sum = 100_000 ≠ total_output`. Phase 4's loop runs once with `remaining = total_output.saturating_sub(100_000) = 0`, then the flow trips the `debug_assert_eq!` at line 527 in debug builds and falls through to the line-538 'Internal selection error' branch in release.
The protocol disallows any transfer with `total_output < min_input_amount` (no input set can satisfy both `Σ inputs == total_output` and per-input `≥ min_input_amount`). This deserves an early, descriptive error like 'Transfer amount X below minimum Y' rather than the internal-error path that's documented as 'should never trip'. Add an early check at the top of `select_inputs` (or in the `transfer` entry-point on `outputs.values().sum()`).
Comment on lines
+285
to
+302
| /// Module-scope re-export of the per-input fee estimator so the | ||
| /// pure [`select_inputs`] helper can be unit-tested without an | ||
| /// instance of [`PlatformAddressWallet`]. | ||
| fn estimate_fee_for_inputs_pub( | ||
| input_count: usize, | ||
| output_count: usize, | ||
| fee_strategy: &[AddressFundsFeeStrategyStep], | ||
| outputs: &BTreeMap<PlatformAddress, Credits>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Credits { | ||
| PlatformAddressWallet::estimate_fee_for_inputs( | ||
| input_count, | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: estimate_fee_for_inputs_pub is a tautological re-export (carryover)
Carryover from prior review. estimate_fee_for_inputs doesn't take &self and isn't routed through a trait — the only reason it stays a static method on PlatformAddressWallet is so module-scope callers (select_inputs plus the new test sites at lines 652-653, 728-729, 780-781, 974-975) can invoke it via this wrapper. Move estimate_fee_for_inputs to a free function (or pub(super)) and delete the wrapper.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Deferring this cleanup — the wrapper does have a real reason today (select_inputs is a free function but estimate_fee_for_inputs is a static method on PlatformAddressWallet that's also called by other impl PlatformAddressWallet methods, so flipping it to pub(super) free fn is a slightly larger move than it looks). Logging this for a follow-up alongside any future selector refactor that already touches the surrounding scope; not in PR #3554's scope. Leaving open as a deliberate non-fix.
Comment on lines
+392
to
+423
| for (address, balance) in candidates { | ||
| prefix.push((address, balance)); | ||
| accumulated = accumulated.saturating_add(balance); | ||
|
|
||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| prefix.len(), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); | ||
|
|
||
| if accumulated >= required { | ||
| covered = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !covered { | ||
| let estimated_fee = estimate_fee_for_inputs_pub( | ||
| prefix.len().max(1), | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ); | ||
| let required = total_output.saturating_add(estimated_fee); | ||
| return Err(PlatformWalletError::AddressOperation(format!( | ||
| "Insufficient balance: available {} credits, required {} (outputs {} + estimated fee {})", | ||
| accumulated, required, total_output, estimated_fee | ||
| ))); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: accumulated.saturating_add masks overflow in the error path (carryover)
Carryover from prior review. accumulated = accumulated.saturating_add(balance) (line 394) quietly caps at u64::MAX. Once saturated, the loop trivially passes accumulated >= required and reports misleading values in the line 420-423 error message. Same applies to total_output.saturating_add(estimated_fee) (lines 403, 419) and total_output.saturating_sub(other_total) (line 484). Practically unreachable given total credit supply, but if total_output ever comes from less-trusted state these saturations should be hard errors.
source: ['claude']
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. Practically unreachable — Credits = u64 and the protocol's max_credits_supply is far below u64::MAX, so the saturating_add cap is unreachable for any legitimately-sourced balance / total_output. Switching to checked_add would be cleaner defensively but isn't a real-world bug. Logging for a future hardening pass alongside other arithmetic-safety review; not in PR #3554's scope. Leaving open as a deliberate non-fix.
…egy, retry on Phase 3 fail Addresses the second wave of review findings on PR #3554: 1. [BLOCKING] Phase 4 distribution no longer produces inputs below `min_input_amount`. `auto_select_inputs` now filters candidates with `balance < min_input_amount` upfront — they cannot legally appear in the inputs map. In Phase 4, when a non-fee-target tail entry would consume less than `min_input_amount`, the residue rolls back into the fee target's consumption (which has surplus headroom by construction). Returns a descriptive error if rollback would violate the fee-target headroom invariant. 2. [BLOCKING] `transfer()` rejects unsupported `fee_strategy` shapes for `InputSelection::Auto`. Auto-select currently only implements protocol-correct logic for `[DeductFromInput(0)]`; any other strategy returns `PlatformWalletError::AddressOperation` with a clear message redirecting callers to `InputSelection::Explicit`. Explicit paths still accept arbitrary strategies (caller's responsibility). 3. [BLOCKING] When Phase 3 (`fee_target_min > fee_target_max`) fails in `select_inputs`, the algorithm now extends the prefix with the next candidate and retries instead of erroring out. Larger prefixes may yield a different lex-smallest fee target with sufficient headroom. Errors out only when candidates are exhausted and no covering prefix is feasible. 4. [SUGGESTION] `select_inputs` returns an early descriptive error when `total_output < min_input_amount` — the protocol forbids this regardless of input shape, so an explicit error beats the internal "should never trip" branch that some callers were reaching. 5. [SUGGESTION] Existing selector tests now also build a minimal `AddressFundsTransferTransitionV0` and run `validate_structure`, asserting protocol-level validity in addition to the `Σ inputs == total_output` invariant. Catches future regressions without needing a live node. Coderabbit findings DUuz (#3554), DUu1 (#3554), E5L5 (#3554), thepastaclaw findings F9fo, GMHz, GMH5, GMH_, F9fv addressed. Outdated F9fk references the renamed test from before 9ea9e70. Nitpicks F9fz/GMID/F9f5/GMIH deferred (unreachable / low value). Verification: - cargo check --tests -p platform-wallet OK - cargo clippy --tests -p platform-wallet -- -D warnings OK - cargo fmt -p platform-wallet OK - cargo test -p platform-wallet --lib 121/121 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (1)
607-631: Phase 5 invariant checks are partially release-protected.The
debug_assertstatements at lines 611-624 verify critical protocol invariants:
Σ inputs == total_output— also checked in release at line 626 ✓- Fee target is BTreeMap index-0 (lex-smallest)
- Fee target retains ≥
estimated_feeremaining balance- Every input ≥
min_input_amountInvariants 2-4 are only asserted in debug builds. If any of these fail in release (due to a future regression in Phase 1-4 logic), the transition would be submitted and rejected by the protocol layer rather than caught here.
Given the algorithm's structure and test coverage, these should never trip. However, promoting invariant 3 (the fee headroom check) to a release-mode error would provide defense-in-depth for the exact bug this PR fixes.
♻️ Optional: Add release-mode check for fee headroom
if input_sum != total_output { return Err(PlatformWalletError::AddressOperation(format!( "Internal selection error: Σ inputs ({}) != total_output ({})", input_sum, total_output ))); } + + let fee_target_remaining = fee_target_balance.saturating_sub(fee_target_consumed); + if fee_target_remaining < estimated_fee { + return Err(PlatformWalletError::AddressOperation(format!( + "Internal selection error: fee target {} remaining {} < estimated fee {}", + format_address(&fee_target_addr), + fee_target_remaining, + estimated_fee, + ))); + } Ok(selected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs` around lines 607 - 631, The fee-headroom check currently uses debug_assert! on fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which is only active in debug builds; replace that debug-only assertion with a release-mode runtime check inside the same scope (where selected, fee_target_balance, fee_target_consumed and estimated_fee are available) so that if the condition fails you return Err(PlatformWalletError::AddressOperation(...)) with a clear message including the computed headroom and required estimated_fee; keep the other debug_asserts as-is and ensure the new check mirrors the existing error style used for input_sum != total_output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- Around line 607-631: The fee-headroom check currently uses debug_assert! on
fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, which
is only active in debug builds; replace that debug-only assertion with a
release-mode runtime check inside the same scope (where selected,
fee_target_balance, fee_target_consumed and estimated_fee are available) so that
if the condition fails you return
Err(PlatformWalletError::AddressOperation(...)) with a clear message including
the computed headroom and required estimated_fee; keep the other debug_asserts
as-is and ensure the new check mirrors the existing error style used for
input_sum != total_output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e663ab0e-42e2-48de-b207-cd8e4fa9740b
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs
…allet changes Adds `rs-platform-wallet` as a filter entry in `.github/package-filters/rs-packages-no-workflows.yml`. Without this, crate-only changes under `packages/rs-platform-wallet/` evaluate to `rs-packages = '[]'` and the `rs-workspace-tests` job in `.github/workflows/tests.yml` gates off — meaning the crate's unit tests never run in CI when only that crate is touched. This gap surfaced on PR #3554 itself: five commits, 121 unit tests, none of them executed by `Rust workspace tests` (all reported as SKIPPED). Local `cargo test -p platform-wallet --lib` was the only validation. Reviewers seeing "all green" could miss that the actual Rust validation was skipped. The filter entry mirrors the existing pattern: list the crate path and inherit the SDK alias (`*sdk`) so transitive SDK changes also trigger workspace tests for the wallet, matching how `wasm-sdk` and `rs-sdk-ffi` are wired. Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…allet-auto-select-inputs
…now run) The CI filter addition in 79c2b28 made `Rust workspace tests` run on `rs-platform-wallet` for the first time in a while, surfacing three pre-existing breaks that the silently-skipped pipeline had been accumulating: 1. `src/changeset/core_bridge.rs` (`build_core_changeset`) — `field_reassign_with_default` lint. `let mut cs = CoreChangeSet::default(); cs.new_utxos = ...; cs.spent_utxos = ...;` replaced with a struct literal carrying the derived values plus `..CoreChangeSet::default()` for forward-compat fields. 2. `src/wallet/apply.rs:316` — `let_unit_value` lint. `WalletInfoInterface::update_balance` returns `()`; the `let _ = ...` discards a unit value. Calling the method directly is the intended shape. 3. `tests/spv_sync.rs:74-78` — stale field access. The integration test still walked `core.chain.synced_height` even though `CoreChangeSet` was flattened (see existing rustdoc on `synced_height` direct field). Replaced with `core.synced_height` directly. None of these are bugs — clippy hardening and a stale test field that `cargo test --lib` never compiled. Verified: - `cargo clippy --workspace --tests -- -D warnings` clean - `cargo clippy -p platform-wallet --tests -- -D warnings` clean - `cargo test -p platform-wallet --lib` 121/121 Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
… 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
5 tasks
thepastaclaw
reviewed
May 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All prior blocking issues from earlier review rounds are resolved. Codex's new blocking claim about Phase 4's greedy redistribution rejecting feasible transfers does not reproduce: with the actual fee constants (input_cost=500k, output_cost=6M, min_input=100k), prefix=1 already terminates as feasible in their concrete example (B_a=7.62M, T=330k → fee_target_max=1.12M ≥ fee_target_min=330k). I could not construct any scenario reachable from auto_select_inputs (which filters <min_input and sorts balance-desc) where Phase 4 fold-fail triggers, because Phase 1⇔Phase 3 at prefix=1 forces the loop to terminate before fold-fail conditions can arise. Remaining issues are a stale public docstring, a test that doesn't actually exercise its documented path, and carryover nitpicks.
Reviewed commit: 3c4f919
🟡 3 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: `InputSelection::Auto` docstring no longer matches the implementation's selection policy
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
The doc says Auto consumes addresses "from lowest derivation index to highest". This PR replaced that policy: auto_select_inputs now sorts candidates by balance descending (transfer.rs:233-247) to mirror the dash-evo-tool allocator. InputSelection is part of this crate's public API surface, so the stale contract is misleading for downstream callers — it changes externally visible behavior (which addresses are spent first, how many inputs get consumed, the privacy/fragmentation profile). Update the doc to describe balance-descending consumption with the min_input_amount filter.
💡 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
/// — they cannot legally appear as inputs and would be rejected
/// by `validate_structure`.
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 implementation's selection policy
The doc says `Auto` consumes addresses "from lowest derivation index to highest". This PR replaced that policy: `auto_select_inputs` now sorts candidates by balance descending (transfer.rs:233-247) to mirror the dash-evo-tool allocator. `InputSelection` is part of this crate's public API surface, so the stale contract is misleading for downstream callers — it changes externally visible behavior (which addresses are spent first, how many inputs get consumed, the privacy/fragmentation profile). Update the doc to describe balance-descending consumption with the `min_input_amount` filter.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 1176-1221: `non_fee_target_below_min_input_redistributes` never reaches the redistribute path it documents
The test claims to exercise the in-helper redistribution path (sub-minimum tail consumption folded back into the fee target). With current fee constants (`input_cost=500_000`, `output_cost=6_000_000`), `estimate_min_fee(1,1)=6_500_000` and `estimate_min_fee(2,1)=7_000_000`. Tracing:
- prefix=[addr_x]: accumulated=1_000_000, required=950_000+6_500_000=7_450_000 → Phase 1 fails, continue.
- prefix=[addr_x, addr_y]: accumulated=1_030_000, required=950_000+7_000_000=7_950_000 → Phase 1 fails, continue.
- Loop exhausted, returns `Err("Insufficient balance...")`.
The `Err(PlatformWalletError::AddressOperation(_)) => {}` arm silently accepts this, and the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test today only verifies "doesn't panic with an unrelated error variant" — it does not pin redistribution behavior.
To actually reach lines 569-577 + 579-603, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below `min_input_amount`. A construction that genuinely reaches the path: two ≥ min_input candidates where adding a second entry produces `tentative < min_input` because `remaining = T - fee_target_min` is small. Either rebuild this test around such numbers, or split it: one test that genuinely reaches the fold-back, and a separate one that pins the "sub-min input never silently shipped" guarantee.
- [SUGGESTION] lines 233-247: All-candidates-below-min_input case surfaces as generic "Insufficient balance"
`auto_select_inputs` filters candidates with `balance < min_input_amount` upstream (lines 237-245). If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, `candidates` ends up empty and `select_inputs` returns `Insufficient balance: available 0 credits, required ...`. That message is misleading: the wallet does hold credits — they're just stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop for this real failure mode. Detect and report it before delegating to `select_inputs`.
Comment on lines
+1176
to
+1221
| #[test] | ||
| fn non_fee_target_below_min_input_redistributes() { | ||
| let addr_x = p2pkh(0x01); // lex-smallest → fee target | ||
| let addr_y = p2pkh(0x02); | ||
| let target = p2pkh(0x99); | ||
| let pv = LATEST_PLATFORM_VERSION; | ||
| let min_input = pv.dpp.state_transitions.address_funds.min_input_amount; | ||
|
|
||
| // GMHz numbers, scaled so total_output is comfortably above | ||
| // min_output_amount (500_000) — the protocol's per-output | ||
| // minimum is checked by validate_structure separately and is | ||
| // unrelated to the input-side redistribution we're exercising. | ||
| let total_output = 950_000u64; | ||
| let addr_x_balance = 1_000_000u64; // covers total_output + fee on its own | ||
| let addr_y_balance = 30_000u64; // below min_input_amount | ||
| let outputs = outputs_for(target, total_output); | ||
| let candidates = vec![(addr_x, addr_x_balance), (addr_y, addr_y_balance)]; | ||
| let fee_strategy = vec![AddressFundsFeeStrategyStep::DeductFromInput(0)]; | ||
|
|
||
| let result = select_inputs(candidates, &outputs, total_output, &fee_strategy, pv); | ||
|
|
||
| match result { | ||
| Ok(selected) => { | ||
| // Every selected input must satisfy the per-input minimum. | ||
| for (addr, amount) in selected.iter() { | ||
| assert!( | ||
| *amount >= min_input, | ||
| "input {} consumes {} which is below min_input_amount {}", | ||
| format_address(addr), | ||
| amount, | ||
| min_input, | ||
| ); | ||
| } | ||
| let input_sum: Credits = selected.values().sum(); | ||
| assert_eq!(input_sum, total_output); | ||
| assert_selection_validates(&selected, &outputs, fee_strategy, pv); | ||
| } | ||
| Err(PlatformWalletError::AddressOperation(_)) => { | ||
| // Acceptable: the helper opted to error out rather | ||
| // than redistribute. Either outcome is valid; the | ||
| // failure mode we're guarding against is a silent | ||
| // sub-minimum input. | ||
| } | ||
| Err(other) => panic!("unexpected error variant: {other:?}"), | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: non_fee_target_below_min_input_redistributes never reaches the redistribute path it documents
The test claims to exercise the in-helper redistribution path (sub-minimum tail consumption folded back into the fee target). With current fee constants (input_cost=500_000, output_cost=6_000_000), estimate_min_fee(1,1)=6_500_000 and estimate_min_fee(2,1)=7_000_000. Tracing:
- prefix=[addr_x]: accumulated=1_000_000, required=950_000+6_500_000=7_450_000 → Phase 1 fails, continue.
- prefix=[addr_x, addr_y]: accumulated=1_030_000, required=950_000+7_000_000=7_950_000 → Phase 1 fails, continue.
- Loop exhausted, returns
Err("Insufficient balance...").
The Err(PlatformWalletError::AddressOperation(_)) => {} arm silently accepts this, and the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test today only verifies "doesn't panic with an unrelated error variant" — it does not pin redistribution behavior.
To actually reach lines 569-577 + 579-603, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below min_input_amount. A construction that genuinely reaches the path: two ≥ min_input candidates where adding a second entry produces tentative < min_input because remaining = T - fee_target_min is small. Either rebuild this test around such numbers, or split it: one test that genuinely reaches the fold-back, and a separate one that pins the "sub-min input never silently shipped" guarantee.
source: ['claude-rust-quality']
🤖 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 1176-1221: `non_fee_target_below_min_input_redistributes` never reaches the redistribute path it documents
The test claims to exercise the in-helper redistribution path (sub-minimum tail consumption folded back into the fee target). With current fee constants (`input_cost=500_000`, `output_cost=6_000_000`), `estimate_min_fee(1,1)=6_500_000` and `estimate_min_fee(2,1)=7_000_000`. Tracing:
- prefix=[addr_x]: accumulated=1_000_000, required=950_000+6_500_000=7_450_000 → Phase 1 fails, continue.
- prefix=[addr_x, addr_y]: accumulated=1_030_000, required=950_000+7_000_000=7_950_000 → Phase 1 fails, continue.
- Loop exhausted, returns `Err("Insufficient balance...")`.
The `Err(PlatformWalletError::AddressOperation(_)) => {}` arm silently accepts this, and the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test today only verifies "doesn't panic with an unrelated error variant" — it does not pin redistribution behavior.
To actually reach lines 569-577 + 579-603, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below `min_input_amount`. A construction that genuinely reaches the path: two ≥ min_input candidates where adding a second entry produces `tentative < min_input` because `remaining = T - fee_target_min` is small. Either rebuild this test around such numbers, or split it: one test that genuinely reaches the fold-back, and a separate one that pins the "sub-min input never silently shipped" guarantee.
Comment on lines
+233
to
+247
| let mut candidates: Vec<(PlatformAddress, Credits)> = account | ||
| .addresses | ||
| .addresses | ||
| .values() | ||
| .filter_map(|addr_info| { | ||
| let p2pkh = PlatformP2PKHAddress::from_address(&addr_info.address).ok()?; | ||
| let balance = account.address_credit_balance(&p2pkh); | ||
| if balance == 0 { | ||
| continue; | ||
| if balance < min_input_amount { | ||
| None | ||
| } else { | ||
| Some((PlatformAddress::P2pkh(p2pkh.to_bytes()), balance)) | ||
| } | ||
| }) | ||
| .collect(); | ||
| candidates.sort_by(|a, b| b.1.cmp(&a.1)); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: All-candidates-below-min_input case surfaces as generic "Insufficient balance"
auto_select_inputs filters candidates with balance < min_input_amount upstream (lines 237-245). If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, candidates ends up empty and select_inputs returns Insufficient balance: available 0 credits, required .... That message is misleading: the wallet does hold credits — they're just stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop for this real failure mode. Detect and report it before delegating to select_inputs.
source: ['claude-general']
🤖 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 233-247: All-candidates-below-min_input case surfaces as generic "Insufficient balance"
`auto_select_inputs` filters candidates with `balance < min_input_amount` upstream (lines 237-245). If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, `candidates` ends up empty and `select_inputs` returns `Insufficient balance: available 0 credits, required ...`. That message is misleading: the wallet does hold credits — they're just stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop for this real failure mode. Detect and report it before delegating to `select_inputs`.
Comment on lines
192
to
+610
| @@ -171,55 +209,59 @@ impl PlatformAddressWallet { | |||
| )) | |||
| })?; | |||
|
|
|||
| // BTreeMap<u32, _> iteration is already in ascending index order. | |||
| let mut selected = BTreeMap::new(); | |||
| let mut accumulated: Credits = 0; | |||
| let min_input_amount = platform_version | |||
| .dpp | |||
| .state_transitions | |||
| .address_funds | |||
| .min_input_amount; | |||
|
|
|||
| for addr_info in account.addresses.addresses.values() { | |||
| if let Ok(p2pkh) = PlatformP2PKHAddress::from_address(&addr_info.address) { | |||
| // Snapshot addresses with balance ≥ `min_input_amount`, then sort | |||
| // them by balance descending so [`select_inputs`] sees the | |||
| // largest candidates first. Mirrors the dash-evo-tool allocator | |||
| // (`src/ui/wallets/send_screen.rs:155-157`) and means the | |||
| // common case — one address holds enough to cover | |||
| // `total_output + estimated_fee` — bypasses the multi-input | |||
| // fee-headroom branch entirely. Addresses with balance below | |||
| // `min_input_amount` are filtered out: the protocol's | |||
| // structural validator (`AddressFundsTransferTransitionV0:: | |||
| // validate_structure`, see `state_transition_validation.rs:146`) | |||
| // rejects any input with `amount < min_input_amount`, so such | |||
| // an address cannot legally appear in the inputs map and is | |||
| // useless as a standalone candidate. Materialising a `Vec` | |||
| // here also lets the selection loop run as a pure helper that's | |||
| // amenable to direct unit testing. | |||
| let mut candidates: Vec<(PlatformAddress, Credits)> = account | |||
| .addresses | |||
| .addresses | |||
| .values() | |||
| .filter_map(|addr_info| { | |||
| let p2pkh = PlatformP2PKHAddress::from_address(&addr_info.address).ok()?; | |||
| let balance = account.address_credit_balance(&p2pkh); | |||
| if balance == 0 { | |||
| continue; | |||
| if balance < min_input_amount { | |||
| None | |||
| } else { | |||
| Some((PlatformAddress::P2pkh(p2pkh.to_bytes()), balance)) | |||
| } | |||
| }) | |||
| .collect(); | |||
| candidates.sort_by(|a, b| b.1.cmp(&a.1)); | |||
|
|
|||
| let address = PlatformAddress::P2pkh(p2pkh.to_bytes()); | |||
| selected.insert(address, balance); | |||
| accumulated = accumulated.saturating_add(balance); | |||
|
|
|||
| // Re-estimate fee with the current input count. | |||
| let estimated_fee = Self::estimate_fee_for_inputs( | |||
| selected.len(), | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ); | |||
| let required = total_output.saturating_add(estimated_fee); | |||
|
|
|||
| if accumulated >= required { | |||
| return Ok(selected); | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // Not enough funds. | |||
| let estimated_fee = Self::estimate_fee_for_inputs( | |||
| selected.len().max(1), | |||
| output_count, | |||
| fee_strategy, | |||
| select_inputs( | |||
| candidates, | |||
| outputs, | |||
| total_output, | |||
| fee_strategy, | |||
| platform_version, | |||
| ); | |||
| let required = total_output.saturating_add(estimated_fee); | |||
| Err(PlatformWalletError::AddressOperation(format!( | |||
| "Insufficient balance: available {} credits, required {} (outputs {} + estimated fee {})", | |||
| accumulated, required, total_output, estimated_fee | |||
| ))) | |||
| ) | |||
| } | |||
|
|
|||
| /// Simulate the fee strategy to determine how much additional balance | |||
| /// the inputs need beyond the output amounts. | |||
| /// | |||
| /// Re-exposed at module scope via [`estimate_fee_for_inputs_pub`] | |||
| /// so [`select_inputs`] (the pure helper) can drive the same | |||
| /// estimator without going through `Self`. | |||
| /// | |||
| /// Walks through the fee strategy steps in order, deducting from the | |||
| /// available sources (outputs or inputs) until the fee is covered. | |||
| /// Returns the portion of the fee that must come from inputs. | |||
| @@ -266,3 +308,915 @@ impl PlatformAddressWallet { | |||
| remaining_fee | |||
| } | |||
| } | |||
|
|
|||
| /// Module-scope re-export of the per-input fee estimator so the | |||
| /// pure [`select_inputs`] helper can be unit-tested without an | |||
| /// instance of [`PlatformAddressWallet`]. | |||
| fn estimate_fee_for_inputs_pub( | |||
| input_count: usize, | |||
| output_count: usize, | |||
| fee_strategy: &[AddressFundsFeeStrategyStep], | |||
| outputs: &BTreeMap<PlatformAddress, Credits>, | |||
| platform_version: &PlatformVersion, | |||
| ) -> Credits { | |||
| PlatformAddressWallet::estimate_fee_for_inputs( | |||
| input_count, | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ) | |||
| } | |||
|
|
|||
| /// Pure input-selection helper. | |||
| /// | |||
| /// Given a `candidates` list of `(address, balance)` pairs in the | |||
| /// caller's preferred selection order (balance-descending in | |||
| /// practice — see [`PlatformAddressWallet::auto_select_inputs`] — | |||
| /// but `select_inputs` itself is order-agnostic: it walks | |||
| /// `candidates` as-is and picks the smallest covering prefix), | |||
| /// produce an inputs map satisfying TWO invariants demanded by the | |||
| /// validator: | |||
| /// | |||
| /// 1. `Σ selected.values() == total_output` — the protocol's | |||
| /// structural balance invariant for transfers. | |||
| /// 2. The address selected for fee deduction (currently the | |||
| /// lex-smallest address in `selected`, which is the | |||
| /// `BTreeMap` index-0 entry that | |||
| /// [`AddressFundsFeeStrategyStep::DeductFromInput(0)`] targets) | |||
| /// must have **post-consumption remaining balance ≥ estimated | |||
| /// fee**. Otherwise drive's | |||
| /// `deduct_fee_from_outputs_or_remaining_balance_of_inputs` | |||
| /// cannot fully cover the fee, the transition fails with | |||
| /// `fee_fully_covered = false`, and validation rejects the | |||
| /// state transition (see | |||
| /// `rs-drive-abci/.../validate_fees_of_event/v0/mod.rs:209-224`). | |||
| /// | |||
| /// CodeRabbit caught the bug where the previous implementation | |||
| /// satisfied invariant (1) but not (2): if candidates were | |||
| /// `[(addr_a, 20M), (addr_b, 50M)]`, `total_output` was 30M, and the | |||
| /// strategy was `[DeductFromInput(0)]`, the previous build returned | |||
| /// `{addr_a: 20M, addr_b: 10M}`. `addr_a` was fully drained, so its | |||
| /// post-consumption remaining was 0 — the fee couldn't be deducted, | |||
| /// and the transition was rejected. This rewrite ensures the fee | |||
| /// target keeps enough headroom by consuming the **minimum | |||
| /// allowable** amount (`min_input_amount` from the platform version) | |||
| /// from it, and shifting the rest of the consumption onto the other | |||
| /// selected inputs. | |||
| /// | |||
| /// # Algorithm (single `DeductFromInput(0)` strategy — the only supported case) | |||
| /// | |||
| /// 1. Pick the smallest prefix of `candidates` (in the order the | |||
| /// caller supplied — balance-descending in practice) such that | |||
| /// `Σ balances ≥ total_output + estimated_fee_for(prefix.len())`. | |||
| /// Error out if no prefix covers it. | |||
| /// 2. Identify the prospective fee target = lex-smallest address in | |||
| /// that prefix (this is the address at `BTreeMap` index 0 of the | |||
| /// eventual selected map, which is what `DeductFromInput(0)` | |||
| /// targets). | |||
| /// 3. Pick the consumption distribution: | |||
| /// - `fee_target_max = max(0, fee_target_balance − estimated_fee)` | |||
| /// — the largest amount we can consume from the fee target | |||
| /// while still leaving ≥ `estimated_fee` of remaining balance. | |||
| /// - `other_total = Σ balances of non-fee-target prefix entries` | |||
| /// - `fee_target_min = max(min_input_amount, total_output − other_total)` | |||
| /// — the smallest amount we can consume from the fee target | |||
| /// while still keeping it in the inputs map (`min_input_amount`, | |||
| /// so the protocol's per-input minimum is respected) AND | |||
| /// reaching the `Σ inputs == total_output` invariant. | |||
| /// - If `fee_target_min > fee_target_max`, **extend the prefix | |||
| /// with the next candidate and retry steps 1-3**. A larger | |||
| /// prefix can lower `fee_target_min` (more `other_total` to | |||
| /// absorb consumption) and may also pull in a smaller | |||
| /// lex-key candidate that becomes the new fee target. Only | |||
| /// after candidates are exhausted do we error out. | |||
| /// 4. Build the result: | |||
| /// - Insert `(fee_target_addr, fee_target_min)` first | |||
| /// (always ≥ `min_input_amount`, so always present in the map | |||
| /// and lex-smallest of the result). | |||
| /// - Distribute `total_output − fee_target_min` across the other | |||
| /// prefix entries in caller-supplied order | |||
| /// (`min(balance, remaining)`). If a tail entry's tentative | |||
| /// consumption falls below `min_input_amount` (the protocol's | |||
| /// per-input minimum), the residue is rolled back into the | |||
| /// fee target's consumption rather than inserted as a | |||
| /// sub-minimum input. After roll-back the fee target's | |||
| /// consumption must still be ≤ `fee_target_max`; otherwise | |||
| /// we error out (this should not happen given that Phase 3 | |||
| /// already proved the prefix has slack, but the check is | |||
| /// kept as a defensive guard). | |||
| /// 5. Final defensive invariant check. | |||
| /// | |||
| /// `select_inputs` only supports `fee_strategy == [DeductFromInput(0)]`. | |||
| /// The public `transfer()` rejects other shapes for the | |||
| /// `InputSelection::Auto` path before they reach this helper. | |||
| fn select_inputs( | |||
| 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::DeductFromInput(0)] | |||
| ), | |||
| "select_inputs only supports [DeductFromInput(0)]; \ | |||
| the public `transfer()` should have validated this already" | |||
| ); | |||
| if !matches!( | |||
| fee_strategy, | |||
| [AddressFundsFeeStrategyStep::DeductFromInput(0)] | |||
| ) { | |||
| return Err(PlatformWalletError::AddressOperation( | |||
| "select_inputs only supports fee_strategy = [DeductFromInput(0)]; \ | |||
| other shapes must use InputSelection::Explicit" | |||
| .to_string(), | |||
| )); | |||
| } | |||
|
|
|||
| let output_count = outputs.len(); | |||
| let min_input_amount = platform_version | |||
| .dpp | |||
| .state_transitions | |||
| .address_funds | |||
| .min_input_amount; | |||
|
|
|||
| // Finding #4: the protocol rejects any input below `min_input_amount`, | |||
| // and an input always covers (a portion of) `total_output`. So if | |||
| // `total_output < min_input_amount`, no input can be sized within | |||
| // both bounds simultaneously — error out cleanly here 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+2+3: walk candidates in caller-supplied order, growing | |||
| // the prefix one candidate at a time. After each push, re-run | |||
| // Phase 1 (does the prefix cover `total_output + estimated_fee`?) | |||
| // and, if so, Phase 2/3 (does the lex-smallest prefix entry have | |||
| // enough headroom to absorb the fee?). Either accept the prefix | |||
| // or extend further. Errors out only when candidates are | |||
| // exhausted with no feasible prefix. | |||
| let mut prefix: Vec<(PlatformAddress, Credits)> = Vec::new(); | |||
| let mut accumulated: Credits = 0; | |||
| let mut last_estimated_fee: Credits = 0; | |||
| let mut feasible: Option<(PlatformAddress, Credits, Credits, Credits)> = None; | |||
|
|
|||
| for (address, balance) in candidates { | |||
| prefix.push((address, balance)); | |||
| accumulated = accumulated.saturating_add(balance); | |||
|
|
|||
| let estimated_fee = estimate_fee_for_inputs_pub( | |||
| prefix.len(), | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ); | |||
| last_estimated_fee = estimated_fee; | |||
| let required = total_output.saturating_add(estimated_fee); | |||
|
|
|||
| if accumulated < required { | |||
| continue; | |||
| } | |||
|
|
|||
| // Phase 2: lex-smallest of the current prefix is the fee target. | |||
| let (fee_target_addr, fee_target_balance) = prefix | |||
| .iter() | |||
| .min_by_key(|(addr, _)| *addr) | |||
| .copied() | |||
| .expect("prefix is non-empty: we just pushed"); | |||
|
|
|||
| let fee_target_max = fee_target_balance.saturating_sub(estimated_fee); | |||
| let other_total: Credits = prefix | |||
| .iter() | |||
| .filter(|(addr, _)| addr != &fee_target_addr) | |||
| .map(|(_, bal)| *bal) | |||
| .sum(); | |||
| let fee_target_min = | |||
| std::cmp::max(min_input_amount, total_output.saturating_sub(other_total)); | |||
|
|
|||
| if fee_target_min <= fee_target_max { | |||
| feasible = Some(( | |||
| fee_target_addr, | |||
| fee_target_balance, | |||
| fee_target_min, | |||
| estimated_fee, | |||
| )); | |||
| break; | |||
| } | |||
| // Phase 3 failed for this prefix size: keep growing. | |||
| } | |||
|
|
|||
| let Some((fee_target_addr, fee_target_balance, fee_target_min, estimated_fee)) = feasible | |||
| else { | |||
| // Distinguish "couldn't cover total_output + fee" from | |||
| // "covered but no headroom-feasible fee target". | |||
| if accumulated < total_output.saturating_add(last_estimated_fee) { | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Insufficient balance: available {} credits, required {} \ | |||
| (outputs {} + estimated fee {})", | |||
| accumulated, | |||
| total_output.saturating_add(last_estimated_fee), | |||
| total_output, | |||
| last_estimated_fee, | |||
| ))); | |||
| } | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Cannot satisfy fee headroom: no covering prefix of the available inputs \ | |||
| leaves the lex-smallest entry with ≥ estimated fee {} of remaining balance \ | |||
| after consumption. Consider providing more inputs or using a different \ | |||
| fee strategy.", | |||
| last_estimated_fee, | |||
| ))); | |||
| }; | |||
|
|
|||
| // Phase 4: build the result map. | |||
| // | |||
| // Start by consuming the minimum from the fee target so it | |||
| // retains maximum remaining balance for the on-chain fee | |||
| // deduction. Then walk the remaining prefix entries (in | |||
| // caller-supplied order) and distribute what's left of | |||
| // `total_output`. If a tail entry's tentative consumption is | |||
| // below `min_input_amount`, roll the residue back onto the | |||
| // fee target instead of producing a sub-minimum input — | |||
| // the protocol's `validate_structure` would reject the | |||
| // transition otherwise (`InputBelowMinimumError`). | |||
| let mut fee_target_consumed = fee_target_min; | |||
| let fee_target_max = fee_target_balance.saturating_sub(estimated_fee); | |||
| let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); | |||
|
|
|||
| let mut remaining = total_output.saturating_sub(fee_target_consumed); | |||
| let mut residue_to_fee_target: Credits = 0; | |||
| for (addr, bal) in prefix.iter() { | |||
| if *addr == fee_target_addr { | |||
| continue; | |||
| } | |||
| if remaining == 0 { | |||
| break; | |||
| } | |||
| let tentative = (*bal).min(remaining); | |||
| if tentative == 0 { | |||
| continue; | |||
| } | |||
| if tentative < min_input_amount { | |||
| // Sub-minimum input — fold into the fee target. | |||
| residue_to_fee_target = residue_to_fee_target.saturating_add(tentative); | |||
| remaining = remaining.saturating_sub(tentative); | |||
| continue; | |||
| } | |||
| selected.insert(*addr, tentative); | |||
| remaining = remaining.saturating_sub(tentative); | |||
| } | |||
|
|
|||
| if residue_to_fee_target > 0 { | |||
| let new_consumed = fee_target_consumed.saturating_add(residue_to_fee_target); | |||
| if new_consumed > fee_target_max { | |||
| // Should be unreachable: Phase 3 only accepts a prefix | |||
| // when fee_target_min ≤ fee_target_max, and the residue | |||
| // we're folding here represents amounts that *would* | |||
| // have been consumed by other entries — the prefix | |||
| // covers `total_output + estimated_fee`, so the fee | |||
| // target's headroom up to `fee_target_max` should | |||
| // accommodate any residue from the tail. We still | |||
| // guard against it because silently producing an | |||
| // invalid transition is worse than a loud error. | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Cannot satisfy fee headroom after redistributing sub-minimum tail \ | |||
| inputs: fee-target {} would consume {} (balance {}, max {}), leaving \ | |||
| less than estimated fee {} of remaining balance", | |||
| format_address(&fee_target_addr), | |||
| new_consumed, | |||
| fee_target_balance, | |||
| fee_target_max, | |||
| estimated_fee, | |||
| ))); | |||
| } | |||
| fee_target_consumed = new_consumed; | |||
| } | |||
|
|
|||
| selected.insert(fee_target_addr, fee_target_consumed); | |||
|
|
|||
| // Phase 5: defensive invariant checks. These should never trip | |||
| // if Phase 1+3+4 are correct, but we'd much rather fail loudly | |||
| // here than ship a transition the validator silently rejects. | |||
| let input_sum: Credits = selected.values().sum(); | |||
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Unchecked/saturating arithmetic on credit sums could mask overflow at the public API boundary
total_output: Credits = outputs.values().sum() (line 192) and let input_sum: Credits = selected.values().sum() (line 610) panic in debug builds on overflow; accumulated.saturating_add(balance) (line 474), total_output.saturating_add(estimated_fee) (lines 484, 522, 527), and total_output.saturating_sub(fee_target_consumed) (line 556) silently cap. The protocol validator (AddressFundsTransferTransitionV0::validate_structure) uses checked_add and returns OverflowError deterministically. Practically unreachable given total Dash credit supply (~1.8e10 Dash for u64::MAX), but transfer() is a public library API accepting raw u64 from callers; saturated values produce misleading error-message numbers and a saturated accumulated could trivially pass accumulated >= required. Use checked_add / checked_sum and surface a typed PlatformWalletError::Overflow variant at the boundary. Carryover from prior reviews.
source: ['claude-general', 'claude-rust-quality', 'codex-rust-quality']
Comment on lines
+312
to
+329
| /// Module-scope re-export of the per-input fee estimator so the | ||
| /// pure [`select_inputs`] helper can be unit-tested without an | ||
| /// instance of [`PlatformAddressWallet`]. | ||
| fn estimate_fee_for_inputs_pub( | ||
| input_count: usize, | ||
| output_count: usize, | ||
| fee_strategy: &[AddressFundsFeeStrategyStep], | ||
| outputs: &BTreeMap<PlatformAddress, Credits>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Credits { | ||
| PlatformAddressWallet::estimate_fee_for_inputs( | ||
| input_count, | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: estimate_fee_for_inputs_pub is a tautological wrapper around a static method (carryover)
PlatformAddressWallet::estimate_fee_for_inputs doesn't take &self, isn't dispatched through a trait, and reads no per-instance state. The only reason estimate_fee_for_inputs_pub exists is to forward calls from module scope (select_inputs at line 476 and four test sites) to the static method. The wrapper adds an indirection layer with no isolation benefit. Move estimate_fee_for_inputs out of the impl as a free pub(super) fn (or keep it inside the impl and make select_inputs an associated fn), then delete the wrapper. Carryover from prior reviews.
source: ['claude-general', 'claude-rust-quality']
thepastaclaw
reviewed
May 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review of latest push (3c4f919 → 545fa99). All four prior blocking issues remain resolved. The new push adds a [ReduceOutput(0)] selector path and admits it into InputSelection::Auto alongside [DeductFromInput(0)]. Codex flagged the new admission as blocking; I downgrade it to a suggestion — the author has explicitly documented the platform #3040 fee-underestimation trade-off in-line, the same underestimator affects DeductFromInput, and the failure mode is chain-time rejection with no fund loss. All five prior carryover suggestions/nitpicks remain unaddressed and are still valid. Two new doc/test-coverage suggestions on the new helper.
Reviewed commit: 545fa99
🟡 6 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: `InputSelection::Auto` docstring no longer matches the implementation
packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs (lines 27-30)
Carryover from the prior round, still unaddressed. The doc says Auto consumes addresses "from lowest derivation index to highest". Both clauses are wrong: (1) auto_select_inputs (transfer.rs:166-247) sorts candidates by balance descending and skips balances below min_input_amount; (2) the supported fee-strategy set is now [DeductFromInput(0)] or [ReduceOutput(0)] (transfer.rs:75-88), with very different selector semantics. InputSelection is part of this crate's public API surface, so the stale contract misleads downstream callers about which addresses get spent first, the fragmentation profile, and which strategies the Auto path will accept.
💡 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.
/// 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/transfer.rs`:
- [SUGGESTION] lines 75-90: `InputSelection::Auto` admits `[ReduceOutput(0)]` despite the documented chain-time fee shortfall (#3040)
This push widens `Auto` to accept `[ReduceOutput(0)]`. The new helper's own Phase 4 comment (transfer.rs:656-672) acknowledges that `estimate_fee_for_inputs_pub` returns only the static `state_transition_min_fees` floor (~6.5M for 1-in/1-out), while the 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`. There is no fund loss or consensus risk (the protocol validator catches it), but the auto-selector now advertises a path it knows it cannot validate.
This is a judgment call rather than a clear regression: (1) the same underestimator weakness affects `[DeductFromInput(0)]` — the headroom reserved on input 0 is only as large as the static estimate, so a real-fee overshoot still leaves the input short; the difference is that DeductFromInput's failure surfaces from the input residue rather than the absorbing output. (2) The author signaled intent with the in-line comment + the parallel `docs(rs-platform-wallet): note platform #3040 fee-estimation bug` commit. (3) Restricting `Auto` would remove a useful selector for callers who knowingly accept the trade-off. Options, ordered by reversibility cost: (a) keep current behaviour but mention #3040 in the user-facing error message when output_0 ≥ static estimate but the helper succeeds (i.e. log a warn-level diagnostic); (b) gate `[ReduceOutput(0)]` behind a `output_0 ≥ K * estimated_fee` safety margin until #3040 lands; (c) accept Codex's prescription and revert `Auto` to `[DeductFromInput(0)]`-only until the estimator is fixed.
- [SUGGESTION] lines 555-654: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
Both the algorithm summary at lines 555-558 and the inline comment at lines 630-632 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 637-641 instead iterates `prefix` (the caller-supplied order, which is 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. Functionally either ordering is valid (any peer with sufficient headroom satisfies the invariants), but the prior `select_inputs_deduct_from_input` helper takes care 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` (which IS lex-ordered) so the documented invariant is enforced.
- [SUGGESTION] lines 540-706: `select_inputs_reduce_output` lacks tests for Phase 3 redistribution and insufficient-balance branches
The four new ReduceOutput tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, and the Phase 4 fee-cannot-be-absorbed error. 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 643-649 are dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 608-614). The Phase 3 redistribution branch is the most non-trivial new logic and is symmetric to the input-side helper's `non_fee_target_below_min_input_redistributes` — pin it directly. 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 1176-1221: `non_fee_target_below_min_input_redistributes` still never reaches the redistribute path it documents
Carryover from the prior round, still unaddressed. With current fee constants (`input_cost=500_000`, `output_cost=6_000_000`), `estimate_min_fee(1,1)=6_500_000` and `estimate_min_fee(2,1)=7_000_000`. Tracing the test's inputs `[(addr_x, 1_000_000), (addr_y, 30_000)]` with `total_output=950_000`: prefix=[addr_x] yields accumulated=1_000_000 vs required=7_450_000 (Phase 1 fails, continue); prefix=[addr_x, addr_y] yields accumulated=1_030_000 vs required=7_950_000 (Phase 1 fails, continue); loop exhausts and returns `Err("Insufficient balance...")`. The `Err(PlatformWalletError::AddressOperation(_)) => {}` arm silently accepts this, so the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test currently only proves "didn't panic with an unrelated error variant". To reach the documented branch, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below `min_input_amount` — construct two ≥ min_input candidates where adding a second entry produces `tentative < min_input` because `remaining = T - fee_target_min` is small.
- [SUGGESTION] lines 199-217: All-candidates-below-min_input still surfaces as generic "Insufficient balance"
Carryover, still unaddressed and now applies to both selector helpers. `auto_select_inputs` filters out every address with `balance < min_input_amount` upstream. If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, `candidates` is empty and the dispatched helper returns `Insufficient balance: available 0 credits, required ...`. That message is misleading: the wallet does hold credits — they're stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop, especially now that the new `[ReduceOutput(0)]` path produces the same misleading message.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs`:
- [SUGGESTION] lines 27-30: `InputSelection::Auto` docstring no longer matches the implementation
Carryover from the prior round, still unaddressed. The doc says `Auto` consumes addresses "from lowest derivation index to highest". Both clauses are wrong: (1) `auto_select_inputs` (transfer.rs:166-247) sorts candidates by balance descending and skips balances below `min_input_amount`; (2) the supported fee-strategy set is now `[DeductFromInput(0)]` or `[ReduceOutput(0)]` (transfer.rs:75-88), with very different selector semantics. `InputSelection` is part of this crate's public API surface, so the stale contract misleads downstream callers about which addresses get spent first, the fragmentation profile, and which strategies the `Auto` path will accept.
Comment on lines
75
to
90
| InputSelection::Auto => { | ||
| // Auto-select supports `[DeductFromInput(0)]` and | ||
| // `[ReduceOutput(0)]`; any other shape must use `Explicit`. | ||
| if !matches!( | ||
| fee_strategy.as_slice(), | ||
| [AddressFundsFeeStrategyStep::DeductFromInput(0)] | ||
| | [AddressFundsFeeStrategyStep::ReduceOutput(0)] | ||
| ) { | ||
| return Err(PlatformWalletError::AddressOperation( | ||
| "InputSelection::Auto supports fee_strategy = [DeductFromInput(0)] \ | ||
| or [ReduceOutput(0)]; for other strategies use InputSelection::Explicit" | ||
| .to_string(), | ||
| )); | ||
| } | ||
| let inputs = self | ||
| .auto_select_inputs(account_index, &outputs, &fee_strategy, version) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: InputSelection::Auto admits [ReduceOutput(0)] despite the documented chain-time fee shortfall (#3040)
This push widens Auto to accept [ReduceOutput(0)]. The new helper's own Phase 4 comment (transfer.rs:656-672) acknowledges that estimate_fee_for_inputs_pub returns only the static state_transition_min_fees floor (~6.5M for 1-in/1-out), while the 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. There is no fund loss or consensus risk (the protocol validator catches it), but the auto-selector now advertises a path it knows it cannot validate.
This is a judgment call rather than a clear regression: (1) the same underestimator weakness affects [DeductFromInput(0)] — the headroom reserved on input 0 is only as large as the static estimate, so a real-fee overshoot still leaves the input short; the difference is that DeductFromInput's failure surfaces from the input residue rather than the absorbing output. (2) The author signaled intent with the in-line comment + the parallel docs(rs-platform-wallet): note platform #3040 fee-estimation bug commit. (3) Restricting Auto would remove a useful selector for callers who knowingly accept the trade-off. Options, ordered by reversibility cost: (a) keep current behaviour but mention #3040 in the user-facing error message when output_0 ≥ static estimate but the helper succeeds (i.e. log a warn-level diagnostic); (b) gate [ReduceOutput(0)] behind a output_0 ≥ K * estimated_fee safety margin until #3040 lands; (c) accept Codex's prescription and revert Auto to [DeductFromInput(0)]-only until the estimator is fixed.
source: ['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 75-90: `InputSelection::Auto` admits `[ReduceOutput(0)]` despite the documented chain-time fee shortfall (#3040)
This push widens `Auto` to accept `[ReduceOutput(0)]`. The new helper's own Phase 4 comment (transfer.rs:656-672) acknowledges that `estimate_fee_for_inputs_pub` returns only the static `state_transition_min_fees` floor (~6.5M for 1-in/1-out), while the 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`. There is no fund loss or consensus risk (the protocol validator catches it), but the auto-selector now advertises a path it knows it cannot validate.
This is a judgment call rather than a clear regression: (1) the same underestimator weakness affects `[DeductFromInput(0)]` — the headroom reserved on input 0 is only as large as the static estimate, so a real-fee overshoot still leaves the input short; the difference is that DeductFromInput's failure surfaces from the input residue rather than the absorbing output. (2) The author signaled intent with the in-line comment + the parallel `docs(rs-platform-wallet): note platform #3040 fee-estimation bug` commit. (3) Restricting `Auto` would remove a useful selector for callers who knowingly accept the trade-off. Options, ordered by reversibility cost: (a) keep current behaviour but mention #3040 in the user-facing error message when output_0 ≥ static estimate but the helper succeeds (i.e. log a warn-level diagnostic); (b) gate `[ReduceOutput(0)]` behind a `output_0 ≥ K * estimated_fee` safety margin until #3040 lands; (c) accept Codex's prescription and revert `Auto` to `[DeductFromInput(0)]`-only until the estimator is fixed.
Comment on lines
+555
to
+654
| /// 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. | ||
| /// 4. Estimate the fee for the chosen input count and verify | ||
| /// `output[0] ≥ estimated_fee`; otherwise the chain-time | ||
| /// `ReduceOutput(0)` deduction would leave the fee uncovered. | ||
| /// 5. Defensive invariant checks. | ||
| 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 = accumulated.saturating_add(balance); | ||
| 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 { | ||
| 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_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= min_input_amount.saturating_add(shift)) | ||
| .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, | ||
| min_input_amount.saturating_add(shift), | ||
| ))); | ||
| }; | ||
| 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.
🟡 Suggestion: select_inputs_reduce_output Phase 3 doc says "lex-smallest peer" but the code uses caller order
Both the algorithm summary at lines 555-558 and the inline comment at lines 630-632 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 637-641 instead iterates prefix (the caller-supplied order, which is 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. Functionally either ordering is valid (any peer with sufficient headroom satisfies the invariants), but the prior select_inputs_deduct_from_input helper takes care 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 (which IS lex-ordered) so the documented invariant is enforced.
source: ['claude']
🤖 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 555-654: `select_inputs_reduce_output` Phase 3 doc says "lex-smallest peer" but the code uses caller order
Both the algorithm summary at lines 555-558 and the inline comment at lines 630-632 describe lifting the trimmed last entry "from the lex-smallest peer with spare balance". The implementation at lines 637-641 instead iterates `prefix` (the caller-supplied order, which is 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. Functionally either ordering is valid (any peer with sufficient headroom satisfies the invariants), but the prior `select_inputs_deduct_from_input` helper takes care 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` (which IS lex-ordered) so the documented invariant is enforced.
Comment on lines
+540
to
+706
| /// `[ReduceOutput(0)]` selector. Output 0 absorbs the fee at chain | ||
| /// time, so inputs only need to sum to `total_output` — no fee | ||
| /// headroom on inputs. Order-agnostic: walks `candidates` as-is and | ||
| /// picks the smallest covering prefix. | ||
| /// | ||
| /// Produces an inputs map satisfying: | ||
| /// 1. `Σ selected.values() == total_output`. | ||
| /// 2. Every selected input ≥ `min_input_amount`. | ||
| /// 3. The BTreeMap-index-0 output (lex-smallest) holds enough to | ||
| /// absorb the estimated fee at chain time. | ||
| /// | ||
| /// Algorithm (mirrors the 5-phase shape of the input-side helper): | ||
| /// 1. Grow the prefix until `Σ balances ≥ total_output`. | ||
| /// 2. Trim the last prefix entry by `surplus = Σ − total_output` so | ||
| /// `Σ inputs == Σ outputs`. Earlier entries stay at full balance. | ||
| /// 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. | ||
| /// 4. Estimate the fee for the chosen input count and verify | ||
| /// `output[0] ≥ estimated_fee`; otherwise the chain-time | ||
| /// `ReduceOutput(0)` deduction would leave the fee uncovered. | ||
| /// 5. Defensive invariant checks. | ||
| 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 = accumulated.saturating_add(balance); | ||
| 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 { | ||
| 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_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= min_input_amount.saturating_add(shift)) | ||
| .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, | ||
| min_input_amount.saturating_add(shift), | ||
| ))); | ||
| }; | ||
| 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: `estimate_fee_for_inputs_pub` 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 = estimate_fee_for_inputs_pub( | ||
| 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 lacks tests for Phase 3 redistribution and insufficient-balance branches
The four new ReduceOutput tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, and the Phase 4 fee-cannot-be-absorbed error. 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 643-649 are dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 608-614). The Phase 3 redistribution branch is the most non-trivial new logic and is symmetric to the input-side helper's non_fee_target_below_min_input_redistributes — pin it directly. 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']
🤖 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 540-706: `select_inputs_reduce_output` lacks tests for Phase 3 redistribution and insufficient-balance branches
The four new ReduceOutput tests cover the single-input happy path, a multi-input no-Phase-3 trim, a multi-output absorbance case, and the Phase 4 fee-cannot-be-absorbed error. 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 643-649 are dead from the test suite's perspective), or (c) the Phase 1 insufficient-balance error (lines 608-614). The Phase 3 redistribution branch is the most non-trivial new logic and is symmetric to the input-side helper's `non_fee_target_below_min_input_redistributes` — pin it directly. 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
+1176
to
+1221
| /// exercise its in-helper redistribution path. | ||
| #[test] | ||
| fn non_fee_target_below_min_input_redistributes() { | ||
| let addr_x = p2pkh(0x01); // lex-smallest → fee target | ||
| let addr_y = p2pkh(0x02); | ||
| let target = p2pkh(0x99); | ||
| let pv = LATEST_PLATFORM_VERSION; | ||
| let min_input = pv.dpp.state_transitions.address_funds.min_input_amount; | ||
|
|
||
| // total_output sits above `min_output_amount` (500_000) so the | ||
| // separate per-output minimum check doesn't shadow what we're | ||
| // testing — the input-side redistribution path. | ||
| let total_output = 950_000u64; | ||
| let addr_x_balance = 1_000_000u64; // covers total_output + fee on its own | ||
| let addr_y_balance = 30_000u64; // below min_input_amount | ||
| let outputs = outputs_for(target, total_output); | ||
| let candidates = vec![(addr_x, addr_x_balance), (addr_y, addr_y_balance)]; | ||
| let fee_strategy = vec![AddressFundsFeeStrategyStep::DeductFromInput(0)]; | ||
|
|
||
| let result = | ||
| select_inputs_deduct_from_input(candidates, &outputs, total_output, &fee_strategy, pv); | ||
|
|
||
| match result { | ||
| Ok(selected) => { | ||
| // Every selected input must satisfy the per-input minimum. | ||
| for (addr, amount) in selected.iter() { | ||
| assert!( | ||
| *amount >= min_input, | ||
| "input {} consumes {} which is below min_input_amount {}", | ||
| format_address(addr), | ||
| amount, | ||
| min_input, | ||
| ); | ||
| } | ||
| let input_sum: Credits = selected.values().sum(); | ||
| assert_eq!(input_sum, total_output); | ||
| assert_selection_validates(&selected, &outputs, fee_strategy, pv); | ||
| } | ||
| Err(PlatformWalletError::AddressOperation(_)) => { | ||
| // Acceptable: the helper errored out rather than | ||
| // redistribute. The failure we're guarding against | ||
| // is a silent sub-minimum input. | ||
| } | ||
| Err(other) => panic!("unexpected error variant: {other:?}"), | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: non_fee_target_below_min_input_redistributes still never reaches the redistribute path it documents
Carryover from the prior round, still unaddressed. With current fee constants (input_cost=500_000, output_cost=6_000_000), estimate_min_fee(1,1)=6_500_000 and estimate_min_fee(2,1)=7_000_000. Tracing the test's inputs [(addr_x, 1_000_000), (addr_y, 30_000)] with total_output=950_000: prefix=[addr_x] yields accumulated=1_000_000 vs required=7_450_000 (Phase 1 fails, continue); prefix=[addr_x, addr_y] yields accumulated=1_030_000 vs required=7_950_000 (Phase 1 fails, continue); loop exhausts and returns Err("Insufficient balance..."). The Err(PlatformWalletError::AddressOperation(_)) => {} arm silently accepts this, so the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test currently only proves "didn't panic with an unrelated error variant". To reach the documented branch, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below min_input_amount — construct two ≥ min_input candidates where adding a second entry produces tentative < min_input because remaining = T - fee_target_min is small.
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 1176-1221: `non_fee_target_below_min_input_redistributes` still never reaches the redistribute path it documents
Carryover from the prior round, still unaddressed. With current fee constants (`input_cost=500_000`, `output_cost=6_000_000`), `estimate_min_fee(1,1)=6_500_000` and `estimate_min_fee(2,1)=7_000_000`. Tracing the test's inputs `[(addr_x, 1_000_000), (addr_y, 30_000)]` with `total_output=950_000`: prefix=[addr_x] yields accumulated=1_000_000 vs required=7_450_000 (Phase 1 fails, continue); prefix=[addr_x, addr_y] yields accumulated=1_030_000 vs required=7_950_000 (Phase 1 fails, continue); loop exhausts and returns `Err("Insufficient balance...")`. The `Err(PlatformWalletError::AddressOperation(_)) => {}` arm silently accepts this, so the documented Ok-branch invariants (every input ≥ min_input, sum == total_output, validate_structure passes) are never asserted. The test currently only proves "didn't panic with an unrelated error variant". To reach the documented branch, addr_x alone must be infeasible while a multi-input prefix becomes feasible AND the tail's tentative falls below `min_input_amount` — construct two ≥ min_input candidates where adding a second entry produces `tentative < min_input` because `remaining = T - fee_target_min` is small.
Comment on lines
+199
to
+217
| // Filter to addresses with balance ≥ `min_input_amount` (the | ||
| // protocol's per-input minimum — anything smaller cannot | ||
| // legally appear as an input) and sort balance-descending so | ||
| // the helper picks the smallest covering prefix. | ||
| let mut candidates: Vec<(PlatformAddress, Credits)> = account | ||
| .addresses | ||
| .addresses | ||
| .values() | ||
| .filter_map(|addr_info| { | ||
| let p2pkh = PlatformP2PKHAddress::from_address(&addr_info.address).ok()?; | ||
| let balance = account.address_credit_balance(&p2pkh); | ||
| if balance == 0 { | ||
| continue; | ||
| if balance < min_input_amount { | ||
| None | ||
| } else { | ||
| Some((PlatformAddress::P2pkh(p2pkh.to_bytes()), balance)) | ||
| } | ||
| }) | ||
| .collect(); | ||
| candidates.sort_by(|a, b| b.1.cmp(&a.1)); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: All-candidates-below-min_input still surfaces as generic "Insufficient balance"
Carryover, still unaddressed and now applies to both selector helpers. auto_select_inputs filters out every address with balance < min_input_amount upstream. If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, candidates is empty and the dispatched helper returns Insufficient balance: available 0 credits, required .... That message is misleading: the wallet does hold credits — they're stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop, especially now that the new [ReduceOutput(0)] path produces the same misleading message.
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 199-217: All-candidates-below-min_input still surfaces as generic "Insufficient balance"
Carryover, still unaddressed and now applies to both selector helpers. `auto_select_inputs` filters out every address with `balance < min_input_amount` upstream. If a wallet has accumulated micro-payments such that every address holds < 100_000 credits but the aggregate is non-zero, `candidates` is empty and the dispatched helper returns `Insufficient balance: available 0 credits, required ...`. That message is misleading: the wallet does hold credits — they're stranded across sub-minimum addresses. A diagnostic error here ("available balance is held across N addresses each below the protocol minimum input amount; consolidate before transferring") would meaningfully shorten the diagnostic loop, especially now that the new `[ReduceOutput(0)]` path produces the same misleading message.
Comment on lines
+616
to
+654
| // 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 { | ||
| 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_addr = prefix | ||
| .iter() | ||
| .filter(|(addr, _)| *addr != last_addr) | ||
| .find(|(_, balance)| *balance >= min_input_amount.saturating_add(shift)) | ||
| .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, | ||
| min_input_amount.saturating_add(shift), | ||
| ))); | ||
| }; | ||
| 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
Phase 2 (lines 616-628) 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 693-696 is a no-op in release. In production this is unreachable because auto_select_inputs filters < min_input_amount candidates upstream (transfer.rs:199-217), 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']
Comment on lines
173
to
+706
| @@ -171,58 +190,59 @@ impl PlatformAddressWallet { | |||
| )) | |||
| })?; | |||
|
|
|||
| // BTreeMap<u32, _> iteration is already in ascending index order. | |||
| let mut selected = BTreeMap::new(); | |||
| let mut accumulated: Credits = 0; | |||
| let min_input_amount = platform_version | |||
| .dpp | |||
| .state_transitions | |||
| .address_funds | |||
| .min_input_amount; | |||
|
|
|||
| for addr_info in account.addresses.addresses.values() { | |||
| if let Ok(p2pkh) = PlatformP2PKHAddress::from_address(&addr_info.address) { | |||
| // Filter to addresses with balance ≥ `min_input_amount` (the | |||
| // protocol's per-input minimum — anything smaller cannot | |||
| // legally appear as an input) and sort balance-descending so | |||
| // the helper picks the smallest covering prefix. | |||
| let mut candidates: Vec<(PlatformAddress, Credits)> = account | |||
| .addresses | |||
| .addresses | |||
| .values() | |||
| .filter_map(|addr_info| { | |||
| let p2pkh = PlatformP2PKHAddress::from_address(&addr_info.address).ok()?; | |||
| let balance = account.address_credit_balance(&p2pkh); | |||
| if balance == 0 { | |||
| continue; | |||
| if balance < min_input_amount { | |||
| None | |||
| } else { | |||
| Some((PlatformAddress::P2pkh(p2pkh.to_bytes()), balance)) | |||
| } | |||
| }) | |||
| .collect(); | |||
| candidates.sort_by(|a, b| b.1.cmp(&a.1)); | |||
|
|
|||
| let address = PlatformAddress::P2pkh(p2pkh.to_bytes()); | |||
| selected.insert(address, balance); | |||
| accumulated = accumulated.saturating_add(balance); | |||
|
|
|||
| // Re-estimate fee with the current input count. | |||
| let estimated_fee = Self::estimate_fee_for_inputs( | |||
| selected.len(), | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ); | |||
| let required = total_output.saturating_add(estimated_fee); | |||
|
|
|||
| if accumulated >= required { | |||
| return Ok(selected); | |||
| } | |||
| } | |||
| match fee_strategy { | |||
| [AddressFundsFeeStrategyStep::DeductFromInput(0)] => select_inputs_deduct_from_input( | |||
| candidates, | |||
| outputs, | |||
| total_output, | |||
| fee_strategy, | |||
| platform_version, | |||
| ), | |||
| [AddressFundsFeeStrategyStep::ReduceOutput(0)] => select_inputs_reduce_output( | |||
| candidates, | |||
| outputs, | |||
| total_output, | |||
| fee_strategy, | |||
| platform_version, | |||
| ), | |||
| _ => Err(PlatformWalletError::AddressOperation( | |||
| "auto_select_inputs supports fee_strategy = [DeductFromInput(0)] \ | |||
| or [ReduceOutput(0)]; other shapes must use InputSelection::Explicit" | |||
| .to_string(), | |||
| )), | |||
| } | |||
|
|
|||
| // Not enough funds. | |||
| let estimated_fee = Self::estimate_fee_for_inputs( | |||
| selected.len().max(1), | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ); | |||
| let required = total_output.saturating_add(estimated_fee); | |||
| Err(PlatformWalletError::AddressOperation(format!( | |||
| "Insufficient balance: available {} credits, required {} (outputs {} + estimated fee {})", | |||
| accumulated, required, total_output, estimated_fee | |||
| ))) | |||
| } | |||
|
|
|||
| /// Simulate the fee strategy to determine how much additional balance | |||
| /// the inputs need beyond the output amounts. | |||
| /// | |||
| /// Walks through the fee strategy steps in order, deducting from the | |||
| /// available sources (outputs or inputs) until the fee is covered. | |||
| /// Returns the portion of the fee that must come from inputs. | |||
| /// the inputs need beyond the output amounts. Walks the strategy | |||
| /// steps in order, deducting from outputs/inputs until the fee is | |||
| /// covered, and returns the portion that must come from inputs. | |||
| fn estimate_fee_for_inputs( | |||
| input_count: usize, | |||
| output_count: usize, | |||
| @@ -266,3 +286,1103 @@ impl PlatformAddressWallet { | |||
| remaining_fee | |||
| } | |||
| } | |||
|
|
|||
| /// Module-scope view of the per-input fee estimator so [`select_inputs`] | |||
| /// can drive it without an instance of [`PlatformAddressWallet`]. | |||
| fn estimate_fee_for_inputs_pub( | |||
| input_count: usize, | |||
| output_count: usize, | |||
| fee_strategy: &[AddressFundsFeeStrategyStep], | |||
| outputs: &BTreeMap<PlatformAddress, Credits>, | |||
| platform_version: &PlatformVersion, | |||
| ) -> Credits { | |||
| PlatformAddressWallet::estimate_fee_for_inputs( | |||
| input_count, | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ) | |||
| } | |||
|
|
|||
| /// `[DeductFromInput(0)]` selector. Order-agnostic: walks | |||
| /// `candidates` as-is and picks the smallest covering prefix. | |||
| /// | |||
| /// Produces an inputs map satisfying two protocol invariants: | |||
| /// 1. `Σ selected.values() == total_output`. | |||
| /// 2. The `DeductFromInput(0)` fee target — the lex-smallest entry, | |||
| /// which is the `BTreeMap` index-0 — must keep | |||
| /// `balance − consumed ≥ estimated_fee` so drive can deduct | |||
| /// the fee from its remaining balance (otherwise | |||
| /// `fee_fully_covered = false` and the transition is rejected). | |||
| /// | |||
| /// Algorithm: | |||
| /// 1. Grow the prefix until `Σ balances ≥ total_output + estimated_fee`. | |||
| /// 2. Within that prefix, the lex-smallest entry is the fee target. | |||
| /// 3. Solve for `fee_target_consumed` in | |||
| /// `[max(min_input_amount, total_output − other_total), | |||
| /// fee_target_balance − estimated_fee]`. If the range is empty | |||
| /// (no headroom), extend the prefix and retry; error out only | |||
| /// when candidates are exhausted. | |||
| /// 4. Insert the fee target at its minimum consumption, then | |||
| /// distribute the remainder of `total_output` across the other | |||
| /// prefix entries in caller-supplied order. Tail consumptions | |||
| /// below `min_input_amount` get folded back into the fee target | |||
| /// rather than producing a sub-minimum input. | |||
| /// 5. Defensive invariant checks. | |||
| /// | |||
| /// Caller (`auto_select_inputs`) sorts candidates balance-descending | |||
| /// in practice, but the helper itself doesn't rely on that order. | |||
| fn select_inputs_deduct_from_input( | |||
| 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::DeductFromInput(0)] | |||
| ), | |||
| "select_inputs_deduct_from_input requires [DeductFromInput(0)]; \ | |||
| the dispatcher should have routed other shapes elsewhere" | |||
| ); | |||
| if !matches!( | |||
| fee_strategy, | |||
| [AddressFundsFeeStrategyStep::DeductFromInput(0)] | |||
| ) { | |||
| return Err(PlatformWalletError::AddressOperation( | |||
| "select_inputs_deduct_from_input only supports fee_strategy = \ | |||
| [DeductFromInput(0)]; other shapes must route through the dispatcher" | |||
| .to_string(), | |||
| )); | |||
| } | |||
|
|
|||
| let output_count = outputs.len(); | |||
| let min_input_amount = platform_version | |||
| .dpp | |||
| .state_transitions | |||
| .address_funds | |||
| .min_input_amount; | |||
|
|
|||
| // No input can simultaneously be ≥ `min_input_amount` AND sum to | |||
| // `total_output` if `total_output < min_input_amount`. Reject upfront | |||
| // 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-3: extend the prefix one candidate at a time until it | |||
| // covers `total_output + estimated_fee` AND the lex-smallest | |||
| // prefix entry has headroom to absorb the fee. | |||
| let mut prefix: Vec<(PlatformAddress, Credits)> = Vec::new(); | |||
| let mut accumulated: Credits = 0; | |||
| let mut last_estimated_fee: Credits = 0; | |||
| let mut feasible: Option<(PlatformAddress, Credits, Credits, Credits)> = None; | |||
|
|
|||
| for (address, balance) in candidates { | |||
| prefix.push((address, balance)); | |||
| accumulated = accumulated.saturating_add(balance); | |||
|
|
|||
| let estimated_fee = estimate_fee_for_inputs_pub( | |||
| prefix.len(), | |||
| output_count, | |||
| fee_strategy, | |||
| outputs, | |||
| platform_version, | |||
| ); | |||
| last_estimated_fee = estimated_fee; | |||
| let required = total_output.saturating_add(estimated_fee); | |||
|
|
|||
| if accumulated < required { | |||
| continue; | |||
| } | |||
|
|
|||
| // Phase 2: lex-smallest of the current prefix is the fee target. | |||
| let (fee_target_addr, fee_target_balance) = prefix | |||
| .iter() | |||
| .min_by_key(|(addr, _)| *addr) | |||
| .copied() | |||
| .expect("prefix is non-empty: we just pushed"); | |||
|
|
|||
| let fee_target_max = fee_target_balance.saturating_sub(estimated_fee); | |||
| let other_total: Credits = prefix | |||
| .iter() | |||
| .filter(|(addr, _)| addr != &fee_target_addr) | |||
| .map(|(_, bal)| *bal) | |||
| .sum(); | |||
| let fee_target_min = | |||
| std::cmp::max(min_input_amount, total_output.saturating_sub(other_total)); | |||
|
|
|||
| if fee_target_min <= fee_target_max { | |||
| feasible = Some(( | |||
| fee_target_addr, | |||
| fee_target_balance, | |||
| fee_target_min, | |||
| estimated_fee, | |||
| )); | |||
| break; | |||
| } | |||
| // Phase 3 failed for this prefix size: keep growing. | |||
| } | |||
|
|
|||
| let Some((fee_target_addr, fee_target_balance, fee_target_min, estimated_fee)) = feasible | |||
| else { | |||
| // Distinguish "couldn't cover total_output + fee" from | |||
| // "covered but no headroom-feasible fee target". | |||
| if accumulated < total_output.saturating_add(last_estimated_fee) { | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Insufficient balance: available {} credits, required {} \ | |||
| (outputs {} + estimated fee {})", | |||
| accumulated, | |||
| total_output.saturating_add(last_estimated_fee), | |||
| total_output, | |||
| last_estimated_fee, | |||
| ))); | |||
| } | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Cannot satisfy fee headroom: no covering prefix of the available inputs \ | |||
| leaves the lex-smallest entry with ≥ estimated fee {} of remaining balance \ | |||
| after consumption. Consider providing more inputs or using a different \ | |||
| fee strategy.", | |||
| last_estimated_fee, | |||
| ))); | |||
| }; | |||
|
|
|||
| // Phase 4: consume `fee_target_min` from the fee target, distribute | |||
| // the rest of `total_output` over the remaining prefix in caller | |||
| // order. Tail consumptions below `min_input_amount` get folded into | |||
| // the fee target — `validate_structure` would otherwise reject the | |||
| // transition with `InputBelowMinimumError`. | |||
| let mut fee_target_consumed = fee_target_min; | |||
| let fee_target_max = fee_target_balance.saturating_sub(estimated_fee); | |||
| let mut selected: BTreeMap<PlatformAddress, Credits> = BTreeMap::new(); | |||
|
|
|||
| let mut remaining = total_output.saturating_sub(fee_target_consumed); | |||
| let mut residue_to_fee_target: Credits = 0; | |||
| for (addr, bal) in prefix.iter() { | |||
| if *addr == fee_target_addr { | |||
| continue; | |||
| } | |||
| if remaining == 0 { | |||
| break; | |||
| } | |||
| let tentative = (*bal).min(remaining); | |||
| if tentative == 0 { | |||
| continue; | |||
| } | |||
| if tentative < min_input_amount { | |||
| // Sub-minimum input — fold into the fee target. | |||
| residue_to_fee_target = residue_to_fee_target.saturating_add(tentative); | |||
| remaining = remaining.saturating_sub(tentative); | |||
| continue; | |||
| } | |||
| selected.insert(*addr, tentative); | |||
| remaining = remaining.saturating_sub(tentative); | |||
| } | |||
|
|
|||
| if residue_to_fee_target > 0 { | |||
| let new_consumed = fee_target_consumed.saturating_add(residue_to_fee_target); | |||
| if new_consumed > fee_target_max { | |||
| // Should be unreachable given Phase 3's headroom check, but | |||
| // guarded explicitly: silently shipping an invalid | |||
| // transition would be worse than a loud error here. | |||
| return Err(PlatformWalletError::AddressOperation(format!( | |||
| "Cannot satisfy fee headroom after redistributing sub-minimum tail \ | |||
| inputs: fee-target {} would consume {} (balance {}, max {}), leaving \ | |||
| less than estimated fee {} of remaining balance", | |||
| format_address(&fee_target_addr), | |||
| new_consumed, | |||
| fee_target_balance, | |||
| fee_target_max, | |||
| estimated_fee, | |||
| ))); | |||
| } | |||
| fee_target_consumed = new_consumed; | |||
| } | |||
|
|
|||
| selected.insert(fee_target_addr, fee_target_consumed); | |||
|
|
|||
| // 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_eq!( | |||
| selected.keys().next().copied(), | |||
| Some(fee_target_addr), | |||
| "fee target must be the BTreeMap index-0 (lex-smallest) entry" | |||
| ); | |||
| debug_assert!( | |||
| fee_target_balance.saturating_sub(fee_target_consumed) >= estimated_fee, | |||
| "fee target must retain ≥ estimated_fee remaining balance for DeductFromInput(0)" | |||
| ); | |||
| 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) | |||
| } | |||
|
|
|||
| /// `[ReduceOutput(0)]` selector. Output 0 absorbs the fee at chain | |||
| /// time, so inputs only need to sum to `total_output` — no fee | |||
| /// headroom on inputs. Order-agnostic: walks `candidates` as-is and | |||
| /// picks the smallest covering prefix. | |||
| /// | |||
| /// Produces an inputs map satisfying: | |||
| /// 1. `Σ selected.values() == total_output`. | |||
| /// 2. Every selected input ≥ `min_input_amount`. | |||
| /// 3. The BTreeMap-index-0 output (lex-smallest) holds enough to | |||
| /// absorb the estimated fee at chain time. | |||
| /// | |||
| /// Algorithm (mirrors the 5-phase shape of the input-side helper): | |||
| /// 1. Grow the prefix until `Σ balances ≥ total_output`. | |||
| /// 2. Trim the last prefix entry by `surplus = Σ − total_output` so | |||
| /// `Σ inputs == Σ outputs`. Earlier entries stay at full balance. | |||
| /// 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. | |||
| /// 4. Estimate the fee for the chosen input count and verify | |||
| /// `output[0] ≥ estimated_fee`; otherwise the chain-time | |||
| /// `ReduceOutput(0)` deduction would leave the fee uncovered. | |||
| /// 5. Defensive invariant checks. | |||
| 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 = accumulated.saturating_add(balance); | |||
| 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 { | |||
| 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_addr = prefix | |||
| .iter() | |||
| .filter(|(addr, _)| *addr != last_addr) | |||
| .find(|(_, balance)| *balance >= min_input_amount.saturating_add(shift)) | |||
| .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, | |||
| min_input_amount.saturating_add(shift), | |||
| ))); | |||
| }; | |||
| 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: `estimate_fee_for_inputs_pub` 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 = estimate_fee_for_inputs_pub( | |||
| 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.
💬 Nitpick: Unchecked/saturating arithmetic on credit sums extends to the new ReduceOutput selector
Carryover with new sites. total_output: Credits = outputs.values().sum() (line 173) panics in debug on overflow but saturates in release; the new helper adds accumulated.saturating_add(balance) (line 602), balance.saturating_sub(surplus) (line 623), and min_input_amount.saturating_add(shift) (lines 640, 648). The protocol validator (AddressFundsTransferTransitionV0::validate_structure) uses checked_add and returns OverflowError deterministically. Practically unreachable given the credit supply, but transfer() is a public library API accepting raw u64 from callers; saturated values produce misleading error-message numbers and a saturated accumulated could trivially pass accumulated >= total_output. Use checked_add / checked_sum and surface a typed PlatformWalletError::Overflow variant at the boundary.
source: ['claude', 'codex']
Comment on lines
+290
to
+306
| /// Module-scope view of the per-input fee estimator so [`select_inputs`] | ||
| /// can drive it without an instance of [`PlatformAddressWallet`]. | ||
| fn estimate_fee_for_inputs_pub( | ||
| input_count: usize, | ||
| output_count: usize, | ||
| fee_strategy: &[AddressFundsFeeStrategyStep], | ||
| outputs: &BTreeMap<PlatformAddress, Credits>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Credits { | ||
| PlatformAddressWallet::estimate_fee_for_inputs( | ||
| input_count, | ||
| output_count, | ||
| fee_strategy, | ||
| outputs, | ||
| platform_version, | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: estimate_fee_for_inputs_pub is a tautological wrapper around a static method
Carryover, still unaddressed. The latest delta makes this clearer because estimate_fee_for_inputs_pub is now called from both select_inputs_deduct_from_input and the new select_inputs_reduce_output. PlatformAddressWallet::estimate_fee_for_inputs doesn't take &self, isn't dispatched through a trait, and reads no per-instance state. The wrapper exists only to forward module-scope callers to the static method. Move estimate_fee_for_inputs out of the impl as a free pub(super) fn, or keep it inside the impl and make the helpers associated fns, then delete the wrapper.
source: ['claude', 'codex']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
auto_select_inputsinpackages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rswas inserting each selected address with its full balance as the input'sCreditsvalue, then returning as soon as accumulated coveredoutput + fee. The address-funds-transfer protocol enforcesΣ inputs.credits == Σ outputs.credits(strict equality), so a bank with ~500B credits funding a 50M output produced:Verified at
rs-dpp/.../address_funds_transfer_transition/v0/state_transition_validation.rsand asserted on-chain byrs-drive-abci/.../address_funds_transfer/tests.rs::test_input_balance_decreased_correctly(new_balance == initial_balance - transfer_amount - fee).Protocol semantics (now respected)
inputs[addr].credits= consumed amount fromaddroutputs[addr]= credited amount toaddrΣ inputs.credits == Σ outputs.credits(strict equality)AddressFundsFeeStrategy.DeductFromInput(0)reduces the remaining balance by the fee — never the inputs map'sCreditsvalueconsumed >= min_input_amount(currently 100_000)What changed
The PR grew through review feedback. Each commit is a focused step.
Selector correctness (5 commits)
aaf8be74— initialΣ inputs == Σ outputsfix. Extracted the selection loop into a pure module-scope helperselect_inputsthat walks candidates and trims the result so the inputs map sums to exactlytotal_output.9ea9e703— fee-headroom guarantee atDeductFromInput(0)target (CodeRabbit critical). The original fix proved aggregate balance covered the fee but not that the specific fee-bearing input had remaining headroom. Now identifies the prospective fee target (lex-smallest of selected) and reserves at leastestimated_feeof remaining balance on it.687b1f86— protocol-level reproduction test. Reconstructs the OLD buggy selector output for the CodeRabbit example, feeds the post-consumptioninput_current_balancesthroughdpp::address_funds::fee_strategy::deduct_fee_from_outputs_or_remaining_balance_of_inputs, and asserts!fee_fully_covered. Proves the rejection at the protocol layer rather than asserting "the new output looks different."60f7850a— sort candidates by balance descending (mirrorsdash-evo-tool's allocator). Reduces the frequency of multi-input cases — when the largest single balance coverstotal_output + fee, the result is a 1-input map and the lex-smallest fee-target headroom logic doesn't fire at all. Bonus:fee_headroom_violation_errorsnow produces a debuggable error message.9ff937ff— second review wave (4 blocking, 1 suggestion):min_input_amountenforcement.auto_select_inputsfilters candidates <min_input_amountupfront; Phase 4 distribution rolls any sub-minimum tail residue back into the fee target's consumption rather than producing anInputBelowMinimumError-prone tail.fee_strategyrestriction intransfer().InputSelection::Autonow rejects any shape other than[DeductFromInput(0)]with a clear redirect toInputSelection::Explicit. The previous fallback path was publicly reachable but only protocol-correct for that single shape.fee_target_min > fee_target_max, the algorithm extends the prefix with the next candidate and retries instead of erroring out — larger prefixes can yield a different lex-smallest fee target with sufficient headroom.total_output < min_input_amounterror (replaces the internal-error fallthrough).assert_selection_validateshelper builds anAddressFundsTransferTransitionV0from each selector test's output and runsvalidate_structure. Catches future protocol-level regressions without depending on testnet.CI / infrastructure (3 commits)
79c2b285—ci(rs-packages-filter): trigger Rust workspace tests on rs-platform-wallet changes. The path filter at.github/package-filters/rs-packages-no-workflows.ymldidn't listrs-platform-wallet, so any crate-only change there evaluatedrs-packages = '[]'andRust workspace testssilently skipped. This PR's prior 5 commits had never been validated by Rust CI — only by localcargo test. The filter entry mirrors the existing pattern (path +*sdkalias for transitive triggers).d610502— mergev3.1-dev(9bd37f203a).3c4f9199— Rust 1.92 clippy hardening that the previously-skipped pipeline had been quietly accumulating:field_reassign_with_defaultincore_bridge.rs::build_core_changeset→ struct literal initlet_unit_valueinwallet/apply.rs:316(WalletInfoInterface::update_balancereturns()) → drop thelet _ =core.chain.synced_heightaccess intests/spv_sync.rs→ flattened tocore.synced_height(struct shape changed upstream; the test never recompiled because workspace tests were skipping)Tests (121 lib tests, all passing)
auto_select_testsmodule — 11 tests:single_input_oversized_balance_trims_to_output_amounttwo_input_selection_keeps_fee_headroom_at_index_zero(renamed from the original..._trims_only_the_lastafter the headroom fix)fee_target_keeps_remaining_for_fee_deduction— direct regression for CodeRabbit's examplepre_fix_buggy_selector_output_is_rejected_by_protocol_fee_deduction— protocol-level reproduction (asserts!fee_fully_covered)fee_only_tail_input_does_not_inflate_input_sumdescending_order_picks_single_largest_when_sufficientnon_fee_target_below_min_input_redistributestotal_output_below_min_input_amount_errorsfee_headroom_violation_errorsinsufficient_balance_errorsno_candidates_errors5 of them now also assert structural validity via
assert_selection_validates→AddressFundsTransferTransitionV0::validate_structure.Note on withdrawal selector
The
auto_select_inputs_for_withdrawalrustdoc clarifies the asymmetry: withdrawal validatesΣ inputs > output_amount(strictly greater, surplus = fee), so its drain-everything strategy is correct by design. Not the same bug; no code change.Verification
cargo fmt -p platform-wallet --check✓cargo clippy --workspace --tests -- -D warnings✓cargo test -p platform-wallet --lib— 121/121 passing3c4f9199(Rust workspace tests / macOS now exercising this crate, was previously skipping)Test plan
deduct_fee_from_outputs_or_remaining_balance_of_inputstransfer(),auto_select_inputs,select_inputssignatures stable;transfer()now rejects non-[DeductFromInput(0)]forAutowith a clear error)Provenance
Originally surfaced and fixed during work on PR #3549 (
rs-platform-wallete2e harness). Split out so the production-code fix can ship independently of the long-running e2e branch. Subsequent commits address review feedback from CodeRabbit andthepastaclawreviewers and close a CI coverage gap that was hiding pre-existing breaks onv3.1-dev.🤖 Generated with Claude Code