feat: add SelfRemove proposal support for voluntary group departure (MIP-03)#236
Merged
feat: add SelfRemove proposal support for voluntary group departure (MIP-03)#236
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MLS SelfRemove support: new proposal constants and Nostr tag formatting, mixed-ciphertext defaults for group create/join, SelfRemove proposal handling with auto-commit, expanded non-admin commit authorization with admin-depletion validation, and a KeyPackage Changes
Sequence Diagram(s)sequenceDiagram
participant Member
participant Group as "MLS Group"
participant Processor as "MessageProcessor"
participant Validator
Member->>Group: try_self_remove(signer)
Group->>Group: set_configuration(MIXED_PLAINTEXT_WIRE_FORMAT_POLICY)
Group->>Group: leave_group_via_self_remove() -> produce SelfRemove message
Group->>Group: restore_configuration(MIXED_CIPHERTEXT_WIRE_FORMAT_POLICY)
Group-->>Member: MlsMessageOut (SelfRemove)
Member->>Processor: process_proposal(SelfRemove)
Processor->>Validator: validate_admin_depletion(departing_leaves)
alt validation fails
Validator-->>Processor: Error
Processor-->>Member: IgnoredProposal (rejection)
else validation succeeds
Validator-->>Processor: Ok
Processor->>Group: auto_commit_proposal()
Processor-->>Member: MessageProcessingResult::Proposal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
UniFFI Binding Coverage10 public method(s) not bound in UniFFI Coverage: 31/41 bindable methods (75.6%)
|
| Method | Source |
|---|---|
delete_key_package_from_storage |
crates/mdk-core/src/key_packages.rs:603 |
delete_key_package_from_storage_by_hash_ref |
crates/mdk-core/src/key_packages.rs:618 |
get_ratchet_tree_info |
crates/mdk-core/src/groups.rs:600 |
group_leaf_map |
crates/mdk-core/src/groups.rs:566 |
own_leaf_index |
crates/mdk-core/src/groups.rs:557 |
pending_added_members_pubkeys |
crates/mdk-core/src/groups.rs:656 |
pending_member_changes |
crates/mdk-core/src/groups.rs:724 |
pending_removed_members_pubkeys |
crates/mdk-core/src/groups.rs:689 |
prepare_group_image_for_upload_with_options |
crates/mdk-core/src/extension/group_image.rs:459 |
process_message_with_context |
crates/mdk-core/src/messages/process.rs:327 |
✅ Bound methods (31)
| Method | Source |
|---|---|
accept_welcome |
crates/mdk-core/src/welcomes.rs:315 |
add_members |
crates/mdk-core/src/groups.rs:770 |
clear_pending_commit |
crates/mdk-core/src/groups.rs:1654 |
create_group |
crates/mdk-core/src/groups.rs:1208 |
create_key_package_for_event |
crates/mdk-core/src/key_packages.rs:54 |
create_key_package_for_event_with_options |
crates/mdk-core/src/key_packages.rs:92 |
create_message |
crates/mdk-core/src/messages/create.rs:79 |
decline_welcome |
crates/mdk-core/src/welcomes.rs:350 |
decrypt_group_image |
crates/mdk-core/src/extension/group_image.rs:227 |
derive_upload_keypair |
crates/mdk-core/src/extension/group_image.rs:332 |
get_group |
crates/mdk-core/src/groups.rs:496 |
get_groups |
crates/mdk-core/src/groups.rs:508 |
get_last_message |
crates/mdk-core/src/messages/mod.rs:278 |
get_members |
crates/mdk-core/src/groups.rs:542 |
get_message |
crates/mdk-core/src/messages/mod.rs:211 |
get_messages |
crates/mdk-core/src/messages/mod.rs:250 |
get_pending_welcomes |
crates/mdk-core/src/welcomes.rs:72 |
get_relays |
crates/mdk-core/src/groups.rs:1154 |
get_welcome |
crates/mdk-core/src/welcomes.rs:40 |
groups_needing_self_update |
crates/mdk-core/src/groups.rs:526 |
leave_group |
crates/mdk-core/src/groups.rs:1601 |
merge_pending_commit |
crates/mdk-core/src/groups.rs:1705 |
parse_key_package |
crates/mdk-core/src/key_packages.rs:268 |
prepare_group_image_for_upload |
crates/mdk-core/src/extension/group_image.rs:413 |
process_message |
crates/mdk-core/src/messages/process.rs:320 |
process_welcome |
crates/mdk-core/src/welcomes.rs:178 |
remove_members |
crates/mdk-core/src/groups.rs:868 |
self_demote |
crates/mdk-core/src/groups.rs:1547 |
self_update |
crates/mdk-core/src/groups.rs:1380 |
sync_group_metadata_from_mls |
crates/mdk-core/src/groups.rs:1776 |
update_group_data |
crates/mdk-core/src/groups.rs:1088 |
➖ Intentionally excluded (5)
| Method | Reason |
|---|---|
builder |
Wrapped by new_mdk(), new_mdk_with_key(), new_mdk_unencrypted() free functions |
load_mls_group |
Only available behind debug-examples feature flag, not for production use |
media_manager |
Returns borrowed reference with lifetime — cannot cross FFI boundary |
migrate_group_image_v1_to_v2 |
Migration utility for internal use only, not needed in client bindings |
new |
Wrapped by new_mdk(), new_mdk_with_key(), new_mdk_unencrypted() free functions |
❌ Coverage: 91.43% → 91.22% (-0.21%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/mdk-core/src/key_packages.rs (2)
109-112: Update the tag count in the internal documentation.The comment references outdated tag counts (7/8) that should be updated to reflect the new
mls_proposalstag (8/9).📝 Suggested documentation update
/// The `protected` parameter controls whether the NIP-70 protected tag (`["-"]`) is /// included in the output tags. When `true`, the tag is inserted between the `i` -/// and `client` tags, resulting in 8 total tags. When `false`, the protected tag is -/// omitted, resulting in 7 total tags. +/// and `client` tags, resulting in 9 total tags. When `false`, the protected tag is +/// omitted, resulting in 8 total tags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/key_packages.rs` around lines 109 - 112, Update the inline doc comment that describes tag counts: the description of the `protected` parameter and the NIP-70 protected tag currently lists totals as 7/8; adjust these counts to 8/9 to account for the new `mls_proposals` tag and ensure the wording mentions the added `mls_proposals` tag between the `client`/`i` tags where appropriate so the comment correctly reflects the new tag totals and the presence/absence of the protected tag.
37-44: Update documentation to include the newmls_proposalstag.The tag list in the doc comment should include the new
mls_proposalstag to match the actual output.📝 Suggested documentation update
/// * A vector of tags for the Nostr event: /// - `mls_protocol_version` - MLS protocol version (e.g., "1.0") /// - `mls_ciphersuite` - Ciphersuite identifier (e.g., "0x0001") /// - `mls_extensions` - Required MLS extensions +/// - `mls_proposals` - Supported proposal types (e.g., "0x000a") /// - `relays` - Relay URLs for distribution /// - `i` - Hex-encoded KeyPackageRef for efficient relay queries (MIP-00) /// - `client` - Client identifier and version /// - `encoding` - The encoding format tag ("base64")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/key_packages.rs` around lines 37 - 44, Update the doc comment that lists the Nostr event tags (the vector-of-tags comment block in crates::mdk_core::key_packages.rs) to include the new mls_proposals tag; specifically add an entry for `mls_proposals` alongside the existing entries (`mls_protocol_version`, `mls_ciphersuite`, `mls_extensions`, `relays`, `i`, `client`, `encoding`) so the documentation matches the actual output produced by the KeyPackage/Nostr event code.crates/mdk-core/src/messages/proposal.rs (1)
388-455: Test correctly validates non-admin auto-commit for SelfRemove.The test properly verifies that Charlie (non-admin) can auto-commit Bob's SelfRemove proposal, which is the key behavioral difference from legacy Remove proposals.
Consider adding a complementary test for the admin depletion rejection case — where a SelfRemove is rejected because it would leave the group with no admins.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/proposal.rs` around lines 388 - 455, Add a complementary test that ensures a SelfRemove is rejected when it would leave the group with no admins: create a group where the only admin is the leaver (use Keys::generate(), create_test_mdk(), create_group with create_nostr_group_config_data(admins)), have that admin call leave_group(), have another member call process_message(...) on the admin's evolution_event, and assert the result is a rejection (match MessageProcessingResult::Rejected or the appropriate rejection variant) instead of Proposal; name the test e.g. test_self_remove_rejected_due_to_admin_depletion to mirror test_self_remove_proposal_auto_committed_by_non_admin.crates/mdk-core/src/messages/validation.rs (1)
214-225: Consider adding unit tests for the new helper functions.The new
is_self_remove_only_commitandvalidate_admin_depletionfunctions lack direct unit tests in this file. Consider adding tests for:
is_self_remove_only_commit: empty proposals (should return false), single SelfRemove, multiple SelfRemoves, mixed proposal typesvalidate_admin_depletion: single admin departing from single-admin group, non-admin departing, partial admin departure leaving at least one adminWould you like me to generate unit test stubs for these helper functions, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/messages/validation.rs` around lines 214 - 225, Add unit tests covering is_self_remove_only_commit and validate_admin_depletion: for is_self_remove_only_commit add cases for an empty StagedCommit (expect false), a single queued Proposal::SelfRemove (true), multiple SelfRemove proposals (true), and mixed proposal types including at least one non-SelfRemove (false); for validate_admin_depletion add cases for a single admin leaving a single-admin group (should error/indicate depletion), a non-admin departing (no depletion), and partial admin departures that leave at least one admin (no depletion). Use the existing StagedCommit/queued_proposals and Proposal enum constructors used in the module to build test inputs and assert expected boolean/result conditions. Ensure tests live alongside other unit tests for these helpers and use the module-private visibility (pub(super)) access pattern as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 109-112: Update the inline doc comment that describes tag counts:
the description of the `protected` parameter and the NIP-70 protected tag
currently lists totals as 7/8; adjust these counts to 8/9 to account for the new
`mls_proposals` tag and ensure the wording mentions the added `mls_proposals`
tag between the `client`/`i` tags where appropriate so the comment correctly
reflects the new tag totals and the presence/absence of the protected tag.
- Around line 37-44: Update the doc comment that lists the Nostr event tags (the
vector-of-tags comment block in crates::mdk_core::key_packages.rs) to include
the new mls_proposals tag; specifically add an entry for `mls_proposals`
alongside the existing entries (`mls_protocol_version`, `mls_ciphersuite`,
`mls_extensions`, `relays`, `i`, `client`, `encoding`) so the documentation
matches the actual output produced by the KeyPackage/Nostr event code.
In `@crates/mdk-core/src/messages/proposal.rs`:
- Around line 388-455: Add a complementary test that ensures a SelfRemove is
rejected when it would leave the group with no admins: create a group where the
only admin is the leaver (use Keys::generate(), create_test_mdk(), create_group
with create_nostr_group_config_data(admins)), have that admin call
leave_group(), have another member call process_message(...) on the admin's
evolution_event, and assert the result is a rejection (match
MessageProcessingResult::Rejected or the appropriate rejection variant) instead
of Proposal; name the test e.g. test_self_remove_rejected_due_to_admin_depletion
to mirror test_self_remove_proposal_auto_committed_by_non_admin.
In `@crates/mdk-core/src/messages/validation.rs`:
- Around line 214-225: Add unit tests covering is_self_remove_only_commit and
validate_admin_depletion: for is_self_remove_only_commit add cases for an empty
StagedCommit (expect false), a single queued Proposal::SelfRemove (true),
multiple SelfRemove proposals (true), and mixed proposal types including at
least one non-SelfRemove (false); for validate_admin_depletion add cases for a
single admin leaving a single-admin group (should error/indicate depletion), a
non-admin departing (no depletion), and partial admin departures that leave at
least one admin (no depletion). Use the existing StagedCommit/queued_proposals
and Proposal enum constructors used in the module to build test inputs and
assert expected boolean/result conditions. Ensure tests live alongside other
unit tests for these helpers and use the module-private visibility (pub(super))
access pattern as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14b583ac-3f12-4362-b913-282818b6d18d
📒 Files selected for processing (9)
crates/mdk-core/src/constant.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/lib.rscrates/mdk-core/src/messages/commit.rscrates/mdk-core/src/messages/proposal.rscrates/mdk-core/src/messages/validation.rscrates/mdk-core/src/util.rscrates/mdk-core/src/welcomes.rs
👮 Files not reviewed due to content moderation or server errors (1)
- crates/mdk-core/src/groups.rs
❌ Coverage: 91.43% → 91.2% (-0.23%) |
❌ Coverage: 91.43% → 91.25% (-0.18%) |
erskingardner
requested changes
Mar 26, 2026
❌ Coverage: 91.43% → 91.29% (-0.14%) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
170-173: Prefer using the shared proposal-tag constant instead of a string literal.Using a shared constant here avoids protocol-string drift across files and tests.
Proposed refactor
-use crate::constant::{DEFAULT_CIPHERSUITE, TAG_EXTENSIONS}; +use crate::constant::{DEFAULT_CIPHERSUITE, TAG_EXTENSIONS, TAG_PROPOSALS}; let mut tags = vec![ Tag::custom(TagKind::MlsProtocolVersion, ["1.0"]), Tag::custom(TagKind::MlsCiphersuite, [self.ciphersuite_value()]), Tag::custom(TagKind::MlsExtensions, self.extensions_value()), Tag::custom( - TagKind::Custom("mls_proposals".into()), + TagKind::Custom(TAG_PROPOSALS.into()), self.proposals_value(), ), Tag::relays(relays), Tag::custom(TagKind::i(), [key_package_ref_hex]), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/key_packages.rs` around lines 170 - 173, Replace the string literal "mls_proposals" in the Tag::custom call with the shared proposal-tag constant used across the crate (instead of hardcoding the protocol string), i.e. use the canonical constant (import it if needed) in place of "mls_proposals". Keep the surrounding code intact (Tag::custom(TagKind::Custom(...), self.proposals_value())) and ensure the constant is converted the same way the literal was (e.g., .into() or as required by TagKind::Custom).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 170-173: Replace the string literal "mls_proposals" in the
Tag::custom call with the shared proposal-tag constant used across the crate
(instead of hardcoding the protocol string), i.e. use the canonical constant
(import it if needed) in place of "mls_proposals". Keep the surrounding code
intact (Tag::custom(TagKind::Custom(...), self.proposals_value())) and ensure
the constant is converted the same way the literal was (e.g., .into() or as
required by TagKind::Custom).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 579e71da-c7d8-49df-a691-34a154ce34fb
📒 Files selected for processing (4)
crates/mdk-core/src/constant.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/messages/proposal.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/src/constant.rs
👮 Files not reviewed due to content moderation or server errors (2)
- crates/mdk-core/src/messages/proposal.rs
- crates/mdk-core/src/groups.rs
jgmontoya
force-pushed
the
feat/auto-commit-leave
branch
from
f5b7d77 to
89201bf
Compare
❌ Coverage: 91.43% → 91.27% (-0.16%) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 1496-1503: The restore-of-ciphertext-mode inside leave_group
currently logs failures from group.set_configuration(self.storage(),
&ciphertext_config) and continues; change this so failures are propagated
instead of swallowed: update leave_group to return an Err when
group.set_configuration(...) fails (use ? or map_err to convert the error into
the function's error type), or if you prefer a recovery path, attempt to
reload/reset the group state from persistent storage via self.storage() before
returning an Err; reference the set_configuration call, ciphertext_config
variable, and leave_group function and ensure the error is surfaced to the
caller rather than only logged.
- Around line 5595-5608: The test currently checks
charlie_mdk.pending_member_changes(&group_id) which only tracks
Proposal::Add/Remove and cannot detect a queued Proposal::SelfRemove; update the
assertion to directly inspect the pending proposals or commit and re-check
membership: call charlie_mdk.pending_proposals(&group_id) (or equivalent) and
assert there is no Proposal::SelfRemove for Bob (using bob's id from
bob_leave_result) or alternatively apply/merge the generated commit via
charlie_mdk.process_message(...) / charlie_mdk.merge_commit(...) and then assert
Bob is no longer in the group membership instead of relying on
pending_member_changes().
🪄 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: bb308ffb-f155-4970-bfd9-063b259de7d5
📒 Files selected for processing (4)
crates/mdk-core/src/constant.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/messages/proposal.rs
✅ Files skipped from review due to trivial changes (1)
- crates/mdk-core/src/messages/proposal.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/src/key_packages.rs
jgmontoya
force-pushed
the
feat/auto-commit-leave
branch
from
89201bf to
7874fb8
Compare
❌ Coverage: 91.43% → 91.33% (-0.10%) |
❌ Coverage: 91.51% → 91.4% (-0.11%) |
❌ Coverage: 91.51% → 91.3% (-0.21%) |
jgmontoya
force-pushed
the
feat/auto-commit-leave
branch
from
c3bbbfa to
de16d64
Compare
❌ Coverage: 91.57% → 91.32% (-0.25%) |
erskingardner
requested changes
Mar 30, 2026
❌ Coverage: 91.57% → 91.36% (-0.21%) |
…lfRemove-only, and test receiving-side admin rejection
jgmontoya
force-pushed
the
feat/auto-commit-leave
branch
from
a7e6747 to
668de3f
Compare
❌ Coverage: 91.57% → 91.36% (-0.21%) |
dannym-arx
approved these changes
Mar 31, 2026
mubarakcoded
approved these changes
Mar 31, 2026
erskingardner
approved these changes
Mar 31, 2026
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
4 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 implements MIP-03 SelfRemove proposal support so members can voluntarily leave groups via SelfRemove proposals that are auto-committed, removing the need for an admin-initiated Remove while ensuring departures cannot deplete the admin set. It also advertises SelfRemove capability in MLS capabilities and KeyPackage Nostr tags and adjusts wire-format defaults to accept SelfRemove as PublicMessage for interoperability.
What changed:
Security impact:
Protocol changes:
API surface:
Testing: