xmtp-ios icon indicating copy to clipboard operation
xmtp-ios copied to clipboard

Update to libxmtp 4.6.0-dev.9df3055

Open xmtptools opened this issue 2 months ago β€’ 3 comments

This PR updates the iOS bindings to libxmtp version 4.6.0-dev.9df3055.

Changes:

  • Updated XMTP.podspec version to 4.6.0-dev.9df3055
  • Updated static binary (LibXMTPSwiftFFI) URL and checksum in Package.swift
  • Updated dynamic binary (LibXMTPSwiftFFIDynamic) URL and checksum in Package.swift
  • Updated Swift source files (xmtpv3.swift) for both static and dynamic bindings

Base branch: insipx/dynamic-swift

xmtptools avatar Oct 24 '25 23:10 xmtptools

Claude finished @xmtptools's task β€”β€” View job


Code Review Complete βœ…

I've reviewed this PR which updates iOS bindings to libxmtp 4.6.0-dev.9df3055. Here's my detailed feedback:


πŸ”΄ Critical Issues

1. Thread-Safety Race Condition in Dynamic Bindings (Sources/XMTPiOSDynamic/Libxmtp/xmtpv3.swift:391)

Severity: HIGH - Already flagged by Macroscope

The ConcurrentHandleMap.count property reads map.count without acquiring the lock, while all mutations are protected by locks. This creates a data race.

Location: Sources/XMTPiOSDynamic/Libxmtp/xmtpv3.swift:391

var count: Int {
    get {
        map.count  // ❌ Unprotected read while mutations use lock
    }
}

Fix needed:

var count: Int {
    get {
        lock.withLock { map.count }  // βœ… Protected read
    }
}

Impact: This can cause crashes or undefined behavior when the map is read while being modified concurrently. Since this is generated code, the fix may need to be applied upstream in the UniFFI generator.


⚠️ Medium Priority Issues

2. Missing Test Coverage for New FFI Methods

Severity: MEDIUM

The following new FFI methods have no test coverage:

  • FfiConversation.leaveGroup() - No tests found in the test suite
  • FfiGroupSyncSummary return type - Not validated in tests
  • FfiForkRecoveryOpts parameter - No tests for fork recovery functionality
  • FfiCursor type - New type with no usage tests

Recommendation: Add tests for:

// Test leaveGroup functionality
func testLeaveGroup() async throws {
    // Create group, member leaves, verify state
}

// Test sync summary return values
func testSyncAllConversationsReturnsCorrectSummary() async throws {
    // Verify numEligible and numSynced are populated correctly
}

// Test fork recovery options
func testClientCreationWithForkRecoveryOpts() async throws {
    // Test different FfiForkRecoveryPolicy values
}

3. Unsafe Sendable Conformance - @unchecked Sendable

Severity: MEDIUM

Found 18 instances of @unchecked Sendable in the FFI bindings. While this is standard for UniFFI-generated code, it bypasses Swift 6's concurrency safety checks.

Concern: The classes marked with @unchecked Sendable (like FfiConversation, FfiConversations, etc.) need careful manual verification that they're actually thread-safe, especially given the race condition found in ConcurrentHandleMap.

Recommendation: Document why each FFI object is safe to send across actor boundaries, or consider using proper actor isolation in the high-level Swift wrapper code.

4. Breaking API Change - Sync Methods Now Return Data

Severity: MEDIUM

The signature of sync methods has changed:

  • syncAllConversations(consentStates:) now returns FfiGroupSyncSummary (was void)
  • syncPreferences() now returns FfiGroupSyncSummary (was void)

Impact: Any existing code that calls these methods needs to handle the return value. The high-level Swift API wrappers may need updates.

Recommendation: Check if Sources/XMTPiOS/Conversations.swift or similar files use these methods and update them to utilize the sync summary data.


ℹ️ Low Priority / Informational

5. Large Generated File Added (14,666 lines)

The new dynamic bindings file (Sources/XMTPiOSDynamic/Libxmtp/xmtpv3.swift) is massive. This is expected for generated code, but consider:

  • Ensure it's properly marked as generated in .gitattributes to exclude from diffs
  • Consider splitting the module if build times become an issue
  • Verify the dynamic vs static bindings have identical APIs (beyond the thread-safety bug)

6. Package.swift and Podspec Updates Look Correct

βœ… Binary checksums updated properly βœ… URLs point to correct release artifacts
βœ… Version strings are consistent across files (4.6.0-dev.9df3055)

7. New Types Are Well-Structured

The new FFI types follow good Swift conventions:

  • FfiCursor: Simple value type with proper Equatable and Hashable conformance
  • FfiGroupSyncSummary: Clear naming, appropriate types (UInt64)
  • FfiForkRecoveryOpts: Good use of optionals for optional parameters
  • FfiForkRecoveryPolicy: Clean enum with clear cases

πŸ”’ Security Considerations

8. Fork Recovery Feature Needs Documentation

The new FfiForkRecoveryOpts parameter in createClient() introduces fork recovery functionality. This is a security-sensitive feature that could affect group state consistency.

Questions to address:

  • What are the security implications of different FfiForkRecoveryPolicy values?
  • Should disableRecoveryResponses default to true in production?
  • What happens if workerIntervalNs is set too low?

Recommendation: Add documentation or assertions to guide developers on safe usage.

9. Data Exposure in Error Handling

The generic error handling in this FFI layer is appropriate, but verify that error messages don't leak sensitive data (encryption keys, private keys, etc.) through the FFI boundary.


πŸ“Š Performance Considerations

10. Sync Summary Could Reduce Redundant Calls

Positive: The new FfiGroupSyncSummary return type is a good improvement. It allows callers to see numEligible vs numSynced without making additional queries.

11. Async/Await Properly Used

βœ… All new FFI methods that should be async are correctly marked and use uniffiRustCallAsync


βœ… Summary

Merge Recommendation: ⚠️ MERGE WITH CAUTION

This PR should be mergeable after addressing:

  1. MUST FIX: Thread-safety issue in ConcurrentHandleMap.count (Sources/XMTPiOSDynamic/Libxmtp/xmtpv3.swift:391)
  2. SHOULD ADD: Test coverage for leaveGroup(), FfiGroupSyncSummary, and fork recovery
  3. SHOULD VERIFY: High-level Swift code handles new return values from sync methods

Strengths:

  • Clean generated code structure
  • Good type safety with new structs and enums
  • Proper async/await usage
  • Version updates are consistent

Generated Code Note: Since this is largely generated from UniFFI, some issues (like the thread-safety bug) may need to be fixed in the upstream generator rather than manually in this PR.


claude[bot] avatar Oct 24 '25 23:10 claude[bot]

Update XMTP iOS bindings to libxmtp 4.6.0-dev.9df3055 and add FfiConversation.leaveGroup() plus structured sync results in Sources/XMTPiOS/Libxmtp/xmtpv3.swift

Update the Swift bindings to the specified libxmtp build and adjust FFI APIs and models to current Rust interfaces.

  • Add FfiConversation.leaveGroup() implemented via uniffi_xmtpv3_fn_method_fficonversation_leave_group in xmtpv3.swift

  • Change FfiConversationsProtocol.syncAllConversations(consentStates:) and FfiXmtpClientProtocol.syncPreferences() to return FfiGroupSyncSummary with updated rust buffer converters in xmtpv3.swift

  • Modify connectToBackend(v3Host:gatewayHost:isSecure:appVersion:) and createClient(..., forkRecoveryOpts:) signatures and FFI calls in xmtpv3.swift

  • Introduce new types and converters: FfiCursor, FfiForkRecoveryOpts, FfiGroupSyncSummary, FfiForkRecoveryPolicy, and extend FfiMessage, FfiGroupUpdated, and GenericError in xmtpv3.swift

  • Add generated dynamic bindings file xmtpv3.swift

  • Update binary target URLs/checksums in Package.swift and bump podspec version in XMTP.podspec

πŸ“Where to Start

Start with the updated FFI API surface and method implementations in Sources/XMTPiOS/Libxmtp/xmtpv3.swift, focusing on FfiConversation.leaveGroup(), FfiConversations.syncAllConversations(...), and FfiXmtpClient.syncPreferences() in xmtpv3.swift.


πŸ“Š Macroscope summarized bc1d6fd. 3 files reviewed, 12 issues evaluated, 10 issues filtered, 0 comments posted. View details

macroscopeapp[bot] avatar Oct 24 '25 23:10 macroscopeapp[bot]

[!WARNING] This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

insipx avatar Oct 24 '25 23:10 insipx