Skip to content

Conversation

@insipx
Copy link
Contributor

@insipx insipx commented Nov 14, 2025

Annotate group message send with dependency cursors and add SHA-256 hashing across d14n envelopes in xmtp_api_d14n and xmtp_mls

Implement dependency resolution for group message publish by computing payload SHA-256s and setting AAD.depends_on in XmtpMlsClient::send_group_messages, add CursorStore::find_message_dependencies, propagate cursors and hashes through Envelope, EnvelopeCollection, and extractors, and extend SQLite cursor store to return dependency cursors.

📍Where to Start

Start with dependency injection and AAD setting in XmtpMlsClient::send_group_messages in mls.rs, then review the CursorStore extensions in cursor_store.rs and the SQLite implementation in cursor_store.rs.


Macroscope summarized d2ad8ec.

@insipx
Copy link
Contributor Author

insipx commented Nov 14, 2025

This PR is part of a stack of 3 bookmarks:

  1. trunk()
  2. push-tkyvzsqvnkyz
  3. insipx/dependency-query
  4. insipx/set-dependencies ← this PR

Created with jj-stack

@insipx insipx marked this pull request as draft November 14, 2025 15:56
@claude
Copy link

claude bot commented Nov 14, 2025

Claude finished @insipx's task —— View job


Code Review Complete

Reviewed PR #2788: set dependencies in send message

Review Date: December 1, 2025, 16:03 UTC


Summary

This PR implements dependency tracking for group messages by computing SHA-256 hashes of message payloads and attaching dependency cursors via AAD.depends_on. The implementation is generally solid with good test coverage for the core functionality. I've identified several areas that need attention:


🔴 Critical Issues

1. Panic in InMemoryCursorStore (xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs:102)

fn find_message_dependencies(&self, hash: &[&[u8]]) -> Result<HashMap<Vec<u8>, Cursor>, CursorStoreError> {
    Err(CursorStoreError::NoDependenciesFound(hash.iter().map(hex::encode).collect()))
}

Issue: This returns an error instead of an empty HashMap. This could break in-memory testing scenarios.
Recommendation: Return Ok(HashMap::new()) to match the behavior of NoCursorStore::find_message_dependencies (which correctly returns an empty HashMap).


⚠️ High Priority

2. Missing Error Propagation in send_group_messages (xmtp_api_d14n/src/queries/d14n/mls.rs:100-107)

The try_for_each silently propagates errors through ? operator, but if sha256_hash() fails on any envelope, the entire batch fails:

envelopes.iter_mut().try_for_each(|envelope| {
    let data = envelope.sha256_hash()?;
    let dependency = dependencies.remove(&data);
    let mut aad = envelope.aad.clone().unwrap_or_default();
    aad.depends_on = dependency.map(Into::into);
    envelope.aad = Some(aad);
    Ok(())
})?;

Issue: No logging or context for why the batch failed. This makes debugging difficult in production.
Recommendation: Add tracing to log which envelope failed and why.

3. Potential Performance Issue: HashMap Lookups (xmtp_api_d14n/src/queries/d14n/mls.rs:92-98)

The code computes all hashes first, queries dependencies, then iterates again to attach them:

let hashes = request.messages.sha256_hashes()?;
let mut dependencies = self.cursor_store.find_message_dependencies(
    hashes.iter().map(AsRef::as_ref).collect::<Vec<_>>().as_slice()
)?;

Issue: Two passes over the messages could be expensive for large batches. The intermediate Vec allocation is also unnecessary.
Recommendation: Consider whether a single-pass approach would be more efficient, though the current design is acceptable if message batches are small.


🟡 Medium Priority

4. Test Coverage Gaps (Per Codecov Report)

  • xmtp_api_d14n/src/queries/d14n/test/send_group_message.rs: 25% coverage (24 lines missing)
  • xmtp_proto/src/types/global_cursor.rs: 51.21% coverage (20 lines missing)
  • xmtp_api_d14n/src/protocol/impls/protocol_envelopes.rs: 0% coverage for new SHA-256 methods (18 lines)

Recommendation: Add integration tests covering:

  • Error cases when SHA-256 computation fails
  • Behavior when some messages have dependencies and others don't
  • Edge case: empty message batch

5. Cursor Store Dependency Query Semantics (xmtp_mls/src/cursor_store.rs:171-188)

The find_message_dependencies implementation joins with refresh_state to find commits:

let dependencies: HashMap<Vec<u8>, IntentDependency> = self
    .db
    .find_dependant_commits(hashes)
    .map_err(CursorStoreError::other)?
    .into_iter()
    .map(|(k, v)| (k.into(), v))
    .collect();

Issue: The semantics of "what is a dependency" are not documented. It appears to return the last commit in a group, but this should be explicitly documented in the trait.
Recommendation: Add documentation to the CursorStore::find_message_dependencies trait method explaining what constitutes a valid dependency.


🟢 Low Priority / Suggestions

6. Type Safety: Hash Type

Multiple places use Vec<u8> for SHA-256 hashes. Consider defining a newtype:

pub struct Sha256Hash([u8; 32]);

This would catch bugs at compile time if non-hash bytes are accidentally used.

7. AAD Cloning (xmtp_api_d14n/src/queries/d14n/mls.rs:103)

let mut aad = envelope.aad.clone().unwrap_or_default();

The clone is necessary here, but could be avoided with more careful ownership management. This is a micro-optimization and not critical.


