feat: expiry

[chris13524]

Implements expiration a two separate parts:

• Protection against usage of expired resources. There are various if statements that prevent using, for example, an expired session. If an expired resource is trying to be used, it will simply block the usage. It doesn't try to expire the resource, or emit an event in this place.
• Expiry thread. This is a high-accuracy timer system which is engaged exactly when a resource expires. This is the part that both 1) actually cleans up the resource and 2) emits expiration events.

Remaining work:

• tests & UI integration
• consider using ms instead of seconds for timer system for better event emitting accuracy
• audit wake is called in all situations

[claude]

Claude finished @chris13524's task —— View job

---

Code Review: feat: expiry

This PR adds expiry functionality for sessions, session proposals, and pairings in the WalletConnect Sign implementation. Overall the implementation is solid, but there are several issues that should be addressed.

---

Critical Issues

1. Unsafe unwrap() calls in production code

File: crates/yttrium/src/uniffi_compat/sign/storage.rs:115

.map(|s| s.try_into().unwrap()))

This will panic if the Vec doesn't have exactly 32 bytes. Should return a proper error.

File: crates/yttrium/src/uniffi_compat/sign/storage.rs:143-144

sym_key: pairing.sym_key.try_into().unwrap(),
self_key: pairing.self_key.try_into().unwrap(),

Same issue - will panic on malformed data from storage.

Recommendation: Use map_err() to convert to StorageError instead of panicking:

.and_then(|s| s.try_into().map_err(|_| StorageError::Runtime("Invalid key length".to_string())))

2. Race condition in expiry monitoring

File: crates/yttrium/src/sign/expiry.rs:45-93

The monitor_expiry function has a potential race condition. It calls get_all_pairings() and get_all_sessions() separately, then processes them. Between these calls and the actual expiry checks, the data could have been modified by other operations.

Additionally, the timestamp comparison logic may have issues:

if timestamp_now >= expiry {
    // delete
}

If system time changes (NTP sync, manual adjustment), items could expire prematurely or not at all.

Recommendation:

• Add monotonic time tracking alongside wall-clock time
• Consider adding a grace period for edge cases
• Handle storage errors more gracefully (currently just logs)

3. Missing error handling for concurrent deletions

File: crates/yttrium/src/sign/expiry.rs:52-91

When deleting expired items, errors are logged but not properly handled. If multiple clients are running, concurrent deletions could cause issues.

if let Err(e) = storage.delete_pairing(topic.clone()) {
    tracing::error!("Failed to delete pairing: {e}");
}

Recommendation: Consider whether these errors should bubble up or be handled differently.

---

Security Issues

4. Expiry validation bypass in session requests

File: crates/yttrium/src/sign/verify.rs:101-125

The session request expiry validation has a critical logic issue:

if let Some(expiry) = params.request.expiry {
    if expiry > 0 {
        let expiry_ms = expiry * 1000;
        let min = 300_000;
        let max = 604_800_000;
        
        if expiry_ms < min {
            return Err(VerifyError::ExpiryTooShort);
        }
        if expiry_ms > max {
            return Err(VerifyError::ExpiryTooLong);
        }
    }
}

Problems:

1. expiry == 0 bypasses validation entirely (only checks if expiry > 0)
2. The actual expiration time is never calculated or validated against current time
3. The validation only checks the duration range, not if the request has already expired

Recommendation:

• Reject requests with expiry == 0 or make it explicit what zero means
• Validate that current_time + expiry < max_allowed_time
• Check if requests are already expired on arrival

5. Timezone/timestamp synchronization issues

The code uses SystemTime::now() throughout without considering:

• Clock skew between client and server
• NTP synchronization issues
• Timezone handling

Recommendation: Add documentation about time synchronization requirements and consider adding clock skew tolerance.

---

Code Quality Issues

6. Missing error variants documentation

File: crates/yttrium/src/sign/client_errors.rs:114-139

New error types added but lack documentation:

#[error("Expiry is too short")]
ExpiryTooShort,
#[error("Expiry is too long")]
ExpiryTooLong,

Recommendation: Add doc comments explaining:

• What the min/max values are
• What units are expected (ms vs seconds)
• How to fix the error

7. Magic numbers without constants

File: crates/yttrium/src/sign/verify.rs:104-105

let min = 300_000;
let max = 604_800_000;

File: crates/yttrium/src/sign/client.rs:1058

let expiry_time = 604_800;

These magic numbers should be named constants with documentation.

Recommendation:

