fix(platform-wallet): arithmetic + sync + auto-select hardening#3577
Draft
Claudius-Maginificent wants to merge 15 commits intofeat/rs-platform-wallet-e2efrom
Draft
fix(platform-wallet): arithmetic + sync + auto-select hardening#3577Claudius-Maginificent wants to merge 15 commits intofeat/rs-platform-wallet-e2efrom
Claudius-Maginificent wants to merge 15 commits intofeat/rs-platform-wallet-e2efrom
Conversation
…3576) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…puts candidates [QA-001] When the only sufficiently-funded address is also a destination output, auto_select_inputs would propose it as both input and output, and the protocol would reject the resulting transition with `Output address cannot also be an input address`. Filter outputs.keys() out of the candidate set up-front; callers wanting to spend from an output address must use InputSelection::Explicit and split the operation. Surfaced by pa_003_fee_scaling in PR #3571's e2e suite (QA-001). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ransfer [CMT-005/006/007]
Replace 13 of the 17 saturating_add/saturating_sub sites on Credits in
auto_select_inputs and its helpers (select_inputs_deduct_from_input,
select_inputs_reduce_output) with checked_add/checked_sub, surfacing a
typed PlatformWalletError::ArithmeticOverflow { context } at each
call site. Total Dash supply is far below u64::MAX so overflow is
unreachable in practice — this is defensive correctness, not a bug fix.
Four sites are kept saturating with explanatory comments because the
saturate-to-zero path is part of the algorithm rather than an unreachable
overflow guard:
- fee_target_max may legitimately go below zero for a thin fee target;
the headroom check then rejects that prefix size.
- total_output - other_total may go below zero when peers alone cover
the outputs; the max(min_input_amount, ..) wrapper recovers the
intended floor.
- The Phase 5 debug_assert exists to catch a negative remaining
(saturating to 0 trips the >= estimated_fee check).
- Phase 2's last-entry trim has a proven-by-construction lower bound
(surplus < last_balance) — saturating is documentary defense.
Addresses thepastaclaw's deferred review feedback on PR #3554.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…apper [CMT-008] The pub wrapper around the static estimate_fee_for_inputs was a no-op trampoline kept around to give module-scope helpers (select_inputs_*) a callable name. Module-scope items in the same file can call non-pub impl items directly, so the wrapper carried no behavior. Inlined the 8 production + helper-test call sites to call PlatformAddressWallet::estimate_fee_for_inputs directly and dropped the wrapper definition; the docstring referencing it was updated to match. Addresses thepastaclaw's deferred review feedback on PR #3554. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stributes [CMT-009]
The previous fixture (addr_x=1M, addr_y=30k, total_output=950k) never
reached the helper's Ok branch — Phase 1 exhausted candidates without
covering total_output + 6.5M static fee, so the helper returned the
"Insufficient balance" AddressOperation error path that the test's
panic-on-unexpected-variants happily accepted. The Ok-branch
redistribute invariants the docstring promised were never asserted.
Engineer the fixture against the real fee schedule
(input_cost=500_000, output_cost=6_000_000): addr_x=10M (fee target),
addr_y=80k (sub-min peer), addr_z=2M (large peer), total_output=4M.
Phase 1 grows to [x,y,z]; Phase 3 finds headroom; Phase 4 folds y's
80k residue into x; final selected = {x: 2M, z: 2M}.
Replaced the lenient panic-on-unexpected-variant guard with hard
assertions on the Ok branch — every selected input ≥ min_input_amount,
sub-min y must NOT appear in the inputs map, the fee target absorbs
the folded residue, Σ inputs == Σ outputs, and validate_structure
greenlights the result.
Addresses thepastaclaw's deferred review feedback on PR #3554.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…014 + QA-001/002] PR #3554's QA-001 fix excluded output addresses from the auto-select candidate set, but the remaining "all funded addresses are outputs" failure mode still surfaced as a generic AddressOperation insufficient- balance string. Replace that with a typed PlatformWalletError::OnlyOutputAddressesFunded { outputs } variant, detected after build_auto_select_candidates returns empty by re- scanning address_balances with the outputs filter dropped. The Display template interpolates {outputs:?} so error.to_string() carries the offending addresses across boundaries that flatten typed error variants (notably FFI). Pure-helper unit tests pin three branches: typed-payload happy path, none when no funded address, none when a funded non-output exists. An end-to-end integration test driving auto_select_inputs through the typed-error branch (QA-002) would require a WalletManager harness this crate doesn't yet expose; the production code path is annotated with a TODO(QA-002) referencing the pure-helper coverage. Removed the QA-001-followup TODO superseded by the typed error variant. Addresses Marvin's QA-001 (Display interpolation) and QA-002 (the detection logic), and PR #3554's deferred TODO. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…QA-002] The provider's three sync watermarks (sync_height, sync_timestamp, last_known_recent_block) were unconditionally overwritten on every incremental sync result. Out-of-order completion of a stale scan (network jitter, retry, parallel pass) could roll the watermarks backwards and trigger redundant rescanning, undoing earlier progress. Replace the unconditional assignment with a per-field max so each counter is monotonic in isolation. Per-field rather than all-or-nothing: a result that advances some fields and regresses others should still lift the advancing ones — tying the watermarks together would either lose progress (reject the whole result) or roll some fields back (accept the whole result). `set_stored_sync_state` keeps the unconditional overwrite — it's the load-from-persistence entry point, used before any incremental result has merged. Documented the asymmetry in both rustdocs. Four unit tests in `provider::tests` pin the four observable shapes: forward advance, full backwards rejection, per-field merge, and the loader's unconditional overwrite. The provider is constructed with a minimal in-memory `WalletManager::new(Network::Testnet)` plus empty maps — no I/O, no SDK round-trips. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ssesFunded explicitly [QA-003] Both new typed wallet error variants previously flattened to ErrorUnknown at the FFI boundary because the From<PlatformWalletError> impl unconditionally used that catch-all code. Downstream consumers (Swift, FFI tests, telemetry) couldn't distinguish these failures from a generic unknown error, defeating the typed-error work in the upstream rs-platform-wallet hardening pass. Allocate two new FFI codes (ErrorArithmeticOverflow=13, ErrorOnlyOutputAddressesFunded=14) and route the matching wallet variants to them via an explicit `match` in the From impl. The Display rendering — including QA-001's outputs payload interpolation — still flows through as the message, so callers without typed-error access can recover the offending addresses by parsing the message. Three new tests in error::tests: each new variant maps to its dedicated code with the typed Display preserved as the message; the catch-all ErrorUnknown remains the only fallback for unmapped variants. Surfaced by Marvin's QA audit of the rs-platform-wallet hardening branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dresses_funded tests Rust 1.92's `clippy::useless_vec` flagged three test fixtures created with `vec![...]` only to drive `.iter().copied()`. Replace with array literals — the tests don't need a heap-allocated `Vec`. Pure cleanup, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r() in tokens/group_info tests Rust 1.92's `clippy::redundant_pattern_matching` flagged two test-only `matches!(result, Err(_))` patterns. Replace with the suggested `result.is_err()` form. Pure cleanup, no behavior change. Pre-existing on the base branch — surfaced once -D warnings was turned on for this branch's CI gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…platform-wallet-arithmetic-and-sync-hardening
lklimek
changed the title
lklimek
changed the base branch from
fix/rs-platform-wallet-auto-select-inputs
to
feat/rs-platform-wallet-e2e
…o fix/rs-platform-wallet-arithmetic-and-sync-hardening Conflicts: - packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs Resolution: kept #3577's max-merge update_sync_state and enhanced set_stored_sync_state docstring; slotted #3549's new public last_known_recent_block() getter immediately after update_sync_state (which writes the same field). Both sides' additions preserved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…o fix/rs-platform-wallet-arithmetic-and-sync-hardening
…s.rs
- line 350: manual_unwrap_or_default — replace match { Some(n) => n, None => 0 }
with .unwrap_or_default() on IdentitySyncManager::try_queue_depth()
- line 705: unnecessary_cast — remove redundant `as u32` cast on *reg_idx
(RegistrationIndex is already u32)
- line 745: redundant_closure — replace |info| addr_info_snapshot(info)
with addr_info_snapshot (eta-reduction)
No behavioural change. Pure lint hygiene, passes cargo clippy -- -D warnings
and 133 lib unit tests on Linux.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cluster of deferred review feedback from PR #3554 plus QA findings surfaced by PR #3571's e2e suite, all on the same
rs-platform-walletarithmetic / sync / auto-select surface.Creditsinauto_select_inputsand its helpers — replaces 13 of 17saturating_*sites withchecked_*returning typedPlatformWalletError::ArithmeticOverflow. The remaining 4 sites where saturate-to-zero is part of the algorithm are kept with rationale comments. (addresses CMT-005/006/007)estimate_fee_for_inputs_pubwrapper — pub wrapper added no behavior, inlined at 8 callers. (CMT-008)non_fee_target_below_min_input_redistributesfixture — previous fixture exhausted onAddressOperationand silently passed; rewrite uses engineered cost values to actually reach Phase 3 feasibility + Phase 4 fold and asserts the post-redistribute invariants. (CMT-009)OnlyOutputAddressesFundederror — auto-select previously surfaced "Insufficient balance" when the only funded address was also a destination output; new variant carries the offending outputs in the Display template (soerror.to_string()keeps the payload across FFI). (CMT-014 + Marvin QA-001 from PR test(platform-wallet): implement PA P0 — multi-output, partial-fund, sweep-back #3571 audit)update_sync_statewas rolling backlast_known_recent_block/sync_height/sync_timestampto 0 on no-progress syncs, breaking the contractsync_watermark()documents. Per-field.max()merge with full impact analysis (/tmp/claudius-pr3571/qa002-impact-analysis.md). Fixes PR test(platform-wallet): implement PA P0 — multi-output, partial-fund, sweep-back #3571'spa_007_sync_watermark. (QA-002)ArithmeticOverflowandOnlyOutputAddressesFundedpreviously flattened toErrorUnknownat the FFI boundary; now dedicated result codes with explicit match arms + 3 unit tests. (QA-003)Stacked on PR #3554 (
fix/rs-platform-wallet-auto-select-inputs). Base auto-rebases tov3.1-devonce #3554 merges.Test plan
cargo fmt --check— greencargo check --tests -p platform-wallet— greencargo clippy --tests -p platform-wallet -- -D warnings— greencargo test --lib -p platform-wallet— 141 passedplatform-wallet-ffi— 76 passedCarryover
auto_select_inputspath through the typed-error branch (Marvin QA-002 from PR test(platform-wallet): implement PA P0 — multi-output, partial-fund, sweep-back #3571 audit) — deferred withTODO(QA-002)because noWalletManagertest harness exists yet. Recommend a follow-up PR introducing a reusable harness fortransfer/withdrawal/fund_from_asset_lock.🤖 Co-authored by Claudius the Magnificent AI Agent