simple timestamp sort

[insipx]

Add timestamp-based envelope sorting and expose protocol::traits::Envelope::timestamp() to support simple timestamp sort

Introduce protocol::sort::timestamp with TimestampSort and a timestamp helper, add TimestampExtractor and CursorExtractor, and extend protocol::traits::Envelope with timestamp() while refactoring protocol::traits::Sort to a single sort method and renaming Cursor::welcomes to Cursor::v3_welcomes.

📍Where to Start

Start with the TimestampSort implementation in xmtp_api_d14n/src/protocol/sort/timestamp.rs, then review Envelope::timestamp() in xmtp_api_d14n/src/protocol/traits/envelopes.rs and the TimestampExtractor in xmtp_api_d14n/src/protocol/extractors/timestamp.rs.

---

Macroscope summarized 7e857bd.

[insipx]

• simple timestamp sort #2676 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Claude finished @insipx's task —— View job

---

PR Review Complete

Reviewed on: 2025-11-13 21:13 UTC

---

Summary

The timestamp-based sorting implementation looks solid overall. The key concerns have already been identified by other reviewers. Below are additional observations not yet mentioned:

Code Quality & Architecture

✅ Well-structured: The visitor pattern implementation is clean and the extractor abstraction is well-designed.

✅ Good test coverage approach: Property-based tests with proptest are appropriate for sorting validation.

Issues Found (Not Yet Mentioned)

1. Test Coverage Gap - Empty/None Timestamps (xmtp_api_d14n/src/protocol/sort/timestamp.rs:26)

The documentation states:

If a timestamp does not have a cursor (extractor return Option::None) it is sorted according to Ord

However, the test suite only generates envelopes with Some(timestamp) values (line 109-111). Missing coverage for:

• All None timestamps
• Mixed Some/None scenarios
• Edge cases where None values should sort to beginning/end

Suggestion: Add test cases like:

#[xmtp_common::test]
fn sorts_none_timestamps_consistently() {
    let mut envelopes = vec![
        TestEnvelope { time: None },
        TestEnvelope::new(100),
        TestEnvelope { time: None },
    ];
    sort::timestamp(&mut envelopes).sort();
    // Assert None values are grouped appropriately
}

2. Type Safety Concern - u64 → i64 Cast (xmtp_api_d14n/src/protocol/extractors/timestamp.rs:45,50,54)

While Macroscope correctly identified the overflow risk, the issue is more nuanced:

• originator_ns is already i64 in the proto (safe)
• created_ns fields are u64 in protos (unsafe cast)

The cast message.created_ns as i64 will wrap for values ≥ 2^63. Given these are nanosecond timestamps:

• i64::MAX = 2^63-1 nanoseconds ≈ year 2262
• While unlikely in practice, this violates type safety

However, the existing codebase already has this pattern in other extractors (group_messages.rs:59, welcomes.rs:56), suggesting this might be an accepted risk or architectural decision.

Recommendation: If this is intentional, document it. Otherwise, align all timestamp handling to use saturating conversion consistently across the codebase.

3. Documentation Accuracy (xmtp_api_d14n/src/protocol/sort/timestamp.rs:27)

Line 27 states: "This sort will never return any missing envelopes."

This is correct for timestamp sort but contrasts with the Sort trait documentation which mentions dependency resolution. Consider clarifying that this simple timestamp sort is distinct from causal/dependency-aware sorting to avoid confusion when the more complex sorts are introduced in the stack.

Performance

✅ The change from sort_by_key to sort_unstable_by_key (as suggested by tylerhawkes) has been implemented correctly.

Test Quality

⚠️ Test randomness range is narrow (timestamp.rs:109): 1..u32::MAX as nanoseconds only covers ~4 seconds. Consider using full i64 range to test actual timestamp spans:

fn envelope_all_some()(ns in 0i64..i64::MAX) -> TestEnvelope {
    TestEnvelope::new(ns)
}

---

Priority Issues

1. HIGH: Add test coverage for None timestamp handling
2. MEDIUM: Expand test timestamp range to realistic values
3. LOW: Document u64→i64 casting decision if intentional

---

Task List:

• Read and analyze core implementation files
• Review code quality and identify issues
• Check test coverage
• Provide concise feedback

---

[codecov]

Codecov Report

❌ Patch coverage is 21.13821% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.32%. Comparing base (0e12ccf) to head (7e857bd).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/protocol/extractors/cursor.rs	0.00%	31 Missing ⚠️
xmtp_api_d14n/src/protocol/sort/timestamp.rs	46.15%	28 Missing ⚠️
xmtp_api_d14n/src/protocol/extractors/timestamp.rs	0.00%	25 Missing ⚠️
xmtp_api_d14n/src/protocol/traits/envelopes.rs	0.00%	5 Missing ⚠️
xmtp_api_d14n/src/protocol/traits/extractor.rs	0.00%	3 Missing ⚠️
...tp_api_d14n/src/protocol/types/missing_envelope.rs	0.00%	3 Missing ⚠️
..._d14n/src/protocol/traits/dependency_resolution.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2676      +/-   ##
==========================================
- Coverage   77.41%   77.32%   -0.10%     
==========================================
  Files         365      371       +6     
  Lines       53312    53461     +149     
==========================================
+ Hits        41271    41338      +67     
- Misses      12041    12123      +82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[insipx]

This PR is part of a stack of 11 bookmarks:

1. trunk()
2. insipx/dependency-resolution ← this PR
3. insipx/causal-sort
4. insipx/resolution
5. insipx/network-backoff
6. insipx/ordered-combinator
7. insipx/test-resolution
8. insipx/test-ordering
9. insipx/add-published-in-epoch
10. insipx/sql-queries
11. insipx/sync-conn
12. insipx/set-dependencies

---

Created with jj-stack

[insipx]

these tests are a bit extra since this sort is exactly one call to sort_by_key on Vec::, but it is consistent with how the causal sort is tested

[tylerhawkes]

why can't we proptest in wasm?

[insipx]

proptest exists in wasm, but they dont support the fork feature for wasm so had to turn it off