-
Notifications
You must be signed in to change notification settings - Fork 10
feat: conversation API with credentials [WPB 19576] #1689
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: main
Are you sure you want to change the base?
feat: conversation API with credentials [WPB 19576] #1689
Conversation
f996884 to
9b8998d
Compare
It isn't used anywhere
An error message like that would have spared me hours of debugging. I hope it will be helpful in the future.
… don't use config parameter We're always using the default. This simplifies their API.
We're not changing the DB schema of this entity though, because we might want to be able to have multiple different configurations for a single session in the future; keeping the column avoids migrations based on assumptions.
Our DS doesn't support it, our API doesn't support it anymore, so we can stop testing it.
I don't understand how the test was passing before, because only in this way it makes sense to me. Happy to have someone point out to me how it was testing reverse-order decryption.
We already had a method on the conversation gaurd that did exactly what we need (with a little refactoring): `update_key_material_inner()`. So that becomes `set_credential()`.
9b8998d to
20b84fd
Compare
20b84fd to
dc81264
Compare
f1517ce to
1cfdbfa
Compare
1cfdbfa to
b1ec956
Compare
coriolinus
left a comment
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.
Had a few nits, but the only important question is how you determined that JBEC and processing welcome messages does not require custom configuration. If there's a good answer to that (which I'm assuming there is) then this all makes sense. Nicely done!
|
|
||
| /// Like [Self::invite], but the key packages of the invited members will be of the provided credential type. | ||
| pub async fn invite_with_credential_type( | ||
| pub async fn invite_with_credential( |
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.
Doc-comment wasn't updated.
| .into() | ||
| })) | ||
| .await; | ||
| let new_memembers_with_credentials = sessions_with_credentials.into_iter().collect::<Vec<_>>(); |
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.
Why collect into a vector here? If it's about reusing the iterator, you could write it like this:
pub async fn invite_with_credential<S>(self, sessions_with_credentials: S) -> OperationGuard<'a, Commit>
where
S: IntoIterator<Item = (&'a SessionContext, &'a CredentialRef)>,
<S as IntoIterator>::IntoIter: Clone| } | ||
|
|
||
| /// See [core_crypto::transaction_context::TransactionContext::process_raw_welcome_message] | ||
| pub async fn process_welcome_message( |
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.
How did you determine that we always use the default custom configuration in these two cases (process welcome message and join by external commit)? Couldn't the clients do something non-default with this public interface without telling us?
What's new in this PR
See above, corresponding jira item, and commit sequence
PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.