test(rs-platform-wallet): e2e suite, Found-025 fix + triage pins#3549
Draft
Claudius-Maginificent wants to merge 284 commits into
Draft
test(rs-platform-wallet): e2e suite, Found-025 fix + triage pins#3549Claudius-Maginificent wants to merge 284 commits into
Claudius-Maginificent wants to merge 284 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAn end-to-end testing framework for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Harness as E2eContext Harness
participant Registry as Wallet Registry
participant Bank as BankWallet
participant TWallet as TestWallet
participant Manager as PlatformWalletManager
participant SDK as SDK/PlatformWallet
participant Cleanup as Cleanup
Test->>Harness: init() first call
Harness->>Registry: open(test_wallets.json)
Harness->>Cleanup: sweep_orphans()
Cleanup->>Registry: list_orphans()
Cleanup->>Manager: create from orphan seed
Cleanup->>SDK: sync & drain to bank
Cleanup->>Registry: remove_orphan_entry
Harness->>Bank: load from mnemonic
Harness->>Bank: sync_balances()
Harness->>Bank: fund_address(test_addr1, credits)
Harness->>SDK: transfer via bank wallet
Test->>Test: setup() generates seed
Test->>Manager: create TestWallet
Test->>TWallet: create(seed)
Test->>TWallet: next_unused_address() → addr2
Test->>Bank: fund_address(addr2, TRANSFER_CREDITS)
Test->>SDK: transfer via bank
Test->>TWallet: wait_for_balance(addr2, expected)
TWallet->>SDK: sync_balances()
Test->>SDK: transfer(addr2 → addr1, TRANSFER_CREDITS)
SDK->>SDK: execute, compute fee
Test->>TWallet: verify balances & fee
Test->>Test: teardown()
Test->>Cleanup: teardown_one(test_wallet)
Cleanup->>TWallet: drain all addresses to bank
Cleanup->>Registry: remove_entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end (wallet → SDK → broadcast) integration test harness to rs-platform-wallet and introduces the first live test case (address-funds transfer), alongside a production fix to InputSelection::Auto input selection so generated transitions satisfy protocol structure rules.
Changes:
- Added a reusable E2E framework under
packages/rs-platform-wallet/tests/e2e/(workdir slot locking, bank wallet, persistent registry, cleanup/sweep, wait hub, signer, SDK wiring). - Added the first E2E test case: transferring credits between two platform-payment addresses in a test wallet (ignored by default).
- Fixed
auto_select_inputsin production code to avoid selecting full balances as “input credits”, and added unit tests for the selection logic.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs | Fixes auto input selection; adds pure helper + unit tests for selection behavior. |
| packages/rs-platform-wallet/tests/e2e.rs | Adds the integration test crate root and module wiring for the e2e suite. |
| packages/rs-platform-wallet/tests/e2e/README.md | Operator/setup documentation for running live e2e tests. |
| packages/rs-platform-wallet/tests/e2e/cases/mod.rs | Declares e2e test modules. |
| packages/rs-platform-wallet/tests/e2e/cases/transfer.rs | First e2e test exercising funding + self-transfer + teardown. |
| packages/rs-platform-wallet/tests/e2e/framework/mod.rs | Framework public surface (setup, errors, prelude) and module layout. |
| packages/rs-platform-wallet/tests/e2e/framework/harness.rs | E2eContext singleton init: config, workdir locking, SDK, manager, bank, registry, startup sweep. |
| packages/rs-platform-wallet/tests/e2e/framework/config.rs | Env/.env configuration loader for the harness. |
| packages/rs-platform-wallet/tests/e2e/framework/sdk.rs | Constructs dash_sdk::Sdk with TrustedHttpContextProvider and DAPI address resolution. |
| packages/rs-platform-wallet/tests/e2e/framework/workdir.rs | Cross-process workdir slot selection via flock. |
| packages/rs-platform-wallet/tests/e2e/framework/panic_hook.rs | Installs panic hook to cancel background work on panic. |
| packages/rs-platform-wallet/tests/e2e/framework/wait_hub.rs | Notify-based hub bridging wallet/SPV/platform events to async waiters. |
| packages/rs-platform-wallet/tests/e2e/framework/wait.rs | Async waiting helpers (event-driven balance wait + generic polling). |
| packages/rs-platform-wallet/tests/e2e/framework/signer.rs | Seed-backed Signer<PlatformAddress> with eager DIP-17 key cache. |
| packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs | Test wallet factory + SetupGuard (panic-safe registry-backed lifecycle). |
| packages/rs-platform-wallet/tests/e2e/framework/registry.rs | JSON-backed persistent registry for panic-safe orphan recovery. |
| packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs | Startup sweep + per-test teardown draining funds back to bank. |
| packages/rs-platform-wallet/tests/e2e/framework/bank.rs | Loads and syncs a pre-funded bank wallet; serialized funding API. |
| packages/rs-platform-wallet/tests/e2e/framework/context_provider.rs | Retained (disabled) SPV-backed SDK context provider module for future re-enable. |
| packages/rs-platform-wallet/tests/e2e/framework/spv.rs | Retained (disabled) SPV startup/readiness helpers for future re-enable. |
| packages/rs-platform-wallet/Cargo.toml | Adds dev-dependencies needed by the e2e harness. |
| Cargo.lock | Locks new/updated dependencies for the added test tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6 tasks
8 tasks
lklimek
changed the base branch from
v3.1-dev
to
fix/rs-platform-wallet-auto-select-inputs
7 tasks
lklimek
reviewed
Apr 29, 2026
lklimek
reviewed
Apr 29, 2026
…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>
…om key_wallet Replace the duplicated DEFAULT_ACCOUNT_INDEX / DEFAULT_KEY_CLASS constants with a default_platform_payment_account_key() helper that destructures key_wallet's PlatformPaymentAccountSpec::default(), and pin the const _PUB values to the same canonical struct's fields. A colocated drift test asserts PlatformPaymentAccountSpec::default() still matches our pinned constants — preventing silent drift if upstream defaults change. WalletAccountCreationOptions::Default is a unit variant (the (account, key_class) shape lives in the BTreeSet variant, not Default itself), so destructuring Default directly was not viable. Pinning to PlatformPaymentAccountSpec — the canonical "what does Default mean for a PlatformPayment account" struct — is the closest equivalent. Resolves PR #3549 thread r-aA6u. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t(0) Pay fees by reducing output 0 instead of deducting from input 0. Simpler to reason about for test authors — recipients see (requested - fee_share), no input-side reservation needed. KNOWN BREAKAGE: the existing transfer_between_two_platform_addresses test asserts an exact recipient balance and will fail under the new default (recipient receives 10M - fee_share). Test fixture update is a follow-up. Also re-aligns import ordering in this file with `cargo fmt --all` defaults (a minor stray drift from the previous commit). Resolves PR #3549 thread r-aCky. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dex 0 Sweep-back target uses the bank's address-0 deterministically instead of advancing the unused-address pool every test run. Avoids accumulation of empty addresses on the bank wallet across test invocations. Implementation: derive the DIP-17 platform-payment address at index 0 directly from the bank seed (mirroring simple-signer's derivation logic), side-stepping the AddressPool's "next unused" cursor that would skip index 0 once it gets marked used. Resolves PR #3549 thread r-Jhi_. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…put-from-output Sweep-to-bank uses ReduceOutput(0) so the bank absorbs the fee from its incoming sum. Drops SWEEP_FEE_ESTIMATE constant and the multi-input fee headroom math. Sweep gate is now "if address balance > 0". Resolves PR #3549 thread r-ZluD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…noop stubs Split drain_to_bank into per-source helpers: sweep_platform_addresses (active), sweep_identities, sweep_core_addresses, sweep_unused_core_asset_locks, sweep_shielded (all noop with TODOs). teardown_one and sweep_orphans now walk every source type so future sweep implementations slot in cleanly. Resolves PR #3549 thread r-Zoq9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-platform-wallet-e2e
…tication_path
`AccountType::IdentityAuthenticationEcdsa { identity_index }` was
removed from `key-wallet` between revs `4c8bec3` and `ea33cbc8`.
Replaced with the new top-level `DerivationPath::identity_authentication_path(
network, KeyDerivationType::ECDSA, identity_index, key_index)` API
(`key-wallet/src/bip32.rs:1115`), which bakes both identity_index and
key_index into the path directly — `key_index` becomes the loop
variable instead of an external `extend([leaf])` step.
…ectly PR #3549 dedup re-audit (PROJ-001): the local DEFAULT_GAP_LIMIT = 20 shadows key_wallet's canonical pub const DIP17_GAP_LIMIT (rust-dashcore ea33cbc8: key-wallet/src/gap_limit.rs:26). Drop the local constant and import the upstream one — drift here would silently de-sync the harness from key-wallet's own gap policy.
…gh PlatformWallet PR #3549 dedup re-audit (PROJ-002): derive_platform_address_at_index was running BIP-32 manually from raw seed bytes. The bank already holds a PlatformWallet whose Wallet::derive_public_key (key-wallet/src/wallet/ helper.rs:763) does the same thing, so the parallel-derivation surface was redundant. Take the existing &Arc<PlatformWallet>, call .state().await.wallet().derive_public_key(&path), hash the result. Drops the bip39 seed-bytes consumer (the seed bytes are still derived once for SeedBackedPlatformAddressSigner::new four lines below). Net removes RootExtendedPrivKey, Secp256k1, PublicKey imports from bank.rs.
…r wrapper `framework/signer.rs` was a 78-line do-nothing shell around `SimpleSigner::from_seed_for_platform_address_account`: - the `Signer<PlatformAddress>` trait impl just delegated to inner; - `SimpleSigner` already implements that trait directly (`packages/simple-signer/src/signer.rs:338`); - `cached_key_count` and `new_with_gap` had zero callers outside the module; - the only added value was pinning `account=0`/`key_class=0`, which collapses to four lines of construction code. Replace with `framework::make_platform_signer(seed_bytes, network) -> SimpleSigner` next to the `FrameworkError`/`FrameworkResult` types in `mod.rs`. The three call sites (`bank.rs`, `wallet_factory.rs`, `cleanup.rs`) now hold `SimpleSigner` directly and pass it straight to `PlatformAddressWallet::transfer`. `TestWallet::address_signer()` returns `&SimpleSigner` for the same reason.
Contributor
|
@coderabbitai review all |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![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: 9
♻️ Duplicate comments (1)
packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs (1)
57-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sub-threshold wallets recoverable.
If
0 < total <= SWEEP_DUST_THRESHOLD, both cleanup paths skipsweep_platform_addressesand still delete the registry entry. That permanently abandons the remaining credits and will slowly drain the shared bank across repeated runs. Either sweep every positive balance withReduceOutput(0)or only remove the entry once the wallet is actually empty.Also applies to: 109-121, 145-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs` around lines 57 - 75, The cleanup currently deletes registry entries even when 0 < total <= SWEEP_DUST_THRESHOLD, abandoning recoverable credits; update the logic in the sweep_one match branches (the block that calls registry.remove and registry.set_status) to: if the wallet balance is > 0 but <= SWEEP_DUST_THRESHOLD, call sweep_platform_addresses with ReduceOutput(0) (or otherwise perform a full sweep for any positive balance) and only call registry.remove when the wallet is actually empty; ensure failed-path still sets EntryStatus::Failed when sweep fails and that successful-path only increments swept and removes the registry entry when the post-sweep balance is zero (reference symbols: sweep_one, sweep_platform_addresses, SWEEP_DUST_THRESHOLD, ReduceOutput(0), registry.remove, registry.set_status, EntryStatus::Failed).
🤖 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/tests/e2e/cases/transfer.rs`:
- Line 31: Rename the test function transfer_between_two_platform_addresses to
follow the convention by renaming it to
should_transfer_between_two_platform_addresses; update the async fn declaration
(and any internal references or usages of
transfer_between_two_platform_addresses) to the new name so the test name begins
with "should" while keeping the function body and attributes unchanged.
- Around line 51-79: This test performs real network calls via
s.ctx.bank().fund_address and s.test_wallet.transfer / wait_for_balance; change
it to comply with the "no network in unit/integration tests" rule by either (A)
moving this file/case to an e2e-only suite (so it runs under an e2e test runner)
or (B) refactoring to inject mocked implementations for the bank client and
wallet observer used by wait_for_balance and transfer (replace s.ctx.bank() and
any network-dependent wait_for_balance calls with test doubles that simulate
funding/transfer and observable balance updates); update references to
next_unused_address, transfer, and wait_for_balance to use the mocks or the
e2e-only harness accordingly.
In `@packages/rs-platform-wallet/tests/e2e/framework/config.rs`:
- Around line 34-50: Config currently derives Debug and will print sensitive
bank_mnemonic; replace the automatic derive with a manual impl Debug for Config
that omits or redacts bank_mnemonic (e.g., display "REDACTED" or hide its value)
and prints the other fields normally; implement Debug in the same module
referencing the struct name Config and its fields (bank_mnemonic, network,
dapi_addresses, min_bank_credits, workdir_base, trusted_context_url) so future
secret fields can also be redacted consistently.
In `@packages/rs-platform-wallet/tests/e2e/framework/registry.rs`:
- Around line 225-259: Rename the three test functions to follow the "should …"
naming convention: change missing_file_opens_empty to a descriptive name like
should_open_empty_if_file_missing, change insert_remove_round_trip_persists to
should_persist_insert_remove_round_trip, and change
corrupt_file_falls_back_to_empty to should_fall_back_to_empty_on_corrupt_file;
update the fn identifiers in
packages/rs-platform-wallet/tests/e2e/framework/registry.rs (the tests currently
named missing_file_opens_empty, insert_remove_round_trip_persists,
corrupt_file_falls_back_to_empty) and run cargo test to ensure no references
break.
In `@packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- Around line 291-293: Rename the test function
default_spec_matches_pinned_constants to follow the repository "should …"
convention (e.g., should_default_spec_match_pinned_constants or
should_match_pinned_constants_by_default) so the test name starts with "should";
update the function declaration fn default_spec_matches_pinned_constants() to
the new name and keep the body (including PlatformPaymentAccountSpec::default())
unchanged so references and assertions remain valid.
In `@packages/rs-platform-wallet/tests/e2e/framework/workdir.rs`:
- Line 92: Rename the test function
first_call_takes_slot_zero_second_falls_through to follow the required "should
..." convention (for example
should_first_call_take_slot_zero_and_second_fall_through); update the function
identifier wherever referenced (the test declaration itself and any uses in
attributes or calls) so the Rust test name begins with "should_" and keep the
original behavior and test annotation (e.g., #[test]) unchanged.
- Around line 50-61: The current error handling in the lock acquisition loop
treats every Err(err) as a busy slot; update the branch in the function that
opens/locks `lock_file` (the block that logs "workdir slot busy, trying next")
to inspect the IO error kind: if the error indicates contention (e.g.,
would-block / ErrorKind::WouldBlock or the platform-specific WouldBlock
equivalent), keep the existing tracing::debug and continue; for any other errors
(permission, other IO), log an error and propagate/return the error instead of
retrying so real failures aren’t swallowed.
In `@packages/rs-platform-wallet/tests/e2e/README.md`:
- Around line 99-106: The fenced code blocks in the e2e README (the blocks
starting with the "Bank wallet under-funded." message and the "SetupGuard
dropped without explicit teardown — wallet <id>" message) lack language tags,
causing MD040 lint failures; update those fenced blocks to include a language
specifier (e.g., change ``` to ```text) for both occurrences (the block
containing "Bank wallet under-funded." and the later block containing
"SetupGuard dropped without explicit teardown") so the markdown linter accepts
them.
- Around line 233-235: Update the stale troubleshooting example to match the
current error shape emitted by the pick_available_workdir routine: replace the
quoted `No available workdir slots (tried 0..10)` text with the actual error
text produced by pick_available_workdir (copy exact current message/format), and
note that this occurs when all 10 workdir slots are locked so operators search
logs for the correct string; reference pick_available_workdir in the note so
maintainers can locate the implementation for future changes.
---
Duplicate comments:
In `@packages/rs-platform-wallet/tests/e2e/framework/cleanup.rs`:
- Around line 57-75: The cleanup currently deletes registry entries even when 0
< total <= SWEEP_DUST_THRESHOLD, abandoning recoverable credits; update the
logic in the sweep_one match branches (the block that calls registry.remove and
registry.set_status) to: if the wallet balance is > 0 but <=
SWEEP_DUST_THRESHOLD, call sweep_platform_addresses with ReduceOutput(0) (or
otherwise perform a full sweep for any positive balance) and only call
registry.remove when the wallet is actually empty; ensure failed-path still sets
EntryStatus::Failed when sweep fails and that successful-path only increments
swept and removes the registry entry when the post-sweep balance is zero
(reference symbols: sweep_one, sweep_platform_addresses, SWEEP_DUST_THRESHOLD,
ReduceOutput(0), registry.remove, registry.set_status, EntryStatus::Failed).
🪄 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: 0379415c-b6af-4b82-b05c-635af13cb042
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/tests/.env.examplepackages/rs-platform-wallet/tests/e2e.rspackages/rs-platform-wallet/tests/e2e/README.mdpackages/rs-platform-wallet/tests/e2e/cases/mod.rspackages/rs-platform-wallet/tests/e2e/cases/transfer.rspackages/rs-platform-wallet/tests/e2e/framework/bank.rspackages/rs-platform-wallet/tests/e2e/framework/cleanup.rspackages/rs-platform-wallet/tests/e2e/framework/config.rspackages/rs-platform-wallet/tests/e2e/framework/context_provider.rspackages/rs-platform-wallet/tests/e2e/framework/harness.rspackages/rs-platform-wallet/tests/e2e/framework/mod.rspackages/rs-platform-wallet/tests/e2e/framework/registry.rspackages/rs-platform-wallet/tests/e2e/framework/sdk.rspackages/rs-platform-wallet/tests/e2e/framework/spv.rspackages/rs-platform-wallet/tests/e2e/framework/wait.rspackages/rs-platform-wallet/tests/e2e/framework/wait_hub.rspackages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rspackages/rs-platform-wallet/tests/e2e/framework/workdir.rspackages/rs-sdk/src/platform/transition.rspackages/rs-sdk/src/platform/transition/address_inputs.rspackages/simple-signer/Cargo.tomlpackages/simple-signer/src/signer.rs
… amplifier note Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
Picks up dashpay/rust-dashcore#756 which adds chainlock-driven transaction finalization in the wallet layer. Previously, `WalletInterface` had no `process_chain_lock` method and `dash-spv`'s `SyncEvent::ChainLockReceived` was emitted but never consumed, so wallet records were stuck at `TransactionContext:: InBlock(_)` forever even when the network produced a chainlock for the containing block. The new pin promotes records `InBlock → InChainLockedBlock` on chainlock arrival and emits a new `WalletEvent::TransactionsChainlocked` variant carrying the chainlock proof and per-account net-new finalized txids. For our `wait_for_proof` poll loop this means the chainlock branch (`record.context.is_chain_locked()`) actually flips when peers deliver the chainlock — the iter-4 IS→CL fallback path now resolves correctly instead of timing out at the secondary 180 s deadline. The new `WalletEvent` variant forces match-arm coverage in two sites: - packages/rs-platform-wallet/src/changeset/core_bridge.rs `build_core_changeset` returns `CoreChangeSet::default()` for the new variant. The wallet has already mutated the in-memory record by the time the event fires (upstream is "mutate-then- emit"), and the poll loop reads `record.context.is_chain_locked()` directly, so no additional persister projection is needed today. A future enhancement could persist `WalletMetadata:: last_applied_chain_lock` for crash recovery, but that's out of scope here. - packages/rs-platform-wallet/src/wallet/core/balance_handler.rs `BalanceUpdateHandler::on_wallet_event` returns early for the new variant. Chainlocks promote finality (`InBlock → InChainLockedBlock`) without changing UTXO state, so there's no balance update to deliver. Extracted from PR #3634 commit 4184a42 onto feat/rs-platform-wallet-e2e. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 546 (QA-901) TRACE re-investigation 2026-05-14 confirmed the earlier deterministic failure was a test-side dust-threshold mismatch (test assumed 2,730 duffs; upstream `transaction_builder.rs:294`, rev `5313086…`, uses 546). Headroom changed from 2,500 → 700; new change range [200, 474] is fully sub-dust across the observed [226, 500] testnet fee range, so the builder folds it into the fee and the BIP-32 account is truly drained. CR-004 reclassified red-by-design (dash-evo-tool#845) → passing-as-regression. The test now pins the symmetric BIP-32 spent-marking path (TransactionRouter → ManagedAccountCollection → check_transaction_for_match → update_utxos) plus the upstream sub-dust fold contract. The dash-evo-tool#845 reference is retained as a historical footnote — the symmetric spent-marking path was confirmed working; any remaining DET surface lives in dash-evo-tool's own UI refresh path, outside this suite's contract. TEST_SPEC.md updates: matrix row, detailed body (Layer 2 description + bug repro note), changelog entry, post-v47 status section, and counts annotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cross-link the newly-filed #3642 from the 5 hard-coded BIP-44 lookup sites in proof.rs + recovery.rs (TODO comments, no logic change) and from TEST_SPEC.md (matrix rows + detail front-matter + changelog entry). Found-012 and Found-023 unify on the same downstream-only fix: replace `info.core_wallet.accounts.standard_bip44_accounts.get(&account_index)` with iteration over `all_funding_accounts()` — no upstream change required; SPV-side tracking already covers all account types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(already fixed) Both pins describe contracts that are already satisfied in HEAD: - Found-019 (SeedBackedIdentitySigner ECDSA_HASH160 re-hash) — fix landed at packages/rs-platform-wallet/tests/e2e/framework/signer.rs:148-154 in commit 59cba08 (PR #3563). identity_key_lookup branches on key_type; ECDSA_HASH160 uses key.data() as-is, no re-hash. Production packages/simple-signer/src/signer.rs does NOT have the bug shape (different storage models). - Found-020 (PA-001b output_change_address spec/impl drift) resolved via spec realignment in PR #3609 (option a). PA-001b rewritten to match implicit-change semantics. The parameter doesn't exist in production and isn't planned. Knowledge preserved in memcan; spec clutter dropped. Total Found-bug pins 26 → 24. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
…routing proof CR-004 now pins the dash-evo-tool#845 contract directly: a BIP-32 send leaves an above-dust change UTXO, post-broadcast check_core_transaction routes it back onto the BIP-32 account (count == 1), and a follow-up spend consumes exactly that routed-back change and succeeds — proving the change was tracked back into BIP-32, not orphaned. Module doc states only the present contract (history narration dropped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e fee with symmetric pre-markers PA-003 measures the real chain-time fee (Σ gross outputs − Σ destination balance deltas, the canonical Σ inputs − Σ outputs under [ReduceOutput(0)]) for two self-transfers that draw inputs exclusively from a single source address (InputSelection::Explicit). Every measured destination — including the 1-output dest_1 — is pre-markered so both shapes hit address-funds UPDATE storage ops with no one-off CREATE skew; output count is the sole varied factor. Restored guards: strict fee_5 > fee_1, sub-linear fee_5 < fee_1*5, and the explicit FEE_DELTA_CEILING linear-fee-schedule tripwire. Funding defect fix: the explicit-input map value is the actual input amount the transition encodes (it must balance Σ outputs and be backed by the address balance), not a placeholder weight. inputs_1 now uses OUTPUT_AMOUNT and inputs_5 uses 5×OUTPUT_AMOUNT. addr_src funding bumped 500M → 700M to cover six MARKER_AMOUNT pre-markers (180M) plus both measured transfers (50M + 250M) with headroom, so addr_src always holds ≥ the explicit input amount when each transition is built. TEST_SPEC.md: PA-003 status flipped to green (table row + body) with a concise changelog line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…real eager-pool state Production's DIP-17 platform-payment pool is built by the eager AddressPool::new, which fills indices 0..=gap_limit-1 at construction (highest_generated = Some(gap_limit-1)); the QA-002 setup hook then marks index 0 used. From that real state the batch helper's fresh-past-highest_generated headroom is highest_used + 1 = 1, so the triplet's empty-pool full-window premise is unreachable in production. Rather than suppressing eager fill or changing the (correct) shared helper math at framework/gap_limit.rs, the test now models a real wallet that has cycled its first DIP-17 gap window: a test-scoped open_full_gap_window marks index gap_limit-1 used, shifting the ceiling up by gap_limit to open a genuine gap_limit-wide fresh window. The same DIP-17 boundary triad is pinned from the production starting state: - A: request gap_limit-1 fresh addresses, assert success + all distinct - B: request gap_limit (boundary), assert success + all distinct - C: request gap_limit+1, assert GapLimitError::Exceeded with every field pinned against the LIVE post-mark watermarks (requested, available=gap_limit, gap_limit, highest_used=Some(gap_limit-1), highest_generated=Some(gap_limit-1)), then a boundary retry proves the rejection did not mutate the pool Shared helper semantics are unchanged. TEST_SPEC.md PA-005b updated in all three places (quick-index, body Status, changelog) to the accurate rebaselined contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ead-back (QA-014)
PA-009 sub-case C's post-teardown observation re-derived the gone test
wallet and trusted its recent-zone sync watermark. A watermark-less
re-derived wallet's sync_balances(AddressSyncConfig{
full_rescan_after_time_s: 0 }) resolves to a recent-zone-only query
that returned 0 for addr_1 even though the sub-min_input dust was
correctly abandoned and never swept — a non-deterministic harness gap
(QA-014), not a production defect. The v53 14-thread run failed at the
re-derive read-back assertion (pa_009_min_input_amount.rs:290,
left: 0, right: 1000) while the cleanup-gate abandon-dust path itself
worked correctly.
Replace the re-derive read-back with a direct proof-verified on-chain
read of addr_1 via wait_for_address_balance_chain_confirmed — the same
AddressInfo::fetch gate the funding step already uses successfully
earlier in the same test. The post-condition now asserts addr_1 still
holds exactly TARGET_RESIDUAL on chain, reading real chain state
instead of a stale re-derived local view.
All three pinned invariants are preserved and strengthened:
(a) below-gate dust abandoned, no sweep transition broadcast (a swept
dust drops the balance to ~0, timing out the gate);
(b) gate == PlatformVersion::latest().dpp.state_transitions.address_funds
.min_input_amount and is positive (sub-cases A/B, untouched);
(c) addr_1 residual remains on chain at exactly TARGET_RESIDUAL.
#[ignore] and #[tokio_shared_rt::test(shared)] retained (network-gated,
the standard for all on-chain e2e cases; suite runs --include-ignored).
TEST_SPEC.md PA-009 references (quick-index, body Status/Scenario,
changelog) updated consistently; no stale QA-014/degenerate drift.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… Found-025 sync-discard race TK-001 and TK-014 failed in the v53 14-thread run, both timing out in the SETUP FUNDING gate before any token logic ran (tk_001_token_transfer.rs:67 setup_with_token_and_two_identities; tk_014_token_group_action.rs:109 setup_with_per_identity_funding). In both, bank.fund_address chain-confirmed the funding (nonce streak 2/2) before the wait, then the rs-sdk address-sync silently discarded the fetched balance update because the target address was not yet in pending_addresses — Found-025, amplified by 14-thread concurrency. Not production defects: transfer/group-action/co-sign code never executed and siblings (TK-001b/c, TK-009/010/012) were green in the same run. Root cause in the shared chokepoint framework/mod.rs::setup_with_per_identity_funding: it gated on wait_for_balance, whose proof-verified hand-off only runs AFTER the Found-025-poisoned local sync map (balances().get(addr)) first reaches target — so under Found-025 the proof gate was never reached and the budget expired in the local-view branch (60-62 polls, no chain-confirmed line). Fix: observe funding directly via the proof-verified AddressInfo::fetch path (wait_for_address_balance_chain_confirmed_n, CHAIN_CONFIRMED_CONSECUTIVE_SUCCESSES) — the same chain-state read the validator walks and the family PA-009c adopted — bypassing the poisoned map entirely. The existing strong wait_for_address_known_to_platform gate is unchanged. Only the funding-observation mechanism changed: no funding amounts, identity counts, contract publish, propose/co-sign, or token/identity assertions altered. Deterministic and concurrency-independent, so it hardens the whole setup-helper blast radius (all 22 TK-*/ID-*/CR-003/DPNS-001 cases routing through setup_with_per_identity_funding). No new Found-NNN pin and no upstream issue (Found-025 already owns the root cause). A TK-wave serialization / worker-pool cap remains a documented fallback only — not implemented, since the proof-verified read-back structurally bypasses the poisoned map. TEST_SPEC.md: TK-001 (quick-index + body) and TK-014 (quick-index + body) reclassified green -> red-real-fail mirroring TK-007 wording, cross-linked to Found-025; one changelog entry added. All three references per test are mutually consistent (no stale green/PASS-in-v47 drift). Live e2e requires a bank-funded node (yarn start) unavailable in this environment; verified by inspection + cargo build --tests + cargo clippy (both clean). Live re-validation deferred to the combined v54 run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ound-008 env-mask FIX 1 (#475): the e2e README documented the opt-in invocation with `--ignored`, which runs ONLY the `#[ignore]`-attributed subset (~40/108) and silently skips owned-fix cases. Corrected to `--include-ignored` so the full suite runs, with a one-line note explaining why `--ignored` alone is wrong. FIX 2 (#474, CLAUDE.md infra-blocker rule): added a `// TODO(env):` marker in al_001 near the Core-funded setup gate and a brief note in TEST_SPEC.md's AL-001/Found-008 entry recording that the Found-008 pin is env-masked when the e2e testnet Core L1 bank is depleted (al_001 dies at the setup gate, not at the designed FinalityTimeout; same depletion also fails cr_003 + id_002b). Funding address phrased generically — the specific address is being verified separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
…of-band) User will top up the e2e Core L1 bank wallet directly, so the env-mask documentation is unnecessary. Removes the FIX 2 content added in the previous commit: the `// TODO(env):` marker in al_001 near the Core-funded setup gate and the AL-001/Found-008 "pin coverage degraded under Core-bank depletion" bullet in TEST_SPEC.md. Net effect of this commit pair is README-only — the `--ignored` -> `--include-ignored` run-flag correction (#475). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
Claudius-Maginificent
changed the title
14 tasks
…ter store-error wallet loss) register_wallet logs and swallows the registration-round persister `store` error (manager/wallet_lifecycle.rs:276-282) then inserts the wallet into self.wallets unconditionally (wallet_lifecycle.rs:347-349). A successful-looking import leaves no persisted record and vanishes on the next launch — HIGH-severity silent data loss. Note the asymmetry: the load_persisted / initialize_from_persisted failure paths in the same function already roll back and return Err; the registration store does not. Deterministic pin (no live network, no concurrency): injects a StoreFailsPersister whose `store` returns Err while `load`/`flush` succeed (so the already-correct load_persisted rollback path does not mask the defect), drives create_wallet_from_seed_bytes through a mock-SDK manager, and asserts the correct atomic-failure contract — the call returns Err AND the wallet is absent from wallet_ids(). Fails today for the real reason; flips green once the registration store is treated as load-bearing. #[ignore]d — live run deferred. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
10 tasks
… for post-snapshot addresses (Found-025) `incremental_catch_up` built its `key_to_tag` lookup once from a single pre-RPC `provider.pending_addresses()` snapshot and passed it by immutable reference into both apply loops. The `if let Some(..) = address_lookup.get(..)` predicate had no `else`, so any balance change the platform returned for an address derived *after* the snapshot was dropped with no log, metric, or error — `result.found` never got it and `on_address_found` was never called. Under concurrent multi-identity funding the derive-fund-sync interleave is routine, which is why e2e gates TK-001/007/013/014 and id_005 flaked here. Extract the two inline apply loops into a pure `pub(crate) apply_address_changes` seam (no Sdk, no network, no async) that returns applied updates plus the addresses absent from the snapshot. The new `apply_block_changes` re-polls `pending_addresses()` when an unknown address appears (mirroring the tree-scan refresh) and replays only the previously-unknown subset, so a fresh receive address is recovered and known-address `AddToCredits` deltas are never double-counted. An address still unknown after the refresh is logged at `warn` — observable, never silently dropped. Known-address behavior is byte-for-byte identical. Adds three deterministic `#[cfg(test)]` regression guards on the pure seam (no proof/Sdk needed): unknown-address surfacing, post-snapshot recovery through the refresh, and delta double-count safety. All three fail on the pre-fix silent-discard logic and pass post-fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 35925f8) (cherry picked from commit 5c590341f65c89221e043ef89720fa948250e187)
…ex 0 `primary_core_receive_address` routed through `CoreWallet::next_receive_address_for_account(0)`, whose pool advances its "next unused" cursor off index 0 as soon as a UTXO lands there — so the operator-funded Core top-up address drifted run-to-run, stranding duffs on stale empty addresses. Mirror the existing `derive_platform_address_at_index` pattern with `derive_core_receive_address_at_index`: derive a deterministic `m/44'/coin'/0'/0/0` address via the live wallet's `derive_public_key`, reconstructing the P2PKH address exactly as key-wallet's own address pool does. The under-funded preflight now reports this same stable address; `CORE_TX_FEE_RESERVE` semantics and the under-funded arithmetic are unchanged. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> (cherry picked from commit 07f160c1d09eebfed320dbd0408195ee7bd5ea19)
…down with preflight The Core-bank auto-refill defaults were 3-4 orders of magnitude too low to keep the bank funded. A FULL e2e pass burns ~13 tDASH ≈ 1.3e9 duffs (1 DASH = 1e8 duffs); the old defaults were 100_000 / 1_000_000 duffs (0.001 / 0.01 DASH) — not even 0.1% of one pass. Re-anchor sizing on the measured per-pass burn (CORE_BURN_PER_FULL_PASS_DUFF = 1_300_000_000): - threshold: 2_000_000_000 duffs (~20 tDASH) — one full pass + ~0.5-pass margin so the bank is topped up before it can starve mid-pass. - target: 5_000_000_000 duffs (~50 tDASH ≈ 3.8 passes) — one slow Platform→Core withdrawal buys several passes of runway. Run the refill on BOTH ends of the lifecycle: - setup: existing call (harness.rs, unchanged) fires first. - teardown: new call in SetupGuard::teardown after the sweep returns this test's funds to the bank — the cheapest point to refill for the next pass. Below-threshold-guarded inside the helper, so it's a no-op when already funded; best-effort, never fails a teardown. Add a setup preflight (assert_core_funded_for_one_pass): after the refill attempt, if confirmed Core is still below one full pass (CORE_REFILL_OPERATIONAL_MIN_DUFF), fail fast with a FrameworkError::Bank naming the fixed index-0 top-up address and the exact shortfall — mirrors the existing send_core_to under-funded error shape — instead of silently entering a doomed run. Unit test pins the defaults to the measured burn (threshold ≥1 pass, target ≥3 passes, preflight floor == 1 pass). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> (cherry picked from commit 6662725c4c95a8866eb62014fc070ca23ca38310)
…nfirmed (Found-025) The funding-gate `wait_for_balance` in the identity/address state-transition tests checks the wallet's local sync map before handing off to the proof-verified chain gate. Under multi-thread churn the rs-sdk address-sync silently drops a fetched balance update when the address isn't yet in `pending_addresses` (Found-025), poisoning that local map so the precondition never reaches target and the proof-verified hand-off never runs — the gate times out before the immediately-following broadcast. Swap the 9 funding-then-broadcast gates (register_identity_from_addresses / top_up_from_addresses inputs) to `wait_for_address_balance_chain_confirmed_n` with `CHAIN_CONFIRMED_CONSECUTIVE_SUCCESSES`, mirroring the `setup_with_per_identity_funding` precedent exactly. Post-broadcast `wait_for_balance` sites whose assertion subject IS the wallet's local `.balances()` view (id_005 dest readback, all PA self-transfer tests) are left untouched — swapping them would un-sync the local map those assertions depend on. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> (cherry picked from commit bf4779e288ed2b22f52680b22543f57fc8690848)
…unmask QA-501/502 (id_005:127, id_002:117): Found-025 fix unmasked a downstream Found-026-family production cursor race — next_unused_address() returns a DUPLICATE under 14-thread churn. RED-by-design pins + TEST_SPEC reclass (green → red-by-design concurrency-only). No production fix; assertions stay genuinely RED for the real reason. Same root component + concurrency trigger as Found-026 (PA-008b); distinct observable mechanism (duplicate-derivation vs enqueue-miss) — linked to Found-026 family, no new Found-NNN (#496 holds filing; Found-026 still suspected). QA-503 (id_sweep:167): HARNESS test-defect, minimal fix. The secondary bank-identity post<=pre invariant is structurally unobservable under concurrent harness bank_rebalance core-refill (which by design tops up the bank identity; growth delta exactly matches topup_credits). Sweep correctness already pinned by the race-immune swept_identity_credits assertion — same flaw class QA-V39-001 fixed for the primary check. Removed the unobservable invariant (not green-paint: no real check weakened, no production source touched). QA-504 (pa_006b:83): Found-025-family known-fail under documented multi-thread conditions; un-swapped wait_for_balance reads the poisoned local map (#480 intentional non-swap). TEST_SPEC status corrected (green → red-real-fail multi-thread only). Flagged a swap-scope recommendation (swap ONLY the funding gate) — NOT applied (out of code scope). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> (cherry picked from commit e53d543a8f24e3ff3dbb917fd122f71322d52e67)
… chain-confirmed (QA-504) Corrects a #480 mis-scoping: pa_006b:81 is a funding PRECONDITION gate, not a post-broadcast `.balances()` assertion, so the #480 local-map rationale never applied. Swapped ONLY :81 to `wait_for_address_balance_chain_confirmed_n` (same pattern / arg-order / CHAIN_CONFIRMED_CONSECUTIVE_SUCCESSES as the #480 funding-gate swaps) — resolves the v-run's documented deterministic 14-thread funding-gate timeout. Post-broadcast `wait_for_balance(&addr_dst)` at :170 stays correctly un-swapped per #480. TEST_SPEC PA-006b reclassified `red-real-fail` → `partially-fixed (QA-504)`: the documented failure is fixed, but a clean multi-thread pass is NOT claimed — :170 retains residual Found-025-family exposure and no live re-run was possible (no bank-funded node). Honest, not green-washed. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> (cherry picked from commit 92881fde6cce24ccc47123ef7982e5b961682184)
lklimek
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
End-to-end test suite for
rs-platform-wallet, carrying the Found-025 rs-sdk fix plus a focused triage of e2e findings on top of the Found-017 red-by-design base. The Found-025 fix corrects a real production bug where address-sync silently discarded balance changes for post-snapshot addresses; the suite proves that fix authentic under a 14-thread e2e run and pins the remaining known upstream behaviors red-by-design.What was done?
6 commits atop the Found-017 base (
435b972fcc):91f1adaeea—fix(rs-sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025). This production fix is ALSO separately PR'd as fix(rs-sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025) #3650 againstv3.1-dev; it is carried here for combined e2e validation.d5fb7f5a8f—fix(rs-platform-wallet/e2e): pin Core bank top-up to a fixed BIP-44 index 0.1110bd0f4b—fix(rs-platform-wallet/e2e): Option C realistic Core self-refill on setup + teardown, with preflight.0376706cb5—test(rs-platform-wallet/e2e): swap 9 identity funding gates to chain-confirmed (Found-025).db00fbe910—test(rs-platform-wallet/e2e): triage of 4 v-run findings post-Found-025 unmask — RED-by-design pins, 1 harness invalid-check removal, plus spec corrections.e6bd370bc2—test(rs-platform-wallet/e2e): swap thepa_006b:81funding precondition to chain-confirmed (QA-504).How Has This Been Tested?
Validation status (stated precisely — not overclaimed):
A 14-thread e2e v-run (#501) proved the Found-025 fix AUTHENTIC (genuine proof-verified bypass, not flake-masking — 1845 upstream WARNs still emit; TK-001/007/013/014 genuine-green with deep assertions; CR-004/PA-003/PA-005b/PA-009c green). All 6 commits were independently Adams-verified GENUINE (byte-faithful, scope-clean, no green-paint, no injection). A FINAL confirmation v-run (post-triage delta: QA-503 invalid-invariant removal + pa_006b:81 swap) was still IN PROGRESS at the time of this update — this PR remains DRAFT pending that final confirmation.
RED-by-design pins are INTENTIONAL (the failure IS the proof), NOT failures to fix in this PR: Found-017, Found-021, Found-022, Found-006, Found-008, al_001, pa_3040, and QA-501/QA-502 (the Found-026-family
next_unused_addressduplicate-address race, UNMASKED by the now-working Found-025 fix — a pre-existing production concurrency bug, pinned not fixed, out of this PR's scope and tracked as backlog).Breaking Changes
None. The e2e suite and triage changes are test-only. The single production-code change (
91f1adaeea, Found-025 rs-sdk fix) is a correctness fix with no API-surface change and is separately PR'd as #3650 againstv3.1-dev.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent