Skip to content

enable migration client#3354

Open
insipx wants to merge 4 commits intomainfrom
d14n-migration
Open

enable migration client#3354
insipx wants to merge 4 commits intomainfrom
d14n-migration

Conversation

@insipx
Copy link
Copy Markdown
Contributor

@insipx insipx commented Mar 24, 2026

Enable migration client path in mobile and WASM bindings

  • Adds MigrationLocal, MigrationStaging, and MigrationProduction environment variants to the WASM and Node bindings; when selected, the backend builder calls build() (migration-capable) instead of build_optional_d14n().
  • Splits connect_to_backend in the mobile bindings into a non-exclusive (build()) path and connect_to_backend_exclusive (build_optional_d14n()), reversing the previous default behavior.
  • Adds connectToApiBackendExclusive and connectToSyncApiBackendExclusive to the Android and iOS clients, using a separate cache namespace keyed by an exclusive flag.
  • Updates all Android and iOS instrumented tests to pass an explicit gatewayHost (local, dev, or production) when constructing ClientOptions.Api, and switches relevant tests to connectToApiBackendExclusive.
  • Enables migration in the local dev Docker node via --api.enable-migration and a far-future cutover timestamp.
  • Behavioral Change: connect_to_backend in mobile bindings now uses build() (auto-migration) by default; callers that relied on the old non-migrating behavior must switch to connect_to_backend_exclusive.

Macroscope summarized fda9664.

@insipx insipx requested a review from a team as a code owner March 24, 2026 14:34
@claude
Copy link
Copy Markdown

claude bot commented Mar 24, 2026

Claude finished @insipx's task —— View job


PR Review - April 15, 2026 20:16 UTC

I've reviewed this PR focusing on areas not already covered by the automated review. Here are my findings:

✅ Strengths

  1. Consistent migration pattern: The is_migration() helper method is used consistently across Node, WASM, and mobile bindings to determine whether to call build() vs build_optional_d14n() (bindings/node/src/client/backend.rs:83-86, bindings/wasm/src/client/backend.rs:58-64)

  2. Good test infrastructure updates: Test helpers now properly include gatewayHost for local testing (sdks/android/library/src/androidTest/java/org/xmtp/android/library/TestHelpers.kt:24-32, sdks/ios/Sources/XMTPTestHelpers/TestHelpers.swift:102)

  3. Docker configuration supports migration: The local dev node is correctly configured with --api.enable-migration and a far-future cutover timestamp (dev/docker/docker-compose.yml:18-19)

⚠️ Issues Requiring Attention

1. Performance test cache key mismatch (already flagged by macroscopeapp)
The issue at sdks/android/library/src/androidTest/java/org/xmtp/android/library/PerformanceTest.kt:154 is valid - the test pre-warms with connectToApiBackendExclusive() but Client.create() uses the non-exclusive variant, resulting in different cache keys and invalidating the performance measurement.

2. Missing test coverage for migration environments
While the new MigrationLocal, MigrationStaging, and MigrationProduction environment variants are added to Node and WASM (bindings/node/src/client/options.rs:65-67, bindings/wasm/src/client.rs:105-107), I don't see any tests that specifically exercise these migration variants. Consider adding tests that:

  • Verify is_migration() returns true for migration variants
  • Confirm the correct backend construction method is called (build() vs build_optional_d14n())
  • Test that migration clients can communicate with the migration-enabled local node

3. Deprecation strategy
The connectToApiBackendExclusive and connectToSyncApiBackendExclusive methods are marked deprecated but lack deprecation timelines or migration guides. Consider adding doc comments explaining:

  • When these will be removed
  • What developers should use instead
  • Any behavioral differences they should be aware of

💡 Recommendations

  1. Fix the performance test as noted by macroscopeapp - use connectToApiBackend() instead of connectToApiBackendExclusive() for cache pre-warming

  2. Add migration environment tests to verify the new environment variants work correctly

  3. Document the migration behavior - add comments explaining why migration environments use build() instead of build_optional_d14n() for future maintainers

📊 Overall Assessment

The implementation is solid and follows good patterns. The main concerns are the performance test issue (already identified) and lack of explicit test coverage for the new migration environment variants. The code quality is good, and the architectural approach of using environment-based switching is clean.


Comment thread bindings/mobile/src/mls.rs
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Mar 24, 2026

Approvability

Verdict: Needs human review

2 blocking correctness issues found. This PR enables a new migration client feature that changes default backend connection behavior across mobile, Node, and WASM bindings. The changes affect runtime behavior by switching from optional D14n to migration-enabled builds, introducing new environment types, and enabling migration in docker config. An unresolved review comment raises concerns about potential breaking changes when gateway_host is None.

You can customize Macroscope's approvability policy. Learn more.

Comment thread bindings/mobile/src/mls.rs Outdated
Comment thread bindings/mobile/src/mls.rs Outdated
Comment on lines +166 to +168
// switch v3/d14n based on presence of gateway host to preserve
// previous behavior and avoid breaking changes
let client_bundle = client_bundle.build_optional_d14n()?;
let client_bundle = client_bundle.build()?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/mls.rs:166

build() returns an error when gateway_host is None, but the surrounding comment explicitly states the intent is to "switch v3/d14n based on presence of gateway host to preserve previous behavior and avoid breaking changes." Since gateway_host is Option<String>, callers passing None (the previous default behavior) will now receive an error instead of a V3 client. This contradicts the documented intent. Consider reverting to build_optional_d14n() or updating the comment to reflect the new requirement.

     // switch v3/d14n based on presence of gateway host to preserve
     // previous behavior and avoid breaking changes
-    let client_bundle = client_bundle.build()?;
+    let client_bundle = client_bundle.build_optional_d14n()?;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file bindings/mobile/src/mls.rs around lines 166-168:

`build()` returns an error when `gateway_host` is `None`, but the surrounding comment explicitly states the intent is to "switch v3/d14n based on presence of gateway host to preserve previous behavior and avoid breaking changes." Since `gateway_host` is `Option<String>`, callers passing `None` (the previous default behavior) will now receive an error instead of a V3 client. This contradicts the documented intent. Consider reverting to `build_optional_d14n()` or updating the comment to reflect the new requirement.

Evidence trail:
bindings/mobile/src/mls.rs:139-141 (function signature with `gateway_host: Option<String>`), bindings/mobile/src/mls.rs:167-169 (comment and `build()` call), crates/xmtp_api_d14n/src/queries/client_bundle.rs:247-250 (`build()` method calling `inner_build_d14n()`), crates/xmtp_api_d14n/src/queries/client_bundle.rs:170-176 (`inner_build_d14n()` erroring on missing gateway_host), crates/xmtp_api_d14n/src/queries/client_bundle.rs:255-265 (`build_optional_d14n()` method that implements the behavior the comment describes)

@insipx insipx force-pushed the d14n-migration branch 4 times, most recently from 5d679cb to 4601db9 Compare March 30, 2026 16:26
@insipx insipx force-pushed the d14n-migration branch 2 times, most recently from 5ea7efe to c338984 Compare April 3, 2026 15:20
Comment thread nix/shells/local.nix
Comment on lines 98 to 102
kotlin
ktlint
jdk17
swiftformat
kotlin-language-server
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High shells/local.nix:98

swiftformat is included unconditionally at line 101, but it's a Darwin-only package that downloads a pre-built macOS binary. On Linux systems, nix develop will fail because the shell tries to include a macOS Mach-O executable that cannot run. Consider removing the unconditional swiftformat since it's already correctly added via lib.optionals isDarwin at line 124.

         ktlint
         jdk17
-        swiftformat
         kotlin-language-server
 
         # Misc dev
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file nix/shells/local.nix around lines 98-102:

`swiftformat` is included unconditionally at line 101, but it's a Darwin-only package that downloads a pre-built macOS binary. On Linux systems, `nix develop` will fail because the shell tries to include a macOS Mach-O executable that cannot run. Consider removing the unconditional `swiftformat` since it's already correctly added via `lib.optionals isDarwin` at line 124.

insipx pushed a commit that referenced this pull request Apr 10, 2026
…x swiftformat

