feat(mip-00): migrate KeyPackage events to addressable kind:30443#233
feat(mip-00): migrate KeyPackage events to addressable kind:30443#233dannym-arx merged 11 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements MIP-00 KeyPackage event kind migration, moving MLS KeyPackage events from Nostr Changes
Sequence DiagramsequenceDiagram
participant Caller
participant MDK as MDK Core
participant KeyPackageBuilder
participant EventFactory as Nostr Event
Caller->>MDK: create_key_package_for_event()
MDK->>KeyPackageBuilder: Build key package content
KeyPackageBuilder->>KeyPackageBuilder: Generate random 32-byte d tag
KeyPackageBuilder->>KeyPackageBuilder: Create tags_30443 (with d tag)
KeyPackageBuilder->>KeyPackageBuilder: Create tags_443 (without d tag)
KeyPackageBuilder-->>MDK: Return KeyPackageEventData{content, tags_30443, tags_443, d_tag, hash_ref}
MDK-->>Caller: KeyPackageEventData
Caller->>EventFactory: Build event with kind 30443
Caller->>EventFactory: Add tags_30443
EventFactory-->>Caller: Event 30443
Caller->>EventFactory: Sign event
Caller->>EventFactory: Build event with kind 443 (legacy)
Caller->>EventFactory: Add tags_443
EventFactory-->>Caller: Event 443
Caller->>EventFactory: Sign event
Caller->>Caller: Publish both events until May 1, 2026
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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:731 |
delete_key_package_from_storage_by_hash_ref |
crates/mdk-core/src/key_packages.rs:746 |
get_ratchet_tree_info |
crates/mdk-core/src/groups.rs:559 |
group_leaf_map |
crates/mdk-core/src/groups.rs:525 |
own_leaf_index |
crates/mdk-core/src/groups.rs:516 |
pending_added_members_pubkeys |
crates/mdk-core/src/groups.rs:615 |
pending_member_changes |
crates/mdk-core/src/groups.rs:683 |
pending_removed_members_pubkeys |
crates/mdk-core/src/groups.rs:648 |
prepare_group_image_for_upload_with_options |
crates/mdk-core/src/extension/group_image.rs:463 |
process_message_with_context |
crates/mdk-core/src/messages/process.rs:332 |
✅ Bound methods (31)
| Method | Source |
|---|---|
accept_welcome |
crates/mdk-core/src/welcomes.rs:315 |
add_members |
crates/mdk-core/src/groups.rs:729 |
clear_pending_commit |
crates/mdk-core/src/groups.rs:1613 |
create_group |
crates/mdk-core/src/groups.rs:1167 |
create_key_package_for_event |
crates/mdk-core/src/key_packages.rs:88 |
create_key_package_for_event_with_options |
crates/mdk-core/src/key_packages.rs:127 |
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:229 |
derive_upload_keypair |
crates/mdk-core/src/extension/group_image.rs:334 |
get_group |
crates/mdk-core/src/groups.rs:455 |
get_groups |
crates/mdk-core/src/groups.rs:467 |
get_last_message |
crates/mdk-core/src/messages/mod.rs:288 |
get_members |
crates/mdk-core/src/groups.rs:501 |
get_message |
crates/mdk-core/src/messages/mod.rs:221 |
get_messages |
crates/mdk-core/src/messages/mod.rs:260 |
get_pending_welcomes |
crates/mdk-core/src/welcomes.rs:72 |
get_relays |
crates/mdk-core/src/groups.rs:1113 |
get_welcome |
crates/mdk-core/src/welcomes.rs:40 |
groups_needing_self_update |
crates/mdk-core/src/groups.rs:485 |
leave_group |
crates/mdk-core/src/groups.rs:1560 |
merge_pending_commit |
crates/mdk-core/src/groups.rs:1664 |
parse_key_package |
crates/mdk-core/src/key_packages.rs:330 |
prepare_group_image_for_upload |
crates/mdk-core/src/extension/group_image.rs:416 |
process_message |
crates/mdk-core/src/messages/process.rs:325 |
process_welcome |
crates/mdk-core/src/welcomes.rs:178 |
remove_members |
crates/mdk-core/src/groups.rs:827 |
self_demote |
crates/mdk-core/src/groups.rs:1506 |
self_update |
crates/mdk-core/src/groups.rs:1339 |
sync_group_metadata_from_mls |
crates/mdk-core/src/groups.rs:1735 |
update_group_data |
crates/mdk-core/src/groups.rs:1047 |
➖ 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.4% → 91.39% (-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: 2
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
177-184: Move the RNG import to module scope.The new inner
usebreaks the repo's Rust style rule. Keep theOsRng/RngCoreimport with the other module imports instead.♻️ Small cleanup
+use nostr::secp256k1::rand::{rngs::OsRng, RngCore}; @@ - use nostr::secp256k1::rand::{RngCore, rngs::OsRng}; let mut d_bytes = [0u8; 32];As per coding guidelines, "All
usestatements at the top of their scope, never inside functions."🤖 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 177 - 184, The local `use` for OsRng and RngCore inside the key package generation should be moved to module scope to satisfy the repo style rule; remove the inner `use nostr::secp256k1::rand::{RngCore, rngs::OsRng};` and instead add that import alongside the other top-level imports in the module so the code that fills `d_bytes` and creates `d_value` continues to call `OsRng.fill_bytes(&mut d_bytes)` unchanged (refer to symbols `OsRng`, `RngCore`, `d_bytes`, and `d_value` to locate the code).
🤖 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/key_packages.rs`:
- Around line 287-318: Add regression tests in the key_packages module that
exercise the three new branches around event.kind and d-tag handling: (1) a test
that constructs an event with kind == MLS_KEY_PACKAGE_KIND_LEGACY and verifies
parsing succeeds, (2) a test that constructs an event with kind ==
MLS_KEY_PACKAGE_KIND (addressable kind) but missing the `d` tag and asserts it
returns Err(Error::KeyPackage) for a missing d tag, and (3) a test that
constructs kind == MLS_KEY_PACKAGE_KIND with a `d` tag whose value is empty and
asserts it returns Err(Error::KeyPackage) for empty d value; target the same
parsing function/entrypoint used by the changed code (the code that checks
event.kind against MLS_KEY_PACKAGE_KIND and MLS_KEY_PACKAGE_KIND_LEGACY and the
d_tag match block) and assert the exact Error variants (Error::UnexpectedEvent
and Error::KeyPackage) so these branches stay covered until the legacy
acceptance is removed.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 1084-1093: The docs for KeyPackageResult are misleading: update
the comments for the tags and d_tag fields on the KeyPackageResult struct so
tags is described as the structured UniFFI tag list (Vec<Vec<String>>), not
JSON-encoded, and clarify that reusing d_tag during rotation requires callers to
replace the generated ["d", "..."] entry in tags with the same d_tag value
before signing the new KeyPackage (otherwise the relay will treat it as a new
slot); reference the KeyPackageResult::tags and KeyPackageResult::d_tag fields
when making this change.
---
Nitpick comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 177-184: The local `use` for OsRng and RngCore inside the key
package generation should be moved to module scope to satisfy the repo style
rule; remove the inner `use nostr::secp256k1::rand::{RngCore, rngs::OsRng};` and
instead add that import alongside the other top-level imports in the module so
the code that fills `d_bytes` and creates `d_value` continues to call
`OsRng.fill_bytes(&mut d_bytes)` unchanged (refer to symbols `OsRng`, `RngCore`,
`d_bytes`, and `d_value` to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da4e4ce3-043e-44d9-898e-d95311b194e0
📒 Files selected for processing (10)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/examples/group_inspection.rscrates/mdk-core/examples/key_package_inspection.rscrates/mdk-core/examples/mls_memory.rscrates/mdk-core/examples/mls_sqlite.rscrates/mdk-core/src/constant.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/test_util.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
396-406:⚠️ Potential issue | 🟡 MinorAdd a UniFFI assertion for the new
d_tagfield.These methods now surface
d_tag, but the binding tests still do not check that it is populated, matches the["d", ...]entry intags, and is wired through on thewith_optionspath too.As per coding guidelines, "ensure test coverage does not decrease (CI fails if it does)."
Also applies to: 429-439
🤖 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 396 - 406, The new KeyPackageResult includes d_tag but tests don't assert it; update the UniFFI binding tests that exercise create_key_package_for_event (and the with_options path variant) to assert that KeyPackageResult.d_tag is populated, equals the "d" tag entry inside the returned tags (i.e. tags contains an entry whose first element is "d" and the rest matches d_tag), and that the with_options code path propagates the same d_tag; locate references to create_key_package_for_event, KeyPackageResult, d_tag, tags and the with_options wrapper in the binding tests and add assertions that d_tag is non-empty and matches the corresponding ["d", ...] tags element.
🧹 Nitpick comments (2)
crates/mdk-core/src/key_packages.rs (1)
177-184: Move the RNG import to module scope.The inline
usebreaks the repo's Rust style rule and makes this helper harder to scan.♻️ Proposed cleanup
+use nostr::secp256k1::rand::{rngs::OsRng, RngCore}; use nostr::{Event, PublicKey, RelayUrl, Tag, TagKind}; @@ - use nostr::secp256k1::rand::{RngCore, rngs::OsRng}; let mut d_bytes = [0u8; 32];As per coding guidelines, "All use statements at the top of their scope, never inside functions."
🤖 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 177 - 184, Move the inline import "use nostr::secp256k1::rand::{RngCore, rngs::OsRng};" out of the function and into the module scope (top of the file), then remove the inline use so the function uses OsRng and RngCore directly when filling d_bytes and creating d_value; update any references in the same file (e.g., the code that declares d_bytes, calls OsRng.fill_bytes(&mut d_bytes), and computes d_value) to rely on the module-scoped import (no other code changes required).crates/mdk-uniffi/src/lib.rs (1)
2056-2065: Factor the key-package kind literal out of the tests.
Kind::Custom(30443)is repeated in several spots. A local test constant/helper will keep these tests aligned if the shared key-package kind changes again.♻️ Suggested cleanup
#[cfg(test)] mod tests { use super::*; use nostr::{EventBuilder, JsonUtil, Keys, Kind, Tag, UnsignedEvent}; use tempfile::TempDir; + + const KEY_PACKAGE_KIND: Kind = Kind::Custom(30443); @@ - let kp_event = EventBuilder::new(Kind::Custom(30443), kp_result.key_package) + let kp_event = EventBuilder::new(KEY_PACKAGE_KIND, kp_result.key_package)Also applies to: 2170-2179, 2240-2249, 2299-2308, 2555-2564, 2869-2878
🤖 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 2056 - 2065, Multiple tests directly use the numeric literal Kind::Custom(30443); introduce a shared test constant or helper (e.g., const KEY_PACKAGE_KIND or a fn key_package_kind() -> Kind) and replace all occurrences of Kind::Custom(30443) used when building key-package events (e.g., in EventBuilder::new(...) calls and related test sites) with that constant/helper so the kind value is centralized and easy to update across tests.
🤖 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/key_packages.rs`:
- Around line 2653-2662: The test currently republishes the modern tag set under
MLS_KEY_PACKAGE_KIND_LEGACY, so to simulate a true legacy event remove the "d"
tag from the tags list before building/signing the legacy event; locate the tags
returned by mdk.create_key_package_for_event (symbols:
create_key_package_for_event, key_package_str, tags) and produce a filtered_tags
= tags without any tag whose name/key equals "d", then call
EventBuilder::new(MLS_KEY_PACKAGE_KIND_LEGACY,
key_package_str).tags(filtered_tags).sign_with_keys(&keys) to create the legacy
event.
---
Outside diff comments:
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 396-406: The new KeyPackageResult includes d_tag but tests don't
assert it; update the UniFFI binding tests that exercise
create_key_package_for_event (and the with_options path variant) to assert that
KeyPackageResult.d_tag is populated, equals the "d" tag entry inside the
returned tags (i.e. tags contains an entry whose first element is "d" and the
rest matches d_tag), and that the with_options code path propagates the same
d_tag; locate references to create_key_package_for_event, KeyPackageResult,
d_tag, tags and the with_options wrapper in the binding tests and add assertions
that d_tag is non-empty and matches the corresponding ["d", ...] tags element.
---
Nitpick comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 177-184: Move the inline import "use
nostr::secp256k1::rand::{RngCore, rngs::OsRng};" out of the function and into
the module scope (top of the file), then remove the inline use so the function
uses OsRng and RngCore directly when filling d_bytes and creating d_value;
update any references in the same file (e.g., the code that declares d_bytes,
calls OsRng.fill_bytes(&mut d_bytes), and computes d_value) to rely on the
module-scoped import (no other code changes required).
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 2056-2065: Multiple tests directly use the numeric literal
Kind::Custom(30443); introduce a shared test constant or helper (e.g., const
KEY_PACKAGE_KIND or a fn key_package_kind() -> Kind) and replace all occurrences
of Kind::Custom(30443) used when building key-package events (e.g., in
EventBuilder::new(...) calls and related test sites) with that constant/helper
so the kind value is centralized and easy to update across tests.
🪄 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: c9dd5b73-deb9-4a49-9ee8-7d2b59dac66e
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rscrates/mdk-uniffi/src/lib.rs
|
back to the drawing board, missed a key part... during the transitionary period alice who has updated her mdk version should still publish 443s so that bob who hasn't can parse them |
❌ Coverage: 91.43% → 91.42% (-0.01%) |
|
@coderabbitai run a full review. spend a lot of your focus on making sure the code is compliant with mip-00, as defined in this updated spec: https://github.com/hzrd149/marmot/blob/0442b7ca3aced6b7068321b4f9954ca66f7dbd9e/00.md be thorough in your review, make sure that a chat between alice and bob where alice has updated but bob has not can still be created |
|
Custom finishing touch recipe not found: |
|
@coderabbitai huh... what... full review... |
🧩 Analysis chainThe PR description image shows a charming pixel art marmot wearing a construction helmet, chiseling a stone with "443" while a new shiny sign reading "30443" sits nearby — a perfect visual metaphor for the migration! Let me fetch the updated MIP-00 spec and review the code thoroughly. Let me fetch the spec directly and review the relevant code. 🏁 Script executed: Length of output: 24932 🏁 Script executed: Length of output: 116356
Full Review: PR
|
| Issue | Severity | Fix Required |
|---|---|---|
mls_proposals tag missing from creation & validation |
🔴 Critical | Add ["mls_proposals", "0x000a"] to tag builder and validator |
i tag required for legacy kind:443 |
🔴 Critical | Make i optional for kind:443, compute from content if absent |
Examples don't dual-publish kind:443 |
🔴 Critical (breaks Alice→Bob) | Update all examples to also publish kind:443 |
| Legacy regression test is unrealistic | 🟡 Important | Add test with bare kind:443 without d/i tags |
| CHANGELOG missing dual-publish guidance | 🟡 Important | Add note for callers |
🧠 Learnings used
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T13:56:23.563Z
Learning: Ensure all code follows the Marmot Protocol specification exactly. Ask clarifying questions if any part of the spec is unclear
Learnt from: mubarakcoded
Repo: marmot-protocol/mdk PR: 111
File: crates/mdk-uniffi/CHANGELOG.md:31-31
Timestamp: 2026-01-07T08:18:40.637Z
Learning: In the marmot-protocol/mdk repository, when a feature spans multiple layers (core Rust API and UniFFI bindings), different PRs may be used for each layer. Each crate's CHANGELOG should reference the PR that actually modified that specific crate, not necessarily the PR that introduced the feature in the core API. For example, core API changes reference one PR while UniFFI binding exposure references a separate PR.
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-10T13:56:23.563Z
Learning: Applies to crates/*/CHANGELOG.md : Always include a PR reference at the end of each changelog entry using the format `([`#123`](https://github.com/marmot-protocol/mdk/pull/123))`. Always reference the PR number, not the issue number
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/mdk-core/examples/mls_sqlite.rs (1)
43-53:⚠️ Potential issue | 🟡 MinorDual-publishing should be demonstrated for migration period.
This comprehensive example is likely to be referenced by implementers. Currently it only publishes
kind:30443, but during the migration period callers should publish bothkind:30443andkind:443so legacy clients (Bob running old software) can still discover KeyPackages.The
KeyPackageEventDatastruct providestags_443specifically for this purpose. Consider adding a code block or comment showing how to publish the legacy event alongside the new one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/mls_sqlite.rs` around lines 43 - 53, The example currently only publishes the new KeyPackage event (Kind::Custom(30443)) using KeyPackageEventData.tags_30443; change it to dual-publish during migration by also building and signing a legacy event using KeyPackageEventData.tags_443 and Kind::Custom(443). After calling bob_mdk.create_key_package_for_event (which yields KeyPackageEventData with bob_key_package_encoded, tags_30443: tags, and tags_443), construct and sign two events: one with Kind::Custom(30443) and tags (tags_30443) as currently done, and a second with Kind::Custom(443) using tags_443 and the same bob_key_package_encoded, then publish both so legacy and new clients can discover the KeyPackage.crates/mdk-core/examples/key_package_inspection.rs (1)
78-95:⚠️ Potential issue | 🟡 MinorExample should demonstrate dual-publishing for migration interoperability.
Per the PR discussion, during the migration period (until May 1, 2026), callers SHOULD dual-publish both
kind:30443(for updated clients) andkind:443(for legacy clients). TheKeyPackageEventDatastruct exposes bothtags_30443andtags_443for this purpose, but this example only publisheskind:30443.Consider adding a comment or code demonstrating how to publish both events:
// Publish kind:30443 for updated clients (addressable, replaceable) let key_package_event_30443 = EventBuilder::new(Kind::Custom(30443), key_package_encoded.clone()) .tags(tags_30443) // ... // During migration (until May 1, 2026), also publish kind:443 for legacy clients // let key_package_event_443 = EventBuilder::new(Kind::Custom(443), key_package_encoded.clone()) // .tags(tags_443) // Note: uses tags_443 without d tag // // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/key_package_inspection.rs` around lines 78 - 95, The example only publishes Kind::Custom(30443) but should demonstrate dual-publishing for migration interoperability: add a second event build/publish path showing EventBuilder::new(Kind::Custom(443), key_package_encoded.clone()) alongside the existing EventBuilder for Kind::Custom(30443), using KeyPackageEventData's tags_30443 for the 30443 event and tags_443 for the 443 event (omitting the d-tag on the 443 variant), sign both with the same keys (as with .build(keys.public_key()).sign(&keys).await?), and include a short comment mentioning the migration period (until May 1, 2026) and why both are published.crates/mdk-core/src/key_packages.rs (3)
212-229:⚠️ Potential issue | 🔴 CriticalEmit the required
mls_proposalscapability tag.The updated MIP-00 KeyPackage shape makes
mls_proposalsa required field and says implementations MUST advertise0x000a(self_remove). This builder still omits it, and the validator never checks for it, so MDK will publish and accept KeyPackages that spec-compliant peers should reject. (github.com)🤖 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 212 - 229, The tag vector tags_30443 currently omits the required capability for mls_proposals; update the KeyPackage builder to push a Tag advertising the mls_proposals capability with the required value (0x000a for self_remove) into tags_30443 (similar to the existing Tag::custom pushes for TagKind::MlsProtocolVersion/MlsCiphersuite); locate the tags_30443 construction and add a Tag::custom with TagKind::Custom("capabilities".into()) or the appropriate TagKind for capabilities containing the mls_proposals entry (including the 0x000a value) so produced KeyPackages advertise the required capability and will be accepted by spec-compliant peers.
64-79:⚠️ Potential issue | 🟡 MinorUpdate the return docs to match
KeyPackageEventData.These
# Returnssections still describe a tuple API even though both methods now return a named struct. That rustdoc mismatch will send callers looking for positional destructuring instead of fields.Also applies to: 113-117
🤖 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 64 - 79, Docs currently describe a tuple return but the functions now return the named struct KeyPackageEventData; update the `# Returns` rustdoc for the KeyPackage-related methods to describe the fields on KeyPackageEventData (e.g., serialized_key_package_base64, tags, hash_ref_bytes, d_tag) instead of a positional tuple and include the same tag explanations (d, mls_protocol_version, mls_ciphersuite, mls_extensions, relays, i, client, encoding) so callers know to access struct fields rather than destructure by position.
415-456:⚠️ Potential issue | 🔴 CriticalDon't require
ion legacykind:443.The migration section explicitly allows legacy
kind:443KeyPackages to omit theitag and says consumers should computeKeyPackageReffrom the content when it is missing.validate_key_package_tags()still unconditionally requiresi, so any real legacy event without that tag is rejected before content parsing. (github.com)🤖 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 415 - 456, The code currently unconditionally requires the `i` tag via the `require` closure, which rejects legacy kind:443 KeyPackage events that are allowed to omit `i`; update the logic in the function containing the `require` closure (and the subsequent calls to validate_*), replacing the forced `i` requirement with an optional lookup (e.g., use event.tags.iter().find(|t| Self::is_key_package_ref_tag(self, t)) -> Option<Tag>), call `validate_key_package_ref_tag` only when that `i` tag is present, and adjust the later integrity check so when `key_package` exists but `i` is None you compute the KeyPackageRef from content (using kp.hash_ref(self.provider.crypto())) and proceed without returning an error for a missing `i` tag for legacy kind:443 events.
🧹 Nitpick comments (5)
crates/mdk-core/examples/group_inspection.rs (1)
66-90: Key package creation updated correctly, but consider adding dual-publish note.The destructuring and event creation are correct. Similar to
key_package_inspection.rs, this example only publisheskind:30443. Since this example focuses on group inspection rather than key package publishing, a brief comment noting the dual-publish requirement would suffice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/group_inspection.rs` around lines 66 - 90, Add a brief inline comment near the key package creation and event publishing (around create_key_package_for_event / KeyPackageEventData and the member1_event / member2_event builders for nostr::Kind::Custom(30443)) explaining that this example intentionally only publishes kind:30443 for group inspection and that in real usage the key-package should also be dual-published (both the key-package event and the corresponding SNS/relay publish) so reviewers understand the omission is deliberate and not a bug.crates/mdk-core/examples/mls_memory.rs (1)
36-46: Same dual-publishing consideration asmls_sqlite.rs.The API usage is correct. See the comment on
mls_sqlite.rsregarding dual-publishing during the migration period.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/mls_memory.rs` around lines 36 - 46, This duplicates the dual-publishing concern from mls_sqlite.rs: when creating and signing the key-package event via bob_mdk.create_key_package_for_event and building bob_key_package_event (using KeyPackageEventData and bob_mdk), ensure the event is published to both legacy storage and the new MLS memory path during migration (or gated behind a feature/flag) to avoid lost or inconsistent state; update the publish path where bob_key_package_event is dispatched to perform dual publish (or add a clear feature toggle and comment) so both systems receive the event until migration is complete.crates/mdk-uniffi/src/lib.rs (2)
2074-2083: Add one UniFFI test that actually usestags_legacy.All of the updated KeyPackage tests in this file still build only
kind:30443events fromkp_result.tags. Sincetags_legacyis now a public binding field, please add at least one companion test that signs it intokind:443and feeds it through the same group-creation path.Based on learnings, "Add tests for new functionality in test modules before submitting a PR."
🤖 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 2074 - 2083, Add a UniFFI test that exercises the new public field tags_legacy by building an Event with Kind::Custom(443) (instead of 30443) using kp_result.tags_legacy, signing it with the same sign_with_keys(&member_keys) flow, and passing that event through the same group-creation path used by the existing tests; locate the kp_event/EventBuilder::new usage and duplicate the test flow but replace kp_result.tags -> kp_result.tags_legacy and Kind::Custom(30443) -> Kind::Custom(443) so the signed legacy-tag event is validated by the group-creation code paths alongside the existing tests.
1102-1105: State the migration behavior next totags_legacy.The field description explains what
tags_legacycontains, but not that legacy-peer interoperability currently requires publishing that second event alongsidetags. Calling that out here would make the public UniFFI contract much harder to misuse during the migration window. (github.com)🤖 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 1102 - 1105, Update the doc comment for the public field `tags_legacy` (next to `pub tags: Vec<Vec<String>>` and `pub tags_legacy: Vec<Vec<String>>`) to explicitly state migration behavior: note that during the migration window you must publish the legacy kind:443 event alongside the new kind:30443 event (i.e., emit both `tags` and `tags_legacy`) to maintain legacy-peer interoperability, and indicate that `tags_legacy` omits the `d` tag; modify the comment in `lib.rs` where `tags_legacy` is declared to include this instruction.crates/mdk-core/src/key_packages.rs (1)
203-210: Hoist the RNG import to module scope.Line 207 adds a
useinside the function body. This repo's Rust style rules forbid block-local imports, so please move it to the top-level import section with the othernostrimports.As per coding guidelines, "All
usestatements must be placed at the TOP of their containing scope, never inside functions, methods, blocks, or individual test functions"🤖 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 203 - 210, The block-local import `use nostr::secp256k1::rand::{RngCore, rngs::OsRng};` used when generating the `d` tag (variables `d_bytes` and `d_value`) must be moved to the module-level imports with the other `nostr` uses; update the top-of-file use group to include `RngCore` and `rngs::OsRng`, remove the in-function `use`, and leave the `OsRng.fill_bytes(&mut d_bytes)` and hex encoding logic in place so `d_bytes`/`d_value` generation remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/mdk-core/examples/key_package_inspection.rs`:
- Around line 78-95: The example only publishes Kind::Custom(30443) but should
demonstrate dual-publishing for migration interoperability: add a second event
build/publish path showing EventBuilder::new(Kind::Custom(443),
key_package_encoded.clone()) alongside the existing EventBuilder for
Kind::Custom(30443), using KeyPackageEventData's tags_30443 for the 30443 event
and tags_443 for the 443 event (omitting the d-tag on the 443 variant), sign
both with the same keys (as with .build(keys.public_key()).sign(&keys).await?),
and include a short comment mentioning the migration period (until May 1, 2026)
and why both are published.
In `@crates/mdk-core/examples/mls_sqlite.rs`:
- Around line 43-53: The example currently only publishes the new KeyPackage
event (Kind::Custom(30443)) using KeyPackageEventData.tags_30443; change it to
dual-publish during migration by also building and signing a legacy event using
KeyPackageEventData.tags_443 and Kind::Custom(443). After calling
bob_mdk.create_key_package_for_event (which yields KeyPackageEventData with
bob_key_package_encoded, tags_30443: tags, and tags_443), construct and sign two
events: one with Kind::Custom(30443) and tags (tags_30443) as currently done,
and a second with Kind::Custom(443) using tags_443 and the same
bob_key_package_encoded, then publish both so legacy and new clients can
discover the KeyPackage.
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 212-229: The tag vector tags_30443 currently omits the required
capability for mls_proposals; update the KeyPackage builder to push a Tag
advertising the mls_proposals capability with the required value (0x000a for
self_remove) into tags_30443 (similar to the existing Tag::custom pushes for
TagKind::MlsProtocolVersion/MlsCiphersuite); locate the tags_30443 construction
and add a Tag::custom with TagKind::Custom("capabilities".into()) or the
appropriate TagKind for capabilities containing the mls_proposals entry
(including the 0x000a value) so produced KeyPackages advertise the required
capability and will be accepted by spec-compliant peers.
- Around line 64-79: Docs currently describe a tuple return but the functions
now return the named struct KeyPackageEventData; update the `# Returns` rustdoc
for the KeyPackage-related methods to describe the fields on KeyPackageEventData
(e.g., serialized_key_package_base64, tags, hash_ref_bytes, d_tag) instead of a
positional tuple and include the same tag explanations (d, mls_protocol_version,
mls_ciphersuite, mls_extensions, relays, i, client, encoding) so callers know to
access struct fields rather than destructure by position.
- Around line 415-456: The code currently unconditionally requires the `i` tag
via the `require` closure, which rejects legacy kind:443 KeyPackage events that
are allowed to omit `i`; update the logic in the function containing the
`require` closure (and the subsequent calls to validate_*), replacing the forced
`i` requirement with an optional lookup (e.g., use event.tags.iter().find(|t|
Self::is_key_package_ref_tag(self, t)) -> Option<Tag>), call
`validate_key_package_ref_tag` only when that `i` tag is present, and adjust the
later integrity check so when `key_package` exists but `i` is None you compute
the KeyPackageRef from content (using kp.hash_ref(self.provider.crypto())) and
proceed without returning an error for a missing `i` tag for legacy kind:443
events.
---
Nitpick comments:
In `@crates/mdk-core/examples/group_inspection.rs`:
- Around line 66-90: Add a brief inline comment near the key package creation
and event publishing (around create_key_package_for_event / KeyPackageEventData
and the member1_event / member2_event builders for nostr::Kind::Custom(30443))
explaining that this example intentionally only publishes kind:30443 for group
inspection and that in real usage the key-package should also be dual-published
(both the key-package event and the corresponding SNS/relay publish) so
reviewers understand the omission is deliberate and not a bug.
In `@crates/mdk-core/examples/mls_memory.rs`:
- Around line 36-46: This duplicates the dual-publishing concern from
mls_sqlite.rs: when creating and signing the key-package event via
bob_mdk.create_key_package_for_event and building bob_key_package_event (using
KeyPackageEventData and bob_mdk), ensure the event is published to both legacy
storage and the new MLS memory path during migration (or gated behind a
feature/flag) to avoid lost or inconsistent state; update the publish path where
bob_key_package_event is dispatched to perform dual publish (or add a clear
feature toggle and comment) so both systems receive the event until migration is
complete.
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 203-210: The block-local import `use
nostr::secp256k1::rand::{RngCore, rngs::OsRng};` used when generating the `d`
tag (variables `d_bytes` and `d_value`) must be moved to the module-level
imports with the other `nostr` uses; update the top-of-file use group to include
`RngCore` and `rngs::OsRng`, remove the in-function `use`, and leave the
`OsRng.fill_bytes(&mut d_bytes)` and hex encoding logic in place so
`d_bytes`/`d_value` generation remains unchanged.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 2074-2083: Add a UniFFI test that exercises the new public field
tags_legacy by building an Event with Kind::Custom(443) (instead of 30443) using
kp_result.tags_legacy, signing it with the same sign_with_keys(&member_keys)
flow, and passing that event through the same group-creation path used by the
existing tests; locate the kp_event/EventBuilder::new usage and duplicate the
test flow but replace kp_result.tags -> kp_result.tags_legacy and
Kind::Custom(30443) -> Kind::Custom(443) so the signed legacy-tag event is
validated by the group-creation code paths alongside the existing tests.
- Around line 1102-1105: Update the doc comment for the public field
`tags_legacy` (next to `pub tags: Vec<Vec<String>>` and `pub tags_legacy:
Vec<Vec<String>>`) to explicitly state migration behavior: note that during the
migration window you must publish the legacy kind:443 event alongside the new
kind:30443 event (i.e., emit both `tags` and `tags_legacy`) to maintain
legacy-peer interoperability, and indicate that `tags_legacy` omits the `d` tag;
modify the comment in `lib.rs` where `tags_legacy` is declared to include this
instruction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17489538-4cc7-4c8c-8006-0b24598daa4b
📒 Files selected for processing (10)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/examples/group_inspection.rscrates/mdk-core/examples/key_package_inspection.rscrates/mdk-core/examples/mls_memory.rscrates/mdk-core/examples/mls_sqlite.rscrates/mdk-core/src/constant.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/test_util.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/mdk-core/examples/key_package_inspection.rs (1)
45-55:⚠️ Potential issue | 🟠 MajorDecode the example payload as base64, not hex.
KeyPackageEventData.contentis base64, but Step 5 still feedskey_package_encodedintohex::decode(...). On valid output the example now exits before the raw-inspection section completes.🛠️ Suggested fix
+use base64::Engine; +use base64::engine::general_purpose::STANDARD as BASE64; use mdk_core::Error; use mdk_core::prelude::*; use mdk_memory_storage::MdkMemoryStorage; @@ - let key_package_bytes = hex::decode(&key_package_encoded)?; + let key_package_bytes = BASE64 + .decode(&key_package_encoded) + .map_err(|e| Error::KeyPackage(format!("Invalid base64 key package: {}", e)))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/key_package_inspection.rs` around lines 45 - 55, The example incorrectly treats KeyPackageEventData.content (key_package_encoded) as hex and calls hex::decode on it; change the decoding to base64 (e.g., use base64::decode or the crate's decode API) when converting key_package_encoded into raw bytes, update any imports accordingly, and keep the existing error handling so the raw-inspection section can run on valid output (refer to KeyPackageEventData and the variable key_package_encoded and remove/replace hex::decode usage).crates/mdk-core/src/key_packages.rs (2)
64-79:⚠️ Potential issue | 🟡 MinorUpdate the rustdoc to match the new API.
These blocks still describe tuple returns, pre-
mls_proposalstag counts, and older tag semantics. Since this is a breaking public API change, the generated docs now point callers at the wrong destructuring pattern and expected tag layout.Also applies to: 113-117, 143-150, 289-299
🤖 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 64 - 79, The rustdoc in this file still describes an old tuple return shape and pre-mls_proposals tag layout; update the documentation to match the new public API return type and current tag semantics (include the mls_proposals tag and the new tag ordering/meanings, remove the old tuple destructuring examples, and show the actual named fields or struct/enum shape now returned), and apply the same corrections to the other doc blocks noted (around the other ranges previously listed) so callers are pointed to the correct destructuring/usage pattern and tag names like mls_proposals, mls_protocol_version, mls_ciphersuite, mls_extensions, relays, i, client, encoding, and any new tag keys introduced by the breaking change.
151-220:⚠️ Potential issue | 🟠 MajorFail fast on empty relay sets.
validate_key_package_tags()rejects arelaystag with no URLs, but this builder still emits one whenrelaysis empty. That lets the public creation API returnKeyPackageEventDatathat its own parser will reject.🛠️ Suggested fix
- let mut tags_30443 = vec![ + let relay_urls: Vec<RelayUrl> = relays.into_iter().collect(); + if relay_urls.is_empty() { + return Err(Error::KeyPackage( + "Relays tag must have at least one relay URL".to_string(), + )); + } + + let mut tags_30443 = vec![ Tag::identifier(&d_value), 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()), ["0x000a"]), - Tag::relays(relays), + Tag::relays(relay_urls), 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 151 - 220, The builder currently always emits a Tag::relays even when the provided relays iterator is empty which later causes validate_key_package_tags() to reject the event; change create_key_package_for_event_internal to collect relays into a Vec first (e.g. let relays_vec: Vec<RelayUrl> = relays.into_iter().collect()), then if relays_vec.is_empty() return an error immediately (fail-fast) or otherwise push Tag::relays(relays_vec) into tags_30443; update the Tag::relays call site to use the collected Vec so an empty relays set never produces an empty relays tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/mdk-core/examples/key_package_inspection.rs`:
- Around line 45-55: The example incorrectly treats KeyPackageEventData.content
(key_package_encoded) as hex and calls hex::decode on it; change the decoding to
base64 (e.g., use base64::decode or the crate's decode API) when converting
key_package_encoded into raw bytes, update any imports accordingly, and keep the
existing error handling so the raw-inspection section can run on valid output
(refer to KeyPackageEventData and the variable key_package_encoded and
remove/replace hex::decode usage).
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 64-79: The rustdoc in this file still describes an old tuple
return shape and pre-mls_proposals tag layout; update the documentation to match
the new public API return type and current tag semantics (include the
mls_proposals tag and the new tag ordering/meanings, remove the old tuple
destructuring examples, and show the actual named fields or struct/enum shape
now returned), and apply the same corrections to the other doc blocks noted
(around the other ranges previously listed) so callers are pointed to the
correct destructuring/usage pattern and tag names like mls_proposals,
mls_protocol_version, mls_ciphersuite, mls_extensions, relays, i, client,
encoding, and any new tag keys introduced by the breaking change.
- Around line 151-220: The builder currently always emits a Tag::relays even
when the provided relays iterator is empty which later causes
validate_key_package_tags() to reject the event; change
create_key_package_for_event_internal to collect relays into a Vec first (e.g.
let relays_vec: Vec<RelayUrl> = relays.into_iter().collect()), then if
relays_vec.is_empty() return an error immediately (fail-fast) or otherwise push
Tag::relays(relays_vec) into tags_30443; update the Tag::relays call site to use
the collected Vec so an empty relays set never produces an empty relays tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4569fbb5-908a-4342-b934-468646aaa497
📒 Files selected for processing (8)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/examples/group_inspection.rscrates/mdk-core/examples/key_package_inspection.rscrates/mdk-core/examples/mls_memory.rscrates/mdk-core/examples/mls_sqlite.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/key_packages_test.rscrates/mdk-uniffi/CHANGELOG.md
✅ Files skipped from review due to trivial changes (3)
- crates/mdk-core/src/key_packages_test.rs
- crates/mdk-uniffi/CHANGELOG.md
- crates/mdk-core/examples/mls_sqlite.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/examples/mls_memory.rs
✅ Coverage: 91.51% → 91.53% (+0.02%) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/key_packages.rs (1)
52-80:⚠️ Potential issue | 🟡 MinorRefresh the rustdoc to match
KeyPackageEventDataand the migration rules.These blocks still describe the old tuple API, old tag counts, and unconditional
ihandling, and they omit the new requiredmls_proposalstag. That makes the public API docs misleading for callers publishing bothkind:30443and legacykind:443events.Also applies to: 112-118, 144-151, 289-298, 725-726
🤖 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 52 - 80, Update the rustdoc for the public API to describe the new KeyPackageEventData struct and migration rules instead of the old tuple return: replace the tuple-oriented description and outdated tag list with the current fields in KeyPackageEventData, document that `i` (KeyPackageRef) is conditional per migration rules (not unconditional), add the newly required `mls_proposals` tag, and clarify differences when publishing kind:30443 vs legacy kind:443 events so callers know which tags are mandatory vs optional; ensure all other doc blocks referenced (lines around 112-118, 144-151, 289-298, 725-726) are updated consistently.
🤖 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/key_packages.rs`:
- Around line 425-431: Add two targeted regression tests in the key_packages
module tests that exercise the new mls_proposals rejection branches: one that
constructs/feeds a KeyPackage-like input missing the mls_proposals tag to the
parser and asserts it returns the KeyPackage error for missing tag (use the
require/is_proposals_tag flow as the trigger), and another that supplies a
present mls_proposals tag with a wrong value (not "0x000a") and asserts the
parser returns Err(Error::KeyPackage(...)) with the "Invalid mls_proposals tag
value" path; reference the require and Self::is_proposals_tag checks and the
mls_proposals handling in key_packages.rs so tests fail if either branch
regresses.
---
Outside diff comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 52-80: Update the rustdoc for the public API to describe the new
KeyPackageEventData struct and migration rules instead of the old tuple return:
replace the tuple-oriented description and outdated tag list with the current
fields in KeyPackageEventData, document that `i` (KeyPackageRef) is conditional
per migration rules (not unconditional), add the newly required `mls_proposals`
tag, and clarify differences when publishing kind:30443 vs legacy kind:443
events so callers know which tags are mandatory vs optional; ensure all other
doc blocks referenced (lines around 112-118, 144-151, 289-298, 725-726) are
updated 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: 2b1b59c2-9f6c-430f-866e-d7e815b63376
📒 Files selected for processing (1)
crates/mdk-core/src/key_packages.rs
✅ Coverage: 91.51% → 91.58% (+0.07%) |
dannym-arx
force-pushed
the
update-new-spec
branch
from
a6b2363 to
89eaa52
Compare
✅ Coverage: 93.53% → 93.6% (+0.07%) |
✅ Coverage: 93.53% → 93.56% (+0.03%) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/key_packages.rs (1)
65-80:⚠️ Potential issue | 🟡 MinorFix the stale public return-shape docs.
Both rustdoc blocks still describe a tuple with one tag vector, but the API now returns
KeyPackageEventDatawithtags_30443,tags_443,hash_ref, andd_tag. Callers reading the current docs will get the wrong shape.Also applies to: 113-120
🤖 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 65 - 80, The rustdoc currently describes a returned tuple but the function now returns KeyPackageEventData; update the doc comments to list the actual return shape and field meanings: document KeyPackageEventData with fields tags_30443 (vector of tags for NIP-304/30443-style events), tags_443 (vector of tags for NIP-443-style events), hash_ref (the serialized hash_ref bytes for lifecycle tracking), d_tag (the 32-byte hex `d` identifier to store and reuse on rotation), and any encoded_key_package (base64-encoded serialized key package) or client/encoding metadata as applicable; replace the old single "tags" description with separate explanations for tags_30443 and tags_443 and ensure the examples and the duplicate docs at lines 113-120 are updated likewise.
🧹 Nitpick comments (2)
crates/mdk-core/examples/mls_memory.rs (1)
50-55: Make the legacy publish path explicit in the example.These legacy events are the interoperability path until May 1, 2026, but the underscore-prefixed variables make them look safely ignorable. Please either show both events being published or add an inline note that callers must publish both kinds during the migration window.
Also applies to: 272-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/examples/mls_memory.rs` around lines 50 - 55, The example currently creates a legacy event assigned to _bob_legacy_key_package_event (using EventBuilder::new(Kind::Custom(443), bob_key_package_encoded)...) which the underscore makes look ignorable; update the example to explicitly publish both the legacy and new events (i.e., remove the leading underscore from the legacy variable or call publish/send for both events) or add a clear inline comment next to EventBuilder::new(Kind::Custom(443), ...) stating that callers must publish both legacy (Custom(443)) and the new event form during the migration window (until May 1, 2026), and ensure the example shows both events being created and sent so readers can copy the correct interoperability behavior.crates/mdk-uniffi/src/lib.rs (1)
999-1011: Add binding-level coverage fortags_legacyandd_tag.These new fields are now part of the UniFFI contract, but nothing in this layer locks down that
tags_legacyreally omits thedtag or thatd_tagstays aligned with the["d", ...]entry intags. A small regression test here would keep mobile bindings from drifting silently.🤖 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 999 - 1011, Add a small binding-level regression test that verifies the UniFFI contract for the fields tags_legacy and d_tag: construct the struct instance that contains tags, tags_legacy and d_tag (the same type declared in this file), assert that tags_legacy does not contain any entry whose first element is "d", and assert that tags contains a ["d", ...] entry whose value matches d_tag (or that the "d" entry in tags equals d_tag). Place the test alongside existing unit tests in this crate so CI will catch regressions.
🤖 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/key_packages.rs`:
- Around line 429-442: The current validation only checks the first payload
element of the found proposals tag and allows multi-value tags; update the check
around prop_tag (and the block that uses prop.as_slice().get(1)) to require
exactly two elements (the tag name and a single payload) and that the single
payload equals "0x000a". In practice, verify prop.as_slice().len() == 2 and
prop.as_slice()[1].as_str() == "0x000a" (or equivalent) before accepting, and
return the existing KeyPackage errors when either condition fails; keep using
Self::is_proposals_tag, prop_tag, event.tags and MLS_KEY_PACKAGE_KIND to locate
the logic to modify.
---
Outside diff comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 65-80: The rustdoc currently describes a returned tuple but the
function now returns KeyPackageEventData; update the doc comments to list the
actual return shape and field meanings: document KeyPackageEventData with fields
tags_30443 (vector of tags for NIP-304/30443-style events), tags_443 (vector of
tags for NIP-443-style events), hash_ref (the serialized hash_ref bytes for
lifecycle tracking), d_tag (the 32-byte hex `d` identifier to store and reuse on
rotation), and any encoded_key_package (base64-encoded serialized key package)
or client/encoding metadata as applicable; replace the old single "tags"
description with separate explanations for tags_30443 and tags_443 and ensure
the examples and the duplicate docs at lines 113-120 are updated likewise.
---
Nitpick comments:
In `@crates/mdk-core/examples/mls_memory.rs`:
- Around line 50-55: The example currently creates a legacy event assigned to
_bob_legacy_key_package_event (using EventBuilder::new(Kind::Custom(443),
bob_key_package_encoded)...) which the underscore makes look ignorable; update
the example to explicitly publish both the legacy and new events (i.e., remove
the leading underscore from the legacy variable or call publish/send for both
events) or add a clear inline comment next to
EventBuilder::new(Kind::Custom(443), ...) stating that callers must publish both
legacy (Custom(443)) and the new event form during the migration window (until
May 1, 2026), and ensure the example shows both events being created and sent so
readers can copy the correct interoperability behavior.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 999-1011: Add a small binding-level regression test that verifies
the UniFFI contract for the fields tags_legacy and d_tag: construct the struct
instance that contains tags, tags_legacy and d_tag (the same type declared in
this file), assert that tags_legacy does not contain any entry whose first
element is "d", and assert that tags contains a ["d", ...] entry whose value
matches d_tag (or that the "d" entry in tags equals d_tag). Place the test
alongside existing unit tests in this crate so CI will catch regressions.
🪄 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: ef616f67-60d0-49e5-8308-8d4a8f62cb7e
📒 Files selected for processing (11)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/examples/group_inspection.rscrates/mdk-core/examples/key_package_inspection.rscrates/mdk-core/examples/mls_memory.rscrates/mdk-core/examples/mls_sqlite.rscrates/mdk-core/src/constant.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/key_packages_test.rscrates/mdk-core/src/test_util.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
- crates/mdk-core/src/key_packages_test.rs
- crates/mdk-core/examples/mls_sqlite.rs
- crates/mdk-core/examples/key_package_inspection.rs
- crates/mdk-core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mdk-core/src/test_util.rs
- crates/mdk-core/src/constant.rs
- crates/mdk-uniffi/CHANGELOG.md
✅ Coverage: 93.54% → 93.57% (+0.03%) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-uniffi/src/lib.rs (1)
1974-1984: Tests correctly use the new kind:30443.These tests validate the new addressable event kind, which is correct. The dual-publish migration guidance is documented in the CHANGELOG. However, consider adding a test that verifies
tags_legacyexcludes thedtag (to ensure the core filtering from context snippet 2 is working correctly at the UniFFI boundary).🧪 Suggested test addition (can be deferred)
#[test] fn test_key_package_tags_legacy_excludes_d_tag() { let mdk = create_test_mdk(); let keys = Keys::generate(); let pubkey_hex = keys.public_key().to_hex(); let relays = vec!["wss://relay.example.com".to_string()]; let result = mdk.create_key_package_for_event(pubkey_hex, relays).unwrap(); // tags should contain d tag assert!(result.tags.iter().any(|t| t.first() == Some(&"d".to_string()))); // tags_legacy should NOT contain d tag assert!(!result.tags_legacy.iter().any(|t| t.first() == Some(&"d".to_string()))); }🤖 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 1974 - 1984, Add a unit test that calls create_key_package_for_event (via the test helper like create_test_mdk and Keys::generate) and asserts that the produced result.tags contains a "d" tag while result.tags_legacy does NOT contain any tag whose first element is "d"; locate the test around other key-package tests and use the same result structure (result.tags and result.tags_legacy) to verify the legacy filtering at the UniFFI boundary.
🤖 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-uniffi/src/lib.rs`:
- Around line 1974-1984: Add a unit test that calls create_key_package_for_event
(via the test helper like create_test_mdk and Keys::generate) and asserts that
the produced result.tags contains a "d" tag while result.tags_legacy does NOT
contain any tag whose first element is "d"; locate the test around other
key-package tests and use the same result structure (result.tags and
result.tags_legacy) to verify the legacy filtering at the UniFFI boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af2e8757-179b-4849-8f40-cfef29981e29
📒 Files selected for processing (3)
crates/mdk-core/examples/mls_sqlite.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/examples/mls_sqlite.rs
dannym-arx
force-pushed
the
update-new-spec
branch
from
7669363 to
dff451c
Compare
✅ Coverage: 93.54% → 93.57% (+0.03%) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-uniffi/src/lib.rs (1)
1880-1895:⚠️ Potential issue | 🟡 MinorAdd assertions for newly exposed
tags_legacyandd_tagfields.This test validates pre-existing fields only. Please also assert that
d_tagis non-empty/hex and thattags_legacyis populated and excludes thedtag, so UniFFI migration semantics stay covered.Based on learnings, "Add tests for new functionality in test modules before submitting a PR."
🤖 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 1880 - 1895, The test test_create_key_package_for_event only asserts existing fields; add assertions for the newly exposed fields by checking that key_package_result.d_tag is non-empty and valid hex and that key_package_result.tags_legacy is populated and does not contain a "d" tag; update the test to assert !key_package_result.d_tag.is_empty() (and optionally that d_tag parses as hex) and assert !key_package_result.tags_legacy.is_empty() and that none of tags_legacy entries have a key equal to "d" to ensure UniFFI migration semantics are covered.crates/mdk-core/src/key_packages.rs (1)
67-80:⚠️ Potential issue | 🟡 MinorUpdate Rustdoc return docs to match
KeyPackageEventData(not tuple).The return docs still describe a tuple, but both APIs now return
KeyPackageEventData. This can mislead callers and generated docs.📝 Suggested doc fix
- /// A tuple containing: - /// * A base64-encoded string containing the serialized key package - /// * A vector of tags for the Nostr event: + /// A [`KeyPackageEventData`] containing: + /// * `content`: base64-encoded serialized key package + /// * `tags_30443`: tags for kind:30443 (includes `d`) + /// * `tags_443`: tags for legacy kind:443 (omits `d`) + /// * `hash_ref`: serialized hash_ref bytes for lifecycle tracking + /// * `d_tag`: 32-byte hex identifier for replaceable slot rotationAlso applies to: 115-119
🤖 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 67 - 80, The Rustdoc currently describes a tuple return but the APIs return KeyPackageEventData; update the doc comments where the return is described to document the KeyPackageEventData struct instead of a tuple: describe the struct's fields (e.g., the base64-encoded serialized key package, the vector of Nostr tags with names like "d", "mls_protocol_version", "mls_ciphersuite", "mls_extensions", "mls_proposals", "relays", "i", "client", "encoding", the serialized hash_ref bytes, and the d tag value) and ensure the documented return type name is KeyPackageEventData for the functions that return it (replace any tuple wording in the comments around KeyPackageEventData).
🧹 Nitpick comments (1)
crates/mdk-uniffi/src/lib.rs (1)
417-422: Document dual-publish behavior for migration (30443 + 443) in the UniFFI API docs.These docs explain
tagsandtags_legacy, but they still don’t explicitly tell binding consumers to publish both kinds during migration. Please add a direct note to publishtagsas kind30443andtags_legacyas kind443until May 1, 2026, otherwise legacy peers may not discover KeyPackages.✏️ Suggested doc patch
/// Create a key package for a Nostr event /// /// This function does NOT add the NIP-70 protected tag, ensuring maximum relay /// compatibility. Many popular relays (Damus, Primal, nos.lol) reject protected events. /// If you need the protected tag, use `create_key_package_for_event_with_options` instead. +/// During migration, callers SHOULD dual-publish: +/// - kind:30443 using `KeyPackageResult.tags` +/// - kind:443 using `KeyPackageResult.tags_legacy` +/// Legacy publishing SHOULD continue until May 1, 2026.Also applies to: 452-462, 1002-1012
🤖 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 417 - 422, Update the UniFFI API docs on create_key_package_for_event (and the related create_key_package_for_event_with_options) to explicitly instruct binding consumers to publish both tag sets during migration: publish `tags` as kind 30443 and `tags_legacy` as kind 443, and continue doing so until May 1, 2026 so legacy peers can discover KeyPackages; apply the same explicit note to the other related doc comment blocks referenced (the second block around the same API and the later block near 1002-1012) so all language bindings surface the dual-publish guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 67-80: The Rustdoc currently describes a tuple return but the APIs
return KeyPackageEventData; update the doc comments where the return is
described to document the KeyPackageEventData struct instead of a tuple:
describe the struct's fields (e.g., the base64-encoded serialized key package,
the vector of Nostr tags with names like "d", "mls_protocol_version",
"mls_ciphersuite", "mls_extensions", "mls_proposals", "relays", "i", "client",
"encoding", the serialized hash_ref bytes, and the d tag value) and ensure the
documented return type name is KeyPackageEventData for the functions that return
it (replace any tuple wording in the comments around KeyPackageEventData).
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 1880-1895: The test test_create_key_package_for_event only asserts
existing fields; add assertions for the newly exposed fields by checking that
key_package_result.d_tag is non-empty and valid hex and that
key_package_result.tags_legacy is populated and does not contain a "d" tag;
update the test to assert !key_package_result.d_tag.is_empty() (and optionally
that d_tag parses as hex) and assert !key_package_result.tags_legacy.is_empty()
and that none of tags_legacy entries have a key equal to "d" to ensure UniFFI
migration semantics are covered.
---
Nitpick comments:
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 417-422: Update the UniFFI API docs on
create_key_package_for_event (and the related
create_key_package_for_event_with_options) to explicitly instruct binding
consumers to publish both tag sets during migration: publish `tags` as kind
30443 and `tags_legacy` as kind 443, and continue doing so until May 1, 2026 so
legacy peers can discover KeyPackages; apply the same explicit note to the other
related doc comment blocks referenced (the second block around the same API and
the later block near 1002-1012) so all language bindings surface the
dual-publish guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e830eb5-adbb-4330-a108-6ca2681bb090
📒 Files selected for processing (4)
crates/mdk-core/examples/mls_sqlite.rscrates/mdk-core/src/key_packages.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/mdk-uniffi/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-core/examples/mls_sqlite.rs
✅ Coverage: 93.62% → 93.65% (+0.03%) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
3283-3317: Consider extending this test to also omitmls_proposalsfor more realistic legacy coverage.The test correctly verifies backward compatibility for legacy kind:443 events without
danditags. However, truly legacy events (published before this migration) would also lack themls_proposalstag.While the validation logic at lines 441-447 correctly makes
mls_proposalsoptional for kind:443, this test doesn't exercise that path sincetags_443still includesmls_proposalsfromcreate_key_package_for_event.🧪 Suggested test extension
// Remove i tag for bare kind:443 (d tag is already removed in tags_443) -let bare_tags: Vec<Tag> = tags +// Also remove mls_proposals to simulate truly legacy events (pre-migration) +let bare_tags: Vec<Tag> = tags .into_iter() .filter(|t| t.kind() != TagKind::i()) + .filter(|t| !matches!(t.kind(), TagKind::Custom(ref s) if s == "mls_proposals")) .collect();🤖 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 3283 - 3317, The test test_parse_key_package_accepts_bare_legacy_kind_443 should also remove the mls_proposals tag to simulate truly legacy events: when building bare_tags (the Vec<Tag> filtered from tags_443 produced by create_key_package_for_event), additionally filter out tags with TagKind::mls_proposals (or the tag name used for mls_proposals in your Tag API) so the event lacks d, i and mls_proposals before calling mdk.parse_key_package(&event); this ensures the optional-mls_proposals code path is exercised.
🤖 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 3283-3317: The test
test_parse_key_package_accepts_bare_legacy_kind_443 should also remove the
mls_proposals tag to simulate truly legacy events: when building bare_tags (the
Vec<Tag> filtered from tags_443 produced by create_key_package_for_event),
additionally filter out tags with TagKind::mls_proposals (or the tag name used
for mls_proposals in your Tag API) so the event lacks d, i and mls_proposals
before calling mdk.parse_key_package(&event); this ensures the
optional-mls_proposals code path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0583f6ef-06e5-4452-94ea-e4fb20695179
📒 Files selected for processing (1)
crates/mdk-core/src/key_packages.rs
2 participants
Summary
Adapts the MDK implementation to the updated MIP-00 spec (marmot-protocol/marmot#54), which migrates KeyPackage events from
kind:443(regular) tokind:30443(addressable, NIP-33).kind:443tokind:30443: All KeyPackage event creation now uses the addressable event kind. Relays automatically replace events sharing the same(kind, pubkey, d)tuple, enabling rotation without NIP-09 deletion.dtag: A random 32-byte hex identifier is generated per KeyPackage slot and included as the first tag. Thedvalue is returned to callers so they can reuse it when rotating (samedmeans the relay replaces the old event).create_key_package_for_eventandcreate_key_package_for_event_with_optionsnow return a 4-tuple(content, tags, hash_ref, d_tag)instead of a 3-tuple. The UniFFIKeyPackageResultstruct gains ad_tag: Stringfield.parse_key_packageaccepts bothkind:30443(new) andkind:443(legacy) during the migration period until May 1, 2026. Forkind:30443events, thedtag is validated as present and non-empty.This PR migrates KeyPackage events from Nostr kind:443 (regular) to kind:30443 (addressable, NIP-33) per the updated MIP-00 specification. Each KeyPackage slot now includes a random 32-byte hex identifier (
dtag) that enables relay-based event rotation without explicit deletion, while maintaining backward compatibility with legacy kind:443 events through May 1, 2026.What changed:
create_key_package_for_eventandcreate_key_package_for_event_with_optionschanges from a 3-tuple(content, tags, hash_ref)to aKeyPackageEventDatastruct with fieldscontent,tags_30443,tags_443,hash_ref, andd_tag.dtag and produces separate tag sets:tags_30443(includesdfor kind:30443 events) andtags_443(legacy format, excludesd).MLS_KEY_PACKAGE_KIND(kind:30443) andMLS_KEY_PACKAGE_KIND_LEGACY(kind:443) incrates/mdk-core/src/constant.rs.group_inspection.rs,key_package_inspection.rs,mls_memory.rs,mls_sqlite.rs) updated to destructure the new struct and create both kind:30443 (primary) and kind:443 (legacy) events for dual-publishing during migration.parse_key_packageaccepts both kind:30443 and kind:443 with kind-specific validation: kind:30443 requires thedtag (non-empty, 64 hex characters), while kind:443 makes it optional.mls_proposalstag validation added for kind:30443 events;itag (KeyPackageRef) made required for kind:30443 and optional for legacy kind:443.Protocol changes:
(kind, pubkey, d)tuple uniquely identifies a slot and relays automatically replace events with identical tuples.dtag value must be reused for KeyPackage rotation on the same slot.API surface:
create_key_package_for_eventandcreate_key_package_for_event_with_optionsnow returnKeyPackageEventDatastruct instead of tuple; callers must update destructuring.KeyPackageResultgainsd_tag: Stringandtags_legacy: Vec<Vec<String>>fields.KeyPackageEventDatastruct exported fromcrates/mdk-core/src/key_packages.rs.Testing:
dtag intags_30443).mls_proposals, missing/invaliddtag for kind:30443, acceptance of bare legacy kind:443 events (withoutdanditags), andparse_key_packagekind compatibility.test_util.rs) updated to useKeyPackageEventDatadestructuring andMLS_KEY_PACKAGE_KINDconstant.Known gaps requiring follow-up (per PR comments):
mls_proposalstag validation currently enforces presence but examples do not include it in tag generation.itag handling documentation needs clarification on when it must be computed from content.