[V3] Consent follows ups #309
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve significant updates across multiple test and implementation classes, primarily focusing on renaming methods and restructuring logic to shift from group-based handling to conversation-based handling. This includes updates to consent management methods, where terms like "group" have been replaced with "conversation." Additionally, new methods have been introduced to enhance conversation handling, and deprecated methods have been noted to guide developers toward the updated API. 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: 1
🧹 Outside diff range and nitpick comments (8)
library/src/main/java/org/xmtp/android/library/Dm.kt (1)
209-210: Update parameter name to match new conversation terminology.While the method names have been updated to use "conversation" terminology, the parameter name
groupIdsis still using the old "group" terminology. This should be updated for consistency.Apply this diff to maintain consistent terminology:
- ConsentState.ALLOWED -> client.contacts.allowConversations(groupIds = listOf(id)) - ConsentState.DENIED -> client.contacts.denyConversations(groupIds = listOf(id)) + ConsentState.ALLOWED -> client.contacts.allowConversations(conversationIds = listOf(id)) + ConsentState.DENIED -> client.contacts.denyConversations(conversationIds = listOf(id))library/src/androidTest/java/org/xmtp/android/library/DmTest.kt (1)
347-347: LGTM with enhancement suggestion.The consent state transition test is well structured and correctly updated to use conversation-based terminology. Consider enhancing the test to verify message delivery behavior in different consent states.
Consider adding assertions to verify:
- Message delivery succeeds in ALLOWED state
- Message delivery fails in DENIED state
- Message delivery resumes after transitioning back to ALLOWED
Example addition:
// In ALLOWED state dm.send("Should succeed") assert(dm.messages().last().body == "Should succeed") // In DENIED state assertThrows(XMTPException::class.java) { dm.send("Should fail") } // Back in ALLOWED state dm.send("Should succeed again") assert(dm.messages().last().body == "Should succeed again")Also applies to: 350-351, 355-355
library/src/main/java/org/xmtp/android/library/Contacts.kt (1)
Line range hint
300-309: Update parameter name for consistencyThe parameter name 'groupId' should be updated to 'conversationId' for consistency with the new terminology.
-suspend fun conversationState(groupId: String): ConsentState { +suspend fun conversationState(conversationId: String): ConsentState { client.v3Client?.let { return ConsentState.fromFfiConsentState( it.getConsentState( FfiConsentEntityType.CONVERSATION_ID, - groupId + conversationId ) ) } - val entry = entries[ConsentListEntry.conversationId(groupId).key] + val entry = entries[ConsentListEntry.conversationId(conversationId).key] return entry?.consentType ?: ConsentState.UNKNOWN }library/src/main/java/org/xmtp/android/library/Group.kt (1)
183-187: Consider adding deprecation notices for V2 clients.Since this change affects V2 clients (
if (client.hasV2Client)), consider adding deprecation notices to guide users toward the new conversation-based terminology.Example implementation:
suspend fun updateConsentState(state: ConsentState) { if (client.hasV2Client) { + @Deprecated("Group consent methods are deprecated in favor of conversation consent methods") when (state) { ConsentState.ALLOWED -> client.contacts.allowConversations(groupIds = listOf(id)) ConsentState.DENIED -> client.contacts.denyConversations(groupIds = listOf(id)) ConsentState.UNKNOWN -> Unit } }library/src/androidTest/java/org/xmtp/android/library/V3ClientTest.kt (2)
244-252: LGTM! Method calls updated correctly to use conversation terminology.The changes correctly implement the new conversation-based consent methods and maintain the same test logic.
Consider renaming the test method from
testGroupConsenttotestConversationConsentto maintain consistency with the new terminology.- @Test - fun testGroupConsent() { + @Test + fun testConversationConsent() {
Line range hint
1-437: Consider standardizing terminology across all test methods.While the consent-related method calls have been updated to use "conversation" terminology, there are several test methods that still use "group" in their names. For consistency with the new terminology, consider reviewing and updating these method names:
testsCanCreateGrouptestsCanSendMessagesToGrouptestCanStreamGroupsAndConversationsFromV3UserstestCanStreamGroupsAndConversationsFromV2andV3UsersThis would help maintain consistency with the new conversation-based terminology throughout the codebase.
library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt (1)
816-825: Consider adding edge case tests for consent state transitions.While the basic consent state transitions are tested, consider adding tests for:
- Concurrent consent state changes
- State transitions during message delivery
- Error cases when consent operations fail
Example test case:
@Test fun testConcurrentConsentStateChanges() { runBlocking { val group = boClient.conversations.newGroup(listOf(alix.walletAddress)) // Simulate concurrent state changes launch { group.updateConsentState(ConsentState.DENIED) } launch { group.updateConsentState(ConsentState.ALLOWED) } delay(100) // Allow both operations to complete // Verify final state is consistent assertEquals(group.consentState(), ConsentState.ALLOWED) } }library/src/main/java/org/xmtp/android/library/Conversations.kt (1)
164-164: Terminology update aligns with PR objectives.The change from
allowGroupstoallowConversationscorrectly implements the terminology standardization. Consider reviewing the method namenewGroupInternalin future updates for complete terminology consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- library/src/androidTest/java/org/xmtp/android/library/DmTest.kt (2 hunks)
- library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt (4 hunks)
- library/src/androidTest/java/org/xmtp/android/library/SmartContractWalletTest.kt (1 hunks)
- library/src/androidTest/java/org/xmtp/android/library/V3ClientTest.kt (1 hunks)
- library/src/main/java/org/xmtp/android/library/Client.kt (0 hunks)
- library/src/main/java/org/xmtp/android/library/Contacts.kt (9 hunks)
- library/src/main/java/org/xmtp/android/library/Conversation.kt (0 hunks)
- library/src/main/java/org/xmtp/android/library/Conversations.kt (3 hunks)
- library/src/main/java/org/xmtp/android/library/Dm.kt (1 hunks)
- library/src/main/java/org/xmtp/android/library/Group.kt (1 hunks)
💤 Files with no reviewable changes (2)
- library/src/main/java/org/xmtp/android/library/Client.kt
- library/src/main/java/org/xmtp/android/library/Conversation.kt
🔇 Additional comments (17)
library/src/main/java/org/xmtp/android/library/Dm.kt (1)
209-210: Verify the V2 client compatibility changes.The consent state updates are wrapped in a V2 client compatibility check. Let's verify that all V2 client references have been updated consistently.
✅ Verification successful
Let me gather more specific information about the consent-related method implementations to verify the V2 client compatibility.
Based on the results, I can now generate the final response as we have sufficient information about the V2 client compatibility implementation.
V2 client compatibility changes are correctly implemented
The consent state updates are properly handled with V2 client compatibility:
- The
updateConsentStatemethod in bothDm.ktandGroup.ktcorrectly checkshasV2Clientbefore applying V2-specific consent operations- The consent methods in
Contacts.kthave been consistently updated to use conversation terminology- The implementation properly handles all consent states (ALLOWED, DENIED, UNKNOWN)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining group-related terms in V2 client compatibility code # and verify that all consent-related method calls are consistently updated. # Test 1: Look for any remaining group-related terms in V2 client compatibility blocks echo "Checking for remaining group terminology in V2 client blocks..." rg -l "hasV2Client.*group" # Test 2: Verify that all consent-related method calls use the new conversation terminology echo "Verifying consent method calls..." rg -l "allow(Groups|Conversations)|deny(Groups|Conversations)"Length of output: 821
Script:
#!/bin/bash # Check the actual implementation of allow/deny methods and their V2 client checks echo "Checking allow/deny implementations in Contacts.kt..." rg -A 5 "fun allow.*Conversations|fun deny.*Conversations" library/src/main/java/org/xmtp/android/library/Contacts.kt echo "\nChecking V2 client checks in Dm.kt..." rg -B 5 -A 5 "hasV2Client|ConsentState" library/src/main/java/org/xmtp/android/library/Dm.kt echo "\nChecking Group.kt for any V2 client related consent handling..." rg -B 5 -A 5 "hasV2Client|ConsentState" library/src/main/java/org/xmtp/android/library/Group.ktLength of output: 4428
library/src/androidTest/java/org/xmtp/android/library/SmartContractWalletTest.kt (2)
162-162: LGTM! Consent-related method calls have been correctly updated.The changes properly reflect the transition from group-based to conversation-based terminology in the consent management methods:
isConversationAllowedreplacesisGroupAlloweddenyConversationsreplacesdenyGroupsisConversationDeniedreplacesisGroupDeniedAlso applies to: 165-166, 170-170
Line range hint
1-277: Verify consistent terminology updates across related files.Several methods in this test file still use the "group" terminology (e.g.,
testsCanCreateGroup,findGroup,syncGroups,listGroups). Let's verify if these correspond to deprecated APIs and if there are new conversation-based alternatives.library/src/androidTest/java/org/xmtp/android/library/DmTest.kt (1)
152-153: LGTM: Consent terminology updates are correctly applied.The changes properly reflect the shift from group-based to conversation-based consent terminology while maintaining the original test logic.
library/src/main/java/org/xmtp/android/library/Contacts.kt (7)
43-43: LGTM: Consistent terminology change from GROUP to CONVERSATIONThe renaming maintains correct FFI type mappings while standardizing terminology.
Also applies to: 50-50, 58-58
157-157: LGTM: Consistent consent operation renamingThe load method correctly updates the consent operations for conversations.
Also applies to: 160-160
192-197: LGTM: Correct mapping updates in publish methodThe publish method properly maps the new CONVERSATION_ID type to the appropriate group actions.
259-269: LGTM: Clear method renaming for conversation consentMethods are consistently renamed while maintaining their original functionality.
354-364: LGTM: Well-structured conversation consent methodsThe new conversation consent methods are properly implemented with:
- Consistent suspend function usage
- Clear naming aligned with address consent pattern
- Proper delegation to ConsentList
Also applies to: 390-395
Line range hint
1-434: Verify complete migration of group terminologyThe changes look good overall, with consistent implementation of the group to conversation rename. Let's verify the complete migration across the codebase.
#!/bin/bash # Description: Comprehensive check for remaining group terminology in Kotlin files # excluding test files and comments # Check for remaining group terminology in function names and variables echo "Checking for remaining group terminology in Kotlin files..." rg "group(?i)" --type kotlin \ --glob '!**/*test*' \ --glob '!**/*Test*' \ -g '!*.md' \ -g '!*.txt' \ --no-heading \ --line-number # Check specifically for GroupId terminology echo "Checking for GroupId terminology..." rg "GroupId" --type kotlin \ --glob '!**/*test*' \ --glob '!**/*Test*' \ -g '!*.md' \ -g '!*.txt' \ --no-heading \ --line-number
78-82: Verify usage of new conversationId methodThe method renaming looks good, maintaining consistent parameters and implementation.
✅ Verification successful
Let me analyze the results and generate a final response.
Verify usage of new conversationId method
The search results show that the
groupIdterminology has been completely migrated toconversationIdin theContacts.ktfile. The remaining occurrences of "group" in the codebase are related to different concepts:
- Group-related functionality (e.g.,
Group.kt, group messages, group permissions)- FFI bindings and native implementations
- UI/UX elements for group conversations
The method renaming from
groupIdtoconversationIdinConsentListEntryis consistent with the codebase, and there are no remaining references to the old method name that need to be updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to groupId to ensure complete migration rg "groupId" --type kotlinLength of output: 27014
library/src/main/java/org/xmtp/android/library/Group.kt (1)
184-185: LGTM! Terminology change aligns with project objectives.The update from
allowGroups/denyGroupstoallowConversations/denyConversationscorrectly implements the terminology standardization from the PR objectives.Let's verify consistent terminology usage across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the terminology usage across the codebase.
Terminology migration is complete and consistent
The codebase search confirms that:
- The migration from "group consent" to "conversation consent" is consistently implemented
- All consent-related operations in
Group.ktandDm.ktnow use the newallowConversations/denyConversationsmethods- The underlying data structures and APIs (like
ConsentListEntry,conversationState, etc.) are aligned with the new terminology- Test files appropriately use the new methods while retaining "group" in test method names (which is acceptable as they're just test identifiers)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of "group consent" terminology # that might have been missed during the migration. echo "Checking for remaining 'group consent' terminology..." rg -i "group.*consent|consent.*group" --type kotlin echo "Checking for new 'conversation consent' terminology..." rg -i "conversation.*consent|consent.*conversation" --type kotlinLength of output: 13157
library/src/androidTest/java/org/xmtp/android/library/V3ClientTest.kt (1)
244-252: Verify complete migration of consent-related method names.Let's ensure all group consent related methods have been properly renamed throughout the codebase.
✅ Verification successful
Let me gather more context about the test methods that still contain "Group" in their names to ensure they're using the new conversation consent methods internally.
Based on the gathered evidence, I can now generate the final response:
Migration to conversation consent methods is complete and consistent
The verification shows:
- No instances of old method names (
isGroupAllowed,isGroupDenied,denyGroups,allowGroups) were found in the codebase- New conversation consent methods are consistently used across the main implementation files and tests
- While test method names still contain "Group" (e.g.,
testGroupConsent), their implementations correctly use the new conversation consent methods🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of old group consent method names # and verify consistent usage of new conversation consent method names echo "Checking for any remaining instances of old group consent methods..." rg -l "isGroupAllowed|isGroupDenied|denyGroups|allowGroups" echo "Verifying usage of new conversation consent methods..." rg -l "isConversationAllowed|isConversationDenied|denyConversations|allowConversations" echo "Checking for potential missed renames in test method names..." rg -l "test.*Group.*Consent"Length of output: 1313
Script:
#!/bin/bash # Check the content of test methods containing "Group" and "Consent" to verify they use new method names echo "Checking content of GroupTest.kt..." rg "test.*Group.*Consent" library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt -A 10 echo -e "\nChecking content of V3ClientTest.kt..." rg "test.*Group.*Consent" library/src/androidTest/java/org/xmtp/android/library/V3ClientTest.kt -A 10 echo -e "\nChecking content of SmartContractWalletTest.kt..." rg "test.*Group.*Consent" library/src/androidTest/java/org/xmtp/android/library/SmartContractWalletTest.kt -A 10Length of output: 1977
library/src/androidTest/java/org/xmtp/android/library/GroupTest.kt (2)
134-138: LGTM: Consent state checks updated consistently.The renaming from group to conversation consent is implemented correctly here, maintaining consistency with the PR objectives.
907-909: LGTM: Consent check for unpublished messages.The test correctly verifies that preparing a message automatically allows the conversation, which is important for the message delivery flow.
library/src/main/java/org/xmtp/android/library/Conversations.kt (2)
80-84: Clean and clear conditional logic!The refactored if-else structure improves readability while maintaining correct type handling for both DM and Group conversations.
209-209: API simplification improves consistency.The change to use
contacts.allowinstead ofallowGroupsstreamlines the permission API while maintaining functionality. This aligns well with the unified conversation handling approach.
library/src/androidTest/java/org/xmtp/android/library/SmartContractWalletTest.kt
Outdated
Show resolved
Hide resolved
…3-consent-upgrades
library/src/androidTest/java/org/xmtp/android/library/DmTest.kt
Outdated
Show resolved
Hide resolved
library/src/androidTest/java/org/xmtp/android/library/DmTest.kt
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores