Merged
Conversation
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Core as Core (message creation)
participant Storage as Storage (GroupStorage)
participant DB as DB (SQLite / Memory)
Core->>Core: now = Timestamp::now()\nconstruct Message(processed_at = now)
Core->>Storage: save_message(message)
Storage->>DB: INSERT/UPDATE message (created_at, processed_at, id)
DB-->>Storage: write ok
Storage->>Storage: group.update_last_message_if_newer(message)
Storage->>DB: INSERT/UPDATE group (last_message_at, last_message_processed_at, last_message_id)
DB-->>Storage: group write ok
Storage-->>Core: ack saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Coverage: 90.77% → 90.83% (+0.06%) |
Closed
jgmontoya
reviewed
Feb 2, 2026
jgmontoya
approved these changes
Feb 2, 2026
|
✅ Coverage: 90.79% → 90.86% (+0.07%) |
jgmontoya
reviewed
Feb 4, 2026
✅ Coverage: 90.79% → 90.86% (+0.07%) |
✅ Coverage: 90.79% → 90.86% (+0.07%) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/mdk-storage-traits/src/messages/types.rs`:
- Around line 91-106: The rustdoc links in the compare_display_keys function
comment reference Group fields as [Group::last_message_at],
[Group::last_message_processed_at], and [Group::last_message_id] which won't
resolve because Group isn't in scope; update those links to use fully qualified
paths like crate::groups::types::Group::last_message_at,
crate::groups::types::Group::last_message_processed_at, and
crate::groups::types::Group::last_message_id so cargo doc can resolve them.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-storage-traits/src/groups/types.rs (1)
80-106:⚠️ Potential issue | 🔴 CriticalRedact sensitive identifiers from
GroupDebug output.
GroupderivesDebugwhile containingmls_group_id,nostr_group_id,image_key, andimage_nonce, which must not be exposed in Debug output. Replace the derive with a redactedDebugimpl.🔒 Proposed redacted Debug implementation
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub struct Group { ... } + +impl fmt::Debug for Group { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Group") + .field("mls_group_id", &"[REDACTED]") + .field("nostr_group_id", &"[REDACTED]") + .field("name", &self.name) + .field("description", &self.description) + .field("image_hash", &self.image_hash) + .field("image_key", &"[REDACTED]") + .field("image_nonce", &"[REDACTED]") + .field("admin_pubkeys", &self.admin_pubkeys) + .field("last_message_id", &self.last_message_id) + .field("last_message_at", &self.last_message_at) + .field("last_message_processed_at", &self.last_message_processed_at) + .field("epoch", &self.epoch) + .field("state", &self.state) + .finish() + } +}crates/mdk-storage-traits/src/messages/types.rs (1)
38-66:⚠️ Potential issue | 🔴 CriticalImplement a custom
Debugimpl forMessageto redact sensitive identifiers.The
Messagestruct at line 38 derivesDebugwhile containingmls_group_idand other sensitive fields (content,tags,event) that must not be exposed in debug output. Remove theDebugderive and implement a customDebugtrait that redacts these fields.🔒 Proposed redacted Debug implementation
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub struct Message { ... } + +impl fmt::Debug for Message { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Message") + .field("id", &self.id) + .field("pubkey", &self.pubkey) + .field("kind", &self.kind) + .field("mls_group_id", &"[REDACTED]") + .field("created_at", &self.created_at) + .field("processed_at", &self.processed_at) + .field("content", &"[REDACTED]") + .field("tags", &"[REDACTED]") + .field("event", &"[REDACTED]") + .field("wrapper_event_id", &self.wrapper_event_id) + .field("epoch", &self.epoch) + .field("state", &self.state) + .finish() + } +}This addresses the security requirement that sensitive identifiers (
mls_group_id, group-related content, event data) must never be exposed in Debug trait implementations.
✅ Coverage: 90.79% → 90.87% (+0.08%) |
erskingardner
approved these changes
Feb 6, 2026
erskingardner
added a commit
to alltheseas/mdk
that referenced
this pull request
Feb 18, 2026
…store The snapshot_groups_table() and restore_group_from_snapshot() functions were missing the last_message_processed_at column added in V002 (PR marmot-protocol#166). After a MIP-03 commit race rollback, this column would reset to NULL, undoing the message ordering fix for that group.
erskingardner
added a commit
to alltheseas/mdk
that referenced
this pull request
Feb 18, 2026
…store The snapshot_groups_table() and restore_group_from_snapshot() functions were missing the last_message_processed_at column added in V002 (PR marmot-protocol#166). After a MIP-03 commit race rollback, this column would reset to NULL, undoing the message ordering fix for that group.
erskingardner
added a commit
that referenced
this pull request
Feb 18, 2026
* feat: add self-update tracking to Group struct (MIP-02) Adds two fields to the Group struct: - `needs_self_update: bool` — set by accept_welcome(), cleared when a pure self-update commit is merged - `last_self_update_at: Option<Timestamp>` — updated every time a pure self-update commit is merged, enabling periodic rotation tracking The merge_pending_commit() method inspects the StagedCommit before merging to detect pure self-updates (update proposals only, no add/remove/psk), matching the validation logic in is_pure_self_update_commit(). Includes SQLite migration, UniFFI bindings, serde backward-compat defaults, and test updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add groups_needing_self_update() query Default trait method on GroupStorage that returns active groups needing a self-update: either post-join (needs_self_update=true) or stale rotation (last_self_update_at older than threshold). Exposed via MDK public API and UniFFI bindings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: replace two self-update fields with SelfUpdateState enum Replace needs_self_update (bool) and last_self_update_at (Option<Timestamp>) with a single self_update_state: SelfUpdateState field on Group. The enum has three variants: NotRequired, Required (MIP-02 post-join), and CompletedAt(Timestamp) (MIP-00 periodic rotation). This eliminates impossible state combinations, reduces the SQLite migration to a single column (last_self_update_at INTEGER), and fixes the snapshot/ restore mechanism to preserve self-update state across MIP-03 rollbacks. Adds changelogs for mdk-storage-traits, mdk-core, mdk-sqlite-storage, and mdk-uniffi. * fix: consolidate changelog headers and prefix UniFFI self_update_state - Remove duplicate '### Breaking changes' and '### Added' headers in mdk-storage-traits and mdk-uniffi changelogs, merging PR #184 entries into the existing sections. - Prefix the CompletedAt timestamp in UniFFI with 'completed_at:' so FFI clients can reliably distinguish state variants by string prefix rather than parsing an ambiguous bare integer. * style: fix import placement, derive order, comment clarity, and add serde tests - Move in-function imports to module scope in epoch_snapshots.rs tests and cross_storage.rs (per AGENTS.md import placement rules) - Fix SelfUpdateState derive order: Default at end per project convention - Clarify is_self_update comment: all() is vacuously true on empty iterators, which is expected for path-based self-updates - Rename local variable all_updates to no_non_update_proposals for clarity - Add serde roundtrip tests for SelfUpdateState (NotRequired/Required/ CompletedAt) and verify #[serde(default)] behavior on Group * fix(sqlite-storage): include last_message_processed_at in snapshot/restore The snapshot_groups_table() and restore_group_from_snapshot() functions were missing the last_message_processed_at column added in V002 (PR #166). After a MIP-03 commit race rollback, this column would reset to NULL, undoing the message ordering fix for that group. * fix(core): track self-update state in OwnCommitPending path The OwnCommitPending handler in process.rs called OpenMLS merge_pending_commit() directly, bypassing the self-update detection in MDK::merge_pending_commit(). After a rollback/reprocess of a self-update commit, self_update_state would remain Required instead of transitioning to CompletedAt, causing a redundant self-update. * refactor: remove NotRequired variant from SelfUpdateState Group creators now start with CompletedAt(now) since creating a group with a fresh key is effectively the first rotation. This ensures every group is always on the periodic rotation clock and eliminates the blind spot where creators were never flagged for staleness. The enum now has two variants: Required (post-join, MIP-02) and CompletedAt(Timestamp) (last rotation, MIP-00). The SQLite column becomes NOT NULL DEFAULT 0. --------- Co-authored-by: alltheseas <alltheseas@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Jeff Gardner <202880+erskingardner@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes message timing and ordering inconsistencies by recording when messages are processed locally (processed_at) and using that timestamp as a secondary sort key so ordering is deterministic even when created_at ties or devices have clock skew. It updates message construction, group last-message bookkeeping, in-memory and SQLite storage, storage traits, and UniFFI bindings so group.last_message_id aligns with messages()[0].id under the canonical display order.
What changed:
Security impact:
Protocol changes:
API surface:
Testing: