feat: V3 only dms #411
feat: V3 only dms #411
Conversation
…-contract-wallets
…tp-ios into np/smart-contract-wallets
…-contract-wallets
…-contract-wallets
…-contract-wallets
|
Warning Rate limit exceeded@nplasterer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes include significant updates to the XMTPiOS framework, particularly in error handling and conversation management. A new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (39)
Sources/XMTPiOS/Extensions/Ffi.swift (1)
202-202: Remove trailing whitespace.There are trailing whitespace characters on empty lines 202 and 206.
func groupFromFFI(client: Client) -> Group { Group(ffiGroup: self, client: client) } - + func dmFromFFI(client: Client) -> Dm { Dm(ffiConversation: self, client: client) } - + func toConversation(client: Client) throws -> Conversation {Also applies to: 206-206
🧰 Tools
🪛 SwiftLint
[Warning] 202-202: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/XMTPTests/DmTests.swift (1)
14-14: Consider documenting the iOS 16 requirement rationale.The
@available(iOS 16, *)marker indicates a minimum iOS requirement. It would be helpful to document why iOS 16 is specifically required for these tests, especially if it's related to certain APIs or features being used.Tests/XMTPTests/V3ClientTests.swift (2)
74-91: Enhance test coverage with additional edge casesWhile the test covers basic DM functionality well, consider adding these scenarios:
- Verify DM content persistence across client restarts
- Test DM creation with invalid wallet addresses
- Assert specific error types when V2 client attempts access
Would you like me to provide example test cases for these scenarios?
🧰 Tools
🪛 SwiftLint
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
154-156: Add time delays between messages for reliable ordering testTo ensure consistent ordering in the test:
- Add delays between messages to guarantee different timestamps
- Verify message timestamps in the ordering assertion
Example implementation:
_ = try await dm.send(content: "Howdy") + try await Task.sleep(nanoseconds: 1_000_000_000) // 1 second _ = try await group2.send(content: "Howdy") + try await Task.sleep(nanoseconds: 1_000_000_000) // 1 secondTests/XMTPTests/IntegrationTests.swift (1)
Line range hint
12-24: Consider enhancing test data persistence coverageThe
testUsingSavedCredentialsAndKeyMaterialtest verifies basic persistence, but with the introduction of V3 DMs, we should enhance it to cover:
- V3-specific key material persistence
- Migration scenarios
- Compatibility checks
Consider expanding the test to include V3-specific scenarios:
func testUsingSavedCredentialsAndKeyMaterial() async throws { let opt = ClientOptions(api: .init(env: .local, isSecure: false)) - let alice = try await Client.create(account: try PrivateKey.generate(), options: opt) + let alice = try await Client.createV3(account: try PrivateKey.generate(), options: opt) let bob = try await Client.create(account: try PrivateKey.generate(), options: opt) // Alice starts a conversation with Bob let aliceConvo = try await alice.conversations.newConversation(Also applies to: 157-159
Sources/XMTPiOS/Client.swift (1)
699-699: Remove trailing whitespaceSwiftLint detected trailing whitespace on these lines. While this is a minor issue, it's good practice to maintain consistent formatting.
Also applies to: 707-707, 723-723
🧰 Tools
🪛 SwiftLint
[Warning] 699-699: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/XMTPTests/GroupTests.swift (1)
763-766: LGTM! Consider enhancing the error message.The addition of the
.dmcase is correct and aligns with the PR objectives for V3-only DM support.Consider making the error message more specific:
-XCTFail("failed converting conversation to group") +XCTFail("unexpected DM type when converting conversation to group")Sources/XMTPiOS/Dm.swift (1)
248-259: Simplify closure by removing unnecessary parenthesesThe closures used for setting
statuscan be simplified by removing unnecessary parentheses.Simplify the closure syntax:
- let status: FfiDeliveryStatus? = { + let status: FfiDeliveryStatus? = switch deliveryStatus { case .published: FfiDeliveryStatus.published case .unpublished: FfiDeliveryStatus.unpublished case .failed: FfiDeliveryStatus.failed default: nil } - }()This leverages Swift 5.7's ability to omit return statements in single-expression closures.
Also applies to: 306-317
Sources/XMTPiOS/Conversation.swift (4)
Line range hint
427-434: Standardize error handling inencodemethodIn the
encodemethod, the.v1case throwsRemoteAttachmentError.v1NotSupported, while the.groupand.dmcases throwConversationError.v3NotSupported("encode"). For consistency, consider using a single error type or a unified error handling approach for unsupported cases.
Line range hint
34-98: Refactor repetitive error handling codeMultiple methods contain repetitive patterns where the
.v1and.v2cases throw aConversationErrorindicating the method is not supported. Consider refactoring this repetitive code to reduce duplication, perhaps by implementing a helper function or default case to handle unsupported versions.Also applies to: 113-124
🧰 Tools
🪛 SwiftLint
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 47-47: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 60-60: Lines should not have trailing whitespace
(trailing_whitespace)
32-32: Remove trailing whitespaceLines 32, 47, 60, 86, 99, 125, 222, 305, and 307 contain trailing whitespace. Please remove the trailing whitespace to adhere to coding standards and improve code cleanliness.
Also applies to: 47-47, 60-60, 86-86, 99-99, 125-125, 222-222, 305-305, 307-307
🧰 Tools
🪛 SwiftLint
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
479-479: Limit vertical whitespace to a single empty lineLines 479 and 493 have multiple consecutive empty lines. Limiting vertical whitespace to a single empty line enhances code readability and maintains consistency throughout the codebase.
Also applies to: 493-493
🧰 Tools
🪛 SwiftLint
[Warning] 479-479: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
Sources/XMTPiOS/Conversations.swift (27)
130-130: Remove the trailing whitespace.Line 130 has unnecessary trailing whitespace, which can affect code cleanliness.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
154-154: Remove the trailing whitespace.Line 154 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
174-174: Remove the trailing whitespace.Line 174 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 174-174: Lines should not have trailing whitespace
(trailing_whitespace)
156-158: Improve the error message for clarity.The error message in the
v2NotSupportedexception could be clearer and more grammatically correct.Apply this diff to improve the error message:
-throw ConversationError.v2NotSupported("Only supported with V3 only clients use newConversation instead") +throw ConversationError.v2NotSupported("This function is only supported with V3 clients. Please use `newConversation` instead.")
347-349: Improve the error message for clarity.The error message in the
v2NotSupportedexception could be clearer.Apply this diff to improve the error message:
-throw ConversationError.v2NotSupported("Only supported with V3 only clients use newConversation instead") +throw ConversationError.v2NotSupported("This function is only supported with V3 clients. Please use `newConversation` instead.")
355-356: Consider usinglocalizedComparefor case-insensitive string comparison.When comparing addresses in a case-insensitive manner, it's recommended to use
localizedCaseInsensitiveComparefor better locale support.Apply this diff to enhance the string comparison:
-if peerAddress.lowercased() == client.address.lowercased() { +if peerAddress.caseInsensitiveCompare(client.address) == .orderedSame {
472-472: Remove unnecessary empty line to adhere to style guidelines.Line 472 contains an extra empty line. Swift style guidelines recommend limiting vertical whitespace.
Apply this diff to remove the unnecessary empty line:
-
1033-1034: Replace debug print statement with a more informative message.The debug message
"huh \(envelope)"is unclear. Consider providing a more descriptive message.Apply this diff to improve the debug message:
-print("huh \(envelope)") +print("Received an envelope with an unexpected content topic: \(envelope.contentTopic)")
992-992: Address function length warning from static analysis.The function
streamAllV2Messagesexceeds the recommended function body length.Refer to the earlier suggestion to refactor this function into smaller parts to comply with the style guidelines.
[function_body_length]
🧰 Tools
🪛 SwiftLint
[Warning] 992-992: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
1043-1044: Replace unused closure parameter with an underscore.In line 1043, the closure parameter in
continuation.onTerminationis unused and should be replaced with an underscore to indicate it's intentionally ignored.Apply this diff to replace the unused parameter:
-continuation.onTermination = { @Sendable reason in +continuation.onTermination = { @Sendable _ in🧰 Tools
🪛 SwiftLint
[Warning] 1043-1043: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
1052-1052: Address function length warning from static analysis.The function
streamAllV2DecryptedMessagesexceeds the recommended function body length.Refer to the earlier suggestion to refactor this function into smaller parts to comply with the style guidelines.
[function_body_length]
🧰 Tools
🪛 SwiftLint
[Warning] 1052-1052: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
892-892: Ensure consistency in error messages and exception handling.In the
importTopicDatafunction, the error message forv3NotSupportedshould match the style of other error messages.Apply this diff to improve the error message:
-throw ConversationError.v3NotSupported("importTopicData only supported with V2 clients") +throw ConversationError.v3NotSupported("This function is only supported with V2 clients.")
921-923: Avoid magic numbers by defining constants.The
maxQueryRequestsPerBatchis set to 50 directly. Consider defining it as a constant at the top of the class or file for better maintainability.Apply this diff to define the constant:
+private let maxQueryRequestsPerBatch = 50 // Then, remove the local variable declaration in the function -let maxQueryRequestsPerBatch = 50
241-241: Remove empty parentheses when using trailing closures.According to Swift style guidelines, when using trailing closures, empty parentheses should be avoided after the method call.
Apply this diff to remove the empty parentheses:
-AsyncThrowingStream { continuation in +AsyncThrowingStream { continuation in[empty_parentheses_with_trailing_closure]
🧰 Tools
🪛 SwiftLint
[Warning] 241-241: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
308-308: Remove empty parentheses when using trailing closures.Similarly, remove the empty parentheses in line 308.
Apply this diff:
-let stream = await self.client.v3Client?.conversations().stream( +let stream = await self.client.v3Client?.conversations().stream[empty_parentheses_with_trailing_closure]
🧰 Tools
🪛 SwiftLint
[Warning] 308-308: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
274-274: Remove empty parentheses when using trailing closures.In line 274, avoid empty parentheses when using a trailing closure.
Apply this diff:
-let stream = await self.client.v3Client?.conversations().streamGroups( +let stream = await self.client.v3Client?.conversations().streamGroups[empty_parentheses_with_trailing_closure]
🧰 Tools
🪛 SwiftLint
[Warning] 274-274: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
330-330: Replace unused closure parameter with an underscore.In line 330, the parameter in the closure is unused.
Apply this diff:
-continuation.onTermination = { @Sendable reason in +continuation.onTermination = { @Sendable _ in[unused_closure_parameter]
🧰 Tools
🪛 SwiftLint
[Warning] 330-330: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
337-337: Replace unused closure parameter with an underscore.Similarly, in line 337.
Apply this diff:
-continuation.onTermination = { @Sendable reason in +continuation.onTermination = { @Sendable _ in[unused_closure_parameter]
🧰 Tools
🪛 SwiftLint
[Warning] 337-337: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
448-448: Remove the trailing whitespace.Line 448 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 448-448: Lines should not have trailing whitespace
(trailing_whitespace)
484-484: Remove the trailing whitespace.Line 484 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 484-484: Lines should not have trailing whitespace
(trailing_whitespace)
665-665: Remove the trailing whitespace.Line 665 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 665-665: Lines should not have trailing whitespace
(trailing_whitespace)
762-762: Remove the trailing whitespace.Line 762 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 762-762: Lines should not have trailing whitespace
(trailing_whitespace)
991-991: Remove the trailing whitespace.Line 991 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 991-991: Lines should not have trailing whitespace
(trailing_whitespace)
1042-1042: Remove the trailing whitespace.Line 1042 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 1042-1042: Lines should not have trailing whitespace
(trailing_whitespace)
1051-1051: Remove the trailing whitespace.Line 1051 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 1051-1051: Lines should not have trailing whitespace
(trailing_whitespace)
1101-1101: Remove the trailing whitespace.Line 1101 has unnecessary trailing whitespace.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 SwiftLint
[Warning] 1101-1101: Lines should not have trailing whitespace
(trailing_whitespace)
1102-1103: Replace unused closure parameter with an underscore.In
continuation.onTermination, the parameterreasonis unused.Apply this diff:
-continuation.onTermination = { @Sendable reason in +continuation.onTermination = { @Sendable _ in[unused_closure_parameter]
🧰 Tools
🪛 SwiftLint
[Warning] 1102-1102: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- Sources/XMTPiOS/Client.swift (3 hunks)
- Sources/XMTPiOS/Conversation.swift (11 hunks)
- Sources/XMTPiOS/Conversations.swift (12 hunks)
- Sources/XMTPiOS/Dm.swift (1 hunks)
- Sources/XMTPiOS/Extensions/Ffi.swift (1 hunks)
- Sources/XMTPiOS/Group.swift (1 hunks)
- Tests/XMTPTests/DmTests.swift (1 hunks)
- Tests/XMTPTests/GroupTests.swift (1 hunks)
- Tests/XMTPTests/IntegrationTests.swift (1 hunks)
- Tests/XMTPTests/V3ClientTests.swift (1 hunks)
🧰 Additional context used
🪛 SwiftLint
Sources/XMTPiOS/Client.swift
[Warning] 699-699: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 707-707: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 723-723: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/XMTPiOS/Conversation.swift
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 47-47: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 60-60: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 86-86: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 99-99: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 125-125: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 222-222: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 305-305: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 307-307: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 479-479: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 493-493: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
Sources/XMTPiOS/Conversations.swift
[Warning] 131-131: Return arrow and return type should be separated by a single space or on a separate line
(return_arrow_whitespace)
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 241-241: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 174-174: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 274-274: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 308-308: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 298-298: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 345-345: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 330-330: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 337-337: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 448-448: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 484-484: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 521-521: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 665-665: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 762-762: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 992-992: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 1052-1052: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 884-884: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 886-886: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 991-991: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 999-999: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1042-1042: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1051-1051: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1101-1101: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1043-1043: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 1102-1102: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Sources/XMTPiOS/Dm.swift
[Warning] 19-19: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 47-47: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 92-92: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 143-143: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 148-148: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 178-178: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 208-208: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 262-262: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 305-305: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 318-318: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 320-320: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 179-179: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 184-184: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 209-209: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 214-214: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Sources/XMTPiOS/Extensions/Ffi.swift
[Warning] 202-202: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 206-206: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/XMTPTests/V3ClientTests.swift
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 131-131: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 139-139: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (11)
Sources/XMTPiOS/Extensions/Ffi.swift (2)
199-202: LGTM! Clear method naming.The rename from
fromFFItogroupFromFFIimproves code clarity by explicitly indicating its purpose for group conversion.🧰 Tools
🪛 SwiftLint
[Warning] 202-202: Lines should not have trailing whitespace
(trailing_whitespace)
203-205: 🛠️ Refactor suggestionConsider adding V3 validation.
Since this PR aims to support V3 only dms, consider adding validation to ensure the conversation is V3 compatible before conversion.
Consider adding version validation:
func dmFromFFI(client: Client) -> Dm { + guard isV3Compatible() else { + throw XMTPError.incompatibleVersion("Direct messages require V3") + } return Dm(ffiConversation: self, client: client) }Tests/XMTPTests/DmTests.swift (1)
120-142: Verify message count assertions in testCanSendMessageToDm.There's an inconsistency in the message count assertions:
- Line 133 expects 3 messages
- Line 140 expects 2 messages
- Only 2 messages are explicitly sent in the test
This discrepancy needs clarification or correction. Consider adding comments explaining the expected message count differences between clients.
✅ Verification successful
Let me gather more context about the DM messaging implementation to understand the message count behavior better.
Let me try a different approach to understand the message handling implementation.
Let me check the Dm.swift implementation to understand the message handling better.
After reviewing the Dm.swift implementation, I can now generate the final response.
Message count discrepancy is expected due to message delivery status
The difference in message counts between clients is expected because:
- The
messages()method by default includes all message delivery statuses (published, unpublished, failed)- The sender's client (bo) sees both published and unpublished messages (3 total: "howdy", "gm", plus potentially an unpublished message)
- The receiver's client (alix) only sees published messages (2 total: "howdy", "gm")
The test is correctly verifying this behavior, though it would benefit from a comment explaining the expected count difference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other message count assertions in tests rg -A 2 "messages\.count.*\d+" Tests/Length of output: 911
Script:
#!/bin/bash # Search for DM message handling implementation ast-grep --pattern 'func messages() async throws -> [Message' # Also look for any send message implementation ast-grep --pattern 'func send(content: $$$) async throws' # And search for any comments or documentation about message syncing rg -l "sync|message" Sources/ --type mdLength of output: 165
Script:
#!/bin/bash # Search for DM implementation files fd "Dm" Sources/ # Look for message-related code in the test file rg -B 5 -A 5 "send\(content:" Tests/XMTPTests/DmTests.swift # Search for any sync-related implementation rg -B 3 -A 3 "func sync" Sources/Length of output: 3998
Script:
#!/bin/bash # Look at the Dm.swift implementation cat Sources/XMTPiOS/Dm.swift # Also check for any message-related implementations rg -B 3 -A 3 "func messages\(\)" Sources/Length of output: 8672
Sources/XMTPiOS/Group.swift (2)
232-234: Verify V2/V3 message separationAs per PR objectives, we need to ensure V2 clients cannot access V3 messages. Please verify that the FFI layer's
processStreamedConversationMessageimplements this separation correctly.Let's check for V2/V3 separation mechanisms:
#!/bin/bash # Search for V2/V3 client checks in the codebase rg "hasV2Client|isV3" --type swift # Look for version-specific message handling rg "V3.*Message|V2.*Message" --type swift
232-234: Breaking change: Verify impact of return type changeThe change from
DecodedMessagetoMessageV3return type is a breaking change that could affect existing clients. This aligns with the V3-only objective but requires careful migration handling.Let's verify the usage of this method across the codebase:
Consider adding migration documentation or deprecation warnings for existing clients.
Tests/XMTPTests/IntegrationTests.swift (2)
157-157: LGTM: Error handling improvementThe addition of
trykeyword aligns with the updated error handling in the conversation management system.
Line range hint
1-669: Add test coverage for V3 DMsGiven that this PR implements V3-only direct messages support, we should add test cases to verify:
- V3 DM creation and message exchange
- V2 client access prevention
- V3 DM persistence and restoration
- Error cases when V2 clients attempt to access V3 DMs
Let's verify if there are any V3 DM tests in the codebase:
Consider adding the following test cases:
testV3DMCreationtestV2ClientCannotAccessV3DMtestV3DMPersistencetestV3DMErrorHandlingWould you like me to help generate these test cases?
Sources/XMTPiOS/Client.swift (3)
18-18: LGTM: Error case addition for missing inbox IDThe new error case and its description are well-implemented and align with the V3-only DMs support objective.
Also applies to: 28-29
700-706: LGTM: Conversation finding implementationThe method properly validates the V3 client and handles conversation ID conversion.
724-733: LGTM: DM finding implementationThe method properly validates the V3 client and inbox ID presence, with appropriate error handling. This implementation aligns well with the V3-only DMs support objective.
Sources/XMTPiOS/Dm.swift (1)
74-74: Verify timestamp conversion accuracyIn the
createdAtproperty, the conversion from nanoseconds to aDatemight be incorrect if the methodffiConversation.createdAtNs()returns nanoseconds butDate(millisecondsSinceEpoch:)expects milliseconds.Please confirm the units returned by
ffiConversation.createdAtNs(). If it returns nanoseconds, adjust the conversion:public var createdAt: Date { - Date(millisecondsSinceEpoch: ffiConversation.createdAtNs()) + Date(millisecondsSinceEpoch: ffiConversation.createdAtNs() / 1_000_000) }
| guard !Task.isCancelled else { | ||
| continuation.finish() | ||
| Task { | ||
| await ffiStreamActor.endStream() // End the stream upon cancellation | ||
| } | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary nesting of Task within a Task.
Inside the messageCallback, you're creating a new Task inside an existing Task, which can be avoided.
Apply this diff to refactor the code:
-guard !Task.isCancelled else {
- continuation.finish()
- Task {
- await ffiStreamActor.endStream() // End the stream upon cancellation
- }
- return
+if Task.isCancelled {
+ continuation.finish()
+ await ffiStreamActor.endStream() // End the stream upon cancellation
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard !Task.isCancelled else { | |
| continuation.finish() | |
| Task { | |
| await ffiStreamActor.endStream() // End the stream upon cancellation | |
| } | |
| return | |
| if Task.isCancelled { | |
| continuation.finish() | |
| await ffiStreamActor.endStream() // End the stream upon cancellation | |
| return |
There was a problem hiding this comment.
Not sure if we should try these suggestions since the streams seem to be working and it makes me a little nervous changing the way they previously worked since we've had a lot of issues in the past.
There was a problem hiding this comment.
@nplasterer, understood. It's important to maintain stability, especially given past issues with streams. Please let me know if there's anything else I can assist with.
✏️ Learnings added
Learnt from: nplasterer
PR: xmtp/xmtp-ios#411
File: Sources/XMTPiOS/Conversations.swift:469-474
Timestamp: 2024-10-24T20:26:50.514Z
Learning: In the XMTPiOS project, changes to stream handling code can be risky due to past issues. Avoid suggesting modifications to streams unless necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| try await streamManager.updateStream(with: subscriptionRequest) | ||
| } else { |
There was a problem hiding this comment.
Handle potential errors when updating the stream.
When calling try await streamManager.updateStream(with: subscriptionRequest), if an error occurs, it isn't being caught, which could cause the stream to terminate unexpectedly.
Consider wrapping the call in a do-catch block and handling the error appropriately:
+do {
try await streamManager.updateStream(with: subscriptionRequest)
+} catch {
+ continuation.finish(throwing: error)
+}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Same for this one.
There was a problem hiding this comment.
@nplasterer Understood, I'll skip this suggestion.
(つ﹏⊂)
✏️ Learnings added
Learnt from: nplasterer
PR: xmtp/xmtp-ios#411
File: Sources/XMTPiOS/Conversations.swift:1041-1042
Timestamp: 2024-10-24T20:27:04.886Z
Learning: In `Sources/XMTPiOS/Conversations.swift`, when calling `streamManager.updateStream(with: subscriptionRequest)`, the user prefers not to handle errors with a `do-catch` block.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
Sources/XMTPiOS/Conversations.swift (3)
52-64: Clean up callback initialization format.The callback initialization can be simplified by removing empty parentheses.
-let callback = ConversationStreamCallback() { group in +let callback = ConversationStreamCallback { group in
131-131: Fix return arrow spacing.The return arrow should have consistent spacing.
-public func syncAllConversations() async throws -> UInt32 { +public func syncAllConversations() async throws -> UInt32 {🧰 Tools
🪛 SwiftLint
[Warning] 131-131: Return arrow and return type should be separated by a single space or on a separate line
(return_arrow_whitespace)
179-179: Implement missing functionality for order and consent state.The TODO comment indicates that ordering and consent state functionality is not yet implemented. This could affect the completeness of the V3 client implementation.
Would you like help implementing the missing functionality for ordering and consent state?
Tests/XMTPTests/V3ClientTests.swift (5)
88-90: Clarify expected error in async error assertionThe test expects an error to be thrown when creating a DM with
fixtures.alixV2.walletAddress. To ensure the test accurately validates the expected behavior, specify the type of error or error message that should be thrown.
191-191: Correct function name to match naming conventionsThe function is named
testsCanSendMessagesToDm, which is inconsistent with the naming convention of other test functions. Rename it totestCanSendMessagesToDmfor consistency.
204-204: Remove extra blank lineThere are multiple consecutive blank lines at line 204. Limit vertical whitespace to a single empty line to improve code readability.
🧰 Tools
🪛 SwiftLint
[Warning] 204-204: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
270-288: Ensure task cancellation after fulfilling expectationsIn the
testCanStreamAllMessagesFromV3Usersfunction, the backgroundTaskcontinues to run even after the test expectations are fulfilled, which might lead to resource leaks or unpredictable behavior. Consider canceling the task once the expected messages have been received.Apply this diff to handle task cancellation:
Task(priority: .userInitiated) { for try await _ in await fixtures.boV3Client.conversations.streamAllConversationMessages() { expectation1.fulfill() + if expectation1.expectedFulfillmentCount == expectation1.fulfillmentCount { + break + } } + // Optionally, you can add code here to cancel the task if needed }
80-80: Remove trailing whitespace from specified linesLines 80, 131, 139, 197, 213, 269, 289, and 309 contain trailing whitespace. Please remove the extra spaces at the end of these lines to comply with code style guidelines.
Also applies to: 131-131, 139-139, 197-197, 213-213, 269-269, 289-289, 309-309
🧰 Tools
🪛 SwiftLint
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Sources/XMTPiOS/Conversations.swift (12 hunks)
- Tests/XMTPTests/GroupTests.swift (14 hunks)
- Tests/XMTPTests/V3ClientTests.swift (3 hunks)
- XMTP.podspec (1 hunks)
🧰 Additional context used
🪛 SwiftLint
Sources/XMTPiOS/Conversations.swift
[Warning] 131-131: Return arrow and return type should be separated by a single space or on a separate line
(return_arrow_whitespace)
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 241-241: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 174-174: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 274-274: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 308-308: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 298-298: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 345-345: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 330-330: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 337-337: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 448-448: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 484-484: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 521-521: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 665-665: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 762-762: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 992-992: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 1052-1052: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 884-884: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 886-886: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 991-991: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 999-999: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1042-1042: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1051-1051: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1101-1101: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1043-1043: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 1102-1102: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Tests/XMTPTests/V3ClientTests.swift
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 131-131: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 139-139: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 197-197: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 213-213: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 204-204: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 269-269: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 289-289: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 309-309: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (13)
XMTP.podspec (2)
19-19: Consider if a major version bump is needed.The change from 0.15.2 to 0.16.0 represents a minor version bump. However, since this PR introduces V3-only DMs and explicitly prevents V2 clients from accessing V3 DMs, this could be considered a breaking change that warrants a major version bump (e.g., to 1.0.0) according to semantic versioning principles.
Would you like me to help analyze the impact on existing V2 clients to determine if this is indeed a breaking change?
19-19: Verify dependency compatibility with V3-only features.While the dependencies remain unchanged, please verify that the current versions of
LibXMTP(0.5.10) and other dependencies fully support V3-only DM functionality.✅ Verification successful
Let me check the release notes and changelog to verify V3 support.
Let me check the actual code for V3 support indicators.
Based on the extensive code analysis, I can now provide a definitive response about V3 support.
V3 functionality is fully supported and properly implemented in the codebase
The codebase shows comprehensive V3 support with:
- Full V3 client implementation with dedicated V3 message handling
- Complete V3-specific DM functionality including creation, sending, and streaming
- Proper V3 consent management and permissions
- V3 group messaging capabilities
- Latest LibXMTP version (0.5.8-beta4) includes V3 streaming improvements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any recent releases of LibXMTP that might include V3 support gh release list --repo xmtp/libxmtp-swift --limit 10Length of output: 520
Script:
#!/bin/bash # Check release notes and changelog for V3 support mentions gh release view --repo xmtp/libxmtp-swift 0.5.8-beta4 # Also check the changelog file if it exists fd changelog -i -t f --exec cat {}Length of output: 845
Script:
#!/bin/bash # Search for V3-related code and configurations rg -i "v3|version.*3" --type swift -A 3 # Look for protocol version related code ast-grep --pattern 'enum VersionedApi { $$$ }'Length of output: 80094
Sources/XMTPiOS/Conversations.swift (3)
Line range hint
5-18: Well-structured error handling for version compatibility.The new error cases and their descriptions clearly communicate version compatibility requirements to API consumers.
🧰 Tools
🪛 SwiftLint
[Warning] 21-21: Lines should not have trailing whitespace
(trailing_whitespace)
201-227: Well-implemented sorting and filtering logic.The helper functions are robust, handle edge cases well, and maintain good separation of concerns.
346-364: Secure implementation of DM functionality.The implementation includes proper validation and security checks:
- Prevents self-messaging
- Validates recipient network status
- Includes proper consent handling
Tests/XMTPTests/V3ClientTests.swift (2)
119-123: Previous comment still applies: Fix potential race condition in conversation sync testAs previously noted, the test assumes immediate sync completion. Network delays could make this test flaky. Consider adding a retry mechanism or verifying sync completion before making assertions to enhance test reliability.
140-142:⚠️ Potential issueFix incorrect consent state filter in assertion
Both
convoCountAllowedandconvoCountDeniedare usingconsentState: .allowed, which makes the assertions incorrect. UpdateconvoCountDeniedto useconsentState: .deniedto reflect the correct consent state.Apply this diff to fix the filtering:
let convoCountAllowed = try await fixtures.boV3Client.conversations.listConversations(consentState: .allowed).count -let convoCountDenied = try await fixtures.boV3Client.conversations.listConversations(consentState: .allowed).count +let convoCountDenied = try await fixtures.boV3Client.conversations.listConversations(consentState: .denied).countLikely invalid or redundant comment.
Tests/XMTPTests/GroupTests.swift (6)
42-46: Declaration of new variables for V3 client is appropriateThe addition of
davonV3anddavonV3Clientis consistent with existing fixtures, and correctly sets up a V3-only client for testing purposes.
51-56: Initialization ofClientOptionswith V3 enabledThe
ClientOptionsare properly initialized withenableV3: trueand include the necessary codec[GroupUpdatedCodec()], which aligns with the objective of testing V3 functionalities.
73-76: Use ofClient.createV3for V3-only client creationInitializing
davonV3ClientwithClient.createV3ensures it is a V3-only client, which is appropriate for testing V3 direct messages as per the PR objectives.
83-87: Inclusion of new V3 client in returned fixturesAll clients, including the new
davonV3anddavonV3Client, are correctly included in theLocalFixturesreturn statement, ensuring they are available for testing.
216-217: Inconsistent use ofwalletAddressinstead ofaddressConsistent with the previous comment on lines 199-200, replace
walletAddresswithaddressfor consistency.
781-784: 🛠️ Refactor suggestionHandle unexpected conversation types in switch statement
The switch statement includes a case for
.dm(_)that fails the test if encountered. Consider handling the.dm(_)case appropriately or adding adefaultcase to account for all possible conversation types.Apply this diff to include a
defaultcase:switch bobConv { case .v1(_): XCTFail("failed converting conversation to group") return case .v2(_): XCTFail("failed converting conversation to group") return case .group(let group): bobGroup = group + default: + XCTFail("failed converting conversation to group") + return }Alternatively, handle the
.dm(_)case specifically if necessary.Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
All the tests are green! 
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
Sources/XMTPiOS/Extensions/Ffi.swift (1)
202-202: Remove trailing whitespace.Remove trailing whitespace from lines 202 and 206 to maintain code formatting consistency.
Also applies to: 206-206
🧰 Tools
🪛 SwiftLint
[Warning] 202-202: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/XMTPTests/V3ClientTests.swift (3)
74-91: Add negative test cases for DM creationConsider adding test cases for:
- Creating DMs with invalid wallet addresses
- Creating DMs with empty addresses
- Creating DMs with the user's own address
🧰 Tools
🪛 SwiftLint
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
270-288: Add error handling test cases for streamingThe streaming tests only cover the happy path. Consider adding test cases for:
- Network disconnection scenarios
- Invalid message formats
- Stream timeout scenarios
80-80: Fix formatting issuesSeveral lines contain trailing whitespace or excessive vertical spacing.
Consider running SwiftFormat or similar tool to automatically fix these issues.
Also applies to: 131-131, 139-139, 197-197, 213-213, 204-204, 269-269, 289-289, 309-309
🧰 Tools
🪛 SwiftLint
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/XMTPiOS/Conversation.swift (2)
306-307: Enhance deprecation documentationConsider adding more detailed documentation about the deprecation strategy for V1/V2 functionality:
- When will these methods be removed?
- What's the migration path for V1/V2 clients?
- Are there any compatibility guarantees?
🧰 Tools
🪛 SwiftLint
[Warning] 307-307: Lines should not have trailing whitespace
(trailing_whitespace)
32-32: Clean up code formatting and documentationSeveral maintenance items to address:
- Remove trailing whitespace on lines 32, 47, 60, 86, 99, 125, 222, 305, 307
- Fix vertical spacing (limit to single empty line) at lines 479, 493
- Consider adding documentation for new DM methods similar to existing method documentation
Apply this diff to fix the whitespace issues:
- +Also applies to: 47-47, 60-60, 86-86, 99-99, 125-125, 222-222, 305-305, 307-307, 479-479, 493-493
🧰 Tools
🪛 SwiftLint
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/XMTPiOS/Client.swift (2)
Line range hint
203-223: Enhance error message clarityThe error message could be more specific about the inbox ID requirement for better debugging.
- throw ClientError.creationError("No encryption key passed for the database. Please store and provide a secure encryption key.") + throw ClientError.creationError("Database encryption key is required for V3 client initialization. Please store and provide a secure encryption key.")🧰 Tools
🪛 SwiftLint
[Warning] 213-213: Lines should not have trailing whitespace
(trailing_whitespace)
733-746: Improve error message for missing inbox IDThe implementation is correct, but the error message could be more descriptive.
- throw ClientError.creationError("No inboxId present") + throw ClientError.missingInboxIdConsider using the dedicated
missingInboxIderror case that was added specifically for this purpose, instead of using a generic creation error.Tests/XMTPTests/GroupTests.swift (2)
42-46: Add documentation for V3 client setup.Consider adding documentation comments to explain the purpose and significance of the
davonV3client, particularly its role in testing V3-only direct messages functionality.Also applies to: 73-76
566-567: Enhance test assertions for DM operations.Consider adding specific assertions to verify that DM operations don't interfere with group functionality. For example, verify that the DM creation doesn't affect the group count or group streaming behavior.
Also applies to: 585-586
Sources/XMTPiOS/Conversations.swift (3)
155-173: Standardize error message format.The error message format in
dmsmethod differs from other similar methods. Consider standardizing the error message format across all methods.Apply this diff to maintain consistency:
- throw ConversationError.v2NotSupported("Only supported with V3 only clients use newConversation instead") + throw ConversationError.v2NotSupported("Only supported with V3 clients. Use newConversation instead.")
459-491: Enhance error handling in streamAllConversationMessages.The error handling in the message processing block could be more informative. Consider adding more context to the error message.
Apply this diff to improve error handling:
} catch { - print("Error onMessage \(error)") + print("Failed to process message in streamAllConversationMessages: \(error)") }
895-896: Add deprecation warning comments.Consider adding proper deprecation warning comments to help developers transition away from V2 functionality.
Add deprecation warnings:
// ------- V1 V2 to be deprecated ------ +// DEPRECATED: These methods will be removed in a future version. +// Please migrate to V3-specific methods.🧰 Tools
🪛 SwiftLint
[Warning] 896-896: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- Sources/XMTPiOS/Client.swift (5 hunks)
- Sources/XMTPiOS/Conversation.swift (11 hunks)
- Sources/XMTPiOS/Conversations.swift (11 hunks)
- Sources/XMTPiOS/Extensions/Ffi.swift (1 hunks)
- Tests/XMTPTests/GroupTests.swift (14 hunks)
- Tests/XMTPTests/V3ClientTests.swift (3 hunks)
🧰 Additional context used
🪛 SwiftLint
Sources/XMTPiOS/Client.swift
[Warning] 700-700: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 712-712: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 732-732: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/XMTPiOS/Conversation.swift
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 47-47: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 60-60: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 86-86: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 99-99: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 125-125: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 222-222: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 305-305: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 307-307: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 479-479: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 493-493: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
Sources/XMTPiOS/Conversations.swift
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 241-241: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 174-174: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 274-274: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 308-308: When using trailing closures, empty parentheses should be avoided after the method call
(empty_parentheses_with_trailing_closure)
[Warning] 298-298: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 345-345: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 361-361: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 367-367: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 371-371: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 330-330: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 337-337: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 458-458: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 494-494: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 531-531: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 675-675: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 772-772: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1002-1002: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 1062-1062: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
[Warning] 894-894: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 896-896: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1001-1001: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1009-1009: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1052-1052: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1061-1061: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1111-1111: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 1053-1053: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 1112-1112: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Sources/XMTPiOS/Extensions/Ffi.swift
[Warning] 202-202: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 206-206: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/XMTPTests/V3ClientTests.swift
[Warning] 80-80: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 131-131: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 139-139: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 197-197: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 213-213: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 204-204: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
[Warning] 269-269: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 289-289: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 309-309: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (13)
Sources/XMTPiOS/Extensions/Ffi.swift (2)
199-201: LGTM! Clear method naming.The rename from
fromFFItogroupFromFFIimproves code clarity by explicitly indicating its group conversion purpose.
207-213: Enhance conversation type determination.The current implementation needs more robust error handling as suggested in the previous review.
The previous review comment suggesting enhanced error handling for metadata parsing, V3 compatibility validation, and unknown conversation types is still applicable.
Tests/XMTPTests/V3ClientTests.swift (2)
198-202: Verify message count assertionThe test asserts
dmMessages.countis 3, but only sends 2 messages ("howdy" and "gm"). This might indicate:
- An initialization message is being counted
- The count assertion is incorrect
Please verify the expected message count and update the assertion accordingly.
119-123:⚠️ Potential issueFix potential race condition in conversation sync
The sync operation might not complete immediately, making the test flaky.
Apply this diff to add retry logic:
- try await fixtures.caroV2V3Client.conversations.sync() + let maxAttempts = 3 + var attempt = 0 + while attempt < maxAttempts { + try await fixtures.caroV2V3Client.conversations.sync() + let syncedCount = try await fixtures.caroV2V3Client.conversations.list(includeGroups: true).count + if syncedCount == 1 { break } + try await Task.sleep(nanoseconds: 1_000_000_000) // 1 second + attempt += 1 + }Likely invalid or redundant comment.
Sources/XMTPiOS/Conversation.swift (3)
27-27: LGTM: Clean enum extension for DM supportThe addition of the
dmcase to bothConversationandVersionenums is well-structured and maintains backward compatibility while introducing V3 DM support.Also applies to: 30-31
165-176: LGTM: Comprehensive DM message sending implementationThe implementation of message sending for DMs is thorough and follows consistent patterns, with proper delegation to the underlying
Dmtype.Also applies to: 191-203
280-291: Verify DM consent proof handlingThe
consentProofimplementation returnsnilfor DMs without clear documentation of why. If this is intentional, consider adding a comment explaining why DMs don't require consent proofs.✅ Verification successful
Let me try a different search strategy to understand the consent proof handling better.
Let me search for more specific information about DM implementation and consent handling.
Based on the test files and implementation, I can now generate the final response.
DM consent is handled through a separate consent state mechanism
DMs use a different consent management system through the contacts API and group state, as evidenced by the test files. The
consentProofproperty returnsnilfor DMs because they useconsentState()andupdateConsentState()methods instead of the consent proof payload mechanism used by v2 conversations.
Sources/XMTPiOS/Conversation.swift: DMs implement consent throughdm.consentState()anddm.updateConsentState()Tests/XMTPTests/DmTests.swift: Shows DMs using a group-based consent system with states (allowed/denied) managed through the contacts API🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation or related code about DM consent handling rg -l "consent.*proof.*dm" --type swiftLength of output: 41
Script:
#!/bin/bash # Search for any mentions of consent proof in Swift files rg "consentProof" --type swift -A 3 # Also look for DM related implementations rg "case.*dm.*:" --type swift -A 3Length of output: 35861
Script:
#!/bin/bash # Look for DM implementation and consent related code ast-grep --pattern 'class DM { $$$ }' # Also search for any documentation about DM consent rg -i "dm.*consent" --type swift -A 3Length of output: 2077
Sources/XMTPiOS/Client.swift (2)
18-18: LGTM: Error case addition is well-structuredThe new error case follows the existing pattern and provides a clear description.
Also applies to: 28-29
713-731: Implementation looks good, but consider previous review feedbackThe implementation is solid with proper error handling and consistent patterns.
As mentioned in a previous review, consider extracting the regex pattern as a class-level constant for better maintainability.
Tests/XMTPTests/GroupTests.swift (2)
216-217: Inconsistent address property usage.
Line range hint
1-1024: LGTM! The implementation successfully integrates V3 DM testing.The changes effectively implement and test V3-only direct messages functionality while maintaining comprehensive test coverage for group operations. The integration is clean and follows the existing testing patterns.
Sources/XMTPiOS/Conversations.swift (2)
Line range hint
5-18: LGTM: Well-structured error handling for version-specific features.The new error cases and their descriptions are clear and properly implemented to handle version-specific functionality restrictions.
🧰 Tools
🪛 SwiftLint
[Warning] 21-21: Lines should not have trailing whitespace
(trailing_whitespace)
52-54: LGTM: Clear enumeration for conversation ordering.The
ConversationOrderenum provides a clean way to specify conversation ordering preferences.
This is the support for just the v3 only dms and no dual sending. This is how someone would get a full working v3 only client.
Puts checks in place to catch if a V2 client is trying to access V3 dms.
Tests to make sure V3 dms are not being exposed anywhere when a V3 only client is not present.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
These updates enhance user experience by providing improved messaging capabilities and ensuring reliability in conversation handling.