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

Test dynamic library build

Open xmtptools opened this issue 1 month ago • 2 comments

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

Changes:

  • Updated XMTP.podspec version to 4.6.0-dev.c4b44a9
  • 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: main

xmtptools avatar Nov 06 '25 18:11 xmtptools

Claude finished @xmtptools's task —— View job


Code Review: libxmtp 4.6.0-dev.c4b44a9 Update

I've reviewed the PR and here's my feedback:


✅ Code Quality & Best Practices

Auto-generated Code: The vast majority of changes (14k+ lines) are auto-generated Swift bindings from the Rust uniffi system. This is standard practice and the code follows the expected patterns for uniffi-generated bindings.

Package Updates: The Package.swift and XMTP.podspec updates look correct:

  • Binary URL references updated to swift-bindings-1.6.1-rc3.c4b44a9
  • Checksums updated for the new binaries
  • Version bumped to 4.6.1-dev in podspec

New Feature: The streamMessageDeletions implementation follows the same pattern as other streaming callbacks (streamMessages, streamConsent, etc.), which is good for consistency.


⚠️ Potential Bugs & Issues

1. Integer Overflow Risk (CONFIRMED)

Macroscope correctly identified a potential issue in Sources/XMTPiOSDynamic/Libxmtp/xmtpv3.swift:367:

currentHandle += 1  // Line 367

This can overflow at UInt64.max (18,446,744,073,709,551,615) and cause a runtime crash. While reaching this limit is extremely unlikely in practice (would require billions of billions of callback registrations), it's still a potential crash vector.

Impact: Low probability but HIGH severity if it occurs (app crash).

Recommended fix:

currentHandle &+= 1  // Use wrapping increment
if currentHandle == 0 { currentHandle = 1 }  // Skip 0 to avoid null pointer confusion

Note: The same issue exists in the static version at Sources/XMTPiOS/Libxmtp/xmtpv3.swift:364, but since that's an older file not modified in this PR, it's a pre-existing issue.


🔒 Security Concerns

No major security issues identified. The changes are primarily:

  1. Binary checksum updates (good security practice)
  2. Auto-generated FFI bindings (inherits security from Rust libxmtp)
  3. Callback handling that follows existing patterns

⚡ Performance Considerations

Auto-generated Code: The uniffi-generated code uses standard patterns for FFI (Foreign Function Interface) with Rust. No performance red flags.

Handle Map: The UniffiHandleMap<T> uses NSLock for thread safety, which is appropriate for this use case. The lock scope is minimal (just the critical section), which is good.

Callback Pattern: The new FfiMessageDeletionCallback uses the same vtable pattern as other callbacks, so performance should be consistent with existing streaming functionality.


🧪 Test Coverage

Missing Tests: There are no tests for the new streamMessageDeletions functionality. While this is an auto-generated binding to libxmtp (which presumably has its own tests), it would be beneficial to have integration tests in the iOS SDK.

Suggested test coverage:

  1. Test that streamMessageDeletions callback receives message IDs when messages are deleted
  2. Test that the stream closer properly terminates the stream
  3. Test error handling if the callback throws

Files to check for test patterns: Tests/XMTPTests/GroupTests.swift, Tests/XMTPTests/DmTests.swift (which already test disappearing messages)


📋 Summary

Approve with recommendations:

  1. HIGH PRIORITY: Fix the integer overflow issue in UniffiHandleMap.insert() in the dynamic module (line 367)
  2. MEDIUM PRIORITY: Consider fixing the same issue in the static module (line 364) as a separate cleanup
  3. LOW PRIORITY: Add integration tests for streamMessageDeletions to verify the callback works as expected

The binary updates and version bumps look correct. The new feature implementation follows existing patterns. The main concern is the overflow risk which, while unlikely, should be addressed.


claude[bot] avatar Nov 06 '25 18:11 claude[bot]

Upgrade XMTP iOS bindings to libxmtp 4.6.0-dev.c4b44a9 and add xmtpv3.FfiConversationsProtocol.streamMessageDeletions with dynamic LibXMTPSwiftFFIDynamic packaging

Switch the Swift package to a dynamic binary target and add a new xmtpv3.FfiConversationsProtocol.streamMessageDeletions(callback: FfiMessageDeletionCallback) -> FfiStreamCloser, including callback bridging, vtable registration, and checksum validation. Update podspec version to 4.6.1-dev.

📍Where to Start

Start with the new streaming API in xmtpv3.streamMessageDeletions, then review the callback implementation and vtable registration in the same file.


Macroscope summarized 5a157f5.

macroscopeapp[bot] avatar Nov 06 '25 18:11 macroscopeapp[bot]