feat(platform-wallet): e2e test spec and harness extensions

[lklimek]

Issue being fixed or feature implemented

Stacks on top of #3549 to deliver the next layer of the rs-platform-wallet e2e test suite: a written test specification, plus the harness extensions that subsequent test-implementation PRs will consume.

No test cases are added in this PR — the existing cases/transfer.rs is unchanged. The cases themselves come in follow-up PRs.

What was done?

Documentation

• tests/e2e/TEST_SPEC.md — 31 prioritized test cases across Platform Addresses (8), Identity (6), Tokens (4), Core/SPV (3), Contracts (3), DPNS (2), Dashpay (3), Contested (2). Each case has scenario steps, assertions, harness extensions required, DET parallels, complexity estimate.

Wallet public API additions (small, surgical)

• PlatformAddressChangeSet::fee_paid() — direct fee accessor populated in transfer() via AddressFundsTransferTransition::estimate_min_fee. Exact for the canonical DeductFromInput(_) strategy.
• PlatformAddressWallet::sync_watermark() -> Option<u64> — monotonic block watermark for sync assertions.
• IdentityManager::identity_count() / identity_ids() — convenience accessors.
• PlatformPaymentAddressProvider::last_known_recent_block getter — required plumbing for sync_watermark.

Harness — Wave A: identity signer + setup helpers

• SeedBackedIdentitySigner implementing Signer<IdentityPublicKey> (DIP-9 derivation).
• derive_identity_key(seed, network, identity_index, key_index, purpose, security_level) -> IdentityPublicKey — placeholder identity construction helper.
• TestWallet::register_identity_from_addresses(funding) -> RegisteredIdentity — end-to-end registration helper.
• wait_for_identity_balance and wait_for_dpns_name_visible in framework/wait.rs.

Harness — Wave B: multi-identity setup

• setup_with_n_identities(n) -> MultiIdentitySetupGuard — funds n distinct addresses on one test wallet and registers an identity from each. Wrapper around the existing SetupGuard (chosen over field extension because SetupGuard.teardown_called is pub(crate)).

Harness — Wave F: test-only utilities

• TestWallet::transfer_with_inputs (PA-002 negative variant — bypasses auto_select_inputs).
• TestWallet::transfer_capturing_st_bytes (PA-006 — returns serialized state-transition bytes alongside the changeset).
• PersistentTestWalletRegistry::get_status (PA-004 — single-entry status read).

Out of scope (per spec §4)

• Wave C (contract fixtures), Wave D (token contract config), Wave E (SPV re-enablement, blocked on Task fix(sdk): hash is not a function #15) — each tied to specific test-case PRs and gated on prerequisites.

How Has This Been Tested?

• cargo check -p platform-wallet --tests ✓
• cargo clippy -p platform-wallet --tests --all-features -- -D warnings ✓
• cargo fmt --all -- --check ✓
• No live e2e run — that requires PLATFORM_WALLET_E2E_BANK_MNEMONIC and a funded testnet wallet; documented in tests/e2e/README.md.

Breaking Changes

None. All additions are net-new public API or framework-only code.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (TEST_SPEC.md)
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works — N/A; this PR is harness infrastructure. Test cases land in follow-up PRs per TEST_SPEC.md.
• New and existing unit tests pass locally with my changes
• I have made corresponding changes to the documentation

---

Stacks on #3549. Merge order: #3549 → this PR → follow-up test-case PRs.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2ae2b77-bf1b-4f9b-a52f-688cd3763724

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/rs-platform-wallet-e2e-cases

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[thepastaclaw]

Review Gate

Commit: d5fdc70b

• ✅ Debounce: 57m ago (need 30m)

• ✅ CI checks: builds passed, 0/0 tests passed

• ✅ CodeRabbit review: comment found

• ⏳ Off-peak hours: peak window (5am-11am PT) — currently 06:00 AM PT Monday

• Run review now (check to override)

[Copilot]

Pull request overview

This PR extends rs-platform-wallet’s end-to-end (e2e) testing effort by adding a written test-case specification and expanding the e2e harness with identity-related tooling and a few small, test-driven wallet API additions to enable upcoming follow-up test PRs.

Changes:

• Added a comprehensive e2e test specification document (TEST_SPEC.md) enumerating prioritized test cases and required harness capabilities.
• Extended the e2e harness with an identity signer, identity registration helpers, new wait utilities, and multi-identity setup support.
• Introduced small wallet/public-API additions to support e2e assertions (transfer fee accessor plumbing via changesets, sync watermark getter, identity manager convenience accessor, and a provider watermark getter).

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs	Adds transfer helpers, identity registration helper, and a RegisteredIdentity bundle used by future e2e cases
packages/rs-platform-wallet/tests/e2e/framework/wait.rs	Adds polling waiters for identity balance visibility and DPNS name propagation
packages/rs-platform-wallet/tests/e2e/framework/signer.rs	New seed-backed Signer<IdentityPublicKey> and identity key derivation helper for harness usage
packages/rs-platform-wallet/tests/e2e/framework/registry.rs	Adds a single-entry status accessor for registry lifecycle assertions
packages/rs-platform-wallet/tests/e2e/framework/mod.rs	Adds setup_with_n_identities and guard type for multi-identity test setup
packages/rs-platform-wallet/tests/e2e/TEST_SPEC.md	New written e2e test plan/spec with prioritized cases and harness roadmap
packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs	Adds sync_watermark() accessor for incremental-sync assertions
packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs	Computes and attaches an estimated fee into PlatformAddressChangeSet on transfer
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs	Exposes provider watermark getter used by wallet-level sync_watermark()
packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs	Adds identity_ids() convenience accessor
packages/rs-platform-wallet/src/changeset/changeset.rs	Extends PlatformAddressChangeSet with a fee field and fee_paid() accessor; updates merge/is_empty logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thepastaclaw]

