crates/mdk-sqlite-storage/migrations/V004__add_self_update_tracking.sql (1)

5-6: Consider an index for groups_needing_self_update queries.

groups_needing_self_update() will scan the entire groups table and filter on needs_self_update and last_self_update_at. For current user-scale this is fine, but a partial index can make the query constant-time if the groups table ever grows (e.g. shared-server or multi-account usage):

CREATE INDEX IF NOT EXISTS idx_groups_self_update
    ON groups (needs_self_update, last_self_update_at)
    WHERE state = 'Active';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-sqlite-storage/migrations/V004__add_self_update_tracking.sql`
around lines 5 - 6, Add a partial index to the migration so queries from
groups_needing_self_update() are fast: after adding the columns
needs_self_update and last_self_update_at in V004__add_self_update_tracking.sql,
create an index named idx_groups_self_update on (needs_self_update,
last_self_update_at) with a WHERE clause filtering state = 'Active' (use CREATE
INDEX IF NOT EXISTS idx_groups_self_update ... WHERE state = 'Active'); this
keeps scanning constant-time as the groups table grows and ties directly to the
new columns and the groups_needing_self_update() lookup.

crates/mdk-storage-traits/src/groups/types.rs (1)

111-115: Consider #[serde(default)] for needs_self_update.

This preserves backward-compatible deserialization of older serialized Group payloads that won’t have the new boolean field.

♻️ Suggested tweak

-    pub needs_self_update: bool,
+    #[serde(default)]
+    pub needs_self_update: bool,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-storage-traits/src/groups/types.rs` around lines 111 - 115, The
deserialization will fail for older Group payloads missing the new boolean
field; add serde's default behavior to the field by annotating the
needs_self_update field with #[serde(default)] (i.e., change the field
declaration in the Group type to #[serde(default)] pub needs_self_update: bool)
so missing values deserialize to false and preserve backward compatibility.

crates/mdk-storage-traits/src/groups/types.rs (1)

389-421: Consider asserting the new fields in the serialization test.

The struct literal now includes needs_self_update: false and last_self_update_at: None, but test_group_serialization never asserts their serialized values. Adding assertions would confirm both the field names and the #[serde(default)] round-trip behavior.

🔧 Suggested addition

     assert_eq!(serialized["state"], json!("active"));
+    assert_eq!(serialized["needs_self_update"], json!(false));
+    assert_eq!(serialized["last_self_update_at"], json!(null));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-storage-traits/src/groups/types.rs` around lines 389 - 421, Add
assertions in the test_group_serialization test to verify the serialized values
for the newly added fields needs_self_update and last_self_update_at: assert
that serialized["needs_self_update"] equals json!(false) and
serialized["last_self_update_at"] equals json!(null) (or the expected default
representation), so the Group struct's serde default/round-trip behavior for
needs_self_update and last_self_update_at is covered.

crates/mdk-uniffi/src/lib.rs (1)

1182-1209: Consider a less stringly-typed self_update_state for FFI.
Mixing sentinel strings with raw timestamps can be easy to mis-parse across languages. Prefixing the timestamp (or exposing a UniFFI enum) would reduce ambiguity for clients.

♻️ Low-impact prefix to make the value self-describing

-    /// - `"<unix_timestamp>"`: Last self-update merged at this time (MIP-00).
+    /// - `"completed_at:<unix_timestamp>"`: Last self-update merged at this time (MIP-00).

-                group_types::SelfUpdateState::CompletedAt(ts) => ts.as_secs().to_string(),
+                group_types::SelfUpdateState::CompletedAt(ts) => format!("completed_at:{}", ts.as_secs()),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-uniffi/src/lib.rs` around lines 1182 - 1209, The current
conversion for Group::from maps group_types::SelfUpdateState into a string
mixing sentinel values and raw timestamps via the self_update_state field, which
is error-prone for FFI clients; change the mapping in the
From<group_types::Group> implementation so self_update_state becomes
self-describing (e.g., prefix the timestamp like "completed_at:<unix_ts>" or
switch the exposed type to a UniFFI enum) by updating the match over
group_types::SelfUpdateState (variants NotRequired, Required, CompletedAt) to
emit the chosen prefixed string form or adjust the Group type to an enum variant
instead so callers can reliably parse the state across languages.

crates/mdk-sqlite-storage/migrations/V004__add_self_update_tracking.sql (2)

6-6: Consider adding an index if groups_needing_self_update is called frequently.

The new groups_needing_self_update(threshold_secs) query will filter on last_self_update_at. While groups tables tend to be small enough for a full scan today, an index on this column would keep the query fast as the table grows.

CREATE INDEX IF NOT EXISTS idx_groups_last_self_update_at
    ON groups (last_self_update_at);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-sqlite-storage/migrations/V004__add_self_update_tracking.sql` at
line 6, Add a non-blocking index for the new column to keep queries like
groups_needing_self_update(threshold_secs) fast as the table grows: after the
ALTER TABLE statement that adds last_self_update_at, create an index named
idx_groups_last_self_update_at on the groups(last_self_update_at) column using
CREATE INDEX IF NOT EXISTS so migration is idempotent and safe for production.

---

6-6: Three states packed into one column via magic values — consider two dedicated columns.

The current encoding (NULL / 0 / >0) is compact and documented, but conflating "no obligation" with NULL and "obligation pending" with the sentinel 0 means every reader must remember the magic-value contract. A future-proof alternative is two explicit columns:

-- epoch sentinel removed; intent is unambiguous at the schema level
ALTER TABLE groups ADD COLUMN needs_self_update  INTEGER NOT NULL DEFAULT 0;
ALTER TABLE groups ADD COLUMN last_self_update_at INTEGER;

needs_self_update = 1 (flag set by accept_welcome) and last_self_update_at IS NULL (no rotation yet) would map one-to-one to the enum variants without any sentinel. Both columns default naturally for existing rows (0 / NULL).

This is a purely optional design preference; the current single-column approach is correct and safe given that a real last_self_update_at = 0 (Unix epoch) is implausible in production.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-sqlite-storage/migrations/V004__add_self_update_tracking.sql` at
line 6, The migration adds a single column last_self_update_at that encodes
three states via NULL/0/>0; instead, add an explicit boolean-like flag to make
intent unambiguous: modify the migration that alters the groups table (ALTER
TABLE groups ADD COLUMN last_self_update_at INTEGER;) to also add a
needs_self_update INTEGER NOT NULL DEFAULT 0 column and keep last_self_update_at
as nullable; ensure application code that sets/reads last_self_update_at and the
new needs_self_update flag (e.g., the accept_welcome flow and any readers
expecting the magic 0 sentinel) are updated to set needs_self_update = 1 when an
update is pending and to use last_self_update_at IS NULL to mean "never
updated."

crates/mdk-storage-traits/src/groups/types.rs (1)

35-60: Add a unit test for the custom SelfUpdateState serde roundtrip.

The sentinel value 0 for Required is a non-obvious encoding. The existing test_group_serialization test doesn't assert on self_update_state, and there's no test that exercises Required ↔ Some(0), CompletedAt(ts) ↔ Some(ts), and NotRequired ↔ None. A wrong impl here would silently corrupt stored state.

✅ Suggested test

