D14z client prerelease updates

[cameronvoell]

Add gatewayUrl-based backend client selection and caching and propagate MessageVisibilityOptions through Conversation, Dm, and Group send/prepare flows in Client.kt and messaging classes to support D14z client prerelease updates

This pull request introduces gateway-aware backend client caching and extends messaging methods to carry message visibility options through to the FFI layer. It adds a gatewayUrl field to ClientOptions, replaces cache keys with a configuration-derived key including environment URL, gateway URL, isSecure, and appVersion, and updates connection logic accordingly. It also modifies Conversation, Dm, and Group APIs to compute and pass MessageVisibilityOptions (including shouldPush) into send and optimistic send paths, and regenerates UniFFI bindings to accept the new FFI options. Test coverage is expanded to validate client creation and gateway usage.

• Update backend client caching and connection creation to key by env.getUrl(), gatewayUrl, isSecure, and appVersion, and propagate gatewayUrl into backend connections in Client.kt
• Add MessageVisibilityOptions with shouldPush and FFI conversion in SendOptions.kt; modify Conversation, Dm, and Group send/prepare signatures to accept and forward visibility options in Conversation.kt, Dm.kt, and Group.kt
• Regenerate UniFFI Kotlin shims and JNA bindings to pass FfiSendMessageOpts for send and sendOptimistic in xmtpv3.kt and update bundled native libraries
• Update tests for client creation with gatewayUrl and configuration-driven cache keys in ClientTest.kt
• Bump libxmtp version metadata in libxmtp-version.txt

📍Where to Start

Start with the backend client cache key generation in Client.createCacheKey and its use in connection setup in Client.kt, then review how MessageVisibilityOptions is computed and propagated through Dm.encodeContent, Group.encodeContent, and Conversation.send/prepare in Dm.kt, Group.kt, and Conversation.kt.

---

📊 Macroscope summarized c78f91f. 6 files reviewed, 12 issues evaluated, 12 issues filtered, 0 comments posted

🗂️ Filtered Issues

library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt — 0 comments posted, 2 evaluated, 2 filtered

• line 157: The assertion on canMessage can be silently skipped due to the use of a safe-call on a potentially missing map entry. In client.canMessage(listOf(clientIdentity))[clientIdentity.identifier]?.let { assert(it) }, if the key clientIdentity.identifier is not present in the returned map, the safe-call (?.) prevents let from running, and no assertion is performed. This yields a false positive test pass while the core condition (that the identity is present and can be messaged) remains unverified. Replace with an explicit presence check and value assertion (e.g., assertTrue(client.canMessage(listOf(clientIdentity))[clientIdentity.identifier] == true) or assertTrue(requireNotNull(client.canMessage(...)[clientIdentity.identifier]))). [ Test / Mock code ]
• line 184: The assertion on canMessage is vulnerable to being skipped for missing keys due to ?.let. In client.canMessage(listOf(clientIdentity))[clientIdentity.identifier]?.let { assert(it) }, if the identity is absent from the result map, the let block will not run and no assertion is made, allowing the test to pass without proving the expected behavior (that the client can message the identity). Use an explicit assertion that both the key exists and the value is true (e.g., assertTrue(client.canMessage(listOf(clientIdentity))[clientIdentity.identifier] == true) or assertTrue(requireNotNull(client.canMessage(...)[clientIdentity.identifier]))). [ Test / Mock code ]

library/src/main/java/org/xmtp/android/library/Client.kt — 0 comments posted, 2 evaluated, 2 filtered

• line 161: createCacheKey() uses a simple pipe | delimiter between fields without escaping values. If any field value itself contains | (e.g., a malformed or custom gatewayUrl or an appVersion string), different tuples of values can produce identical concatenated strings, creating cache key collisions and leading to incorrect reuse of XmtpApiClient instances. Use an encoding that escapes delimiters, or a structured key (e.g., JSON with escaping, map-based key, or length-prefixed fields) to ensure uniqueness. [ Low confidence ]
• line 161: createCacheKey() encodes nullable values by substituting the literal string "null" for gatewayUrl and appVersion. This can collide with legitimate inputs where a caller passes the literal value "null" for either field, causing different API configurations to map to the same cache key and potentially reuse a wrong XmtpApiClient instance. To avoid collisions, encode optionals in a way that cannot be produced by a legitimate value (e.g., prefixing with a marker or using structured encoding/escaping). [ Low confidence ]

library/src/main/java/org/xmtp/android/library/Conversation.kt — 0 comments posted, 2 evaluated, 2 filtered

• line 165: Defaulting opts to MessageVisibilityOptions(shouldPush = true) in Conversation.prepareMessage(encodedContent, opts) can cause messages to be pushed even when the associated codec's policy would prefer not to. Callers using the encoded-content overload have no way to inherit codec-derived visibility and may unintentionally publish messages. This is a behavior regression introduced by adding opts with a default true value. [ Low confidence ]
• line 182: Defaulting opts to MessageVisibilityOptions(shouldPush = true) in Conversation.send(encodedContent, opts) can result in always pushing messages for callers who supply pre-encoded content, regardless of codec policy. Other overloads derive shouldPush from the codec via encodeContent, causing inconsistency and potential unintended publication. [ Low confidence ]

library/src/main/java/org/xmtp/android/library/Dm.kt — 0 comments posted, 3 evaluated, 3 filtered

• line 107: Dm.send(encodedContent, opts) defaults opts to MessageVisibilityOptions(shouldPush = true), which may publish messages even when a codec would have suggested shouldPush = false. This makes the encoded-content path behavior diverge from send<T> and send(text), which derive shouldPush from the codec. [ Low confidence ]
• line 137: Dm.encodeContent catches all Exception and rethrows XMTPException("Codec type is not registered"), losing the original cause and stack. This misclassifies unrelated failures (e.g., compression errors, codec runtime errors) as registration problems, hindering debugging and accurate error handling. [ Low confidence ]
• line 144: Dm.prepareMessage(encodedContent, opts) defaults opts to MessageVisibilityOptions(shouldPush = true). Even though this path is "optimistic" (not immediately publishing), the visibility option likely influences later publish behavior. Defaulting to true may be inconsistent with codec policy and other overloads that derive shouldPush from codec. [ Low confidence ]

library/src/main/java/org/xmtp/android/library/Group.kt — 0 comments posted, 3 evaluated, 3 filtered

• line 148: Group.send(encodedContent, opts) defaults opts to MessageVisibilityOptions(shouldPush = true), which may publish messages for callers supplying pre-encoded content even when a codec would prefer shouldPush = false. This diverges from other overloads that derive shouldPush via encodeContent. [ Low confidence ]
• line 179: Group.encodeContent catches all Exception and rethrows XMTPException("Codec type is not registered"), discarding the original cause and stack trace. Non-registration errors (e.g., compression failures, codec runtime errors) are misreported and hard to diagnose. [ Low confidence ]
• line 186: Group.prepareMessage(encodedContent, opts) defaults opts to MessageVisibilityOptions(shouldPush = true), potentially marking messages for push later even when a codec would suggest shouldPush = false. This is inconsistent with prepareMessage<T> that derives shouldPush via encodeContent. [ Low confidence ]

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @cameronvoell's task —— View job

---

Code Review Complete ✅

Reviewed PR #493: D14z client prerelease updates

Summary

This PR updates the SDK to use the latest libxmtp version (c0462383b on d14z-client-prerelease branch) and adds test coverage for the d14n staging environment. The changes include API client caching improvements, the addition of getPushTopics() method to Dm, and new test cases for d14n staging connectivity.

Detailed Findings

✅ Positive Aspects

1. Improved API Client Caching - The refactored cache key generation in Client.kt:160-162 now properly incorporates all API configuration parameters (URL, gateway URL, security flag, app version), preventing configuration conflicts
2. Comprehensive Test Coverage - Two new test methods validate d14n staging connectivity for both client creation and bundle building scenarios
3. Proper Async/Coroutine Usage - All suspend functions correctly use withContext(Dispatchers.IO) for I/O operations
4. Null Safety - Extensive use of nullable types and safe calls throughout the codebase

🔍 Code Quality Issues

Client.kt (library/src/main/java/org/xmtp/android/library/Client.kt)

1. Inconsistent API Client Caching - Lines 164-177 and 180-193

   • Both connectToApiBackend() and connectToSyncApiBackend() have identical implementations with separate caches
   • Concern: This duplication could lead to maintenance issues. Consider extracting common logic or documenting why separate caches are necessary
   • Performance: Double the cache memory usage without clear justification

2. Mutex Lock Efficiency - Lines 166 and 182

   • The withLock blocks return the client directly, but the function signature already declares a return type
   • Minor Issue: The return@withLock on line 176 is unnecessary - the last expression in the lambda is automatically returned

3. Potential Connection Leak - Lines 169, 185

   • If isConnected(cached) returns false, the old client is replaced without being explicitly closed
   • Security/Resource Concern: This could lead to connection leaks if the FFI layer doesn't handle cleanup automatically
   • Recommendation: Add explicit cleanup: if (cached != null && !isConnected(cached)) { /* cleanup code */ }

Dm.kt (library/src/main/java/org/xmtp/android/library/Dm.kt)

1. Breaking Change Warning - Line 605

   • getPushTopics() changed from suspend to synchronous
   • Impact: This is inconsistent with Group.getPushTopics() which remains synchronous but could cause issues if callers expected async behavior
   • Note: The implementation at lines 385-390 does not actually suspend, so this appears correct, but verify no callers were expecting suspend behavior

2. Potential Performance Issue - Lines 386-389

   val duplicates = libXMTPGroup.findDuplicateDms()
   val topicIds = duplicates.map { it.id().toHex() }.toMutableList()
   topicIds.add(id)

   • Concern: findDuplicateDms() is called every time getPushTopics() is invoked
   • Mobile Impact: This could be expensive on mobile devices if called frequently (e.g., in a list view)
   • Recommendation: Consider caching the result or making this suspendable to run on IO dispatcher

Group.kt (library/src/main/java/org/xmtp/android/library/Group.kt)

1. Inconsistency with Dm.kt - Line 605
   • getPushTopics() is not marked as suspend, unlike many other similar methods
   • This is actually correct (returns simple list), but creates an inconsistency with the Dm version which IS suspend

SendOptions.kt (library/src/main/java/org/xmtp/android/library/SendOptions.kt)

1. Deprecated Property Without Proper Handling - Line 10

   @Deprecated("This option is no longer supported and does nothing")
   var ephemeral: Boolean = false

   • Issue: The property is still mutable and part of the data class
   • Better Practice: Add @Suppress("DEPRECATION") to the constructor if needed, or consider removal in next major version
   • Security Note: Dead code in security-sensitive messaging SDK should be removed to reduce attack surface

ClientTest.kt (library/src/androidTest/java/org/xmtp/android/library/ClientTest.kt)

1. Test Duplication - Lines 67-97 and 165-189

   • testClientD14NStagingCanBeCreatedWithBundle() and testCreatesAClientD14NStaging() are nearly identical to existing tests with only the gateway URL changed
   • Concern: This increases maintenance burden
   • Recommendation: Consider parameterized tests or a helper method to test different environments

2. Hardcoded Test URL - Lines 72, 170

   gatewayUrl = "https://payer.testnet-staging.xmtp.network:443"

   • Concern: Hardcoded URLs make tests brittle
   • Recommendation: Extract to a constant or configuration file for easier updates

3. Missing Test Validation - Lines 82, 184

   • Tests only verify canMessage() returns true but don't validate actual message sending/receiving on d14n staging
   • Recommendation: Add integration tests that verify end-to-end messaging on the staging environment

⚠️ Security Considerations

1. API Client Connection Validation - Client.kt:169, 185

   • The isConnected() check is good, but there's no validation of the connection's authenticity
   • Recommendation: Ensure the underlying FFI layer validates TLS certificates and connection security

2. Gateway URL Injection - Client.kt:174, 190

   • Gateway URLs are passed directly from ClientOptions.Api
   • Concern: Ensure the app validates these URLs to prevent MITM attacks
   • Note: The test uses HTTPS which is good, but ensure this is enforced in production

📱 Mobile-Specific Concerns

1. Memory Management - Client.kt:89-92

   • Two separate cache maps for API clients
   • Concern: On Android with limited memory, these caches could grow unbounded
   • Recommendation: Consider LRU cache or periodic cleanup

2. Main Thread Safety - Dm.kt:385-390

   • getPushTopics() is synchronous but calls potentially expensive operations
   • Impact: Could cause ANR if called on main thread
   • Recommendation: Document that this should not be called on main thread or make it suspend

🧪 Test Coverage Assessment

Strengths:

• New tests cover d14n staging environment connectivity
• Tests verify both client creation and bundle building flows
• Good use of runBlocking for async test operations

Gaps:

• No tests for the new Dm.getPushTopics() behavior with duplicate DMs
• No tests validating the new API client caching logic
• No negative tests for d14n staging (e.g., invalid gateway URLs)

📊 Overall Assessment

Code Quality: 7.5/10 - Generally solid Kotlin code with good practices, but some duplication and consistency issues

Security: 8/10 - No major security issues found, but connection cleanup and URL validation should be verified

Performance: 7/10 - Some potential mobile performance concerns with caching and duplicate DM lookups

Test Coverage: 8/10 - Good basic coverage, but missing tests for new behaviors

Recommendations

High Priority

1. Clarify API Client Cache Strategy - Document why separate caches are needed for regular and sync API clients, or consolidate if unnecessary
2. Add Connection Cleanup - Ensure disconnected API clients are properly disposed of
3. Test Dm.getPushTopics() - Add unit tests for the new duplicate DM handling

Medium Priority

4. Extract Test Configuration - Move hardcoded d14n staging URL to a constant
5. Consider Parameterized Tests - Reduce test duplication for different environments
6. Document Threading Requirements - Add kdoc to getPushTopics() about thread safety

Low Priority

7. Remove Deprecated Code - Plan removal of ephemeral property in next major version
8. Add LRU Cache - Consider bounded cache for API clients on memory-constrained devices

---

Changed Files Analysis

• Client.kt (+22/-5): API client caching improvements, minor refactoring
• Conversation.kt (+12/-6): Wrapper changes for suspend functions (delegation updates)
• Dm.kt (+27/-12): Added getPushTopics() with duplicate DM support
• Group.kt (+30/-14): Consistency improvements with Dm (similar pattern)
• SendOptions.kt (+8/-0): No functional changes (formatting/deprecation)
• ClientTest.kt (+61/-1): New d14n staging tests
• libxmtp-version.txt (+3/-3): Version bump to c0462383b
• xmtpv3.kt (+7039/-7274): FFI bindings regenerated (auto-generated, not reviewed in detail)
• Native libraries (4 files): Updated binaries (not reviewable)

---