Refactor codebase to reduce duplication#239
Merged
dannym-arx merged 24 commits intomasterfrom Apr 2, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)(omitted) 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:598 |
delete_key_package_from_storage_by_hash_ref |
crates/mdk-core/src/key_packages.rs:613 |
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: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: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: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:455 |
get_groups |
crates/mdk-core/src/groups.rs:467 |
get_last_message |
crates/mdk-core/src/messages/mod.rs:278 |
get_members |
crates/mdk-core/src/groups.rs:501 |
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: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: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: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.51% → 91.88% (+0.37%) |
✅ Coverage: 91.51% → 93.19% (+1.68%) |
✅ Coverage: 91.51% → 93.28% (+1.77%) |
✅ Coverage: 91.51% → 93.28% (+1.77%) |
✅ Coverage: 91.51% → 93.29% (+1.78%) |
✅ Coverage: 91.51% → 93.28% (+1.77%) |
✅ Coverage: 91.51% → 93.26% (+1.75%) |
✅ Coverage: 91.51% → 93.32% (+1.81%) |
✅ Coverage: 91.51% → 93.29% (+1.78%) |
✅ Coverage: 91.51% → 93.33% (+1.82%) |
✅ Coverage: 91.51% → 93.32% (+1.81%) |
✅ Coverage: 91.51% → 93.33% (+1.82%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-sqlite-storage/src/welcomes.rs (1)
20-56:⚠️ Potential issue | 🟠 MajorCall
validate_welcome_fieldsfrom the SQLite backend too.Line 20 still only validates name/description lengths and serialized JSON sizes. Since the relay/admin/url rules now live in
crates/mdk-storage-traits/src/welcomes/validation.rs, SQLite can accept welcomes that the memory backend rejects whenever those inputs stay under the JSON byte caps. Please run the shared helper here before serialization and add a SQLite regression test for relay count, relay URL length, and admin count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/welcomes.rs` around lines 20 - 56, Call the shared validation helper validate_welcome_fields from crates/mdk-storage-traits (the function named validate_welcome_fields) at the start of the SQLite save flow in welcomes.rs before serializing admin pubkeys and relays (i.e. before serde_json::to_string calls that create group_admin_pubkeys_json and group_relays_json), propagating its error into WelcomeError::InvalidParameters as done elsewhere; then add SQLite regression tests that exercise relay count, relay URL length, and admin count limits to ensure the SQLite backend rejects inputs the memory backend already does.crates/mdk-core/src/key_packages.rs (1)
408-437:⚠️ Potential issue | 🟠 MajorReject extra values on single-valued key-package tags.
tag_first_value()makesmls_protocol_versionandmls_ciphersuiteaccept tags like["mls_protocol_version", "1.0", "junk"]and["mls_ciphersuite", "0x0001", "junk"]. That weakens the MIP-00 validation here and can leave malformed events valid locally while other implementations reject them.🔐 Proposed fix
+ fn tag_single_value<'a>( + tag: &'a Tag, + missing_msg: &str, + multi_value_msg: &str, + ) -> Result<&'a str, Error> { + match tag.as_slice() { + [_, value] => Ok(value.as_str()), + [_] => Err(Error::KeyPackage(missing_msg.to_string())), + _ => Err(Error::KeyPackage(multi_value_msg.to_string())), + } + } + /// Validates protocol version tag format and value. /// /// **SPEC-COMPLIANT**: Per MIP-00, only "1.0" is currently supported. fn validate_protocol_version_tag(&self, tag: &Tag) -> Result<(), Error> { - let version_value = Self::tag_first_value(tag, "Protocol version tag must have a value")?; + let version_value = Self::tag_single_value( + tag, + "Protocol version tag must have a value", + "Protocol version tag must contain exactly one value", + )?; @@ /// Validates ciphersuite tag format and value per MIP-00. /// /// Currently only accepts: "0x0001" (MLS_128_DHKEMX25519_AES128GCM_SHA256_Ed25519) fn validate_ciphersuite_tag(&self, tag: &Tag) -> Result<(), Error> { - let ciphersuite_value = Self::tag_first_value(tag, "Ciphersuite tag must have a value")?; + let ciphersuite_value = Self::tag_single_value( + tag, + "Ciphersuite tag must have a value", + "Ciphersuite tag must contain exactly one value", + )?;Based on learnings, "Ensure all code follows the Marmot Protocol specification exactly. Ask clarifying questions if any part of the spec is unclear".
🤖 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 408 - 437, tag_first_value currently returns the first value but allows extra entries (e.g. ["mls_protocol_version","1.0","junk"]); change tag_first_value to verify the tag has exactly two elements (name and single value) and return Err(Error::KeyPackage(...)) when len != 2 so single-valued tags are rejected; ensure validate_protocol_version_tag and validate_ciphersuite_tag continue to call tag_first_value and rely on its stricter check, and update the error message produced by tag_first_value to indicate unexpected extra values for clarity.
🧹 Nitpick comments (8)
crates/mdk-memory-storage/src/lib.rs (1)
165-176: Carry the field docs onto the generatedwith_*setters.These public builders currently only expose a generic
# Panicssection, so rustdoc doesn't say what each setter actually configures. Reusing the field docs here keeps the public API readable after the macro refactor.📝 Suggested rustdoc fix
$( pastey::paste! { + $(#[doc = $doc])* + /// /// # Panics /// /// Panics if `limit` is 0. pub fn [<with_ $field>](mut self, limit: usize) -> Self {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-memory-storage/src/lib.rs` around lines 165 - 176, The generated builder setters [<with_ $field>] lack the original field documentation; update the macro that emits the with_* functions so it reuses the field's doc attributes when generating each setter (i.e. carry the field doc attrs into the function), by adding the existing field doc meta (e.g. the #[doc = ...] / #[$field_doc] entries) to the generated pub fn [<with_ $field>](...) so rustdoc shows what the setter configures while keeping the existing panic note.crates/mdk-sqlite-storage/src/groups.rs (2)
24-34: Use a lightweight existence probe here instead of deserializing the full group.This helper currently does
SELECT *and full row decoding just to answer “does the group exist?”. That adds avoidable work to hot paths likemessages()/last_message()and also makes unrelated metadata corruption block relay/message lookups.♻️ Proposed refactor
#[inline] fn ensure_group_exists( storage: &MdkSqliteStorage, mls_group_id: &GroupId, ) -> Result<(), GroupError> { - if storage.find_group_by_mls_group_id(mls_group_id)?.is_some() { - Ok(()) - } else { - Err(group_not_found()) - } + let exists = storage.with_connection(|conn| { + conn.query_row( + "SELECT 1 FROM groups WHERE mls_group_id = ? LIMIT 1", + params![mls_group_id.as_slice()], + |_| Ok(()), + ) + .optional() + .map_err(into_group_err) + })?; + + exists.ok_or_else(group_not_found) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/groups.rs` around lines 24 - 34, The helper ensure_group_exists should avoid deserializing the full group row; replace the call to storage.find_group_by_mls_group_id(...) with a lightweight existence probe (e.g., add and call a new storage method like has_group_with_mls_group_id or exists_group_by_mls_group_id that performs SELECT 1 / EXISTS ... LIMIT 1) and return Ok(()) when that probe is true, otherwise Err(group_not_found()). Implement the new storage method to run a minimal SQL existence check (no full row decode) and update ensure_group_exists to use that method (retain GroupId and GroupError symbols).
291-333: Run the shared relay validation before mutating the table.
validate_relay_set()was added to centralize relay-count and URL-length rules, but this path still deletes and reinserts without calling it. That leaves SQLite with different acceptance rules from the shared validation module and pushes oversized relay sets to fail later at the DB layer instead of asInvalidParameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/groups.rs` around lines 291 - 333, The replace_group_relays function mutates the DB without first running the shared validation, so oversized or invalid relay sets slip past and later fail at the SQLite layer; before deleting/inserting rows in replace_group_relays call validate_relay_set(&relays) (and propagate its InvalidParameters error into the GroupError path) so validation runs early (i.e., perform the validate_relay_set check immediately after ensure_group_exists and before any DB execute_batch/delete/insert work).crates/mdk-sqlite-storage/src/db.rs (1)
160-164: Preserve the enum-specific parse error here.This helper now collapses every bad state in every table into the same
"Invalid state"string, which makes corrupted rows much harder to diagnose and throws away thestring_enum!messages you just centralized. ForwardT::Errthroughmap_to_text_boxed_errorinstead.♻️ Proposed refactor
-fn text_to_state<T: FromStr>(row: &Row, col: &str) -> SqliteResult<T> { +fn text_to_state<T>(row: &Row, col: &str) -> SqliteResult<T> +where + T: FromStr, + T::Err: std::error::Error + Send + Sync + 'static, +{ let s = row.get_ref(col)?.as_str()?; - T::from_str(s).map_err(|_| map_invalid_text_data("Invalid state")) + T::from_str(s).map_err(map_to_text_boxed_error) }As per coding guidelines, "All trait bounds in
whereclauses, not inline."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/db.rs` around lines 160 - 164, The helper text_to_state currently maps all parse errors to the generic "Invalid state" string; change it to preserve and forward the enum-specific parse error by returning T::Err converted through map_to_text_boxed_error instead of the fixed message. Update the function signature to put the FromStr bound in a where clause (e.g., fn text_to_state<T>(row: &Row, col: &str) -> SqliteResult<T> where T: FromStr, and then use T::from_str(s).map_err(map_to_text_boxed_error) so the original T::Err is preserved and boxed).crates/mdk-core/src/error.rs (1)
251-280: Keep the generic bounds inwhereclauses in the generated impls.The new macroized conversions reintroduce inline bounds (
impl<T: fmt::Display> ...) in both the macro and the explicitCommitToPendingProposalsErrorimpl. Please move those towhereclauses so the generated code matches the repo’s Rust style.♻️ Proposed refactor
macro_rules! impl_from_generic_display_error { ($($source:ident => $variant:ident),+ $(,)?) => { $( - impl<T: fmt::Display> From<$source<T>> for Error { + impl<T> From<$source<T>> for Error + where + T: fmt::Display, + { fn from(e: $source<T>) -> Self { Self::$variant(e.to_string()) } } )+ @@ -impl<T: fmt::Display> From<CommitToPendingProposalsError<T>> for Error { +impl<T> From<CommitToPendingProposalsError<T>> for Error +where + T: fmt::Display, +{ fn from(_e: CommitToPendingProposalsError<T>) -> Self { Self::CommitToPendingProposalsError } }As per coding guidelines, "All trait bounds in
whereclauses, not inline."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/error.rs` around lines 251 - 280, The generated impls currently use inline bounds like `impl<T: fmt::Display> From<...>`; change them to use `where` clauses instead—update the macro `impl_from_generic_display_error!` (and the `impl_from_display_error!` usage if needed) so it emits `impl<T> From<$source<T>> for Error where T: fmt::Display { ... }`, and modify the explicit impl for `From<CommitToPendingProposalsError<T>> for Error` to `impl<T> From<CommitToPendingProposalsError<T>> for Error where T: fmt::Display { ... }`; ensure all trait bounds are moved from inline `impl<...>` headers into `where` clauses to match the repo style.crates/mdk-storage-traits/src/groups/validation.rs (1)
13-76: Add direct tests around these shared boundary checks.This module is now the validation source of truth for multiple storage backends, but the new file has no local tests for exact-limit / over-limit cases on name, description, admin count, relay count, and relay URL length. A small table-driven suite here would catch drift before it hits both backends.
As per coding guidelines, "ensure CI test coverage does not decrease: run the appropriate
just test*targets and add/adjust tests if any new paths were introduced or existing paths lost coverage due to macro consolidation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-storage-traits/src/groups/validation.rs` around lines 13 - 76, Add unit tests for validate_group_fields and validate_relay_set that exercise exact-limit and over-limit cases for name length, description length, admin_pubkeys count, relay count, and individual relay URL length; create a table-driven test table that constructs Group instances (testing validate_group_fields) and BTreeSet<RelayUrl> instances (testing validate_relay_set), asserting Ok for values equal to the limits and Err(GroupError::InvalidParameters) for values exceeding limits; use the existing types Group, RelayUrl, GroupError, and call validate_group_fields and validate_relay_set directly in the tests to prevent regressions when storage backends rely on these shared validators.crates/mdk-core/src/groups.rs (1)
168-177: Use.to_owned()in generated string setters to match repository style rules.The
into_stringsetter currently uses.into()forStringconversion; this conflicts with the project rule for string conversions.♻️ Suggested adjustment
- (into_string: $name:ident, $doc:expr) => { + (into_string: $name:ident, $doc:expr) => { #[doc = $doc] pub fn $name<T>(mut self, $name: T) -> Self where - T: Into<String>, + T: AsRef<str>, { - self.$name = Some($name.into()); + self.$name = Some($name.as_ref().to_owned()); self } };As per coding guidelines: use
.to_string()or.to_owned(), not.into()orString::fromfor string conversions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/src/groups.rs` around lines 168 - 177, The generated setter in the update_setter! macro (the into_string: $name variant) uses $name.into() to convert to String; change that conversion to $name.to_owned() (or to_string()) so the setter assigns self.$name = Some($name.to_owned()) to conform to the repository string-conversion style and keep the macro-generated setters consistent.crates/mdk-uniffi/src/lib.rs (1)
259-289: Add one happy-path test for this sharedUpdateGroupResultmapper.This helper now defines the public output shape for
add_members,remove_members,self_update,leave_group,update_group_data, and the proposal branch ofprocess_message(), but nothing in this file asserts the serializedevolution_event_json/welcome_rumors_json/mls_group_idcontract. One focused test here would cover all of those paths at once.As per coding guidelines, ensure CI test coverage does not decrease and add or adjust tests when new paths are introduced.
🤖 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 259 - 289, Add a focused happy-path unit test that constructs an mdk_core::groups::UpdateGroupResult with a concrete evolution_event, an Option<Vec<welcome_rumor>> containing at least one rumor, and a known mls_group_id byte array, then call update_group_result_to_uniffi and assert the returned UpdateGroupResult has evolution_event_json that parses back to the original evolution_event (or contains expected fields), welcome_rumors_json is Some with each entry parsing back to the original rumors, and mls_group_id equals hex::encode of the original bytes; reference the function update_group_result_to_uniffi and the exported UpdateGroupResult shape when adding the test to ensure the serialized JSON/hex contract is validated.
🤖 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/CHANGELOG.md`:
- Around line 31-32: Update the two changelog bullets shown (the one about
replacing ~15 identical `From<...> for Error` impls with
`impl_from_display_error!` and `impl_from_generic_display_error!` macros, and
the one about consolidating duplicate test wiring into `storage_backend_tests!`
macro) to append the PR reference suffix in the required format, e.g.
`([`#239`](https://github.com/marmot-protocol/mdk/pull/239))`; ensure each bullet
ends with its respective PR link and use the same bracket/parentheses syntax for
both entries.
In `@crates/mdk-memory-storage/CHANGELOG.md`:
- Around line 32-33: Update the two changelog bullets in CHANGELOG.md that start
with "Replaced 6 identical MLS storage..." and "Migrated group and welcome
storage..." to append the PR reference in the required format; add "
([`#239`](https://github.com/marmot-protocol/mdk/pull/239))" at the end of each
bullet so both entries end with the trailing PR link as per the guidelines.
In `@crates/mdk-sqlite-storage/CHANGELOG.md`:
- Around line 32-33: Update the two new CHANGELOG.md bullets to include the
required PR reference suffix
"([`#239`](https://github.com/marmot-protocol/mdk/pull/239))": append this exact
suffix to the end of the "Extracted row-parsing helpers..." bullet (which
mentions blob_to_group_id, blob_to_event_id, blob_to_pubkey, text_to_state,
text_to_json, text_to_parsed and row_to_* functions) and to the "Migrated group
and welcome storage..." bullet (which references mdk-storage-traits), ensuring
both entries follow the project's changelog format.
In `@crates/mdk-storage-traits/CHANGELOG.md`:
- Around line 34-38: Update each Unreleased changelog bullet to append the PR
reference suffix in the required format; for the bullets mentioning
GroupState/MessageState/WelcomeState/ProcessedMessageState/ProcessedWelcomeState
(string_enum!), Pagination/bounded_pagination!,
GroupError/MessageError/WelcomeError (storage_error!), groups::validation and
welcomes::validation extraction, and the removed test_utils::cross_storage
lines, add " ([`#239`](https://github.com/marmot-protocol/mdk/pull/239))" to the
end of each entry so they match the repository changelog format.
In `@crates/mdk-uniffi/CHANGELOG.md`:
- Around line 32-33: Append the PR citation suffix
"([`#239`](https://github.com/marmot-protocol/mdk/pull/239))" to the end of both
changelog bullets currently reading "Extracted `update_group_result_to_uniffi()`
and `mdk_from_storage()` helpers to eliminate duplicated serialization and
constructor logic across multiple exported functions." and "Simplified iterator
collection patterns to use `.collect::<Result<_, _>>()` instead of two-step
collect-then-unwrap.", ensuring each bullet ends with the exact PR reference per
the changelog guideline.
---
Outside diff comments:
In `@crates/mdk-core/src/key_packages.rs`:
- Around line 408-437: tag_first_value currently returns the first value but
allows extra entries (e.g. ["mls_protocol_version","1.0","junk"]); change
tag_first_value to verify the tag has exactly two elements (name and single
value) and return Err(Error::KeyPackage(...)) when len != 2 so single-valued
tags are rejected; ensure validate_protocol_version_tag and
validate_ciphersuite_tag continue to call tag_first_value and rely on its
stricter check, and update the error message produced by tag_first_value to
indicate unexpected extra values for clarity.
In `@crates/mdk-sqlite-storage/src/welcomes.rs`:
- Around line 20-56: Call the shared validation helper validate_welcome_fields
from crates/mdk-storage-traits (the function named validate_welcome_fields) at
the start of the SQLite save flow in welcomes.rs before serializing admin
pubkeys and relays (i.e. before serde_json::to_string calls that create
group_admin_pubkeys_json and group_relays_json), propagating its error into
WelcomeError::InvalidParameters as done elsewhere; then add SQLite regression
tests that exercise relay count, relay URL length, and admin count limits to
ensure the SQLite backend rejects inputs the memory backend already does.
---
Nitpick comments:
In `@crates/mdk-core/src/error.rs`:
- Around line 251-280: The generated impls currently use inline bounds like
`impl<T: fmt::Display> From<...>`; change them to use `where` clauses
instead—update the macro `impl_from_generic_display_error!` (and the
`impl_from_display_error!` usage if needed) so it emits `impl<T>
From<$source<T>> for Error where T: fmt::Display { ... }`, and modify the
explicit impl for `From<CommitToPendingProposalsError<T>> for Error` to `impl<T>
From<CommitToPendingProposalsError<T>> for Error where T: fmt::Display { ... }`;
ensure all trait bounds are moved from inline `impl<...>` headers into `where`
clauses to match the repo style.
In `@crates/mdk-core/src/groups.rs`:
- Around line 168-177: The generated setter in the update_setter! macro (the
into_string: $name variant) uses $name.into() to convert to String; change that
conversion to $name.to_owned() (or to_string()) so the setter assigns self.$name
= Some($name.to_owned()) to conform to the repository string-conversion style
and keep the macro-generated setters consistent.
In `@crates/mdk-memory-storage/src/lib.rs`:
- Around line 165-176: The generated builder setters [<with_ $field>] lack the
original field documentation; update the macro that emits the with_* functions
so it reuses the field's doc attributes when generating each setter (i.e. carry
the field doc attrs into the function), by adding the existing field doc meta
(e.g. the #[doc = ...] / #[$field_doc] entries) to the generated pub fn [<with_
$field>](...) so rustdoc shows what the setter configures while keeping the
existing panic note.
In `@crates/mdk-sqlite-storage/src/db.rs`:
- Around line 160-164: The helper text_to_state currently maps all parse errors
to the generic "Invalid state" string; change it to preserve and forward the
enum-specific parse error by returning T::Err converted through
map_to_text_boxed_error instead of the fixed message. Update the function
signature to put the FromStr bound in a where clause (e.g., fn
text_to_state<T>(row: &Row, col: &str) -> SqliteResult<T> where T: FromStr, and
then use T::from_str(s).map_err(map_to_text_boxed_error) so the original T::Err
is preserved and boxed).
In `@crates/mdk-sqlite-storage/src/groups.rs`:
- Around line 24-34: The helper ensure_group_exists should avoid deserializing
the full group row; replace the call to storage.find_group_by_mls_group_id(...)
with a lightweight existence probe (e.g., add and call a new storage method like
has_group_with_mls_group_id or exists_group_by_mls_group_id that performs SELECT
1 / EXISTS ... LIMIT 1) and return Ok(()) when that probe is true, otherwise
Err(group_not_found()). Implement the new storage method to run a minimal SQL
existence check (no full row decode) and update ensure_group_exists to use that
method (retain GroupId and GroupError symbols).
- Around line 291-333: The replace_group_relays function mutates the DB without
first running the shared validation, so oversized or invalid relay sets slip
past and later fail at the SQLite layer; before deleting/inserting rows in
replace_group_relays call validate_relay_set(&relays) (and propagate its
InvalidParameters error into the GroupError path) so validation runs early
(i.e., perform the validate_relay_set check immediately after
ensure_group_exists and before any DB execute_batch/delete/insert work).
In `@crates/mdk-storage-traits/src/groups/validation.rs`:
- Around line 13-76: Add unit tests for validate_group_fields and
validate_relay_set that exercise exact-limit and over-limit cases for name
length, description length, admin_pubkeys count, relay count, and individual
relay URL length; create a table-driven test table that constructs Group
instances (testing validate_group_fields) and BTreeSet<RelayUrl> instances
(testing validate_relay_set), asserting Ok for values equal to the limits and
Err(GroupError::InvalidParameters) for values exceeding limits; use the existing
types Group, RelayUrl, GroupError, and call validate_group_fields and
validate_relay_set directly in the tests to prevent regressions when storage
backends rely on these shared validators.
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 259-289: Add a focused happy-path unit test that constructs an
mdk_core::groups::UpdateGroupResult with a concrete evolution_event, an
Option<Vec<welcome_rumor>> containing at least one rumor, and a known
mls_group_id byte array, then call update_group_result_to_uniffi and assert the
returned UpdateGroupResult has evolution_event_json that parses back to the
original evolution_event (or contains expected fields), welcome_rumors_json is
Some with each entry parsing back to the original rumors, and mls_group_id
equals hex::encode of the original bytes; reference the function
update_group_result_to_uniffi and the exported UpdateGroupResult shape when
adding the test to ensure the serialized JSON/hex contract is validated.
🪄 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: f312f7a2-63f0-4ac4-bbf9-7222d42d06db
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
Cargo.tomlcrates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/error.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/tests/shared/mod.rscrates/mdk-core/tests/storage_traits_memory.rscrates/mdk-core/tests/storage_traits_sqlite.rscrates/mdk-memory-storage/CHANGELOG.mdcrates/mdk-memory-storage/Cargo.tomlcrates/mdk-memory-storage/src/groups.rscrates/mdk-memory-storage/src/lib.rscrates/mdk-memory-storage/src/mls_storage/mod.rscrates/mdk-memory-storage/src/welcomes.rscrates/mdk-sqlite-storage/CHANGELOG.mdcrates/mdk-sqlite-storage/src/db.rscrates/mdk-sqlite-storage/src/groups.rscrates/mdk-sqlite-storage/src/lib.rscrates/mdk-sqlite-storage/src/messages.rscrates/mdk-sqlite-storage/src/welcomes.rscrates/mdk-storage-traits/CHANGELOG.mdcrates/mdk-storage-traits/src/groups/error.rscrates/mdk-storage-traits/src/groups/mod.rscrates/mdk-storage-traits/src/groups/types.rscrates/mdk-storage-traits/src/groups/validation.rscrates/mdk-storage-traits/src/lib.rscrates/mdk-storage-traits/src/messages/error.rscrates/mdk-storage-traits/src/messages/types.rscrates/mdk-storage-traits/src/test_utils.rscrates/mdk-storage-traits/src/welcomes/error.rscrates/mdk-storage-traits/src/welcomes/mod.rscrates/mdk-storage-traits/src/welcomes/types.rscrates/mdk-storage-traits/src/welcomes/validation.rscrates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
✅ Coverage: 91.51% → 93.64% (+2.13%) |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 174-178: The setters macro invocation mdk_macros::setters!
currently uses "name: impl Into<String>;" and "description: impl Into<String>;"
which expands to .into() calls; change both parameter types to accept String
directly (e.g., "name: String;" and "description: String;") so the generated
setters use owned Strings and avoid .into(); if &str call sites are common,
consider extending mdk_macros::setters! with a dedicated string arm that emits
.to_owned() instead of .into() to preserve ergonomics.
In `@crates/mdk-macros/src/lib.rs`:
- Around line 1-20: The crate root (crates/mdk-macros/src/lib.rs) is missing the
workspace unsafe gate; add a crate-level directive at the top of lib.rs such as
#![forbid(unsafe_code)] (or #![deny(unsafe_code)] if preferred) so the macros
crate inherits the repo-wide prohibition of unsafe code; place it before any
item (e.g., before the doc comment and before #[macro_export]) to ensure it
applies to the whole crate.
- Around line 24-90: The macro arms that generate impl Into setters use the
generic parameter name T which can shadow a surrounding generic and trigger
E0403; update all four "impl Into" arms in both the setters! and mut_setters!
macros to replace the generic parameter T with a private, unlikely-to-conflict
identifier (for example __MdkIntoValue) and update the where clause and
parameter usage accordingly (e.g., pub fn $method<__MdkIntoValue>(mut self,
$field: __MdkIntoValue) -> Self where __MdkIntoValue: Into<$inner> { ... } ),
making the same replacement for $name/$method variants and for the
direct/renamed forms so no generic name collision can occur.
🪄 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: a639f9de-aa89-4034-b733-c674dbf841f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/mdk-core/Cargo.tomlcrates/mdk-core/src/extension/types.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/lib.rscrates/mdk-macros/Cargo.tomlcrates/mdk-macros/src/lib.rscrates/mdk-uniffi/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- crates/mdk-macros/Cargo.toml
- crates/mdk-core/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
✅ Coverage: 91.51% → 93.64% (+2.13%) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/mdk-sqlite-storage/src/db.rs (3)
227-227: Simplify closure to method reference.The closure
|s| UnsignedEvent::from_json(s)can be replaced with the method directly.♻️ Suggested diff
- let event = text_to_parsed(row, "event", |s| UnsignedEvent::from_json(s))?; + let event = text_to_parsed(row, "event", UnsignedEvent::from_json)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/db.rs` at line 227, Replace the inline closure passed to text_to_parsed with the method reference: instead of text_to_parsed(row, "event", |s| UnsignedEvent::from_json(s))?, pass text_to_parsed(row, "event", UnsignedEvent::from_json)? so the call uses the function pointer to UnsignedEvent::from_json directly when constructing event from row.
280-280: Same simplification applies here.♻️ Suggested diff
- let event = text_to_parsed(row, "event", |s| UnsignedEvent::from_json(s))?; + let event = text_to_parsed(row, "event", UnsignedEvent::from_json)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/db.rs` at line 280, Replace the trivial closure passed to text_to_parsed with the function pointer directly: change the call that creates event from text_to_parsed(row, "event", |s| UnsignedEvent::from_json(s)) to use UnsignedEvent::from_json as the parser argument so text_to_parsed(row, "event", UnsignedEvent::from_json) is used; this removes the unnecessary closure while keeping the same behavior (referencing text_to_parsed, UnsignedEvent::from_json, row and the "event" column).
19-63: PreferSelfover the type name in impl blocks.Per coding guidelines, use
Selfinstead of the type name when possible. Within macro-generated impl blocks,Selfresolves correctly and improves consistency.♻️ Suggested diff
impl From<[u8; $len]> for $name { fn from(arr: [u8; $len]) -> Self { - $name(arr) + Self(arr) } }if blob.len() == $len { let mut arr = [0u8; $len]; arr.copy_from_slice(blob); - Ok($name(arr)) + Ok(Self(arr)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/db.rs` around lines 19 - 63, In the sqlite_blob_newtype macro, replace explicit uses of the generated type name ($name) with Self where allowed inside impl blocks: return Self(arr) in impl From<[u8; $len]> for $name, use Self(arr) in the FromSql::column_result Ok path, and use Self where the impl's return type denotes the concrete type (e.g., fn from(val: $name) -> Self in impl From<$name> for [u8; $len]). Keep trait impl headers unchanged but prefer Self for constructors and return types inside the impl bodies to follow the coding guideline.crates/mdk-storage-traits/src/welcomes/types.rs (1)
62-84: Restore direct coverage for every enum mapping.These literals are part of the stored/wire format, but the remaining test in this file only exercises
"processed".Failed, everyWelcomeStatevariant, and the invalid parse paths are now uncovered, so a typo in one mapping would slip through.🧪 Example test shape
@@ #[cfg(test)] mod tests { + use std::str::FromStr; use serde_json::json; @@ fn test_processed_welcome_serialization() { @@ } + + #[test] + fn test_welcome_state_enum_round_trips() { + let processed_cases = [ + (ProcessedWelcomeState::Processed, "processed"), + (ProcessedWelcomeState::Failed, "failed"), + ]; + for (state, expected) in processed_cases { + assert_eq!(state.to_string(), expected); + assert_eq!(serde_json::to_string(&state).unwrap(), format!("\"{expected}\"")); + assert_eq!(ProcessedWelcomeState::from_str(expected).unwrap(), state); + } + + let welcome_cases = [ + (WelcomeState::Pending, "pending"), + (WelcomeState::Accepted, "accepted"), + (WelcomeState::Declined, "declined"), + (WelcomeState::Ignored, "ignored"), + ]; + for (state, expected) in welcome_cases { + assert_eq!(state.to_string(), expected); + assert_eq!(serde_json::to_string(&state).unwrap(), format!("\"{expected}\"")); + assert_eq!(WelcomeState::from_str(expected).unwrap(), state); + } + } + + #[test] + fn test_welcome_state_enum_invalid_values_are_rejected() { + assert!(ProcessedWelcomeState::from_str("invalid").is_err()); + assert!(WelcomeState::from_str("invalid").is_err()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-storage-traits/src/welcomes/types.rs` around lines 62 - 84, Add comprehensive unit tests that directly exercise every enum string mapping and invalid parse paths for ProcessedWelcomeState and WelcomeState: assert that ProcessedWelcomeState::Processed and ::Failed round-trip to/from their string literals ("processed", "failed"), assert that each WelcomeState variant (Pending, Accepted, Declined, Ignored) round-trips to/from their respective literals, and add negative tests that attempt to parse invalid strings to ensure the error path (WelcomeError) triggers; locate the enums ProcessedWelcomeState and WelcomeState in types.rs to wire these assertions to the exact variants and parsing functions used by string_enum!.
🤖 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-storage-traits/src/welcomes/types.rs`:
- Around line 62-84: The generated error messages currently echo the invalid
input via the string_enum! macro (used for ProcessedWelcomeState and
WelcomeState), so replace the format-style invalid_message strings that include
"{}" with fixed messages that do not interpolate the invalid value (e.g. change
"Invalid processed welcome state: {}" to "Invalid processed welcome state" and
"Invalid welcome state: {}" to "Invalid welcome state") so deserialization
failures no longer reflect raw input into WelcomeError.
---
Nitpick comments:
In `@crates/mdk-sqlite-storage/src/db.rs`:
- Line 227: Replace the inline closure passed to text_to_parsed with the method
reference: instead of text_to_parsed(row, "event", |s|
UnsignedEvent::from_json(s))?, pass text_to_parsed(row, "event",
UnsignedEvent::from_json)? so the call uses the function pointer to
UnsignedEvent::from_json directly when constructing event from row.
- Line 280: Replace the trivial closure passed to text_to_parsed with the
function pointer directly: change the call that creates event from
text_to_parsed(row, "event", |s| UnsignedEvent::from_json(s)) to use
UnsignedEvent::from_json as the parser argument so text_to_parsed(row, "event",
UnsignedEvent::from_json) is used; this removes the unnecessary closure while
keeping the same behavior (referencing text_to_parsed, UnsignedEvent::from_json,
row and the "event" column).
- Around line 19-63: In the sqlite_blob_newtype macro, replace explicit uses of
the generated type name ($name) with Self where allowed inside impl blocks:
return Self(arr) in impl From<[u8; $len]> for $name, use Self(arr) in the
FromSql::column_result Ok path, and use Self where the impl's return type
denotes the concrete type (e.g., fn from(val: $name) -> Self in impl From<$name>
for [u8; $len]). Keep trait impl headers unchanged but prefer Self for
constructors and return types inside the impl bodies to follow the coding
guideline.
In `@crates/mdk-storage-traits/src/welcomes/types.rs`:
- Around line 62-84: Add comprehensive unit tests that directly exercise every
enum string mapping and invalid parse paths for ProcessedWelcomeState and
WelcomeState: assert that ProcessedWelcomeState::Processed and ::Failed
round-trip to/from their string literals ("processed", "failed"), assert that
each WelcomeState variant (Pending, Accepted, Declined, Ignored) round-trips
to/from their respective literals, and add negative tests that attempt to parse
invalid strings to ensure the error path (WelcomeError) triggers; locate the
enums ProcessedWelcomeState and WelcomeState in types.rs to wire these
assertions to the exact variants and parsing functions used by string_enum!.
🪄 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: 07d546ca-9f96-40f3-b188-21c75c7d02f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/mdk-core/src/key_packages.rscrates/mdk-sqlite-storage/src/db.rscrates/mdk-sqlite-storage/src/mls_storage/mod.rscrates/mdk-storage-traits/Cargo.tomlcrates/mdk-storage-traits/src/groups/types.rscrates/mdk-storage-traits/src/lib.rscrates/mdk-storage-traits/src/messages/types.rscrates/mdk-storage-traits/src/welcomes/types.rs
✅ Files skipped from review due to trivial changes (1)
- crates/mdk-storage-traits/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mdk-storage-traits/src/messages/types.rs
- crates/mdk-storage-traits/src/groups/types.rs
- crates/mdk-storage-traits/src/lib.rs
✅ Coverage: 91.51% → 93.62% (+2.11%) |
✅ Coverage: 91.51% → 93.74% (+2.23%) |
Contributor
Author
|
Only 2488 effective lines reduced, with I could have done more given that I've personally introduced more lines than that in a single pr lol |
✅ Coverage: 91.57% → 93.72% (+2.15%) |
✅ Coverage: 91.57% → 93.72% (+2.15%) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-sqlite-storage/src/db.rs (1)
297-314:⚠️ Potential issue | 🟡 MinorReject oversized
member_countvalues instead of truncating them.Line 314 narrows a
u64withas u32, so a malformed row aboveu32::MAXsilently wraps. Convert withu32::try_fromand return a conversion error instead.🔎 Safe integer conversion
- let member_count: u64 = row.get("member_count")?; + let member_count = u32::try_from(row.get::<_, u64>("member_count")?).map_err(|_| { + Error::FromSqlConversionFailure( + 0, + Type::Integer, + Box::new(IoError::new(ErrorKind::InvalidData, "Invalid member count")), + ) + })?; @@ - member_count: member_count as u32, + member_count,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/db.rs` around lines 297 - 314, The code currently casts member_count: member_count as u32 which silently truncates values > u32::MAX; instead perform a checked conversion using u32::try_from(member_count) and return an error on failure so oversized values are rejected. Update the construction of Welcome (the member_count field) to call u32::try_from(member_count) and propagate a conversion error (matching the function's error type) rather than using a plain as cast; this touches the same function that reads the row (where member_count is obtained) alongside text_to_state and blob_to_event_id.crates/mdk-memory-storage/src/groups.rs (1)
159-163:⚠️ Potential issue | 🔴 CriticalClamp before adding
offset + limit.Line 161 can overflow on large offsets (for example
usize::MAX), which turns this pagination path into a panic via arithmetic overflow or an invalid slice range. Use a saturating add from the clamped start index.🛡️ Safe pagination bounds
- let start = offset.min(messages.len()); - let end = (offset + limit).min(messages.len()); + let start = offset.min(messages.len()); + let end = start.saturating_add(limit).min(messages.len());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-memory-storage/src/groups.rs` around lines 159 - 163, The pagination currently computes end as (offset + limit).min(messages.len()) which can overflow when offset is large; change the computation to base end off the already-clamped start: compute start = offset.min(messages.len()) and then end = start.saturating_add(limit).min(messages.len()), and return messages[start..end].to_vec(); update the pagination code in the function where start, offset, limit and messages are used to use saturating_add on start.
🧹 Nitpick comments (3)
crates/mdk-storage-traits/src/groups/mod.rs (1)
122-149: Add a shared regression test forlegacy-group-event.The new macro now routes a third exporter-secret namespace, but the shared backend tests in
crates/mdk-core/tests/shared/group_tests.rsonly exercisegroup-eventandencrypted-media. A label typo or cache mix-up in the legacy path would compile and still pass CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-storage-traits/src/groups/mod.rs` around lines 122 - 149, Add a shared regression test that exercises the new "legacy-group-event" exporter-secret namespace to ensure the macros route to the correct cache; specifically, extend the existing shared backend group tests to call get_group_legacy_exporter_secret and save_group_legacy_exporter_secret and verify they read/write to the group_legacy_exporter_secrets_cache using the label "legacy-group-event" (similar to existing tests for "group-event" and "encrypted-media") to catch any label typos or cache mix-ups.crates/mdk-sqlite-storage/src/groups.rs (1)
79-89: Avoid deserializing the full group for an existence guard.
find_group_by_mls_group_id()reads and decodes the entiregroupsrow just to answer a yes/no question, so every guarded path now pays an extra row decode and can fail on unrelated row-shape issues. ASELECT EXISTS(...)check keeps this cheap and isolated.Suggested change
#[inline] fn ensure_group_exists( storage: &MdkSqliteStorage, mls_group_id: &GroupId, ) -> Result<(), GroupError> { - if storage.find_group_by_mls_group_id(mls_group_id)?.is_some() { - Ok(()) - } else { - Err(group_not_found()) - } + let exists = storage.with_connection(|conn| { + conn.query_row( + "SELECT EXISTS(SELECT 1 FROM groups WHERE mls_group_id = ?)", + params![mls_group_id.as_slice()], + |row| row.get::<_, bool>(0), + ) + .map_err(into_group_err) + })?; + + if exists { + Ok(()) + } else { + Err(group_not_found()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/groups.rs` around lines 79 - 89, The ensure_group_exists function currently calls storage.find_group_by_mls_group_id which reads and deserializes the entire groups row; change this to perform a lightweight existence check (e.g., a SELECT EXISTS query) so we don't decode the row. Implement or call a new/existing helper like storage.exists_group_by_mls_group_id(mls_group_id) or run a direct SELECT EXISTS(...) against the same database connection inside ensure_group_exists, return Ok(()) when true and Err(group_not_found()) when false, and keep the function signature and error behavior unchanged.crates/mdk-core/tests/shared/mod.rs (1)
24-186: Use$crate-qualified paths inside the exported macro.
shared::...is resolved at the invocation site, so this harness only works when the caller happens to have asharedmodule in scope.$crate::shared::...makes the macro hygienic and keeps future wrappers or nested test modules from breaking.Suggested change
- shared::group_tests::test_save_and_find_group(storage); + $crate::shared::group_tests::test_save_and_find_group(storage);Apply the same rewrite to the other
shared::...calls in this macro.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-core/tests/shared/mod.rs` around lines 24 - 186, The exported macro storage_backend_tests currently uses bare paths like shared::group_tests::test_save_and_find_group which are resolved at the call site; change all such occurrences to $crate::shared::... (e.g. $crate::shared::group_tests::test_save_and_find_group) so the macro is hygienic; update every invocation inside the macro (all shared::group_tests::..., shared::message_tests::..., shared::welcome_tests::...) to use $crate::shared::... instead.
🤖 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-sqlite-storage/src/db.rs`:
- Around line 20-23: The blob wrapper macro sqlite_blob_newtype currently
derives Debug and Copy which exposes and makes key bytes trivially duplicable;
remove Debug and Copy from the derive list in sqlite_blob_newtype, add Zeroize
and ZeroizeOnDrop derives (from the zeroize crate) so raw bytes are zeroed when
dropped, keep Clone/PartialEq/Eq as needed but do NOT implement Copy, and ensure
any public uses of Hash32 (and places that produce image_key and
group_image_key) wrap the value in Secret<T> before exposing it outside internal
APIs so keys are not printable or clonable.
In `@crates/mdk-sqlite-storage/src/groups.rs`:
- Around line 185-190: The call to validate_group_fields currently passes
usize::MAX which effectively disables the admin-count check; replace usize::MAX
with the real admin limit constant (e.g., MAX_GROUP_ADMINS or whatever project
constant enforces max admins) so validate_group_fields can enforce
admin_pubkeys.len() > max_admins; ensure that constant is defined/imported and
keep the JSON byte-size validation as an additional check rather than a
substitute.
---
Outside diff comments:
In `@crates/mdk-memory-storage/src/groups.rs`:
- Around line 159-163: The pagination currently computes end as (offset +
limit).min(messages.len()) which can overflow when offset is large; change the
computation to base end off the already-clamped start: compute start =
offset.min(messages.len()) and then end =
start.saturating_add(limit).min(messages.len()), and return
messages[start..end].to_vec(); update the pagination code in the function where
start, offset, limit and messages are used to use saturating_add on start.
In `@crates/mdk-sqlite-storage/src/db.rs`:
- Around line 297-314: The code currently casts member_count: member_count as
u32 which silently truncates values > u32::MAX; instead perform a checked
conversion using u32::try_from(member_count) and return an error on failure so
oversized values are rejected. Update the construction of Welcome (the
member_count field) to call u32::try_from(member_count) and propagate a
conversion error (matching the function's error type) rather than using a plain
as cast; this touches the same function that reads the row (where member_count
is obtained) alongside text_to_state and blob_to_event_id.
---
Nitpick comments:
In `@crates/mdk-core/tests/shared/mod.rs`:
- Around line 24-186: The exported macro storage_backend_tests currently uses
bare paths like shared::group_tests::test_save_and_find_group which are resolved
at the call site; change all such occurrences to $crate::shared::... (e.g.
$crate::shared::group_tests::test_save_and_find_group) so the macro is hygienic;
update every invocation inside the macro (all shared::group_tests::...,
shared::message_tests::..., shared::welcome_tests::...) to use
$crate::shared::... instead.
In `@crates/mdk-sqlite-storage/src/groups.rs`:
- Around line 79-89: The ensure_group_exists function currently calls
storage.find_group_by_mls_group_id which reads and deserializes the entire
groups row; change this to perform a lightweight existence check (e.g., a SELECT
EXISTS query) so we don't decode the row. Implement or call a new/existing
helper like storage.exists_group_by_mls_group_id(mls_group_id) or run a direct
SELECT EXISTS(...) against the same database connection inside
ensure_group_exists, return Ok(()) when true and Err(group_not_found()) when
false, and keep the function signature and error behavior unchanged.
In `@crates/mdk-storage-traits/src/groups/mod.rs`:
- Around line 122-149: Add a shared regression test that exercises the new
"legacy-group-event" exporter-secret namespace to ensure the macros route to the
correct cache; specifically, extend the existing shared backend group tests to
call get_group_legacy_exporter_secret and save_group_legacy_exporter_secret and
verify they read/write to the group_legacy_exporter_secrets_cache using the
label "legacy-group-event" (similar to existing tests for "group-event" and
"encrypted-media") to catch any label typos or cache mix-ups.
🪄 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: 5ebb32b7-3625-429c-8852-0622e3b845d6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/mdk-core/src/key_packages.rscrates/mdk-core/tests/shared/group_tests.rscrates/mdk-core/tests/shared/message_tests.rscrates/mdk-core/tests/shared/mod.rscrates/mdk-core/tests/shared/welcome_tests.rscrates/mdk-memory-storage/src/groups.rscrates/mdk-sqlite-storage/src/db.rscrates/mdk-sqlite-storage/src/groups.rscrates/mdk-sqlite-storage/src/lib.rscrates/mdk-sqlite-storage/src/mls_storage/mod.rscrates/mdk-storage-traits/Cargo.tomlcrates/mdk-storage-traits/src/groups/mod.rscrates/mdk-storage-traits/src/groups/types.rscrates/mdk-storage-traits/src/lib.rscrates/mdk-storage-traits/src/messages/types.rscrates/mdk-storage-traits/src/welcomes/types.rs
✅ Files skipped from review due to trivial changes (3)
- crates/mdk-storage-traits/Cargo.toml
- crates/mdk-core/tests/shared/welcome_tests.rs
- crates/mdk-core/tests/shared/message_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-storage-traits/src/welcomes/types.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-macros/src/lib.rs (1)
10-21: Add compile-time coverage for the macro contract.These examples are all
ignoreblocks, so they won't catch expansion drift on their own. If this crate doesn't already have atrybuild/UI suite, I'd add a small matrix for<direct>vs wrapped setters, renamed fields, and nested-Optiongetters.Also applies to: 165-171, 305-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-macros/src/lib.rs` around lines 10 - 21, The doc examples for the setters! macro are marked ignore so they won't detect expansion regressions; add compile-time UI/trybuild tests that exercise the setters! macro across the matrix of variants (plain vs impl Into, <direct> vs wrapped setter, renamed method -> field forms, and nested Option getter behavior) and include positive and negative cases (expected expansions or errors) to catch expansion drift; create trybuild test cases named to reflect each scenario and add them to the crate's test suite so running cargo test/trybuild will verify the macro contract for setters! (and repeat the same test matrix for the other example blocks referenced).
🤖 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-macros/src/lib.rs`:
- Around line 10-21: The doc examples for the setters! macro are marked ignore
so they won't detect expansion regressions; add compile-time UI/trybuild tests
that exercise the setters! macro across the matrix of variants (plain vs impl
Into, <direct> vs wrapped setter, renamed method -> field forms, and nested
Option getter behavior) and include positive and negative cases (expected
expansions or errors) to catch expansion drift; create trybuild test cases named
to reflect each scenario and add them to the crate's test suite so running cargo
test/trybuild will verify the macro contract for setters! (and repeat the same
test matrix for the other example blocks referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0a7a985-1558-42df-b31f-ea1a834deb7c
📒 Files selected for processing (2)
crates/mdk-macros/src/lib.rscrates/mdk-sqlite-storage/src/db.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/mdk-sqlite-storage/src/db.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 (1)
crates/mdk-sqlite-storage/src/groups.rs (1)
346-352:⚠️ Potential issue | 🟠 MajorReapply relay-set validation before mutating the table.
replace_group_relays()now normalizes the missing-group case, but it still deletes and reinserts whatever it is given without running the shared relay validator. That lets SQLite accept oversized relay sets or overlong URLs that the sharedgroups::validationlayer was introduced to reject, so backend behavior can diverge from memory storage. Please callvalidate_relay_set(...)before opening the savepoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/groups.rs` around lines 346 - 352, In replace_group_relays, call the shared validator validate_relay_set(&relays) (or the crate's equivalent) before mutating the DB: after ensure_group_exists(self, group_id)? and before opening the savepoint/transaction and performing delete/insert operations, return any validation error instead of proceeding; this ensures oversized relay sets or overlong RelayUrl values are rejected consistently with groups::validation.
🧹 Nitpick comments (1)
crates/mdk-memory-storage/src/groups.rs (1)
5-41: Consider movingusestatements above macro definitions.Per coding guidelines, all
usestatements should be placed at the TOP of their containing scope. Currently, thegroup_exporter_secret_get!andgroup_exporter_secret_save!macros (lines 5-33) are defined before theusestatements (lines 35-41).📦 Suggested reorder
//! Memory-based storage implementation of the MdkStorageProvider trait for MDK groups use std::collections::BTreeSet; +use mdk_storage_traits::GroupId; +use mdk_storage_traits::groups::error::GroupError; +use mdk_storage_traits::groups::types::*; +use mdk_storage_traits::groups::validation::{validate_group_fields, validate_relay_set}; +use mdk_storage_traits::groups::{ + GroupStorage, MessageSortOrder, Pagination, group_not_found, validate_message_limit, +}; +use mdk_storage_traits::messages::types::Message; +use nostr::{PublicKey, RelayUrl}; + +use crate::MdkMemoryStorage; + /// Implements `get_group_*_exporter_secret` for the memory backend. ... - -use mdk_storage_traits::GroupId; -...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-memory-storage/src/groups.rs` around lines 5 - 41, Move the module-level use statements so they appear before the macro definitions: place the lines importing GroupId, GroupError, the groups::types, validation functions, and GroupStorage-related items above the macro_rules! group_exporter_secret_get and macro_rules! group_exporter_secret_save definitions; this keeps the imports at the top of the scope and preserves the macro bodies (group_exporter_secret_get! and group_exporter_secret_save!) unchanged except for their new position below the use declarations.
🤖 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-sqlite-storage/src/groups.rs`:
- Around line 346-352: In replace_group_relays, call the shared validator
validate_relay_set(&relays) (or the crate's equivalent) before mutating the DB:
after ensure_group_exists(self, group_id)? and before opening the
savepoint/transaction and performing delete/insert operations, return any
validation error instead of proceeding; this ensures oversized relay sets or
overlong RelayUrl values are rejected consistently with groups::validation.
---
Nitpick comments:
In `@crates/mdk-memory-storage/src/groups.rs`:
- Around line 5-41: Move the module-level use statements so they appear before
the macro definitions: place the lines importing GroupId, GroupError, the
groups::types, validation functions, and GroupStorage-related items above the
macro_rules! group_exporter_secret_get and macro_rules!
group_exporter_secret_save definitions; this keeps the imports at the top of the
scope and preserves the macro bodies (group_exporter_secret_get! and
group_exporter_secret_save!) unchanged except for their new position below the
use declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4425e3d0-7204-491d-b32c-3882e7029f7e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/mdk-core/src/key_packages.rscrates/mdk-core/tests/shared/group_tests.rscrates/mdk-core/tests/shared/message_tests.rscrates/mdk-core/tests/shared/mod.rscrates/mdk-core/tests/shared/welcome_tests.rscrates/mdk-macros/src/lib.rscrates/mdk-memory-storage/src/groups.rscrates/mdk-sqlite-storage/src/db.rscrates/mdk-sqlite-storage/src/groups.rscrates/mdk-sqlite-storage/src/lib.rscrates/mdk-sqlite-storage/src/mls_storage/mod.rscrates/mdk-storage-traits/Cargo.tomlcrates/mdk-storage-traits/src/groups/mod.rscrates/mdk-storage-traits/src/groups/types.rscrates/mdk-storage-traits/src/lib.rscrates/mdk-storage-traits/src/messages/types.rscrates/mdk-storage-traits/src/welcomes/types.rs
✅ Files skipped from review due to trivial changes (4)
- crates/mdk-core/tests/shared/group_tests.rs
- crates/mdk-core/tests/shared/welcome_tests.rs
- crates/mdk-storage-traits/Cargo.toml
- crates/mdk-storage-traits/src/welcomes/types.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/mdk-macros/src/lib.rs
- crates/mdk-storage-traits/src/messages/types.rs
- crates/mdk-storage-traits/src/lib.rs
- crates/mdk-storage-traits/src/groups/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-sqlite-storage/src/lib.rs (1)
78-85: Move generic trait bounds to awhereclause indb_error_fn!.This helper currently uses an inline bound (
T: std::error::Error), which diverges from the repo’s Rust style rule.♻️ Suggested patch
macro_rules! db_error_fn { ($fn_name:ident, $error_type:ty) => { #[inline] - fn $fn_name<T: std::error::Error>(e: T) -> $error_type { + fn $fn_name<T>(e: T) -> $error_type + where + T: std::error::Error, + { <$error_type>::DatabaseError(e.to_string()) } }; }As per coding guidelines, “All trait bounds in
whereclauses, not inline”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mdk-sqlite-storage/src/lib.rs` around lines 78 - 85, The macro db_error_fn! currently generates functions with an inline generic bound `T: std::error::Error`; change the generated function signature to move that bound into a where clause (i.e., generate `fn $fn_name<T>(e: T) -> $error_type where T: std::error::Error { ... }`) so all trait bounds follow the repo style; keep the body returning `<$error_type>::DatabaseError(e.to_string())` and preserve #[inline] and macro argument names ($fn_name, $error_type).
🤖 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-sqlite-storage/src/lib.rs`:
- Around line 78-85: The macro db_error_fn! currently generates functions with
an inline generic bound `T: std::error::Error`; change the generated function
signature to move that bound into a where clause (i.e., generate `fn
$fn_name<T>(e: T) -> $error_type where T: std::error::Error { ... }`) so all
trait bounds follow the repo style; keep the body returning
`<$error_type>::DatabaseError(e.to_string())` and preserve #[inline] and macro
argument names ($fn_name, $error_type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8640c292-3668-4bfe-a630-8b0ed27879a1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/mdk-core/src/key_packages.rscrates/mdk-core/tests/shared/group_tests.rscrates/mdk-core/tests/shared/message_tests.rscrates/mdk-core/tests/shared/mod.rscrates/mdk-core/tests/shared/welcome_tests.rscrates/mdk-macros/src/lib.rscrates/mdk-memory-storage/src/groups.rscrates/mdk-sqlite-storage/src/db.rscrates/mdk-sqlite-storage/src/groups.rscrates/mdk-sqlite-storage/src/lib.rscrates/mdk-sqlite-storage/src/mls_storage/mod.rscrates/mdk-storage-traits/Cargo.tomlcrates/mdk-storage-traits/src/groups/mod.rscrates/mdk-storage-traits/src/groups/types.rscrates/mdk-storage-traits/src/lib.rscrates/mdk-storage-traits/src/messages/types.rscrates/mdk-storage-traits/src/welcomes/types.rs
✅ Files skipped from review due to trivial changes (3)
- crates/mdk-core/tests/shared/welcome_tests.rs
- crates/mdk-storage-traits/Cargo.toml
- crates/mdk-core/tests/shared/message_tests.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/mdk-core/src/key_packages.rs
- crates/mdk-macros/src/lib.rs
- crates/mdk-core/tests/shared/mod.rs
- crates/mdk-memory-storage/src/groups.rs
✅ Coverage: 91.36% → 93.54% (+2.18%) |
erskingardner
approved these changes
Apr 2, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Large-scale refactor across the MDK workspace to eliminate hand-written boilerplate. Introduces a new
mdk-macroscrate as the colony's stamp press — generating setters, getters, state enums, pagination types, storage errors, and cross-backend test harnesses so no marmot has to write the same tunnel twice.Changes
New crate:
mdk-macrossetters!,mut_setters!,ref_getters!— builder and accessor generationstring_enum!,bounded_pagination!,storage_error!— type and error standardizationstorage_backend_tests!— shared test harness macro; cross-backend test wrappers collapse to a single lineValidation consolidation (
mdk-storage-traits)groups::validationandwelcomes::validationmodules; both memory and SQLite backends call the same validators instead of duplicating the logicSQLite cleanup (
mdk-sqlite-storage)blob_to_*/text_to_*parsing helpers centralizeddb_error_fn!macro replaces repeatedinto_*_errboilerplateMemory storage cleanup (
mdk-memory-storage)mls_store_base!,mls_single_key_ops!,validation_limits!,StorageProvidergenerators remove ~300 lines of MLS in-memory store scaffoldingCredential hardening (
mdk-core)pubkey_from_credentialcentralizes MLS credential public-key extraction; all pubkey lookup paths now go through one non-public helperTest reorganization
mdk-core/tests/shared; per-backend wrappers replaced withstorage_backend_tests!invocationsWhat didn't change
Reviewers: pay attention to
pubkey_from_credential— centralized identity binding for MLS credentials; confirm error semantics are preservedDisplayimplementations for storage errors — verify no sensitive identifiers are now leaked or obscuredThis PR is a large-scale refactor that eliminates hand-written boilerplate across the MDK workspace by introducing generator macros, centralizing validation, and consolidating test harnesses to reduce duplicated code and maintenance burden. It moves many repetitive implementations into macro expansions (new crate mdk-macros) and shared helper modules so that memory and SQLite backends reuse the same validation and parsing logic while preserving protocol and cryptographic behavior.
What changed:
Security impact:
Protocol changes:
API surface:
Testing: