feat: validation + NIP-40 expiration + UniFFI surface for disappearing messages (2/3)#306
Merged
Merged
Conversation
…aring messages (2/3) Part 2 of splitting #253. Builds on the v3 wire format + storage groundwork landed in #258. Adds: - create_group / update_group_data reject Some(0); callers use None or Some(None) to disable. - build_message_event auto-inserts a NIP-40 expiration tag on every outer kind:445 wrapper (messages, proposals, commits) when the group has a disappearing_message_secs set. If the caller also supplies an expiration, the earlier of (caller, group) wins — the caller can request a shorter lifetime but never extend the group's setting. The wrapper's created_at and the expiration math share a single Timestamp::now() snapshot so they cannot drift. - UniFFI: disappearing_message_secs on the Group and GroupDataUpdate records, new parameter on the create_group binding. Part 3 will add MessageStorage::delete_message and friends, the corresponding UniFFI exports, and PRAGMA secure_delete=ON.
dannym-arx
requested review from
erskingardner,
jgmontoya and
mubarakcoded
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds disappearing-message duration handling: rejects zero-duration encodings on create/update, computes/attaches NIP-40 ChangesDisappearing Messages with NIP-40 Expiration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Coverage: 94.51% → 94.52% (+0.01%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-uniffi/src/lib.rs (1)
1079-1102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
Some(0)at the UniFFI boundary to preserve the declared FFI contract.
disappearing_message_secsis forwarded directly in both create/update flows. With currentFrom<MdkError>mapping, downstream validation won’t reliably surface asMdkUniffiError::InvalidInputat the boundary.🛠️ Suggested fix
pub fn create_group( &self, @@ admins: Vec<String>, disappearing_message_secs: Option<u64>, ) -> Result<CreateGroupResult, MdkUniffiError> { + if disappearing_message_secs == Some(0) { + return Err(MdkUniffiError::InvalidInput( + "disappearing_message_secs must be > 0 or None".to_string(), + )); + } + let creator_pubkey = parse_public_key(&creator_public_key)?; @@ pub fn update_group_data( @@ ) -> Result<UpdateGroupResult, MdkUniffiError> { + if matches!(update.disappearing_message_secs, Some(Some(0))) { + return Err(MdkUniffiError::InvalidInput( + "disappearing_message_secs must be > 0 when provided".to_string(), + )); + } + let group_id = parse_group_id(&mls_group_id)?;Also applies to: 1323-1325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mdk-uniffi/src/lib.rs` around lines 1079 - 1102, The UniFFI boundary must explicitly validate disappearing_message_secs so that Some(0) is rejected as InvalidInput before constructing NostrGroupConfigData; update the create and update entry points (the function building NostrGroupConfigData in lib.rs, and the analogous update function around lines ~1323-1325) to check disappearing_message_secs: if Some(0) return Err(MdkUniffiError::InvalidInput(...)) rather than forwarding it into NostrGroupConfigData::new; ensure the error uses the MdkUniffiError variant expected at the FFI boundary so callers receive InvalidInput consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mdk-core/CHANGELOG.md`:
- Around line 38-39: Update the two changelog bullets that end with "Part 2 of
`#253`" to append the proper PR reference for this change: replace the trailing
"Part 2 of `#253`" text with "Part 2 of `#253`.
([`#306`](https://github.com/marmot-protocol/mdk/pull/306))" so each entry ends
with the required markdown PR link; locate the lines in CHANGELOG.md containing
the phrases "create_group and update_group_data now reject" and "Outer kind:445
wrappers built by build_message_event" and update their endings accordingly.
In `@crates/mdk-core/src/groups.rs`:
- Line 8732: The test-local `use nostr::{TagKind, TagStandard, Timestamp};` must
be moved out of the test function and placed in the module-level `use` block;
locate the test containing that inline import in groups.rs and remove the
in-function `use` and add the same `use nostr::{TagKind, TagStandard,
Timestamp};` to the top of the containing module's `use` section (alongside
other `use nostr::...` entries) so all imports live at the module scope per the
repo guideline.
In `@crates/mdk-core/src/messages/create.rs`:
- Around line 966-967: Move the repeated function-local import `use
nostr::TagStandard;` out of the test functions and add a single `use
nostr::TagStandard;` at the top of the module scope so it is available to all
tests; remove the inline `use nostr::TagStandard;` statements inside the test
bodies (the tests that currently contain that import) and rely on the
module-level import instead. Ensure there are no other function/block-local `use
nostr::TagStandard;` occurrences left in the file.
In `@crates/mdk-uniffi/CHANGELOG.md`:
- Around line 33-34: The changelog entries for Mdk::create_group and
GroupDataUpdate currently end with an issue reference (`#253`); update both
entries in CHANGELOG.md to append the mandatory PR link suffix (e.g., "See PR
#<number>" or the repository's standard "[#<PR>](...)" format) referencing the
correct PR number(s) that implemented these changes so they no longer point to
issue `#253` and comply with the **/CHANGELOG.md** guideline; ensure both the
Mdk::create_group line and the GroupDataUpdate line include the PR link format
used elsewhere in the file.
---
Outside diff comments:
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 1079-1102: The UniFFI boundary must explicitly validate
disappearing_message_secs so that Some(0) is rejected as InvalidInput before
constructing NostrGroupConfigData; update the create and update entry points
(the function building NostrGroupConfigData in lib.rs, and the analogous update
function around lines ~1323-1325) to check disappearing_message_secs: if Some(0)
return Err(MdkUniffiError::InvalidInput(...)) rather than forwarding it into
NostrGroupConfigData::new; ensure the error uses the MdkUniffiError variant
expected at the FFI boundary so callers receive InvalidInput consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82577860-c8bc-40f6-b293-050b2d687811
📒 Files selected for processing (6)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/extension/types.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/messages/create.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
- Append PR #306 link to Part-2 changelog entries (mdk-core + mdk-uniffi) - Hoist in-function nostr imports (TagKind, TagStandard, Timestamp) to module-level use blocks in groups.rs tests and messages/create.rs tests
…nput The CHANGELOG for #306 claims that `create_group` and `update_group_data` surface `Some(0)` (and `Some(Some(0))` on updates) as `MdkUniffiError::InvalidInput`, but the rejection actually happens in `mdk_core`, which returns `Error::Group`. The `impl From<MdkError> for MdkUniffiError` then maps every `MdkError` to `MdkUniffiError::Mdk(...)`, so mobile clients pattern-matching on the error variant get the catch-all `Mdk` instead of the documented `InvalidInput`. Add an explicit pre-check at the FFI boundary, matching how other parameter-format errors are reported in the uniffi crate (e.g. invalid relay URLs, invalid admin keys). The CHANGELOG contract now holds. Adds binding-level tests asserting the `InvalidInput` variant for both paths, following the existing `test_create_group_invalid_*` / `test_update_group_data_invalid_*` style.
dannym-arx
added a commit
that referenced
this pull request
May 22, 2026
…PR refs The cherry-pick from the original combined PR #253 placed the deletion-API and secure_delete entries in the [0.8.0] section because that PR predated the 0.8.0 release cut. Move them to Unreleased and append the #315 PR link, matching the convention used for Part 1 (#258) and Part 2 (#306). Also adds the missing mdk-uniffi changelog entry for the new deletion bindings.
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
2 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.
Step 2 of 3: splitting #253. Part 1 (#258) landed the v3 wire format and storage column. This PR adds validation, the NIP-40 expiration tag, and the UniFFI surface. Part 3 handles deletion.
What changed
Validation.
create_groupandupdate_group_datarejectSome(0)withError::Group. Callers passNone(orSome(None)on the update path) to disable. The extension constructor keeps its silent zero filter as a backstop; surfaced errors live at the public API where callers can act on them.NIP-40 expiration on outer wrappers.
build_message_eventbuilds every outer kind:445 event: messages, proposals, commits. When the group has adisappearing_message_secssetting, the wrapper now carries anexpirationtag. If the caller also supplies one (say, an ephemeral location burst), the earlier of the two wins. Group sets a ceiling; caller can ask for shorter but never longer. Bothcreated_atand the expiration math share oneTimestamp::now()snapshot so they cannot drift.UniFFI surface. The
Grouprecord now exposesdisappearing_message_secs: Option<UInt64>. OnGroupDataUpdate, the field isOption<Option<UInt64>>(outer = specified, inner = enabled). Forcreate_group, the binding takes a newdisappearing_message_secsparameter. Mobile clients can read and write the duration through the binding now that the field is plumbed on both sides.What this PR does NOT add
PRAGMA secure_delete=ON(Part 3)Files touched (6)
mdk-core/src/groups.rsmdk-core/src/messages/create.rsmdk-uniffi/src/lib.rsmdk-core/src/extension/types.rs8 new tests pass. Full workspace suite (458 tests) green. Precommit (fmt, clippy, docs, cargo audit) passes on stable and MSRV.
A second batch of marmots, this one teaching messages how to stop existing on schedule.
This PR (step 2 of 3 for disappearing messages) adds validation for disappearing-message configuration, ensures outer wrapper events receive deterministic NIP-40 expiration tags when a group configures disappearing messages, and exposes the disappearing-message setting through the UniFFI surface so mobile clients can read/write it.
What changed:
Security impact:
⚠️ Security-sensitive changes
Protocol changes:
API surface:
Testing: