Skip to content

Conversation

@insipx
Copy link
Contributor

@insipx insipx commented Nov 18, 2025

  • add published_in_epoch column to group message
  • requires companion pr in xmtp/proto
  • sets the epoch of a group_message when storing in the database.

This will help ensure message dependencies can be programmed defensively by ensuring the epoch of a commit is 1 less then the message we are setting a dependency on.

@insipx
Copy link
Contributor Author

insipx commented Nov 18, 2025

This was referenced Nov 18, 2025
@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @insipx's task —— View job


Code Review Complete

Review Date: November 21, 2025 00:16 UTC

I've reviewed this PR which adds the published_in_epoch column to group messages. The implementation is solid overall with good test coverage. Here are my findings:

✅ Strengths

  • Clean database migration with proper up/down scripts
  • Consistent field propagation through all layers (DB → proto → ORM → MLS)
  • Proper handling as Option<i64> allowing backward compatibility
  • Good test coverage (all modified lines covered per Codecov)
  • Field correctly populated in both process_group_message (line 1089) and save_transcript_message (line 2021) in xmtp_mls/src/groups/mls_sync.rs

🔍 Observations

1. Schema Inconsistency (Minor)
The group_intents table already has a published_in_epoch column (xmtp_db/src/encrypted_store/schema_gen.rs:43), but there's no corresponding update or validation logic ensuring the epoch on an intent matches the epoch on its resulting message. Since the PR description mentions "ensuring the epoch of a commit is 1 less than the message we are setting a dependency on," consider whether intents and their messages should have matching epochs.

2. Test Helper Gap
The test helper generate_message() in xmtp_db/src/encrypted_store/group_message/tests.rs:34 hardcodes published_in_epoch: None. While this works, future tests validating epoch-specific logic may need to pass this value explicitly. Consider adding it as an optional parameter.

3. Migration Timing
Migration file naming: 2025-11-18-174226-0000_group_message_epoch. The year appears to be 2025 which is unusual. Verify this is intentional or if it should be 2024.

Summary

The implementation correctly adds epoch tracking to messages as described. The concerns above are minor - the inconsistency observation is primarily for future work in the stack, and the test helper is purely a maintainability suggestion. No blocking issues found.


@insipx insipx force-pushed the insipx/add-published-in-epoch branch from d027392 to d9e4053 Compare November 18, 2025 18:39
Comment on lines +2 to +24
/// Token is used by clients to prove to the nodes
/// that they are serving a specific wallet.
#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)]
pub struct Token {
/// identity key signed by a wallet
#[prost(message, optional, tag = "1")]
pub identity_key: ::core::option::Option<super::super::message_contents::PublicKey>,
/// encoded bytes of AuthData
#[prost(bytes = "vec", tag = "2")]
pub auth_data_bytes: ::prost::alloc::vec::Vec<u8>,
/// identity key signature of AuthData bytes
#[prost(message, optional, tag = "3")]
pub auth_data_signature: ::core::option::Option<
super::super::message_contents::Signature,
>,
}
impl ::prost::Name for Token {
const NAME: &'static str = "Token";
const PACKAGE: &'static str = "xmtp.message_api.v1";
fn full_name() -> ::prost::alloc::string::String {
"xmtp.message_api.v1.Token".into()
}
fn type_url() -> ::prost::alloc::string::String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can be done so that generation of these are stable and don't move around when we add more code?

Copy link
Contributor Author

@insipx insipx Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could try using https://docs.rs/tonic/latest/tonic/macro.include_proto.html

This would generate the protos into the rust OUT_DIR. the downside is first compile will be a bit longer as it needs to clone xmtp/proto and googleapis to generate the protos. but it will delete the entire gen/ directory, which will create smaller PRs.

at that point, i would argue for moving xmtp_proto, or at least just the "proto" part into xmtp/proto, then renaming xmtp_proto to xmtp_types or xmtp_primitives or something. Then we could get rid of the companion PR pattern and proto generation could just be a cargo update which makes versioning clearer (rather than ad-hoc commit refs we store in txt, cargo would take care of it in the lockfile)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we open an issue with prost. My guess is they're using some hash that is dependent on the contents of the protos and that's why things change around when a single field is added.

@insipx insipx force-pushed the insipx/add-published-in-epoch branch from d9e4053 to 69c6929 Compare November 19, 2025 00:42
@macroscopeapp
Copy link
Contributor

macroscopeapp bot commented Nov 19, 2025

Add published_in_epoch to group messages and persist it from xmtp_mls::MlsGroup.process_group_message and xmtp_mls::MlsGroup.save_transcript_message into the group_messages.published_in_epoch BIGINT column

Introduce an optional epoch field on stored group messages, propagate it through proto, conversion, and ORM, and populate it during message processing and transcript saves; include DB migration adding published_in_epoch.

📍Where to Start

Start with the epoch capture and persistence in MlsGroup.process_group_message and MlsGroup.save_transcript_message in xmtp_mls/src/groups/mls_sync.rs.


Macroscope summarized cfd66ae.

@insipx insipx force-pushed the insipx/add-published-in-epoch branch from 69c6929 to cee6002 Compare November 19, 2025 00:47
@insipx insipx force-pushed the insipx/test-ordering branch 2 times, most recently from 41d2537 to 9f5e2f8 Compare November 19, 2025 00:50
@insipx insipx force-pushed the insipx/add-published-in-epoch branch from cee6002 to 5046d87 Compare November 19, 2025 00:50
@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.70%. Comparing base (6c66152) to head (695ab3c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2805      +/-   ##
==========================================
+ Coverage   74.67%   74.70%   +0.02%     
==========================================
  Files         385      385              
  Lines       49762    49780      +18     
==========================================
+ Hits        37158    37186      +28     
+ Misses      12604    12594      -10     

☔ 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.

Base automatically changed from insipx/test-ordering to main November 19, 2025 01:01
@insipx insipx force-pushed the insipx/add-published-in-epoch branch 3 times, most recently from 1fce6b2 to 695ab3c Compare November 19, 2025 22:43
@insipx insipx force-pushed the insipx/add-published-in-epoch branch 2 times, most recently from 0eb3f0a to ea8a79a Compare November 21, 2025 00:14
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.

3 participants