Expose clear_pending_commit in UniFFI bindings#196
Conversation
📝 WalkthroughWalkthroughThe PR adds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
❌ Coverage: 91.04% → 91.02% (-0.02%) |
❌ Coverage: 91.04% → 91.02% (-0.02%) |
erskingardner
left a comment
There was a problem hiding this comment.
All good, unless you want to add a test, but given it's the thinnest wrapper possible, I'm fine with this.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mdk-uniffi/src/lib.rs (1)
810-814: Consider adding a unit test forclear_pending_commit.While the implementation is straightforward and follows the same pattern as
merge_pending_commit, adding a test would verify the FFI binding works correctly end-to-end. A test could create a group, verifyclear_pending_commitsucceeds on a group with a pending commit, and returns an error for a non-existent group.🤖 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 810 - 814, Add a unit test for clear_pending_commit similar to the existing merge_pending_commit test: instantiate the MdkUniffi object, create a test group (so there is a pending commit), call clear_pending_commit(&self, mls_group_id: String) and assert it returns Ok and that the group's pending commit is cleared; then call clear_pending_commit with a non-existent group id and assert it returns an Err (or the appropriate MdkUniffiError). Use parse_group_id/lock behavior indirectly via the public API and mirror the structure/assertions from the merge_pending_commit test to verify the FFI binding end-to-end.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-uniffi/CHANGELOG.mdcrates/mdk-uniffi/src/lib.rs
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/mdk-uniffi/src/lib.rs`:
- Around line 810-814: Add a unit test for clear_pending_commit similar to the
existing merge_pending_commit test: instantiate the MdkUniffi object, create a
test group (so there is a pending commit), call clear_pending_commit(&self,
mls_group_id: String) and assert it returns Ok and that the group's pending
commit is cleared; then call clear_pending_commit with a non-existent group id
and assert it returns an Err (or the appropriate MdkUniffiError). Use
parse_group_id/lock behavior indirectly via the public API and mirror the
structure/assertions from the merge_pending_commit test to verify the FFI
binding end-to-end.
|
I don't think we need a test personally, the useful tests are in the PR introducing clear_pending_commit. Any test added here would just be silly imo |
This PR exposes the
clear_pending_commit()method through the UniFFI bindings, allowing foreign language clients to roll back uncommitted pending MLS commits. This is a recovery mechanism for failed relay publishes that would otherwise permanently block group operations.What changed:
clear_pending_commit(mls_group_id: String)public method to theMdkstruct in the mdk-uniffi crate, which accepts a hex-encoded MLS group ID and rolls back to the pre-commit stateAPI surface:
Mdktype that mirrors the existingmerge_pending_commit()flow but performs a rollback instead of merge