✅ Positive Observations

  1. Strong test coverage for core functionality: test_message_dependencies.rs provides comprehensive integration tests
  2. Good separation of concerns: Cursor store abstraction is clean
  3. Proptest usage: The use of property-based testing in send_group_message.rs is excellent for catching edge cases
  4. SHA-256 implementation: The hashing appears to be implemented correctly through the extractor pattern

Action Items

  1. Fix the InMemoryCursorStore::find_message_dependencies panic (Critical)
  2. Add error logging in send_group_messages for failed hash computations (High)
  3. Add integration tests for error paths (Medium)
  4. Document dependency semantics in trait docs (Medium)

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

❌ Patch coverage is 58.42294% with 116 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.91%. Comparing base (5f75f4e) to head (d2ad8ec).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...i_d14n/src/queries/d14n/test/send_group_message.rs 25.00% 24 Missing ⚠️
xmtp_proto/src/types/global_cursor.rs 51.21% 20 Missing ⚠️
..._api_d14n/src/protocol/impls/protocol_envelopes.rs 0.00% 18 Missing ⚠️
xmtp_api_d14n/src/protocol/traits/cursor_store.rs 41.37% 17 Missing ⚠️
xmtp_proto/src/impls.rs 0.00% 11 Missing ⚠️
...tp_api_d14n/src/protocol/in_memory_cursor_store.rs 0.00% 8 Missing ⚠️
xmtp_api_d14n/src/queries/d14n/client.rs 50.00% 7 Missing ⚠️
xmtp_mls/src/cursor_store.rs 81.48% 5 Missing ⚠️
xmtp_api_d14n/src/protocol/sort/timestamp.rs 0.00% 2 Missing ⚠️
..._api_d14n/src/protocol/utils/test/test_envelope.rs 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2788      +/-   ##
==========================================
- Coverage   73.93%   73.91%   -0.03%     
==========================================
  Files         390      392       +2     
  Lines       49841    50095     +254     
==========================================
+ Hits        36852    37026     +174     
- Misses      12989    13069      +80     

☔ 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 insipx force-pushed the insipx/set-dependencies branch from fa74c34 to 67391ef Compare November 17, 2025 23:47
@insipx insipx force-pushed the insipx/set-dependencies branch 2 times, most recently from 2eb2896 to dd2a940 Compare November 18, 2025 16:57
@insipx insipx changed the base branch from insipx/ordered-combinator to insipx/add-published-in-epoch November 18, 2025 18:30
@insipx insipx force-pushed the insipx/set-dependencies branch from dd2a940 to d943b80 Compare November 18, 2025 18:30
@insipx insipx force-pushed the insipx/add-published-in-epoch branch from d027392 to d9e4053 Compare November 18, 2025 18:39
@insipx insipx force-pushed the insipx/set-dependencies branch 2 times, most recently from 2a40598 to 0ed5444 Compare November 19, 2025 00:42
@insipx insipx force-pushed the insipx/add-published-in-epoch branch from d9e4053 to 69c6929 Compare November 19, 2025 00:42
@insipx insipx force-pushed the insipx/set-dependencies branch from 0ed5444 to 5a0ae8e Compare November 19, 2025 00:47
@insipx insipx force-pushed the insipx/add-published-in-epoch branch 2 times, most recently from cee6002 to 5046d87 Compare November 19, 2025 00:50
@insipx insipx force-pushed the insipx/set-dependencies branch 3 times, most recently from 81c73a2 to 9a445a1 Compare November 19, 2025 18:02
@insipx insipx force-pushed the insipx/sync-conn branch 2 times, most recently from 859ec84 to f3afec7 Compare November 25, 2025 16:38
@insipx insipx force-pushed the insipx/set-dependencies branch 4 times, most recently from 22f2a52 to a0a267d Compare November 25, 2025 20:43
@insipx insipx force-pushed the insipx/set-dependencies branch from a0a267d to a03ee2d Compare November 25, 2025 20:44
@insipx insipx changed the base branch from insipx/sync-conn to insipx/dependency-query November 25, 2025 20:49
@insipx insipx force-pushed the insipx/set-dependencies branch 4 times, most recently from 118b203 to 226d0dc Compare November 25, 2025 20:58
@insipx insipx force-pushed the insipx/set-dependencies branch from 226d0dc to 6a1f626 Compare November 26, 2025 14:52
@insipx insipx force-pushed the insipx/dependency-query branch from 344089e to f415cf8 Compare November 26, 2025 14:52
@insipx insipx force-pushed the insipx/set-dependencies branch from 6a1f626 to 0489303 Compare November 26, 2025 23:04
@insipx insipx force-pushed the insipx/dependency-query branch from f415cf8 to 5713910 Compare November 26, 2025 23:04
@insipx insipx force-pushed the insipx/dependency-query branch from 5713910 to a139392 Compare December 1, 2025 15:03
@insipx insipx force-pushed the insipx/set-dependencies branch from 0489303 to 5e941ec Compare December 1, 2025 15:03
Base automatically changed from insipx/dependency-query to main December 1, 2025 15:21
@insipx insipx force-pushed the insipx/set-dependencies branch from 5e941ec to d2ad8ec Compare December 1, 2025 16:01
@insipx insipx enabled auto-merge (squash) December 1, 2025 16:02
@insipx insipx merged commit 4db9815 into main Dec 1, 2025
23 checks passed
@insipx insipx deleted the insipx/set-dependencies branch December 1, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants