feat(platform-wallet): e2e test spec and harness extensions#3563
feat(platform-wallet): e2e test spec and harness extensions#3563lklimek wants to merge 13 commits intofeat/rs-platform-wallet-e2efrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
force-pushed
the
feat/rs-platform-wallet-e2e-cases
branch
2 times, most recently
from
08bbd42 to
3e00df4
Compare
Spec document enumerating prioritized test cases for the e2e harness introduced in PR #3549. No implementation; pure documentation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dentity count - PlatformAddressChangeSet::fee_paid() — direct read of fee paid - PlatformAddressWallet::sync_watermark() — monotonic block watermark - IdentityManager::identity_count() / identity_ids() Supports e2e test assertions noted in TEST_SPEC.md §4 gaps 1-3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…m_addresses - SeedBackedIdentitySigner: Signer<IdentityPublicKey> impl using DIP-9 - derive_identity_key: top-level helper for placeholder identity construction - TestWallet::register_identity_from_addresses: end-to-end registration helper - wait_for_identity_balance / wait_for_dpns_name_visible Implements TEST_SPEC.md §4 Wave A. Unblocks ID-*, DPNS-*, DP-*, TK-*, CT-001. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Returns a guard with N freshly-registered identities on the same test wallet. Builds on register_identity_from_addresses (Wave A). Implements TEST_SPEC.md §4 Wave B. Unblocks ID-003, DP-002, DP-003. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TestWallet::transfer_with_inputs (PA-002 negative variant) - TestWallet::transfer_capturing_st_bytes (PA-006) - TestRegistry::get_status (PA-004) Implements TEST_SPEC.md §4 Wave F. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cret Pure rustfmt-driven line break-up after the Wave A SeedBackedIdentitySigner refactor that lands the composition wrapper over SimpleSigner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… in transfer_capturing_st_bytes
Replace the hand-rolled AddressInfo::fetch_many + manual nonce bump in
transfer_capturing_st_bytes with the now-public
dash_sdk::platform::transition::address_inputs::{fetch_inputs_with_nonce,
nonce_inc} helpers (promoted to pub in PR #3549's dedup base).
Closes the wallet-side half of PR #3549 dedup §3.2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r rebase Bilby B's r-aA6u rename in the parent PR (#3549) propagated the public constant to `DEFAULT_ACCOUNT_INDEX_PUB`. The cases-stack added a `transfer_with_inputs` call referencing the pre-rename name; rebasing onto the updated parent leaves it dangling. One-token rename to match.
lklimek
force-pushed
the
feat/rs-platform-wallet-e2e-cases
branch
from
3e00df4 to
491f65c
Compare
…-agent-a808ae1575a310154 # Conflicts: # packages/rs-platform-wallet/tests/e2e/framework/signer.rs # packages/rs-platform-wallet/tests/e2e/framework/wallet_factory.rs
…o feat/rs-platform-wallet-e2e-cases
lklimek
changed the title
|
✅ Review complete (commit 511d28c) |
There was a problem hiding this comment.
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.
| // Estimated fee = the same min-fee formula the protocol uses | ||
| // for validation. 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. |
There was a problem hiding this comment.
Valid finding — the comment overstates DeductFromInput(_)'s role. The harness defaults to [ReduceOutput(0)] (see framework/wallet_factory.rs::default_fee_strategy) and select_inputs_reduce_output is fully wired. Will reword on the next push.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// Returns `None` when the provider hasn't been initialised yet | ||
| /// (no [`Self::initialize`] call) or when no sync has produced a | ||
| /// watermark in this session. The value is monotonic non-decreasing | ||
| /// across [`Self::sync_balances`](super::sync) calls against the | ||
| /// same chain — a later sync can only advance the watermark, never | ||
| /// roll it back. A zero-valued watermark is reported as `None` to | ||
| /// match the "no stored watermark" convention used elsewhere in | ||
| /// the wallet (see [`Self::apply_sync_state`]). |
There was a problem hiding this comment.
Valid. The line "no sync has produced a watermark in this session" is inaccurate — apply_sync_state restoration also produces a watermark. Docstring already references apply_sync_state at the end but the leading sentence still says session-scoped. Will reword to "the provider's current stored watermark, whether restored from persistence or produced by a sync".
🤖 Co-authored by Claudius the Magnificent AI Agent
| match key.key_type() { | ||
| KeyType::ECDSA_SECP256K1 | KeyType::ECDSA_HASH160 => {} | ||
| other => { | ||
| return Err(ProtocolError::Generic(format!( | ||
| "SeedBackedIdentitySigner: unsupported key type {other:?}" | ||
| ))); | ||
| } | ||
| } | ||
| let secret = lookup_identity_secret(&self.inner, key)?; | ||
| let signature = core_signer::sign(data, &secret)?; | ||
| Ok(signature.to_vec().into()) | ||
| } | ||
|
|
||
| async fn sign_create_witness( | ||
| &self, | ||
| _key: &IdentityPublicKey, | ||
| _data: &[u8], | ||
| ) -> Result<AddressWitness, ProtocolError> { | ||
| // Identity-key signers never produce platform-address witnesses — | ||
| // the DPP signer trait forces both methods on a single impl. | ||
| Err(ProtocolError::Generic( | ||
| "SeedBackedIdentitySigner: AddressWitness is not produced by an identity signer".into(), | ||
| )) | ||
| } | ||
|
|
||
| fn can_sign_with(&self, key: &IdentityPublicKey) -> bool { | ||
| match key.key_type() { | ||
| KeyType::ECDSA_SECP256K1 | KeyType::ECDSA_HASH160 => { | ||
| let pkh = ripemd160_sha256(key.data().as_slice()); | ||
| self.inner.address_private_keys.contains_key(&pkh) | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Resolve an [`IdentityPublicKey`] to its pre-derived 32-byte secret, | ||
| /// or surface a [`ProtocolError`] naming the missing fingerprint. | ||
| #[allow(clippy::result_large_err)] | ||
| fn lookup_identity_secret( | ||
| inner: &SimpleSigner, | ||
| key: &IdentityPublicKey, | ||
| ) -> Result<[u8; 32], ProtocolError> { | ||
| let pkh = ripemd160_sha256(key.data().as_slice()); | ||
| inner | ||
| .address_private_keys | ||
| .get(&pkh) | ||
| .copied() | ||
| .ok_or_else(|| { | ||
| ProtocolError::Generic(format!( | ||
| "SeedBackedIdentitySigner: identity key {} not in pre-derived gap window", | ||
| hex::encode(pkh) | ||
| )) | ||
| }) |
There was a problem hiding this comment.
Confirmed real, not currently triggered. Marvin's audit (commit 511d28cb90) verified the failure path: SeedBackedIdentitySigner::lookup_identity_secret and can_sign_with unconditionally compute ripemd160_sha256(key.data()) for the cache probe, while SimpleSigner::from_seed_for_identity populates the cache keyed by ripemd160_sha256(raw_pubkey). For KeyType::ECDSA_HASH160, key.data() is already a 20-byte hash → second hash → cache miss every time.
Pinned as Found-019 (HIGH severity, dormant) in tests/e2e/TEST_SPEC.md § Found-bug pins, with the proof-shape test scenario. The bug doesn't currently trigger because derive_identity_key hard-codes KeyType::ECDSA_SECP256K1, but the type contract is broken for any caller that hands an ECDSA_HASH160-typed key to this signer. Fix is a separate PR — either special-case ECDSA_HASH160 to use the 20-byte data directly as the lookup key, or drop ECDSA_HASH160 from the supported set.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// 1. Picks a fresh receive address and verifies it has at | ||
| /// least `funding` credits (the caller is responsible for | ||
| /// funding that address — typically via | ||
| /// `bank.fund_address` + [`super::wait::wait_for_balance`] | ||
| /// before this call). | ||
| /// 2. Derives MASTER + HIGH ECDSA auth keys at DIP-9 slot | ||
| /// `(identity_index, 0)` and `(identity_index, 1)`. | ||
| /// 3. Builds a placeholder [`Identity`] populated with those | ||
| /// two keys. | ||
| /// 4. Calls | ||
| /// [`IdentityWallet::register_from_addresses`](platform_wallet::wallet::identity::IdentityWallet::register_from_addresses) | ||
| /// with the funding map `{addr_1 → funding}`. |
There was a problem hiding this comment.
Valid — the docstring needs to match the current implementation, which (a) takes funding_address as a caller-provided argument, and (b) does not pre-check its balance before register_from_addresses. Will reword the docstring to remove the "picks a fresh receive address" and "verifies it has at least funding credits" claims; balance verification is the caller's responsibility (or a follow-up if a fail-fast variant is wanted).
🤖 Co-authored by Claudius the Magnificent AI Agent
| #[derive(Debug, Clone, Default, PartialEq)] | ||
| pub struct PlatformAddressChangeSet { | ||
| /// Updated platform addresses produced by the last sync pass. | ||
| /// A `Vec` rather than a map because the diff already deduplicates | ||
| /// per `(wallet, account, address)` within one changeset, and | ||
| /// consumers either apply each entry independently or merge | ||
| /// append-only. | ||
| pub addresses: Vec<PlatformAddressBalanceEntry>, | ||
| /// Highest block height covered by the last sync, across all | ||
| /// accounts. `None` means "no change". | ||
| pub sync_height: Option<u64>, | ||
| /// Latest timestamp covered by the last sync, across all accounts. | ||
| /// `None` means "no change". | ||
| pub sync_timestamp: Option<u64>, | ||
| /// Last block height with recent address changes (compaction marker). | ||
| /// `None` means "no change". | ||
| pub last_known_recent_block: Option<u64>, | ||
| /// Fee paid by the transfer that produced this changeset, in | ||
| /// credits. `0` for changesets not produced by `transfer()` | ||
| /// (e.g. sync-only changesets). See [`Self::fee_paid`]. | ||
| pub fee: Credits, | ||
| } |
There was a problem hiding this comment.
Acknowledged. PlatformAddressChangeSet is pub; adding pub fee is a semver-breaking change for any external code that constructs it via struct literal or destructure. Two options: (a) mark the struct #[non_exhaustive] going forward (one-time breaking change but immunises future additions), (b) wrap fee behind a private field with a fee_paid() accessor (still requires struct-literal callers to update if they exist). The PR should note the breaking change in its body. Will surface to the milestone owner.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// Fee paid by the transfer that produced this changeset, in | ||
| /// credits. `0` for changesets not produced by `transfer()` | ||
| /// (e.g. sync-only changesets). See [`Self::fee_paid`]. | ||
| pub fee: Credits, | ||
| } | ||
|
|
||
| impl PlatformAddressChangeSet { | ||
| /// Fee paid by the transfer that produced this changeset, in | ||
| /// credits. | ||
| /// | ||
| /// Returns `0` for changesets that didn't originate from a | ||
| /// `transfer()` call — e.g. sync-only changesets, or changesets | ||
| /// constructed via `Default::default()`. The value is computed by | ||
| /// `transfer()` from the transition's input/output counts via | ||
| /// `AddressFundsTransferTransition::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. |
There was a problem hiding this comment.
Confirmed real — pinned as Found-018 in tests/e2e/TEST_SPEC.md § Found-bug pins (LOW severity, doc-vs-impl divergence). The docstring claims "adjusted by the configured fee_strategy" but transfer() stores AddressFundsTransferTransition::estimate_min_fee(input_count, output_count, version) unconditionally. Two consistent ways to resolve (per the Found-018 entry): either update the docs to describe what's actually stored (total estimated min fee), or change the implementation to apply the fee-strategy adjustment. Both are tracked; fix lands in a separate PR.
🤖 Co-authored by Claudius the Magnificent AI Agent
| // Fee: last-non-zero-wins. Sync-only merges (which carry | ||
| // `fee == 0`) preserve a transfer's recorded fee; merging | ||
| // two transfer changesets sums the fees, matching the | ||
| // "total fee paid across operations in this batch" intent. |
There was a problem hiding this comment.
Confirmed — pinned as Found-018 in tests/e2e/TEST_SPEC.md § Found-bug pins. The comment is internally inconsistent (it says "last-non-zero-wins" then immediately says "merging two transfer changesets sums the fees"), and the implementation is saturating_add (sums). Aligns with this finding's recommendation: pick one of (a) accumulate + rename field to total_fee, or (b) preserve last-non-zero + refuse-on-merge for two non-zero fees. Either way the docstring needs to match. Tracked; fix in a separate PR.
🤖 Co-authored by Claudius the Magnificent AI Agent
…e and priority tags Adds 27 test cases from Marvin's edge-case audit (boundary, empty-input, concurrency, malformed-on-disk, async-cancellation, network-failure, large-input, unicode). Introduces P0/P1/P2 priority tags; primary happy-path cases (P0) are listed first within each section, P1 core variants next, P2 edge cases last. Renumbers within each prefix for density. See /tmp/test-spec-edge-case-audit.md for the per-finding rationale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Walks `packages/rs-platform-wallet/src/` looking for contract violations, boundary mishandles, concurrency races, error swallowing, trust mismatches analogous to platform #3040, and panic-vs-typed-error paths. Each suspected bug becomes a Found-NNN test case in TEST_SPEC.md §3, P2 priority, with the proof-shape assertion and the expected-vs-actual contract. Doc only. No code changes; no renumbering of existing cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r double-hashes ECDSA_HASH160 `SeedBackedIdentitySigner::lookup_identity_secret` and `can_sign_with` unconditionally compute `ripemd160_sha256(key.data())` for the cache probe. The cache itself is populated by `SimpleSigner::from_seed_for_identity` keyed by `ripemd160_sha256(serialize(secp256k1_pubkey))`. For `KeyType::ECDSA_SECP256K1` keys this matches (raw 33-byte pubkey hashed once). For `KeyType::ECDSA_HASH160` keys — whose `data` is already the 20-byte `ripemd160_sha256(pubkey)` per the DPP `KeyType` contract — the lookup hashes the already-hashed value, producing `ripemd160_sha256(ripemd160_sha256(pubkey))` and missing every cached secret. Both match arms at signer.rs:90 and signer.rs:116 admit `ECDSA_HASH160`, so the type signature lies: the impl claims support for a key type it can never resolve. The bug is currently dormant because `derive_identity_key` hard-codes `ECDSA_SECP256K1`, but any caller that constructs an `ECDSA_HASH160`-typed `IdentityPublicKey` (e.g. a chain identity registered by another wallet) will fail every sign call with a `not in pre-derived gap window` error on a key that demonstrably IS in the gap window. Documents the contract and the proof-shape unit test under § Found-bug pins. Spec-only — no `.rs` changes, no fix in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| /// Calling [`MultiIdentitySetupGuard::teardown`] consumes the | ||
| /// guard and forwards to the inner [`SetupGuard::teardown`] — | ||
| /// identity-side cleanup is implicit (their funds drain back to | ||
| /// the bank during the wallet sweep). |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex-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/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.
| for identity_index in 0..n { | ||
| let funding_addr = base.test_wallet.next_unused_address().await?; | ||
| base.ctx | ||
| .bank() | ||
| .fund_address(&funding_addr, funding_per) | ||
| .await?; | ||
| wait_for_balance( | ||
| &base.test_wallet, | ||
| &funding_addr, | ||
| funding_per, | ||
| Duration::from_secs(60), | ||
| ) | ||
| .await?; | ||
|
|
||
| let registered = base | ||
| .test_wallet | ||
| .register_identity_from_addresses(funding_addr, funding_per, identity_index) | ||
| .await?; | ||
| identities.push(registered); | ||
| } | ||
|
|
||
| Ok(MultiIdentitySetupGuard { base, identities }) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
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`:
- [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.
| impl PlatformAddressChangeSet { | ||
| /// Fee paid by the transfer that produced this changeset, in | ||
| /// credits. | ||
| /// | ||
| /// Returns `0` for changesets that didn't originate from a | ||
| /// `transfer()` call — e.g. sync-only changesets, or changesets | ||
| /// constructed via `Default::default()`. The value is computed by | ||
| /// `transfer()` from the transition's input/output counts via | ||
| /// `AddressFundsTransferTransition::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. | ||
| pub fn fee_paid(&self) -> Credits { | ||
| self.fee | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude-general', 'claude-rust-quality', '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/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.
| // Estimated fee = the same min-fee formula the protocol uses | ||
| // for validation. 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. | ||
| let fee_paid = | ||
| AddressFundsTransferTransition::estimate_min_fee(input_count, output_count, version); |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 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.
| /// Used by replay-safety tests (PA-006): re-submit the captured | ||
| /// bytes via `sdk.broadcast_state_transition` and assert the | ||
| /// platform rejects the duplicate. 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. For | ||
| /// PA-006 that's the right shape: the test confirms the second | ||
| /// submission is rejected regardless of the nonce relationship | ||
| /// between the two builds. |
There was a problem hiding this comment.
🟡 Suggestion: 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.
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: `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.
| // Fee: last-non-zero-wins. Sync-only merges (which carry | ||
| // `fee == 0`) preserve a transfer's recorded fee; merging | ||
| // two transfer changesets sums the fees, matching the | ||
| // "total fee paid across operations in this batch" intent. | ||
| self.fee = self.fee.saturating_add(other.fee); |
There was a problem hiding this comment.
💬 Nitpick: Merge fee comment opens with "last-non-zero-wins" but the code sums
The comment leads with // Fee: last-non-zero-wins. and then in the same block describes "merging two transfer changesets sums the fees" — directly contradicting itself. The code is self.fee = self.fee.saturating_add(other.fee), i.e. sum semantics; a true last-non-zero-wins merge of fee=100 then fee=50 would yield 50, while saturating_add yields 150. Drop the misleading lead clause so the policy matches the code.
💡 Suggested change
| // Fee: last-non-zero-wins. Sync-only merges (which carry | |
| // `fee == 0`) preserve a transfer's recorded fee; merging | |
| // two transfer changesets sums the fees, matching the | |
| // "total fee paid across operations in this batch" intent. | |
| self.fee = self.fee.saturating_add(other.fee); | |
| // Fee: append-sum across merged changesets. Sync-only merges | |
| // (which carry `fee == 0`) preserve a transfer's recorded | |
| // fee; merging two transfer changesets sums the fees, | |
| // matching the "total fee paid across operations in this | |
| // batch" intent. | |
| self.fee = self.fee.saturating_add(other.fee); |
source: ['claude-general', 'claude-rust-quality']
| loop { | ||
| match Identity::fetch(sdk, identity_id).await { | ||
| Ok(Some(identity)) => { | ||
| let balance = identity.balance(); | ||
| if balance >= expected { | ||
| tracing::info!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| observed = balance, | ||
| expected, | ||
| elapsed = ?start.elapsed(), | ||
| "identity balance reached target" | ||
| ); | ||
| return Ok(balance); | ||
| } | ||
| tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| current = balance, | ||
| expected, | ||
| "identity balance below target" | ||
| ); | ||
| } | ||
| Ok(None) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| "identity not yet visible on chain" | ||
| ), | ||
| Err(err) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| error = %err, | ||
| "fetch::<Identity> failed during wait_for_identity_balance" | ||
| ), | ||
| } | ||
|
|
||
| if Instant::now() >= deadline { | ||
| return Err(FrameworkError::Cleanup(format!( | ||
| "wait_for_identity_balance timed out after {timeout:?} \ | ||
| (identity_id={identity_id:?} expected={expected})" | ||
| ))); | ||
| } | ||
| tokio::time::sleep(BACKSTOP_WAKE_INTERVAL).await; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: wait_for_identity_balance / wait_for_dpns_name_visible can overshoot the supplied timeout by up to BACKSTOP_WAKE_INTERVAL
Both new helpers do match fetch ... ; if Instant::now() >= deadline { err } ; sleep(BACKSTOP_WAKE_INTERVAL=2s) ; loop. If the caller passes a timeout shorter than 2s, the unconditional 2s sleep can run after the deadline check, so the helpers can return a timeout error nearly 2 seconds late — and the next iteration's fetch may succeed against a chain state the test no longer cares about. Cap the sleep against deadline.saturating_duration_since(Instant::now()) (the same pattern wait_for_balance already uses with cap = std::cmp::min(remaining, BACKSTOP_WAKE_INTERVAL)) so the overshoot is bounded by one fetch RTT instead of a fixed 2s. Low-impact today (callers in the harness all pass ≥30s), but cheap to get right while the helpers are new.
source: ['claude-rust-quality']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| /// Calling [`MultiIdentitySetupGuard::teardown`] consumes the | ||
| /// guard and forwards to the inner [`SetupGuard::teardown`] — | ||
| /// identity-side cleanup is implicit (their funds drain back to | ||
| /// the bank during the wallet sweep). | ||
| pub struct MultiIdentitySetupGuard { | ||
| /// Inner single-wallet guard. Holds the [`E2eContext`] and the | ||
| /// shared [`wallet_factory::TestWallet`] every identity is | ||
| /// derived from. | ||
| pub base: SetupGuard, | ||
| /// Identities registered during setup, ordered by DIP-9 index | ||
| /// `0..n`. | ||
| pub identities: Vec<wallet_factory::RegisteredIdentity>, | ||
| } | ||
|
|
||
| impl MultiIdentitySetupGuard { | ||
| /// Forward to the inner [`SetupGuard::teardown`]. | ||
| pub async fn teardown(self) -> FrameworkResult<()> { | ||
| self.base.teardown().await | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/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.
| for identity_index in 0..n { | ||
| let funding_addr = base.test_wallet.next_unused_address().await?; | ||
| base.ctx | ||
| .bank() | ||
| .fund_address(&funding_addr, funding_per) | ||
| .await?; | ||
| wait_for_balance( | ||
| &base.test_wallet, | ||
| &funding_addr, | ||
| funding_per, | ||
| Duration::from_secs(60), | ||
| ) | ||
| .await?; | ||
|
|
||
| let registered = base | ||
| .test_wallet | ||
| .register_identity_from_addresses(funding_addr, funding_per, identity_index) | ||
| .await?; | ||
| identities.push(registered); | ||
| } | ||
|
|
||
| Ok(MultiIdentitySetupGuard { base, identities }) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/e2e/framework/mod.rs`:
- [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.
| impl PlatformAddressChangeSet { | ||
| /// Fee paid by the transfer that produced this changeset, in | ||
| /// credits. | ||
| /// | ||
| /// Returns `0` for changesets that didn't originate from a | ||
| /// `transfer()` call — e.g. sync-only changesets, or changesets | ||
| /// constructed via `Default::default()`. The value is computed by | ||
| /// `transfer()` from the transition's input/output counts via | ||
| /// `AddressFundsTransferTransition::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. | ||
| pub fn fee_paid(&self) -> Credits { | ||
| self.fee | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/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.
| // Estimated fee = the same min-fee formula the protocol uses | ||
| // for validation. 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. | ||
| let fee_paid = | ||
| AddressFundsTransferTransition::estimate_min_fee(input_count, output_count, version); |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`:
- [SUGGESTION] lines 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.
| fn can_sign_with(&self, key: &IdentityPublicKey) -> bool { | ||
| match key.key_type() { | ||
| KeyType::ECDSA_SECP256K1 | KeyType::ECDSA_HASH160 => { | ||
| let pkh = ripemd160_sha256(key.data().as_slice()); | ||
| self.inner.address_private_keys.contains_key(&pkh) | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Resolve an [`IdentityPublicKey`] to its pre-derived 32-byte secret, | ||
| /// or surface a [`ProtocolError`] naming the missing fingerprint. | ||
| #[allow(clippy::result_large_err)] | ||
| fn lookup_identity_secret( | ||
| inner: &SimpleSigner, | ||
| key: &IdentityPublicKey, | ||
| ) -> Result<[u8; 32], ProtocolError> { | ||
| let pkh = ripemd160_sha256(key.data().as_slice()); | ||
| inner | ||
| .address_private_keys | ||
| .get(&pkh) | ||
| .copied() | ||
| .ok_or_else(|| { | ||
| ProtocolError::Generic(format!( | ||
| "SeedBackedIdentitySigner: identity key {} not in pre-derived gap window", | ||
| hex::encode(pkh) | ||
| )) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/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.
| /// Used by replay-safety tests (PA-006): re-submit the captured | ||
| /// bytes via `sdk.broadcast_state_transition` and assert the | ||
| /// platform rejects the duplicate. 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. For | ||
| /// PA-006 that's the right shape: the test confirms the second | ||
| /// submission is rejected regardless of the nonce relationship | ||
| /// between the two builds. |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/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.
| // Fee: last-non-zero-wins. Sync-only merges (which carry | ||
| // `fee == 0`) preserve a transfer's recorded fee; merging | ||
| // two transfer changesets sums the fees, matching the | ||
| // "total fee paid across operations in this batch" intent. | ||
| self.fee = self.fee.saturating_add(other.fee); |
There was a problem hiding this comment.
💬 Nitpick: Merge fee comment opens with "last-non-zero-wins" but the code sums
The comment leads with // Fee: last-non-zero-wins. and then in the same block describes "merging two transfer changesets sums the fees" — directly contradicting itself. The code is self.fee = self.fee.saturating_add(other.fee), i.e. sum semantics; a true last-non-zero-wins merge of fee=100 then fee=50 would yield 50, while saturating_add yields 150. Drop the misleading lead clause so the policy matches the code.
💡 Suggested change
| // Fee: last-non-zero-wins. Sync-only merges (which carry | |
| // `fee == 0`) preserve a transfer's recorded fee; merging | |
| // two transfer changesets sums the fees, matching the | |
| // "total fee paid across operations in this batch" intent. | |
| self.fee = self.fee.saturating_add(other.fee); | |
| // Fee: append-sum across merged changesets. Sync-only merges | |
| // (which carry `fee == 0`) preserve a transfer's recorded | |
| // fee; merging two transfer changesets sums the fees, | |
| // matching the "total fee paid across operations in this | |
| // batch" intent. | |
| self.fee = self.fee.saturating_add(other.fee); |
source: ['claude', 'codex']
| loop { | ||
| match Identity::fetch(sdk, identity_id).await { | ||
| Ok(Some(identity)) => { | ||
| let balance = identity.balance(); | ||
| if balance >= expected { | ||
| tracing::info!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| observed = balance, | ||
| expected, | ||
| elapsed = ?start.elapsed(), | ||
| "identity balance reached target" | ||
| ); | ||
| return Ok(balance); | ||
| } | ||
| tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| current = balance, | ||
| expected, | ||
| "identity balance below target" | ||
| ); | ||
| } | ||
| Ok(None) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| ?identity_id, | ||
| "identity not yet visible on chain" | ||
| ), | ||
| Err(err) => tracing::debug!( | ||
| target: "platform_wallet::e2e::wait", | ||
| error = %err, | ||
| "fetch::<Identity> failed during wait_for_identity_balance" | ||
| ), | ||
| } | ||
|
|
||
| if Instant::now() >= deadline { | ||
| return Err(FrameworkError::Cleanup(format!( | ||
| "wait_for_identity_balance timed out after {timeout:?} \ | ||
| (identity_id={identity_id:?} expected={expected})" | ||
| ))); | ||
| } | ||
| tokio::time::sleep(BACKSTOP_WAKE_INTERVAL).await; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: wait_for_identity_balance / wait_for_dpns_name_visible can overshoot the supplied timeout by up to BACKSTOP_WAKE_INTERVAL
Both new helpers do match fetch ... ; if Instant::now() >= deadline { err } ; sleep(BACKSTOP_WAKE_INTERVAL=2s) ; loop. If the caller passes a timeout shorter than 2s, the unconditional 2s sleep can run after the deadline check, so the helpers can return a timeout error nearly 2 seconds late — and the next iteration's fetch may succeed against a chain state the test no longer cares about. Cap the sleep against deadline.saturating_duration_since(Instant::now()) (the same pattern wait_for_balance already uses with cap = std::cmp::min(remaining, BACKSTOP_WAKE_INTERVAL)) so the overshoot is bounded by one fetch RTT. Low-impact today (callers in the harness all pass ≥30s), but cheap to get right while the helpers are new.
source: ['claude', 'codex']
Issue being fixed or feature implemented
Stacks on top of #3549 to deliver the next layer of the
rs-platform-wallete2e 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.rsis 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 intransfer()viaAddressFundsTransferTransition::estimate_min_fee. Exact for the canonicalDeductFromInput(_)strategy.PlatformAddressWallet::sync_watermark() -> Option<u64>— monotonic block watermark for sync assertions.IdentityManager::identity_count()/identity_ids()— convenience accessors.PlatformPaymentAddressProvider::last_known_recent_blockgetter — required plumbing forsync_watermark.Harness — Wave A: identity signer + setup helpers
SeedBackedIdentitySignerimplementingSigner<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_balanceandwait_for_dpns_name_visibleinframework/wait.rs.Harness — Wave B: multi-identity setup
setup_with_n_identities(n) -> MultiIdentitySetupGuard— fundsndistinct addresses on one test wallet and registers an identity from each. Wrapper around the existingSetupGuard(chosen over field extension becauseSetupGuard.teardown_calledispub(crate)).Harness — Wave F: test-only utilities
TestWallet::transfer_with_inputs(PA-002 negative variant — bypassesauto_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)
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✓PLATFORM_WALLET_E2E_BANK_MNEMONICand a funded testnet wallet; documented intests/e2e/README.md.Breaking Changes
None. All additions are net-new public API or framework-only code.
Checklist:
Stacks on #3549. Merge order: #3549 → this PR → follow-up test-case PRs.
🤖 Generated with Claude Code