Fix #247: Use borrowed &str keys for existing_tx_map in process_wallet_transactions

[schjonhaug]

Closes #247

Hatchery Report Back for Issue #247

Title: Use borrowed &str keys for existing_tx_map in process_wallet_transactions

Summary

Update backend/src/sync.rs so process_wallet_transactions uses a borrowed HashMap<&str, &TransactionWithWallet> for existing_tx_map, matching the two other sync paths in the same file. This removes one String clone per existing transaction during wallet sync and standardizes lookup behavior across wallet/address-watch flows.

Changed Files

• backend/src/sync.rs

Worker Results

• backend-1773304997: completed (backend)

Lead Agent Results

• report: warning - I have the spec and current placeholder report. Next I’m pulling the worker and validation logs so the handoff stays tied to what actually ran, especially the remaining warning.
• rework: completed - - cargo test --lib failed, but on an unrelated pre-existing config test: backend/src/config.rs#L774 expected CANARY_SYNC_INTERVAL=42 and got 300. - cargo test --test two_stage_send_scenarios test_alice_partial_send_bob_two_stage -- --ignored --nocapture failed during environment setup with No available ports found after extensive search. - cargo test --test advanced_transactions test_single_rbf_sender_and_receiver_perspective -- --ignored --nocapture failed for the same port-allocation reason. No reviewer-facing code questions remain for issue Use borrowed &str keys for existing_tx_map in process_wallet_transactions #247. The only open items are the unrelated test failure and the local system-test environment constraint. tokens used 38 360 The reviewer’s requested fix is already present on this branch, so I did not make an additional code edit. In backend/src/sync.rs#L414, process_wallet_transactions now builds existing_tx_map as HashMap<&str, &TransactionWithWallet> using tx.txid.as_str(), and the lookups in the same flow were updated to use borrowed &str keys at backend/src/sync.rs#L450, backend/src/sync.rs#L492, backend/src/sync.rs#L615, and backend/src/sync.rs#L735. Validation: - cargo test --lib failed, but on an unrelated pre-existing config test: backend/src/config.rs#L774 expected CANARY_SYNC_INTERVAL=42 and got 300. - cargo test --test two_stage_send_scenarios test_alice_partial_send_bob_two_stage -- --ignored --nocapture failed during environment setup with No available ports found after extensive search. - cargo test --test advanced_transactions test_single_rbf_sender_and_receiver_perspective -- --ignored --nocapture failed for the same port-allocation reason. No reviewer-facing code questions remain for issue Use borrowed &str keys for existing_tx_map in process_wallet_transactions #247. The only open items are the unrelated test failure and the local system-test environment constraint.

Validation Results

• backend: completed - Executable tests/wallet_key_format_tests.rs (backend/target/debug/deps/wallet_key_format_tests-a06fd220e9c39e50)
• backend-log: warning - backend log file missing

Browser Validation

No browser validation summary recorded.

PR Status

• State: created
• URL: Fix #247: Use borrowed &str keys for existing_tx_map in process_wallet_transactions #261
• Review: PR review status: ready (CLEAN, MERGEABLE, approvals=0, comments=4)
• Context: PR review status: ready (CLEAN, MERGEABLE, approvals=0, comments=4)
• Checks:
• build: completed (success)
• claude-review: completed (success)
• codex: completed (success)
• gemini-review: completed (success)
• post_feedback: completed (success)

Warnings

• backend-log: warning - backend log file missing

Ready for Human Review

• Review the changed files listed above.
• Confirm the validation results are acceptable for the issue scope.
• Follow the issue-specific verification checklist below when it exists.
• Inspect .hatchery/pr-review.md after PR review starts so change requests and comments are explicit.
• If runtime behavior matters, inspect the tmux runtime console before approving the solution.

Suggested Verification Checklist

• cd backend && cargo test --lib
• cd backend && cargo test --test two_stage_send_scenarios test_alice_partial_send_bob_two_stage -- --ignored --nocapture
• cd backend && cargo test --test advanced_transactions test_single_rbf_sender_and_receiver_perspective -- --ignored --nocapture
• If Docker-based tests are not available, at minimum ensure the backend test suite compiles and note the skipped system-test coverage in the handoff.

[github-actions]

🤖 Codex Code Review

No PR-specific defects found in the changed lines.

Findings

1. No bugs, security issues, or performance regressions identified in the HashMap key-type change from String to &str in sync.rs, sync.rs, sync.rs, sync.rs, and sync.rs.
   This is a valid optimization that avoids cloning txids while preserving lookup behavior.

Test coverage

1. No test updates appear in this diff.
2. Residual risk is low, but a focused sync-path test covering both existing and new tx lookups (including conflicted tx lookup path) would lock in this behavior.

[github-actions]

🤖 Gemini Code Review

Here is a review of the proposed changes to backend/src/sync.rs:

Code Quality and Best Practices

• Excellent Zero-Copy Optimization: Changing the HashMap key from String to &str is a fantastic and highly idiomatic Rust optimization. By borrowing the string slice directly from the existing TransactionWithWallet structs (tx.txid.as_str()), you completely eliminate unnecessary heap allocations that were previously caused by .clone().
• Idiomatic Lookups: Using .as_str() during lookups (e.g., existing_tx_map.get(txid.as_str())) is the correct way to query a HashMap<&str, V> using an owned String or a string reference without allocating.

Performance Considerations

• Memory & Speed Improvements: This will noticeably improve performance during sync operations, especially for wallets with a large transaction history. Building the lookup map is now strictly cheaper because it avoids O(N) string allocations. You save both memory and CPU cycles that would otherwise be spent by the allocator.

Potential Bugs or Issues

• Lifetimes: In Rust, storing a &str requires the referenced data to outlive the HashMap. Because the strings are borrowed directly from existing_transactions (which clearly outlives existing_tx_map within the scope of this function), the borrow checker will correctly validate this. Assuming the code compiles, there are no bugs or lifetime issues introduced here.

Security Concerns

• None: This is a purely internal, low-level memory optimization. It does not alter the business logic or external surface area, so there are no security implications.

Test Coverage

• No New Tests Needed: Because this is a structural optimization that doesn't change the behavior or logic of the syncing mechanism, no new tests are strictly required. Assuming you have existing tests that cover the wallet synchronization flows (which would exercise this map), they will automatically verify that the behavior remains identical.

[github-actions]

🤖 Claude Code Review

---

Pull Request Review

Two PRs were merged in the recent commits: #247 (backend fix) and #253 (frontend layout refactor).

---

PR #247 — Fix: Use borrowed &str keys for existing_tx_map

Files changed: backend/src/sync.rs

Summary

Changes the HashMap<String, &TransactionWithWallet> key type to HashMap<&str, &TransactionWithWallet> in process_wallet_transactions, avoiding a clone() per transaction when building the lookup table.

Correctness: ✅

The fix is correct. The lifetimes are sound: existing_tx_map borrows string slices from existing_transactions, and both live for the same function scope. All four call sites are updated consistently.

Code Quality: ✅

The change is minimal, focused, and resolves a straightforward allocation inefficiency. It also brings process_wallet_transactions in line with other functions in the same file (e.g., around lines 1555 and 1839) that already used the &str key pattern.

One observation

At sync.rs:733-735, a temporary String is still created solely to call .as_str() for the map lookup:

let conflicted_txid_str = conflicted_txid.to_string();
if let Some(pending_tx) = existing_tx_map.get(conflicted_txid_str.as_str()) {

This is correct Rust (the temporary lives long enough), but the intermediate conflicted_txid_str binding is only needed because HashMap::get takes Borrow<K>, which Txid doesn't implement for str. It's fine as-is — just a side effect of the BDK type not implementing Borrow<str>. No change needed.

Performance: ✅

Eliminates N String::clone() calls per sync cycle, where N is the number of existing transactions. For wallets with large transaction histories this is a meaningful improvement.

---

PR #253 — Align donations route with shared app layout

Files changed: frontend/src/app/donations/layout.tsx (new), frontend/src/app/donations/page.tsx

Summary

Extracts AppHeader, AppFooter, and DemoBanner out of the page component into a dedicated layout.tsx, consistent with how wallets/, settings/, subscription/, and contact/ routes are structured.

Code Quality: ✅

The refactor is clean and the right structural move. The follow-up commit correctly removed the unnecessary "use client" directive from the layout (it has no hooks or browser APIs), making it a proper Next.js server component.

One observation

settings/layout.tsx:1 still carries "use client" despite containing no client-side hooks — identical to the issue fixed in this PR's follow-up. Worth a small follow-up cleanup to make settings/layout.tsx a server component too.

Layout hierarchy: ✅

The outer layout sets max-w-6xl (full-width shell), while the page keeps max-w-2xl mx-auto py-16 for the narrower centered content — this matches the original visual result and is intentional.

Security

The donationsBaseUrl in page.tsx:12-14 is constructed from process.env.NODE_ENV (a build-time constant), not from user input, and only used for href attributes with rel="noopener noreferrer". No XSS concern.

---

Overall Assessment

Both changes are well-scoped and correct. The Rust fix is a targeted allocation improvement with no logic changes, and the frontend layout extraction improves consistency with the rest of the app's routing architecture. No bugs or security issues identified.

Minor follow-up suggestion: Remove "use client" from frontend/src/app/settings/layout.tsx to match the pattern just established in this PR.

[schjonhaug]

Hatchery PR handoff

Issue: #247 Use borrowed &str keys for existing_tx_map in process_wallet_transactions
PR: #261
Review status: PR is merged.

Summary

Update backend/src/sync.rs so process_wallet_transactions uses a borrowed HashMap<&str, &TransactionWithWallet> for existing_tx_map, matching the two other sync paths in the same file. This removes one String clone per existing transaction during wallet sync and standardizes lookup behavior across wallet/address-watch flows.

Review context

No review context available yet.

Changed files

• backend/src/sync.rs

Validation

• backend: completed - Executable tests/wallet_key_format_tests.rs (backend/target/debug/deps/wallet_key_format_tests-a06fd220e9c39e50)
• backend-log: warning - backend log file missing

Lead-agent runs

• report: warning - I have the spec and current placeholder report. Next I’m pulling the worker and validation logs so the handoff stays tied to what actually ran, especially the remaining warning.
• rework: completed - - cargo test --lib failed, but on an unrelated pre-existing config test: backend/src/config.rs#L774 expected CANARY_SYNC_INTERVAL=42 and got 300. - cargo test --test two_stage_send_scenarios test_alice_partial_send_bob_two_stage -- --ignored --nocapture failed during environment setup with No available ports found after extensive search. - cargo test --test advanced_transactions test_single_rbf_sender_and_receiver_perspective -- --ignored --nocapture failed for the same port-allocation reason. No reviewer-facing code questions remain for issue Use borrowed &str keys for existing_tx_map in process_wallet_transactions #247. The only open items are the unrelated test failure and the local system-test environment constraint. tokens used 38 360 The reviewer’s requested fix is already present on this branch, so I did not make an additional code edit. In backend/src/sync.rs#L414, process_wallet_transactions now builds existing_tx_map as HashMap<&str, &TransactionWithWallet> using tx.txid.as_str(), and the lookups in the same flow were updated to use borrowed &str keys at backend/src/sync.rs#L450, backend/src/sync.rs#L492, backend/src/sync.rs#L615, and backend/src/sync.rs#L735. Validation: - cargo test --lib failed, but on an unrelated pre-existing config test: backend/src/config.rs#L774 expected CANARY_SYNC_INTERVAL=42 and got 300. - cargo test --test two_stage_send_scenarios test_alice_partial_send_bob_two_stage -- --ignored --nocapture failed during environment setup with No available ports found after extensive search. - cargo test --test advanced_transactions test_single_rbf_sender_and_receiver_perspective -- --ignored --nocapture failed for the same port-allocation reason. No reviewer-facing code questions remain for issue Use borrowed &str keys for existing_tx_map in process_wallet_transactions #247. The only open items are the unrelated test failure and the local system-test environment constraint.