-
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
feat: conversation API with credentials [WPB 19576] #1689
Conversation
9b8998d to
20b84fd
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!
| } | ||
|
|
||
| /// 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?
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.
The CustomConfiguration that was passed in had two fields: wire policy (where our server only supports plain text), and a key rotation span (where a comment said "this isn't currently implemented") so it is obvious that providing any non-default values of this configuration doesn't work for us in practice.
It isn't used anywhere
b1ec956 to
4a1853c
Compare
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()`.
4a1853c to
b5a6a1f
Compare
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.