Code Review

Verified all findings against the worktree at HEAD 895fa93. Two blocking issues are real and dovetail: setup_with_n_identities() advertises safe teardown but sweep_identities() is a no-op so identity credits leak while the registry entry is removed, and the same setup helper returns a wallet whose platform-address cache is stale because register_from_addresses explicitly skips the cache update (confirmed by an in-source TODO). Suggestion-tier findings cluster around the new fee_paid() API: its doc promises strategy-adjusted accounting but the implementation just stores estimate_min_fee, which the same crate elsewhere flags (issue #3040) as a known under-estimate of real chain-time fees — and the harness's default strategy is exactly the case that under-estimates the most. Two comment-correctness issues round out the review.

Reviewed commit: 895fa93

🔴 2 blocking | 🟡 3 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/tests/e2e/framework/mod.rs`:
- [BLOCKING] lines 220-223: `setup_with_n_identities()` leaks identity credits because `sweep_identities()` is still a no-op
`MultiIdentitySetupGuard::teardown()` (mod.rs:236-238) forwards to `SetupGuard::teardown()`, whose docstring at mod.rs:220-223 advertises identity-side cleanup as implicit because "funds drain back to the bank during the wallet sweep". They don't: `cleanup.rs:170` calls `sweep_identities()` which is `Ok(())` (cleanup.rs:261-263, with a TODO blocking on a `Signer<IdentityPublicKey>` and CreditTransfer wiring). Meanwhile in the new flow, every funded address is consumed by `register_identity_from_addresses()`, so the credits sit on the identities, not on platform addresses, and the platform-address sweep at line 159 cannot recover them. Then `cleanup.rs:177` removes the registry breadcrumb, so the next startup sweep can no longer recover them either. Net effect: every test using this helper permanently burns `n * funding_per` bank credits while reporting a clean teardown. Fix: either implement `sweep_identities()` before merging this helper, or change the multi-identity guard's docstring + teardown to surface the leak (e.g. fail-loud teardown unless identity sweep lands), so tests don't silently drain the testnet bank.
- [BLOCKING] lines 193-215: `setup_with_n_identities()` returns a wallet with stale platform-address balances and nonces
After registration, `IdentityWallet::register_from_addresses()` explicitly does NOT push the returned address infos into the platform-address cache — see the TODO at `src/wallet/identity/network/register_from_addresses.rs:139-143` ("For now the next BLAST sync round refreshes them"). `setup_with_n_identities()` registers `n` identities sequentially and returns immediately, so the `TestWallet` it returns thinks each funding address still holds its pre-registration balance with the pre-registration nonce. Any consumer that calls `balances()`, `total_credits()`, or attempts an address-funded transfer before triggering a sync will read stale state and likely select already-spent inputs, producing spurious `InsufficientFunds` / nonce-mismatch failures from the very tests this helper is meant to enable. A setup helper should leave the wallet in a consistent post-setup state — invoke `sync_balances()` on each consumed address (or push the returned `address_infos` into the cache directly) before returning the guard.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 591-606: `fee_paid()` doc promises a `fee_strategy` adjustment the implementation doesn't perform — and underestimates real chain-time fees
The doc on `fee_paid()` claims the value is computed via `estimate_min_fee` "then adjusted by the configured `fee_strategy` so the returned number reflects the portion of the fee charged to the fee-bearing input's remaining balance." The producer at `transfer.rs:118-119` does no such adjustment — it stores the raw `estimate_min_fee(input_count, output_count, version)`. Worse, the in-tree comment at `transfer.rs:685-698` (cross-referencing platform issue #3040) acknowledges that `estimate_min_fee` returns only the static `state_transition_min_fees` floor, which the comment notes can be ~6.5M for 1in/1out while the real chain-time fee is ~14.94M. The harness's own `default_fee_strategy()` is `ReduceOutput(0)` (`wallet_factory.rs:399-401`) — exactly the case the bug report calls out. Any test that uses `cs.fee_paid()` to compute a post-transfer balance will be off by a meaningful and non-deterministic amount, producing flaky equality assertions on real testnets. Recommended fix: tighten the doc to say "lower-bound static estimate from `estimate_min_fee`, NOT the actual on-chain fee — see platform issue #3040" and consider renaming the accessor to `estimated_min_fee` (or returning `Option<Credits>`) until #3040 is resolved, so consumers cannot mistake the static floor for ground truth.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 113-119: Comment claims `DeductFromInput(_)` is the canonical strategy, but the crate's default is `ReduceOutput(0)`
The new comment immediately above the `fee_paid` computation says: "With `DeductFromInput(_)` (the canonical strategy used everywhere in this crate) the entire fee is charged to the targeted input's remaining balance, so this value matches the on-chain debit." This is wrong on two counts. (1) `default_fee_strategy()` in `tests/e2e/framework/wallet_factory.rs:399-401` is `vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]`, and the cleanup sweeps in `cleanup.rs:231` also use `ReduceOutput(0)` — there is no `DeductFromInput` use in this crate. (2) Under `ReduceOutput(0)`, the fee is taken from output 0's amount, not from any input's remaining balance, so the "matches the on-chain debit" claim doesn't hold for the strategy actually in use. The total `estimate_min_fee` value is the same regardless of strategy, so the field semantics are unaffected — but the justification is misleading and will confuse anyone trying to reconcile `fee_paid` against per-input deltas.
In `packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 237-245: `transfer_capturing_st_bytes` doc misattributes byte differences to nonce divergence
The doc says: "The captured bytes are taken from a sibling build (separate nonce fetch, separate signing pass) — they are NOT byte-equal to the broadcast transition, because the production path bumps nonces independently." The sibling build runs `fetch_inputs_with_nonce` + `nonce_inc` BEFORE `transfer_with_inputs` is called; `transfer_with_inputs` then does its own fetch of on-chain state and bumps the nonce identically. Because the sibling transition is never broadcast, on-chain state is unchanged between the two fetches, so both transitions end up with the *same* address nonces. The bytes diverge only because of ECDSA signature non-determinism (assuming no RFC 6979). This matters for PA-006: with identical nonces but different signatures the second submission is rejected by nonce-duplicate detection, not by content-hash duplicate detection. Future test authors writing assertions around this helper will be misled by the current explanation.

[thepastaclaw]

Code Review

Verified all findings against worktree HEAD 511d28c. Two blocking issues persist around setup_with_n_identities(): it forwards teardown to a no-op sweep_identities() (leaking identity credits while removing the registry breadcrumb), and it returns a wallet with stale platform-address cache because register_from_addresses deliberately skips the cache update. The new SeedBackedIdentitySigner ECDSA_HASH160 double-hash bug flagged by Codex is real but dormant on this PR's reachable paths and the author already pinned it as Found-019 in TEST_SPEC.md — kept as suggestion for visibility on the follow-up test wiring. Remaining items are doc/comment correctness on the new fee_paid API and a wait-helper timeout overshoot.

Reviewed commit: 511d28c

🔴 2 blocking | 🟡 4 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/tests/e2e/framework/mod.rs`:
- [BLOCKING] lines 220-238: `setup_with_n_identities()` leaks identity credits because `sweep_identities()` is still a no-op
`MultiIdentitySetupGuard::teardown()` (mod.rs:236-238) just forwards to `SetupGuard::teardown()`, whose docstring at mod.rs:220-223 advertises identity-side cleanup as implicit because "funds drain back to the bank during the wallet sweep". They don't: `cleanup.rs:170` calls `sweep_identities()` which is `Ok(())` (cleanup.rs:261-263, with a TODO blocking on `Signer<IdentityPublicKey>` and CreditTransfer wiring). In the new flow every funded address is consumed by `register_identity_from_addresses()`, so credits sit on identities, not on platform addresses — the platform-address sweep at line 159 cannot recover them. Then `cleanup.rs:177` removes the registry breadcrumb, so the next startup sweep also loses the only handle to the wallet. Net effect: every test using this helper permanently burns `n * funding_per` bank credits while reporting a clean teardown. Fix: implement `sweep_identities()` before merging this helper, or change the multi-identity guard's docstring + teardown to surface the leak (fail-loud teardown until identity sweep lands) so tests don't silently drain the testnet bank.
- [BLOCKING] lines 193-215: `setup_with_n_identities()` returns a wallet with stale platform-address balances and nonces
`IdentityWallet::register_from_addresses()` explicitly does NOT push the SDK-returned address infos into the platform-address cache — see the TODO at `src/wallet/identity/network/register_from_addresses.rs:139-143` ("For now the next BLAST sync round refreshes them"). `setup_with_n_identities()` registers `n` identities sequentially and returns immediately, so the `TestWallet` it returns thinks each funding address still holds its pre-registration balance with the pre-registration nonce. Any consumer that calls `balances()`, `total_credits()`, or attempts another address-funded transfer before triggering a sync will read stale state and likely select already-spent inputs, producing spurious `InsufficientFunds` / nonce-mismatch failures from the very tests this helper is meant to enable. A setup helper should leave the wallet in a consistent post-setup state — invoke `sync_balances()` on each consumed address (or push the returned `address_infos` into the cache directly) before returning the guard.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 591-606: `fee_paid()` doc promises a `fee_strategy` adjustment the implementation never performs — and underestimates real chain-time fees
The doc on `fee_paid()` claims the value is computed via `estimate_min_fee` "then adjusted by the configured `fee_strategy` so the returned number reflects the portion of the fee charged to the fee-bearing input's remaining balance." The producer at `transfer.rs:118-119` does no such adjustment — it stores the raw `estimate_min_fee(input_count, output_count, version)`. Worse, the in-tree comment at `transfer.rs:685-698` (cross-referencing platform issue #3040) acknowledges that `estimate_min_fee` returns only the static `state_transition_min_fees` floor; for 1in/1out that floor is ~6.5M while real chain-time fees are ~14.94M. The harness's own `default_fee_strategy()` is `ReduceOutput(0)` (`wallet_factory.rs:399-401`) — exactly the case the bug report calls out. Tests that use `cs.fee_paid()` to compute post-transfer balances will be off by a meaningful and non-deterministic amount, producing flaky equality assertions. Fix: tighten the doc to say "lower-bound static estimate from `estimate_min_fee`, NOT the actual on-chain fee — see platform issue #3040" and consider renaming the accessor to `estimated_min_fee` (or returning `Option<Credits>`) until #3040 is resolved.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 113-119: Comment claims `DeductFromInput(_)` is the canonical strategy, but the crate's default is `ReduceOutput(0)`
The new comment immediately above the `fee_paid` computation says: "With `DeductFromInput(_)` (the canonical strategy used everywhere in this crate) the entire fee is charged to the targeted input's remaining balance, so this value matches the on-chain debit." This is wrong on two counts. (1) `default_fee_strategy()` in `tests/e2e/framework/wallet_factory.rs:399-401` is `vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]`, and the cleanup sweeps in `cleanup.rs:231` also use `ReduceOutput(0)` — there is no `DeductFromInput` use in this crate. (2) Under `ReduceOutput(0)`, the fee is taken from output 0's amount, not from any input's remaining balance, so the "matches the on-chain debit" claim doesn't hold for the strategy actually in use. The total `estimate_min_fee` value is the same regardless of strategy, so field semantics are unaffected — but the justification will mislead anyone reconciling `fee_paid` against per-input deltas.
In `packages/rs-platform-wallet/tests/e2e/framework/signer.rs`:
- [SUGGESTION] lines 114-143: `SeedBackedIdentitySigner` double-hashes `ECDSA_HASH160` lookup keys
Both `can_sign_with()` (lines 114-122) and `lookup_identity_secret()` (lines 128-143) compute `ripemd160_sha256(key.data())` before probing `inner.address_private_keys`. That lookup is correct for `ECDSA_SECP256K1` because the cache is keyed by `hash160(pubkey)`. It is wrong for `ECDSA_HASH160` where `key.data()` is already the 20-byte hash — hashing it again yields `hash160(hash160(pubkey))`, which never matches the stored `hash160(pubkey)`. Result: any `ECDSA_HASH160` key reports `can_sign_with == false` and `sign()` fails with the misleading "not in pre-derived gap window" error even when the secret was pre-derived. Currently dormant — only `ECDSA_SECP256K1` keys are derived on reachable paths in this PR — and already documented as Found-019 in `tests/e2e/TEST_SPEC.md`. Worth fixing alongside the test pin so the test author isn't reasoning about a moving target. Fix: branch on `key.key_type()` so `ECDSA_HASH160` uses `key.data().as_slice()` directly as the lookup fingerprint.
In `packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 237-245: `transfer_capturing_st_bytes` doc misattributes byte differences to nonce divergence
The doc says: "The captured bytes are taken from a sibling build (separate nonce fetch, separate signing pass) — they are NOT byte-equal to the broadcast transition, because the production path bumps nonces independently." In practice the sibling build runs `fetch_inputs_with_nonce` + `nonce_inc` first against unchanged on-chain state; `transfer_with_inputs` then does its own fetch and bumps the nonce identically. Both transitions end up with the *same* address nonces — the bytes diverge only because of ECDSA signature non-determinism (no RFC 6979 in the signing path). This matters for PA-006: with identical nonces but different signatures, the second submission is rejected by nonce-duplicate detection, not by content-hash duplicate detection. The current explanation will mislead future test authors writing assertions around this helper, and risks landing a green PA-006 test that doesn't actually exercise pure-bytes-replay protection. Fix the doc to attribute the divergence to signature non-determinism, and either capture the bytes from the production submission (so re-broadcast shares both nonces and signature) or assert the rejection reason is duplicate-content rather than nonce-replay.

[thepastaclaw]

Code Review

No agent findings survived verification. Both candidate Codex findings flagged behaviors that are already explicitly documented and justified in the surrounding doc comments — captured-bytes semantics for PA-006 and deferred identity-credit cleanup with a tracked follow-up. No blocking issues remain.

Reviewed commit: 696ae3d

[thepastaclaw]

Code Review

PR is a clean test-harness extension with no blocking issues. The most material concern is the new PlatformAddressChangeSet::fee/estimated_min_fee surface, where the public field name, the accessor's per-transfer doc, and the merge's batch-summing semantics are mutually inconsistent and form a real footgun. Several smaller doc/API-shape suggestions in the e2e harness round out the review.

Reviewed commit: d18fd4d

🟡 8 suggestion(s) | 💬 2 nitpick(s)

1 additional finding

🟡 suggestion: `cs.fee` is unconditionally written even when the wallet-manager update branch falls through

packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs (lines 122-181)

fee_paid is written into cs.fee (line 137) before the wallet_manager.write().await block. If wm.get_wallet_info_mut(&self.wallet_id) returns None (line 140), the function falls through without populating cs.addresses — but the returned changeset is non-empty (fee != 0 ⇒ is_empty() is false) and carries no address entries. Persistence consumers that key on addresses will see a stray fee with no corresponding address mutation.

The on-chain transition has already broadcast in this case; silently dropping the address-side update is also wrong. Probably theoretical (the wallet_id is supplied by self, so the wallet is virtually always in the manager), but the fall-through case currently yields a malformed changeset rather than an error or a warn! log. Consider logging or returning an error if the manager doesn't know the wallet at this point.

🤖 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/changeset/changeset.rs`:
- [SUGGESTION] lines 585-644: `fee` field name, `estimated_min_fee()` accessor doc, and `merge()` semantics are mutually inconsistent
Three layers disagree on what this number means:
1. Field (line 589): `pub fee: Credits` — public, named `fee`, suggesting "the fee for this transfer".
2. Accessor doc (lines 593–614): explicitly warns this is a `state_transition_min_fees` lower-bound floor only — NOT the on-chain fee, NOT adjusted by `fee_strategy`, ~6.5M static vs ~14.94M observed for 1in/1out. Phrased per-transfer ("the transfer that produced this changeset", singular).
3. Merge (lines 639–644): `self.fee = self.fee.saturating_add(other.fee)` with a comment that the intent is "total fee paid across operations in this batch".
A caller reading only the field name reasonably writes a balance-delta assertion against `cs.fee`, expecting the actual debit. A caller reading the accessor doc reasonably treats `estimated_min_fee()` as a single-transfer estimate. Either of those callers, if a `merge()` runs in between, gets a value that means neither — it is the sum of static lower-bound estimates across N transfers in the batch.
Fix one of:
- Rename the field to match the accessor (`pub estimated_min_fee: Credits`) and rewrite the merge comment + accessor doc to consistently describe "sum of static lower-bound fee estimates across all transfers contributing to this changeset".
- Make the field `pub(crate)` so all reads go through the accessor, and clarify in the accessor doc that the value is per-batch (post-merge) rather than per-transfer.
In `packages/rs-platform-wallet/tests/e2e/framework/signer.rs`:
- [SUGGESTION] lines 176-212: `derive_identity_key` accepts `purpose`/`security_level` but always derives from the AUTHENTICATION sub-tree
The helper takes `purpose: Purpose` and `security_level: SecurityLevel` and stamps them onto the returned `IdentityPublicKeyV0` (lines 203–204), but the underlying derivation is unconditionally `derive_ecdsa_identity_auth_keypair_from_master(...)` — i.e. the DIP-9 AUTHENTICATION sub-tree at `m/9'/coin'/5'/0'/ECDSA'/identity'/key'`. Passing `Purpose::ENCRYPTION`, `Purpose::TRANSFER`, or `Purpose::VOTING` yields an `IdentityPublicKey` whose `purpose` field claims one thing while its bytes were derived from a different path entirely.
`SeedBackedIdentitySigner` only pre-derives AUTHENTICATION-path secrets via `SimpleSigner::from_seed_for_identity`, so signing with such a misclassified key happens to work — masking the inconsistency rather than surfacing it. As a `pub` test helper this is a real footgun for anyone writing future test cases that exercise non-AUTHENTICATION purposes.
Either (a) drop the `purpose`/`security_level` parameters, hardcode `Purpose::AUTHENTICATION`, and infer security level from `key_index` (matching the MASTER/HIGH convention used at the call site at wallet_factory.rs:324–339), or (b) reject (return `Err`) when called with a non-AUTHENTICATION purpose so the inconsistency fails loudly at construction.
In `packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 373-386: `funding / 2` is both too loose and silently coupled to a `funding ≥ 2 × register_fee` precondition
After `register_from_addresses`, the helper waits for the on-chain identity balance to reach only `funding / 2`. Two problems:
1. The actual post-fee balance is already returned in `registered.balance()` — this is proof-verified by the SDK call. Waiting for `funding / 2` instead of `registered.balance()` weakens the setup invariant: a fee regression or partial-credit bug that still leaves the identity above 50% of the requested funding would silently pass setup and surface as a confusing assertion failure later in the test body.
2. The threshold is only correct under the unstated precondition `funding ≥ 2 × register_fee`. For the canonical 30M test funding it's safe, but tests that pass smaller `funding` values (e.g. probing edge cases near `min_input_amount`) could see registration fees exceed `funding/2` and time out even on a normal registration.
Use `registered.balance()` directly as the wait target — it's already an exact, proven value — and the implicit precondition disappears.
- [SUGGESTION] lines 237-245: Doc rationale for `transfer_capturing_st_bytes` rejection mechanism is wrong
The doc claims captured bytes are NOT byte-equal to the broadcast transition because "the production path bumps nonces independently." That isn't what happens. Both code paths run sequentially with no broadcast in between (line 260: capture build calls `fetch_inputs_with_nonce` + `nonce_inc`; line 280: production `transfer_with_inputs` re-fetches nonces from chain). The capture build never broadcasts, so the on-chain nonce stays at N for both reads — both bump to N+1, both use the same inputs, outputs, fee strategy, and signer; ECDSA signing is deterministic via `core_signer`. The two transitions are very likely byte-equal, or at minimum share nonces.
PA-006's replay rejection still works, but for a different reason: the broadcast in step 2 advances the chain nonce to N+1, and the captured bytes (which use nonce N+1) are then rejected on replay because that nonce has already been consumed. Suggested replacement docstring: "Both builds use the same fetched + bumped nonce; the captured transition is rejected on replay because the production submission has already consumed that nonce on-chain."
- [SUGGESTION] lines 246-282: `transfer_capturing_st_bytes` hard-codes `default_fee_strategy()` for both builds
Both the captured sibling build (line 268) and the in-call `transfer_with_inputs` (line 280) use `default_fee_strategy()` (`[ReduceOutput(0)]`). This is fine for PA-006 as written, but anchors the helper to one strategy — replay-safety / serialization tests for `[DeductFromInput(0)]` would need a parallel helper. Threading a `fee_strategy: AddressFundsFeeStrategy` parameter through to both call sites (with a private `transfer_with_inputs_and_strategy` variant) would let the harness cover both shipped strategies symmetrically.
In `packages/rs-platform-wallet/tests/e2e/framework/mod.rs`:
- [SUGGESTION] lines 226-250: `setup_with_n_identities` amplifies the documented identity-credit leak by `n`
Teardown forwards to `SetupGuard::teardown` which calls `cleanup::teardown_one`, whose `sweep_identities` is currently a no-op (cleanup.rs:261–263, gated on the `#identity-sweep` TODO). The PR's own doc on `MultiIdentitySetupGuard` (lines 230–234) acknowledges this and points at the same TODO. So every identity created by `setup_with_n_identities` keeps its post-registration credit balance until the follow-up sweep lands — and `n`-identity setup multiplies the per-test leak by `n`.
This is not a regression introduced by this PR (the same leak applies to the existing single-identity helper), and the docstring is honest about it. The reason to flag is sizing: any test that uses `setup_with_n_identities(n, funding_per)` permanently consumes `n × (funding_per − register_fee)` credits from the bank wallet per run, scaling worse than the single-identity case. Either (a) prioritize the `#identity-sweep` follow-up before adopting this helper widely, or (b) gate the helper on a CI-only env var until the sweep lands so accidental local runs don't quietly drain the bank.
In `packages/rs-platform-wallet/tests/e2e/framework/wait.rs`:
- [SUGGESTION] lines 143-242: Wait helpers downgrade every SDK error to a generic timeout
Both `wait_for_identity_balance` (lines 143–188) and `wait_for_dpns_name_visible` (lines 200–242) treat every `Err` from the SDK as transient: log at `debug`, sleep, retry. Proof-verification failures, malformed queries, and other deterministic regressions get hidden behind the eventual timeout error and lose the concrete error type — the harness reports "timed out after 30s" rather than the specific verification or transport failure.
Retrying `Ok(None)` is correct (the identity / name simply isn't visible yet). Narrow transport-flake retries are also fine. But blanket-retrying every `Err` makes the harness much worse at distinguishing real failures from chain latency. Consider classifying errors and only retrying transport / not-found shapes; surface deterministic failures (proof verification, malformed request) immediately.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 122-181: `cs.fee` is unconditionally written even when the wallet-manager update branch falls through
`fee_paid` is written into `cs.fee` (line 137) before the `wallet_manager.write().await` block. If `wm.get_wallet_info_mut(&self.wallet_id)` returns `None` (line 140), the function falls through without populating `cs.addresses` — but the returned changeset is non-empty (`fee != 0` ⇒ `is_empty()` is false) and carries no address entries. Persistence consumers that key on `addresses` will see a stray fee with no corresponding address mutation.
The on-chain transition has already broadcast in this case; silently dropping the address-side update is also wrong. Probably theoretical (the `wallet_id` is supplied by `self`, so the wallet is virtually always in the manager), but the fall-through case currently yields a malformed changeset rather than an error or a `warn!` log. Consider logging or returning an error if the manager doesn't know the wallet at this point.

[thepastaclaw]

🟡 Suggestion: fee field name, estimated_min_fee() accessor doc, and merge() semantics are mutually inconsistent

Three layers disagree on what this number means:

1. Field (line 589): pub fee: Credits — public, named fee, suggesting "the fee for this transfer".
2. Accessor doc (lines 593–614): explicitly warns this is a state_transition_min_fees lower-bound floor only — NOT the on-chain fee, NOT adjusted by fee_strategy, ~6.5M static vs ~14.94M observed for 1in/1out. Phrased per-transfer ("the transfer that produced this changeset", singular).
3. Merge (lines 639–644): self.fee = self.fee.saturating_add(other.fee) with a comment that the intent is "total fee paid across operations in this batch".

A caller reading only the field name reasonably writes a balance-delta assertion against cs.fee, expecting the actual debit. A caller reading the accessor doc reasonably treats estimated_min_fee() as a single-transfer estimate. Either of those callers, if a merge() runs in between, gets a value that means neither — it is the sum of static lower-bound estimates across N transfers in the batch.

Fix one of:

• Rename the field to match the accessor (pub estimated_min_fee: Credits) and rewrite the merge comment + accessor doc to consistently describe "sum of static lower-bound fee estimates across all transfers contributing to this changeset".
• Make the field pub(crate) so all reads go through the accessor, and clarify in the accessor doc that the value is per-batch (post-merge) rather than per-transfer.

_{source: ['claude-rust-quality', 'codex-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/changeset/changeset.rs`:
- [SUGGESTION] lines 585-644: `fee` field name, `estimated_min_fee()` accessor doc, and `merge()` semantics are mutually inconsistent
Three layers disagree on what this number means:
1. Field (line 589): `pub fee: Credits` — public, named `fee`, suggesting "the fee for this transfer".
2. Accessor doc (lines 593–614): explicitly warns this is a `state_transition_min_fees` lower-bound floor only — NOT the on-chain fee, NOT adjusted by `fee_strategy`, ~6.5M static vs ~14.94M observed for 1in/1out. Phrased per-transfer ("the transfer that produced this changeset", singular).
3. Merge (lines 639–644): `self.fee = self.fee.saturating_add(other.fee)` with a comment that the intent is "total fee paid across operations in this batch".
A caller reading only the field name reasonably writes a balance-delta assertion against `cs.fee`, expecting the actual debit. A caller reading the accessor doc reasonably treats `estimated_min_fee()` as a single-transfer estimate. Either of those callers, if a `merge()` runs in between, gets a value that means neither — it is the sum of static lower-bound estimates across N transfers in the batch.
Fix one of:
- Rename the field to match the accessor (`pub estimated_min_fee: Credits`) and rewrite the merge comment + accessor doc to consistently describe "sum of static lower-bound fee estimates across all transfers contributing to this changeset".
- Make the field `pub(crate)` so all reads go through the accessor, and clarify in the accessor doc that the value is per-batch (post-merge) rather than per-transfer.

[thepastaclaw]

🟡 Suggestion: derive_identity_key accepts purpose/security_level but always derives from the AUTHENTICATION sub-tree

The helper takes purpose: Purpose and security_level: SecurityLevel and stamps them onto the returned IdentityPublicKeyV0 (lines 203–204), but the underlying derivation is unconditionally derive_ecdsa_identity_auth_keypair_from_master(...) — i.e. the DIP-9 AUTHENTICATION sub-tree at m/9'/coin'/5'/0'/ECDSA'/identity'/key'. Passing Purpose::ENCRYPTION, Purpose::TRANSFER, or Purpose::VOTING yields an IdentityPublicKey whose purpose field claims one thing while its bytes were derived from a different path entirely.

SeedBackedIdentitySigner only pre-derives AUTHENTICATION-path secrets via SimpleSigner::from_seed_for_identity, so signing with such a misclassified key happens to work — masking the inconsistency rather than surfacing it. As a pub test helper this is a real footgun for anyone writing future test cases that exercise non-AUTHENTICATION purposes.

Either (a) drop the purpose/security_level parameters, hardcode Purpose::AUTHENTICATION, and infer security level from key_index (matching the MASTER/HIGH convention used at the call site at wallet_factory.rs:324–339), or (b) reject (return Err) when called with a non-AUTHENTICATION purpose so the inconsistency fails loudly at construction.

_{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/tests/e2e/framework/signer.rs`:
- [SUGGESTION] lines 176-212: `derive_identity_key` accepts `purpose`/`security_level` but always derives from the AUTHENTICATION sub-tree
The helper takes `purpose: Purpose` and `security_level: SecurityLevel` and stamps them onto the returned `IdentityPublicKeyV0` (lines 203–204), but the underlying derivation is unconditionally `derive_ecdsa_identity_auth_keypair_from_master(...)` — i.e. the DIP-9 AUTHENTICATION sub-tree at `m/9'/coin'/5'/0'/ECDSA'/identity'/key'`. Passing `Purpose::ENCRYPTION`, `Purpose::TRANSFER`, or `Purpose::VOTING` yields an `IdentityPublicKey` whose `purpose` field claims one thing while its bytes were derived from a different path entirely.
`SeedBackedIdentitySigner` only pre-derives AUTHENTICATION-path secrets via `SimpleSigner::from_seed_for_identity`, so signing with such a misclassified key happens to work — masking the inconsistency rather than surfacing it. As a `pub` test helper this is a real footgun for anyone writing future test cases that exercise non-AUTHENTICATION purposes.
Either (a) drop the `purpose`/`security_level` parameters, hardcode `Purpose::AUTHENTICATION`, and infer security level from `key_index` (matching the MASTER/HIGH convention used at the call site at wallet_factory.rs:324–339), or (b) reject (return `Err`) when called with a non-AUTHENTICATION purpose so the inconsistency fails loudly at construction.

[thepastaclaw]

🟡 Suggestion: funding / 2 is both too loose and silently coupled to a funding ≥ 2 × register_fee precondition

After register_from_addresses, the helper waits for the on-chain identity balance to reach only funding / 2. Two problems:

1. The actual post-fee balance is already returned in registered.balance() — this is proof-verified by the SDK call. Waiting for funding / 2 instead of registered.balance() weakens the setup invariant: a fee regression or partial-credit bug that still leaves the identity above 50% of the requested funding would silently pass setup and surface as a confusing assertion failure later in the test body.
2. The threshold is only correct under the unstated precondition funding ≥ 2 × register_fee. For the canonical 30M test funding it's safe, but tests that pass smaller funding values (e.g. probing edge cases near min_input_amount) could see registration fees exceed funding/2 and time out even on a normal registration.

Use registered.balance() directly as the wait target — it's already an exact, proven value — and the implicit precondition disappears.

💡 Suggested change

Suggested change

	// The balance check uses a post-fee threshold of `funding /
	// 2` — registration fees on testnet are well below half the
	// funding amount, so this gives us a deterministic "the
	// identity exists and has been credited" assertion without
	// hard-coding a specific fee number that a protocol bump
	// could invalidate.
	wait_for_identity_balance(
	self.wallet.sdk(),
	registered.id(),
	funding / 2,
	DEFAULT_IDENTITY_VISIBILITY_TIMEOUT,
	)
	.await?;
	wait_for_identity_balance(
	self.wallet.sdk(),
	registered.id(),
	registered.balance(),
	DEFAULT_IDENTITY_VISIBILITY_TIMEOUT,
	)
	.await?;

_{source: ['claude-general', 'codex-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/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 373-386: `funding / 2` is both too loose and silently coupled to a `funding ≥ 2 × register_fee` precondition
After `register_from_addresses`, the helper waits for the on-chain identity balance to reach only `funding / 2`. Two problems:
1. The actual post-fee balance is already returned in `registered.balance()` — this is proof-verified by the SDK call. Waiting for `funding / 2` instead of `registered.balance()` weakens the setup invariant: a fee regression or partial-credit bug that still leaves the identity above 50% of the requested funding would silently pass setup and surface as a confusing assertion failure later in the test body.
2. The threshold is only correct under the unstated precondition `funding ≥ 2 × register_fee`. For the canonical 30M test funding it's safe, but tests that pass smaller `funding` values (e.g. probing edge cases near `min_input_amount`) could see registration fees exceed `funding/2` and time out even on a normal registration.
Use `registered.balance()` directly as the wait target — it's already an exact, proven value — and the implicit precondition disappears.

[thepastaclaw]

🟡 Suggestion: Doc rationale for transfer_capturing_st_bytes rejection mechanism is wrong

The doc claims captured bytes are NOT byte-equal to the broadcast transition because "the production path bumps nonces independently." That isn't what happens. Both code paths run sequentially with no broadcast in between (line 260: capture build calls fetch_inputs_with_nonce + nonce_inc; line 280: production transfer_with_inputs re-fetches nonces from chain). The capture build never broadcasts, so the on-chain nonce stays at N for both reads — both bump to N+1, both use the same inputs, outputs, fee strategy, and signer; ECDSA signing is deterministic via core_signer. The two transitions are very likely byte-equal, or at minimum share nonces.

PA-006's replay rejection still works, but for a different reason: the broadcast in step 2 advances the chain nonce to N+1, and the captured bytes (which use nonce N+1) are then rejected on replay because that nonce has already been consumed. Suggested replacement docstring: "Both builds use the same fetched + bumped nonce; the captured transition is rejected on replay because the production submission has already consumed that nonce on-chain."

_{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/tests/e2e/framework/wallet_factory.rs`:
- [SUGGESTION] lines 237-245: Doc rationale for `transfer_capturing_st_bytes` rejection mechanism is wrong
The doc claims captured bytes are NOT byte-equal to the broadcast transition because "the production path bumps nonces independently." That isn't what happens. Both code paths run sequentially with no broadcast in between (line 260: capture build calls `fetch_inputs_with_nonce` + `nonce_inc`; line 280: production `transfer_with_inputs` re-fetches nonces from chain). The capture build never broadcasts, so the on-chain nonce stays at N for both reads — both bump to N+1, both use the same inputs, outputs, fee strategy, and signer; ECDSA signing is deterministic via `core_signer`. The two transitions are very likely byte-equal, or at minimum share nonces.
PA-006's replay rejection still works, but for a different reason: the broadcast in step 2 advances the chain nonce to N+1, and the captured bytes (which use nonce N+1) are then rejected on replay because that nonce has already been consumed. Suggested replacement docstring: "Both builds use the same fetched + bumped nonce; the captured transition is rejected on replay because the production submission has already consumed that nonce on-chain."

[thepastaclaw]

🟡 Suggestion: setup_with_n_identities amplifies the documented identity-credit leak by n

Teardown forwards to SetupGuard::teardown which calls cleanup::teardown_one, whose sweep_identities is currently a no-op (cleanup.rs:261–263, gated on the #identity-sweep TODO). The PR's own doc on MultiIdentitySetupGuard (lines 230–234) acknowledges this and points at the same TODO. So every identity created by setup_with_n_identities keeps its post-registration credit balance until the follow-up sweep lands — and n-identity setup multiplies the per-test leak by n.

This is not a regression introduced by this PR (the same leak applies to the existing single-identity helper), and the docstring is honest about it. The reason to flag is sizing: any test that uses setup_with_n_identities(n, funding_per) permanently consumes n × (funding_per − register_fee) credits from the bank wallet per run, scaling worse than the single-identity case. Either (a) prioritize the #identity-sweep follow-up before adopting this helper widely, or (b) gate the helper on a CI-only env var until the sweep lands so accidental local runs don't quietly drain the bank.

_{source: ['codex-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/tests/e2e/framework/mod.rs`:
- [SUGGESTION] lines 226-250: `setup_with_n_identities` amplifies the documented identity-credit leak by `n`
Teardown forwards to `SetupGuard::teardown` which calls `cleanup::teardown_one`, whose `sweep_identities` is currently a no-op (cleanup.rs:261–263, gated on the `#identity-sweep` TODO). The PR's own doc on `MultiIdentitySetupGuard` (lines 230–234) acknowledges this and points at the same TODO. So every identity created by `setup_with_n_identities` keeps its post-registration credit balance until the follow-up sweep lands — and `n`-identity setup multiplies the per-test leak by `n`.
This is not a regression introduced by this PR (the same leak applies to the existing single-identity helper), and the docstring is honest about it. The reason to flag is sizing: any test that uses `setup_with_n_identities(n, funding_per)` permanently consumes `n × (funding_per − register_fee)` credits from the bank wallet per run, scaling worse than the single-identity case. Either (a) prioritize the `#identity-sweep` follow-up before adopting this helper widely, or (b) gate the helper on a CI-only env var until the sweep lands so accidental local runs don't quietly drain the bank.

[thepastaclaw]

🟡 Suggestion: Wait helpers downgrade every SDK error to a generic timeout

Both wait_for_identity_balance (lines 143–188) and wait_for_dpns_name_visible (lines 200–242) treat every Err from the SDK as transient: log at debug, sleep, retry. Proof-verification failures, malformed queries, and other deterministic regressions get hidden behind the eventual timeout error and lose the concrete error type — the harness reports "timed out after 30s" rather than the specific verification or transport failure.

Retrying Ok(None) is correct (the identity / name simply isn't visible yet). Narrow transport-flake retries are also fine. But blanket-retrying every Err makes the harness much worse at distinguishing real failures from chain latency. Consider classifying errors and only retrying transport / not-found shapes; surface deterministic failures (proof verification, malformed request) immediately.

_{source: ['codex-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/tests/e2e/framework/wait.rs`:
- [SUGGESTION] lines 143-242: Wait helpers downgrade every SDK error to a generic timeout
Both `wait_for_identity_balance` (lines 143–188) and `wait_for_dpns_name_visible` (lines 200–242) treat every `Err` from the SDK as transient: log at `debug`, sleep, retry. Proof-verification failures, malformed queries, and other deterministic regressions get hidden behind the eventual timeout error and lose the concrete error type — the harness reports "timed out after 30s" rather than the specific verification or transport failure.
Retrying `Ok(None)` is correct (the identity / name simply isn't visible yet). Narrow transport-flake retries are also fine. But blanket-retrying every `Err` makes the harness much worse at distinguishing real failures from chain latency. Consider classifying errors and only retrying transport / not-found shapes; surface deterministic failures (proof verification, malformed request) immediately.