#[test]
fn test_self_update_state_serde_roundtrip() {
    // NotRequired serializes as null and deserializes back
    let not_req = SelfUpdateState::NotRequired;
    let json = serde_json::to_value(&not_req).unwrap();
    assert_eq!(json, serde_json::Value::Null);
    let back: SelfUpdateState = serde_json::from_value(json).unwrap();
    assert_eq!(back, SelfUpdateState::NotRequired);

    // Required serializes as 0 and deserializes back
    let req = SelfUpdateState::Required;
    let json = serde_json::to_value(&req).unwrap();
    assert_eq!(json, serde_json::json!(0u64));
    let back: SelfUpdateState = serde_json::from_value(json).unwrap();
    assert_eq!(back, SelfUpdateState::Required);

    // CompletedAt serializes as the timestamp seconds and deserializes back
    let ts = Timestamp::from_secs(1_700_000_000);
    let completed = SelfUpdateState::CompletedAt(ts);
    let json = serde_json::to_value(&completed).unwrap();
    assert_eq!(json, serde_json::json!(1_700_000_000u64));
    let back: SelfUpdateState = serde_json::from_value(json).unwrap();
    assert_eq!(back, SelfUpdateState::CompletedAt(ts));

    // Missing field defaults to NotRequired via #[serde(default)]
    let group_json = serde_json::json!({"mls_group_id": {"value": {"vec": [1,2,3]}}, "nostr_group_id": [0u8;32], "name": "t", "description": "", "admin_pubkeys": [], "last_message_id": null, "last_message_at": null, "last_message_processed_at": null, "epoch": 0, "state": "active"});
    let g: Group = serde_json::from_value(group_json).unwrap();
    assert_eq!(g.self_update_state, SelfUpdateState::NotRequired);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-storage-traits/src/groups/types.rs` around lines 35 - 60, Add a
unit test named test_self_update_state_serde_roundtrip that verifies Serde
roundtrips for SelfUpdateState: assert SelfUpdateState::NotRequired serializes
to JSON null and deserializes back, SelfUpdateState::Required serializes to
numeric 0u64 and deserializes back, and SelfUpdateState::CompletedAt(ts)
serializes to the timestamp seconds and deserializes back (use
Timestamp::from_secs for ts); also assert that deserializing a Group JSON
missing self_update_state yields SelfUpdateState::NotRequired (to cover the
#[serde(default]) behavior. Use serde_json::to_value/from_value and equality
checks against expected serde_json::Value values to exercise the custom
Serialize/Deserialize impls.

crates/mdk-core/src/groups.rs (3)

1295-1306: Clarify the vacuous-truth behaviour of all() on an empty iterator.

staged_commit.queued_proposals().all(|p| matches!(p.proposal(), Proposal::Update(_))) returns true when queued_proposals() is empty — and for a self_update_with_new_signer commit the iterator is always empty (the key rotation goes through the update path, not a queued proposal). This is the intended behaviour, but the comment only says "All proposals must be Update type", which implies there must be proposals. A reader unfamiliar with Rust's vacuous-all semantics could misread this as "at least one Update proposal must exist".

Consider adding a short inline clarification:

💬 Suggested comment tweak

-            // All proposals must be Update type (no add/remove/psk/etc.)
+            // All queued proposals, if any, must be Update type (no add/remove/psk/etc.).
+            // NOTE: for a self_update_with_new_signer commit the queue is empty,
+            // so this vacuously returns `true` — that is intentional.
             let all_updates = staged_commit
                 .queued_proposals()
                 .all(|p| matches!(p.proposal(), Proposal::Update(_)));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-core/src/groups.rs` around lines 1295 - 1306, The comment for the
is_self_update check is misleading about
staged_commit.queued_proposals().all(...) because Rust's all() returns true for
an empty iterator; update-only semantics are vacuously true when there are no
queued proposals (as in self_update_with_new_signer where the update comes via
update_path_leaf_node()). Update the surrounding comment above the
is_self_update calculation (referring to mls_group.pending_commit(),
staged_commit.update_path_leaf_node(), staged_commit.update_proposals(), and
staged_commit.queued_proposals()) to explicitly state that all(...) is vacuously
true on empty iterators and that an empty queued_proposals() is expected for
path-based updates, so the check means “no non-update proposals present” rather
than “at least one Update proposal exists.”

---

1292-1327: Missing test: non-self-update merge_pending_commit must not mutate self_update_state.

The PR objective states "Non-self-update commits leave both fields unchanged", and the inline comment at Line 1317 repeats this guarantee. However, none of the existing tests assert this explicitly — for example, that add_members + merge_pending_commit preserves a pre-existing Required or CompletedAt state. A targeted test (analogous to the test_groups_needing_self_update test mentioned in the PR) would close this gap and guard against regressions.

Would you like me to draft a test for this scenario?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-core/src/groups.rs` around lines 1292 - 1327, Add a unit test
(e.g., test_merge_pending_commit_non_self_update_preserves_self_update_state)
that sets up a group with a pre-existing self_update_state (both variants:
Required and CompletedAt), performs a non-self-update operation such as
add_members to produce a pending commit, calls merge_pending_commit(group_id) on
the GroupStore, then reloads the group via get_group(group_id) and asserts that
group.self_update_state is unchanged (compare to the original SelfUpdateState
value). Use the existing helper setup used by other group tests, ensure the
staged commit is indeed non-self-update (no update_path_leaf_node and includes
an Add proposal), and fail the test if merge_pending_commit mutates
self_update_state.

---

1318-1324: Two storage round-trips when is_self_update is true — consider consolidating.

When is_self_update, the flow performs:

1. sync_group_metadata_from_mls → get_group → mutate non-self-update fields → save_group
2. get_group → set self_update_state = CompletedAt(now) → save_group

The second write reads back the group that was just persisted and saves it again with only one field changed. This is functionally correct because sync_group_metadata_from_mls intentionally preserves whatever self_update_state is already stored, and the second write then overrides it. However the intermediate state (synced metadata, old self_update_state) is briefly observable by concurrent readers of the storage layer.

A lightweight fix is to apply the self_update_state update to the same stored_group inside sync_group_metadata_from_mls's write, or accept an Option<SelfUpdateState> override parameter. At minimum, the current two-write path warrants a short comment:

♻️ Option A – pass override into a private sync helper (avoids second write)

-    pub fn sync_group_metadata_from_mls(&self, group_id: &GroupId) -> Result<(), Error> {
+    pub fn sync_group_metadata_from_mls(&self, group_id: &GroupId) -> Result<(), Error> {
+        self.sync_group_metadata_from_mls_with_state(group_id, None)
+    }
+
+    fn sync_group_metadata_from_mls_with_state(
+        &self,
+        group_id: &GroupId,
+        self_update_override: Option<group_types::SelfUpdateState>,
+    ) -> Result<(), Error> {
         let mls_group = self.load_mls_group(group_id)?.ok_or(Error::GroupNotFound)?;
         let mut stored_group = self.get_group(group_id)?.ok_or(Error::GroupNotFound)?;
         let group_data = NostrGroupDataExtension::from_group(&mls_group)?;
         stored_group.epoch = mls_group.epoch().as_u64();
         stored_group.name = group_data.name;
         // ...existing field updates...
+        if let Some(state) = self_update_override {
+            stored_group.self_update_state = state;
+        }
         self.storage()
             .save_group(stored_group)
             .map_err(|e| Error::Group(e.to_string()))?;
         Ok(())
     }

Then in merge_pending_commit:

-    self.sync_group_metadata_from_mls(group_id)?;
-
-    if is_self_update {
-        let mut group = self.get_group(group_id)?.ok_or(Error::GroupNotFound)?;
-        group.self_update_state = group_types::SelfUpdateState::CompletedAt(Timestamp::now());
-        self.storage()
-            .save_group(group)
-            .map_err(|e| Error::Group(e.to_string()))?;
-    }
+    let state_override = is_self_update
+        .then(|| group_types::SelfUpdateState::CompletedAt(Timestamp::now()));
+    self.sync_group_metadata_from_mls_with_state(group_id, state_override)?;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-core/src/groups.rs` around lines 1318 - 1324, When is_self_update
is true we perform two storage round-trips (sync_group_metadata_from_mls ->
save_group, then get_group -> modify self_update_state -> save_group);
consolidate by applying the SelfUpdateState change in the same write: update
sync_group_metadata_from_mls (or its private helper) to accept an
Option<group_types::SelfUpdateState> override (or detect is_self_update and set
group.self_update_state before calling save_group), then call that from
merge_pending_commit so the stored_group written by sync_group_metadata_from_mls
already has self_update_state = CompletedAt(Timestamp::now()), eliminating the
extra get_group/save_group cycle and preventing the intermediate stale state
from being observable.

crates/mdk-sqlite-storage/src/lib.rs (1)

3856-3884: Missing SelfUpdateState import — relies on use super::*;.

snapshot_openmls_tests uses SelfUpdateState::NotRequired at line 3883 but doesn't import it explicitly (line 3860 only imports Group, GroupState). It works via use super::*; but is fragile. Consider adding it to the explicit import for consistency with the sibling snapshot_tests module (line 3264).

Also, none of the snapshot tests exercise a Group with SelfUpdateState::Required or CompletedAt(ts). Since the snapshot path serializes the raw last_self_update_at column (where Required maps to Some(0) and CompletedAt maps to Some(ts)), a round-trip test with non-default state values would help catch encoding regressions.

Suggested import fix

         use mdk_storage_traits::groups::GroupStorage;
-        use mdk_storage_traits::groups::types::{Group, GroupState};
+        use mdk_storage_traits::groups::types::{Group, GroupState, SelfUpdateState};
         use mdk_storage_traits::mls_codec::MlsCodec;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/mdk-sqlite-storage/src/lib.rs` around lines 3856 - 3884, The test
module snapshot_openmls_tests relies on use super::* for SelfUpdateState but
should import it explicitly and add round-trip cases for non-default states;
update the module imports to include SelfUpdateState (alongside Group and
GroupState) and extend create_test_group / the snapshot test cases to exercise
at least one Group with SelfUpdateState::Required and one with
SelfUpdateState::CompletedAt(ts) so the snapshot serialization of
last_self_update_at (Required -> Some(0), CompletedAt -> Some(ts)) is validated
end-to-end.