chore: refactor attestation creation

[chris13524]

• Fix that native app would send empty attestation (Some("")) instead of null attestation (None).
• Refactor and greatly simplify Verify API code.
• Use proper state machine element to implement the timeout (and cleanup signal).
• Fix that onmessage listener wasn't being cleaned up

[claude]

Claude finished @chris13524's task —— View job

---

Code Review Complete

Refactoring attestation creation into separate module - found issues needing attention.

• Read and analyze all changed files
• Check for bugs and code quality issues
• Verify error handling and async patterns
• Check for external domain URLs
• Post review findings

---

Issues Found

Issue 1: Silent attestation failures (relay.rs:1267, 1463, 1649, 1655)
create_attestation errors converted to empty attestations via attestation_error(). Error context lost - can't distinguish "not supported" vs "failed" vs "timed out". Consider using enum: Result<Attestation, AttestationError> where AttestationError::NotSupported | Failed(String) | TimedOut.

Issue 2: Missing message origin validation (verify/creation.rs:62-72)
MessageEvent handler parses data without validating event.origin(). Attacker on different origin could inject fake attestations via postMessage. Add:

if event.origin() != expected_verify_origin { return; }

Issue 3: Memory leak in attestation iframe (verify/creation.rs:56-97)
If verify server never responds (network issue/blocked), closure + iframe never cleaned up. Add timeout mechanism to remove event listener + iframe.

Issue 4: Race condition in parallel attestation (relay.rs:1227-1277)
Attestation spawned in parallel with websocket connect. If attestation completes before websocket ready and oneshot consumed early, could miss result. Current code appears safe due to state machine sequencing but fragile - add comment explaining ordering guarantees.

Issue 5: Inconsistent conditional compilation (relay.rs:484)
Removed use tracing::Instrument from top-level imports but re-added conditionally inside WASM closure. Use #[cfg(target_arch = "wasm32")] at import level instead.

Issue 6: Type name collision (relay.rs:661, validation.rs:268)
Type alias pub type Attestation = Option<Arc<str>> shadows struct Attestation from validation module. Rename type alias to AttestationResult or OptionalAttestation to avoid confusion.

Issue 7: Attestation timeout insufficient (relay.rs:78)
VERIFY_TIMEOUT = 5s same as REQUEST_TIMEOUT = 5s. If attestation times out, entire request times out with no fallback to unverified request. Consider longer timeout or graceful degradation.

Issue 8: Error type too generic (verify/creation.rs:24)
Returns Result<_, String> making errors unactionable. Use typed error enum for better error handling upstream.

Issue 9: Unsafe unwrap in test (test_helpers/mod.rs:152-154)
.unwrap() calls could panic on malformed data. Use ? or map_err for better error messages.

Issue 10: Missing cleanup on attestation send failure (verify/creation.rs:78-82)
If tx.send() fails (receiver dropped), iframe remains in DOM and event listener stays registered. Add cleanup in else branch.

---

✅ No external domain URLs detected (only walletconnect.org/reown.com domains found)

[chris13524]

Clippy complains about unused constant on non-wasm build target.

[chris13524]

Avoid clippy unused warnings for non-wasm build targets

[chris13524]

@claude review

[claude]

Claude finished @chris13524's task —— View job

---

🔄 Incremental Review: Attestation Refactoring

• Read and analyze changed files
• Check previous review issues status
• Identify new issues in refactored code
• Validate clippy warning concerns from review comments
• Post review findings

---

Previously Identified Issues - Status Update

Previously Issue 2 (validate.rs:268): Type name collision - ✅ RESOLVED
Type alias Attestation at relay.rs:661 no longer collides with struct. Struct renamed from Attestation (validation.rs:268 old) to proper separation in new module structure.

Previously Issue 3 (verify/creation.rs:56-97): Memory leak in attestation iframe - ✅ RESOLVED
Timeout mechanism now properly implemented using DurableSleep in state machine (relay.rs:1263, 1458, 1645, 1766). Event listeners and iframe cleaned up on timeout via state transitions.

Previously Issue 2 (verify/creation.rs:62-72): Missing message origin validation - ✅ RESOLVED
Origin validation now present at verify/create.rs:63-65. Validates event.origin() != VERIFY_SERVER_URL before processing attestation.

Previously Issue 10 (verify/creation.rs:78-82): Missing cleanup on send failure - ✅ RESOLVED
Cleanup logic now handles send failures properly at verify/create.rs:86-101 with iframe and listener removal.

---

New Issues Found

Issue 1: Clippy unused constant warning (relay.rs:78)
VERIFY_TIMEOUT constant only used on wasm32 targets but not marked with #[cfg(target_arch = "wasm32")] attribute. Will trigger clippy unused warnings on non-wasm builds (confirmed by review comment).

Issue 2: Clippy unused variable warning (relay.rs:1282-1283)
Explicit drop(encrypted_id) and drop(decrypted_id) in non-wasm branch will trigger clippy warnings about unnecessary drops. Use underscore prefix _encrypted_id, _decrypted_id in pattern match or #[allow(unused_variables)] attribute.

Issue 3: Race in attestation channel receive (relay.rs:1254-1277, 1634-1659)
Using match attestation_rx (creating receiver) then conditionally transitioning to awaiting state. If create_attestation returns Ok(rx) but attestation completes immediately before state transition, could lose result. Should handle receiver consistently without intermediate match destructuring.

Issue 4: Silent attestation failure loses error context (relay.rs:1267, 1463, 1649, 1655)
attestation_error() returns Some("") on failure. Error reason from create_attestation (line 1267: "Failed to create attestation: {e:?}") logged but not preserved. Downstream cannot distinguish "iframe creation failed" vs "timeout" vs "origin mismatch". Empty string attestation treated as failed attestation by server.

Issue 5: Missing iframe cleanup on state machine exit (relay.rs:1496-1506, 1804-1814)
When cleanup_rx.cancelled() fires during ConnectRequestAwaitingAttestation or ConnectedAwaitingAttestation, websocket closes but attestation iframe/listener remain in DOM. Need to cancel attestation receiver or track cleanup handle.

Issue 6: DurableSleep not cancelled on early state transitions (relay.rs:1438-1495)
DurableSleep created at line 1438 but not explicitly cancelled when transitioning to ConnectRequestAttestationReady via attestation completion (line 1447-1456) or websocket close (line 1466-1495). While Rust drops should handle it, explicit cancellation clearer for timeout futures.

---

✅ No external domain URLs detected

---