The migration client PR (#3354) changed connect_to_backend to require
both v3_host and gateway_host. All test code was passing gatewayHost=null,
causing MissingGatewayHost errors.

Changes:
- Add TestGateway constants object to Android TestHelpers.kt
- Update all ClientOptions.Api calls in Android test files with gateway host
- Update iOS TestHelpers.swift and all iOS test files with gateway host
- Remove unconditional swiftformat from nix/shells/local.nix (Darwin-only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
insipx pushed a commit that referenced this pull request Apr 15, 2026
…x swiftformat

The migration client PR (#3354) changed connect_to_backend to require
both v3_host and gateway_host. All test code was passing gatewayHost=null,
causing MissingGatewayHost errors.

Changes:
- Add TestGateway constants object to Android TestHelpers.kt
- Update all ClientOptions.Api calls in Android test files with gateway host
- Update iOS TestHelpers.swift and all iOS test files with gateway host
- Remove unconditional swiftformat from nix/shells/local.nix (Darwin-only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
insipx pushed a commit that referenced this pull request Apr 15, 2026
…x swiftformat

The migration client PR (#3354) changed connect_to_backend to require
both v3_host and gateway_host. All test code was passing gatewayHost=null,
causing MissingGatewayHost errors.

Changes:
- Add TestGateway constants object to Android TestHelpers.kt
- Update all ClientOptions.Api calls in Android test files with gateway host
- Update iOS TestHelpers.swift and all iOS test files with gateway host
- Remove unconditional swiftformat from nix/shells/local.nix (Darwin-only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +154 to +157
Client.connectToApiBackendExclusive(
ClientOptions.Api(XMTPEnvironment.DEV, true, gatewayHost = TestGateway.DEV),
)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low library/PerformanceTest.kt:154

The test calls Client.connectToApiBackendExclusive() to pre-warm the API client cache, but Client.create() internally uses connectToApiBackend() (non-exclusive). The updated toCacheKey() function includes the exclusive parameter in the cache key, so these generate different keys (|true vs |false suffix). The pre-warmed client is never reused, making the performance measurement invalid.

-        runBlocking {
-            Client.connectToApiBackendExclusive(
-                ClientOptions.Api(XMTPEnvironment.DEV, true, gatewayHost = TestGateway.DEV),
-            )
-        }
+        runBlocking {
+            Client.connectToApiBackend(
+                ClientOptions.Api(XMTPEnvironment.DEV, true, gatewayHost = TestGateway.DEV),
+            )
+        }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file sdks/android/library/src/androidTest/java/org/xmtp/android/library/PerformanceTest.kt around lines 154-157:

The test calls `Client.connectToApiBackendExclusive()` to pre-warm the API client cache, but `Client.create()` internally uses `connectToApiBackend()` (non-exclusive). The updated `toCacheKey()` function includes the `exclusive` parameter in the cache key, so these generate different keys (`|true` vs `|false` suffix). The pre-warmed client is never reused, making the performance measurement invalid.

Evidence trail:
sdks/android/library/src/main/java/org/xmtp/android/library/Client.kt lines 152-153: `toCacheKey()` implementation shows `|$exclusive` suffix in key
sdks/android/library/src/main/java/org/xmtp/android/library/Client.kt line 221: `connectToApiBackend()` calls `api.toCacheKey()` (exclusive defaults to false)
sdks/android/library/src/main/java/org/xmtp/android/library/Client.kt line 270: `connectToApiBackendExclusive()` calls `api.toCacheKey(exclusive = true)`
sdks/android/library/src/main/java/org/xmtp/android/library/Client.kt line 585: `createClient` (in Client.create) uses `connectToApiBackend(options.api)`
sdks/android/library/src/androidTest/java/org/xmtp/android/library/PerformanceTest.kt lines 154-167: Test pre-warms with `connectToApiBackendExclusive()` then calls `Client.create()`

insipx pushed a commit that referenced this pull request Apr 15, 2026
…x swiftformat

The migration client PR (#3354) changed connect_to_backend to require
both v3_host and gateway_host. All test code was passing gatewayHost=null,
causing MissingGatewayHost errors.

Changes:
- Add TestGateway constants object to Android TestHelpers.kt
- Update all ClientOptions.Api calls in Android test files with gateway host
- Update iOS TestHelpers.swift and all iOS test files with gateway host
- Remove unconditional swiftformat from nix/shells/local.nix (Darwin-only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…x swiftformat

The migration client PR (#3354) changed connect_to_backend to require
both v3_host and gateway_host. All test code was passing gatewayHost=null,
causing MissingGatewayHost errors.

Changes:
- Add TestGateway constants object to Android TestHelpers.kt
- Update all ClientOptions.Api calls in Android test files with gateway host
- Update iOS TestHelpers.swift and all iOS test files with gateway host
- Remove unconditional swiftformat from nix/shells/local.nix (Darwin-only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants