-
Notifications
You must be signed in to change notification settings - Fork 20
feat: initiate reset broken mls conversation - WPB-18800 #3348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: initiate reset broken mls conversation - WPB-18800 #3348
Conversation
Test Results 7 files 989 suites 9m 18s ⏱️ For more details on these failures, see this check. Results for commit ea1f19f. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 7493 Passed, 27 Skipped, 4m 8.74s Total Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, left a couple comments before approving
WireDomain/Sources/WireDomain/Helpers/ResetMLSConversationHandler.swift
Outdated
Show resolved
Hide resolved
"A referenced leaf node index points to a blank or non-existing node: \(message)" | ||
|
||
case let .mlsInvalidLeafNodeSignature(message): | ||
"TBD" // TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
initiateResetMLSConversationUseCaseFactory: { [weak self] context in | ||
guard let self else { | ||
fatal("userSession not reachable") | ||
} | ||
return makeInitiateResetMLSConversationUseCase(context: context) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: i know we talked about this? but just to double check, is this the only way?
Maybe could be good to leave a comment here why this is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean pass it from ZMUserSession or use factory block instead of passing already created use case instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok actually what would be nice is to leave a comment why we do it:
"Passing useCase from WireDomain to WireRequestStrategy's MessageSender"
wire-ios-request-strategy/Sources/Message Sending/MessageSenderTests.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice work
WireDomain/Sources/WireDomain/Helpers/InitiateResetMLSConversationUseCase.swift
Outdated
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Helpers/InitiateResetMLSConversationUseCase.swift
Outdated
Show resolved
Hide resolved
1a8b9b0
to
51bdeaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found just one issue with establish group, then we're good
let users = await conversationLocalStore.localParticipantsAsMLSUsers(in: conversation) | ||
_ = try await mlsService.establishGroup(for: groupID, with: users, removalKeys: nil) | ||
|
||
WireLogger.mls.info("Initiate reset broken MLS conversation use case finished") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
add conversationID as attribute to all logs so we know which conversation we're talking about
i.e.:
.conversationId: message.conversation?.qualifiedID?.safeForLoggingDescription ?? "<nil>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
|
||
// re-create group and re-add all participants | ||
let users = await conversationLocalStore.localParticipantsAsMLSUsers(in: conversation) | ||
_ = try await mlsService.establishGroup(for: groupID, with: users, removalKeys: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: localParticipants returns all participants including selfUser and establishGroup add the users and internally adds also self. Use localParticipantsExcludingSelf if we use establishGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
initiateResetMLSConversationUseCaseFactory: { [weak self] context in | ||
guard let self else { | ||
fatal("userSession not reachable") | ||
} | ||
return makeInitiateResetMLSConversationUseCase(context: context) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok actually what would be nice is to leave a comment why we do it:
"Passing useCase from WireDomain to WireRequestStrategy's MessageSender"
Issue
Please describe the issue.
Optional: add details about technical approach, solutions etc.
Optional: reference dependencies to other pull requests etc.
Testing
Describe how to test.
Optional: attachments like images, videos, etc.
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: