add leave-request-codec

[mchenani]

Add LeaveRequestCodec to encode/decode xmtp.org/leave_request v1.0 messages and update group tests to assert leave_request emission

Introduce ContentTypeLeaveRequest, a LeaveRequest marker, and LeaveRequestCodec that encodes an empty payload, decodes to LeaveRequest, returns no fallback, and never pushes. Update group tests to register the codec, send a leave request via SendOptions with ContentTypeLeaveRequest, and assert leave_request messages appear after Group.leaveGroup and in end-to-end sync. Add an ignored test for membership state transitions on re-add. See LeaveRequestCodec.kt and GroupTest.kt.

📍Where to Start

Start with the codec definition in LeaveRequestCodec.kt, then review its usage and assertions in GroupTest.kt.

---

Macroscope summarized 22c979c.

[claude]

Claude finished @mchenani's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed the LeaveRequestCodec implementation and associated tests. Overall, this is a well-crafted PR with clean code and comprehensive test coverage. Here's my detailed feedback:

---

🎯 Strengths

1. Clean Architecture: The codec follows established patterns in the codebase (similar to GroupUpdatedCodec)
2. Comprehensive Tests: Excellent test coverage with both unit tests for the codec and integration tests for group behavior
3. Documentation: Good KDoc comments explaining the purpose and behavior
4. Null Safety: Properly handles Kotlin null safety throughout

---

🔍 Code Quality & Best Practices

✅ LeaveRequestCodec.kt

The implementation is solid and follows Kotlin/Android best practices:

• Marker object pattern: Using object LeaveRequest is appropriate for a content-less message type
• Content type definition: Correctly structured with proper versioning
• Codec implementation: Clean and minimal, appropriate for a marker message

⚠️ Minor Suggestions

1. data class for codec (library/src/main/java/org/xmtp/android/library/codecs/LeaveRequestCodec.kt:40)

   data class LeaveRequestCodec(
       override var contentType: ContentTypeId = ContentTypeLeaveRequest,
   ) : ContentCodec<LeaveRequest>

   Using data class for a codec is unusual since:

   • It only has one field (contentType) which has a default value
   • Codecs are typically stateless
   • This creates unnecessary copy(), equals(), etc. methods

   Recommendation: Consider changing to a regular class or object (if no state is needed):

   // Option 1: Regular class
   class LeaveRequestCodec(
       override val contentType: ContentTypeId = ContentTypeLeaveRequest
   ) : ContentCodec<LeaveRequest>
   
   // Option 2: Object (if contentType never changes)
   object LeaveRequestCodec : ContentCodec<LeaveRequest> {
       override val contentType: ContentTypeId = ContentTypeLeaveRequest
       // ... rest of implementation
   }

   Note: GroupUpdatedCodec also uses data class, so this may be a codebase convention. If so, maintain consistency.

2. Mutable var in codec (library/src/main/java/org/xmtp/android/library/codecs/LeaveRequestCodec.kt:41)

   override var contentType: ContentTypeId = ContentTypeLeaveRequest,

   The contentType is declared as var (mutable), but should never change after initialization.

   Recommendation: Use val instead for immutability:

   override val contentType: ContentTypeId = ContentTypeLeaveRequest

   This prevents accidental modification and signals intent more clearly. (Same issue exists in GroupUpdatedCodec—may be interface requirement)

---

🔒 Security Considerations

✅ No security concerns identified:

• Empty payload prevents injection attacks
• No sensitive data handling
• Proper validation through content type checking
• shouldPush = false prevents notification spam

---

⚡ Performance Considerations

✅ Well-optimized for mobile:

• Minimal memory footprint (marker object, empty payload)
• No complex serialization/deserialization
• No heavy computations
• ByteString.EMPTY reuses singleton instance

---

🧪 Test Coverage

✅ Excellent Coverage

The tests cover all critical scenarios:

1. Unit tests for codec (library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt:1536-1593):

   • Content type validation ✅
   • Encode/decode roundtrip ✅
   • Fallback behavior ✅
   • Push notification behavior ✅

2. Integration tests (library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt:1322-1645):

   • testCanLeaveGroup: Full leave flow with message verification
   • testSelfRemovalWithMembershipState: Membership state transitions
   • testLeaveRequestInGroupMessage: End-to-end message sending/receiving

📝 Test Quality Notes

1. Good use of Thread.sleep (library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt:1373):

   Thread.sleep(3000) // 3 seconds

   While not ideal, this is acceptable for integration tests where you're waiting for worker processing. Consider adding a comment explaining what you're waiting for:

   // Wait for admin worker to process leave request and remove member
   Thread.sleep(3000)

2. Ignored test (library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt:1448):

   @Ignore("activate after latest libxmtp update")
   fun testMembershipStateAfterReAdd()

   Good practice to include the reason. Make sure to track this with a GitHub issue or TODO to re-enable it.

3. Assertion messages: Excellent! Tests include descriptive messages:

   assertEquals(
       "Should have one more message after leaving (the leave request)",
       messageCountBefore + 1,
       messagesAfterLeave.size,
   )

---

🐛 Potential Issues

⚠️ Minor: Codec registration not automatic

In the tests, the codec must be manually registered:

Client.register(codec = LeaveRequestCodec())

Question: Should LeaveRequestCodec be automatically registered like TextCodec, or is manual registration intentional?

If this is a core feature (which leaving groups seems to be), consider adding it to the default codec registry. If it's optional, document this requirement in the codec KDoc.

⚠️ Edge case: Multiple leave requests

The tests don't verify behavior if a user sends multiple LeaveRequest messages before the admin processes the first one.

Recommendation: Add a test case for this scenario:

@Test
fun testMultipleLeaveRequestsHandledGracefully() {
    // Send multiple leave requests rapidly
    // Verify only one removal occurs and no errors
}

---

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐½	Minor improvements possible (data class, mutability)
Security	⭐⭐⭐⭐⭐	No concerns
Performance	⭐⭐⭐⭐⭐	Optimal for mobile
Test Coverage	⭐⭐⭐⭐⭐	Comprehensive and well-structured
Documentation	⭐⭐⭐⭐	Good, could add codec registration note

---

✅ Recommendation

Approve with minor suggestions. This is a solid implementation that follows established patterns and includes thorough testing. The suggestions above are optional improvements—the code is production-ready as-is.

Great work! 🚀

---