fix: add pagination to prevent memory exhaustion in pending_welcomes by mubarakcoded · Pull Request #110 · marmot-protocol/mdk

mubarakcoded · 2026-01-02T16:57:39Z

Summary by CodeRabbit

Breaking Changes
- Public pending_welcomes/get_pending_welcomes APIs now accept an optional Pagination parameter; callers should pass None for default behavior. A previous offset limit was removed.
New Features
- Paginated retrieval of pending welcomes with a public Pagination type and configurable limit/offset.
- New paginated binding for foreign-language clients.
Bug Fixes
- Pagination added to prevent memory exhaustion and ensure deterministic ordering.
Tests
- Expanded pagination tests covering ordering, boundaries, overlaps, and invalid limits.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-02T16:57:48Z

Walkthrough

Replace parameterless pending_welcomes with an optional pagination-aware API using a new Pagination struct and limit constants; update storage implementations, core, UniFFI, examples, and tests to accept/propagate pagination, add validation, ordering, and pagination-focused tests.

Changes

Cohort / File(s)	Summary
Storage trait & pagination types `crates/mdk-storage-traits/src/welcomes/mod.rs`, `crates/mdk-storage-traits/CHANGELOG.md`, `crates/mdk-storage-traits/src/test_utils.rs`	Add `Pagination` struct and constants `DEFAULT_PENDING_WELCOMES_LIMIT` / `MAX_PENDING_WELCOMES_LIMIT`; change `WelcomeStorage::pending_welcomes` signature to accept `Option<Pagination>`; provide defaulting and accessor methods; update docs and test call sites; remove prior offset-constant validation.
In-memory storage implementation `crates/mdk-memory-storage/src/welcomes.rs`, `crates/mdk-memory-storage/CHANGELOG.md`	Replace no-arg `pending_welcomes()` with `pending_welcomes(&self, pagination: Option<Pagination>)`; validate `limit`, filter Pending welcomes, sort by `id` DESC, apply offset/limit slicing; add unit tests for pagination and invalid-limit handling.
SQLite storage implementation `crates/mdk-sqlite-storage/src/welcomes.rs`, `crates/mdk-sqlite-storage/CHANGELOG.md`	Update `pending_welcomes(&self, pagination: Option<Pagination>)` to extract/validate `limit`/`offset`; use `ORDER BY id DESC LIMIT ? OFFSET ?` with parameters; update tests to exercise pagination pages, ordering, offsets, and invalid-limit errors.
Core API & examples `crates/mdk-core/src/welcomes.rs`, `crates/mdk-core/CHANGELOG.md`, `crates/mdk-core/examples/...`, `crates/mdk-core/src/key_packages.rs`	Change `get_pending_welcomes` to accept `Option<Pagination>` and forward to storage; update examples and tests to call `get_pending_welcomes(None)` or `Some(Pagination::new(...))`; add documentation and tests demonstrating pagination behavior.
UniFFI bindings `crates/mdk-uniffi/src/lib.rs`, `crates/mdk-uniffi/CHANGELOG.md`	Export `Pagination` in welcomes imports; add `get_pending_welcomes_paginated(limit: Option<u32>, offset: Option<u32>)` that constructs `Pagination` and delegates to core; retain non-paginated path calling core with `None`.
Tests & shared test updates `crates/mdk-storage-traits/tests/shared/welcome_tests.rs`, `crates/mdk-memory-storage/src/welcomes.rs` (tests), `crates/mdk-sqlite-storage/src/welcomes.rs` (tests), other tests...	Update call sites to new signature (`pending_welcomes(None)` or with `Pagination`); add pagination-specific tests covering pages, ordering, overlaps, empty results, and invalid-limit error cases.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the primary change: adding pagination to the pending_welcomes method to address a memory exhaustion vulnerability.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-01-02T16:59:39Z

✅ Coverage: 86.35% → 86.38% (+0.03%)

coderabbitai

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 (1)

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

117-144: Add PR reference links to CHANGELOG entries for pagination changes.

CHANGELOG.md files were updated for the three affected crates, but the pagination entries in all three (mdk-storage-traits, mdk-memory-storage, mdk-sqlite-storage) are missing PR reference links. Each pagination entry must include the PR link in the format ([#XXX](https://github.com/marmot-protocol/mdk/pull/XXX)), as documented in the project's changelog guidelines.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fedc362 and 7863f20.

📒 Files selected for processing (6)

crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-memory-storage/src/welcomes.rs
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-sqlite-storage/src/welcomes.rs
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-storage-traits/src/welcomes/mod.rs

🧰 Additional context used

📓 Path-based instructions (3)

crates/*/CHANGELOG.md

📄 CodeRabbit inference engine (AGENTS.md)

crates/*/CHANGELOG.md: Every change that modifies functionality must update the appropriate crate CHANGELOG.md file (mdk-core, mdk-storage-traits, mdk-memory-storage, or mdk-sqlite-storage)
Follow Keep a Changelog format with sections for Unreleased, Breaking changes, Changed, Added, Fixed, Removed, and Deprecated in CHANGELOG files
Always include a PR reference link in changelog entries using the format ([#123](https://github.com/marmot-protocol/mdk/pull/123))

Files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Never use bare URLs in markdown files. All URLs must be properly formatted: use [link text](url) for inline links, <url> for standalone URLs, ([#123](https://github.com/marmot-protocol/mdk/pull/123)) for PR/issue references, and ([abc1234](https://github.com/.../commit/abc1234...)) for commit references

Files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds must be placed in where clauses, not inline with generic parameters in Rust code
Use Self instead of the type name when possible in Rust code
Derive macros must be ordered as: Debug, Clone, Copy, PartialEq, Eq, Hash in Rust code
Always use tracing::warn!(...) with explicit module path, never im...

Files:

crates/mdk-storage-traits/src/welcomes/mod.rs
crates/mdk-memory-storage/src/welcomes.rs
crates/mdk-sqlite-storage/src/welcomes.rs

🧠 Learnings (3)

📚 Learning: 2026-01-01T16:28:05.759Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to crates/*/CHANGELOG.md : Every change that modifies functionality must update the appropriate crate CHANGELOG.md file (mdk-core, mdk-storage-traits, mdk-memory-storage, or mdk-sqlite-storage)

Applied to files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-memory-storage/src/welcomes.rs

📚 Learning: 2026-01-01T16:28:05.759Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to crates/*/CHANGELOG.md : Follow Keep a Changelog format with sections for Unreleased, Breaking changes, Changed, Added, Fixed, Removed, and Deprecated in CHANGELOG files

Applied to files:

crates/mdk-sqlite-storage/CHANGELOG.md

📚 Learning: 2026-01-01T15:51:58.505Z

Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.

Applied to files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md

🧬 Code graph analysis (2)

crates/mdk-storage-traits/src/welcomes/mod.rs (2)

crates/mdk-memory-storage/src/welcomes.rs (1)

pending_welcomes_paginated (18-37)

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

pending_welcomes_paginated (117-144)

crates/mdk-memory-storage/src/welcomes.rs (2)

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

pending_welcomes_paginated (117-144)

crates/mdk-storage-traits/src/welcomes/mod.rs (1)

pending_welcomes_paginated (48-52)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Test Coverage

🔇 Additional comments (3)

crates/mdk-storage-traits/src/welcomes/mod.rs (1)

18-36: LGTM! Well-designed backward-compatible pagination.

The constant provides a sensible default limit to prevent memory exhaustion, and the default implementation maintains backward compatibility by delegating to the paginated method. The documentation clearly explains the behavior.

crates/mdk-sqlite-storage/src/welcomes.rs (2)

117-144: Implementation looks correct with proper ordering and pagination.

The SQL query correctly implements pagination with ORDER BY id DESC LIMIT ? OFFSET ?, ensuring consistent ordering. The implementation properly handles result mapping and error conversion.

Minor note: The cast from usize to i64 on Line 133 could theoretically overflow on 64-bit systems if limit or offset exceed i64::MAX, though this is extremely unlikely in practice. If defensive programming is a priority, consider adding a validation that caps these values or documents the assumption.

328-372: Excellent comprehensive test coverage for pagination.

The test thoroughly validates:

Default behavior with pending_welcomes()

Multiple page retrievals with different offsets

Edge case when offset exceeds available data

Non-overlapping pages

This provides strong confidence in the pagination implementation.

crates/mdk-memory-storage/CHANGELOG.md

crates/mdk-memory-storage/src/welcomes.rs

crates/mdk-sqlite-storage/CHANGELOG.md

crates/mdk-storage-traits/CHANGELOG.md

crates/mdk-storage-traits/src/welcomes/mod.rs

github-actions · 2026-01-02T17:23:31Z

✅ Coverage: 86.35% → 86.45% (+0.10%)

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/mdk-memory-storage/src/welcomes.rs (1)
75-157: Excellent test coverage! Minor import ordering suggestion.

The test comprehensively covers pagination boundaries, empty results, page non-overlap, ordering, and error cases. This fully addresses the past review comment requesting comprehensive pagination tests.

As per coding guidelines, reorder imports to: (1) external crates, (2) current sub-modules, (3) internal crate modules.
🔎 Suggested import order per coding guidelines
-    use super::*;
-    use mdk_storage_traits::GroupId;
-    use mdk_storage_traits::test_utils::cross_storage::create_test_welcome;
-    use nostr::EventId;
-    use openmls_memory_storage::MemoryStorage;
+    use nostr::EventId;
+    use openmls_memory_storage::MemoryStorage;
+
+    use super::*;
+
+    use mdk_storage_traits::GroupId;
+    use mdk_storage_traits::test_utils::cross_storage::create_test_welcome;

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7863f20 and 98b403a.

📒 Files selected for processing (6)

crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-memory-storage/src/welcomes.rs
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-sqlite-storage/src/welcomes.rs
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-storage-traits/src/welcomes/mod.rs

🚧 Files skipped from review as they are similar to previous changes (3)

crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds must be placed in where clauses, not inline with generic parameters in Rust code
Use Self instead of the type name when possible in Rust code
Derive macros must be ordered as: Debug, Clone, Copy, PartialEq, Eq, Hash in Rust code
Always use tracing::warn!(...) with explicit module path, never im...

Files:

crates/mdk-storage-traits/src/welcomes/mod.rs
crates/mdk-sqlite-storage/src/welcomes.rs
crates/mdk-memory-storage/src/welcomes.rs

🧠 Learnings (1)

📚 Learning: 2026-01-01T16:28:05.759Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to **/*.rs : When adding or modifying code related to encrypted media, ensure tests are run with the `mip04` feature using `cargo test --features mip04`

Applied to files:

crates/mdk-memory-storage/src/welcomes.rs

🧬 Code graph analysis (1)

crates/mdk-storage-traits/src/welcomes/mod.rs (2)

crates/mdk-memory-storage/src/welcomes.rs (1)

pending_welcomes_paginated (18-46)

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

pending_welcomes_paginated (117-153)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Test Coverage

🔇 Additional comments (5)

crates/mdk-memory-storage/src/welcomes.rs (1)

18-46: LGTM! Pagination implementation is correct and consistent.

The implementation properly validates the limit, filters pending welcomes, sorts by ID descending (consistent with the SQLite backend), and applies pagination correctly. The error handling is clear and informative.

crates/mdk-storage-traits/src/welcomes/mod.rs (2)

18-22: Well-designed constants that address security concerns.

The constants provide clear bounds for pending welcomes queries, directly addressing the memory exhaustion vulnerability. The default of 1000 is practical, and the maximum of 10000 prevents abuse while maintaining flexibility. This fully addresses the past review comment requesting limit validation.

33-67: Excellent API design with comprehensive documentation.

The trait design elegantly addresses the vulnerability while maintaining backward compatibility through the default implementation. The documentation thoroughly covers all aspects: parameters, validation, ordering, errors, and usage guidance. The delegation pattern ensures consistent behavior across implementations.

crates/mdk-sqlite-storage/src/welcomes.rs (2)

117-153: LGTM! SQL implementation correctly mirrors in-memory version.

The implementation properly validates limits, uses database-level pagination with ORDER BY id DESC LIMIT ? OFFSET ?, and maintains consistency with the memory storage backend. The error handling and validation are identical across both implementations, ensuring predictable behavior.

337-401: Comprehensive pagination test coverage.

The test thoroughly validates pagination behavior including boundary conditions, error cases, and page non-overlap. The parallel structure with the memory storage tests ensures consistent behavior across both backends, which is critical for a storage abstraction layer.

github-actions · 2026-01-02T21:45:04Z

✅ Coverage: 86.35% → 86.47% (+0.12%)

jgmontoya

The get_pending_welcomes public method in crates/mdk-core/src/welcomes.rs used to return all of the pending welcomes, and now it just returns the first page. This is an undocumented breaking change.

Also, I think it's a bit odd not to have any exposed any way to perform the pagination 'from the outside' as a user. I'd add some method to the public interface so clients can access any page and are not limited to the first one.

crates/mdk-memory-storage/src/welcomes.rs

crates/mdk-sqlite-storage/src/welcomes.rs

crates/mdk-storage-traits/src/welcomes/mod.rs

dannym-arx · 2026-01-03T06:59:43Z

oh, and I agree with javier, get_pending_welcomes in mdk-core should absolutely use the pagination struct.

and you also need to make sure that get_pending_welcomes in mdk-uniffi uses that struct, so non-rust users of mdk also benefit

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

crates/mdk-memory-storage/src/welcomes.rs (1)
27-32: Consider using direct comparison for clarity.

While the current contains() check is functionally correct and O(1), a past reviewer suggested using if limit < 1 || limit > MAX_PENDING_WELCOMES_LIMIT for improved readability.

Based on learnings, a past reviewer preferred the explicit comparison approach.
🔎 Optional refactor for readability
-        if !(1..=MAX_PENDING_WELCOMES_LIMIT).contains(&limit) {
+        if limit < 1 || limit > MAX_PENDING_WELCOMES_LIMIT {
             return Err(WelcomeError::InvalidParameters(format!(
                 "Limit must be between 1 and {}, got {}",
                 MAX_PENDING_WELCOMES_LIMIT, limit
             )));
         }
crates/mdk-storage-traits/src/welcomes/mod.rs (1)
24-31: Add PartialEq and Eq derives per coding guidelines.

The Pagination struct is a simple public type with Copy fields, and the coding guidelines recommend deriving PartialEq and Eq for such types. This would improve testability and enable use in comparisons and collections.

As per coding guidelines, derive order should include PartialEq and Eq for public types when possible.
🔎 Suggested enhancement
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub struct Pagination {
     /// Maximum number of welcomes to return
     pub limit: Option<usize>,
     /// Number of welcomes to skip
     pub offset: Option<usize>,
 }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6a4bf6 and c7a521f.

📒 Files selected for processing (12)

crates/mdk-core/CHANGELOG.md
crates/mdk-core/src/welcomes.rs
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-memory-storage/src/welcomes.rs
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-sqlite-storage/src/welcomes.rs
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-storage-traits/src/test_utils.rs
crates/mdk-storage-traits/src/welcomes/mod.rs
crates/mdk-storage-traits/tests/shared/welcome_tests.rs
crates/mdk-uniffi/CHANGELOG.md
crates/mdk-uniffi/src/lib.rs

🧰 Additional context used

📓 Path-based instructions (3)

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds in generics must be placed in where clauses, not inline
Use Self instead of the type name when possible
Derive order must be: Debug, Clone, Copy, PartialEq, Eq, Hash (in this order)
Always use tracing::warn!(...), never import and use warn!(...)
Use .to_string() or .to_owned() for string ...

Files:

crates/mdk-storage-traits/src/test_utils.rs
crates/mdk-sqlite-storage/src/welcomes.rs
crates/mdk-core/src/welcomes.rs
crates/mdk-uniffi/src/lib.rs
crates/mdk-memory-storage/src/welcomes.rs
crates/mdk-storage-traits/tests/shared/welcome_tests.rs
crates/mdk-storage-traits/src/welcomes/mod.rs

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Never use bare URLs in markdown files; all URLs must be properly formatted using [link text](url), <url>, or ([#123](url)) for PR/issue references

Files:

crates/mdk-core/CHANGELOG.md
crates/mdk-uniffi/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md

crates/*/CHANGELOG.md

📄 CodeRabbit inference engine (AGENTS.md)

Every crate CHANGELOG entry must include a PR reference using the format ([#123](https://github.com/marmot-protocol/mdk/pull/123))

Files:

crates/mdk-core/CHANGELOG.md
crates/mdk-uniffi/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md

🧠 Learnings (3)

📚 Learning: 2026-01-01T15:51:58.505Z

Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.

Applied to files:

crates/mdk-core/CHANGELOG.md
crates/mdk-uniffi/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md

📚 Learning: 2026-01-03T08:39:39.771Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.771Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate

Applied to files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md

📚 Learning: 2026-01-03T08:39:39.771Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.771Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`

Applied to files:

crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md

🧬 Code graph analysis (5)

crates/mdk-sqlite-storage/src/welcomes.rs (2)

crates/mdk-memory-storage/src/welcomes.rs (1)

pending_welcomes (18-48)

crates/mdk-storage-traits/src/welcomes/mod.rs (4)

pending_welcomes (96-99)

limit (40-42)

offset (45-47)

new (35-37)

crates/mdk-core/src/welcomes.rs (1)

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

get_pending_welcomes_paginated (290-302)

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

crates/mdk-core/src/welcomes.rs (1)

get_pending_welcomes_paginated (76-85)

crates/mdk-storage-traits/src/welcomes/mod.rs (3)

limit (40-42)

offset (45-47)

new (35-37)

crates/mdk-storage-traits/tests/shared/welcome_tests.rs (1)

crates/mdk-core/src/lib.rs (2)

storage (205-207)

storage (295-297)

crates/mdk-storage-traits/src/welcomes/mod.rs (2)

crates/mdk-memory-storage/src/welcomes.rs (1)

pending_welcomes (18-48)

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

pending_welcomes (117-155)

🔇 Additional comments (22)

crates/mdk-uniffi/CHANGELOG.md (1)

34-35: LGTM! CHANGELOG entry is properly formatted.

The entry correctly documents the new get_pending_welcomes_paginated() method and includes the required PR reference in the proper format.

crates/mdk-core/CHANGELOG.md (2)

36-37: LGTM! Well-documented addition.

The CHANGELOG entry clearly describes the new paginated API method and includes the required PR reference.

43-43: LGTM! Security fix properly documented.

The entry correctly categorizes this as a security fix for Audit Issue AA and includes the required PR reference.

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

5-5: LGTM! Correct import addition.

The Pagination import is properly placed with other external crate imports.

49-51: LGTM! Good delegation pattern for backward compatibility.

The existing method now delegates to the paginated version with None, maintaining the API while enabling pagination internally.

53-85: LGTM! Excellent implementation and documentation.

The new paginated API is well-designed with:

Clear, comprehensive documentation including usage examples

Clean method signature accepting Option<Pagination>

Proper delegation to the storage layer

Consistent error handling

The examples demonstrate common use cases (default pagination, first page, next page) which will help users adopt this API correctly.

crates/mdk-sqlite-storage/CHANGELOG.md (1)

30-54: LGTM! All CHANGELOG entries properly formatted.

All entries now include the required PR references in the correct format. The previous review comments about missing PR references have been addressed. The entries are well-organized across appropriate sections (Breaking changes, Changed, Added, Fixed, Removed).

crates/mdk-memory-storage/CHANGELOG.md (1)

30-47: LGTM! All CHANGELOG entries properly formatted.

All entries now include the required PR references in the correct format. The previous review comments about missing PR references have been addressed. The changelog accurately documents the breaking changes, improvements, additions, security fixes, and removals related to pagination support.

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

601-601: LGTM!

The update correctly adapts to the new pending_welcomes(pagination: Option<Pagination>) API signature. Passing None uses the default pagination behavior, maintaining the original test intent.

crates/mdk-storage-traits/CHANGELOG.md (1)

30-47: LGTM!

The changelog entries are well-documented with proper PR references, correctly categorized breaking changes, and clear security annotations. The changes accurately reflect the pagination refactor.

crates/mdk-storage-traits/tests/shared/welcome_tests.rs (1)

44-44: LGTM!

The update correctly adapts to the new pagination-aware API. Passing None maintains the original test behavior while conforming to the updated trait signature.

crates/mdk-sqlite-storage/src/welcomes.rs (4)

5-5: LGTM!

The import additions for MAX_PENDING_WELCOMES_LIMIT and Pagination are necessary for the pagination implementation and properly placed.

117-155: Well-implemented pagination with proper validation.

The implementation correctly:

Validates limit bounds (1 to MAX_PENDING_WELCOMES_LIMIT)

Uses parameterized queries to prevent SQL injection

Orders results consistently by ID descending

Handles errors appropriately

The removal of offset validation aligns with the design decision to support legitimate large-scale use cases.

247-247: LGTM!

Test correctly updated to use the new pagination-aware API.

339-416: Excellent comprehensive pagination test coverage.

The test thoroughly validates:

Default pagination behavior

Offset and limit functionality

Boundary conditions (zero limit, excessive limit)

Large offset handling (confirming no MAX_OFFSET constraint)

Page overlap prevention

This provides strong confidence in the pagination implementation.

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

25-28: LGTM!

Import updates correctly add Pagination while maintaining the existing import structure and conventions.

280-302: Well-implemented UniFFI binding for paginated welcomes.

The method correctly:

Documents parameters and return values

Accepts FFI-compatible u32 parameters

Safely converts to internal usize types

Delegates to the core pagination API

Maps results to the UniFFI-exposed type

The implementation properly exposes pagination to non-Rust clients as mentioned in the PR objectives.

crates/mdk-memory-storage/src/welcomes.rs (2)

3-8: Import order follows coding guidelines.

The imports are correctly ordered: external crates (mdk_storage_traits, nostr) followed by internal crate modules (crate::MdkMemoryStorage), and all imports are at the file top as required.

77-174: Excellent test coverage for pagination behavior.

The test module comprehensively covers pagination scenarios including boundaries, empty results, ordering, error cases, and page non-overlap. This directly addresses the past review concern about missing pagination tests.

crates/mdk-storage-traits/src/welcomes/mod.rs (3)

18-22: Well-defined limits prevent resource exhaustion.

The constants provide sensible defaults and maximum bounds, directly addressing the memory exhaustion concern mentioned in the PR objectives and past review comments.

33-57: Pagination implementation is correct and well-documented.

The accessor methods correctly unwrap to defaults, and the Default implementation provides sensible values that align with the defined constants.

68-99: Comprehensive trait documentation with clear API contract.

The documentation clearly specifies parameter behavior, error conditions, ordering guarantees, and provides practical examples. The updated signature unifying the API (using Option<Pagination>) addresses past review feedback.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

280-302: Documentation is accurate; consider adding test coverage for pagination.

The default limit value of 1000 is correctly documented and matches the DEFAULT_PENDING_WELCOMES_LIMIT constant in the codebase. While tests for pagination exist in the core module, adding a test for get_pending_welcomes_paginated in the uniffi module would ensure the FFI boundary properly handles pagination parameters.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7a521f and 41d71a6.

📒 Files selected for processing (7)

crates/mdk-core/CHANGELOG.md
crates/mdk-core/examples/mls_memory.rs
crates/mdk-core/examples/mls_sqlite.rs
crates/mdk-core/src/key_packages.rs
crates/mdk-core/src/welcomes.rs
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-uniffi/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (1)

crates/mdk-storage-traits/CHANGELOG.md

🧰 Additional context used

📓 Path-based instructions (3)

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds in generics must be placed in where clauses, not inline
Use Self instead of the type name when possible
Derive order must be: Debug, Clone, Copy, PartialEq, Eq, Hash (in this order)
Always use tracing::warn!(...), never import and use warn!(...)
Use .to_string() or .to_owned() for string ...

Files:

crates/mdk-uniffi/src/lib.rs
crates/mdk-core/examples/mls_sqlite.rs
crates/mdk-core/examples/mls_memory.rs
crates/mdk-core/src/key_packages.rs
crates/mdk-core/src/welcomes.rs

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Never use bare URLs in markdown files; all URLs must be properly formatted using [link text](url), <url>, or ([#123](url)) for PR/issue references

Files:

crates/mdk-core/CHANGELOG.md

crates/*/CHANGELOG.md

📄 CodeRabbit inference engine (AGENTS.md)

Every crate CHANGELOG entry must include a PR reference using the format ([#123](https://github.com/marmot-protocol/mdk/pull/123))

Files:

crates/mdk-core/CHANGELOG.md

🧠 Learnings (3)

📚 Learning: 2026-01-03T08:39:39.771Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.771Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`

Applied to files:

crates/mdk-core/CHANGELOG.md

📚 Learning: 2026-01-03T08:39:39.771Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.771Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate

Applied to files:

crates/mdk-core/CHANGELOG.md

📚 Learning: 2026-01-01T15:51:58.505Z

Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.

Applied to files:

crates/mdk-core/CHANGELOG.md

🧬 Code graph analysis (2)

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

crates/mdk-storage-traits/src/welcomes/mod.rs (3)

limit (40-42)

offset (45-47)

new (35-37)

crates/mdk-core/src/welcomes.rs (2)

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

get_pending_welcomes (271-278)

crates/mdk-storage-traits/src/welcomes/mod.rs (1)

new (35-37)

🔇 Additional comments (16)

crates/mdk-core/CHANGELOG.md (3)

30-30: LGTM! Clear breaking change documentation.

The breaking change entry correctly documents the API change with clear migration guidance. The PR reference is properly formatted.

38-39: LGTM! Pagination additions properly documented.

The Added section entries clearly document the new pagination support in the public API, with proper PR references.

46-46: LGTM! Security fix properly documented.

The security fix entry correctly references Audit Issue AA and clearly describes the pagination protection against memory exhaustion.

crates/mdk-core/src/key_packages.rs (1)

2059-2059: LGTM! Test correctly updated for pagination API.

The test properly passes None to use default pagination, aligning with the new API signature.

crates/mdk-core/examples/mls_sqlite.rs (2)

112-112: LGTM! Example correctly demonstrates new API.

The example properly shows passing None for default pagination behavior.

288-288: LGTM! Consistent API usage across example.

Second usage correctly matches the first, demonstrating consistent usage of the pagination parameter.

crates/mdk-core/examples/mls_memory.rs (2)

103-103: LGTM! Example correctly demonstrates new API.

The memory storage example properly shows passing None for default pagination.

276-276: LGTM! Consistent API usage across example.

Second usage correctly matches the first, demonstrating consistent usage pattern.

crates/mdk-core/src/welcomes.rs (6)

5-5: LGTM! Import correctly placed.

The Pagination import is properly placed with other mdk_storage_traits imports, following the coding guidelines for import ordering.

48-74: LGTM! Excellent documentation and API design.

The method signature and documentation are well-crafted:

Clear explanation of the pagination parameter

Multiple practical examples showing default and explicit pagination

Idiomatic use of Option<Pagination> for optional parameter

Examples demonstrate common usage patterns (first page, next page, etc.)

75-80: LGTM! Clean implementation.

The implementation correctly forwards the pagination parameter to the storage layer while maintaining proper error handling.

861-861: LGTM! Test correctly updated.

The test properly passes None for default pagination, maintaining focus on the error recovery scenario being tested.

1019-1019: LGTM! Test correctly updated.

The test appropriately uses default pagination (None) to verify empty state behavior.

1091-1153: LGTM! Comprehensive pagination test coverage.

Excellent test that thoroughly validates the pagination functionality:

Default pagination behavior (None)

Explicit limit and offset

Edge case: offset beyond available items

Edge case: limit of 1

The test setup properly creates a pending welcome and verifies pagination works correctly across different scenarios.

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

24-29: LGTM! Imports updated correctly.

The addition of Pagination to the imports is appropriate for the new paginated API, and the import order follows the coding guidelines.

271-278: LGTM! Backward compatibility maintained.

The existing get_pending_welcomes() method correctly delegates to the core with None, ensuring existing users are unaffected while leveraging the unified paginated implementation.

github-actions · 2026-01-03T23:06:36Z

✅ Coverage: 86.6% → 86.66% (+0.06%)

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/mdk-core/src/welcomes.rs (1)
3-9: Consider reordering imports per coding guidelines.

The import block places internal crate modules (mdk_storage_traits) before external crates (nostr, openmls, tls_codec). Per coding guidelines, the order should be: (1) core/alloc/std, (2) external crates, (3) sub-modules, (4) internal crate modules.
🔎 Suggested import order
-use mdk_storage_traits::MdkStorageProvider;
-use mdk_storage_traits::groups::types as group_types;
-use mdk_storage_traits::welcomes::Pagination;
-use mdk_storage_traits::welcomes::types as welcome_types;
 use nostr::{EventId, Timestamp, UnsignedEvent};
 use openmls::prelude::*;
 use tls_codec::Deserialize as TlsDeserialize;
+
+use mdk_storage_traits::MdkStorageProvider;
+use mdk_storage_traits::groups::types as group_types;
+use mdk_storage_traits::welcomes::Pagination;
+use mdk_storage_traits::welcomes::types as welcome_types;
Based on coding guidelines: "Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules"

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 41d71a6 and bf519b6.

📒 Files selected for processing (7)

crates/mdk-core/CHANGELOG.md
crates/mdk-core/src/welcomes.rs
crates/mdk-memory-storage/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-storage-traits/CHANGELOG.md
crates/mdk-storage-traits/tests/shared/welcome_tests.rs
crates/mdk-uniffi/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (5)

crates/mdk-storage-traits/tests/shared/welcome_tests.rs
crates/mdk-sqlite-storage/CHANGELOG.md
crates/mdk-uniffi/src/lib.rs
crates/mdk-core/CHANGELOG.md
crates/mdk-memory-storage/CHANGELOG.md

🧰 Additional context used

📓 Path-based instructions (3)

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Never use bare URLs in markdown files; all URLs must be properly formatted using [link text](url), <url>, or ([#123](url)) for PR/issue references

Files:

crates/mdk-storage-traits/CHANGELOG.md

crates/*/CHANGELOG.md

📄 CodeRabbit inference engine (AGENTS.md)

Every crate CHANGELOG entry must include a PR reference using the format ([#123](https://github.com/marmot-protocol/mdk/pull/123))

Files:

crates/mdk-storage-traits/CHANGELOG.md

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds in generics must be placed in where clauses, not inline
Use Self instead of the type name when possible
Derive order must be: Debug, Clone, Copy, PartialEq, Eq, Hash (in this order)
Always use tracing::warn!(...), never import and use warn!(...)
Use .to_string() or .to_owned() for string ...

Files:

crates/mdk-core/src/welcomes.rs

🧠 Learnings (3)

📚 Learning: 2026-01-03T08:39:39.799Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`

Applied to files:

crates/mdk-storage-traits/CHANGELOG.md

📚 Learning: 2026-01-03T08:39:39.799Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate

Applied to files:

crates/mdk-storage-traits/CHANGELOG.md

📚 Learning: 2026-01-01T15:51:58.505Z

Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.

Applied to files:

crates/mdk-storage-traits/CHANGELOG.md

🧬 Code graph analysis (1)

crates/mdk-core/src/welcomes.rs (1)

crates/mdk-storage-traits/src/welcomes/mod.rs (1)

new (35-37)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Test Coverage

🔇 Additional comments (3)

crates/mdk-storage-traits/CHANGELOG.md (1)

30-47: LGTM! PR references have been added.

All changelog entries now include the required PR references in the correct format ([#110](https://github.com/marmot-protocol/mdk/pull/110)). This addresses the previous review feedback and complies with the coding guidelines.

crates/mdk-core/src/welcomes.rs (2)

48-80: Excellent documentation and implementation!

The pagination support is well-implemented:

Clear documentation with practical examples showing both default (None) and explicit pagination usage

Proper forwarding of the pagination parameter to the storage layer

Consistent error handling pattern

The breaking change is appropriately documented in the CHANGELOG.

1134-1196: Comprehensive pagination test coverage!

The new test effectively validates the pagination functionality:

Default pagination behavior with None

Explicit limit and offset parameters

Edge cases: offset beyond available welcomes, empty pages

Boundary conditions with limit 1

Existing tests at lines 863 and 1062 have been correctly updated to use get_pending_welcomes(None).

github-actions · 2026-01-05T09:50:05Z

✅ Coverage: 86.6% → 86.66% (+0.06%)

erskingardner

other than the small changelog conflict, LGTM

…elcomes-enables-memory-exhaustion-denial-of-service

github-actions · 2026-01-05T19:34:39Z

✅ Coverage: 86.79% → 86.85% (+0.06%)

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/mdk-core/src/welcomes.rs (1)
1423-1485: Good pagination test coverage with room for enhancement.

The test effectively covers the basic pagination API including default behavior, explicit limits/offsets, and edge cases.

Consider enhancing this test by creating multiple pending welcomes (e.g., 5-10) to more thoroughly verify:

Ordering behavior (descending by ID) with multiple items

Multi-page pagination scenarios (e.g., limit=2, offset=0 then limit=2, offset=2)

Boundary conditions between pages

This would provide stronger confidence in the pagination behavior, though the storage layer tests likely cover detailed pagination logic.
💡 Example enhancement with multiple welcomes
// Create multiple welcomes to test pagination thoroughly
let mut welcome_ids = Vec::new();
for i in 0..5 {
    let member_keys = Keys::generate();
    let member_kp = create_key_package_event(&mdk, &member_keys);
    let group_config = create_nostr_group_config_data(vec![alice_keys.public_key()]);
    
    let result = mdk
        .create_group(&alice_keys.public_key(), vec![member_kp], group_config)
        .expect("Failed to create group");
    
    mdk.merge_pending_commit(&result.group.mls_group_id)
        .expect("Failed to merge pending commit");
    
    let welcome_rumor = &result.welcome_rumors[0];
    let welcome = mdk.process_welcome(&nostr::EventId::from([i; 32]), welcome_rumor)
        .expect("Failed to process welcome");
    welcome_ids.push(welcome.id);
}

// Test pagination with multiple pages
let page1 = mdk.get_pending_welcomes(Some(Pagination::new(Some(2), Some(0))))?;
let page2 = mdk.get_pending_welcomes(Some(Pagination::new(Some(2), Some(2))))?;
assert_eq!(page1.len(), 2);
assert_eq!(page2.len(), 2);

// Verify descending order (most recent first)
assert!(page1[0].id > page1[1].id);

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70e39f5 and a608bee.

📒 Files selected for processing (3)

crates/mdk-core/CHANGELOG.md
crates/mdk-core/src/welcomes.rs
crates/mdk-sqlite-storage/CHANGELOG.md

🧰 Additional context used

📓 Path-based instructions (3)

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

Never use bare URLs in markdown files; all URLs must be properly formatted using [link text](url), <url>, or ([#123](url)) for PR/issue references

Files:

crates/mdk-core/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md

crates/*/CHANGELOG.md

📄 CodeRabbit inference engine (AGENTS.md)

Every crate CHANGELOG entry must include a PR reference using the format ([#123](https://github.com/marmot-protocol/mdk/pull/123))

Files:

crates/mdk-core/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md

**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)

**/*.rs: All trait bounds should be written in where clauses rather than inline with type parameters in Rust
Use Self instead of the explicit type name when referring to the type in impl blocks
Derive Debug, Clone, Copy, PartialEq, Eq, and Hash for public types when possible, in that order
Derive Default for public types when there is a reasonable default value
Always use full paths for logging with tracing::<op>!(...) instead of importing and using <op>!(...)
Prefer .to_string() or .to_owned() for &str to String conversion instead of .into() or String::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import the fmt module rather than using full paths when implementing traits from core::fmt or std::fmt
Use relative self:: imports for sub-modules rather than bare module names
Place all use statements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoid if let ... { } else { } constructs; use match instead when both branches are non-empty
Use if let ... { } without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g., x.rs) with mod x; declaration rather than inline mod x { .. } blocks, except for tests and benches modules
Use inline mod tests { #[cfg(test)] ... } for test modules rather than creating separate tests.rs files
Use inline mod benches { #[cfg(bench)] ... } for benchmark modules rather than creating separate benches.rs files

**/*.rs: All trait bounds in generics must be placed in where clauses, not inline
Use Self instead of the type name when possible
Derive order must be: Debug, Clone, Copy, PartialEq, Eq, Hash (in this order)
Always use tracing::warn!(...), never import and use warn!(...)
Use .to_string() or .to_owned() for string ...

Files:

crates/mdk-core/src/welcomes.rs

🧠 Learnings (3)

📚 Learning: 2026-01-03T08:39:39.799Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`

Applied to files:

crates/mdk-core/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md

📚 Learning: 2026-01-03T08:39:39.799Z

Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate

Applied to files:

crates/mdk-core/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md

📚 Learning: 2026-01-01T15:51:58.505Z

Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.

Applied to files:

crates/mdk-core/CHANGELOG.md
crates/mdk-sqlite-storage/CHANGELOG.md

🧬 Code graph analysis (1)

crates/mdk-core/src/welcomes.rs (2)

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

get_pending_welcomes (271-278)

crates/mdk-storage-traits/src/welcomes/mod.rs (1)

new (35-37)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Test Coverage

🔇 Additional comments (5)

crates/mdk-sqlite-storage/CHANGELOG.md (1)

30-30: ✅ CHANGELOG entries properly formatted with PR references.

All modified entries correctly include PR reference links in the required format ([#NNN](url)). The previous flagging has been resolved, and all entries are compliant with the coding guidelines for CHANGELOG.md files.

Also applies to: 34-34, 44-44, 48-48, 50-50, 51-51

crates/mdk-core/CHANGELOG.md (1)

30-30: CHANGELOG entries are correctly formatted and complete.

All four entries follow the required PR reference format ([#110](https://github.com/marmot-protocol/mdk/pull/110)), are placed in appropriate sections (Breaking changes, Added, Fixed), and accurately document the pagination feature and memory exhaustion fix. No bare URLs or unformatted issue references present.

Also applies to: 44-45, 62-62

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

9-9: LGTM! Import correctly placed.

The Pagination import is necessary for the new pagination parameter and follows the coding guidelines for import ordering.

49-81: Excellent API design and documentation!

The optional pagination parameter provides a clean API that allows:

Default behavior with None (simple for common cases)

Explicit control with Some(Pagination::new(...)) when needed

The documentation is comprehensive with clear examples covering multiple use cases. The implementation correctly delegates to the storage layer.

1152-1152: LGTM! Tests correctly updated.

The existing tests properly use None to maintain default pagination behavior.

Also applies to: 1351-1351

add pagination to pending_welcomes to prevent memory exhaustion

7863f20

mubarakcoded linked an issue Jan 2, 2026 that may be closed by this pull request

Issue AA: Unbounded Loading of Pending Welcomes Enables Memory-Exhaustion Denial of Service #70

Closed

coderabbitai bot reviewed Jan 2, 2026

View reviewed changes

mubarakcoded changed the title ~~add pagination to pending_welcomes to prevent memory exhaustion~~ fix: add pagination to prevent memory exhaustion in pending_welcomes Jan 2, 2026

add pagination to pending_welcomes to prevent memory exhaustion

98b403a

coderabbitai bot reviewed Jan 2, 2026

View reviewed changes

add offset validation for pending welcomes pagination

e6a4bf6

mubarakcoded requested review from dannym-arx, erskingardner and jgmontoya January 2, 2026 21:48

jgmontoya reviewed Jan 2, 2026

View reviewed changes

jgmontoya mentioned this pull request Jan 2, 2026

fix: add pagination and validation to messages query to prevent memory exhaustion #111

Merged

dannym-arx requested changes Jan 3, 2026

View reviewed changes

refactor pagination to use Pagination struct and expose public API

c7a521f

coderabbitai bot reviewed Jan 3, 2026

View reviewed changes

unify get_pending_welcomes to single method with optional pagination

41d71a6

coderabbitai bot reviewed Jan 3, 2026

View reviewed changes

merge master and resolve CHANGELOG conflicts

bf519b6

coderabbitai bot reviewed Jan 3, 2026

View reviewed changes

remove MAX_OFFSET from Removed section - it was never in a release

70e39f5

dannym-arx approved these changes Jan 5, 2026

View reviewed changes

erskingardner approved these changes Jan 5, 2026

View reviewed changes

Merge branch 'master' into 70-issue-aa-unbounded-loading-of-pending-w…

a608bee

…elcomes-enables-memory-exhaustion-denial-of-service

coderabbitai bot reviewed Jan 5, 2026

View reviewed changes

mubarakcoded merged commit 32b8cce into master Jan 5, 2026
12 checks passed

mubarakcoded deleted the 70-issue-aa-unbounded-loading-of-pending-welcomes-enables-memory-exhaustion-denial-of-service branch January 5, 2026 19:49

This was referenced Jan 13, 2026

refactor: unified storage architecture #148

Merged

feat: Deterministic commit race resolution (MIP-03) #152

Merged

Conversation

mubarakcoded commented Jan 2, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Jan 2, 2026

✅ Coverage: 86.35% → 86.38% (+0.03%)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 2, 2026

✅ Coverage: 86.35% → 86.45% (+0.10%)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 2, 2026

✅ Coverage: 86.35% → 86.47% (+0.12%)

Uh oh!

jgmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dannym-arx commented Jan 3, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 3, 2026

✅ Coverage: 86.6% → 86.66% (+0.06%)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 5, 2026

✅ Coverage: 86.6% → 86.66% (+0.06%)

Uh oh!

erskingardner left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 5, 2026

✅ Coverage: 86.79% → 86.85% (+0.06%)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mubarakcoded commented Jan 2, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 2, 2026 •

edited

Loading