/// Minimum session request expiry: 5 minutes (in milliseconds)
const MIN_SESSION_REQUEST_EXPIRY_MS: u64 = 300_000;
/// Maximum session request expiry: 7 days (in milliseconds)
const MAX_SESSION_REQUEST_EXPIRY_MS: u64 = 604_800_000;
/// Default session expiry: 7 days (in seconds)
const DEFAULT_SESSION_EXPIRY_SECONDS: u64 = 604_800;

8. Inconsistent timestamp units

The code mixes seconds and milliseconds inconsistently:

• Session expiry: seconds (crates/yttrium/src/sign/client.rs:1058)
• Request expiry validation: milliseconds (crates/yttrium/src/sign/verify.rs:103)
• Storage timestamps: u64 without clear units

Recommendation:

• Standardize on one unit (preferably milliseconds for precision)
• Use newtype wrappers to prevent mixing: TimestampSeconds(u64) vs TimestampMillis(u64)
• Add doc comments specifying units on all time-related fields

9. TODO comments left in production code

File: crates/rust-sample-wallet/src/app.rs:33-34

// TODO expiring for sessions
// TODO expiring for pairing_keys

File: crates/rust-sample-wallet/src/app.rs:659

std::time::Duration::from_secs(5 * 60), // TODO use actual expiry

Recommendation: Either implement these or create GitHub issues and reference them in comments.

10. Hardcoded test timeout

File: crates/rust-sample-wallet/src/app.rs:659

std::time::Duration::from_secs(5 * 60), // TODO use actual expiry

The UI hardcodes a 5-minute timeout instead of using the actual expiry from the protocol.

---

Testing Issues

11. Insufficient test coverage for expiry edge cases

File: crates/yttrium/src/sign/test_helpers/mod.rs:365

The test sets expiry: Some(0) but doesn't verify:

• What happens when a request expires before being processed
• Concurrent expiry of multiple items
• Clock skew scenarios
• Boundary conditions for min/max expiry values

Recommendation: Add comprehensive tests for:

• Expired requests on arrival
• Requests that expire during processing
• Edge cases for min/max bounds
• Race conditions in expiry monitoring

12. Test uses production code expiry validation

File: crates/yttrium/src/sign/test_helpers/mod.rs:387

assert_eq!(req.params.request.expiry, Some(0));

This test passes expiry: Some(0) which bypasses the validation check (issue #4). The test should fail but doesn't.

---

Performance Concerns

13. Expiry monitoring polls storage repeatedly

File: crates/yttrium/src/sign/expiry.rs:30-36

loop {
    tokio::time::sleep(Duration::from_secs(poll_interval)).await;
    Self::check_expiry(storage.clone(), message_tx.clone()).await;
}

The monitoring loop queries all pairings and sessions every poll interval (default 60s). For clients with many sessions, this could be inefficient.

Recommendation:

• Consider using a priority queue/heap to track next expiry time
• Only poll when items are actually expiring
• Add metrics to monitor performance impact

14. JSON-RPC history cleanup efficiency

File: crates/yttrium/src/sign/expiry.rs:78-89

Fetching all JSON-RPC history entries and filtering in memory could be inefficient for long-lived clients with many requests.

Recommendation: Add a storage method to delete by timestamp range instead of fetching all and filtering.

---

Documentation Issues

15. Missing PR description

The PR has no description explaining:

• What problem this solves
• How the expiry mechanism works
• Breaking changes (new storage requirements)
• Migration guide for existing clients

Recommendation: Add a comprehensive PR description.

16. Undocumented storage contract changes

The storage interface now requires timestamp tracking for JSON-RPC history, but this isn't documented in migration notes.

File: crates/yttrium/src/sign/storage.rs:48-54

Implementers of the Storage trait need to know about the new expiry requirements.

---

External Domain Check

No external domain URLs detected outside of approved domains (reown.com, walletconnect.com, walletconnect.org).

---

Summary

The expiry feature implementation is functionally complete but has critical issues:

Must fix before merge:

1. Security issue with expiry == 0 bypassing validation (chore: setup swift ci #4)
2. Unsafe unwrap() calls that can panic (Rust: enable cargo fmt #1)
3. Race condition in expiry monitoring (Rust: enable clippy in CI #2)

Should fix before merge:
4. Magic numbers → named constants (#7)
5. Inconsistent timestamp units (#8)
6. Missing expiry edge case tests (#11)
7. Missing PR description (#15)

Consider for future:
8. Expiry monitoring performance optimization (#13)
9. Monotonic time tracking (#2)
10. Clock skew tolerance (#5)

[chris13524]

Consider also deleting JSON-RPC history when session/pairings/etc. expire too